Skip to content

Commit

Permalink
Fixed a bug that overwrote existing self.extended method definitions.
Browse files Browse the repository at this point in the history
There is a hack that dynamically defines self.extended when memo_wise is called, but there was a bug that did not consider the case where an existing self.extended existed, so this has been fixed.
  • Loading branch information
alpaca-tc committed Jan 24, 2024
1 parent aae31f2 commit 20fe5fe
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 22 deletions.
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

_No breaking changes!_

**Project enhancements:** none
**Project enhancements:**

- Fixed a bug that overwrote existing self.extended method definitions. [[#324]](https://github.com/panorama-ed/memo_wise/pull/314)

## [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.54x|3.62x|
|`(a)`|1.54x|7.74x|
|`(a, b)`|1.29x|5.58x|
|`(a:)`|1.55x|12.37x|
|`(a:, b:)`|1.15x|8.76x|
|`(a, b:)`|1.15x|8.75x|
|`(a, *args)`|0.84x|1.54x|
|`(a:, **kwargs)`|0.79x|2.13x|
|`(a, *args, b:, **kwargs)`|0.69x|1.38x|
|`()` (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|

\* `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)|24.22x|2.42x|26.43x|2.70x|
|`(a)`|22.08x|15.19x|22.03x|13.67x|
|`(a, b)`|19.12x|14.01x|18.60x|12.80x|
|`(a:)`|30.21x|24.29x|26.43x|23.11x|
|`(a:, b:)`|27.73x|22.97x|25.11x|21.89x|
|`(a, b:)`|26.87x|22.76x|23.71x|21.26x|
|`(a, *args)`|3.15x|2.30x|3.18x|2.11x|
|`(a:, **kwargs)`|2.89x|2.40x|2.69x|2.28x|
|`(a, *args, b:, **kwargs)`|2.12x|1.82x|1.96x|1.74x|
|`()` (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|

You can run benchmarks yourself with:

Expand Down
12 changes: 9 additions & 3 deletions lib/memo_wise.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ def initialize(#{all_args})
end
HEREDOC

module CreateMemoWiseStateOnExtended
def extended(base)
MemoWise::InternalAPI.create_memo_wise_state!(base)
super
end
end
private_constant(:CreateMemoWiseStateOnExtended)

# @private
#
# Private setup method, called automatically by `prepend MemoWise` in a class.
Expand Down Expand Up @@ -136,9 +144,7 @@ def memo_wise(method_name_or_hash)
#
# On method call `@_memo_wise` would still be `nil`
# causing error when fetching cache from `@_memo_wise`
def klass.extended(base)
MemoWise::InternalAPI.create_memo_wise_state!(base)
end
klass.singleton_class.prepend(CreateMemoWiseStateOnExtended)
end
when Hash
unless method_name_or_hash.keys == [:self]
Expand Down
32 changes: 32 additions & 0 deletions spec/memo_wise_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -819,5 +819,37 @@ def test_method
end
end
end

context "with module defined self.extended" do
let(:module_with_memo) do
Module.new do
prepend MemoWise

def self.extended(base)
base.instance_variable_set(:@extended_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.extended" do
klass = Class.new
instance = klass.new
instance.extend(module_with_memo)

expect(instance.instance_variable_get(:@extended_called)).to be(true)

expect(Array.new(4) { instance.no_args }).to all(eq(1))
expect(instance.no_args_counter).to eq(1)
end
end
end
end

0 comments on commit 20fe5fe

Please sign in to comment.