Do not consider remote typespecs as a compile-time dependency #5093
Conversation
ericentin
changed the title
|
Ok, even though the test failed as expected, it wasn't the failure I was expecting. If preferred, let me do some more investigation as to why it fails locally on current master but not in CI before merging. I have manually verified the fix with #5087's test repository however, so at the least the issue is no longer present if we feel good about merging as is. |
|
Ok, tried to be all fancy and leave a paper trail, but it turns out that the PR is checked out in CI by the PR #, so by the time the builds actually started, I had already pushed the second commit. I have verified locally that the test fails on current master, so this PR actually fixes an existing issue. I think it's ready to |
| remote = Macro.expand remote, caller | ||
| # We set a function name to avoid tracking | ||
| # aliases in typespecs as compile time dependencies. | ||
| remote = Macro.expand remote, %{caller | function: {:typespec, 0}} |
There was a problem hiding this comment.
I thought the fix would be something exactly like that. :D
Should we do __info__ or __typespec__ as the function name though? Another option is to disable the lexical tracker since it is a field in the environment and that would disable the tracking.
There was a problem hiding this comment.
There is some existing code that uses this approach here:
https://github.com/antipax/elixir/blob/99a911aee81ce634f0725e274b2ea7ed131799b1/lib/elixir/lib/kernel/typespec.ex#L853
I think it's still a good idea to track the reference as runtime, so personally I'd be against disabling the lexical tracker, unless we want to consider types as existing outside of normal code. I don't think we need to change the env function name since for tracking purposes it's only used in terms of presence vs. nil to determine whether it's compile or runtime.
|
❤️ 💚 💙 💛 💜 |
Signed-off-by: José Valim <jose.valim@plataformatec.com.br>
Fixes #5087.
Also adds some comments to the cases in which a compile time dependency must be added for a typespec (structs, records). Perhaps the documentation should be altered to suggest using a remote type instead of a struct or record macro to avoid this case, if possible.
Also, I am pushing a commit with the added test first, to show that it fails on the current master. I will then immediately push the commit with the actual fix.