Bug #18751: Regression on master for Method#== when comparing public with private method

• Status: Closed
• Assignee: Eregon (Benoit Daloze)
• Target version: -
• ruby -v: ruby 3.2.0dev (2022-04-23T02:59:20Z master e142bea799) [x86_64-linux]
• Backport: 2.7: DONTNEED, 3.0: DONTNEED, 3.1: DONTNEED

[Eregon (Benoit Daloze)]

This script repros:

class C
  class << self
    alias_method :n, :new
    private :new
  end
end

p C.method(:n) == C.method(:new) # => true

puts
p C.method(:n) == Class.method(:new) # => false
p C.method(:n) == Class.method(:new).unbind.bind(C) # => true

puts
p C.method(:new) == Class.method(:new) # => false
p C.method(:new) == Class.method(:new).unbind.bind(C) # => true, BUT false on master
p C.method(:new) == Class.instance_method(:new).bind(C) # => true, BUT false on master
p [C.method(:new), Class.instance_method(:new).bind(C)] # => [#<Method: #<Class:C>(Class)#new(*)>, #<Method: #<Class:C>(Class)#new(*)>]

So this prints the expected results on 2.7.5, 3.0.3, 3.1.1 but not on master, which seems a regression.
Notably this breaks the pattern discussed in https://bugs.ruby-lang.org/issues/18729#note-5, and it means there is no way to find out if two methods share the same "definition/logic/def", which is a big limitation.

[Eregon (Benoit Daloze)]

• Backport changed from 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN to 2.7: DONTNEED, 3.0: DONTNEED, 3.1: DONTNEED

[Eregon (Benoit Daloze)]

• Description updated (diff)

[Eregon (Benoit Daloze)]

• Related to Bug #18729: Method#owner and UnboundMethod#owner are incorrect after using Module#public/protected/private added

[jeremyevans0 (Jeremy Evans)]

This isn't a regression, this was a change I made deliberately in the fix to #18435. It's listed in the commit message:

Consider Method/UnboundMethod objects different if they have
different visibilities.

I neglected to update the documentation for Method#== to state that visibility is now also considered.

I don't have strong feelings about reverting the Method#== change. It seems odd that two methods with different visibilities would be considered equal, but if doing so introduces a backwards compatibility issue in production code, maybe we shouldn't do that. It would be good to add this as a developer meeting topic.

[Eregon (Benoit Daloze)]

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

This isn't a regression

In my POV (as a user of Method/UnboundMethod#==) it's a regression, but I understand you have a different POV.

It seems odd that two methods with different visibilities would be considered equal,

I understand, but then if we change this for 3.2 we also need a new method on Method/UnboundMethod to know if they refer to the same "method definition" i.e., the same code/the same piece of bytecode/the same C function/etc.
Without that, I consider that a regression because there is no way to find that out AFAIK.

I think the primary usage of Method/UnboundMethod#== is to compare "are they the same method definition?".

And not so much for whether two methods should be considered the same key in a Hash.
eql? could potentially be more strict (and consider visibility too), and that would be somewhat similar to the situation for Numeric, but not sure it's a good idea to have #== and #eql? differ for Method/UnboundMethod.

but if doing so introduces a backwards compatibility issue in production code, maybe we shouldn't do that.

It kind of does, it breaks latest RSpec tests when run on ruby-head: https://github.com/rspec/rspec-mocks/runs/6128536621?check_suite_focus=true#step:8:1561
Mocking of new/initialize with RSpec likely doesn't work in real code due to this issue, if anyone has a private new, which doesn't seem unlikely (e.g., that makes sense for a singleton pattern class, or a class which want to cache instances, etc).
Since #18729 is accepted (and that was clearly inconsistent so a needed fix), the old owner check is not usable anymore, and with this issue the better (because it actually checks it's the same definition) Method#== check also does not work.

It would be good to add this as a developer meeting topic.

OK, I will add, but IMHO this is clear we need to fix it, the earlier the better.

[Eregon (Benoit Daloze)]

An advantage of a new method would be it could also ignore the receiver for Method and the source module for UnboundMethod (e.g., the module on which instance_method was called), so it would really only compare the actual definition of the method and nothing else, without needing extra bind/unbind.

[mame (Yusuke Endoh)]

Let me confirm the current situation:

• Both #18435 and #18729 focus on the same issue (an inconsistency due to the fact that a Method object skips ZSUPER method entry)
• In #18435, the visibility information is now stored in a Method object to hide the inconsistency
• In #18729, we determined to allow a Method object for ZSUPER method entry to fix the inconsistency fundamentally
• In this ticket, Method#== has an incompatibility isue because it respects method visibility information stored in a Method object.

Right?

Now, I wonder if it is really needed to store the visibility information in a Method object. Will just reverting 58dc8bf8f15df9a33d191074e8a5d4946a3d59d5 solve this issue?

[ioquatix (Samuel Williams)]

It's okay for == to not be strong equality and be closer to equivalence. That's why we have different methods for "This is the identical thing" or "This is comparably equivalent". I always felt like eql? was better for "These are identical" vs == which is "These are equivalent or represent the same thing".

[Eregon (Benoit Daloze)]

@mame (Yusuke Endoh) I'll let @jeremyevans0 (Jeremy Evans) reply since he knows better about the implementation details.
Your summary seems overall accurate to me.

[jeremyevans0 (Jeremy Evans)]

mame (Yusuke Endoh) wrote in #note-7:

Let me confirm the current situation:

• Both #18435 and #18729 focus on the same issue (an inconsistency due to the fact that a Method object skips ZSUPER method entry)
• In #18435, the visibility information is now stored in a Method object to hide the inconsistency
• In #18729, we determined to allow a Method object for ZSUPER method entry to fix the inconsistency fundamentally
• In this ticket, Method#== has an incompatibility isue because it respects method visibility information stored in a Method object.

Right?

I think that is a good summary.

Now, I wonder if it is really needed to store the visibility information in a Method object. Will just reverting 58dc8bf8f15df9a33d191074e8a5d4946a3d59d5 solve this issue?

I don't think reverting that commit will fix the issue. The rb_method_entry_t* in the Method object still points to the original method, not the ZSUPER method. So if you reverted the commit, the visibility information would be wrong.

If you want to revert 58dc8bf8f15df9a33d191074e8a5d4946a3d59d5, you would have to make it so the rb_method_entry_t* points to the ZSUPER method. I'm not against that approach, but I don't understand the code well enough to know whether it will cause problems. Lacking a detailed understanding of why the code is the way it is, I made the assumption that there is a reason the rb_method_entry_t* points to the original method, and I took the conservative approach of just adding visibility information without changing other internals.

[mame (Yusuke Endoh)]

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

you would have to make it so the rb_method_entry_t* points to the ZSUPER method.

Yes. I meant it by "we determined to allow a Method object for ZSUPER method entry".

Lacking a detailed understanding of why the code is the way it is, I made the assumption that there is a reason the rb_method_entry_t* points to the original method,

I think the original author (matz? nobu? Or ko1? I don't know) wanted to hide ZSUPER method entry from users because it is an implementation detail. However, #18729 showed that the hiding was incomplete. At the previous dev meeting, we discussed whether the hiding is really needed, and agreed to stop hiding it (https://bugs.ruby-lang.org/issues/18729#note-6).

and I took the conservative approach of just adding visibility information without changing other internals.

You are very thoughtful and wonderful! Thank you always.

[matz (Yukihiro Matsumoto)]

• Related to Bug #18435: Calling `protected` on ancestor method changes result of `instance_methods(false)` added

[Eregon (Benoit Daloze)]

I still think we should stop hiding ZSUPER method entries, it causes way more confusion than it helps and this bug, and I believe it would cause very little incompatibility.
@jeremyevans0 (Jeremy Evans) or @mame (Yusuke Endoh) Would you be interested to work on that change? Otherwise I'll give it a try when I have some time.

[Eregon (Benoit Daloze)]

Here is a simplified script which does not depend on Class.new == Class#new:

class C
  class << self
    alias_method :n, :new
    private :new
  end
end

p C.method(:n) == C.method(:new) # => true

puts
p C.method(:n) == Class.method(:new) # => false
p C.method(:n) == Class.instance_method(:new).bind(C) # => true

puts
p C.method(:new) == Class.method(:new) # => false
p C.method(:new) == Class.instance_method(:new).bind(C) # => true, BUT false on master
p [C.method(:new), Class.instance_method(:new).bind(C)] # => [#<Method: #<Class:C>(Class)#new(*)>, #<Method: #<Class:C>(Class)#new(*)>]

Based on this PR to fix this by no longer hiding ZSUPER methods: https://github.com/ruby/ruby/pull/6242,
I think we should add a new method on Method and UnboundMethod to check if they have the same definition.
Because otherwise naturally a method with a different visibility is a separate method, and making them the same with == seems a bit wrong (but IMHO less wrong than hiding ZSUPER methods).

Maybe simply {Method,UnboundMethod}#same_definition?(other).
I would also allow either Method or UnboundMethod as the argument, so there is no need to bind/unbind to compare.

[Eregon (Benoit Daloze)]

https://github.com/ruby/ruby/pull/6242 now makes resolved-through-zsuper methods equal for compatibility.
That means this issue is fixed by that PR too.

It's maybe a little bit strange that they are equal, but until we have a new method such as {Method,UnboundMethod}#same_definition?(other) this seems best for compatibility and generally honors the same behavior as previous versions which resolved ZSUPER methods in Kernel#method/Module#instance_method and now in ==.

In fact the docs of Method#== do hint at definition equality:

Two method objects are equal if they are bound to the same object and
refer to the same method definition and the classes defining the methods
are the same class or module.

If ZSUPER methods are ever removed, this can all be simplified.

[Eregon (Benoit Daloze)]

We discussed this at the dev meeting.
@ko1 (Koichi Sasada) said Method#== should be "is it the same definition?".
I agree, I'll take a look at this with #18729 and #18435.

[Eregon (Benoit Daloze)]

• Status changed from Open to Closed
• Assignee set to Eregon (Benoit Daloze)

Fixed in https://github.com/ruby/ruby/pull/6242

[Eregon (Benoit Daloze)]

Eregon (Benoit Daloze) wrote in #note-16:

@ko1 (Koichi Sasada) said Method#== should be "is it the same definition?".
I agree, I'll take a look at this with #18729 and #18435.

I did this for zsuper methods (resolve before comparing them for compatibility).
But not for other cases, I'll opened a separate issue for that: #18969

[Eregon (Benoit Daloze)]

• Related to Feature #18969: Compare only method definitions for Method#== and UnboundMethod#== added

[Eregon (Benoit Daloze)]

• Related to Feature #11689: Add methods allow us to get visibility from Method and UnboundMethod object. added

[matz (Yukihiro Matsumoto)]

• Related to Feature #18798: `UnboundMethod#==` with inherited classes added

[matz (Yukihiro Matsumoto)]

This should be fixed by #18798 which is accepted.

Matz.