Refinements and super

[ssnickolay]

Closes #2036

We have two problems that this PR should fix.

1. Incomplete DeclarationContext in lookupSuperMethod

Currently, we see two types of refinements:

1. Active when the method was defined

module Test
  using M
  
  def foo
    # currentMethod.getDeclarationContext()
    # returns a context with M
    super
  end
end

2. The current R:

module M
  refine C do
    def foo
      # currentMethod.getDeclarationContext()
      # returns a context with M
      super
    end
  end
end

It works with "flat" Rs, but if we use include:

class C 
  def foo
    "should return this one"
  end
end

module A
 def foo
    # currentMethod.getDeclarationContext()
    # returns a context without any refinements
    super
  end
end

module M
  refine C do
    include A
  end
end

using M

C.new.foo
# (master TR) => NoMethodError

Note: we cannot solve it just forcibly looking at objectMetaClass (aka C), because we must consider other extensions (aka included modules) of the active refinements. (see test "looks in the current active refinement from included modules")

So somehow active M refinement should be available in A#foo.

Solution

The first idea was to change the last place when we have access to the original callerFrame.

RubyArguments.pack extracts DeclarationContext from the method, but we could pass it explicitly.

The problem is in this case we will "activate" more then we should:

module A
  def foo
    # currentMethod.getDeclarationContext()
    # returns a context without any refinements and it is correct
    bar
  end
end

class C; end

module M
  refine C do
    include A

    def bar
      "you don't see me"
    end
  end
end

using M

C.new.foo
# (MRI) => undefined local variable or method `bar'

In other words, super can see active refinements of the current class, but other (standard) methods should not.

Thus, I changed LookupSuperMethodNode to pass active refinements about selfMetaClass (aka C) to the lookup method if exist.

1. Crazy super + refine (https://bugs.ruby-lang.org/issues/16977)

As we discussed on DM, super + refinements have not strict specifications.

using M # where R.ancestors = R, A, B, C, Object, ...
using M1 # where R1.ancestors = R1, D, E, C, Object, ...

# If super is called from the method contained in R table methods
#    - we should ignore all other active refinements and search only in the current one:
declaringModule = R # when def foo; super end is placed
objectMetaClass = C

Serach order: [skip all before declaringModule] -> A -> B -> C -> Object -> ...

# If super is called from method included to R modules
#    - we should go through all active refinements first, and then to C:
declaringModule = D # when def foo; super end is placed
objectMetaClass = C

Serach order: [skip all before declaringModule] -> E -> R -> A -> B -> C -> Object -> ...

Since we are using a recursive, we need to return the variable foundDeclaringModule from the nested calls. To do it, I added SuperMethodLookup helper wrapper.

I'm going to add more questions to the PR by place instead of listing them in the description.

[eregon]

The TCK tests seem to fail in CI, you can reproduce with jt test tck.

[eregon]

Taking the simple example:

class C
  def foo
    [:C]
  end
end

module A
  def foo
    [:A] + super
  end
end

refinement = Module.new do
  refine C do
    include A
  end
end

using refinement
p C.new.foo # => [:A, :C]

Real-world usage in https://github.com/ruby-next/ruby-next/blob/4945bc6ca1c976c3a3873f134168e4b6a87d71df/spec/test_unit_to_mspec.rb#L63

Thank you for the call, I think that clarified quite a few things for me.
As discussed, I think we should try a different approach.
The approach in this PR seems overall fine, but:

• It's expensive to get the caller frame for every call to a method with super in it. At least only methods with a super will trigger that, so that limits it quite a bit. Could potentially pass a separate DeclarationContext for super lookup in CachedDispatchNode#call, but:
• My understanding (might be wrong) is refinements are purely lexical, so we shouldn't need to pass which refinements are active when calling .foo (so that foo can know which super method to call), but rather on include A inside refine C we could copy the methods of A (and A's ancestors) and give them C => R as active refinements (as part of the InternalMethod DeclarationContext), as if def foo was inside refine C without the include.

As we both found out modules included in refinements are not well defined together with super:

• https://bugs.ruby-lang.org/issues/16977 (mentioned above, shows inconsistency when using include vs inlined)
• https://bugs.ruby-lang.org/issues/17007 (new, SystemStackError)

So I'd like to focus on getting reasonable cases first, like the one mentioned above, and not necessarily reproduce potential bugs in CRuby.

[eregon]

The new approach looks great, congrats for finding it!

The PR looks almost ready, mostly polishing left.

[eregon]

FYI I ran benchmarks on the old approach cb1a27c "tmp: keep parent declaration context in RubyArguments" (master...cb1a27c) and I can observe a slowdown on binary-trees and red-black of 8-9%, they don't use super but they do lots of recursive calls so the extra argument probably causes that slowdown.

[ssnickolay]

FYI I ran benchmarks on the old approach cb1a27c "tmp: keep parent declaration context in RubyArguments" (master...cb1a27c) and I can observe a slowdown on binary-trees and red-black of 8-9%, they don't use super but they do lots of recursive calls so the extra argument probably causes that slowdown.

Thank you for sharing with me! I'm surprised that the storage of just one additional argument has such a big impact. Glad we found the better solution

[eregon]

It looks perfect now, thank you for trying many different approaches until we found the right one, and exploring all kinds of edge cases and adding specs for it!

Congrats on finding the new approach, which is elegant, efficient and compatible.