Project

General

Profile

Actions

Bug #18751

closed

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

Added by Eregon (Benoit Daloze) almost 2 years ago. Updated over 1 year ago.

Status:
Closed
Target version:
-
ruby -v:
ruby 3.2.0dev (2022-04-23T02:59:20Z master e142bea799) [x86_64-linux]
[ruby-core:108378]

Description

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.


Related issues 5 (1 open4 closed)

Related to Ruby master - Bug #18729: Method#owner and UnboundMethod#owner are incorrect after using Module#public/protected/privateClosedEregon (Benoit Daloze)Actions
Related to Ruby master - Bug #18435: Calling `protected` on ancestor method changes result of `instance_methods(false)`ClosedEregon (Benoit Daloze)Actions
Related to Ruby master - Feature #18969: Compare only method definitions for Method#== and UnboundMethod#==ClosedActions
Related to Ruby master - Feature #11689: Add methods allow us to get visibility from Method and UnboundMethod object.OpenActions
Related to Ruby master - Feature #18798: `UnboundMethod#==` with inherited classesClosedActions
Actions #1

Updated by Eregon (Benoit Daloze) almost 2 years ago

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

Updated by Eregon (Benoit Daloze) almost 2 years ago

  • Description updated (diff)
Actions #3

Updated by Eregon (Benoit Daloze) almost 2 years ago

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

Updated by jeremyevans0 (Jeremy Evans) almost 2 years ago

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.

Updated by Eregon (Benoit Daloze) almost 2 years ago

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.

Updated by Eregon (Benoit Daloze) almost 2 years ago

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.

Updated by mame (Yusuke Endoh) almost 2 years ago

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?

Updated by ioquatix (Samuel Williams) almost 2 years ago

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".

Updated by Eregon (Benoit Daloze) almost 2 years ago

@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.

Updated by jeremyevans0 (Jeremy Evans) almost 2 years ago

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.

Updated by mame (Yusuke Endoh) almost 2 years ago

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.

Actions #12

Updated by matz (Yukihiro Matsumoto) almost 2 years ago

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

Updated by Eregon (Benoit Daloze) over 1 year ago

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.

Updated by Eregon (Benoit Daloze) over 1 year ago

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.

Updated by Eregon (Benoit Daloze) over 1 year ago

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.

Updated by Eregon (Benoit Daloze) over 1 year ago

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.

Updated by Eregon (Benoit Daloze) over 1 year ago

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

Updated by Eregon (Benoit Daloze) over 1 year ago

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

Actions #19

Updated by Eregon (Benoit Daloze) over 1 year ago

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

Updated by Eregon (Benoit Daloze) over 1 year ago

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

Updated by matz (Yukihiro Matsumoto) over 1 year ago

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

Updated by matz (Yukihiro Matsumoto) over 1 year ago

This should be fixed by #18798 which is accepted.

Matz.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0