Bug #17428: Method#inspect bad output for class methods

• Status: Closed
• Assignee: matz (Yukihiro Matsumoto)
• Target version: -
• ruby -v: ruby 3.0.0dev (2020-12-22T17:37:59Z master 487355873a) [x86_64-darwin18]
• Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: REQUIRED

[marcandre (Marc-Andre Lafortune)]

$ $ ruby -e 'p String.method(:prepend)'
# 2.7.0:
#<Method: String.prepend(*)>
# 3.0.0:
#<Method: #<Class:Object>(Module)#prepend(*)>

@jeremyevans found it shows the method as pertaining to one level too high (which is good for objects as we don't want to show the singleton class there, but not for classes).

Probably due to #15608

[marcandre (Marc-Andre Lafortune)]

Correct output would be:

#<Method: #<Class:String>(Module)#prepend(*)>
# or even better imo to reuse the '.' notation here:
#<Method: String(Module).prepend(*)>

[jeremyevans0 (Jeremy Evans)]

I submitted a pull request to fix this issue: https://github.com/ruby/ruby/pull/3984

[Eregon (Benoit Daloze)]

FWIW, current output on TruffleRuby is:

ruby -e 'p String.method(:prepend)'
#<Method: Class(Module)#prepend(*modules) <internal:core> core/module.rb:92>

Probably it should show #<Class:String> instead of Class though.

#<Method: String(Module).prepend(*)>

doesn't seem correct, prepend is not a class method of Module, it's an instance method.

[Eregon (Benoit Daloze)]

With a trivial fix (using the actual class/rb_class_of(), not just .class), I get this in TruffleRuby, which I think is the correct output:

$ ruby -e 'p String.method(:prepend)'
#<Method: #<Class:String>(Module)#prepend(*modules) <internal:core> core/module.rb:92>

$ ruby -e 'p String.method(:prepend).unbind'
#<UnboundMethod: #<Class:String>(Module)#prepend(*modules) <internal:core> core/module.rb:92>

You'll notice it's quite close to MRI's output for an UnboundMethod:

# #<Class:Object> instead of #<Class:String> is the bug, the rest is fine
$ ruby-master -e 'p String.method(:prepend).unbind'
#<UnboundMethod: #<Class:Object>(Module)#prepend(*)>

$ ruby-2.7.2 -e 'p String.method(:prepend).unbind'
#<UnboundMethod: #<Class:String>#prepend(*)>

The actual logic for Method#inspect and and UnboundMethod#inspect should be similar, and I think the logic in TruffleRuby is correct and makes the most sense:
https://github.com/oracle/truffleruby/blob/0aeb60024d025e3d3d836bd08ff89479d4292bb7/src/main/ruby/truffleruby/core/truffle/method_operations.rb#L37-L42

The crucial part for this case: only use the . notation if the owner of the method (Method#owner) is a singleton class, not if the receiver has a singleton class.

# All Rubies agree on:
$ ruby -e 'p String.method(:prepend).owner.singleton_class?'
false

[marcandre (Marc-Andre Lafortune)]

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

#<Method: String(Module).prepend(*)>

doesn't seem correct, prepend is not a class method of Module, it's an instance method.

When (X) appears, it means actually an instance method of X, even in the case where . appears later. So not incorrect, but I agree it might be confusing. How about this other possibility?

#<Method: String.(Module#)prepend(*)>

[Eregon (Benoit Daloze)]

For me, Foo.bar immediately means "singleton method of Foo", which is not the case here.
So I think we should only use the . form when Method#owner is a singleton class.

I recommend simplicity and consistency (between object singleton classes and metaclasses).
The output of Method#inspect has been confusing in the past and sometimes wrong, the way to solve it IMHO is a simple rule and a consistent output.
So I'm against making it more complicated / more edge cases.

[jeremyevans0 (Jeremy Evans)]

Using #<Class:String>(Module)# instead of String(Module). is a trivial change to make, and I have no preference between the two. String.(Module#) is a significantly more invasive change and I don't think we should go that route. It would be great to get feedback from other committers regarding this today if we want this fix to make 3.0.

[marcandre (Marc-Andre Lafortune)]

• Assignee changed from jeremyevans0 (Jeremy Evans) to matz (Yukihiro Matsumoto)

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

String.(Module#) is a significantly more invasive change

I prefer to decide "what do we want" than "what is easier". Change was actually a few lines changes for that version: https://github.com/ruby/ruby/pull/3985

Matz, which do you prefer:

$ ruby -e 'p String.method(:prepend)'
#<Method: String(Module).prepend(*modules)...>
#<Method: String.(Module#)prepend(*modules)...>
#<Method: #<Class:String>(Module)#prepend(*modules)...>

• Status changed from Open to Closed

Applied in changeset git|1e215a66d26d56befab4fbb72e1d953879411955.

---

Fix class of method in Method#inspect for singleton classes of classes

Previously, due to a change to fix bug 15608, Method#inspect output
changed for class methods:

Ruby 2.7
"#<Method: String.prepend(*)>"

Before change:
"#<Method: #Class:Object(Module)#prepend(*)>"

This is wrong because the Method object was created from String and
not Object. This is because the fix for bug 15608 assumed it was
being called on the singleton class of a instance, and would skip
the first singleton class until it got to the class itself. For
class methods, this results in always using the superclass. Fix
behavior to not skip until the superclass if the singleton class
is the singleton class of a module or class.

After change:
"#<Method: #Class:Object(Module)#prepend(*)>"

Fixes [Bug #17428]

[Eregon (Benoit Daloze)]

marcandre (Marc-Andre Lafortune) wrote in #note-8:

I prefer to decide "what do we want" than "what is easier".

I'm not matz, but what I want for Method#inspect is consistency and a clear rule for formatting.
(https://github.com/oracle/truffleruby/blob/0aeb60024d025e3d3d836bd08ff89479d4292bb7/src/main/ruby/truffleruby/core/truffle/method_operations.rb#L37-L42 seems a good one)
IMHO more special cases just lead to bugs like this, confusion as before #15608, or harder-to-understand output.

Method#inspect doesn't need to be "as nice as possible", I think it needs to be "easy to understand and consistent to help comparison and avoid ambiguity".

I think this is a good example why the behavior of current master with Jeremy's fix is better:

$ ruby -e 'p Module.method(:prepend)'

2.7.2, confusing, prepend is not a singleton method
#<Method: Module.prepend(*)>

CRuby master with Jeremy's fix (same on TruffleRuby): clear, simple, consistent
#<Method: #<Class:Module>(Module)#prepend(*)>

the proposals above, IMHO both are unclear and confusing:
#<Method: Module(Module).prepend(*)>
#<Method: Module.(Module#)prepend(*)>

[jeremyevans0 (Jeremy Evans)]

After thinking about this more, I would prefer :

Module.method(:prepend)
# => #<Method: Module.prepend(Module#prepend)(*)>

Basically, show the method based on the receiver, then, if it is different, the method based on the owner. I think this is conceptually simpler. The only issue it will result in the method name showing up twice even if the method has not been aliased.

Additional examples:

class A
  def self.b; end
end
class C < A;
end
C.method(:b)
# => #<Method: C.b(A.b)(*)>

module M
  def b; end
end
C.extend M
C.method(:b)
# => #<Method: C.b(M#b)(*)>

String.new.method(:to_s)
# => #<Method: String#to_s(*)>

class S < String
end
S.new.method(:to_s)
# => #<Method: S#to_s(String#to_s)(*)>

[Eregon (Benoit Daloze)]

• Backport changed from 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN to 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: REQUIRED

Agreed that looks nice, but what should be shown from the example in #15608?

p obj.method(:foo)
#<Method: C#foo>
vs
#<Method: #<C:0x000055668ebef268>.foo(C#foo)>

based on whether the instance has a singleton class doesn't seem ideal.

I wonder if always using the # notation wouldn't be simpler and more consistent (we'd always see the module in which the method is defined).
If it's a singleton class method, we might as well show the singleton class.

I think a new feature to discuss it would be best, the bug that @marcandre (Marc-Andre Lafortune) found has been fixed now.

[nagachika (Tomoyuki Chikanaga)]

MEMO: Though there should be some preceding changesets, I gave up to find it for the time being.