Skip to content
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

Memoization broken for optional args #76

Open
ashu210890 opened this issue Jul 17, 2018 · 0 comments · May be fixed by #75
Open

Memoization broken for optional args #76

ashu210890 opened this issue Jul 17, 2018 · 0 comments · May be fixed by #75

Comments

@ashu210890
Copy link

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 ashu210890 linked a pull request Jul 17, 2018 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant