Do not hide ZSUPER methods, fix consistency of owner and instance_methods - #6242

I'll review if matz approves the behavior change at the developer meeting.

@matz agreed during the meeting to remove ZSUPER methods, i.e., public/protected/private behave like alias or like define_method(:alias, instance_method(:original)).
That goes further than this PR and makes everything simpler.
So I close this and I'll make a PR to remove ZSUPER methods :)

Replaced by #6251

No, it's opposite. I agree with this request, and disagree with #6251.

I disagree to making copy, to keep symbolic semantics, i.e., public is just declaring visibility of a method public, not keeping the definition at the moment. If you replaced (or removed) the private method of superclass, the public one should also be affected.

@matz Oh, I thought during the meeting you said you were OK with copy-semantics #6251 and the semantics of the example shown as part of the meeting log, I even tried to confirm during the meeting to be sure I understood correctly, but apparently I must have misunderstood.

IMHO #6251 is much cleaner and unifies the behavior with TruffleRuby and JRuby.
It has several additional advantages over this PR:

• Method/UnboundMethod always points to a given method definition which never changes with Make public/protected/private copy the method instead of creating a method entry using super #6251 (was the case before except for ZSUPER methods). I think Ruby users expect a Method/UnboundMethod is a handle to a given method definition, but it's not for ZSUPER methods. I think nobody wants that behavior of e.g. Method#call dynamically calling different method definitions for the same Method object (happens only for ZSUPER methods). Method/UnboundMethod are expected to capture/snapshot in Ruby, isn't it?
• Much simpler implementation
• Faster implementation, because there is no need for loops to deref ZSUPER methods
• Compatible with the current TruffleRuby and JRuby behavior. TruffleRuby is unlikely to implement ZSUPER methods because lots of complexity for apparently no incompatibility in practice (no bug report to TruffleRuby and JRuby about this AFAIK).

@matz Could you confirm you want this PR and not #6251 given these trade-offs?

An example in Ruby code:

class P
  private def foo
    1
  end
end

class C < P
  public :foo
end

m = C.new.method(:foo)
p m
p m.call

class P
  alias old_foo foo # silence warning
  private def foo
    2
  end
end

p m
p m.call

On 3.0.3, TruffleRuby, JRuby and #6251:

ruby 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [x86_64-linux]
#<Method: C(P)#foo() zsuper_method.rb:2>
1
#<Method: C(P)#foo() zsuper_method.rb:2>
1

On this PR:

ruby 3.2.0dev (2022-08-15T14:20:15Z do_not_hide_zsuper.. 2cbf520bb1) [x86_64-linux]
#<Method: C(P)#foo() zsuper_method.rb:2>
1
#<Method: C(P)#foo() zsuper_method.rb:17>   # line changes
2   # result changes

So this an unfortunate side effect, and I think unsolvable (except by removing ZSUPER methods).
(I don't expect anyone does that and redefines in superclass after public, so in practice does not matter much, but it's part of the general semantics)
It only "worked" on 3.0.3 because the lookup in Kernel#method/Module#instance_method skips ZSUPER, which does not accurately represent C#foo but is really just P#foo (v1) with "fetched on class C, shown in inspect".

The semantics of #6251 are clear and already proven (it's the same semantics as alias_method).
The semantics of this PR is still better than current releases (it fixes the consistency between owner and instance_methods), but is not nearly as clean and well-defined as #6251.

Sorry for miscommunication. See the following example:

class C
  private def foo(); "C" end
end
class D<C
  public :foo 
end
m = D.instance_method(:foo)
class C
  def foo(); "D" end
end
d=D.new
d.foo                #=> should be "D"; "C" is *unacceptable*
m.bind_call(d)       #=> "D" or "C"; negotiable; "C" is better (?)
m.owner              #=> C or D; D is acceptable
m2 = D.instance_method(:foo)
m2.bind_call(d)      #=> should be "D"

Making copy by public/private/protected (by CRuby) is unacceptable, because it ignores the method redefinition afterwards. That's the difference from alias (it was intentionally designed to keep the definition at the moment).

In other words, if it could follow the latest definition, it's OK to copy internally. It's implementation detail.

And I admit that it's a very slight edge case, so the other implementations may choose to behave differently as implementation dependent. I am not sure how desirable it is, considering the effort that have already put in the JRuby/TruffleRuby to keep compatibility with CRuby.

The semantics of #6251 are clear and already proven (it's the same semantics as alias_method).
The semantics of this PR is still better than current releases (it fixes the consistency between owner and instance_methods), but is not nearly as clean and well-defined as #6251.

As I said above, "the same semantics as alias_method" is unacceptable. Maybe we just abandon this PR as well, and just make Method#owner point to the class that made the method public.

Thank you for clarifying.
Then I think we should merge this PR, @jeremyevans and @XrXr, could you review it?

and just make Method#owner point to the class that made the method public.

I think that's very hard (to implement and to reason about) without allowing #method/#instance_method to return a ZSUPER method entry.
Specifically the current code on master uses the super method entry and only sets klass in the Method object, which leads to various inconsistencies and surprising behavior (notably it cannot set easily the correct Method#owner, and it caused issues for Method#private?/protected?/public?):

So I think this PR is as close as it gets to fixing Method#owner with minimal changes and still preserve compatibility for {Method,UnboundMethod}#== to compare methods.

Results of the script above:

So this PR fits your semantics requirements, and fixes owner (and the consistency with instance_methods) + makes it simpler what Kernel#method/Module#instance_method returns, i.e. the first method in the ancestry chain just like actual method lookup used for regular calls (recv.foo).

I also realized, with this PR VM_METHOD_TYPE_ZSUPER is less special than before (good!) and is mostly "a method entry whose body is (z)super" and whose "source method" information is resolved dynamically as it's not copied on the zsuper method entry itself to stay dynamic.
Notably it's not treated specially anymore in #method/#instance_method and one can actually get a Method/UnboundMethod for such a method created by public/protected/private which is very nice/intuitive/simple and more consistent with other method types and with regular method lookup.

I've cleaned up the PR, the diff should be easier to review now as it's quite a bit smaller/clearer.
I now resolve ZSUPER methods through rb_method_entry_t directly (in zsuper_resolve) instead of Method/UnboundMethod objects which should avoid some extra allocations and indirections.