Project

General

Profile

Actions

Bug #18729

closed

Method#owner and UnboundMethod#owner are incorrect after using Module#public/protected/private

Added by Eregon (Benoit Daloze) 8 months ago. Updated 2 months ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [x86_64-linux]
[ruby-core:108235]

Description

The #owner should be "the class or module that defines the method".
Or in other words, the owner is the module which has the method table containing that method.

This generally holds, and it seems very likely this assumption is relied upon (e.g., when decorating a method, undefining it, etc).
But the returned value on CRuby is incorrect for this case:

class A
  protected def foo
    :A
  end
end

class B < A
  p [instance_method(:foo), instance_method(:foo).owner, instance_methods(false), A.instance_methods(false)]
  public :foo
  p [instance_method(:foo), instance_method(:foo).owner, instance_methods(false), A.instance_methods(false)]
end

It gives:

[#<UnboundMethod: B(A)#foo() owner.rb:2>, A, [], [:foo]]
[#<UnboundMethod: B(A)#foo() owner.rb:2>, A, [:foo], [:foo]]

So UnboundMethod#owner says A, but clearly there is a :foo method entry in B created by public :foo, and that is shown through B.instance_methods(false).

The expected output is:

[#<UnboundMethod: B(A)#foo() owner.rb:2>, A, [], [:foo]]
[#<UnboundMethod: B#foo() owner.rb:2>, B, [:foo], [:foo]]

Related issues 3 (1 open2 closed)

Related to Ruby master - Bug #18751: Regression on master for Method#== when comparing public with private methodClosedEregon (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 #11689: Add methods allow us to get visibility from Method and UnboundMethod object.OpenActions

Updated by Eregon (Benoit Daloze) 8 months ago

FWIW, I've found 2 CRuby tests fail with this change (through implementing #owner correctly on TruffleRuby):

TestMethod#test_prepended_public_zsuper
TestMethod#test_zsuper_private_override_instance_method

They are trivial to adapt to the change.

Updated by jeremyevans0 (Jeremy Evans) 8 months ago

I'm not sure if this is a bug, since the current behavior seems intentional. However, I do agree it would be best to change it. In addition to modifying #owner/#inspect, we also need to fix #super_method so it returns the correct object. I submitted a pull request that implements this change: https://github.com/ruby/ruby/pull/5802

Updated by Eregon (Benoit Daloze) 8 months ago

Thank you, that looks great.
And as mentioned in the PR it also fixes Method#super_method to actually be the correct one in at least one case.

I'm not sure if this is a bug, since the current behavior seems intentional.

Why do you think it might be intentional?
It sounds like a clear implementation bug to me (which I guess wasn't found until now, or maybe not deemed worth fixing if found by others).

Updated by jeremyevans0 (Jeremy Evans) 8 months ago

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

I'm not sure if this is a bug, since the current behavior seems intentional.

Why do you think it might be intentional?
It sounds like a clear implementation bug to me (which I guess wasn't found until now, or maybe not deemed worth fixing if found by others).

The code deliberates handles ZSUPER methods by looking for the method entry that actually handles the method, instead of using the ZSUPER method entry. There is no way this is an accidental bug, it's a deliberate implementation choice.

Note that my pull request doesn't change the underlying implementation, it still uses the method entry that actually handles the method. My pull request only records the necessary metadata so that owner/inspect/super_method return results as if the ZSUPER method entry was used.

Updated by Eregon (Benoit Daloze) 8 months ago

This RSpec test:
https://github.com/rspec/rspec-mocks/blob/1611752449b5beefa1939dd2a20beb0175d46110/spec/rspec/mocks/partial_double_spec.rb#L620
currently relies on the inconsistent behavior of owner, here:
https://github.com/rspec/rspec-mocks/blob/1611752449b5beefa1939dd2a20beb0175d46110/lib/rspec/mocks/method_reference.rb#L192

The code wants to find out if new on a given class is the same definition as Class#new.
The way used there with owner doesn't work through aliases, which already correctly sets the owner.

There is currently no easy way to know if 2 Method or UnboundMethod "are the same code/definition".
That's because Method#== and UnboundMethod#== both compare more than just the definition, but also the receiver (for Method) or where the method was fetched from (for UnboundMethod): https://github.com/ruby/ruby/blob/v3_0_2/proc.c#L1772-L1780
I think ideally that should change, or we should have a separate method to compare 2 Method/UnboundMethod to tell if they have the same definition.

Exposing the defined_class (where the method definition was first used) as a new method wouldn't really help, because the method might have been redefined in between.

However I found a way using unbind + bind:

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

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

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

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

So that's what RSpec should use, I'll make a PR: https://github.com/rspec/rspec-mocks/pull/1470

But, this doesn't work on master currently #18751 :/

Updated by mame (Yusuke Endoh) 8 months ago

@ko1 (Koichi Sasada) proposed the following, and @matz (Yukihiro Matsumoto) approved this at the dev meeting.

class A
  private def foo
  end
end

class B < A
  public :foo
end

m = B.instance_method(:foo)

p m.owner                   #=> B                                (changed)
p m.public?                 #=> true                             (changed)
p m.source_location         #=> location of `def foo` in class A (no change)
p m.super_method            #=> UnboundMethod: A#foo() ...>      (changed)
p B.instance_methods(false) #=> [:foo]                           (no change)

(Just for record: I wanted m.source_location to return the line of public :foo but @ko1 (Koichi Sasada) rejected it for implementation reasons :-)

Updated by Eregon (Benoit Daloze) 8 months ago

That sounds good, I think it is exactly what I proposed but with more details.
Agreed source_location shouldn't change, that should remain the location of the original definition.

Updated by jeremyevans0 (Jeremy Evans) 8 months ago

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

@ko1 (Koichi Sasada) proposed the following, and @matz (Yukihiro Matsumoto) approved this at the dev meeting.

class A
  private def foo
  end
end

class B < A
  public :foo
end

m = B.instance_method(:foo)

p m.owner                   #=> B                                (changed)
p m.public?                 #=> true                             (changed)
p m.source_location         #=> location of `def foo` in class A (no change)
p m.super_method            #=> UnboundMethod: A#foo() ...>      (changed)
p B.instance_methods(false) #=> [:foo]                           (no change)

(Just for record: I wanted m.source_location to return the line of public :foo but @ko1 (Koichi Sasada) rejected it for implementation reasons :-)

I tested my pull request and it returns the values that @matz (Yukihiro Matsumoto) approved. So it just needs to be reviewed to make sure the implementation is acceptable: https://github.com/ruby/ruby/pull/5802

Updated by Eregon (Benoit Daloze) 8 months ago

On the PR, there is a discussion what should be the return value of super_method for a more complex example:

class C
  def foo
    puts caller(0), nil
    ['C']
  end
end

module Mod
  private def foo
    ['.'] + super
  end
end

obj = C.new
obj.extend(Mod)

class << obj
  public :foo
end

p obj.foo # => [".", "C"]

p obj.singleton_class.ancestors # => [sclass, Mod, C, Object, Kernel, BasicObject]
p obj.method(:foo).owner # => should be sclass, NOT Mod
p obj.method(:foo).super_method # => should be C#foo, NOT Mod#foo

So (public) sclass#foo actually super's into C#foo (because only 1 dot printed), and never goes into private Mod#foo.
Hence Method#super_method should reflect that as much as possible.
But the PR instead changes the last result to Mod#foo, which seems incorrect.

Updated by Eregon (Benoit Daloze) 8 months ago

Actually in the simpler example from #6 we have the same inconsistency:

p m.super_method            #=> UnboundMethod: A#foo() ...>      (changed)

It should be nil. Because if def foo; super; end was used, that would be a NoMethodError:

class A
  private def foo
    super
  end
end

class B < A
  public :foo
end

p B.instance_method(:foo).super_method # => nil currently, as it should remain
p B.new.foo

gives:

nil
-:3:in `foo': super: no superclass method `foo' for #<B:0x0000000000bdd058> (NoMethodError)
	from -:12:in `<main>'

(A#foo is never called, only B#foo, I'm using owner#name notation here)

Actions #11

Updated by Eregon (Benoit Daloze) 8 months ago

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

Updated by jeremyevans0 (Jeremy Evans) 7 months ago

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

Actually in the simpler example from #6 we have the same inconsistency:

p m.super_method            #=> UnboundMethod: A#foo() ...>      (changed)

It should be nil. Because if def foo; super; end was used, that would be a NoMethodError:

class A
  private def foo
    super
  end
end

class B < A
  public :foo
end

p B.instance_method(:foo).super_method # => nil currently, as it should remain
p B.new.foo

gives:

nil
-:3:in `foo': super: no superclass method `foo' for #<B:0x0000000000bdd058> (NoMethodError)
	from -:12:in `<main>'

(A#foo is never called, only B#foo, I'm using owner#name notation here)

I disagree. The ZSUPER method type is basically an optimization of an explicit method call that uses super.

If you want to treat B#foo as it's own method distinct from A#foo, modulo location information, you should treat:

class B < A
  public :foo
end

as:

class B < A
  public def foo; super; end
end

In doing so, it seems that B.instance_method(:foo).super_method should equal A.instance_method(:foo). Your more complex example in https://bugs.ruby-lang.org/issues/18729#note-9 is basically the same issue. So I plan to merge https://github.com/ruby/ruby/pull/5802 after receiving an approval for it, since it implements the behavior approved by @matz (Yukihiro Matsumoto).

Updated by Eregon (Benoit Daloze) 7 months ago

@jeremyevans0 (Jeremy Evans) Do you agree Method#super_method should show the method called by super if there is super in that method, when it's possible to know?
(I think it's always possible for a method defined on a class, but not for a method defined on a module especially if the module is included multiple times in the ancestors)

If so, then I believe my example in https://bugs.ruby-lang.org/issues/18729#note-9 shows that Mod#foo (using owner#name notation) is never called, the only called methods are sclass#foo and C#foo, so sclass#foo super's into C#foo.
And so sclass#foo.super_method should be C#foo, since that's what the super in sclass#foo calls.

Or did I miss something?

public is not equivalent to redeclaring it like you show with def foo; super; end, because B#foo would call A#foo with redeclaring, but with only public :foo in B A#foo is not called for B.new.foo.

Updated by jeremyevans0 (Jeremy Evans) 7 months ago

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

@jeremyevans0 (Jeremy Evans) Do you agree Method#super_method should show the method called by super if there is super in that method, when it's possible to know?
(I think it's always possible for a method defined on a class, but not for a method defined on a module especially if the module is included multiple times in the ancestors)

I think you either treat a ZSUPER method like a method in the current class (this issue), or you treat it as a method in the ancestor (Ruby 3.1 and below behavior). If you treat a ZSUPER method like a method in the current class, then super_method should be treated as if the ZSUPER method was defined in the current class (and not a ZSUPER method), in which case it gives the method defined in the ancestor.

public is not equivalent to redeclaring it like you show with def foo; super; end, because B#foo would call A#foo with redeclaring, but with only public :foo in B A#foo is not called for B.new.foo.

True, that is a difference. I believe in this case that super_method will still give you the correct answer, the Method object for A#foo before A#foo was redeclared. If that is the case, I don't the proposed behavior as a problem.

Updated by Eregon (Benoit Daloze) 7 months ago

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

I think you either treat a ZSUPER method like a method in the current class (this issue), or you treat it as a method in the ancestor (Ruby 3.1 and below behavior).

Yes, it should be like a method in the current class.
A ZSUPER method sounds like a CRuby implementation detail (TruffleRuby has no such concept AFAIK), and I think it should not leak to the user.

If you treat a ZSUPER method like a method in the current class, then super_method should be treated as if the ZSUPER method was defined in the current class (and not a ZSUPER method), in which case it gives the method defined in the ancestor.

That's were we disagree.
I believe super_method should return the method that will be called by super (that's the whole point of super_method).

In the example from https://bugs.ruby-lang.org/issues/18729#note-9,
sclass#foo (using owner#name notation) super's into C#foo, NOT/NEVER in Mod#foo.
So sclass#foo.super_method should be C#foo.

That said, I think this fix is worth on its own and IMHO it's fine to merge it and keep discussing the specifics of super_method after.

Actions #16

Updated by matz (Yukihiro Matsumoto) 6 months ago

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

Updated by mame (Yusuke Endoh) 6 months ago

  • Status changed from Open to Rejected

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

@ko1 (Koichi Sasada) proposed the following, and @matz (Yukihiro Matsumoto) approved this at the dev meeting.

class A
  private def foo
  end
end

class B < A
  public :foo
end

m = B.instance_method(:foo)

p m.owner                   #=> B                                (changed)
p m.public?                 #=> true                             (changed)
p m.source_location         #=> location of `def foo` in class A (no change)
p m.super_method            #=> UnboundMethod: A#foo() ...>      (changed)
p B.instance_methods(false) #=> [:foo]                           (no change)

@matz (Yukihiro Matsumoto) changed his mind: https://bugs.ruby-lang.org/issues/18435#note-11

Now he thinks the behavior of Ruby 3.1 is good, so everything above should be "no change". B.instance_methods(false) should include :foo, but B.instance_method(:foo) should return a method object that wraps A's foo method. So the original problem in this ticket ("Method#owner and UnboundMethod#owner are incorrect") is considered a spec.

Updated by Eregon (Benoit Daloze) 6 months ago

  • Status changed from Rejected to Open

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

@matz (Yukihiro Matsumoto) changed his mind: https://bugs.ruby-lang.org/issues/18435#note-11

So the original problem in this ticket ("Method#owner and UnboundMethod#owner are incorrect") is considered a spec.

I cannot agree that to possibly be spec, I would like @matz (Yukihiro Matsumoto) to reconsider and this time we just remove "ZSUPER methods" entirely to make it simple and intuitive (instead we shallow-copy like for alias_method).
The current CRuby behavior is very confusing and should be removed, it's bringing needless complexity and confusion.
See https://bugs.ruby-lang.org/issues/18435#note-12 for details.

Updated by matz (Yukihiro Matsumoto) 6 months ago

  • Status changed from Open to Rejected

The current method searching is symbolic (not copying at the time) except for aliases intentionally. For example, if you redefine a method in a module after a class included the module, the redefined method is called. Aliasing is an exception because one of its purposes is keeping the current definition of a method (remember alias-method-chain). Copying could be a choice, for example wren (https://github.com/wren-lang/wren) uses copying. But it's not consistent with the other part of Ruby. IMO, the following code should print "hi", not "hello", as currently CRuby does.

class A
  private def foo; puts "hello"; end
end
class B<A
  public :foo
end
class A
  private def foo; puts "hi"; end
end
B.new.foo # => "hi"

Matz.

Updated by matz (Yukihiro Matsumoto) 6 months ago

And if I understand correctly, copying could cause the following code to print "B:B:hello", which is confusing.

class A
  private def foo; puts "hello"; end
end
class B<A
  private def foo; print "B:"; super; end
end
class C<B
  public :foo
end
C.new.foo

Matz.

Updated by Eregon (Benoit Daloze) 6 months ago

@matz (Yukihiro Matsumoto) Right, this is the something you intended initially for public/protected/private.
I think most Rubyists do not understand this detail and the real practical issue is the inconsistencies shown by Method/UnboundMethod objects (e.g., not possible to have a correct/consistent Method object of a zsuper method currently).

One way to address that is https://bugs.ruby-lang.org/issues/18751#note-11 (no longer hide ZSUPER method entry from users).
That will also fix this issue, and the owner will be as it is expected.
Can we do that, as it was agreed in a previous meeting?

Then what public/protected/private do is mostly an implementation detail, and probably no or very little real code relies on public being symbolic or copying.

Updated by Eregon (Benoit Daloze) 6 months ago

matz (Yukihiro Matsumoto) wrote in #note-20:

And if I understand correctly, copying could cause the following code to print "B:B:hello", which is confusing.

That depends on the details of super lookup, which can notice it should go above where the method was originally defined, so C -> A, not C -> B -> A.
On current TruffleRuby it's "B:hello" (same as current CRuby), due to that.

Updated by matz (Yukihiro Matsumoto) 6 months ago

On current TruffleRuby it's "B:hello" (same as current CRuby), due to that.

That's nice. It is an illusion we want to present.

Matz.

Updated by matz (Yukihiro Matsumoto) 6 months ago

One way to address that is https://bugs.ruby-lang.org/issues/18751#note-11 (no longer hide ZSUPER method entry from users).

It is. And I once thought it was a good idea, but the change caused some incompatibility we don't want to see. That's the problem.
Currently, I have no idea to resolve all confusion/problems. So I try to be conservative to keep the current behavior until we get a better idea.

Matz.

Updated by Eregon (Benoit Daloze) 6 months ago

matz (Yukihiro Matsumoto) wrote in #note-24:

One way to address that is https://bugs.ruby-lang.org/issues/18751#note-11 (no longer hide ZSUPER method entry from users).

It is. And I once thought it was a good idea, but the change caused some incompatibility we don't want to see. That's the problem.

Could you point the incompatibility(ies)?
I think we never actually tried to no longer hide ZSUPER methods (it's not the same as https://bugs.ruby-lang.org/issues/18435#note-8, which is different, still hides ZSUPER methods and changes equality semantics).
My understanding is there would be no incompatibility with that change for this issue and the linked issues, it would actually solve all 3 of them.

Can we try no longer hiding ZSUPER methods, and assess the impact (e.g., commit it experimentally)?
I think that solves the consistency problem at least, and I think it should have a very low impact on compatibility if any (it might actually fix some existing bugs/corner cases in mocking libraries).

Updated by Eregon (Benoit Daloze) 4 months ago

  • Status changed from Rejected to Open

This is still an issue.
We need to try to no longer hide ZSUPER methods and estimate if there is any incompatibility in practice due to that.

Also JRuby and TruffleRuby don't use ZSUPER methods, so for them it only makes sense to report public/protected/private-copied methods in instance_methods (#18435) as and as owner.
For TruffleRuby at least there is no plan to add ZSUPER methods, because it seems to not matter at all in practice for compatibility and would increase complexity for no gain.

Updated by Eregon (Benoit Daloze) 4 months ago

Also as @ufuk (Ufuk Kayserilioglu) says in https://bugs.ruby-lang.org/issues/18435#note-2, owner and instance_methods should be consistent.
And they should reflect what is in the actual method table, not hiding some methods like ZSUPER methods which users don't know what they are, but they created a method on that module, hence it should be in the method table.

Updated by Eregon (Benoit Daloze) 4 months ago

PR to fix this by no longer hiding ZSUPER methods: https://github.com/ruby/ruby/pull/6242

Updated by Eregon (Benoit Daloze) 4 months ago

  • Assignee set to Eregon (Benoit Daloze)

@matz (Yukihiro Matsumoto) agreed to remove ZSUPER methods, because the current illusion is incomplete.
In practice the semantics are the same (code is unlikely to monkey-patch a method after making it public), except owner/instance_methods is more consistent with "alias/copy" semantics.
I'll make a PR.
EDIT: see https://github.com/ruby/ruby/pull/6242#issuecomment-1219593598

Updated by Eregon (Benoit Daloze) 4 months ago

To explain my mental model:
I think Method/UnboundMethod captures a given method, and that should never change (even if someone monkey patches or remove/add methods).
This currently holds, except for ZSUPER methods, so the change will fix that.
(note: java.lang.reflect.Method is the opposite, it just captures the name and not a method entry)

I think public and alias should both "copy" the method entry.
Some people think public should instead just change visibility and delegate to super method (current semantics).
Because code is very unlikely to monkey-patch a method after making it public, both mental models work in practice.

Actions #32

Updated by jeremyevans (Jeremy Evans) 4 months ago

  • Status changed from Open to Closed

Applied in changeset git|8212aab81a77a2a91fb7c1681b4968171193b48f.


Make Object#method and Module#instance_method not skip ZSUPER methods

Based on https://github.com/jeremyevans/ruby/commit/c95e7e5329140f640b6497905485761f3336d967

Among other things, this fixes calling visibility methods (public?,
protected?, and private?) on them. It also fixes #owner to show the
class the zsuper method entry is defined in, instead of the original
class it references.

For some backwards compatibility, adjust #parameters and #source_location,
to show the parameters and source location of the method originally
defined. Also have the parameters and source location still be shown
by #inspect.

Clarify documentation of {Method,UnboundMethod}#owner.

Add tests based on the description of https://bugs.ruby-lang.org/issues/18435
and based on https://github.com/ruby/ruby/pull/5356#issuecomment-1005298809

Fixes [Bug #18435] [Bug #18729]

Co-authored-by: Benoit Daloze

Actions #33

Updated by Eregon (Benoit Daloze) 4 months ago

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

Updated by matz (Yukihiro Matsumoto) 2 months ago

After playing with the committed new behavior, I have noticed some points, and changed my mind (I am sorry for being late to reply).

Look at the following example:

class C0
  def foo
    [:foo]
  end
end

class C1<C0
  def foo
    super + [:bar]
  end
end
m1 = C1.instance_method(:foo)

class C2<C1
  private :foo
end

p C2.instance_methods(false)      # (a)
m2 = C2.instance_method(:foo)

class C1
  def foo
    [:bar2]
  end
end

m3 = C2.instance_method(:foo)
c = C2.new
p m1.bind_call(c)                 # (b)
p m1.owner                        # (c)
p m2.bind_call(c)                 # (d)
p m2.owner                        # (e)
p m3.bind_call(c)                 # (f)
p m3.owner                        # (g)

Before the change, the results are a=[]; b=[:foo,:bar]; c=C1; d=[:foo,:bar]; e=C1; f=[:bar2]; g=C1.

After the change, the results are a=[]; b=[:foo,:bar]; c=C1; d=[:bar2]; e=C2; f=[:bar2]; g=C2.

The problem is (a) and (d), especially the latter. As a general principle, retrieving a method object should keep the current behavior, so the expected result from (c) must be d=[:foo,:bar] even after redefining foo in C1. So I have to reject the idea of revealing ZSUPER method, along with copying.

At the same time, making owner C2 is OK for me. But for the consistency with the idea, (a) might need to include :foo which is overridden in the class C2. Probably this also addresses #18435 as well.

Matz.

Updated by Eregon (Benoit Daloze) 2 months ago

@matz (Yukihiro Matsumoto)
(a) should be p C2.private_instance_methods(false) (.instance_methods doesn't show private methods).
Then on all versions foo is included.
And of course that means it should be possible to retrieve that method with Kernel#method/Module#instance_method, which became possible since my change.

As a general principle, retrieving a method object should keep the current behavior

We agree on this.
However, the only way to do that reliably is to remove ZSUPER methods altogether (https://github.com/ruby/ruby/pull/6251).
That removes a lot of complexity and confusion, for a feature nobody needs.
Then it's clear a Method/UnboundMethod points to a specific method definition, and we never have all these subtle edge cases to reason about. Finally we'll have a clear and understandable meta view for methods in Ruby.

The previous logic of skipping ZSUPER methods in Kernel#method and Module#instance_method just works around the problem and confuses everybody, in addition to revealing inconsistent owner, visibility, etc.

Also skipping ZSUPER methods would again introduce issues with Method#public?, etc (#11689).
And in 3.1 those methods exist, and ZSUPER methods are now revealed on the 3.1 branch (https://bugs.ruby-lang.org/issues/18435#note-25), which means those methods behave correctly, as expected and intuitively.
So it seems unacceptable to skip ZSUPER methods entirely in Kernel#method/Module#instance_method, it's just ignoring the fundamental issue.

To compare, the behavior of removing ZSUPER methods (https://github.com/ruby/ruby/pull/6251) and TruffleRuby is:

[:foo, :bar]
C1
[:foo, :bar]
C2
[:foo, :bar]
C2

That is, a Method/UnboundMethod always points to a specific method entry, and a Method/UnboundMethod actually captures the state when it was created.

In any case, I think the behavior for such a corner case that nobody uses in practice does not matter much, the general semantics and understandability of the model seem far more important.

So I think we have 3 choices:

  • Trying to give the correct owner and in Kernel#method/Module#instance_method already lookup the method entry the ZSUPER method would resolve to. I will give it a try, but it feels brittle and as already tried for the visibility by Jeremy will likely result in some weird inconsistencies and more complexity in the implementation where some of the Method state is overridden by extra fields and some not. But maybe it's good enough.
  • Keep as it is on current master, the overall semantics are good, it's only surprising for edge cases nobody uses (redefining a method after changing visibility in subclass and using Method/UnboundMethod objects created before that and wanting the old behavior for that case)
  • Remove ZSUPER methods (https://github.com/ruby/ruby/pull/6251). That simplifies everything tremendously and results in better performance.

The last one is IMHO by far the better and safest option, everything is consistent and simple.

Updated by Eregon (Benoit Daloze) 2 months ago

Looking at UnboundMethod methods, for a zsuper method (ME = method entry):

  • ==, eql?, hash: needs both resolved ME and zsuper ME (owner) (until #18798)
  • to_s, inspect, clone: needs both resolved ME and zsuper ME (owner)
  • arity, parameters, source_location, super_method: needs only resolved ME
  • name: either ME is fine
  • original_name: I think either ME is fine
  • owner, bind, bind_call: needs zsuper ME (owner)
  • visibility/public?/protected?/private?: needs zsuper ME (visibility)

Looks about half-half overall.
Interestingly we only seem to need the owner and visibility info from the zsuper ME, so resolving the zsuper ME on Kernel#method/Module#instance_method seems feasible.

Updated by Eregon (Benoit Daloze) 2 months ago

@matz (Yukihiro Matsumoto) I implemented the first choice in https://github.com/ruby/ruby/pull/6467.
That seems closest to what you requested, and the result of your script is then:

[:foo, :bar]
C1
[:foo, :bar]
C2 # C1 on 3.1
[:bar2]
C2 # C1 on 3.1

So only the owner changes but not the result of bind_call.

It's a bit brittle because one needs to use METHOD->owner and not METHOD->me->owner, but it's not too bad either and it avoids several zsuper lookups in various Method,UnboundMethod methods.
I think overall it's a good compromise of compatibility and remaining intuitive.
It's very similar semantically to removing ZSUPER methods/copying ME like alias.
Performance-wise it's between the previous approach (faster than it since resolve once on lookup) and removing ZSUPER methods (slightly slower than that).

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0