Project

General

Profile

Actions

Bug #18729

open

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

Added by Eregon (Benoit Daloze) about 1 month ago. Updated 9 days ago.

Status:
Open
Priority:
Normal
Assignee:
-
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 1 (1 open0 closed)

Related to Ruby master - Bug #18751: Regression on master for Method#== when comparing public with private methodOpenActions

Updated by Eregon (Benoit Daloze) about 1 month 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) about 1 month 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) about 1 month 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) about 1 month 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) about 1 month 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) about 1 month 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) about 1 month 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) about 1 month 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) about 1 month 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) about 1 month 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) about 1 month ago

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

Updated by jeremyevans0 (Jeremy Evans) about 1 month 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) 28 days 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) 28 days 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) 9 days 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

Also available in: Atom PDF