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: use dynamic symbols by default #41760

Open
findleyr opened this issue Oct 2, 2020 · 2 comments
Open

x/tools/gopls: use dynamic symbols by default #41760

findleyr opened this issue Oct 2, 2020 · 2 comments
Labels

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Oct 2, 2020

The gopls "symbolStyle" option controls how symbols are qualified in workspace symbol responses.

Right now it has three possible values:

  • "full", which qualifies symbols with the full package import path (path/to/foo.Bar)
  • "package", which qualifies symbols using their package name (foo.Bar). This is the current default.
  • "dynamic", which qualifies symbols using whichever '/' or '.' delimited suffix scores highest for the symbol query. This is a little subtle, so is perhaps best illustrated with examples. If the fully qualified symbol in question is path/to/foo.Bar, we'd get the following results for various searches (assuming fuzzy matching):
    • a search for "bar" would return just Bar
    • a search for "foobar" would return foo.Bar
    • a search for "pathbar" would return path/to/foo.Bar

Note that in all three cases the returned 'containerName' in the symbol payload is the package import path, so it's possible to see which package 'Bar' is in even if it's not clear from the symbol name itself.

I would like to change the default from "package" to "dynamic" for the following reasons:

  • fully qualified symbols are simply too long to be usable in most clients.
  • package qualified symbols can also be a bit long, especially for nested fields (e.g. package.FooStruct.Field1).
  • package qualified symbols suffer from sorting problems. For example, a search for "load" in x/tools matches a large number of symbols in the loader package with equal score to the symbol snapshot.load, when the latter seems unambiguously more relevant. In general, matches are more relevant the closer they are to the end of the fully qualified symbol string. Unfortunately, we can't just use dynamic scoring and then return package qualified symbols, because many LSP clients (including VS Code) do their own fuzzy sorting of the results.
  • Sometimes when I search for symbols, I want to be able to type the package name to differentiate them. Sometimes I even have to type other components in the import path (for example: "oh, I want the internal one"). Again, because many LSP clients implement their own filtering and sorting, whatever symbol we return must match the query (c.f. golang/vscode-go#647). Dynamic symbols make these queries work in most clients, by returning a symbol that matches the query in the most relevant way.

I've been using "dynamic" myself for months, and wouldn't go back. In my opinion it's a much better experience, but I worry that it could be confusing for new users.

Opening this issue for discussion, and to record the decision.

One note: if you want to try this out in VS Code, be aware that the "symbolStyle" setting was broken until it was just recently fixed in https://golang.org/cl/259138.

CC @stamblerre @hyangah @heschik @pjweinb @myitcv

@gopherbot gopherbot added this to the Unreleased milestone Oct 2, 2020
@myitcv
Copy link
Member

@myitcv myitcv commented Oct 5, 2020

Thanks again @findleyr for all the work helping to get the revised symbol changes in!

I'm somewhat on the fence about the proposal; not because I clearly see a better option, but rather because I don't see there being a perfect option :)

  • fully qualified symbols are simply too long to be usable in most clients.

Whilst this may be true, if a user needs to rely on the containerName in order to disambiguate a response, they end up looking at the fully qualified path in any case (or worse in the dynamic mode, because the returned symbol name potentially includes parts of the fully qualified path already).

  • Unfortunately, we can't just use dynamic scoring and then return package qualified symbols, because many LSP clients (including VS Code) do their own fuzzy sorting of the results

This seems more than unfortunate not least because the client will also waste time fuzzy matching the results. I agree the solution here is probably microsoft/vscode#98125.

What I really like about the dynamic mode is the better scoring - it just works better than our current scoring approach for fully qualified symbols. And, per our offline chat, it also applies the fzf qualifiers (^, $, etc) to individual path elements. What I don't like is the fact that there is a variable amount of context shown in the results (which I think is mainly a function of your third and fourth bullet points?) - for some reason my brain doesn't "read" that quickly enough (per my comment above, showing the full import path to the right ends up being too busy).

Having said all of that, I think I'm personally going to prefer fully qualified (emphasis important because that clearly should not affect this proposal per se) regardless. I would, however, like the dynamic ranking approach with it.

Going beyond the scope of your proposal for one second, another thing we have talked about offline is search scope. Ignoring how the UI/UX would work in VSCode, if your symbol search is limited to the current package (as determined by the file in which the cursor is currently sitting) then you can drop the entire import path from fully qualified results. If your search is limited to the workspace then you can drop the module path prefix. In either case, because the search is user-directed to a specific scope, they expect to see a shortened version of the result, i.e. not fully qualified. On the basis the user is more likely to be searching for symbols in the current package/workspace (citation required), is there merit in trying to optimise for the fully qualified approach (using the dynamic ranking approach)? The presentation of results will be less subtle. This would, however, require three different types of symbol search in VSCode and other editors - I don't know to what extent that is feasible.

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Oct 5, 2020

@myitcv and I discussed this earlier on Slack, and came to a consensus that it makes sense to use dynamic symbols for now, until microsoft/vscode#98125 is addressed. (Paul: please correct me if you disagree)

A few points (some of which Paul and I discussed, some not):

if a user needs to rely on the containerName in order to disambiguate a response, they end up looking at the fully qualified path in any case

I disagree with 'in any case' :): much of the time the user will not need to look at containerName, as it need only be considered when there is ambiguity. Also, dynamic symbols optimize the relevancy of the left-aligned symbol -- often I find myself not looking at containerName, but rather just typing <space><packageName> to discriminate. My fingers work faster than my brain :)
Granted, this takes practice.

What I really like about the dynamic mode is the better scoring

Agreed. Apart from the somewhat flimsy argument I made above about optimizing the relevancy of the left-aligned text, dynamic symbols are essentially a workaround for microsoft/vscode#98125. Ideally, we could just return unqualified symbols in the payload's 'name' field, the import path in the 'containerName' field, and let the client handle the presentation. But I think this workaround is worth it's weight.

@stamblerre stamblerre removed this from the Unreleased milestone Oct 8, 2020
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
4 participants
You can’t perform that action at this time.