Change workspace symbols to only look at modules in the current application

[axelson]

This improves speed considerably since many more modules are not processed. If the old behavior is preferred by some users we could investigate making it configurable.

However, I think that this behavior is in line with LSP expectation:

The workspace symbol request is sent from the client to the server to list project-wide symbols matching the query string.

from:
https://microsoft.github.io/language-server-protocol/specification#workspace_symbol

@lukaszsamson I'm curious if you have any strong feelings on this. Personally I haven't been using the workspace symbols request because I want to look only at symbols in my project and right now it is too noisy.

[lukaszsamson]

Is it umbrella friendly?

[axelson]

Unfortunately not, we will need to find an umbrella-friendly alternative

[lukaszsamson]

@axelson Go ahead, I don't have a strong opinion on this. To be honest It was easier to get all modules working by public erlang APIs than private Mix APIs.

[axelson]

So there's definitely a race condition in this PR:

02:33:06.054 [error] GenServer ElixirLS.LanguageServer.Providers.WorkspaceSymbols terminating

** (KeyError) key :app not found in: [aliases: [], build_embedded: false, build_per_environment: true, build_scm: Mix.SCM.Path, config_path: "config/config.exs", consolidate_protocols: true, default_task: "run", deps: [], deps_path: "deps", elixirc_paths: ["lib"], erlc_paths: ["src"], erlc_include_path: "include", erlc_options: [:debug_info], lockfile: "mix.lock", preferred_cli_env: [], start_permanent: false]

It appears that Mix.Project.config() is sometimes returning the default config instead of the loaded application config. Might be related to the use of fixtures and project stack peek/pop.

[axelson]

The problem with the tests is that ElixirLS.Utils.MixTest.Case.in_fixture compiles without a mix project which isn't really something that ElixirLS generally supports. So in the current tests the build is actually failing with

mixfile error [%Mix.Task.Compiler.Diagnostic{compiler_name: "ElixirLS", details: nil, file: "/home/jason/dev/forks/elixir-ls/apps/elixir_ls_utils/test/tmp/ElixirLS.LanguageServer.ServerTest/test incremental formatter/mix.exs", message: "No mixfile found in project. To use a subdirectory, set elixirLS.projectDir in your settings", position: nil, severity: :error}]

But since the test is about formatting (this example is from the server "incremental formatter" test), the error has previously gone un-noticed. I think in general we want to revamp how the fixtures work, the current setup is not maintainable and has other issues.

[dgutov]

Logically speaking, I would expect it list the symbols from the same set that "find definition" can reach. Which for languages like Elixir (or Ruby) probably includes libraries. But the problem of too many results is understandable.

So I was curious how other language servers address this.

ccls, which I fully expected to limit itself to symbols within the project, actually includes symbols from system/stdlib header files. And to limit the number of results they introduce both a configuration option and a non-standard parameter folders: MaskRay/ccls#189 (comment). Apparently Neovim's client can use the latter.

eclipse.jdt.ls also includes entries from all libs on the classpath: eclipse-jdtls/eclipse.jdt.ls@26babf4 (see the link to the issue and the discussion that requested this behavior). Though they only limit that to clients that support jdt:// urls: eclipse-jdtls/eclipse.jdt.ls@fdfde47. No folders parameter, however. OTOH, this LS only includes classes in the results, not methods. But this projects does that too, absent an explicit qualifier.

Speaking of filtering, would it help if "functions search" only matched function names, unless the query string includes a .?

[axelson]

Thanks for looking at how other implementations handle this. Maybe this should only be merged once it can be configurable.

Speaking of filtering, would it help if "functions search" only matched function names, unless the query string includes a .?

Can you give an example of when the functions search matches something that is not a function name?

[dgutov]

Can you give an example of when the functions search matches something that is not a function name?

I search for f limit and get results like these (sorry about the sexp notation):

(:kind 12 :location
       (:range
        (:end
         (:character 0 :line 43)
         :start
         (:character 0 :line 42))
        :uri "file:///home/dgutov/abs/xyz/deps/ecto/lib/ecto/query/builder/limit_offset.ex")
       :name "f Ecto.Query.Builder.LimitOffset.apply/3")
(:kind 12 :location
       (:range
        (:end
         (:character 0 :line 8)
         :start
         (:character 0 :line 7))
        :uri "file:///home/dgutov/abc/xyz/deps/ecto/lib/ecto/query/builder/limit_offset.ex")
       :name "f Ecto.Query.Builder.LimitOffset.build/5")
(:kind 12 :location
       (:range
        (:end
         (:character 0 :line 1)
         :start
         (:character 0 :line 0))
        :uri "file:///home/dgutov/abc/xyz/deps/ecto/lib/ecto/query/builder/limit_offset.ex")
       :name "f Ecto.Query.Builder.LimitOffset.module_info/1")

I guess it could be matching additional things (like docstring contents and parameter names -- good candidates for the ability to turn off, BTW!), but the module_info/1 entry seems definitely out of place.

[dgutov]

And another observation.

Backstory: the Emacs client I'm using doesn't show the value of the :name field from the response directly. Instead, in shows the file name and its contents on the specified line.

So, for example, for the query f limit it shows a section like this:

/home/dgutov/abc/xyz/deps/ecto/lib/ecto/query.ex
   1: defmodule Ecto.SubQuery do
1595:   @doc """

Which is a bit confusing. The presentation can be improved, but let's set that aside for now.

Looking at the request-response log, the corresponding entries were:

(:kind 12 :location
       (:range
        (:end
         (:character 0 :line 1595)
         :start
         (:character 0 :line 1594))
        :uri "file:///home/dgutov/abc/xyz/deps/ecto/lib/ecto/query.ex")
       :name "f Ecto.Query.limit/3")
(:kind 12 :location
       (:range
        (:end
         (:character 0 :line 1)
         :start
         (:character 0 :line 0))
        :uri "file:///home/dgutov/abc/xyz/deps/ecto/lib/ecto/query.ex")
       :name "f Ecto.Query.limit/2")

The two problems I see here:

• The first entry specifies :line 1, which led me to believe it found the module itself, and not the function in it.
• The entries duplicate each other. There's no need to list the two overloads of one function (or macro, actually) separately. It's just a macro with one optional argument:

  defmacro limit(query, binding \\ [], expr) do
    Builder.LimitOffset.build(:limit, query, binding, expr, __CALLER__)
  end

As a bonus, it would be nice if the line number always pointed at the function definition (and not at the beginning of its docstring). But that's tangential.

[mattbaker]

Personally in favor of it only searching the project.

In addition to the noise, I think in VS Code the "m", "f", "t" etc. prefixes aren't going to work unless there's a way to turn off this behavior. If we can't, then that's another point in favor of taking actions that speed up search results.

[lukaszsamson]

I think in VS Code the "m", "f", "t" etc. prefixes aren't going to work unless there's a way to turn off this behavior.

@mattbaker It did work at the time workspace symbols was introduced. Looks like it's a recent change and no ways to disable it. The prefixes were introduced to increase search results relevance and speed. There are lots of erlang modules and functions in OTP and sifting through them quickly is not straightforward. Of course it could be achieved with NIF and some custom data structures or some clever prebuilt indexes.
Anyway, I created a separate issue #392

I guess it could be matching additional things (like docstring contents and parameter names

@dgutov No, it matches only modules and function names with a fuzzy algorithm. In your case Ecto.Query.Builder.LimitOffset.module_info/1 module name contains your query

[dgutov]

@lukaszsamson

No, it matches only modules and function names with a fuzzy algorithm. In your case Ecto.Query.Builder.LimitOffset.module_info/1 module name contains your query

Exactly. I don't think f limit should be matching against the module name. Though this is probably moot with the recent news.

[mattbaker]

It did work at the time workspace symbols was introduced.

Oh I know! I remember giving it a try, it was cool :)

[lukaszsamson]

@axelson I revisited this idea after some time and came to a conclusion that we should disable indexing of all modules as in regular work navigation to dependencies or standard lib is much less often. I chose a bit different approach in #1050 - I get the list of modules to index from app controller