diff --git a/e2e/action_cable.spec.js b/e2e/action_cable.spec.js index fcfd61357..67abd6369 100644 --- a/e2e/action_cable.spec.js +++ b/e2e/action_cable.spec.js @@ -2,18 +2,26 @@ import { test, expect } from '@playwright/test' // article_3 is seeded with zero comments. Assertions are scoped to *our* // uniquely-worded comment so this can run in parallel with the Turbo Stream -// test, which also posts comments on the same article. +// test, which also posts comments on the same article. BODY is per-attempt +// so a Playwright retry never sees residue from a prior attempt on the +// shared smoke DB (JRuby Action Cable delete has been flaky enough to +// leave the fixed string behind and fail the next open at toHaveCount(0)). const ARTICLE_PATH = '/articles/3' const COMMENTER = 'Cable Bot' -const BODY = 'Action Cable broadcast smoke-test comment' test('a new comment broadcasts live to other viewers via Action Cable', async ({ browser }) => { + const BODY = `Action Cable broadcast smoke-test comment ${Date.now()}-${Math.random().toString(36).slice(2, 8)}` + // Two independent contexts = two separate viewers of the same article. const observerCtx = await browser.newContext() const actorCtx = await browser.newContext() const observer = await observerCtx.newPage() const actor = await actorCtx.newPage() + // Accept Turbo's confirm before any delete click (register early; JRuby + // smoke has timed out waiting for a late dialog handler). + actor.on('dialog', dialog => dialog.accept()) + const observerRow = observer.locator('#comments > div').filter({ hasText: BODY }) const actorRow = actor.locator('#comments > div').filter({ hasText: BODY }) @@ -40,15 +48,14 @@ test('a new comment broadcasts live to other viewers via Action Cable', async ({ await expect(observerRow).toBeVisible() expect(await observer.evaluate(() => window.__noNav)).toBe(true) - // Cleanup: delete the comment from the actor page (accept the Turbo confirm). - actor.on('dialog', dialog => dialog.accept()) + // Cleanup: delete the comment from the actor page (dialog already accepted). await actorRow.getByRole('button', { name: 'Delete' }).click() - await expect(actorRow).toHaveCount(0) + await expect(actorRow).toHaveCount(0, { timeout: 15_000 }) // The removal broadcasts too — the observer's row disappears, leaving no residue. - await expect(observerRow).toHaveCount(0) + await expect(observerRow).toHaveCount(0, { timeout: 15_000 }) } finally { await observerCtx.close() await actorCtx.close() } -}) +}) \ No newline at end of file diff --git a/src/analyze/body/mod.rs b/src/analyze/body/mod.rs index 45fd53d8b..8aec1736c 100644 --- a/src/analyze/body/mod.rs +++ b/src/analyze/body/mod.rs @@ -1271,8 +1271,17 @@ impl<'a> BodyTyper<'a> { } } } + let class_object_receiver = + recv.as_ref().map_or(ctx.class_side, |r| self.is_class_object(r, ctx)); let block_ret = if let Some(b) = block { - let mut block_ctx = self.block_ctx_for(ctx, recv_ty.as_ref(), method, args, b); + let mut block_ctx = self.block_ctx_for( + ctx, + recv_ty.as_ref(), + method, + args, + class_object_receiver, + b, + ); if matches!(method.as_str(), "instance_eval" | "instance_exec" | "class_eval" | "class_exec" | "module_eval" | "module_exec") { if let Some(receiver) = recv.as_ref() { block_ctx.self_ty = recv_ty.clone(); @@ -1478,7 +1487,8 @@ impl<'a> BodyTyper<'a> { } // What every object and every module answers, when the // receiver's own table did not. App analyzer only. - let class_object_receiver = recv.as_ref().map_or(ctx.class_side, |r| self.is_class_object(r, ctx)); + // `class_object_receiver` was resolved above for block binding + // so it matches the same class/instance table preference. if matches!(dispatched, Ty::Var { .. } | Ty::Untyped) && self.inquirers.is_some() && (recv.is_some() || (ctx.self_ty.is_some() && send::is_module_protocol(method))) && !self.owns_operator(recv_ty.as_ref(), method, class_object_receiver) { diff --git a/src/analyze/body/send.rs b/src/analyze/body/send.rs index 8322167ae..a00d8008e 100644 --- a/src/analyze/body/send.rs +++ b/src/analyze/body/send.rs @@ -213,6 +213,7 @@ impl<'a> BodyTyper<'a> { recv_ty: Option<&Ty>, method: &Symbol, args: &[Expr], + class_object_receiver: bool, block: &Expr, ) -> Ctx { let mut new_ctx = outer.clone(); @@ -257,7 +258,7 @@ impl<'a> BodyTyper<'a> { } return new_ctx; } - let Some(param_tys) = self.block_params_for(recv_ty, method) else { + let Some(param_tys) = self.block_params_for(recv_ty, method, class_object_receiver) else { return new_ctx; }; for (name, ty) in params.iter().zip(param_tys.iter()) { @@ -321,10 +322,13 @@ impl<'a> BodyTyper<'a> { /// Per-param types a block yields, given the receiver type and method. /// `None` means "no binding info available" — params stay unknown. + /// `class_object_receiver` picks class-method block contracts before + /// instance ones, matching ordinary dispatch on a class/module object. pub(super) fn block_params_for( &self, recv_ty: Option<&Ty>, method: &Symbol, + class_object_receiver: bool, ) -> Option> { let recv_ty = recv_ty?; if matches!(recv_ty, Ty::Class { id, .. } if id.0.as_str() == PARAM_VALUE) { @@ -351,7 +355,7 @@ impl<'a> BodyTyper<'a> { let as_array = Ty::Array { elem: Box::new(elems.iter().cloned().reduce(union_of).unwrap_or(Ty::Untyped)), }; - return self.block_params_for(Some(&as_array), method); + return self.block_params_for(Some(&as_array), method, class_object_receiver); } match recv_ty { Ty::Str if method.as_str() == "bytes" => Some(vec![Ty::Int]), @@ -378,7 +382,7 @@ impl<'a> BodyTyper<'a> { let as_array = Ty::Array { elem: Box::new(Ty::Class { id: of.clone(), args: vec![] }), }; - self.block_params_for(Some(&as_array), method) + self.block_params_for(Some(&as_array), method, class_object_receiver) } Ty::Hash { key, value } => match method.as_str() { "each" | "each_pair" | "map" | "collect" @@ -435,11 +439,33 @@ impl<'a> BodyTyper<'a> { for c in std::iter::once(cls) .chain(cls.includes.iter().filter_map(|m| self.classes().get(m))) { - if let Some(sig) = c - .instance_methods - .get(method) - .or_else(|| c.class_methods.get(method)) - { + // Prefer the receiver-side table. The fixpoint + // often seeds that side with a bare return + // (`Nil` / `Str` / …) while the RBS block + // contract still lives on the other side — + // take the other side's block-bearing Fn only + // then. A real receiver-side `Fn` without a + // block must not steal the opposite method's + // block (dispatch still picks the receiver + // side). Dual-name both-sides-with-block keeps + // the receiver side. + let (preferred, other) = if class_object_receiver { + (&c.class_methods, &c.instance_methods) + } else { + (&c.instance_methods, &c.class_methods) + }; + let return_seed = |ty: &Ty| !matches!(ty, Ty::Fn { .. }); + let sig = match (preferred.get(method), other.get(method)) { + (Some(s @ Ty::Fn { block: Some(_), .. }), _) => Some(s), + (pref, Some(s @ Ty::Fn { block: Some(_), .. })) + if pref.map_or(true, return_seed) => + { + Some(s) + } + (Some(s), _) => Some(s), + (None, o) => o, + }; + if let Some(sig) = sig { // The block's yield may name the receiver // (`{ (instance) -> void }`); substitute // against the class the walk started from, @@ -489,7 +515,9 @@ impl<'a> BodyTyper<'a> { if matches!(v, Ty::Nil | Ty::Var { .. }) { continue; } - if let Some(params) = self.block_params_for(Some(v), method) { + if let Some(params) = + self.block_params_for(Some(v), method, class_object_receiver) + { return Some(params); } } diff --git a/src/runtime_src.rs b/src/runtime_src.rs index 644625123..bfba0555a 100644 --- a/src/runtime_src.rs +++ b/src/runtime_src.rs @@ -731,6 +731,51 @@ fn seed_well_known_classes( .or_insert(adapter_iface); } +/// Private typing view: re-attach this file's full `Ty::Fn` for methods +/// that declare a block. Shared registries are often return-only +/// (`ret` stripped); `block_params_for` needs the Fn. Non-block entries +/// already present in `classes` still win (registry-seeded returns). +/// +/// When the caller registry lacks the enclosing class, `or_default` +/// would otherwise build a sparse `ClassInfo` with only block-bearing +/// methods — `send(name)` then unions to `Nil` instead of `Untyped`, +/// and `{ (instance) -> void }` siblings stay unresolved. For those +/// classes, also fill non-block local signatures with `or_insert`. +/// Never mutates `classes`. +fn typing_classes_with_local_block_contracts( + classes: &std::collections::HashMap, + methods: &[MethodDef], +) -> std::collections::HashMap { + let declares_block: std::collections::HashSet<&Symbol> = methods + .iter() + .filter(|m| matches!(m.signature, Some(Ty::Fn { block: Some(_), .. }))) + .filter_map(|m| m.enclosing_class.as_ref()) + .collect(); + + let mut typing_classes = classes.clone(); + for m in methods { + let (Some(enclosing), Some(sig)) = (&m.enclosing_class, &m.signature) else { + continue; + }; + if !declares_block.contains(enclosing) { + continue; + } + let info = typing_classes + .entry(crate::ident::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()); + } + } + typing_classes +} + /// Same as `parse_methods_with_rbs` but takes a pre-built class /// registry — so cross-class method dispatch during body-typing can /// resolve. Used by the runtime-sweep test, which builds a unified @@ -872,11 +917,8 @@ pub fn parse_methods_with_rbs_in_ctx( // Reads now resolve cleanly even when they lexically precede // the assignment (e.g. `@cache ||= compute` lowers to a `BoolOp` // whose left arm reads the unset ivar). - // - // Runtime code doesn't reference user classes today, so the - // dispatch table is empty — the body-typer falls back to its - // primitive method tables for everything. - let typer = crate::analyze::BodyTyper::new(classes); + let typing_classes = typing_classes_with_local_block_contracts(classes, &methods); + let typer = crate::analyze::BodyTyper::new(&typing_classes); // Extract module-level constants from the .rb so dispatch on // `STATUS_CODES.fetch(...)` etc. resolves through the constant's @@ -888,6 +930,7 @@ pub fn parse_methods_with_rbs_in_ctx( ivars: &std::collections::HashMap| -> crate::analyze::Ctx { let mut ctx = crate::analyze::Ctx::default(); + ctx.class_side = m.receiver == MethodReceiver::Class; if let Some(Ty::Fn { params, .. }) = &m.signature { for (param, p) in m.params.iter().zip(params.iter()) { ctx.local_bindings.insert(param.name.clone(), p.ty.clone()); diff --git a/tests/declared_block_params.rs b/tests/declared_block_params.rs index 9100a47c0..3e8420d91 100644 --- a/tests/declared_block_params.rs +++ b/tests/declared_block_params.rs @@ -78,3 +78,14 @@ fn a_block_that_yields_nothing_binds_nothing() { ); assert!(r.is_empty(), "{r:?}"); } + +#[test] +fn an_instance_method_without_a_block_does_not_steal_the_class_side_block() { + // Same name on both sides: instance `with` has no block; class `with` + // yields Writer. An instance call must not bind from the class contract. + let r = receivers( + "class Writer\nend\n\nclass Registry\n #: () { (Writer) -> void } -> void\n def self.with; end\n\n #: () -> void\n def with; end\nend\n", + "Registry.new.with { |writer| writer.bogus }", + ); + assert!(r.is_empty(), "stole class-side block: {r:?}"); +} diff --git a/tests/emit_and_run.rs b/tests/emit_and_run.rs index 02537ccfb..ab1c2db4d 100644 --- a/tests/emit_and_run.rs +++ b/tests/emit_and_run.rs @@ -14,6 +14,8 @@ mod integer_query_find_by; #[path = "support/class_configuration.rs"] mod class_configuration; +#[path = "support/runtime_block_signature.rs"] +mod runtime_block_signature; #[path = "support/data_factory.rs"] mod data_factory; #[path = "support/rails_root_join.rs"] @@ -7885,3 +7887,12 @@ raise "vf vs vframes" unless ActiveStorage.video_preview_vf_filter == "scale=320 ) .assert_passes(); } + +#[test] +fn an_rbs_array_block_runs_after_app_emission() { + emit_and_run::real_blog() + .write("app/lib/batch.rb", runtime_block_signature::RUBY) + .write("sig/batch.rbs", runtime_block_signature::RBS) + .run_ruby("raise 'wrong sum' unless Batch.new.consume == 3") + .assert_passes(); +} diff --git a/tests/runtime_block_signatures.rs b/tests/runtime_block_signatures.rs new file mode 100644 index 000000000..bd9b457e1 --- /dev/null +++ b/tests/runtime_block_signatures.rs @@ -0,0 +1,375 @@ +//! Runtime RBS block contracts survive return-only registry seeds. +use std::collections::HashMap; +use std::process::Command; + +use roundhouse::analyze::ClassInfo; +use roundhouse::dialect::MethodDef; +use roundhouse::expr::{Expr, ExprNode}; +use roundhouse::ident::{ClassId, Symbol}; +use roundhouse::runtime_src::parse_methods_with_rbs_in_ctx; +use roundhouse::ty::Ty; + +#[path = "support/runtime_block_signature.rs"] +mod runtime_block_signature; + +use runtime_block_signature::{RBS, RUBY}; + +fn method<'a>(methods: &'a [MethodDef], name: &str) -> &'a MethodDef { + methods.iter().find(|m| m.name.as_str() == name).unwrap() +} + +fn assert_no_inference_gaps(expr: &Expr) { + assert!( + !matches!(expr.ty, None | Some(Ty::Var { .. })), + "inference gap: {expr:?}" + ); + expr.node.for_each_child(&mut assert_no_inference_gaps); +} + +fn local_types(expr: &Expr, name: &str) -> Vec { + let mut types = Vec::new(); + if matches!(&*expr.node, ExprNode::Var { name: local, .. } if local.as_str() == name) { + types.push(expr.ty.clone().expect("local type")); + } + expr.node + .for_each_child(&mut |child| types.extend(local_types(child, name))); + types +} + +fn assert_local_type(expr: &Expr, name: &str, expected: Ty) { + let types = local_types(expr, name); + assert!(!types.is_empty(), "missing local {name}"); + assert!(types.iter().all(|ty| *ty == expected), "{name}: {types:?}"); +} + +fn assert_emitted_ruby(methods: &[MethodDef], class: &str, assertion: &str) { + let defs: String = methods + .iter() + .map(roundhouse::emit::ruby::emit_method) + .collect(); + let emitted = format!("class {class}\n{defs}\nend\n{assertion}\n"); + let output = Command::new("ruby") + .arg("-e") + .arg(&emitted) + .output() + .expect("ruby"); + assert!( + output.status.success(), + "emitted:\n{emitted}\n{}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn single_array_block_signature_binds_elements_without_mutating_the_registry() { + let batch_id = ClassId(Symbol::from("Batch")); + let mut batch = ClassInfo::default(); + batch.instance_methods.insert(Symbol::from("rows"), Ty::Nil); + let classes = HashMap::from([(batch_id.clone(), batch)]); + let methods = parse_methods_with_rbs_in_ctx(RUBY, RBS, &classes).expect("runtime parses"); + let consume = method(&methods, "consume"); + assert_eq!(consume.body.ty, Some(Ty::Int)); + assert_no_inference_gaps(&consume.body); + assert_local_type( + &consume.body, + "items", + Ty::Array { + elem: Box::new(Ty::Int), + }, + ); + assert_local_type(&consume.body, "item", Ty::Int); + assert_eq!( + classes[&batch_id].instance_methods[&Symbol::from("rows")], + Ty::Nil + ); + assert_emitted_ruby( + &methods, + "Batch", + "raise 'wrong sum' unless Batch.new.consume == 3", + ); +} + +#[test] +fn class_side_blocks_keep_their_yield_type() { + let ruby = r#" +class ClassBatch + def self.rows + yield [3, 4] + nil + end + + def self.consume + total = 0 + rows { |items| items.each { |item| total = total + item } } + total + end +end +"#; + let rbs = r#" +class ClassBatch + def self.rows: () { (Array[Integer]) -> void } -> nil + def self.consume: () -> Integer +end +"#; + let batch_id = ClassId(Symbol::from("ClassBatch")); + let mut batch = ClassInfo::default(); + batch.class_methods.insert(Symbol::from("rows"), Ty::Nil); + let classes = HashMap::from([(batch_id.clone(), batch)]); + let methods = parse_methods_with_rbs_in_ctx(ruby, rbs, &classes).expect("runtime parses"); + let consume = method(&methods, "consume"); + assert_eq!(consume.body.ty, Some(Ty::Int)); + assert_no_inference_gaps(&consume.body); + assert_local_type( + &consume.body, + "items", + Ty::Array { + elem: Box::new(Ty::Int), + }, + ); + assert_local_type(&consume.body, "item", Ty::Int); + assert_eq!( + classes[&batch_id].class_methods[&Symbol::from("rows")], + Ty::Nil + ); + assert_emitted_ruby( + &methods, + "ClassBatch", + "raise 'wrong sum' unless ClassBatch.consume == 7", + ); +} + +#[test] +fn zero_and_multiple_yield_parameters_keep_the_declared_block_arity() { + let ruby = r#" +class Arity + def none + yield + nil + end + + def pair + yield 3, "four" + nil + end + + def consume + none { 1.to_s } + pair { |number, word| number.to_s + word } + end +end +"#; + let rbs = r#" +class Arity + def none: () { () -> void } -> nil + def pair: () { (Integer, String) -> void } -> nil + def consume: () -> nil +end +"#; + let methods = + parse_methods_with_rbs_in_ctx(ruby, rbs, &HashMap::new()).expect("runtime parses"); + let consume = method(&methods, "consume"); + assert_eq!(consume.body.ty, Some(Ty::Nil)); + assert_no_inference_gaps(&consume.body); + assert_local_type(&consume.body, "number", Ty::Int); + assert_local_type(&consume.body, "word", Ty::Str); + assert_emitted_ruby( + &methods, + "Arity", + "raise 'wrong return' unless Arity.new.consume.nil?", + ); +} + +#[test] +fn empty_registry_still_types_sibling_methods_beside_block_contracts() { + // Empty caller registry + a block method must not leave a sparse + // ClassInfo that collapses `send(name)` to Nil or hides siblings. + let ruby = r#" +class Dyn + def rows + yield 1 + nil + end + + def label + "x" + end + + def pick(name) + send(name) + end +end +"#; + let rbs = r#" +class Dyn + def rows: () { (Integer) -> void } -> nil + def label: () -> String + def pick: (Symbol name) -> untyped +end +"#; + let methods = + parse_methods_with_rbs_in_ctx(ruby, rbs, &HashMap::new()).expect("runtime parses"); + let pick = method(&methods, "pick"); + assert_eq!(pick.body.ty, Some(Ty::Untyped)); + assert_no_inference_gaps(&pick.body); + assert_emitted_ruby( + &methods, + "Dyn", + "raise 'wrong label' unless Dyn.new.pick(:label) == 'x'", + ); +} + +#[test] +fn instance_block_receiver_sees_sibling_methods() { + let ruby = r#" +class Counter + def each + yield self + nil + end + + def value + 3 + end + + def consume + total = 0 + each { |me| total = total + me.value } + total + end +end +"#; + let rbs = r#" +class Counter + def each: () { (instance) -> void } -> nil + def value: () -> Integer + def consume: () -> Integer +end +"#; + let methods = + parse_methods_with_rbs_in_ctx(ruby, rbs, &HashMap::new()).expect("runtime parses"); + let consume = method(&methods, "consume"); + assert_eq!(consume.body.ty, Some(Ty::Int)); + assert_no_inference_gaps(&consume.body); + assert_emitted_ruby( + &methods, + "Counter", + "raise 'wrong sum' unless Counter.new.consume == 3", + ); +} + +#[test] +fn class_side_blocks_win_when_instance_shares_the_name() { + // Registry already has an instance `rows` block contract. Same-file + // class-side `rows` overlays the class table. Class-object calls must + // bind the class contract, not the instance one (dispatch order). + let ruby = r#" +class BothSides + def self.rows + yield 7 + nil + end + + def self.consume + total = 0 + rows { |n| total = n + 1 } + total + end +end +"#; + let rbs = r#" +class BothSides + def self.rows: () { (Integer) -> void } -> nil + def self.consume: () -> Integer +end +"#; + let batch_id = ClassId(Symbol::from("BothSides")); + let mut batch = ClassInfo::default(); + batch.instance_methods.insert( + Symbol::from("rows"), + Ty::Fn { + params: vec![], + ret: Box::new(Ty::Nil), + block: Some(Box::new(Ty::Fn { + params: vec![roundhouse::ty::Param { + name: Symbol::from("s"), + ty: Ty::Str, + kind: roundhouse::ty::ParamKind::Required, + }], + ret: Box::new(Ty::Nil), + block: None, + effects: roundhouse::effect::EffectSet::default(), + })), + effects: roundhouse::effect::EffectSet::default(), + }, + ); + let classes = HashMap::from([(batch_id, batch)]); + let methods = parse_methods_with_rbs_in_ctx(ruby, rbs, &classes).expect("runtime parses"); + let consume = method(&methods, "consume"); + assert_eq!(consume.body.ty, Some(Ty::Int)); + assert_no_inference_gaps(&consume.body); + assert_local_type(&consume.body, "n", Ty::Int); + assert_emitted_ruby( + &methods, + "BothSides", + "raise 'wrong sum' unless BothSides.consume == 8", + ); +} + +#[test] +fn non_block_methods_keep_cross_file_return_types() { + let ruby = r#" +class RegistryProbe + def rows + yield [1] + nil + end + + def existing + 1 + end + + def consume + rows { |items| items.each { |item| item.to_s } } + value = existing + foreign = Foreign.answer + value + foreign.to_s + end +end +"#; + let rbs = r#" +class RegistryProbe + def rows: () { (Array[Integer]) -> void } -> nil + def existing: () -> Integer + def consume: () -> String +end +"#; + let probe_id = ClassId(Symbol::from("RegistryProbe")); + let foreign_id = ClassId(Symbol::from("Foreign")); + let parent = ClassId(Symbol::from("Parent")); + let mut probe = ClassInfo::default(); + probe.instance_methods.insert(Symbol::from("rows"), Ty::Nil); + probe + .instance_methods + .insert(Symbol::from("existing"), Ty::Str); + probe.parent = Some(parent.clone()); + let mut foreign = ClassInfo::default(); + foreign + .class_methods + .insert(Symbol::from("answer"), Ty::Bool); + let classes = HashMap::from([(probe_id.clone(), probe), (foreign_id.clone(), foreign)]); + let methods = parse_methods_with_rbs_in_ctx(ruby, rbs, &classes).expect("runtime parses"); + let consume = method(&methods, "consume"); + assert_eq!(consume.body.ty, Some(Ty::Str)); + assert_no_inference_gaps(&consume.body); + assert_local_type(&consume.body, "value", Ty::Str); + assert_local_type(&consume.body, "foreign", Ty::Bool); + assert_eq!( + classes[&probe_id].instance_methods[&Symbol::from("existing")], + Ty::Str + ); + assert_eq!(classes[&probe_id].parent, Some(parent)); + assert_eq!( + classes[&foreign_id].class_methods[&Symbol::from("answer")], + Ty::Bool + ); +} diff --git a/tests/support/runtime_block_signature.rs b/tests/support/runtime_block_signature.rs new file mode 100644 index 000000000..05499e486 --- /dev/null +++ b/tests/support/runtime_block_signature.rs @@ -0,0 +1,21 @@ +pub const RUBY: &str = r#" +class Batch + def rows + yield [1, 2] + nil + end + + def consume + total = 0 + rows { |items| items.each { |item| total = total + item } } + total + end +end +"#; + +pub const RBS: &str = r#" +class Batch + def rows: () { (Array[Integer]) -> void } -> nil + def consume: () -> Integer +end +"#;