Bug #19047
closedDelegateClass displays "method redefined" warning when overriding methods
Description
Perhaps this is not a bug, but it does seem unexpected.
When creating a DelegateClass
class without an intervening ancestor, overriding a method displays "method redefined" warning:
Base = Class.new do
def foo
"foo"
end
end
Delegate1 = DelegateClass(Base) do
def foo
super + "1"
end
end
# warning: method redefined; discarding old foo
Delegate2 = Class.new(DelegateClass(Base)) do
def foo
super + "2"
end
end
# no warning
Delegate1.new(Base.new).foo
# => "foo1"
Delegate2.new(Base.new).foo
# => "foo2"
One possible solution would be to evaluate the DelegateClass
block in a separate module, and prepend that module to the returned class.
Another possible solution would be to silence warnings around when the block is evaluated.
I would be happy to submit a PR to https://github.com/ruby/delegate if this is something we want to address.
Updated by nobu (Nobuyoshi Nakada) about 2 years ago
Yet another possible solution would to add alias_method(method, method)
after define_method
calls.
Updated by byroot (Jean Boussier) about 2 years ago
For context this was originally discussed on a Rails PR to eliminate warnings from our test suite: https://github.com/rails/rails/pull/46189#discussion_r987395361
In my opinion this is a bug, because if you are creating a delegator, it's very likely you'll redefine at least some of the delegated methods.
The alias_method
trick is likely the way to go.
Updated by jonathanhefner (Jonathan Hefner) about 2 years ago
Indeed, alias_method
is another possibility! And also Jean's suggestion of defining the delegator methods in a module.
Unfortunately, my suggestion of evaluating the block in a module does not work for metaprogramming within the block, such as in this test.
I had not really dug into DelegateClass
to understand why the super
in Delegate1
still works after redefining foo
. I see now that it's because the class is a subclass of Delegator
, which also responds to foo
via method_missing
. Therefore, redefining foo
causes super
to fall back to a slower execution path.
So I benchmarked the solutions with this script:
# frozen_string_literal: true
require "benchmark/ips"
$LOAD_PATH.prepend(".../delegate/lib")
require "delegate"
Base = Class.new { def foo; end }
Overridden = DelegateClass(Base) { def foo; super; end }
overridden = Overridden.new(Base.new)
Benchmark.ips do |x|
x.report("overridden") { overridden.foo }
end
With this alias_method
patch:
index 70d4e4a..af95c86 100644
--- a/lib/delegate.rb
+++ b/lib/delegate.rb
@@ -412,10 +412,12 @@ def DelegateClass(superclass, &block)
end
protected_instance_methods.each do |method|
define_method(method, Delegator.delegating_block(method))
+ alias_method(method, method)
protected method
end
public_instance_methods.each do |method|
define_method(method, Delegator.delegating_block(method))
+ alias_method(method, method)
end
end
klass.define_singleton_method :public_instance_methods do |all=true|
the result is:
Warming up --------------------------------------
overridden 76.674k i/100ms
Calculating -------------------------------------
overridden 765.489k (± 0.9%) i/s - 3.834M in 5.008559s
With this delegator methods module patch:
index 70d4e4a..4311bb5 100644
--- a/lib/delegate.rb
+++ b/lib/delegate.rb
@@ -410,6 +410,8 @@ def DelegateClass(superclass, &block)
__raise__ ::ArgumentError, "cannot delegate to self" if self.equal?(obj)
@delegate_dc_obj = obj
end
+ end
+ klass.include(Module.new do
protected_instance_methods.each do |method|
define_method(method, Delegator.delegating_block(method))
protected method
@@ -417,7 +419,7 @@ def DelegateClass(superclass, &block)
public_instance_methods.each do |method|
define_method(method, Delegator.delegating_block(method))
end
- end
+ end)
klass.define_singleton_method :public_instance_methods do |all=true|
super(all) | superclass.public_instance_methods
end
the result is:
Warming up --------------------------------------
overridden 183.938k i/100ms
Calculating -------------------------------------
overridden 1.830M (± 0.9%) i/s - 9.197M in 5.026683s
Comparison: the alias_method
solution is 2.39x slower than the delegator methods module solution.
Is there another reason to prefer the alias_method
solution?
Updated by byroot (Jean Boussier) about 2 years ago
Is there another reason to prefer the alias_method solution?
The main reason to prefer alias_method
is that it's the least amount of changes, so least likely to break anything.
The fast super
call argument in favor of using an included module is interesting though. However it means Overridden.instance_method(:foo).owner
would change. Not that it's a big deal from my point of view though.
Updated by jonathanhefner (Jonathan Hefner) about 2 years ago
The main reason to prefer
alias_method
is that it's the least amount of changes, so least likely to break anything.
That's a good reason! 😄
There is one more use case that would be affected:
Base = Class.new do
def foo
"foo"
end
end
Helper = Module.new do
def foo
super + "!"
end
end
WithHelper = DelegateClass(Base) { include Helper }
WithHelper.new(Base.new).foo
# BEFORE:
# => "foo"
#
# AFTER:
# => "foo!"
In my opinion, the "BEFORE" is surprising, and the "AFTER" is more desirable, but I understand not wanting to break existing code.
If it would be better, I can submit a PR with the alias_method
patch, and then open a new issue for this use case (with the "delegator methods module" patch attached).
Updated by byroot (Jean Boussier) about 2 years ago
I can submit a PR with the alias_method patch, and then open a new issue for this use case (with the "delegator methods module" patch attached).
Sounds like the correct approach to me. First a patch with basically no change which is super easy to merge, then an optimization with potential backward compatibility, that may or may not be merged.
Updated by jonathanhefner (Jonathan Hefner) about 2 years ago
Submitted alias_method
patch as https://github.com/ruby/delegate/pull/13.
Updated by byroot (Jean Boussier) about 2 years ago
Thank you, now the issue is that delegate
doesn't have a maintainer: https://github.com/ruby/ruby/commit/1159dbf305603b60a1e5d2b9ff77a9cf30775296
I'll be unavailable for the rest of the week, but I'll try to ask around next week.
Updated by jonathanhefner (Jonathan Hefner) about 2 years ago
Thank you, now the issue is that
delegate
doesn't have a maintainer: https://github.com/ruby/ruby/commit/1159dbf305603b60a1e5d2b9ff77a9cf30775296
What is involved in being a maintainer? Is that something I can volunteer for?
Updated by hsbt (Hiroshi SHIBATA) about 2 years ago
- Status changed from Open to Closed
Updated by matz (Yukihiro Matsumoto) almost 2 years ago
Sorry, but by this change, we missed the warnings from DelegateClass misuse. We will revert this change.
Matz.
Updated by byroot (Jean Boussier) almost 2 years ago
we missed the warnings from DelegateClass misuse
Do you have a reference to that?
Updated by matz (Yukihiro Matsumoto) almost 2 years ago
- Related to Bug #19079: Modules included in a DelegateClass cannot override delegate methods added
Updated by matz (Yukihiro Matsumoto) almost 2 years ago
For example, the example in #19079 does not give warning for overwriting the method foo
.
Matz.