matthewrudy / memoist

ActiveSupport::Memoizable with a few enhancements
MIT License
919 stars 99 forks source link

Memoization broken for optional args #76

Open ashu210890 opened 6 years ago

ashu210890 commented 6 years ago

Memoization is currently broken for methods with optional args. The piece of code under question is:

if args.length == method.arity.abs + 1 && (args.last == true || args.last == :reload)
  reload = args.pop

There are a couple of things wrong with the this piece of code.

  1. method.arity only represents total number of positional args. 1 is added and the arity is negated if there is an optional/named argument. We should not be depending on method arity and picking a particular positional argument because it's impossible to tell whether the intent was to pass a value for the optional arg or true flag to memoist. e.g.

    def self.foo(a, b=1, c=2, d=3, e:4)
    end

    In this case the arity of foo will be -2.

  2. Because of what's mentioned above, this not only misses the reload flag but also incorrectly modifies the arguments of a method that was only passed a true value for one of the optional args.

We should use a special named parameter memoist_reload always and not depend on arity at all. We could still use arity when it's positive for backward compatibility (because that's the only case when memoization is actually working even now.).

pboling commented 2 months ago

FYI: Added this alert to the new memoist repo

[!IMPORTANT]

Recommendation

Consider using MemoWise instead, as it is maintained, fully tested, provides thread safety guarantees, and is much, much faster.

Other Alternatives

In case you need a tool with this feature set that is currently maintained, check out:

[!TIP]
Seriously though, read the important note above.

[!WARNING]
If you must continue - be aware that this is unmaintained software.