Support finding function definition defined in __using__ macro#330
Support finding function definition defined in __using__ macro#330katafrakt wants to merge 5 commits intoelixir-lsp:masterfrom
__using__ macro#330Conversation
This adds heuristic to find the function source location, even if it's defined insige the `__using__` macro. It supports the following cases: 1. When current module has `use OtherMod` and the function is defined in `OtherMod`'s `__using__` - it correctly points to a relevant line in `OtherMod`. 2. When `MyMod` has `use OtherMod` (as above) and `YetAnotherMod` calls `MyMod.function_defined_in_using`. This basically covers things like `MyApp.Repo.all`, where `Repo` has `use Ecto.Repo` - now ElixirSense correctly finds the definition of `all` inside `Ecto.Repo`. This is not a perfect solution, as it uses regular expressions and heuristic for find the definition and it might something be wrong. However, this covers quite important use cases in language servers, so in my opinion it's worth having, even if it's not 100% precise.
Fallbacks for situations where there was no debug info on the module were a bit too defensive IMO. They make a huge sacrifice in code readability for a very unlikely win. Removing them to keep the code cleaner.
|
|
||
| {node, {[full_parts | stack], modules}} | ||
|
|
||
| {:use, _meta, [module_ast | _]} = node, {[current_parts | _] = stack, modules} -> |
There was a problem hiding this comment.
That's a brittle approach and it only work in simplest cases. It will fail to catch Kernel.use invocations or will false positive match on local calls and variables. What is needed here is AST expansion aware of imports, remote calls and environment, not simple traversal. Look into Compiler module - it does exactly that but it may be an overkill for this use case. Basically, you need to expand the AST, looking for local or remote calls expanding to Kernel.use and then proceed on the subnode in arguments. See Macro.Env API and a simple expander https://github.com/elixir-tools/spitfire/blob/main/lib/spitfire/env.ex
There was a problem hiding this comment.
Unfortunately, even that will not work with dynamic code that generates defs. This is not a full compiler and compiling dynamic modules requires evaluating source code during compilation
There was a problem hiding this comment.
Thanks, I'll look into expanding the AST.
There was a problem hiding this comment.
I went with using Compiler - and it did not feel like an overkill. It turned out quite nice IMO and removed this ugly traversal code. I also added tests for the cases you mentioned. What do you think?
|
@lukaszsamson sorry for pinging, but do you think it's a direction worth pursuing in general? We are using the code from the PR at my company via Expert and it doesn't seem to cause any problem, so I'm wondering if there's a change to get this merged upstream. I'm okay if you think it's not good. There are probably other ways to achieve similar effect, perhaps via using Spitfire directly in Expert, without touching ElixirSense. I just wanted to align with you on the future of this approach. |
|
Sorry for the delay, I forgot about the PR. I'll look into it again |
2 participants
This adds heuristic to find the function source location, even if it's defined insige the
__using__macro. It supports the following cases:When current module has
use OtherModand the function is defined inOtherMod's__using__- it correctly points to a relevant line inOtherMod.When
MyModhasuse OtherMod(as above) andYetAnotherModcallsMyMod.function_defined_in_using. This basically covers things likeMyApp.Repo.all, whereRepohasuse Ecto.Repo- now ElixirSense correctly finds the definition ofallinsideEcto.Repo.This is not a perfect solution, as it uses regular expressions and heuristic for find the definition and it might something be wrong. However, this would enable Expert to have working go-to-definition in quite (in my opinion) important route. See katafrakt/expert#1 where I test these changes in Expert.
Note: I tried to make the code relatively readable, sometime cutting out unlikely cases (a153d89), but there is still this huge
Macro.traverse, which is kind of intimidating. Maybe I'm missing something obvious here to make it better.