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: references does not guarantee ordered result #32572

Closed
myitcv opened this issue Jun 12, 2019 · 8 comments
Closed

x/tools/gopls: references does not guarantee ordered result #32572

myitcv opened this issue Jun 12, 2019 · 8 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Jun 12, 2019

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

$ go version
go version devel +ef84fa082c Mon Jun 10 23:23:39 2019 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20190611222205-d73e1c7e250b

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="/home/myitcv/gostuff/src/github.com/myitcv/govim/cmd/govim/.bin"
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
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-build727910193=/tmp/go-build -gno-record-gcc-switches"

What did you do?

The new references implementation is great; thanks @suzmue!

However I'm seeing that list of locations returned is not guaranteed to be ordered. Given that multiple URIs could possibly be returned, I think the most sensible default is to order by URI then start offset.

If the client then wants to apply some other initial ordering (i.e. they take the filename and map it to some state in their editor) then fine, we at least have a stable result.

I don't see this 100% of the time, hence I suspect this is the result of a map to slice conversion, where the resulting slice is then not sorted?

What did you expect to see?

Guaranteed order of results from a call to the references method.

What did you see instead?

Random ordered results.


cc @stamblerre @ianthehat

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 12, 2019

The LSP spec does not state anywhere that the result has to be ordered - therefore, I'd say that clients should not rely on this behavior (https://microsoft.github.io/language-server-protocol/specification#textDocument_references).

Is there a reason that the client cannot apply this ordering? VSCode definitely orders results, so it seems redundant to me for us to guarantee ordering, when more likely than not, the client will redo the sort.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 12, 2019

Is there a reason that the client cannot apply this ordering?

No reason, other than perhaps there being a sensible default that the server might want to offer to clients so that they don't have to do the work themselves.

But just to note: the current state means that the results sent from server to client will not be stable, which is a problem if you're looking to assert in tests against the behaviour of gopls at a high-level input/output.

@suzmue

This comment has been minimized.

Copy link
Contributor

@suzmue suzmue commented Jun 12, 2019

Thanks for bringing this to our attention! We do not want to commit to sorting the returned results at this time since it is not in the spec.

@suzmue suzmue closed this Jun 12, 2019
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 13, 2019

Thanks @suzmue @stamblerre.

Quite agree that this isn't specified in the LSP spec, which I think means that one can in fact argue that returning sorted results is just as correct/valid as returning them in a random order.

In addition to the points about it being a sensible default from the client's perspective and making testing easier, I think it's also more intuitive that for a list response type the order is defined, whether in the spec or the implementation.

All that said, very happy to defer 😄

Use of the references method has just landed in govim (govim/govim#291) so thanks again for the work to get this into gopls!

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jul 2, 2019

Re-opening this issue to continue discussion on the following specific point.

The textDocument/references method parameter is defined as follows:

interface ReferenceParams extends TextDocumentPositionParams {
	context: ReferenceContext
}

interface ReferenceContext {
	/**
	 * Include the declaration of the current symbol.
	 */
	includeDeclaration: boolean;
}

From govim, we set includeDeclaration: true. This means we're expecting the declaration to be one of the locations returned.

To my mind I think it's most intuitive to have the declaration as the first location, hence we should define that this is what gopls will do, regardless of whether it then sorts the remaining results or not.

What do you think?

@suzmue

This comment has been minimized.

Copy link
Contributor

@suzmue suzmue commented Jul 2, 2019

Yes, that makes sense. Thanks for pointing that out.

Until find references is supported at a project level (#32869), this would mean that find references would return all of the references in the current package + the declaration from the other package (if includeDeclaration=true).

@suzmue suzmue changed the title x/tools/cmd/gopls: references does not guarantee ordered result x/tools/gopls: references does not guarantee ordered result Jul 2, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 2, 2019

Change https://golang.org/cl/184723 mentions this issue: internal/lsp: include declaration for references

gopherbot pushed a commit to golang/tools that referenced this issue Jul 3, 2019
A client can specify "IncludeDeclaration" in its ReferenceParams.
When they do so, we want to include the declaration, even if it was not
in the scope we searched for references.

Additionally, we also return the location of the declaration first in
the result array when it is included in the results.

Updates golang/go#32572

Change-Id: I12837cd98102ee8d531f0f4bac2fb7bded2564c0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184723
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jul 10, 2019

Looks like the CL above fixed the problem. Please re-open if there's anything else we should do here.

@stamblerre stamblerre closed this Jul 10, 2019
movie-travel-code added a commit to movie-travel-code/tools that referenced this issue Jul 11, 2019
A client can specify "IncludeDeclaration" in its ReferenceParams.
When they do so, we want to include the declaration, even if it was not
in the scope we searched for references.

Additionally, we also return the location of the declaration first in
the result array when it is included in the results.

Updates golang/go#32572

Change-Id: I12837cd98102ee8d531f0f4bac2fb7bded2564c0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184723
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
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.