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, 31 insertions, 0 deletions

diff --git a/activesupport/test/core_ext/module_test.rb b/activesupport/test/core_ext/module_test.rb
index 9a8582075d..8872611fb1 100644
--- a/activesupport/test/core_ext/module_test.rb
+++ b/activesupport/test/core_ext/module_test.rb
@@ -82,6 +82,21 @@ class Name
   end
 end
 
+class SideEffect
+  attr_reader :ints
+
+  delegate :to_i, :to => :shift, :allow_nil => true
+  delegate :to_s, :to => :shift
+
+  def initialize
+    @ints = [1, 2, 3]
+  end
+
+  def shift
+    @ints.shift
+  end
+end
+
 class ModuleTest < ActiveSupport::TestCase
   def setup
     @david = Someone.new("David", Somewhere.new("Paulina", "Chicago"))
@@ -171,6 +186,12 @@ class ModuleTest < ActiveSupport::TestCase
     assert_nil rails.name
   end
 
+  # Ensures with check for nil, not for a falseish target.
+  def test_delegation_with_allow_nil_and_false_value
+    project = Project.new(false, false)
+    assert_raise(NoMethodError) { project.name }
+  end
+
   def test_delegation_with_allow_nil_and_invalid_value
     rails = Project.new("Rails", "David")
     assert_raise(NoMethodError) { rails.name }
@@ -233,6 +254,16 @@ class ModuleTest < ActiveSupport::TestCase
            "[#{e.backtrace.inspect}] did not include [#{file_and_line}]"
   end
 
+  def test_delegation_invokes_the_target_exactly_once
+    se = SideEffect.new
+
+    assert_equal 1, se.to_i
+    assert_equal [2, 3], se.ints
+
+    assert_equal '2', se.to_s
+    assert_equal [3], se.ints
+  end
+
   def test_parent
     assert_equal Yz::Zy, Yz::Zy::Cd.parent
     assert_equal Yz, Yz::Zy.parent