x/tools/gopls: use dynamic symbols by default #41760
Comments
gopherbot
added
Tools
|
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 :)
Whilst this may be true, if a user needs to rely on the
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 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. |
|
@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):
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
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. |
|
Change https://golang.org/cl/271626 mentions this issue: |
|
Change https://golang.org/cl/272748 mentions this issue: |
The workspace symbol tests are not really resilient to changes and did not generate/use golden files consistently. This made it really tricky to switch the workspace symbols defaults. To improve the workflow, consolidate the different kinds of tests into one function, generate and use golden files, and require that all of the workspace symbol queries appear in one file only. Also converted the brittle workspace symbol regtest to a marker test. Update golang/go#41760 Change-Id: I41ccd3ae58ae08fea717c7d8e9a2a10330e8c14f Reviewed-on: https://go-review.googlesource.com/c/tools/+/271626 Trust: Rebecca Stambler <rstambler@golang.org> Run-TryBot: Rebecca Stambler <rstambler@golang.org> Reviewed-by: Robert Findley <rfindley@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org>
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 ispath/to/foo.Bar, we'd get the following results for various searches (assuming fuzzy matching):Barfoo.Barpath/to/foo.BarNote 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:
package.FooStruct.Field1).loaderpackage with equal score to the symbolsnapshot.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.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
The text was updated successfully, but these errors were encountered: