Skip to content

Fixed a bug that overwrote existing self.inherited method definitions. #326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
36 changes: 18 additions & 18 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand All @@ -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:

Expand Down
19 changes: 13 additions & 6 deletions lib/memo_wise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)

Expand Down
114 changes: 114 additions & 0 deletions spec/memo_wise_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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