Re-use document symbols functionality for workspace symbols #683

mhanberg · 2022-03-24T04:27:16Z

Previously, the workspace symbols feature used the :code.all_loaded functionality of the OTP standard library to "discover" all modules, and then was able to parse out the different type of symbols from that.

In doing so, this loaded tons of symbols that are completely unrelated to your workspace. As a prior user of the document symbols feature, this really threw me off. As I understand it, the workspace symbols support is for quick navigation of functions, modules, types in your own codebase, so if the source code is not available in your own repository, you really wouldn't be navigating to it.

The workspace symbols feature was also implemented differently than the document symbols feature. This commit works to align the two features to make them work similarly.

In addition, this also allows you to pass an empty query to LSP request in order to retrieve a full list of all of the symbols. This aligns the Elixir implementation with the spec, and makes it useful for those who use a fuzzy searching plugin in their editor to do the querying.

Here are two videos demonstrating the differences between the current and (potentially) new behavior. I uploaded the to YouTube as the vides were over 10MB.

Current Behavior: https://youtu.be/PUkykG7IK8I
New Behavior: https://youtu.be/7CmKaGyTfqk

Notes

I believe this is technically a breaking change, as the results returned in the request are slightly different than they were before.
With the previous implementation, I found that in some codebases (like https://github.com/mhanberg/temple), no symbols were being returned for the project. I suspect it had something to do with the library not having an Application, so no code is loaded at first. The videos uploaded demonstrate it with the Wallaby codebase, which does use an Application however, so I'm not entirely sure why.
I have tested this on the Wallaby codebase (https://github.com/elixir-wallaby/wallaby) and it seems to return instantly, so I don't think there should be any kind of performance regression, but it would be worth checking against larger codebases.
I have also tested this on the aws-elixir codebase, which has about 235kloc, and the performance is very good.

PS

I understand this is a significant and unsolicited change, so thank you in advance for reviewing this patch and considering it for merging.

mhanberg · 2022-03-24T04:43:50Z

The tests pass locally for me on the failed job (elixir 1.13 and otp 24.1), but i can't test the otp 22 failure because i am on mac (I think only 23+ works on mac now a days).

I think it might have been a dialyzer timeout or something, but I can't restart the failed jobs myself it seems.

mhanberg · 2022-03-24T15:09:38Z

I will be testing this in the VSCode extension soon.

I think that the neovim lsp client might be forgiving in whether it sees a DocumentSymbol or a SymbolInformation, whereas there is a chance that VSCode might be more strict.

mhanberg · 2022-03-25T02:00:37Z

seems fine

mhanberg · 2022-03-29T04:39:11Z

All builds green now 😄

lukaszsamson · 2022-04-02T09:50:26Z

apps/language_server/lib/language_server/providers/workspace_symbols.ex

+    symbols =
+      for file <- paths, into: %{} do
+        file = Path.absname(file)
+        uri = "file://#{file}"


use path to URI convert function from SourceFile

lukaszsamson · 2022-04-02T09:53:32Z

apps/language_server/lib/language_server/providers/workspace_symbols.ex

-  end
+    symbols =
+      for uri <- uris, into: state.symbols do
+        file = URI.parse(uri).path


us URI to path conversion from SourceFile

lukaszsamson · 2022-04-02T10:23:10Z

Nice work @mhanberg but I'm not completely convinced we should go this road. Let me state my reasons.

Relying on DocumentSymbols provider is in fact relying on AST parsing and hence it's severely limited in understanding macro generated code. The introspection based on loaded modules approach on the other hand is able to extract complete and accurate information about modules/functions/types. Moreover, with elixir >= 1.10 now required and growing support from the upstream it's highly likely elixir-LS features will start migrating to compiler tracers.
Yes, the current approach loads a lot of modules but the fact it does so means that your project (or elixir-LS itself) is using those dependencies (or at least loads OTP apps with those modules). Wether those modules should be returned is another thing and other language servers take various approaches. I thing it would be best if it was configurable. See Change workspace symbols to only look at modules in the current application #261
I know it's not an extremely important feature but we lose ability to jump to erlang source files in elixir projects.
Empty query is not supported currently only due to performance reasons (List all workspace symbols when an empty query comes in #603). With scope limited to mix project it would probably be fine.

Anyway, I plan to play around with it and see how it works in practice

mhanberg · 2022-04-02T18:02:05Z

@lukaszsamson thank you for taking the time to look over this patch, I really appreciate it.

Relying on DocumentSymbols provider is in fact relying on AST parsing and hence it's severely limited in understanding macro generated code.

This is something I was thinking about when writing this patch. I am trying to think of the utility of navigating to functions that don't exist in the source code. I figure you'd rather go to the actual source code (the macro that generates the code) than the module where it gets injected.

Small example, but I'm not certain the value i'd get from the action/2 function that get injected into every Phoenix controller showing up in the list of symbols.

Is there a useful example that you use regularly?

Moreover, with elixir >= 1.10 now required and growing support from the upstream it's highly likely elixir-LS features will start migrating to compiler tracers.

That seems cool to me. 👍

Yes, the current approach loads a lot of modules but the fact it does so means that your project (or elixir-LS itself) is using those dependencies (or at least loads OTP apps with those modules). Wether those modules should be returned is another thing and other language servers take various approaches. I thing it would be best if it was configurable. See #261

I think configuration is a valid point (while, not applicable with this patch, as it can't pull in functions/modules/types that are used in the project but not present in the source code).

One of the things I noticed which instigated me writing this patch was that the symbols didn't seem to show up at all, except for some (to me, random) OTP functions. You can observe this in the screen recording I shared.

Do you know why that is?

I know it's not an extremely important feature but we lose ability to jump to Erlang source files in elixir projects.

I think this is on oversight in the patch. I think this could possibly be remedied. Thank you for pointing it out 👍.

Thanks again for reviewing this patch. I appreciate all the comments and for the consideration.

I have been using this branch for the last week at work and have been enjoying it, let me know what you think after using it.

mhanberg · 2022-07-07T12:32:18Z

I'm going to try and port the workspace symbols to compiler tracers in the next week or so.

lukaszsamson · 2022-07-07T13:21:49Z

I already have a PoC branch with DocumentSymbols using a database built by compile tracers. It looks promising but requires more work and integration with current AST based approach. Currently compile tracers can only get high quality info only about modules and defs and some limited info about attributes. Anything more than that requires loading the module and doing the usual introspection that WorkspaceSymbols currently do.

mhanberg · 2022-07-07T14:22:24Z

Gotcha, thanks for the update 🚀

Previously, the workspace symbols feature used the `:code.all_loaded` functionality of the OTP standard library to "discover" all modules, and then was able to parse out the different type of symbols from that. In doing so, this loaded _tons_ of symbols that are completely unrelated to your workspace. As a prior user of the document symbols feature, this really threw me off. As I understand it, the workspace symbols support is for quick navigation of functions, modules, types in your own codebase, so if the source code is not available in your own repository, you really wouldn't be navigating to it. The workspace symbols feature was also implemented differently than the document symbols feature. This commit works to align the two features to make them work similarly. In addition, this also allows you to pass an empty query to LSP request in order to retrieve a full list of all of the symbols. This aligns the Elixir implementation with the spec, and makes it useful for those who use a fuzzy searching plugin in their editor to do the querying. _Notes_ - I believe this is technically a breaking change, as the results returned in the request are slightly different than they were before. I believe them to be _more_ accurate now, but they are technically different. - With the previous implementation, I found that in some codebases (like https://github.com/mhanberg/temple), no symbols were being returned for the project. I suspect it had something to do with the library not having an `Application`, so no code is loaded at first. - I have tested this on the Wallaby codebase (https://github.com/elixir-wallaby/wallaby) and it seems to return instantly, so I don't think there should be any kind of performance regression, but it would be worth checking against larger codebases. I plan on trying this branch on my work projects on my next working day.

This was meant to be a normal `<-`.

lukaszsamson · 2024-01-08T22:30:49Z

#1050 should improve workspace symbols experience for now. In the future I plan to build a common database/index layer. If that happens parts of this PR may be yet reused

lukaszsamson reviewed Apr 2, 2022

View reviewed changes

mhanberg force-pushed the mh/all-workspace-symbols branch from cd06c18 to c429111 Compare May 26, 2022 15:55

mhanberg force-pushed the mh/all-workspace-symbols branch from c429111 to ba4371a Compare June 7, 2022 13:55

mhanberg force-pushed the mh/all-workspace-symbols branch from ba4371a to a37ab53 Compare August 9, 2022 22:18

mhanberg added 5 commits September 5, 2022 15:25

Gracefully handle a failed call to DocumentSymbols.symbols/3

1887a3c

Trigger Build

8bdc0cc

Fix mistaken match

41beeb1

This was meant to be a normal `<-`.

Update tests

b66d65c

mhanberg force-pushed the mh/all-workspace-symbols branch from a37ab53 to b66d65c Compare September 5, 2022 19:25

lukaszsamson closed this Jan 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-use document symbols functionality for workspace symbols #683

Re-use document symbols functionality for workspace symbols #683

mhanberg commented Mar 24, 2022

mhanberg commented Mar 24, 2022

mhanberg commented Mar 24, 2022

mhanberg commented Mar 25, 2022

mhanberg commented Mar 29, 2022

lukaszsamson Apr 2, 2022

lukaszsamson Apr 2, 2022

lukaszsamson commented Apr 2, 2022

mhanberg commented Apr 2, 2022

mhanberg commented Jul 7, 2022

lukaszsamson commented Jul 7, 2022

mhanberg commented Jul 7, 2022

lukaszsamson commented Jan 8, 2024

Re-use document symbols functionality for workspace symbols #683

Re-use document symbols functionality for workspace symbols #683

Conversation

mhanberg commented Mar 24, 2022

mhanberg commented Mar 24, 2022

mhanberg commented Mar 24, 2022

mhanberg commented Mar 25, 2022

mhanberg commented Mar 29, 2022

lukaszsamson Apr 2, 2022

Choose a reason for hiding this comment

lukaszsamson Apr 2, 2022

Choose a reason for hiding this comment

lukaszsamson commented Apr 2, 2022

mhanberg commented Apr 2, 2022

mhanberg commented Jul 7, 2022

lukaszsamson commented Jul 7, 2022

mhanberg commented Jul 7, 2022

lukaszsamson commented Jan 8, 2024