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: support advanced query syntax for Symbol #37237

Open
myitcv opened this issue Feb 15, 2020 · 6 comments
Open

x/tools/gopls: support advanced query syntax for Symbol #37237

myitcv opened this issue Feb 15, 2020 · 6 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Feb 15, 2020

What version of Go are you using (go version)?

$ go version
go version devel +b7689f5aa3 Fri Jan 31 06:02:00 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200214144324-88be01311a71
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20200214144324-88be01311a71

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build646330205=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This is a request/proposal more than an issue.

We were discussing with @findleyr a means by which symbol search can be made more useful (in Vim). With #37236 fixed, the existing Symbol method will return matches from within the workspace (main module). But a fairly common query is to find symbols matching the query within the "current" package, where "current" is defined by the file within which the cursor is located. i.e. I want to jump to the definition of the method (*T).M.

Because the editor/client knows nothing about packages, I think this will require a query similar to the following. Imagine we are searching using the term hello, with the cursor in the file main.go

package:/path/to/main.go hello

gopls would then translate the file URI to a package and suitably constrain the results.


cc @stamblerre @findleyr

FYI @leitzler

@myitcv
Copy link
Member Author

@myitcv myitcv commented Feb 19, 2020

Per #37236 (comment), we should also add a module scope, again parameterised by file, to limit the search scope to the module containing the file.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 7, 2020

To make this request more formal I'd like to sketch out a potential query syntax. It is based on the popular fzf search syntax (FYI @junegunn).

In the text that follows, the term "item" refers to what we are matching against (noting that more information/context is returned in protocol.SymbolInformation in the form of location etc). At present an "item" is just the symbol name (variable, function, method etc), but in #38273 I have proposed making the fully qualified symbol the "item".

Search syntax

Unless otherwise specified via an option (see below), Symbol will search in extended-search mode, a mode that takes multiple space-separated search terms. The overall search query is the conjunction of these terms.

Token Match type Description
jsEnc fuzzy-match Items that match jsEnc
'json exact-match (quoted) Items that include json
^encoding prefix-exact-match Items that start with encoding
main$ suffix-exact-match Items that end with main
!json inverse-exact-match Items that do not include json
!^encoding inverse-prefix-exact-match Items that do not start with encoding
!main$ inverse-suffix-exact-match Items that do not end with main

To be explicit, / and . (which appear in some fully qualified symbol paths) also participate in the match if specified in a search term.

Options

Options are provided as special search terms in the form key::value.

  • exact::true - enable exact matches (current behaviour)
  • package::/path/to/file.go - constrain the search to the package that contains /path/to/file.go (filepath must be absolute)
  • module::/path/to/file.go - constrain the search to the module that contains /path/to/file.go (filepath must be absolute)
  • package::example.com/blah - constrain the search to the package example.com/blah
  • module::example.com/blah - constrain the search to the module (that contains package) example.com/blah
  • kind::X,Y,Z - constrain the search to the protocol.SymbolKind list provided (comma separated list of numbers corresponding to the SymbolKind enum

Examples

We assume for the following examples that:

  • #38273 is adopted
  • the type of the symbol is not considered in the match
  • the type of the symbol is returned to the client for display purposes
Example 1

A query of "jsEnc" would match encoding/json.Encoder and encoding/json.NewEncoder func(w io.Writer) *Encoder, and encoding/json.Encoder struct and encoding/json.NewEncoder func(w io.Writer) *Encoder respectively would be returned to the client

Example 2

A query of "package::/path/to/file.go 'main" would search within the package that contains /path/to/file.go for items that include the exact term main somewhere in their fully qualified path.

Outstanding questions

  • Consider support for filepaths with spaces
  • Support disjunctions?
  • Further options
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2020

Change https://golang.org/cl/227677 mentions this issue: internal/lsp: first cut of advanced query syntax for WorkspaceSymbol

myitcv added a commit to myitcvforks/tools that referenced this issue Apr 8, 2020
DO NOT REVIEW
DO NOT SUBMIT

* Add config option for SymbolMatcher
* Fully qualify symbols and match against fully-qualified symbol
* Define and implement workspaceOnly and nonWorkspaceExportedOnly search
  options

Fixes golang/go#38273
Updates golang/go#37237

Change-Id: Ie58ae992bf2d22673c3daa2c495c15279a1fa54b
myitcv added a commit to myitcvforks/tools that referenced this issue Apr 8, 2020
DO NOT REVIEW
DO NOT SUBMIT

* Add config option for SymbolMatcher
* Fully qualify symbols and match against fully-qualified symbol
* Define and implement workspaceOnly and nonWorkspaceExportedOnly search
  options

Fixes golang/go#38273
Updates golang/go#37237

Change-Id: Ie58ae992bf2d22673c3daa2c495c15279a1fa54b
@stamblerre stamblerre removed this from the Unreleased milestone Jul 23, 2020
@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 13, 2020

I experimented with fzf-like syntax in http://golang.org/cl/248418, and it was very straightforward to add. Furthermore, it is backwards compatible with our current fuzzy matching: any query containing , ', ^, or $ will not currently match any symbols. So to me, this seems like an easy win that may allow us to deprecate the symbolMatcher configuration option and consolidate on a single matcher.

Regarding the rest of the proposal, supporting things like package::foo... to me, this blurs the line between user-interface and API. I doubt any user would ever type this interactively -- if they wanted to go to a symbol they would just type the symbol out. The purpose of the workspace symbols request is to find a single symbol to jump to, not a collection of symbols.

So because syntax like package::foo is only likely to be used by clients to implement features over the top of the workspace symbols RPC, it seems like it may be better to instead implement this functionality as a separate, proper API, supported as a non-standard request in gopls.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Aug 15, 2020

The only one comment to add is that the approach I've experimented with in https://go-review.googlesource.com/c/tools/+/227677 makes no additional requirements of the LSP client, only the editor/user. You're quite right: there is no expectation that a user would ever type one of these options interactively, rather that via a user-defined custom key mapping, the editor user would augment a typed search query with certain options.

Either way we end up exposing the end user to some sort of API, namely the structure of the options to specify a particular type of search/scope.

Whilst my approach in this CL works in a setting like Vim where there are powerful key mapping capabilities, I quite acknowledge it's not guaranteed to work in all editors. As such a proper API might well be more appropriate. But that might place a requirement on each LSP client to support it (I have no experience with non-standard requests so can't comment on the merits one way or the other).

Hence it felt like, for the very short term, this approach provided a nice, purely opt-in way to experiment with the types of options that would be appropriate, with zero impact on existing clients, editors, users etc.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 27, 2020

Having discussed this at length with @myitcv today, I'm starting to come around to the idea of supporting at a subset of the advanced filters proposed above within the workspace symbols request.

In particular, having seen that the workspace symbols RPC can get noticeably slow in very large projects, it could generally be helpful to expose a mechanism that limits the scope of packages that are walked when looking for symbol matches. Limiting to the current workspace, module, or package seems natural.

We could of course do this via configuration rather than a dynamic query field. But in that case we'd be supporting the complexity of implementing the filter anyway. How much more complexity is added to support parsing package:path/to/foo.go out of the query? Probably not much. Also, for filtering to e.g. the current module, we'd need to know which file the cursor is in, and we have no way to know that server-side.

I'm less convinced by the argument that users will regularly want to bounce back and forth between searching within all packages, within the current package, and within the current module. In my experience, as long as the symbols request is fast, I have no difficulty jumping to my desired symbol across all packages. If I want to go to the Error symbol in package foo, I just type foo.error or 'foo 'error. It would be more work to think about whether or not package foo is in my module (or even my workspace -- sometimes I forget where I am...). I have similar objections to filtering by kind: I'd have to remember which 'kinds' are supported, whether my symbol is a const or a var, etc.

So in summary: I think we should do a cost-benefit analysis of a subset of the proposed syntax. Specifically, how much complexity would it add, and how much our users would benefit from it.

Regarding complexity, I think the following filters would not add much, the reason being that they only affect the set of packages walked, not the symbol search itself. It would be pretty easy just to put together a CL to do this (sans tests) that we can consider.

  • package:path/to/foo.go
  • module:path/to/foo.go
  • workspace:path/to/foo.go

Regarding benefit, I think we should try to put some numbers behind the performance argument. I wrote a benchmark in CL 250798 that can be used for this: let's pick some known large repo (Kubernetes?) and benchmark different queries. It's worth caring about anything above ~100ms.

I'd be happy to do this work myself, though I won't get to it for at least a couple weeks.

But fair warning, we may look at all this and decide it's not worth it, given all of our other priorities.

@stamblerre stamblerre added this to the gopls/unplanned milestone Oct 21, 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