Module#delegate checks nilness rather that falsehood if :allow_nil is true, and avoids multiple evaluation of the target method

Notes:

1) I hope nilness is a word.

2) See rationale for avoiding multiple evaluation in a comment in the patch, credit goes to @jeremy for pointing out this gotcha in the existing implementation.

3) Embeds a little joke dedicated to @pixeltrix (it could be worse! :D).

References #10347.

1 files changed, 42 insertions, 30 deletions

diff --git a/activesupport/lib/active_support/core_ext/module/delegation.rb b/activesupport/lib/active_support/core_ext/module/delegation.rb
index c0828343d8..e60f9a1fe6 100644
--- a/activesupport/lib/active_support/core_ext/module/delegation.rb
+++ b/activesupport/lib/active_support/core_ext/module/delegation.rb
@@ -1,8 +1,10 @@
 class Module
-  # Provides a delegate class method to easily expose contained objects' public methods
-  # as your own. Pass one or more methods (specified as symbols or strings)
-  # and the name of the target object via the <tt>:to</tt> option (also a symbol
-  # or string). At least one method and the <tt>:to</tt> option are required.
+  # Provides a +delegate+ class method to easily expose contained objects'
+  # public methods as your own.
+  #
+  # The macro receives one or more method names (specified as symbols or
+  # strings) and the name of the target object via the <tt>:to</tt> option
+  # (also a symbol or string).
   #
   # Delegation is particularly useful with Active Record associations:
   #
@@ -89,39 +91,40 @@ class Module
   #   invoice.customer_name    # => 'John Doe'
   #   invoice.customer_address # => 'Vimmersvej 13'
   #
-  # If the delegate object is +nil+ an exception is raised, and that happens
-  # no matter whether +nil+ responds to the delegated method. You can get a
-  # +nil+ instead with the +:allow_nil+ option.
+  # If the target is +nil+ and does not respond to the delegated method a
+  # +NoMethodError+ is raised, as with any other value. Sometimes, however, it
+  # makes sense to be robust to that situation and that is the purpose of the
+  # <tt>:allow_nil</tt> option: If the target is not +nil+, or it is and
+  # responds to the method, everything works as usual. But if it is +nil+ and
+  # does not respond to the delegated method, +nil+ is returned.
   #
-  #   class Foo
-  #     attr_accessor :bar
-  #     def initialize(bar = nil)
-  #       @bar = bar
-  #     end
-  #     delegate :zoo, to: :bar
+  #   class User < ActiveRecord::Base
+  #     has_one :profile
+  #     delegate :age, to: :profile
   #   end
   #
-  #   Foo.new.zoo   # raises NoMethodError exception (you called nil.zoo)
+  #   User.new.age # raises NoMethodError: undefined method `age'
   #
-  #   class Foo
-  #     attr_accessor :bar
-  #     def initialize(bar = nil)
-  #       @bar = bar
-  #     end
-  #     delegate :zoo, to: :bar, allow_nil: true
+  # But if not having a profile yet is fine and should not be an error
+  # condition:
+  #
+  #   class User < ActiveRecord::Base
+  #     has_one :profile
+  #     delegate :age, to: :profile, allow_nil: true
   #   end
   #
-  #   Foo.new.zoo   # returns nil
+  #   User.new.age # nil
   #
-  # If the delegate object is not +nil+ or +false+ and the object doesn't
-  # respond to the delegated method it will raise an exception.
+  # Note that if the target is not +nil+ then the call is attempted regardless of the
+  # <tt>:allow_nil</tt> option, and thus an exception is still raised if said object
+  # does not respond to the method:
   #
   #   class Foo
   #     def initialize(bar)
   #       @bar = bar
   #     end
   #
-  #     delegate :name, to: :@bar
+  #     delegate :name, to: :@bar, :allow_nil => true
   #   end
   #
   #   Foo.new("Bar").name # raises NoMethodError: undefined method `name'
@@ -156,22 +159,31 @@ class Module
       # methods still accept two arguments.
       definition = (method =~ /[^\]]=$/) ? 'arg' : '*args, &block'
 
+      # The following generated methods call the target exactly once, storing
+      # the returned value in a dummy variable.
+      #
+      # Reason is twofold: On one hand doing less calls is in general better.
+      # On the other hand it could be that the target has side-effects,
+      # whereas conceptualy, from the user point of view, the delegator should
+      # be doing one call.
       if allow_nil
-        module_eval(<<-EOS, file, line - 2)
+        module_eval(<<-EOS, file, line - 3)
           def #{method_prefix}#{method}(#{definition})        # def customer_name(*args, &block)
-            if #{to} || #{to}.respond_to?(:#{method})         #   if client || client.respond_to?(:name)
-              #{to}.#{method}(#{definition})                  #     client.name(*args, &block)
+            _ = #{to}                                         #   _ = client
+            if !_.nil? || nil.respond_to?(:#{method})         #   if !_.nil? || nil.respond_to?(:name)
+              _.#{method}(#{definition})                      #     _.name(*args, &block)
             end                                               #   end
           end                                                 # end
         EOS
       else
         exception = %(raise "#{self}##{method_prefix}#{method} delegated to #{to}.#{method}, but #{to} is nil: \#{self.inspect}")
 
-        module_eval(<<-EOS, file, line - 1)
+        module_eval(<<-EOS, file, line - 2)
           def #{method_prefix}#{method}(#{definition})        # def customer_name(*args, &block)
-            #{to}.#{method}(#{definition})                    #   client.name(*args, &block)
+            _ = #{to}                                         #   _ = client
+            _.#{method}(#{definition})                        #   _.name(*args, &block)
           rescue NoMethodError                                # rescue NoMethodError
-            if #{to}.nil?                                     #   if client.nil?
+            if _.nil?                                         #   if _.nil?
               #{exception}                                    #     # add helpful message to the exception
             else                                              #   else
               raise                                           #     raise