diff --git a/NEWS.md b/NEWS.md index 9627c9d677c..e0384870e0f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -7,6 +7,10 @@ Note that each entry is kept to a minimum, see links for details. ## Language changes +* `super` in a module method that was called via `super` from a refinement + method now works correctly. + [[Feature #22309]] + * `Module#clone` and `Module#dup` no longer rewrite the lexical scope of copied methods. Constants and class variables resolve through the original class, consistent with inheritance and mixins. @@ -650,6 +654,7 @@ A lot of work has gone into making Ractors more stable, performant, and usable. [Feature #22232]: https://bugs.ruby-lang.org/issues/22232 [Feature #22238]: https://bugs.ruby-lang.org/issues/22238 [Feature #22297]: https://bugs.ruby-lang.org/issues/22297 +[Feature #22309]: https://bugs.ruby-lang.org/issues/22309 [Feature #22400]: https://bugs.ruby-lang.org/issues/22400 [PR #17201]: https://github.com/ruby/ruby/pull/17201 [GH-psych #805]: https://github.com/ruby/psych/pull/805 diff --git a/gc.c b/gc.c index 6cb007d55f2..32f189fe678 100644 --- a/gc.c +++ b/gc.c @@ -3756,6 +3756,9 @@ gc_mark_classext_iclass(rb_classext_t *ext, bool prime, VALUE box_value, void *a if (RCLASSEXT_INCLUDER(ext)) { gc_mark_internal(ctx, RCLASSEXT_INCLUDER(ext)); } + if (RCLASSEXT_MODULE_REFINEMENT_ICLASSES(ext)) { + gc_mark_internal(ctx, RCLASSEXT_MODULE_REFINEMENT_ICLASSES(ext)); + } mark_m_tbl(ctx, RCLASSEXT_CALLABLE_M_TBL(ext)); gc_mark_internal(ctx, RCLASSEXT_CC_TBL(ext)); if (RCLASSEXT_SUBCLASSES(ext)) { diff --git a/internal/class.h b/internal/class.h index 773ad4e952d..b31a8452ff7 100644 --- a/internal/class.h +++ b/internal/class.h @@ -40,6 +40,12 @@ struct rb_classext_struct { const rb_box_t *box; VALUE super; VALUE fields_obj; // Fields are either ivar or other internal properties stored inline + /** + * For class/module: name of class/module + * For refinement iclass: hidden hash with refinement module keys and module refinement + * iclass values + * TODO: move into as union below + */ VALUE classpath; struct rb_id_table *m_tbl; struct rb_id_table *const_tbl; @@ -180,6 +186,7 @@ static inline rb_classext_t * RCLASS_EXT_WRITABLE(VALUE obj); #define RCLASSEXT_ICLASS_ORIGIN_SHARED_MTBL(ext) (ext->iclass_origin_shared_mtbl) #define RCLASSEXT_SUPERCLASSES_WITH_SELF(ext) (ext->superclasses_with_self) #define RCLASSEXT_CLASSPATH(ext) (ext->classpath) +#define RCLASSEXT_MODULE_REFINEMENT_ICLASSES(ext) (ext->classpath) static inline void RCLASSEXT_SET_ORIGIN(rb_classext_t *ext, VALUE klass, VALUE origin); static inline void RCLASSEXT_SET_INCLUDER(rb_classext_t *ext, VALUE klass, VALUE includer); @@ -221,6 +228,7 @@ static inline void RCLASSEXT_SET_INCLUDER(rb_classext_t *ext, VALUE klass, VALUE #define RCLASS_REFINED_CLASS(c) (RCLASS_EXT_PRIME(c)->refined_class) #define RCLASS_ATTACHED_OBJECT(c) (RCLASS_EXT_PRIME(c)->as.singleton_class.attached_object) #define RCLASS_INCLUDER(c) (RCLASS_EXT_PRIME(c)->as.iclass.includer) +#define RCLASS_MODULE_REFINEMENT_ICLASSES(c) (RCLASS_EXT_PRIME(c)->classpath) // max IV count and variation count are just hints, so they don't need to be per-box #define RCLASS_MAX_IV_COUNT(ext) (RCLASS_EXT_PRIME(ext)->max_iv_count) @@ -259,6 +267,7 @@ static inline void RCLASS_SET_ALLOCATOR(VALUE klass, rb_alloc_func_t allocator); static inline VALUE RCLASS_SET_ATTACHED_OBJECT(VALUE klass, VALUE attached_object); static inline void RCLASS_SET_INCLUDER(VALUE iclass, VALUE klass); +static inline void RCLASS_SET_MODULE_REFINEMENT_ICLASSES(VALUE iclass, VALUE table); static inline void RCLASS_SET_MAX_IV_COUNT(VALUE klass, attr_index_t count); static inline void RCLASS_SET_CLASSPATH(VALUE klass, VALUE classpath, bool permanent); static inline void RCLASS_WRITE_CLASSPATH(VALUE klass, VALUE classpath, bool permanent); @@ -699,6 +708,13 @@ RCLASS_SET_INCLUDER(VALUE iclass, VALUE klass) RB_OBJ_WRITE(iclass, &RCLASS_INCLUDER(iclass), klass); } +static inline void +RCLASS_SET_MODULE_REFINEMENT_ICLASSES(VALUE iclass, VALUE table) +{ + RUBY_ASSERT(RB_TYPE_P(iclass, T_ICLASS)); + RB_OBJ_WRITE(iclass, &RCLASS_MODULE_REFINEMENT_ICLASSES(iclass), table); +} + static inline void RCLASS_WRITE_SUPERCLASSES(VALUE klass, size_t depth, VALUE *superclasses, bool with_self) { diff --git a/test/ruby/test_refinement.rb b/test/ruby/test_refinement.rb index dce09c2dd8d..31c9d164141 100644 --- a/test/ruby/test_refinement.rb +++ b/test/ruby/test_refinement.rb @@ -1035,24 +1035,35 @@ class TestRefinement < Test::Unit::TestCase RUBY end - def test_prohibit_super_in_refined_module_method + def test_super_in_refined_module_method assert_separately([], <<-"end;") - bug22071 = '[ruby-core:125511] [Bug #22071]' class BasicObject - def a; "B" end + def a + raise 'defined?(super) true, should be false' if defined?(super) + "B" + end end module G - def a; "G" + super end + def a + raise 'defined?(super) false, should be true' unless defined?(super) + "G" + super + end end module F include G - def a; "F" + super end + def a + raise 'defined?(super) false, should be true' unless defined?(super) + "F" + super + end end class A - def a; "A" + super end + def a + raise 'defined?(super) false, should be true' unless defined?(super) + "A" + super + end end class B < A @@ -1061,14 +1072,658 @@ class TestRefinement < Test::Unit::TestCase module R refine F do - def a; "R"+super end + def a + raise 'defined?(super) false, should be true' unless defined?(super) + "R"+super + end end end using R - msg = "super in a method in a module that has been refined and that is called via super" + - " from a refinement method is not supported." - assert_raise(NoMethodError, msg, bug22071) { B.new.a } + assert_equal("RFGAB", B.new.a) + end; + end + + def test_super_in_refined_prepended_module_method + assert_separately([], <<-"end;") + module M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:M, *super] + end + end + + class C + prepend M + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + module R + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:R, *super] + end + end + end + using R + + assert_equal([:R, :M, :C], C.new.m) + end; + end + + def test_super_in_refined_prepended_module_method_in_thread + assert_separately([], <<-"end;", timeout: 1) + module M + def m + raise 'defined?(super) false, should be true' unless Thread.new{defined?(super)}.value + [:M, *Thread.new{super}.value] + end + end + + module R + refine M do + def m + raise 'defined?(super) false, should be true' unless Thread.new{defined?(super)}.value + Thread.new{[:R, *super]}.value + end + end + end + using R + + class C + prepend M + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + class SC < C + prepend M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:SC, *super] + end + end + + assert_equal([:R, :M, :SC, :R, :M, :C], SC.new.m) + end; + end + + + def test_super_in_refined_prepended_module_method_called_by_other_method_same_object + assert_separately([], <<-"end;") + module M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:M, *super] + end + end + + module N + include M + end + + class C + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + class SC < C + include N + prepend M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:SC, *super] + end + end + + module R + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:R, *super] + end + end + end + using R + + module N + def n + m + end + end + + assert_equal([:R, :M, :SC, :M, :C], SC.new.n) + end; + end + + def test_super_in_refined_prepended_module_method_with_aliases + assert_separately([], <<-"end;") + module M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:Mm, *super] + end + def n + raise 'defined?(super) false, should be true' unless defined?(super) + [:Mn, *super] + end + def o + raise 'defined?(super) false, should be true' unless defined?(super) + [:Mo, *super] + end + end + + module R + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:Rm, *super] + end + def n + raise 'defined?(super) false, should be true' unless defined?(super) + [:Rn, *super] + end + def o + raise 'defined?(super) false, should be true' unless defined?(super) + [:Ro, *super] + end + end + end + using R + + module N + include M + def n = m + end + + class C + prepend M + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + class SC < C + include N + prepend M + alias n m + end + + class SSC < SC + prepend M + def n + raise 'defined?(super) false, should be true' unless defined?(super) + [:SSC, *super] + end + end + + class SSSC < SSC + prepend M + alias o n + end + + class SSSSC < SSSC + prepend M + def o + raise 'defined?(super) false, should be true' unless defined?(super) + [:SSSSC, *super] + end + end + + assert_equal([:Ro, :Mo, :SSSSC, :Ro, :Mo, :Mn, :Mn, :SSC, :Rn, :Mn, :Mm, :Mm, :C], SSSSC.new.o) + end; + end + + def test_super_in_refined_prepended_module_method_called_by_other_object + assert_separately([], <<-"end;") + module M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:M, *super] + end + end + + class C + prepend M + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + module R + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:R, *super] + end + end + end + using R + + o = Object.new + def o.m = C.new.m + assert_equal([:R, :M, :C], o.m) + end; + end + + def test_super_in_refined_prepended_module_method_refined_multiple_times + assert_separately([], <<-"end;") + module M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:M, *super] + end + end + + class C + prepend M + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + module R1 + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:R1, *super] + end + end + end + using R1 + + module R2 + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:R2, *super] + end + end + end + using R2 + + assert_equal([:R2, :R1, :M, :C], C.new.m) + end; + end + + def test_super_in_refined_module_method_prepended_multiple_times + assert_separately([], <<-"end;") + module M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:M, *super] + end + end + + module R + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:R, *super] + end + end + end + using R + + class C + prepend M + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + class SC < C + prepend M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:SC, *super] + end + end + + assert_equal([:R, :M, :SC, :R, :M, :C], SC.new.m) + end; + end + + def test_super_in_refined_module_method_with_multiple_refined_modules + assert_separately([], <<-"end;") + module M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:M, *super] + end + end + + module N + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:N, *super] + end + end + + module R + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:RM, *super] + end + end + + refine N do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:RN, *super] + end + end + end + using R + + class C + prepend M + prepend N + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + class SC < C + prepend N + prepend M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:SC, *super] + end + end + + assert_equal([:RM, :M, :N, :SC, :RN, :N, :M, :C], SC.new.m) + end; + end + + def test_super_in_refined_module_method_with_super_method_after_refinement + assert_separately([], <<-"end;") + module M + end + + module R + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:R, *super] + end + end + end + using R + + module M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:M, *super] + end + end + + class C + prepend M + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + class SC < C + prepend M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:SC, *super] + end + end + + assert_equal([:R, :M, :SC, :R, :M, :C], SC.new.m) + end; + end + + def test_super_in_refined_module_method_with_multiple_refined_modules_with_super_method_after_refinement + assert_separately([], <<-"end;") + module M + end + + module N + end + + module R + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:RM, *super] + end + end + + refine N do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:RN, *super] + end + end + end + using R + + module M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:M, *super] + end + end + + module N + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:N, *super] + end + end + + class C + prepend M + prepend N + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + class SC < C + prepend N + prepend M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:SC, *super] + end + end + + assert_equal([:RM, :M, :RN, :N, :SC, :RN, :N, :RM, :M, :C], SC.new.m) + end; + end + + def test_super_in_refined_module_method_in_nested_block + assert_separately([], <<-"end;") + module M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:M, *super]}.flatten}.call + end + end + + module R + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:R, *super]}.flatten}.call + end + end + end + using R + + class C + prepend M + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + class SC < C + prepend M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + [:SC, *super] + end + end + + assert_equal([:R, :M, :SC, :R, :M, :C], SC.new.m) + end; + end + + def test_super_in_refined_module_method_combined_cases + assert_separately([], <<-"end;") + module M + define_method(:m) do + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:M, *super()]}.flatten}.call + end + end + + module N + def m + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:N, *super]}.flatten}.call + end + end + + module P + def m + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:P, *super]}.flatten}.call + end + end + + module R1 + refine M do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:R1M, *super]}.flatten}.call + end + end + + refine N do + define_method(:m) do + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:R1N, *super()]}.flatten}.call + end + end + + refine P do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:R1P, *super]}.flatten}.call + end + end + end + using R1 + + module R2 + refine M do + define_method(:m) do + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:R2M, *super()]}.flatten}.call + end + end + + refine N do + def m + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:R2N, *super]}.flatten}.call + end + end + + refine P do + define_method(:m) do + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:R2P, *super()]}.flatten}.call + end + end + end + using R2 + + class C + prepend M + prepend N + def m + raise 'defined?(super) true, should be false' if defined?(super) + :C + end + end + + class SC < C + include P + def m + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:SC, *super]}.flatten}.call + end + end + + class SSC < SC + prepend N + prepend M + def m + raise 'defined?(super) false, should be true' unless defined?(super) + ->{Array.new(1){[:SSC, *super]}.flatten}.call + end + end + + o = Object.new + def o.m = SSC.new.m + assert_equal([:R2M, :R1M, :M, :N, :SSC, :SC, :R2P, :R1P, :P, :N, :M, :C], o.m) + end; + end + + def test_super_in_refined_module_bmethod_with_no_super_method + assert_separately([], <<-"end;") + called = [] + M = Module.new do + define_method(:m) do + called << :M + raise 'defined?(super) true, should be false' if defined?(super) + super() + end + end + + class C + prepend M + end + + R = Module.new do + refine M do + define_method(:m) do + called << :R + raise 'defined?(super) false, should be true' unless defined?(super) + super() + end + end + end + using R + + assert_raise(NoMethodError) { C.new.m } + assert_equal([:R, :M], called) end; end diff --git a/vm_insnhelper.c b/vm_insnhelper.c index 87d0178419c..d395f667150 100644 --- a/vm_insnhelper.c +++ b/vm_insnhelper.c @@ -4603,7 +4603,7 @@ current_method_entry(const rb_execution_context_t *ec, rb_control_frame_t *cfp) { rb_control_frame_t *top_cfp = cfp; - if (CFP_ISEQ(cfp) && ISEQ_BODY(CFP_ISEQ(cfp))->type == ISEQ_TYPE_BLOCK) { + if (CFP_ISEQ(cfp) && !VM_FRAME_BMETHOD_P(cfp) && ISEQ_BODY(CFP_ISEQ(cfp))->type == ISEQ_TYPE_BLOCK) { const rb_iseq_t *local_iseq = ISEQ_BODY(CFP_ISEQ(cfp))->local_iseq; do { @@ -4612,7 +4612,7 @@ current_method_entry(const rb_execution_context_t *ec, rb_control_frame_t *cfp) /* TODO: orphan block */ return top_cfp; } - } while (CFP_ISEQ(cfp) != local_iseq); + } while (CFP_ISEQ(cfp) != local_iseq && !VM_FRAME_BMETHOD_P(cfp)); } return cfp; } @@ -4640,6 +4640,42 @@ refined_method_callable_without_refinement(const rb_callable_method_entry_t *me) return cme; } +// Construct correct super chain for module refinements. +// Each class including/prepending a module has a hash mapping refinement +// modules to refinement module iclasses with the correct super. Like +// singleton classes, these are lazily initialized (both the hash and +// the refinement module iclasses). +static VALUE +module_refinement_iclass(VALUE refinement_iclass, VALUE defined_class) +{ + VALUE refinement_module = RBASIC(refinement_iclass)->klass; + + VALUE cache = RCLASS_MODULE_REFINEMENT_ICLASSES(defined_class); + if (LIKELY(cache)) { + VALUE cached = rb_hash_lookup(cache, refinement_module); + if (!NIL_P(cached)) return cached; + } + else { + cache = rb_obj_hide(rb_ident_hash_new()); + RCLASS_SET_MODULE_REFINEMENT_ICLASSES(defined_class, cache); + } + + VALUE super; + VALUE refinement_iclass_super = RCLASS_SUPER(refinement_iclass); + if (RICLASS_FOR_REFINEMENT_P(refinement_iclass_super)) { + super = module_refinement_iclass(refinement_iclass_super, defined_class); + } + else { + super = defined_class; + } + + VALUE module_ref_iclass = rb_include_class_new(refinement_module, super); + RCLASS_SET_REFINED_CLASS(module_ref_iclass, RCLASS_REFINED_CLASS(refinement_iclass)); + rb_hash_aset(cache, refinement_module, module_ref_iclass); + + return module_ref_iclass; +} + static const rb_callable_method_entry_t * search_refined_method(rb_execution_context_t *ec, rb_control_frame_t *cfp, struct rb_calling_info *calling) { @@ -4649,9 +4685,13 @@ search_refined_method(rb_execution_context_t *ec, rb_control_frame_t *cfp, struc const rb_callable_method_entry_t *cme = vm_cc_cme(cc); for (; cref; cref = CREF_NEXT(cref)) { - const VALUE refinement = find_refinement(CREF_REFINEMENTS(cref), vm_cc_cme(cc)->owner); + VALUE refinement = find_refinement(CREF_REFINEMENTS(cref), vm_cc_cme(cc)->owner); if (NIL_P(refinement)) continue; + if (RB_TYPE_P(vm_cc_cme(cc)->owner, T_MODULE)) { + refinement = module_refinement_iclass(refinement, vm_cc_cme(cc)->defined_class); + } + const rb_callable_method_entry_t *const ref_me = rb_callable_method_entry(refinement, mid); @@ -5098,7 +5138,10 @@ static inline VALUE vm_search_normal_superclass(VALUE klass) { if (RICLASS_FOR_REFINEMENT_P(klass)) { - klass = RBASIC(klass)->klass; + VALUE refinement_module = RBASIC(klass)->klass; + if (!RB_TYPE_P(rb_refinement_module_get_refined_class(refinement_module), T_MODULE)) { + klass = refinement_module; + } } klass = RCLASS_ORIGIN(klass); return RCLASS_SUPER(klass); @@ -5176,13 +5219,6 @@ 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) && - RCLASS_INCLUDER(me->defined_class) == 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);