From 0ee2862e8244661ebad96850ad4aed258cc8ab7f Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Mon, 28 Dec 2015 18:26:02 -0500 Subject: [PATCH] Forwardable: Fix delegating to 'args' and 'block' If you have a class that uses Forwardable to delegate a method to another object, and the method that returns the delegate object is called `args` or `block`, then Forwardable will fail to work. Here's a simple example: class ModelCreator extend Forwardable attr_reader :args def_delegator :args, :model_name def initialize(args) @args = args end end ModelCreator.new.model_name If you run the last line above, then you'll get: NoMethodError: undefined method `model_name' for []:Array This error occurs because `def_delegator` -- as it is written in Ruby -- uses metaprogramming to add methods to the class that will then delegate to the delegate object. So it's as if we had written: class ModelCreator extend Forwardable attr_reader :args def model_name(*args, &block) args.model_name(*args, &block) end def initialize(args) @args = args end end As you can see, `def_delegator` will not only forward the method call onto the delegate object, it will also forward any arguments provided as well. It is here that the bug arises: it splats all of the arguments into a variable which is called `args`, and because of how variable scope works in Ruby, it then attempts to call `model_name` on *this* variable and *not* our delegate object method. The fix is to call the delegate object method manually using `__send__`. (This assumes, of course, that the given receiver is, in fact, the name of a method and not the name of an instance variable, which is also a possibility.) We use `__send__` because the delegate object method could be private. So, that looks like this: def model_name(*args, &block) __send__(:args).model_name(*args, &block) end Because `def_delegators` and `delegate` use `def_delegator` internally, they also get this fix as well. --- lib/forwardable.rb | 11 ++++- test/test_forwardable.rb | 104 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 110 insertions(+), 5 deletions(-) diff --git a/lib/forwardable.rb b/lib/forwardable.rb index cd15eea..d9e0a1a 100644 --- a/lib/forwardable.rb +++ b/lib/forwardable.rb @@ -178,23 +178,30 @@ def def_instance_delegators(accessor, *methods) # q.push 23 #=> NoMethodError # def def_instance_delegator(accessor, method, ali = method) + receiver = + if method_defined?(accessor) || private_method_defined?(accessor) + "__send__(:#{accessor})" + else + accessor + end + line_no = __LINE__; str = %{ def #{ali}(*args, &block) begin - #{accessor}.__send__(:#{method}, *args, &block) + #{receiver}.__send__(:#{method}, *args, &block) rescue ::Exception $@.delete_if{|s| ::Forwardable::FILE_REGEXP =~ s} unless ::Forwardable::debug ::Kernel::raise end end } + # If it's not a class or module, it's an instance begin module_eval(str, __FILE__, line_no) rescue instance_eval(str, __FILE__, line_no) end - end alias delegate instance_delegate diff --git a/test/test_forwardable.rb b/test/test_forwardable.rb index a3d03f4..a474e4a 100644 --- a/test/test_forwardable.rb +++ b/test/test_forwardable.rb @@ -27,6 +27,34 @@ def test_def_instance_delegator end end + def test_def_instance_delegator_using_args_method_as_receiver + %i[def_delegator def_instance_delegator].each do |m| + cls = forwardable_class( + receiver_name: :args, + type: :method, + visibility: :private + ) do + __send__ m, :args, :delegated1 + end + + assert_same RETURNED1, cls.new.delegated1 + end + end + + def test_def_instance_delegator_using_block_method_as_receiver + %i[def_delegator def_instance_delegator].each do |m| + cls = forwardable_class( + receiver_name: :block, + type: :method, + visibility: :private + ) do + __send__ m, :block, :delegated1 + end + + assert_same RETURNED1, cls.new.delegated1 + end + end + def test_def_instance_delegators %i[def_delegators def_instance_delegators].each do |m| cls = forwardable_class do @@ -38,6 +66,36 @@ def test_def_instance_delegators end end + def test_def_instance_delegators_using_args_method_as_receiver + %i[def_delegators def_instance_delegators].each do |m| + cls = forwardable_class( + receiver_name: :args, + type: :method, + visibility: :private + ) do + __send__ m, :args, :delegated1, :delegated2 + end + + assert_same RETURNED1, cls.new.delegated1 + assert_same RETURNED2, cls.new.delegated2 + end + end + + def test_def_instance_delegators_using_block_method_as_receiver + %i[def_delegators def_instance_delegators].each do |m| + cls = forwardable_class( + receiver_name: :block, + type: :method, + visibility: :private + ) do + __send__ m, :block, :delegated1, :delegated2 + end + + assert_same RETURNED1, cls.new.delegated1 + assert_same RETURNED2, cls.new.delegated2 + end + end + def test_instance_delegate %i[delegate instance_delegate].each do |m| cls = forwardable_class do @@ -56,6 +114,36 @@ def test_instance_delegate end end + def test_def_instance_delegate_using_args_method_as_receiver + %i[delegate instance_delegate].each do |m| + cls = forwardable_class( + receiver_name: :args, + type: :method, + visibility: :private + ) do + __send__ m, delegated1: :args, delegated2: :args + end + + assert_same RETURNED1, cls.new.delegated1 + assert_same RETURNED2, cls.new.delegated2 + end + end + + def test_def_instance_delegate_using_block_method_as_receiver + %i[delegate instance_delegate].each do |m| + cls = forwardable_class( + receiver_name: :block, + type: :method, + visibility: :private + ) do + __send__ m, delegated1: :block, delegated2: :block + end + + assert_same RETURNED1, cls.new.delegated1 + assert_same RETURNED2, cls.new.delegated2 + end + end + def test_def_single_delegator %i[def_delegator def_single_delegator].each do |m| cls = single_forwardable_class do @@ -126,12 +214,22 @@ def test_basicobject_subclass private - def forwardable_class(&block) + def forwardable_class( + receiver_name: :receiver, + type: :ivar, + visibility: :public, + &block + ) Class.new do extend Forwardable - def initialize - @receiver = RECEIVER + define_method(:initialize) do + instance_variable_set("@#{receiver_name}", RECEIVER) + end + + if type == :method + attr_reader(receiver_name) + __send__(visibility, receiver_name) end class_exec(&block)