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

memoize doesn't account for method overrides #1388

Open
jwoertink opened this issue Jan 11, 2021 · 4 comments
Open

memoize doesn't account for method overrides #1388

jwoertink opened this issue Jan 11, 2021 · 4 comments
Labels

Comments

@jwoertink
Copy link
Member

I ran in to this random issue where I have 2 methods that I wanted to memoize both, but one takes an additional arg that the other doesn't.

memoize def account : Account
  Account.new(self)
end

memoize def account(date_range : Tuple(Time, Time)) : Account
  Account.new(self, date_range)
end
Error: instance variable '@__memoized_account' of must be (Tuple(Account, Tuple(Time, Time)) | Nil), not Tuple(Account)

If we can figure out how to handle the method overrides, that would be cool, though not at the cost of performance, or massive memory allocation like I'd expect it might lead to. I'm also cool with just raising a compile-time error (if possible) that says you can't memoize the second method because it's already being memoized as account

@jwoertink jwoertink added the bug label Jan 11, 2021
@matthewmcgarvey
Copy link
Member

It would probably be easiest to use the number of arguments of the method in the method name.

If no arguments are given, everything would use _0 at the end

@jwoertink
Copy link
Member Author

That could work. Though, if we do that, then we should also see about raising an error if you try to define one that clashes.

memoize def account(number : String)
end

memoize def account(number : Int32)
end

akadusei added a commit to GrottoPress/shield that referenced this issue Jul 30, 2021
Memoize has [issues][issues], and I've never really had a use case
for memoizing a method that has arguments.

See luckyframework/lucky#1388

[issues]: luckyframework/lucky#1396
@jwoertink jwoertink added the hacktoberfest Valid Issue for Hacktoberfest label Oct 1, 2021
@NotWearingPants
Copy link
Contributor

NotWearingPants commented Oct 2, 2021

The best way is to just

{{ name = UUID.random.hexstring }}
@__memoized_{{name}}

or something like @__memoized_{{Sha1(method_def.body)}}
but I could not for the life of me figure out how Crystal's annoying macro system works. It just won't let me call UUID.

Another way is to keep a counter which just increments with every call to memoize and do @__memoized_{{counter}}, but again Crystal doesn't seem to let me. Tried prefixing with nothing, or $, or @@.

Another way is to define the variable inside a closure and not have to worry about a unique name, but I couldn't figure it out either.

@jwoertink
Copy link
Member Author

@NotWearingPants Thanks for the suggestion. I like that idea of just creating a hash and using that as the differentiator.

And yeah, the macro system can be quite tricky if you're not used to working with it. Thanks again!

@jwoertink jwoertink removed the hacktoberfest Valid Issue for Hacktoberfest label Nov 1, 2021
@jwoertink jwoertink added the hacktoberfest Valid Issue for Hacktoberfest label Sep 30, 2022
@jwoertink jwoertink removed the hacktoberfest Valid Issue for Hacktoberfest label Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants