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

x/tools/gopls: improve workspace/symbol ranking #40548

Open
macintacos opened this issue Aug 3, 2020 · 9 comments
Open

x/tools/gopls: improve workspace/symbol ranking #40548

macintacos opened this issue Aug 3, 2020 · 9 comments
Assignees

Comments

@macintacos
Copy link

@macintacos macintacos commented Aug 3, 2020

If you have a question, please ask it on the #vscode or #vscode-go channels in Gophers Slack](https://invite.slack.golangbridge.org/messages/vscode).

Is your feature request related to a problem? Please describe.

Right now there are a couple of things that I don't particularly like about how this extension handles symbols when using the Go to Symbol in Workspace... action in VSCode:

  1. It doesn't seem to populate results immediately (for me). The prompt comes up and it doesn't give me a list of variables immediately without needing to type in a search (FWIW, the Python extension does this, not sure what's going on with this extension). I don't know if this is related to the next issue I have with it, which is...
  2. When I search for a symbol that I know is in my workspace, the extension seems to provide all of the symbols in my dependencies as well, and doesn't seem to rank the symbols in my project above the others. Here's me searching for start (looking for the startCmd variable) which appears more than halfway down the list somehow:

image

Describe the solution you'd like

One of two things (preferably an option between either because why not):

  1. Rank symbols in my workspace above others that return when searching for symbols in a workspace.
  2. Remove symbols that aren't in my project entirely; I'd personally argue that all of those other symbols shouldn't be in the list of options, and should only be exposed if you've checked an option somewhere in settings.

Describe alternatives you've considered

I've went through a bunch of options in this extension and couldn't figure out how to make this work. Definitely could've missed something though, so please feel free to school me and tell me that this functionality already exists.

@hyangah
Copy link
Contributor

@hyangah hyangah commented Aug 3, 2020

@macintacos Thanks for the report. Can. you please share your settings?
In particular, I want to know whether you are using the language server or not ("go.useLanguageServer": true).

@macintacos
Copy link
Author

@macintacos macintacos commented Aug 3, 2020

Here are all my go-related settings (go.useLanguageServer is set to true):

  "[go.mod]": {
    "editor.codeActionsOnSave": {
      "source.organizeImports": true
    },
    "editor.formatOnSave": true
  },
  "[go]": {
    "editor.codeActionsOnSave": {
      "source.organizeImports": true
    },
  },
  "go.autocompleteUnimportedPackages": true,
  "go.buildOnSave": "package",
  "go.formatTool": "goimports",
  "go.lintFlags": ["--fast"],
  "go.lintOnSave": "package",
  "go.lintTool": "golangci-lint",
  "go.useCodeSnippetsOnFunctionSuggest": true,
  "go.useCodeSnippetsOnFunctionSuggestWithoutType": true,
  "go.useLanguageServer": true,
  "go.vetOnSave": "package",
@hyangah hyangah changed the title Improving the results returned in the `Go to Symbol in Workspace...` action x/tools/gopls: improve workspace/symbol ranking Aug 3, 2020
@hyangah hyangah transferred this issue from golang/vscode-go Aug 3, 2020
@gopherbot gopherbot added this to the Unreleased milestone Aug 3, 2020
@stamblerre stamblerre removed this from the Unreleased milestone Aug 3, 2020
@myitcv
Copy link
Member

@myitcv myitcv commented Aug 3, 2020

There is a stack of changes to improve the workspace symbol support in progress: https://go-review.googlesource.com/c/tools/+/228760/31. This work needs to be rebased but will then hopefully form the basis of more advanced search modes, including better ranking (I experimented with an advanced query syntax in https://go-review.googlesource.com/c/tools/+/227677/41)

cc @findleyr

@macintacos
Copy link
Author

@macintacos macintacos commented Aug 3, 2020

Awesome, thanks @myitcv! I'll follow along.

@findleyr findleyr self-assigned this Aug 11, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 13, 2020

Change https://golang.org/cl/247818 mentions this issue: internal/lsp: refactor workspace Symbol method

@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 13, 2020

The above CL (based on Paul's original CL) improves ranking for fuzzy matching, and prepares for further ranking improvements.

I jump to symbols outside of my workspace all the time, so would object to removing that feature altogether :) Let's start by downranking symbols outside of the workspace and see if that is good enough -- I'd like to avoid adding another configuration option if possible. Regarding "preferably an option between either because why not": because it takes effort to support!

Also, regarding this

It doesn't seem to populate results immediately (for me). The prompt comes up and it doesn't give me a list of variables immediately without needing to type in a search

@macintacos what would you like to see without a search query? In any moderately sized project there will be so many symbols that whatever arbitrary N symbols are returned for the empty query are unlikely to be relevant. We're also limited by the LSP in what we can do here: we don't know which buffer(s) are currently visible in the editor, so we can't do something like prefer symbols in the current file. We could do something, like show recently used symbols or symbols in open files, but I'm not convinced either of those are worth implementing. In practice, the user will almost always type something.

CC @heschik

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 13, 2020

Change https://golang.org/cl/248383 mentions this issue: internal/lsp/source: add some downranking for workspace symbols

@macintacos
Copy link
Author

@macintacos macintacos commented Aug 14, 2020

@findleyr Fair enough on all the initial points!

As for what to show without a search query, showing the most recently used symbols in open files makes sense to me, but I understand the motivation to leave it the way it is. I use VSCode for Python as well, so perhaps that's a good point of reference, considering that it's such a widely used extension? From what I can tell, it just lists symbols in the project in alphabetical order, but I'm not sure of their motivation behind doing it that way. The only thin I can think of is that it's reassuring to see some visual feedback that the extension knows about symbols in your project before you begin typing, so showing symbols alphabetically is "good enough".

I don't have any string opinion about showing symbols before you begin typing though; that has less of an impact in relation to the ranking behind the search itself.

gopherbot pushed a commit to golang/tools that referenced this issue Aug 27, 2020
This is based on Paul Jolly's CL 228760, updated to use the new cache
API, support the symbolStyle configuration option, and lift up the
concept of symbol score for later improvement.

From that CL:

There are a number of issues with the current implementation:

* test variant packages are not handled correctly, meaning duplicate
  symbols are returned
* fuzzy results are not ordered by score

We refactor the implementation of workspace symbol to use a
symbolCollector that carries context during the walk for symbols. As
part of resolving the test variant issue, we first determine a list of
packages to walk.

(*symbolCollector).collectPackages gathers the packages we are going to
inspect for symbols. This pre-step is required in order to filter out
any "duplicate" *types.Package. The duplicates arise for packages that
have test variants.  For example, if package mod.com/p has test files,
then we will visit two packages that have the PkgPath() mod.com/p: the
first is the actual package mod.com/p, the second is a special version
that includes the non-XTest _test.go files. If we were to walk both of
of these packages, then we would get duplicate matching symbols and we
would waste effort. Therefore where test variants exist we walk those
(because they include any symbols defined in non-XTest _test.go files).

One further complication is that even after this filtering, packages
between views might not be "identical" because they can be built using
different build constraints (via the "env" config option). Therefore on
a per view basis we first build up a map of PkgPath() -> *types.Package
preferring the test variants if they exist. Then we merge the results
between views, de-duping by *types.Package.

Finally, when we come to walk these packages and start gathering
symbols, we ignore any files we have already seen (due to different
*types.Package for the same import path as a result of different build
constraints), keeping track of those symbols via symbolCollector.

Then we walk that list of packages in much the same way as before.

For golang/go#40548

Co-authored-by: Paul Jolly <paul@myitcv.io>
Change-Id: I8af5bdedbd4a6c3631a213d73a735aea556a13ae
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247818
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Aug 27, 2020
Downrank symbols in packages outside of the workspace.

For golang/go#40548

Change-Id: Ie83f42f844e57aae5fc397d105f33858d6e44d0f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/248383
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 10, 2020

Change https://golang.org/cl/254037 mentions this issue: internal/lsp/source: add some additional symbol downranking

gopherbot pushed a commit to golang/tools that referenced this issue Sep 16, 2020
Downrank symbols if they are:
 1. Unexported and outside of the workspace. Since one wouldn't jump to
    these symbols to e.g. view documentation, they are less relevant.
 2. Fields and interface methods. Usually one would jump to the type
    name, so having fields highly ranked can be noisy.

To facilitate this change and more generally clean up, symbolCollector
is refactored to pass around slices of *ast.Idents rather than build up
'.' separated names as it traverses nested nodes.

For golang/go#40548

Change-Id: Ice4b91cee07b74a13a9b0007fda5fa9a8e447976
Reviewed-on: https://go-review.googlesource.com/c/tools/+/254037
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Paul Jolly <paul@myitcv.org.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.