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

• Status: Rejected
• Priority: Normal
• Assignee: -
• Target version: -
• ruby -v: ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [x86_64-linux]
• Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN

[Eregon (Benoit Daloze)]

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]]

[Eregon (Benoit Daloze)]

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.

[jeremyevans0 (Jeremy Evans)]

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

[Eregon (Benoit Daloze)]

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

[jeremyevans0 (Jeremy Evans)]

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.

[Eregon (Benoit Daloze)]

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 :/

[mame (Yusuke Endoh)]

@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 :-)

[Eregon (Benoit Daloze)]

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.

[jeremyevans0 (Jeremy Evans)]

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

[Eregon (Benoit Daloze)]

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.

[Eregon (Benoit Daloze)]

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)

[Eregon (Benoit Daloze)]

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

[jeremyevans0 (Jeremy Evans)]

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

[Eregon (Benoit Daloze)]

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

[jeremyevans0 (Jeremy Evans)]

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.

[Eregon (Benoit Daloze)]

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.

[matz (Yukihiro Matsumoto)]

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

[mame (Yusuke Endoh)]

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

[Eregon (Benoit Daloze)]

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

[matz (Yukihiro Matsumoto)]

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

[matz (Yukihiro Matsumoto)]

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.

[Eregon (Benoit Daloze)]

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

[Eregon (Benoit Daloze)]

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.

[matz (Yukihiro Matsumoto)]

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.

[matz (Yukihiro Matsumoto)]

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.

[Eregon (Benoit Daloze)]

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