Bug #18729
closedMethod#owner and UnboundMethod#owner are incorrect after using Module#public/protected/private
Added by Eregon (Benoit Daloze) 2 months ago. Updated 10 days ago.
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]]
Updated by Eregon (Benoit Daloze) 2 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) 2 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) 2 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) 2 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) 2 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) 2 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) 2 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) 2 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 ofpublic :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) 2 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) 2 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)
Updated by Eregon (Benoit Daloze) 2 months ago
- Related to Bug #18751: Regression on master for Method#== when comparing public with private method added
Updated by jeremyevans0 (Jeremy Evans) 2 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 ifdef 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, onlyB#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) about 2 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) about 2 months ago
Eregon (Benoit Daloze) wrote in #note-13:
@jeremyevans0 (Jeremy Evans) Do you agree
Method#super_method
should show the method called bysuper
if there issuper
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 withdef foo; super; end
, because B#foo would call A#foo with redeclaring, but with onlypublic :foo in B
A#foo is not called forB.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) about 1 month 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.
Updated by matz (Yukihiro Matsumoto) 23 days ago
- Related to Bug #18435: Calling `protected` on ancestor method changes result of `instance_methods(false)` added
Updated by mame (Yusuke Endoh) 17 days 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) 17 days 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) 10 days 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) 10 days 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) 10 days 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) 10 days 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) 10 days 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) 10 days 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) 10 days 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).