From 98bb62b8b3ed2936160ac8fdc6fa607857d100dd Mon Sep 17 00:00:00 2001 From: alpaca-tc Date: Tue, 23 Jan 2024 22:57:39 +0900 Subject: [PATCH] Fixed a bug that overwrote existing self.included method definitions. There is a hack that dynamically defines self.inherited when memo_wise is called, but there was a bug that did not consider the case where an existing self.inherited existed, so this has been fixed. And I also fixed a bug that defined an `#inherited` method on the instance. --- CHANGELOG.md | 1 + README.md | 36 ++++++------- lib/memo_wise.rb | 19 ++++--- spec/memo_wise_spec.rb | 114 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 146 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1350f4..100a5a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ _No breaking changes!_ **Project enhancements:** - Fixed a bug that overwrote existing self.extended method definitions. [[#324]](https://github.com/panorama-ed/memo_wise/pull/314) +- Fixed a bug that overwrote existing self.inherited method definitions. [[#325]](https://github.com/panorama-ed/memo_wise/pull/315) ## [v1.8.0](https://github.com/panorama-ed/memo_wise/compare/v1.7.0...v1.8.0) - 2023-10-25 diff --git a/README.md b/README.md index d8bf449..70e86bd 100644 --- a/README.md +++ b/README.md @@ -118,15 +118,15 @@ Results using Ruby 3.2.2: |Method arguments|`Dry::Core`\* (1.0.1)|`Memery` (1.5.0)| |--|--|--| -|`()` (none)|0.59x|3.65x| -|`(a)`|1.49x|8.38x| -|`(a, b)`|1.21x|6.55x| -|`(a:)`|1.43x|13.14x| -|`(a:, b:)`|1.20x|10.32x| -|`(a, b:)`|1.18x|10.10x| -|`(a, *args)`|0.79x|1.61x| -|`(a:, **kwargs)`|0.75x|1.97x| -|`(a, *args, b:, **kwargs)`|0.67x|1.37x| +|`()` (none)|0.60x|3.58x| +|`(a)`|1.37x|7.41x| +|`(a, b)`|1.20x|6.43x| +|`(a:)`|1.47x|13.60x| +|`(a:, b:)`|1.20x|10.55x| +|`(a, b:)`|1.21x|10.36x| +|`(a, *args)`|0.79x|1.52x| +|`(a:, **kwargs)`|0.77x|2.02x| +|`(a, *args, b:, **kwargs)`|0.69x|1.38x| \* `Dry::Core` [may cause incorrect behavior caused by hash collisions](https://github.com/dry-rb/dry-core/issues/63). @@ -135,15 +135,15 @@ Results using Ruby 2.7.8 (because these gems raise errors in Ruby 3.x): |Method arguments|`DDMemoize` (1.0.0)|`Memoist` (0.16.2)|`Memoized` (1.1.1)|`Memoizer` (1.0.3)| |--|--|--|--|--| -|`()` (none)|23.10x|2.28x|23.77x|2.69x| -|`(a)`|21.57x|14.28x|20.61x|12.05x| -|`(a, b)`|19.05x|13.55x|17.83x|11.68x| -|`(a:)`|30.29x|23.54x|25.22x|21.69x| -|`(a:, b:)`|27.79x|22.83x|23.78x|21.08x| -|`(a, b:)`|26.61x|21.40x|21.63x|19.81x| -|`(a, *args)`|3.16x|2.26x|3.08x|1.97x| -|`(a:, **kwargs)`|2.74x|2.25x|2.47x|2.10x| -|`(a, *args, b:, **kwargs)`|2.18x|1.84x|1.93x|1.73x| +|`()` (none)|22.09x|2.35x|23.72x|2.60x| +|`(a)`|20.98x|14.43x|21.20x|12.20x| +|`(a, b)`|17.45x|12.94x|17.69x|11.13x| +|`(a:)`|29.80x|23.38x|25.17x|21.57x| +|`(a:, b:)`|27.00x|22.26x|23.30x|20.91x| +|`(a, b:)`|25.91x|21.20x|21.88x|19.51x| +|`(a, *args)`|3.07x|2.27x|3.17x|1.95x| +|`(a:, **kwargs)`|2.74x|2.28x|2.51x|2.10x| +|`(a, *args, b:, **kwargs)`|2.14x|1.84x|1.95x|1.72x| You can run benchmarks yourself with: diff --git a/lib/memo_wise.rb b/lib/memo_wise.rb index 6c9c67b..dace3d6 100644 --- a/lib/memo_wise.rb +++ b/lib/memo_wise.rb @@ -86,6 +86,14 @@ def extended(base) end private_constant(:CreateMemoWiseStateOnExtended) + module CreateMemoWiseStateOnInherited + def inherited(subclass) + MemoWise::InternalAPI.create_memo_wise_state!(subclass) + super + end + end + private_constant(:CreateMemoWiseStateOnInherited) + # @private # # Private setup method, called automatically by `prepend MemoWise` in a class. @@ -165,12 +173,11 @@ def memo_wise(method_name_or_hash) # This ensures that a memoized method defined on a parent class can # still be used in a child class. - klass.module_eval <<~HEREDOC, __FILE__, __LINE__ + 1 - def inherited(subclass) - super - MemoWise::InternalAPI.create_memo_wise_state!(subclass) - end - HEREDOC + if klass.is_a?(Class) && !klass.singleton_class? + klass.singleton_class.prepend(CreateMemoWiseStateOnInherited) + else + klass.prepend(CreateMemoWiseStateOnInherited) + end raise ArgumentError, "#{method_name.inspect} must be a Symbol" unless method_name.is_a?(Symbol) diff --git a/spec/memo_wise_spec.rb b/spec/memo_wise_spec.rb index 90a88f7..2a3f4b9 100644 --- a/spec/memo_wise_spec.rb +++ b/spec/memo_wise_spec.rb @@ -851,5 +851,119 @@ def no_args_counter expect(instance.no_args_counter).to eq(1) end end + + context "with target defined self.inherited" do + context "when target is class" do + let(:klass) do + Class.new do + prepend MemoWise + + def self.inherited(subclass) + super + subclass.instance_variable_set(:@inherited_called, true) + end + + def no_args + @no_args_counter = no_args_counter + 1 + end + memo_wise :no_args + + def no_args_counter + @no_args_counter ||= 0 + end + end + end + + it "calls defined self.inherited" do + inherited = Class.new(klass) + expect(inherited.instance_variable_get(:@inherited_called)).to be(true) + + instance = inherited.new + expect(Array.new(4) { instance.no_args }).to all(eq(1)) + expect(instance.no_args_counter).to eq(1) + end + + it "doesn't define #inherited" do + expect(klass).to be_respond_to(:inherited) + expect(klass.new).to_not be_respond_to(:inherited) + end + end + + context "when target is singleton class" do + let(:klass) do + Class.new do + class << self + prepend MemoWise + + def inherited(subclass) + super + subclass.instance_variable_set(:@inherited_called, true) + end + + def no_args + @no_args_counter = no_args_counter + 1 + end + memo_wise :no_args + + def no_args_counter + @no_args_counter ||= 0 + end + end + end + end + + it "calls defined self.inherited" do + inherited = Class.new(klass) + expect(inherited.instance_variable_get(:@inherited_called)).to be(true) + + expect(Array.new(4) { inherited.no_args }).to all(eq(1)) + expect(inherited.no_args_counter).to eq(1) + end + + it "doesn't define #inherited" do + expect(klass).to be_respond_to(:inherited) + expect(klass.new).to_not be_respond_to(:inherited) + end + end + + context "when target is module" do + let(:klass) do + mod = Module.new do + prepend MemoWise + + def inherited(subclass) + super + subclass.instance_variable_set(:@inherited_called, true) + end + + def no_args + @no_args_counter = no_args_counter + 1 + end + memo_wise :no_args + + def no_args_counter + @no_args_counter ||= 0 + end + end + + klass = Class.new + klass.extend(mod) + klass + end + + it "calls defined self.inherited" do + inherited = Class.new(klass) + expect(inherited.instance_variable_get(:@inherited_called)).to be(true) + + expect(Array.new(4) { inherited.no_args }).to all(eq(1)) + expect(inherited.no_args_counter).to eq(1) + end + + it "doesn't define #inherited" do + expect(klass).to be_respond_to(:inherited) + expect(klass.new).to_not be_respond_to(:inherited) + end + end + end end end