Repository navigation
A singleton definition on a receiver no binding traces no longer defines a method of main - #10797
Merged
Merged
Conversation
…nes a method of main
`def o.m`, `class << o` and `o.define_singleton_method(:m) { }` on a local
written twice, an instance variable or a global that holds an object define
the method on whichever object that is at run time. The scope walk files each
as a method of the enclosing scope, for register_singleton_defs to move onto
the synthesized class of the one `new` the variable traces to. When it traces
to none, nothing moved the scope: `m` stayed a method of main (or of the
enclosing class), so a bare `m` at the top level and `respond_to?(:m, true)`
found a method CRuby never defines, and a top-level `def m` beside it failed in
the C compiler on a conflicting declaration. The def forms were refused where
emitted instead, so a bare `m` raised NotImplementedError where CRuby raises
NameError.
An untraced definition now lands on a class no object is of
(sg_parked_class), through the reattach a traced one takes
(sg_reattach_scopes): its method is not callable at main, a call that needs it
is refused as before, and the def forms keep their refusals where emitted.
A constant that holds a class (`def Point.inherited` for a Struct) and a
builtin reopening's receiver (is_reopen_self_name) keep the class-method
filing the walk gave them, decided once for the refusal and the reattach.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Findings
 |
| Layer / File(s) | Summary |
|---|---|
Parked class and scope reattachment src/compiler.h, src/analyze_scope.c |
Compiler records the parked class index. Helpers create the shared parked class and reattach definition scopes as instance methods, including nested definitions in singleton-class bodies. |
Receiver classification and parking src/builtin_names.h, src/builtin_names.c, src/analyze_scope.c |
Class-side detection recognizes builtin reopening __self. The untraceable-receiver path retains refusal handling for definitions needing self and moves applicable definitions to the parked class. |
Untraceable receiver regression tests test/singleton_untraced_receiver.rb, test/singleton_untraced_receiver.rb.expected, test/singleton_untraced_name_clash.rb, test/singleton_untraced_name_clash.rb.expected |
Tests cover untraceable and traced receivers, singleton definitions, and a class-name clash. Expected outputs record method availability and NameError results. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Possibly related PRs
- matz/spinel#10738: Addresses
define_singleton_methodon an untraceable method parameter. - matz/spinel#8318: Handles untraceable singleton definitions that need
self.
Merge Risk: 
1bdd8
-
A
def self.xnested inside a method of a tracedclass << objbody no longer attaches to the object, so calling it can fail. The change is otherwise targeted and reportedly well tested. Fix this narrow regression before merging.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the main behavior change: a singleton definition on a receiver that no binding traces no longer defines a method of main. The raw summary shows that untraced singleton definitions are … Docstring Coverage Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main behavior change: a singleton definition on a receiver that no binding traces no longer defines a method of main. The raw summary shows that untraced singleton definitions are moved to a parked class, so the title matches the changeset. The title is specific and single-sentence, though it is long at 89 characters.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 5 files. (2 skipped: 1 unsupported, 1 too large.)
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…gram class can share
The class a singleton definition on an untraceable receiver is parked on
took only the DefNodes that are direct statements of a `class << obj` body,
so a def under an `if` stayed a method of the enclosing scope: a bare call of
its name at the top level found it where CRuby raises NameError. The
reattach now walks the body for every def in it (sg_reattach_body_defs),
stopping at a nested class, module or singleton class, and a def that is a
call's argument (`private def m`) moves too, onto the parked class only: on a
traced singleton class it would lose the visibility the call gives it, so it
stays where the walk filed it.
The parked class was also registered under the fixed name
Object__sg_untraced, so a program class of that name got the same C name and
failed to compile ("redefinition of sp_Object__sg_untraced_s"). It is named
from the node that first parks a definition, as the other classes this pass
synthesizes are (sg_get_or_make); its Ruby-visible name is still Object.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oo's method
The reattach of a `class << obj` body's defs moved every def in it onto the
object's singleton class as an instance method, a def with a receiver
included. A `def Foo.marker` in a method of a traced singleton class's body
defines Foo's own class method when that method runs, so moving it onto the
singleton class left Foo without it: a call of Foo.marker became a refusal
("no class in the program defines a class method"), where the commit before
compiled and answered it.
A def with a receiver now moves, like a def that is a call's argument, only
onto the parked class. There nothing of the body runs (a reachable
`class << obj` on an untraced receiver is refused), so all of it goes and Foo
gets no method from it, as in CRuby when the body never ran.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
|
Merged, thanks. Nothing to add on top. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Probed at master
47c2ee6f7, macOS, Apple clang 21.def o.m,class << oando.define_singleton_method(:m) { ... }define the method on whichever objectoholds when they run; main gets nothing. Whenois a variable Spinel cannot trace to onenewof a user class, the method stays a method of the top level (or of the enclosing class).respond_to?(:hi, true)at mainfalsetruefalsehiat mainNameErrorhiNameErrordef o.hi = "hi"false,NameErrorfalse, NotImplementedError ("singleton method on a receiver spinel cannot trace")false,NameErrorclass << o; def hi = "hi"; endfalse,NameErrortrue, answershifalse,NameErrorfalse,NameErrortrue, answershifalse,NameErrorwalk_scope_in(analyze_scope.c) files each of these as a method of the enclosing scope, with its class left open, forregister_singleton_defsto move onto the synthesized class of the onenewthe variable traces to.respond_to?find it, and a top-leveldefof the same name beside it fails in the C compiler on a conflicting declaration. The def forms were refused where emitted instead, so the call of the name raised NotImplementedError where CRuby raises NameError.Found while following up the review of #10738: the same leak is what kept a parameter's singleton definition at main.
An untraced definition lands on a class no object is of (analysis layer,
register_singleton_defsin analyze_scope.c). The scopes of an untraceddefine_singleton_method,def o.morclass << oare reattached, as a traced one's are (sg_reattach_scopes, the loop the traced path ran inline), to a class made for the purpose (sg_parked_class,Compiler.sg_parked) that nothing instantiates: its methods are not callable at main, a call that needs one is refused as before (the def forms keepsg_untraced_refusedandsg_needs_self_refused, raised where they are emitted), and a bare call of the name is the NameError it is in CRuby. A constant that holds a class (def Point.inheritedfor a Struct constant) and a builtin reopening's receiver (is_reopen_self_name) keep the class-method filing the walk gave them: that fact is computed once (class_side) and read by both the refusal and the reattach.extendhas no scope to move. Nocomp_ntyperead, no inline name test, no file-scope table.Shapes that change. A
define_singleton_method,def o.morclass << oon a local written twice, an instance variable or a global that holds an object, in a method that never runs or whose name is called at main.Corpus C diff. The C of 1,239 programs (every
test/*.rbandpackages/*/test/*.rbthat usesdefine_singleton_method, adefon a receiver,class <<or.extend, 60 randomtest/*.rb, the 73benchmark/*.rband optcarrot) is byte-identical against master in both--load-model=inlineand--load-model=graph, except three tests: the new one,test/define_singleton_method_hook_param.rbandtest/singleton_def_on_new_block_param.rb, whose untraced definitions (a hook's block parameter, a block parameter of anew) now sit on the class. In those the C gains one empty class: a row in the class tables (sp_cls_is_module,sp_class_to_s) and no method that is emitted.make cidentagainst47c2ee6f7: 11,380 identical, 3 differ (those three), 0 refusal changes.Tests.
test/singleton_untraced_receiver.rb(the shapes above on a local written twice and on an instance variable,extendbeside them, a traced local still defining its method) fails on master and passes plain, with--int-overflow=promote, underSPINEL_GC_STRESS=1and with--load-model=graph. 309 programs on these shapes, in both load models (the review's and my own, among them #10738's 69 probes): against master 10 move from wrong to right, 1 from a C error to right and 2 from wrong to a refusal; 101 right stay right, 79 refused stay refused, 90 stay a C error and 26 stay wrong, and none turns from right to refused, from refused to wrong or a C error, or from a C error to wrong; the one that turns from right to wrong is the constant-reflection case below. The two wrong-to-refused moves: a top-levelmethod_missingbeside a parked method and a bare call of its name (CRuby prints whatmethod_missingreturns; master ran the leaked method; Spinel does not dispatchmethod_missingand now stops at compile time with the NameError), andsend(:hi)at main (CRuby raises NoMethodError; master ran the leaked method; it is now an unsupported call). Of the 26 that stay wrong, 25 print master's answer and one (a module hook's block parameter) prints one line fewer wrong. The 100-probe set of #10737 gives master's answer, and the runs of the changed programs that compile give one answer each under GC stress and promote.make infer-testandtools/load_graph_check.shpass.Follow-up commit (CodeRabbit's two findings). A def under an
if(or any other statement) in a parkedclass << objbody stayed a method of main; the reattach now walks the whole body, stopping at a nested class, module or singleton class (sg_reattach_body_defs). And the parked class was registered under the fixed C nameObject__sg_untraced, so a program class of that name failed to compile; it is now named from the node that first parks a definition, as the pass's other synthesized classes are (sg_get_or_make), and its Ruby-visible name is stillObject.test/singleton_untraced_receiver.rbgains the def under anifand aprivate def;test/singleton_untraced_name_clash.rbis new. Both fail at the PR's first head and pass in both load models, plain, with--int-overflow=promoteand underSPINEL_GC_STRESS=1. Of 182 singleton,sclassanddefine_singleton_methodtests and benchmarks, the C of 2 other programs changes, by the parked class's C name only.Not covered, also on master:
register_singleton_defs, so a copy's untraced definition is never parked. Without the include the method is parked.Object__sg_parked_<node>) answersObject.const_defined?withtruefor its name, as master's other synthesized classes (Object__sg_base) do.class << objwhose receiver a binding traces,private def mstill definesmon main:respond_to?(:m, true)at the top level istruewhere CRuby saysfalse.make gate(on this branch merged with current master)Merged with master
47c2ee6f7:.expectedfiles that match CRuby 4.0 run with--enable-frozen-string-literal# spinel: int64(no value past 2^31)🤖 Generated with Claude Code
Summary by CodeRabbit