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: initial diagnostic run "overwrites" subsequent file-open diagnostics #36452

Closed
myitcv opened this issue Jan 8, 2020 · 10 comments
Closed

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Jan 8, 2020

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

$ go version
go version devel +693748e9fa Mon Jan 6 11:46:56 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200107184032-11e9d9cc0042 => github.com/myitcvforks/tools v0.0.0-20200108093634-093e5cb57410
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20200107184032-11e9d9cc0042 => github.com/myitcvforks/tools/gopls v0.0.0-20200108093634-093e5cb57410

Note the replace-d version of x/tools and gopls. Per govim/govim@afa4918, we are in effect using 53017a39 with 11e9d9cc cherry-picked on top.

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-build221852352=/tmp/go-build -gno-record-gcc-switches"

What did you do?

We have a govim test that verifies suggested fixes. The initial setup is:

-- go.mod --
module mod.com

go 1.12
-- main.go.single --
package main

var x int

func main() {
	x = x
}

We then ask for suggested fixes via a CodeAction call and apply the single fix that should be returned.

What did you expect to see?

A single fix returned.

What did you see instead?

No fixes returned.

The gopls log: gopls.log looks somewhat interesting. It has, at a high level, the following order:

  • client -> server: DidOpen for main.go - version 1
  • client -> server: DidChange for main.go - version 2
  • client -> server: DidChange for main.go - version 3
  • server -> client: PublishDiagnostics for version 3. Diagnostics are a single analysis warning diagnostic about self assignment
  • server -> client: PublishDiagnostics for version 0. No diagnostics
  • client -> server: CodeAction call for suggested fixes in the range of the self assignment

We get back no results from that last call.

The diagnostics being published for version 0 appears to be an issue in and of itself (arguably the client could ignore these but gopls should not be publishing them) but it also appears to suggest that the diagnostics including the analysis results (version 3) are being "overwritten", hence why the CodeAction call returns nothing.


cc @stamblerre

FYI @leitzler

@gopherbot gopherbot added this to the Unreleased milestone Jan 8, 2020
@gopherbot gopherbot added the Tools label Jan 8, 2020
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 8, 2020

For the record we've just seen another instance in:

cd $(mktemp -d) && curl -s https://gist.githubusercontent.com/myitcv/d23845374472498eab31447186ca6645/raw/7c6cebdfaea1e9c5e723acfb4e3d9f5a4cbfa1bf/logs.base64 | base64 -d | tar -zx

Log file:

cmd/govim-race/scenario_default/go-test-script729570158/script-suggested_fixes/_tmp/gopls_log
@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Jan 8, 2020
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 8, 2020

The version 0 diagnostics being published too late is likely unrelated, but definitely the first obvious issue. Let me send out a CL to address this and then maybe you can let me know what that changes.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 8, 2020

Change https://golang.org/cl/213820 mentions this issue: internal/lsp: don't send diagnostics for old file versions

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 8, 2020

I think the suggested fixes issue is coming from govim actually. The incorrect diagnostics (for version 0) are being cached and sent along in the textDocument/codeAction request. The quickfix action type has to be provided diagnostics to match the fixes to, but the codeAction sets null for diagnostics, so gopls doesn't return anything.

[Trace - 11:50:53.044 AM] Sending request 'textDocument/codeAction - (2)'.
Params: {"textDocument":{"uri":"file:///artefacts/cmd/govim/scenario_default/go-test-script074041913/script-suggested_fixes/main.go"},"range":{"start":{"line":5,"character":1},"end":{"line":5,"character":1}},"context":{"diagnostics":null,"only":["quickfix"]}}

I would imagine that https://golang.org/cl/213820 will fix this, but it shouldn't be necessary (and I'm not yet convinced that it should go in, I'll wait for @heschik's thoughts there). This seems like a govim bug to me.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 8, 2020

but the codeAction sets null for diagnostics, so gopls doesn't return anything.

Ah that's an excellent spot, thank you.

We'll fix up diagnostics handling on our side to drop/error anything that isn't a higher version (for a given file) than before.

Will also test out with that CL when it lands.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 9, 2020

Per #36476 (comment) we have fixed things on the govim side but that uncovered another issue (#36476)

Whilst we're happy having a safety net in govim to ignore non-new diagnostics, I still think https://go-review.googlesource.com/c/tools/+/213820/ is the more robust solution because we ensure things for all clients that way.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 9, 2020

I actually disagree with this point:

Whilst we're happy having a safety net in govim to ignore non-new diagnostics, I still think https://go-review.googlesource.com/c/tools/+/213820/ is the more robust solution because we ensure things for all clients that way.

It's on the client to know the current version of the file in the editor and to understand that the diagnostics for a different version do not apply.

gopherbot pushed a commit to golang/tools that referenced this issue Jan 9, 2020
This isn't a strictly necessary change, but it simplifies things for
open files when the initial workspace load diagnostics come in too late.

Updates golang/go#36452

Change-Id: I85a9c201876b7c63a97d8dca020fc396b3c57706
Reviewed-on: https://go-review.googlesource.com/c/tools/+/213820
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 9, 2020

Closing this as https://golang.org/cl/213820/ has been merged.

@stamblerre stamblerre closed this Jan 9, 2020
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 9, 2020

Thanks for the fix.

Just to briefly clarify what I meant earlier (it's only a minor point):

It's on the client to know the current version of the file in the editor and to understand that the diagnostics for a different version do not apply.

Indeed, and we do perform this check. Without this check many things could/would go wrong because of races between the user making changes and diagnostics being "applied" (formatting etc)

What I was referring to here is another check that we just inserted to (silently) drop non-new diagnostics. It's this check that I think should be superfluous. Clients should, to my mind, be able to assert that they will only ever receive new diagnostics for a given file.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 12, 2020

Change https://golang.org/cl/214418 mentions this issue: internal/lsp: delete work-around for golang/go#36452

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
3 participants
You can’t perform that action at this time.