Bug #19047: DelegateClass displays "method redefined" warning when overriding methods

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 3.1.2p20
• Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN

[jonathanhefner (Jonathan Hefner)]

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.

[nobu (Nobuyoshi Nakada)]

Yet another possible solution would to add alias_method(method, method) after define_method calls.

[byroot (Jean Boussier)]

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.

[jonathanhefner (Jonathan Hefner)]

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?

[byroot (Jean Boussier)]

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.

[jonathanhefner (Jonathan Hefner)]

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).

[byroot (Jean Boussier)]

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.

[jonathanhefner (Jonathan Hefner)]

Submitted alias_method patch as https://github.com/ruby/delegate/pull/13.

[byroot (Jean Boussier)]

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.

[jonathanhefner (Jonathan Hefner)]

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?

[hsbt (Hiroshi SHIBATA)]

• Status changed from Open to Closed

Merged at https://bugs.ruby-lang.org/projects/ruby-master/repository/git/revisions/60610031009e60bdfe5775b0316df251ee36a973

[matz (Yukihiro Matsumoto)]

Sorry, but by this change, we missed the warnings from DelegateClass misuse. We will revert this change.

Matz.

[byroot (Jean Boussier)]

we missed the warnings from DelegateClass misuse

Do you have a reference to that?

[matz (Yukihiro Matsumoto)]

• Related to Bug #19079: Modules included in a DelegateClass cannot override delegate methods added

[matz (Yukihiro Matsumoto)]

For example, the example in #19079 does not give warning for overwriting the method foo.

Matz.