Repository navigation
Preserve RBS block contracts during runtime typing - #326
calmacleod wants to merge 1 commit into
Conversation
Co-Authored-By: Codex <noreply@openai.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughParsed RBS block contracts are added to a private typing registry before method body typing. Block parameter inference binds one parameter to the block type. Tests cover instance and class methods, block arity, registry preservation, and emitted Ruby execution. ChangesRBS Block Signature Typing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Parse as parse_methods_with_rbs_in_ctx
participant Registry as Private typing registry
participant Typer as BodyTyper
Parse->>Registry: Add parsed method signatures with block contracts
Parse->>Typer: Construct with augmented registry
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue remains identified in this change; it is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Restored signatures remain confined to one analysis call, and the reviewed changes do not expand execution permissions. No introduced security concern was identified, but broader downstream behavior was not exercised during this review. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Integrate rubys#326 pinned at 12f99a8; authored change 12f99a8. Preserve the fork's existing behavior and tests. Validation on the integrated candidate: full default/all-targets each 3459 passed, 0 failed, 110 existing or upstream toolchain ignores; focused regressions/emitted execution and release source-index guards passed. Co-Authored-By: Codex <noreply@openai.com>
bunnykong
left a comment
There was a problem hiding this comment.
Thanks, Callum. The diagnosis is precise: the runtime sweep's registry holds return types only, so block contracts never reached the binder, and a one-parameter block bound the whole Fn type. Both halves are the right shape: spreading the inner params at every arity, and typing against a private copy of the registry.
Verdict: suggest changes. Nothing blocks, but one narrow fix is worth folding in, and a rebase is needed.
Checked on a trial merge into main 68bb3eeb: the four runtime_block_signatures tests still fail without the src/ change (#451 didn't touch this path) and pass with it, as do --lib, analyze, real_blog, lowered_ruby_emit, the runtime_src_* suites and the RBS probe (still 0).
1. Non-blocking: a partial class narrows a dynamic send to Nil. With an empty caller registry (parse_methods_with_rbs, used for Mode::Module stems), send(name) here is Untyped on main but Nil with this PR, while CRuby's Dyn.new.pick(:label) returns "x":
class Dyn
def rows = (yield 1; nil) # () { (Integer) -> void } -> nil
def label = "x" # () -> String
def pick(name) = send(name) # (Symbol name) -> untyped
endor_default() creates an entry with only the block-bearing methods, and receiver_method_return_union treats an entry as the full method list. It's latent (no Mode::Module stem declares a block, and real-blog and store emit identically with and without the PR), but it's a wrong type, not a missing one. A narrow fix fills in the class's other signatures without overwriting:
for m in &methods {
let (Some(enclosing), Some(sig)) = (&m.enclosing_class, &m.signature) else { continue };
if !declares_block.contains(enclosing) { continue } // classes with a `block: Some(_)` sig
let info = typing_classes.entry(ClassId(enclosing.clone())).or_default();
let table = match m.receiver {
MethodReceiver::Instance => &mut info.instance_methods,
MethodReceiver::Class => &mut info.class_methods,
};
if matches!(sig, Ty::Fn { block: Some(_), .. }) {
table.insert(m.name.clone(), sig.clone());
} else {
table.entry(m.name.clone()).or_insert_with(|| sig.clone());
}
}With it, send(name) is Untyped again, me.value in an { (instance) -> void } block resolves to Integer, and everything above still passes, with identical emits.
2. Non-blocking: the emit-and-run test passes without the fix. CRuby runs the block whatever type items gets, so it passes on main's src/ too. An analyzer assertion would pin the app path, as tests/analyze.rs does with app_from_files: on main, items gets the whole Fn type and item stays unresolved, with no diagnostic.
Rebase needed. Only tests/emit_and_run.rs conflicts, where main added mod lines and tests at the same two spots; keeping both sides resolves it, and src/ merges cleanly. Since 13 open PRs touch that file, #329 among them, moving the test into its own tests/emit_and_run/<feature>.rs behind one #[path] mod line, as main's string_bytes.rs does, should keep the next rebase trivial. Until #487 lands, main's emit_and_run target doesn't compile; the runs above apply its fix.
Integrate rubys#326 pinned at 12f99a8; authored change 12f99a8. Preserve the fork's existing behavior and tests. Validation on the integrated candidate: full default/all-targets each 3459 passed, 0 failed, 110 existing or upstream toolchain ignores; focused regressions/emitted execution and release source-index guards passed. Co-Authored-By: Codex <noreply@openai.com>
Problem
Runtime method registries retain return types, but can lose the RBS block contract when a body calls another method from the same file. A block declared as
{ (Array[Integer]) -> void }then leaves its yielded array and elements unresolved. Single-parameter blocks also bind the inner function type rather than its parameter type.Change
Restore locally authored block signatures in a private typing registry, preserving other registry entries and leaving the caller's registry untouched. Bind the inner RBS function's parameters at every arity, including zero and one.
Four regressions cover instance and class methods, zero/single/multiple yields, exact element types, and existing cross-file entries. A canonical emit-and-run regression checks the resulting program and executes the emitted Ruby.
Verification
Based on upstream
e0d8610e:analyze,emit_and_run, runtime typing and round-trip checks, basic-auth blocks, RBS ingestion,real_blog, and lowered Ruby emission.cargo build --release --testspassed; the block-expressionroundhouse-ast --round-tripis stable.Rust 1.98.1 and Ruby 4.0.5; serial builds under a 3 GiB process-tree memory cap.
Boundaries
Native Spinel and other ignored toolchain lanes were not run locally. Open and closed issues/PRs were checked for duplicate fixes, and open PRs were rechecked before publication. Please apply
ci:fullfor the shared inference change.Summary by CodeRabbit