memoist icon indicating copy to clipboard operation
memoist copied to clipboard

Memoization broken for optional args

Open ashu210890 opened this issue 7 years ago • 1 comments

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.

  1. 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.).

ashu210890 avatar Jul 17 '18 17:07 ashu210890

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:

  • https://github.com/makandra/memoized
  • https://github.com/honzasterba/memoist
  • https://github.com/AlexWayfer/alt_memery

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

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

pboling avatar Jun 04 '24 07:06 pboling