Merge pull request #59 from dry-rb/fix-memoization-of-pass-through

Fix memoization of pass-through signature

lib/dry/core/memoizable.rb

@@ -6,6 +6,7 @@ module Dry
   module Core
     module Memoizable
       MEMOIZED_HASH = {}.freeze
+      PARAM_PLACEHOLDERS = %i[* ** &].freeze
 
       module ClassInterface
         module Base
@@ -64,25 +65,26 @@ def initialize(klass, names)
         private
 
         # @api private
-        def define_memoizable(method:)
+        def define_memoizable(method:) # rubocop:disable Metrics/AbcSize
           parameters = method.parameters
 
           if parameters.empty?
             key = method.name.hash
-            module_eval <<~RUBY, __FILE__, __LINE__ + 1
-              def #{method.name}
-                if @__memoized__.key?(#{key})
-                  @__memoized__[#{key}]
-                else
-                  @__memoized__[#{key}] = super
-                end
-              end
+            module_eval(<<~RUBY, __FILE__, __LINE__ + 1)
+              def #{method.name}                    # def slow_fetch
+                if @__memoized__.key?(#{key})       #   if @__memoized__.key?(12345678)
+                  @__memoized__[#{key}]             #     @__memoized__[12345678]
+                else                                #   else
+                  @__memoized__[#{key}] = super     #     @__memoized__[12345678] = super
+                end                                 #   end
+              end                                   # end
             RUBY
           else
             mapping = parameters.to_h { |k, v = nil| [k, v] }
             params, binds = declaration(parameters, mapping)
+            last_param = parameters.last
 
-            if parameters.last[0].eql?(:block)
+            if last_param[0].eql?(:block) && !last_param[1].eql?(:&)
               Deprecations.warn(<<~WARN)
                 Memoization for block-accepting methods isn't safe.
                 Every call creates a new block instance bloating cached results.
@@ -91,21 +93,23 @@ def #{method.name}
               WARN
             end
 
-            module_eval <<~RUBY, __FILE__, __LINE__ + 1
-              def #{method.name}(#{params.join(", ")})
-                key = [:"#{method.name}", #{binds.join(", ")}].hash
-
-                if @__memoized__.key?(key)
-                  @__memoized__[key]
-                else
-                  @__memoized__[key] = super
-                end
-              end
+            m = module_eval(<<~RUBY, __FILE__, __LINE__ + 1)
+              def #{method.name}(#{params.join(", ")})                 # def slow_calc(arg1, arg2, arg3)
+                key = [:"#{method.name}", #{binds.join(", ")}].hash    #   [:slow_calc, arg1, arg2, arg3].hash
+                                                                       #
+                if @__memoized__.key?(key)                             #   if @__memoized__.key?(key)
+                  @__memoized__[key]                                   #     @__memoized__[key]
+                else                                                   #   else
+                  @__memoized__[key] = super                           #     @__memoized__[key] = super
+                end                                                    #   end
+              end                                                      # end
             RUBY
 
             if respond_to?(:ruby2_keywords, true) && mapping.key?(:reyrest)
               ruby2_keywords(method.name)
             end
+
+            m
           end
         end
 
@@ -116,13 +120,13 @@ def declaration(definition, lookup)
           defined = {}
 
           definition.each do |type, name|
-            mapped_type = map_bind_type(type, lookup, defined) do
+            mapped_type = map_bind_type(type, name, lookup, defined) do
               raise ::NotImplementedError, "type: #{type}, name: #{name}"
             end
 
             if mapped_type
               defined[mapped_type] = true
-              bind = name || make_bind_name(binds.size)
+              bind = name_from_param(name) || make_bind_name(binds.size)
 
               binds << bind
               params << param(bind, mapped_type)
@@ -132,18 +136,35 @@ def declaration(definition, lookup)
           [params, binds]
         end
 
+        # @api private
+        def name_from_param(name)
+          if PARAM_PLACEHOLDERS.include?(name)
+            nil
+          else
+            name
+          end
+        end
+
         # @api private
         def make_bind_name(idx)
           :"__lv_#{idx}__"
         end
 
         # @api private
-        def map_bind_type(type, original_params, defined_types) # rubocop:disable Metrics/PerceivedComplexity
+        def map_bind_type(type, name, original_params, defined_types) # rubocop:disable Metrics/PerceivedComplexity
           case type
           when :req
             :reqular
-          when :rest, :keyreq, :keyrest, :block
+          when :rest, :keyreq, :keyrest
             type
+          when :block
+            if name.eql?(:&)
+              # most likely this is a case of delegation
+              # rather than actual block
+              nil
+            else
+              type
+            end
           when :opt
             if original_params.key?(:rest) || defined_types[:rest]
               nil

spec/dry/core/memoizable_spec.rb

@@ -2,9 +2,16 @@
 
 require "concurrent/atomic/atomic_fixnum"
 require "dry/core/memoizable"
+require "tempfile"
 require_relative "../../support/memoized"
 
 RSpec.describe Dry::Core::Memoizable do
+  before do
+    Dry::Core::Deprecations.set_logger!(Tempfile.new("dry_deprecations"))
+  end
+
+  before { Memoized.memoize_methods }
+
   describe ".memoize" do
     describe Object do
       it_behaves_like "a memoizable class" do
@@ -34,37 +41,45 @@
   describe Memoized.new do
     let(:block) { -> {} }
 
-    describe described_class.method(:test1) do
-      it_behaves_like "a memoized method"
+    describe "test1" do
+      it_behaves_like "a memoized method" do
+        let(:new_meth) { described_class.method(:test1) }
 
-      it "does not raise an error" do
-        2.times do
-          described_class.call("a", kwarg1: "world", other: "test", &block)
+        it "does not raise an error" do
+          2.times do
+            new_meth.("a", kwarg1: "world", other: "test", &block)
+          end
         end
       end
     end
 
-    describe described_class.method(:test2) do
-      it_behaves_like "a memoized method"
+    describe "test2" do
+      it_behaves_like "a memoized method" do
+        let(:new_meth) { described_class.method(:test2) }
 
-      it "does not raise an error" do
-        2.times { described_class.call("a", &block) }
+        it "does not raise an error" do
+          2.times { new_meth.("a", &block) }
+        end
       end
     end
 
-    describe described_class.method(:test3) do
-      it_behaves_like "a memoized method"
+    describe "test3" do
+      it_behaves_like "a memoized method" do
+        let(:new_meth) { described_class.method(:test3) }
 
-      it "does not raise an error" do
-        2.times { described_class.call(&block) }
+        it "does not raise an error" do
+          2.times { new_meth.(&block) }
+        end
       end
     end
 
-    describe described_class.method(:test4) do
-      it_behaves_like "a memoized method"
+    describe "test4" do
+      it_behaves_like "a memoized method" do
+        let(:new_meth) { described_class.method(:test4) }
 
-      it "does not raise an error" do
-        2.times { described_class.call }
+        it "does not raise an error" do
+          2.times { new_meth.call }
+        end
       end
     end
   end

spec/support/memoized.rb

@@ -1,6 +1,11 @@
 # frozen_string_literal: true
 
 class Memoized
+  include Module.new {
+    def test9
+      # NOP
+    end
+  }
   include Dry::Core::Memoizable
 
   def test1(arg1, *args, kwarg1:, kwarg2: "default", **kwargs, &block)
@@ -35,5 +40,23 @@ def test8(args = 1, kwargs = 2, *, **)
     # NOP
   end
 
-  memoize :test1, :test2, :test3, :test4, :test5, :test6, :test7, :test8
+  if RUBY_VERSION >= "2.7"
+    module_eval(<<~RUBY, __FILE__, __LINE__ + 1)
+      def test9(...)
+        super
+      end
+    RUBY
+  end
+
+  def self.memoize_methods
+    @memoized ||= begin
+      if RUBY_VERSION >= "2.7"
+        memoize :test1, :test2, :test3, :test4, :test5, :test6, :test7, :test8, :test9
+      else
+        memoize :test1, :test2, :test3, :test4, :test5, :test6, :test7, :test8
+      end
+
+      true
+    end
+  end
 end

spec/support/shared_examples/memoizable.rb

@@ -100,7 +100,6 @@ def falsey
 end
 
 RSpec.shared_examples "a memoized method" do
-  let(:new_meth) { described_class }
   let(:old_meth) { new_meth.super_method }
 
   describe "new != old" do