feat(vm): Module#module_function (partial — symbol form + private switch)#324
Conversation
…tch) After PR #320 (alias_method primitive walk) unblocked the rackup chain, sinatra-4's next failure was `undefined method 'module_function' for Class` at rack-3.1.10/lib/rack/utils.rb:37 (bare form) and :161 (symbol form). Two paths in the visibility intercept: 1. Symbol/String args (`module_function :forwarded_values`): copy each named method from `cls.methods` to `cls.singleton_methods` (callable as `M.foo`), mark the instance copy private. This is the canonical case — `rack/mime.rb` and many other gems use it to expose specific class-body-defined methods at the module level. 2. Bare form (`module_function`): CRuby's full semantics auto-mirror subsequent `def`s to the singleton class. Tier-1 takes the partial-correctness path: switch the current visibility stack entry to Private (so the instance copy is private). The auto-mirror to singleton is NOT implemented; `M.escape(...)` calls on bare-form-defined methods will fail at runtime with NoMethodError. This is enough to LOAD past the `rack/utils.rb:37` style pattern; runtime calls into those methods are out of scope for the sinatra LOAD probe. Diff fixture covers five shapes (private-switch / single Symbol arg / multi Symbol args / String arg / out-of-context send). Sinatra-4 probe now advances past rack/utils.rb to the next layer (`Rack::QueryParser::ParameterTypeError` const lookup — separate PR). (TRY_RUNS pass-10 layer #12.)
gapscan PR diffThis PR doesn't touch any file that affects gapscan classification — See docs/gap-reports/ for the dataset and methodology. |
There was a problem hiding this comment.
Pull request overview
Adds partial support for Ruby’s Module#module_function to unblock Sinatra’s Rack load chain, focusing on (1) retroactive conversion via symbol/string arguments and (2) a Tier-1 “bare form” visibility switch.
Changes:
- Implement
module_functionhandling in the VM’s visibility/dispatch intercept, including a bare-form “switch to private” path and an arg-based retroactive conversion path. - Add a new diff_cruby fixture covering several
module_functionshapes and wire it into the diff test runner. - Document current Tier-1 divergence around the bare form semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/rubyrs/src/vm/dispatch.rs | Adds module_function handling inside the existing visibility intercept logic. |
| crates/rubyrs/tests/diff/module_function.rb | New diff_cruby fixture exercising module_function in several forms. |
| crates/rubyrs/tests/diff_cruby.rs | Registers the new module_function diff fixture test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (sid, m) in methods_snapshot { | ||
| cls.singleton_methods.borrow_mut().insert(sid, m.clone()); | ||
| m.visibility.set(crate::value::Visibility::Private); | ||
| } |
There was a problem hiding this comment.
Adopted - confirmed via M.singleton_methods inspection. Singleton install now constructs a fresh Method with Visibility::Public; original instance gets Private. 4360aa5.
| if &*name == "module_function" { | ||
| if let Value::Class(cls) = &self_val { | ||
| if args.is_empty() { | ||
| // Bare form: subsequent defs in this body |
There was a problem hiding this comment.
Adopted - intercept arm now gated on Value::Class. Non-Class receivers fall through so the runtime raises NoMethodError. Fixture Shape 5 verifies. 4360aa5.
| ## Shape 5: out-of-context bare `module_function`. CRuby | ||
| ## raises NameError at top-level (it's a Module private | ||
| ## method, so implicit-self dispatch from `main` fails as | ||
| ## a private call from the wrong receiver). rubyrs treats it | ||
| ## as a silent no-op — documented Tier-1 divergence on the |
There was a problem hiding this comment.
Adopted - Shape 5 comment now describes Object.new.send(:module_function) shape (round 1 wording was about top-level which doesnt match the actual call). 4360aa5.
| // converts the listed already-defined instance methods. | ||
| // Outside a class body it's a no-op (matches CRuby's | ||
| // toplevel behavior). (TRY_RUNS pass-10 layer #12 — |
There was a problem hiding this comment.
Adopted - dispatch.rs comment block rewritten to be explicit about the Tier-1 divergences (bare-form auto-mirror gap + non-Class fall-through). 4360aa5.
| let methods_snapshot: Vec<(crate::intern::SymId, std::rc::Rc<crate::value::Method>)> = { | ||
| let methods = cls.methods.borrow(); | ||
| args.iter() | ||
| .filter_map(|a| match a { | ||
| Value::Sym(s) => Some(*s), | ||
| Value::Str(s) => Some(self.interner.intern(&s.to_string_lossy())), | ||
| _ => None, | ||
| }) | ||
| .filter_map(|sid| methods.get(&sid).map(|m| (sid, m.clone()))) | ||
| .collect() | ||
| }; | ||
| for (sid, m) in methods_snapshot { | ||
| cls.singleton_methods.borrow_mut().insert(sid, m.clone()); | ||
| m.visibility.set(crate::value::Visibility::Private); | ||
| } | ||
| self.method_gen = self.method_gen.wrapping_add(1); |
There was a problem hiding this comment.
Adopted three sub-findings: fresh Method with Public visibility, TypeError for non-Symbol/String args, NameError for missing source name, Config::max_symbols guard on String coercion. 4360aa5.
| if &*name == "module_function" { | ||
| if let Value::Class(cls) = &self_val { | ||
| if args.is_empty() { | ||
| // Bare form: subsequent defs in this body | ||
| // SHOULD become private instance methods | ||
| // AND get auto-copied to the module's |
There was a problem hiding this comment.
Adopted - module_function added to Vm::responds_to Class whitelist. respond_to?(:module_function, true) now matches the inline dispatch acceptance. 4360aa5.
| ## Shape 5: out-of-context bare `module_function`. CRuby | ||
| ## raises NameError at top-level (it's a Module private | ||
| ## method, so implicit-self dispatch from `main` fails as | ||
| ## a private call from the wrong receiver). rubyrs treats it | ||
| ## as a silent no-op — documented Tier-1 divergence on the |
There was a problem hiding this comment.
Adopted - same fix as the round 1 duplicate (3336085000). Shape 5 comment now describes the Object.new.send(...) call shape correctly. 4360aa5.
…or path
Round-1 introduced `module_function` with four real bugs surfaced
by Copilot review:
1. **Singleton copy shared Rc with private original**: the
symbol-arg form did `singleton_methods.insert(sid, m.clone())`
then `m.visibility.set(Private)` — but `Method.visibility` is
a `Cell` inside the Rc, so the singleton copy also became
private. `M.singleton_methods.include?(:foo)` worked but
reflection-driven `respond_to?` / `public_methods` would have
the wrong visibility. Now constructs a FRESH `Method` for the
singleton with `Visibility::Public`; original instance method
gets Private.
2. **Non-Class receiver silently no-op**: `Object.new.send(
:module_function)` was swallowed because the arm pushed Nil
regardless of receiver type. Now the intercept only fires for
`Value::Class` and falls through otherwise, letting the
runtime raise NoMethodError as CRuby does.
3. **Missing source method silently skipped**: `module_function
:no_such` was a no-op. Now raises NameError with the same
"undefined method `X' for module `Y'" shape CRuby uses.
4. **TypeError for non-Symbol/String args and Config::max_symbols
guard on String coercion**: both were missing. String args
now check the interner cap before coercion; non-coercible
args raise TypeError ("X is not a symbol nor a string").
Also added `module_function` to the `Vm::responds_to` Class
whitelist so `respond_to?(:module_function, true)` matches what
the inline dispatch actually accepts. Fixture Shape 5 comment
rewritten to describe the actual `Object.new.send(...)` call
shape (the round-1 wording falsely claimed top-level form).
(Code-review #324 round 1.)
| err = begin | ||
| Object.new.send(:module_function) | ||
| "no-raise" | ||
| rescue NoMethodError => e | ||
| e.message.include?("module_function") ? "rejected" : "other-#{e.message}" | ||
| end | ||
| puts "main-context=#{err.length > 0}" |
There was a problem hiding this comment.
Adopted - assertion now prints the classification string directly (rejected / no-raise / other-NoMethodError) so the byte-for-byte diff actually catches regressions. c921fb7.
Round-1 review-2 noted Shape 5's `puts "main-context=#{err.length > 0}"`
always prints `true` regardless of what `err` was classified as.
That makes the diff harness blind to regressions in the
non-Class receiver fall-through behavior.
Print the classification string directly (`rejected` /
`no-raise` / `other-NoMethodError`) so the byte-for-byte diff
against CRuby actually catches drift.
(Code-review #324 round 2.)
| let nm = self.interner.resolve(sid).to_string(); | ||
| return Err(self.trap(RubyError::NameError { | ||
| msg: format!( | ||
| "undefined method `{}' for module `{}'", | ||
| nm, cls.name, | ||
| ), | ||
| })); |
There was a problem hiding this comment.
Adopted - NameError message now picks 'module' vs 'class' from cls.is_module. 63b9939.
Round-2 review noted the NameError message hardcoded "for module" even when `module_function` is called inside a class body (`class C; module_function :foo; end`). CRuby uses "for class" in that case. Use `cls.is_module` to pick the right wording. (Code-review #324 round 3.)
| other => return Err(self.trap(RubyError::TypeError { | ||
| msg: format!( | ||
| "{} is not a symbol nor a string", | ||
| other.type_name(), | ||
| ), | ||
| })), |
There was a problem hiding this comment.
Adopted - TypeError now uses to_inspect so the message includes the actual value (e.g. '[:a, 1] is not a symbol nor a string'), matching dispatch.rs:619. 2d45c85.
| params: m.params.clone(), | ||
| proto_idx: m.proto_idx, | ||
| fixed_arity: m.fixed_arity, | ||
| defining_class: m.defining_class.clone(), |
There was a problem hiding this comment.
Adopted - singleton-copy defining_class now uses Rc::downgrade(cls) so super/Method#owner anchor at the class that physically holds it. 2d45c85.
| other => return Err(self.trap(RubyError::TypeError { | ||
| msg: format!( | ||
| "{} is not a symbol nor a string", | ||
| other.type_name(), | ||
| ), | ||
| })), |
There was a problem hiding this comment.
Duplicate of an earlier round-4 finding - addressed in 2d45c85 (see linked unique-content thread root).
| params: m.params.clone(), | ||
| proto_idx: m.proto_idx, | ||
| fixed_arity: m.fixed_arity, | ||
| defining_class: m.defining_class.clone(), |
There was a problem hiding this comment.
Duplicate of an earlier round-4 finding - addressed in 2d45c85 (see linked unique-content thread root).
| other => return Err(self.trap(RubyError::TypeError { | ||
| msg: format!( | ||
| "{} is not a symbol nor a string", | ||
| other.type_name(), | ||
| ), | ||
| })), |
There was a problem hiding this comment.
Duplicate of an earlier round-4 finding - addressed in 2d45c85 (see linked unique-content thread root).
| params: m.params.clone(), | ||
| proto_idx: m.proto_idx, | ||
| fixed_arity: m.fixed_arity, | ||
| defining_class: m.defining_class.clone(), |
There was a problem hiding this comment.
Duplicate of an earlier round-4 finding - addressed in 2d45c85 (see linked unique-content thread root).
Round-3 review noted two consistency issues: 1. TypeError for non-Symbol/String args used `type_name()`. The parallel `Symbol`-coercion site at dispatch.rs:619 uses `to_inspect(...)` so error messages include the actual value (e.g. `[:a, 1] is not a symbol nor a string` rather than `Array is not a symbol nor a string`). Aligning makes the diagnostic less ambiguous and the diff-fixture surface uniform with the rest of the VM. 2. The freshly-built singleton-copy `Method` cloned its `defining_class` from the source instance method. Other singleton-install paths anchor `defining_class` at the class that physically holds the singleton method, so `super` / `Method#owner` walk the right ancestor chain. Switch to `Rc::downgrade(cls)` for consistency. (Code-review #324 round 4.)
| ## dispatch from a non-Module receiver. CRuby raises | ||
| ## NoMethodError ("private method `module_function' called"). | ||
| ## Post-#324 round 2 rubyrs falls through (the intercept arm | ||
| ## only fires for `Value::Class` receivers), so the runtime | ||
| ## NoMethodError surfaces naturally. Substring-tolerant | ||
| ## check on the message accepts both interpreters' wording. |
There was a problem hiding this comment.
Adopted - verified send bypasses private check, so the actual CRuby error is undefined-method NoMethodError. Comment reworded. 152ae13.
Round-4 review noted Shape 5's comment falsely claimed CRuby raises a "private method ... called" NoMethodError. `send` bypasses the private-method check, so the actual error is "undefined method 'module_function' for an instance of Object" (verified). Reword the comment to match. No code change. (Code-review #324 round 5.)
| if &*name == "module_function" | ||
| && let Value::Class(cls) = &self_val | ||
| { |
There was a problem hiding this comment.
Declined as scope. Verified the same gap exists for the entire visibility-intercept family today: C.send(:private, :foo) raises undefined method 'private' for Class on master — same shape as Mod.send(:module_function, :foo). Adopting just for module_function would leave the family inconsistent; the right fix is a broader explicit-receiver/send-bypass routing pattern shared by private/public/protected/attr_*/alias_method/module_function. Tracked as a separate followup; this PR keeps the no_recv-only pattern that's idiomatic in the codebase today (152ae13 + earlier commits). The respond_to? whitelist addition is what matters for sinatra-4's LOAD probe.
| let nm = self.interner.resolve(sid).to_string(); | ||
| let kind = if cls.is_module { "module" } else { "class" }; | ||
| return Err(self.trap(RubyError::NameError { | ||
| msg: format!( | ||
| "undefined method `{}' for {} `{}'", | ||
| nm, kind, cls.name, | ||
| ), | ||
| })); |
There was a problem hiding this comment.
Adopted - matches the allocate-TypeError fallback at dispatch.rs:3032. Empty cls.name now renders as 'Module'/'Class' so the NameError stays actionable for anonymous shells. 0296866.
Round-5 review noted that anonymous modules/classes have an empty `cls.name`, so the NameError message rendered an empty backtick pair (`""`). Mirror the `allocate` TypeError path at dispatch.rs:3032 — fall back to "Module" / "Class" by the `is_module` flag so the error stays actionable. (Code-review #324 round 6.)
…rray) CRuby's Module#module_function return value (verified against MRI 3.x): - bare form → nil - single Sym/Str arg → the arg verbatim (string stays string) - multi args → array of args verbatim Earlier this arm always pushed Nil, which matched the bare form but silently diverged for the argument forms — any caller using the result as an expression got NilClass instead of the symbol/array. Adds Shape 6 to the diff fixture covering bare / single Sym / single Str / multi-arg returns. (Code-review #324 post-empty pass — Finding 1 of 3 adopted; findings 2 and 3 declined as non-bug / out of Tier-1 scope.)
Three passes condensed: layers #11/#12/#13/#14 closed across PRs #209/#324/#335/#337 between 2026-05-28 and 2026-06-01. Highlights: - Pass-10: PR #324 closed `Module#module_function` (Cat H). - Pass-11: PR #335 closed anonymous `def f(*)` → arity 0 (Cat I real bug, uncovered by Mustermann stub). - Pass-12: PR #337 closed `Class#inherited` callback never fired (Cat H, surfaced by `class App < Sinatra::Base`). - sinatra/base.rb now loads fully (REACHED-END) for the first time since the probe started. Next wall (open): #15 — `Kernel#caller`, surfaced by sinatra/base.rb:1913 `cleaned_caller` during base.rb's own load (Sinatra::Application < Base triggers the just-fixed inherited hook, which calls caller_files). Pre-existing follow-up uncovered: `def h(a, *, b)` Method#parameters renders `:opt` where CRuby renders `:rest` and arity -2 vs CRuby's -3. Separate post-rest required-param rendering bug, hidden by #13 until that fix landed.
Summary
*restsplat in method params #12 — sinatra-4 load chain's next blocker after PR feat(vm): alias_method walks ancestor chain for primitive whitelist #320 (alias_method primitive walk). rack-3.1.10'slib/rack/utils.rbuses both forms: baremodule_functionat line 37 to switch the body's visibility mode, andmodule_function :forwarded_valuesat line 161 to retroactively convert.Implementation
Two arms in the existing visibility intercept (dispatch.rs ~4042):
Symbol/String args: copy each named method from
cls.methodstocls.singleton_methods, mark the instance copy private. Canonical case — most gems use this form.Bare form: Tier-1 partial-correctness path. Switches the current visibility stack entry to
Private. CRuby's auto-mirror of subsequentdefs to the singleton class is NOT implemented —M.escape(...)calls on bare-form-defined methods fail at runtime with NoMethodError. This is enough to LOAD pastrack/utils.rb:37patterns; runtime calls into those methods are out of scope for the sinatra LOAD probe.Test plan
cargo test -p rubyrs --test diff_cruby— 304 green (+1module_function)cargo test -p rubyrs— full suite greenrequire "sinatra/base"advances past rack/utils.rb to the next layer (QueryParser::ParameterTypeErrorconst lookup).Documented gap (Tier-1 divergence)
Bare
module_functiondoesn't auto-mirror subsequentdefs to the module's singleton class.Rack::Utils.escape("foo")would fail at request time. Tracked as a follow-up if a real consumer needs the auto-mirror semantics; the fix would extendVisibilitywith aModuleFunctionvariant and routeOp::DefMethodthrough a dual-install path.Out-of-context behavior
Top-level bare
module_functionis a no-op in rubyrs (Nil return). CRuby raises NameError because module_function is a private Module method and main isn't a Module. Documented in the fixture (Shape 5) via substring-tolerant assertion so a future fix can land cleanly.