Fix usage in module singleton class

[PikachuEXE]

Before merging:

• Copy the latest benchmark results into the README.md and update this PR
• If this change merits an update to CHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning

This would fix something like this:

require "memo_wise"

module TestModule1
  class << self
    prepend ::MemoWise

    def test
      "test__#{Time.now.to_i}"
    end
    memo_wise :test
  end
end

TestModule1.test

[codecov]

Codecov Report

Merging #185 (573ceaf) into main (1f56991) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 573ceaf differs from pull request most recent head b7c7cb8. Consider uploading reports for the commit b7c7cb8 to get more accurate results

@@            Coverage Diff            @@
##              main      #185   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          169       173    +4     
=========================================
+ Hits           169       173    +4     

Impacted Files	Coverage Δ
lib/memo_wise.rb	100.00% <100.00%> (ø)
lib/memo_wise/internal_api.rb	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f56991...b7c7cb8. Read the comment docs.

[jemmaissroff]

Thank you so much for the PR, it looks really polished! We left a few questions. It looks like RuboCop failed if you wouldn't mind fixing the linting errors. And let us know if you'd like help digging into the JRuby rspec failures.

[PikachuEXE]

Updated but can't rerun CI

[jemmaissroff]

Thanks for the next round, a few more comments. It looks like this is failing Rubocop still, to run rubocop locally before submitting the next round, you can run bundle exec rubocop.

Also - do you know why this is failing JRuby specs?

[PikachuEXE]

I have fixed most issues except JRuby one
I have never used JRuby once so I have no clue of the cause(s)

Update:
I can't even install all the dependencies in JRuby (jruby-9.2.9.0 via RVM)
I commented it when testing with JRuby locally

[PikachuEXE]

Here is a short code snippet manually created to reproduce the JRuby issue:

require "memo_wise"
# Same outcome for `module TestModule1`, using `Module.new` to be the same as spec
TestModule1 = Module.new do
  prepend MemoWise

  self.module_eval <<~END_OF_METHOD, __FILE__, __LINE__ + 1
    def no_args
      @no_args_counter = no_args_counter + 1
      "no_args"
    end
    memo_wise :no_args
  END_OF_METHOD
end

Update 1: However if I run module TestModule1 twice, no error would be raised on 2nd time (only)
Update 2: Running TestModule1 = Module.new do would raise error twice
Update 3:

Test Code to be run in pry --gem

require "memo_wise"
TestModule1 = Module.new do
  prepend MemoWise

  def no_args
    @no_args_counter = no_args_counter + 1
    "no_args"
  end
  memo_wise :no_args
end

Temp updated code for debugging in memo_wise

api = MemoWise::InternalAPI.new(klass)
visibility = api.method_visibility(method_name)
original_memo_wised_name =
  MemoWise::InternalAPI.original_memo_wised_name(method_name)
method = klass.instance_method(method_name)

klass.send(:alias_method, original_memo_wised_name, method_name)
puts(klass.class.name)
puts(method_name)
puts(original_memo_wised_name)
puts(klass.instance_method(method_name))
puts(klass.instance_methods(false).include?(original_memo_wised_name))
puts(klass.instance_method(original_memo_wised_name))
klass.send(:private, original_memo_wised_name)

Result from JRuby

$ ruby -v
jruby 9.2.19.0 (2.5.8) 2021-06-15 55810c552b OpenJDK 64-Bit Server VM 11.0.11+9 on 11.0.11+9 +jit [darwin-x86_64]

Module
no_args
_memo_wise_original_no_args
#<UnboundMethod: #<Module:0x7a0f06ad>#no_args>
true
NameError: undefined method `_memo_wise_original_no_args' for class `#<Module:0x7a0f06ad>'
from org/jruby/RubyModule.java:2909:in `instance_method'

Result from MRI 3.0

$ ruby -v
ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin20]

Module
no_args
_memo_wise_original_no_args
#<UnboundMethod: #<Module:0x00007ff31b3db600>#no_args() (pry):4>
true
#<UnboundMethod: #<Module:0x00007ff31b3db600>#_memo_wise_original_no_args(no_args)() (pry):4>
=> TestModule1

Result from JRuby Head

$ ruby -v
jruby 9.3.0.0-SNAPSHOT (2.6.5) 2021-07-20 00ddf5da27 OpenJDK 64-Bit Server VM 11.0.11+9 on 11.0.11+9 +jit [darwin-x86_64]

Module
no_args
_memo_wise_original_no_args
#<UnboundMethod: #<Module:0x21ce2e4d>#no_args>
true
NameError: undefined method `_memo_wise_original_no_args' for class `#<Module:0x21ce2e4d>'
from org/jruby/RubyModule.java:2995:in `instance_method'

[ms-ati]

LGTM

[ms-ati]

Thanks so much @PikachuEXE.

Quick question: Do we need test coverage that ensures that two different Classes that extend the same Module do not actually share the same cache for the same memoized method?

[JacobEvelyn]

Thanks for the detailed JRuby investigation, @PikachuEXE! @jemmaissroff and I continued your investigation and ended up reporting jruby/jruby#6758 as a result.

In the meantime I think it's okay to merge this once @ms-ati's question is addressed (we can resolve the git conflicts on our end).

[PikachuEXE]

Test case for 2 classes extending 1 module with memoized method(s) added
Benchmark results also updated in README without merge conflict
But note that some numbers for memery are ratio to fastest implementation.
In some cases the fast implementation is dry-core not memo_wise but I have only copied the ratio from result for now.

[PikachuEXE]

Will this be merged before JRuby fix jruby/jruby#6758 ?

[ms-ati]

Are there paths forward that we can merge this before jruby/jruby#6758 is fixed, but not have all of our builds fail from now on?

[PikachuEXE]

Just updated those "failing on JRuby only" test cases to be skipped on JRuby.
I have tried running rspec on MRI & JRuby locally and both run have no failed test cases reported.

[ms-ati]

I've fixed two Rubocop issues and some mis-named let variables -- in my view this is ready to merge! Yay!

@jemmaissroff are we clear to merge? Currently Github thinks you need to re-review?

[jemmaissroff]

@jemmaissroff are we clear to merge? Currently Github thinks you need to re-review?

Yes! Looks good to me, sorry I didn't mean to be a blocker. Merging now!

[PikachuEXE]

Hope this get released soon so that I can finally start switching to this gem.
I was blocked due to the issues this PR solves.

[jemmaissroff]

Hope this get released soon so that I can finally start switching to this gem.
I was blocked due to the issues this PR solves.

MemoWise v1.1.0 is now released!