Bug #22071
closedsuper in method in module that is refined results in NoMethodError
Description
We currently support refinements of modules, even though doc/syntax/refinements.rdoc and https://github.com/ruby/ruby/wiki/Refinements-Spec imply that we would not respect refinements of included or prepended modules and would only respect refinements of classes and superclasses.
If you refine a module, and have the refined method definition call super and the method in the refined module call super, and that refined method is found first during method lookup, the super call in the method in the refined module results in a NoMethodError. Example:
module F
def a; "F" + super end
end
class A
def a; "A" end
end
class B < A
include F
end
p B.new.a
module R
refine F do
def a; "R" + super end
end
end
using R
p B.new.a
Output:
"FA"
t.rb:2:in 'F#a': super: no superclass method 'a' for an instance of B (NoMethodError)
from t.rb:17:in 'a'
from t.rb:22:in '<main>'
Two things to note:
- Before the refinement is activated, things work as expected, returning "FA".
- After the refinement is activated, the
NoMethodErroris raised not in the refinement methodsuper(line 17), but in thesuperin the method in the module that is refined (line 2).
What should be the expected behavior here? I can think of two possibilities:
-
Per the documentation, we should not consider refinements of included/prepended modules. So in this example, we should print "FA" and ignore the refinement instead of raising the
NoMethodError. In this case, it would be best to raise during therefine Fcall, and prohibit the refining of modules completely. This would break backwards compatibility, though I'm not sure how much real world code uses refinements of modules. -
Fix
superwhen the refinement is activated to use the correct method lookup and return "RFA" instead of raising theNoMethodError. Also fix the documentation to describe how refinements of included/prepended modules work during method lookup.
Updated by shugo (Shugo Maeda) 2 days ago
Refinements for modules were initially prohibited, and were allowed in [Feature #12534] while super was still prohibited. Supporting super in a refinement for a module would go beyond a bug fix, so I think it should be discussed as a new feature rather than as a bug.
Alternatively, it may be better to just change the error message to make the prohibition clearer.
Updated by shugo (Shugo Maeda) 2 days ago
- Status changed from Open to Assigned
- Assignee set to shugo (Shugo Maeda)
Updated by jeremyevans0 (Jeremy Evans) 1 day ago
shugo (Shugo Maeda) wrote in #note-1:
Refinements for modules were initially prohibited, and were allowed in [Feature #12534] while
superwas still prohibited. Supportingsuperin a refinement for a module would go beyond a bug fix, so I think it should be discussed as a new feature rather than as a bug.
I think the problem is that super is not prohibited, it just uses the wrong method lookup. Here's a revised example that showing a case where super does not raise an exception:
class BasicObject
def a; "B" end
end
module G
def a; "G" + super end
end
module F
include G
def a; "F" + super end
end
class A
def a; "A" + super end
end
class B < A
include F
end
p B.new.a
module R
refine F do
def a; "R"+super end
end
end
using R
p B.new.a
Output:
"FGAB"
"RFB"
What happens during the super call in F#a when F is refined by R is it determines the superclass is BasicObject. If BasicObject defines the method, than it doesn't raise NoMethodError.
So if we want to actually prohibit super, we have to decide how and where to do that.
-
Prohibiting
superinside the refinement method is probably easiest, but it breaks backwards compatibility for refinements of modules where the refinement method callssuper. -
Prohibiting
superinside the module itself (F#a) is more compatible. There are still some minor incompatibilities:-
The method implementation in
BasicObject(or a module included or prepended toBasicObject) will not be called. -
If
BasicObjectdoesn't define the method,method_missingin the underlying class will not be called (the example currently callsB#method_missing).
-
Alternatively, it may be better to just change the error message to make the prohibition clearer.
I haven't fully tested this, but it appears to work in some basic testing (passes test/ruby/test_refinement.rb and spec/ruby/core/refinement:
diff --git a/vm_insnhelper.c b/vm_insnhelper.c
index 84daaa85a5..f449ff0324 100644
--- a/vm_insnhelper.c
+++ b/vm_insnhelper.c
@@ -5061,6 +5061,13 @@ vm_search_super_method(const rb_control_frame_t *reg_cfp, struct rb_call_data *c
cc = vm_cc_new(Qundef, NULL, vm_call_method_missing, cc_type_super);
RB_OBJ_WRITE(iseq, &cd->cc, cc);
}
+ else if (klass == rb_cBasicObject &&
+ RB_TYPE_P(me->defined_class, T_ICLASS) &&
+ ((struct RClass_and_rb_classext_t*)me->defined_class)->classext.as.iclass.includer == 0) {
+ rb_raise(rb_eNoMethodError,
+ "super in a method in a module that has been refined and that is called via super"
+ " from a refinement method is not supported.");
+ }
else {
cc = vm_search_method_fastpath(reg_cfp, cd, klass);
const rb_callable_method_entry_t *cached_cme = vm_cc_cme(cc);
This implements the approach described in 2. above. Does this seem reasonable?
Updated by shugo (Shugo Maeda) 1 day ago
jeremyevans0 (Jeremy Evans) wrote in #note-3:
diff --git a/vm_insnhelper.c b/vm_insnhelper.c index 84daaa85a5..f449ff0324 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -5061,6 +5061,13 @@ vm_search_super_method(const rb_control_frame_t *reg_cfp, struct rb_call_data *c cc = vm_cc_new(Qundef, NULL, vm_call_method_missing, cc_type_super); RB_OBJ_WRITE(iseq, &cd->cc, cc); } + else if (klass == rb_cBasicObject && + RB_TYPE_P(me->defined_class, T_ICLASS) && + ((struct RClass_and_rb_classext_t*)me->defined_class)->classext.as.iclass.includer == 0) { + rb_raise(rb_eNoMethodError, + "super in a method in a module that has been refined and that is called via super" + " from a refinement method is not supported."); + } else { cc = vm_search_method_fastpath(reg_cfp, cd, klass); const rb_callable_method_entry_t *cached_cme = vm_cc_cme(cc);This implements the approach described in 2. above. Does this seem reasonable?
Thank you.
It seems reasonable except that RCLASS_INCLUDER(me->defined_class) is better than ((struct RClass_and_rb_classext_t*)me->defined_class)->classext.as.iclass.includer.
Updated by jeremyevans0 (Jeremy Evans) about 21 hours ago
shugo (Shugo Maeda) wrote in #note-4:
It seems reasonable except that
RCLASS_INCLUDER(me->defined_class)is better than((struct RClass_and_rb_classext_t*)me->defined_class)->classext.as.iclass.includer.
Thank you. I updated the patch, added a test, and submitted a PR: https://github.com/ruby/ruby/pull/16995
Updated by jeremyevans (Jeremy Evans) about 9 hours ago
- Status changed from Assigned to Closed
Applied in changeset git|9736b7ab56c87b589b8c8a638a019b6bbb19b127.
Prohibit super in refined module method
Before, this super could result in a call to a method in BasicObject
(or a module included in or prepended to BasicObject), or a call
to method_missing for the receiver's class.
This explicitly disallows the use of super in such a case, with a
specific error message.
Fixes [Bug #22071]