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

Salsa derived functions currently don't support unionall argument types (where clauses don't work) #11

Closed
NHDaly opened this issue Apr 19, 2020 · 1 comment · Fixed by #31
Assignees
Labels
enhancement New feature or request macros

Comments

@NHDaly
Copy link
Collaborator

NHDaly commented Apr 19, 2020

I'm surprised we haven't noticed this until now, but simple where clauses don't work right now:

Salsa.@derived multi_method(db::AbstractComponent, ::Type{T}) where T = sizeof(T)

The reason is because we are currently evaluating all the argument types to figure out what type of dictionary and key type to create for this function. We are currently using the (typeof(f) and Tuple{Type}) to represent individual methods like this, and so we were calling eval for each argument type in order to build that tuple, but if you attempt to eval(Type{T}), of course T is not defined.

There are a few approaches to take that would fix this:

  1. Move the Key construction into the function body, instead of in the macro body, where the types are available. But we were doing it in the macro body because constructing the Dict() itself seemed to be expensive, and we were hoping to precompute this in the macro.
  2. We should consider simplifying how we use these types for keys:
    • Maybe we should stop using separate dictionaries for each derived function, and just use one big dictionary for all derived functions?
    • And then, we can also stop keying the derived values by DependencyKey(key=DerivedKey{F, Tuple{Int,Any}}, args=(2,"hi")) and instead just key it by F and the actual args themselves, something like: DependencyKey{Derived, F}(2,"hi").
      • I don't think we actually need to keep track of which method was called, because julia can track that itself! The only problem would be if a user defines a new, more specific method after starting to use a Salsa state, in which case the key would incorrectly carry over to the new function, but I don't think that's a supported mode anyway.
@NHDaly
Copy link
Collaborator Author

NHDaly commented Apr 19, 2020

Discovered this in #10, while fixing #9.

@NHDaly NHDaly self-assigned this Apr 19, 2020
@NHDaly NHDaly added enhancement New feature or request macros labels Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request macros
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant