Feature #22309: Allow super in a module method to work if method was called by refinement method super

• Status: Open
• Assignee: -
• Target version: -

[jeremyevans0 (Jeremy Evans)]

Ruby started allowing refinements of modules in 2.4. However, there was one limitation, which is that while the refinement method could call super to call the module method, the module method could not call super, because it did not have enough information to determine the appropriate ancestor. This limitation was known at the time and was accepted when module refinements were accepted in #12534.

As discussed in #22071, super wasn't actually prohibited in the module method, it just used the incorrect method lookup, looking for a super method in BasicObject. After discussion in #22071, I fixed this to use an explicit exception for this case, so an error was was raised for it.

While that addressed the incorrect super method lookup issue, it was an unsatisfying conclusion. I kept thinking about ways to actually fix the issue. My initial idea was to use a separate T_ICLASS for the module being refined in every class ancestry chain where it could be called via refinement super. However, that approach would result in a large amount of complexity, as well as potentially significant additional memory.

I am instead proposing an alternative approach which relies on the fact that when we call super in the module method, we know that the current method is a module method, and that the previous method in the call stack is a refinement method. We can walk up the call stack to find the refinement method, look at the previous method calling that to determine the class to use for the super lookup. Note that the previous method may use a different receiver/method, in which case we start with the receiver's class. There are cases where we need to do this multiple times, if there are multiple refinements or multiple modules being refined that have already called super to reach this point. I'm not sure this alternative approach works in all cases, but I have gotten it to work every case I've tried:

• Module and refinement appears multiple times in the ancestor chain
• super calls both directly in method as well as in nested blocks in method
• Both refinement method and module method are bmethods
• Refinement refines multiple modules and both modules are involved in the same super call chain
• Multiple refinements of the same module method
• Use of ZSUPER aliases in the super call chain
• The super call in the module method is not found, and NoMethodError is raised correctly

We want the behavior of module refinement super to be consistent, so in addition to fixing super itself, we also need to fix defined?(super) as well as Method#super_method and UnboundMethod#super_method. defined?(super) is fixed by sharing the lookup that super now uses. However, Method#super_method and UnboundMethod#super_method require a different approach. Thankfully, struct METHOD already has a member named iclass that is used to implement #super_method. So we just need to make adjustments to the setting of the iclass member for #super_method to work.

Simple example:

module M
  def m = [:M, *super]
end

class C
  prepend M
  def m = :C
end

module R
  refine M do
    def m = [:R, *super]
  end
end
using R

C.new.m
# => [:R, :M, :C]

I've submitted a pull request to implement this: https://github.com/ruby/ruby/pull/18797

[shugo (Shugo Maeda)]

Thank you for working on this. I checked out the PR branch and tested it.

The stack walk can be fooled. It assumes that the frame below the refinement frame, if it has the same self and the same method id, is the frame that called super. That frame may instead have reached the refinement by an ordinary call, in which case an ancestor is skipped:

module M
  def m(x) = [:M, *super]
end
module R
  refine M do
    def m(x) = [:R, *super]
  end
end
using R

class Base
  prepend M
  def m(x) = [:Base]
end
class C < Base
  prepend M
  def m(x) = x ? [:C1, *m(false)] : [:C2, *super]
end

C.new.m(true)
# expected: [:R, :M, :C1, :R, :M, :C2, :R, :M, :Base]
# actual:   [:R, :M, :C1, :R, :M, :Base]

This requires M to appear twice in the receiver's ancestry (with prepend as above, or by including M into a superclass after a subclass already included it); with a single iclass of M the search has only one place to land, so it cannot go wrong. The fallback (the first iclass of M in the receiver's ancestry) is not always right either: on C above, C.new.method(:m).super_method.super_method.super_method correctly denotes the refinement method for the inner M, but calling it restarts from the outer M, because the walk stops at the Method#call frame and the iclass the Method object knows about never reaches the frame.

YJIT and ZJIT also need attention. With --yjit or --zjit (call threshold 1 or 2), the scenario of test_super_in_refined_module_method returns "RFB" instead of "RFGAB": both JITs compute the superclass of the refinement's M iclass as BasicObject and, since BasicObject#a exists, dispatch there directly.

I think these problems are inherent in inferring the ancestor from the call stack: the information that is actually needed is which iclass of M in the receiver's ancestry the dispatch went through, and it is discarded when vm_call_refined switches to the refinement's method entry. About the alternative you described:

My initial idea was to use a separate T_ICLASS for the module being refined in every class ancestry chain where it could be called via refinement super. However, that approach would result in a large amount of complexity, as well as potentially significant additional memory.

I am afraid that, despite the additional complexity and memory, this is the only way to get correct behavior. The refinement method's super has to come back to the iclass of M in the receiver's ancestry, hit the REFINED entry there and fall through to orig_me with that iclass as defined_class, the same way class refinements work today; nothing short of a per-iclass refinement chain gives the module method's frame that information. The memory cost could be limited by creating the chain lazily in search_refined_method, keyed by the iclass of M it was dispatched through, so that only the (using scope, M iclass) pairs actually reached through refinement dispatch get one, but the complexity you mentioned remains.

[jeremyevans0 (Jeremy Evans)]

shugo (Shugo Maeda) wrote in #note-1:

Thank you for working on this. I checked out the PR branch and tested it.

The stack walk can be fooled. It assumes that the frame below the refinement frame, if it has the same self and the same method id, is the frame that called super. That frame may instead have reached the refinement by an ordinary call, in which case an ancestor is skipped:

module M
  def m(x) = [:M, *super]
end
module R
  refine M do
    def m(x) = [:R, *super]
  end
end
using R

class Base
  prepend M
  def m(x) = [:Base]
end
class C < Base
  prepend M
  def m(x) = x ? [:C1, *m(false)] : [:C2, *super]
end

C.new.m(true)
# expected: [:R, :M, :C1, :R, :M, :C2, :R, :M, :Base]
# actual:   [:R, :M, :C1, :R, :M, :Base]

This requires M to appear twice in the receiver's ancestry (with prepend as above, or by including M into a superclass after a subclass already included it); with a single iclass of M the search has only one place to land, so it cannot go wrong. The fallback (the first iclass of M in the receiver's ancestry) is not always right either: on C above, C.new.method(:m).super_method.super_method.super_method correctly denotes the refinement method for the inner M, but calling it restarts from the outer M, because the walk stops at the Method#call frame and the iclass the Method object knows about never reaches the frame.

YJIT and ZJIT also need attention. With --yjit or --zjit (call threshold 1 or 2), the scenario of test_super_in_refined_module_method returns "RFB" instead of "RFGAB": both JITs compute the superclass of the refinement's M iclass as BasicObject and, since BasicObject#a exists, dispatch there directly.

I think these problems are inherent in inferring the ancestor from the call stack: the information that is actually needed is which iclass of M in the receiver's ancestry the dispatch went through, and it is discarded when vm_call_refined switches to the refinement's method entry. About the alternative you described:

My initial idea was to use a separate T_ICLASS for the module being refined in every class ancestry chain where it could be called via refinement super. However, that approach would result in a large amount of complexity, as well as potentially significant additional memory.

I am afraid that, despite the additional complexity and memory, this is the only way to get correct behavior. The refinement method's super has to come back to the iclass of M in the receiver's ancestry, hit the REFINED entry there and fall through to orig_me with that iclass as defined_class, the same way class refinements work today; nothing short of a per-iclass refinement chain gives the module method's frame that information. The memory cost could be limited by creating the chain lazily in search_refined_method, keyed by the iclass of M it was dispatched through, so that only the (using scope, M iclass) pairs actually reached through refinement dispatch get one, but the complexity you mentioned remains.

Thank you for the review. I think a possible solution to the incorrect behavior in the example you gave would be a VM frame flag that was specific to super calls, so you could be certain when walking the call stack whether the method call was a super call or not. I think the yjit/zjit issue could potentially be addressed by recognizing the module refinement case and not dispatching to BasicObject directly.

Do you think the VM frame flag for super calls, and fixing yjit/zjit to handle module refinements, would be sufficient? Or should I work on the lazily created refinement iclass approach?

[shugo (Shugo Maeda)]

jeremyevans0 (Jeremy Evans) wrote in #note-2:

Do you think the VM frame flag for super calls, and fixing yjit/zjit to handle module refinements, would be sufficient?

I think a frame flag would fix the example I gave, but I do not see how it could handle the case where the block containing super outlives the refinement method's frame, because then there is no frame to walk or to flag. On the PR branch:

module M
  def m = [:M, *super]
end
module R
  refine M do
    def m = Thread.new { [:R, *super] }.value
  end
end
using R

class Base
  prepend M
  def m = [:Base]
end
class C < Base
  prepend M
  def m = [:C, *super]
end

C.new.m
# expected: [:R, :M, :C, :R, :M, :Base]
# actual:   never returns

On the thread's stack there are only the block frame and the frame of M#m, so the walk finds nothing, falls back to the outer M, re-enters C#m, which reaches the inner refinement again and starts another thread, and so on. A lambda returned from the refinement method and called later has the same problem (it returns a wrong value instead of looping). Fiber and external enumerators would behave like the thread.

[jeremyevans0 (Jeremy Evans)]

shugo (Shugo Maeda) wrote in #note-3:

jeremyevans0 (Jeremy Evans) wrote in #note-2:

Do you think the VM frame flag for super calls, and fixing yjit/zjit to handle module refinements, would be sufficient?

I think a frame flag would fix the example I gave, but I do not see how it could handle the case where the block containing super outlives the refinement method's frame, because then there is no frame to walk or to flag. On the PR branch:

module M
  def m = [:M, *super]
end
module R
  refine M do
    def m = Thread.new { [:R, *super] }.value
  end
end
using R

class Base
  prepend M
  def m = [:Base]
end
class C < Base
  prepend M
  def m = [:C, *super]
end

C.new.m
# expected: [:R, :M, :C, :R, :M, :Base]
# actual:   never returns

On the thread's stack there are only the block frame and the frame of M#m, so the walk finds nothing, falls back to the outer M, re-enters C#m, which reaches the inner refinement again and starts another thread, and so on. A lambda returned from the refinement method and called later has the same problem (it returns a wrong value instead of looping). Fiber and external enumerators would behave like the thread.

Good point. That does make the stack walking approach unworkable. So I'll work on implementing the lazily created refinement iclass approach instead.

[jeremyevans0 (Jeremy Evans)]

@shugo (Shugo Maeda) Thank you for the recommendation for implementing this correctly. I used the approach you recommended, and it ended up being much simpler than I expected, even if there were a couple tricky parts. One of those was how to store the lazily initialized caches. My first approach used an instance variable on an iclass, which apparently is no longer supported due to an optimization involving shapes. In order to avoid increasing the size of rb_classext_struct, I abused the classpath member to hold the cache, as that member is used for classes and modules but unused for iclasses. I'd like to clean this up later and move classpath into the as union, so that the iclass member can use a descriptive name.

The new approach passes the test suite I built while developing the previous approach. I also added a test case using Thread.new{super} (based on your earlier comment), which failed with the previous approach and passes with the new approach.

Here's the new pull request: https://github.com/ruby/ruby/pull/18910