Bug #5015

method_added" is called in addition to "method_undefined

Added by Lazaridis Ilias almost 3 years ago. Updated almost 3 years ago.

[ruby-core:<unknown>]
Status:Closed
Priority:Normal
Assignee:-
Category:-
Target version:-
ruby -v:1.9.2 Backport:

Description

When a method is undefined via "undefmethod :anymethod", the following methods are called back:

  • method_added
  • method_undefined

The expected behaviour is that only "methodundefined" is called (similar to "removemethod").

class String

def self.method_added(id)
puts "callback: added #{id.id2name}"
end

def self.method_undefined(id)
puts "callback: undefined #{id.id2name}"
end

def self.method_removed(id)

puts "callback: removed #{id.id2name}"
end

end

puts "\nundef method (buildin)"
class String
undef_method :capitalize
end

puts "\ndefine method"
class String
def any_method(*args)
nil
end
end

puts "\nundef method"
class String
undefmethod :anymethod
end

puts "\ndefine method"
class String
def any_method(*args)
nil
end
end

puts "\nremove method"
class String
removemethod :anymethod
end

OUTPUT:

undef method (buildin)
callback: added capitalize
callback: undefined capitalize

define method
callback: added any_method

undef method
callback: added anymethod
callback: undefined any
method

define method
callback: added any_method

remove method
callback: removed any_method

method_added_callback.diff Magnifier (1001 Bytes) Lazaridis Ilias, 07/12/2011 04:18 AM

Associated revisions

Revision 32527
Added by Yukihiro Matsumoto almost 3 years ago

  • vmmethod.c (rbaddmethod): should not call methodadded hook for undef operation. [Bug #5015]

History

#1 Updated by Magnus Holm almost 3 years ago

But undef_method does actually add a new method: it adds a method
which prevents the superclass' method being called:

class Foo < String; def +() end; undef_method :+ end
Foo.new + "test" # => raises error

class Bar < String; def +() end; remove_method :+ end
Bar.new + "test" # => calls String#+

Seems consistant to me.

#2 Updated by Lazaridis Ilias almost 3 years ago

Magnus Holm wrote:

But undef_method does actually add a new method: it adds a method
which prevents the superclass' method being called
[...]

Whatever "undefmethod" does, is an internal implementation detail, which should not call "methodadded".

undef method
callback: added anymethod # false callback, "anymethod" was not added by user
callback: undefined anymethod # correct callback, "anymethod" was undefined by user

#3 Updated by John Higgins almost 3 years ago

Lazaridis Ilias wrote:

Magnus Holm wrote:

But undef_method does actually add a new method: it adds a method
which prevents the superclass' method being called
[...]

Whatever "undefmethod" does, is an internal implementation detail, which should not call "methodadded".

What rule exactly says that internal implementations can't fire callbacks? You are specifically interested in internal implementation issues when you are interested in things like "method_added" - there is always a price to peeking under the hood.

undef method
callback: added anymethod # false callback, "anymethod" was not added by user

Where exactly is the rule that a method needs to be added by a user to having a callback fired?

Using your vaunted "I want every literal String to fire initialize" concept - when a user "requires" a file at least 10-15 strings are created in the background for a variety of reasons - is your argument that the initialize method shouldn't fire for those because the user didn't create the strings? Where does one draw the line?

The undef_method call performs a specific task which you have made an assumption as to why it would be done. Lets assume we look at a variation of the example in the docs

class Parent
def hello
puts "In parent"
end
end
class Child < Parent
end

c = Child.new
c.hello

class Child
undef_method :hello # prevent any calls to 'hello'
end

c.hello

Outputs:
In Parent
prog.rb:23: undefined method `hello' for #Child:0x401b3bb4 (NoMethodError)

In this case I am most certainly adding a method to my Child class because I'm specifically stopping a call from going up to Parent when #hello is called. Whether I use a convenience method like undefmethod or write something to throw the method myself - I need to insert something into the call chain to prevent #hello from firing on the Parent class. You've made the assumption, incorrectly, that this can only be used on methods that have been added to a class previous to a undefmethod call - that's simply not the case at all.

Folks again have taken a reasonable step to ensure that both speed and flexibility are met.

You are at no disadvantage in terms of understanding what is occurring either because the callbacks fire in a deterministic order for you. You know that if you have a "undefined" callback fire that there is a "added" callback that has just fired - you can easily revert any tracking that the added callback did for you if you wanted to (except if you want to know which methods actual exist on a class because undef_method certainly can add a method to a class).

#4 Updated by Lazaridis Ilias almost 3 years ago

John Higgins wrote:
[...]

Mr. Higgins, you could write a book in your non-technical tenor about this issue, the fact remains:

  • callback: added anymethod # false callback, "anymethod" was not added by user

I'll await the response of the related developer.

#5 Updated by John Higgins almost 3 years ago

Lazaridis Ilias wrote:

John Higgins wrote:
[...]

Mr. Higgins, you could write a book in your non-technical tenor about this issue, the fact remains:

  • callback: added anymethod # false callback, "anymethod" was not added by user

Would you care to, even in passing, explain how I'm incorrect when it comes to the Child class example I gave? Or does clear, simple, 13 line examples showing specific cases where your hypothesis is incorrect too "non-technical a tenor" for you?

You are nothing if not consistent at least........

#6 Updated by Lazaridis Ilias almost 3 years ago

John Higgins wrote:

Lazaridis Ilias wrote:

John Higgins wrote:
[...]

Mr. Higgins, you could write a book in your non-technical tenor about this issue, the fact remains:

  • callback: added anymethod # false callback, "anymethod" was not added by user

Would you care to, even in passing, explain how I'm incorrect when it comes to the Child class example I gave?
[...]

No, I can't, because it is irrelevant (and thus I do not invest any energy to analyse). I've provided a use-case. In context of this use-case, the callback "method_added :capitalize" is false, and thus a defect. There's really no need to change context or use-cases: the defect in this use-case remains.

#7 Updated by Lazaridis Ilias almost 3 years ago

  • File method_added_callback.diff added

[edit: please ignore the first patch send (32kB), the 1001 Bytes one is the correct]

Attached a simple patch, which avoids the method_added callback for

VMMETHODTYPE_UNDEF

OUTPUT:

undef method (buildin)
callback: undefined capitalize

define method
callback: added any_method

undef method
callback: undefined any_method

define method
callback: added any_method

remove method
callback: removed any_method

#9 Updated by Steve Klabnik almost 3 years ago

+1 on 'not a defect.'

If a method is added, I expect the methodadded callback to be fired. undefmethod adds a method. This is pretty open-and-shut.

#10 Updated by John Higgins almost 3 years ago

Lazaridis Ilias wrote:

John Higgins wrote:

Lazaridis Ilias wrote:

John Higgins wrote:
[...]

Mr. Higgins, you could write a book in your non-technical tenor about this issue, the fact remains:

  • callback: added anymethod # false callback, "anymethod" was not added by user

Would you care to, even in passing, explain how I'm incorrect when it comes to the Child class example I gave?
[...]

No, I can't, because it is irrelevant (and thus I do not invest any energy to analyse). I've provided a use-case. In context of this use-case, the callback "method_added :capitalize" is false, and thus a defect. There's really no need to change context or use-cases: the defect in this use-case remains.

Again, what are you basing this on? I asked you what rule/concept states that internal implementations cannot fire callbacks. Why is it an error if the implementation of undefmethod adds a method to the class? Where is the definition of undefmethod that states anything about its implementation? In fact the docs state only the following "Prevents the current class from responding to calls to the named method." - why exactly is that at odds with a methodadded callback being fired during the execution of an undefmethod call?

Would you only call this a bug in a class with a "direct" definition of a method (i.e. non-inherited method). What about the user that wants to track the addition of the "blocking" method to the subclass if you wanted the method_added subclass suppressed even in a subclass without a direct definition of a method?

#11 Updated by Lazaridis Ilias almost 3 years ago

John Higgins wrote:
[...]

no comment.

"I'll await the response of the related developer."

#12 Updated by John Higgins almost 3 years ago

Lazaridis Ilias wrote:

John Higgins wrote:
[...]

no comment.

"I'll await the response of the related developer."

Very technical response - thanks for the rebuttal, I'm sure the "related developer" will be more than happy to implement a patch which turns off a callback that clearly and without question is valid in a multitude of cases. Your reasoned argument clearly shows the error of my ways.......

Thanks again!

#13 Updated by Michael Klishin almost 3 years ago

Lazaridis Ilias: changing behaviors of the fundamental classes because of one use case doesn't feel right. In fact, it sounds scary.

#14 Updated by Yukihiro Matsumoto almost 3 years ago

  • File deleted (method_added_callback.diff)

#15 Updated by Yukihiro Matsumoto almost 3 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r32527.
Lazaridis, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • vmmethod.c (rbaddmethod): should not call methodadded hook for undef operation. [Bug #5015]

Also available in: Atom PDF