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: diagnostics version is reused for same diagnostic #36476

Closed
leitzler opened this issue Jan 9, 2020 · 8 comments
Closed

x/tools/gopls: diagnostics version is reused for same diagnostic #36476

leitzler opened this issue Jan 9, 2020 · 8 comments
Labels
Milestone

Comments

@leitzler
Copy link
Contributor

@leitzler leitzler commented Jan 9, 2020

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

$ go version
go version go1.13.5 darwin/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200108203644-89082a384178
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20200108203644-89082a384178

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/pontus/Library/Caches/go-build"
GOENV="/Users/pontus/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/pontus/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/pontus/proj/govim/latest/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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/go-build710948783=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Open a main.go with the following content (no errors/warnings):

package main

func main() {

}

gopls sends diagnostics Version 1 with no diagnostics:

[Trace - 10:26:41.794 AM] Received notification 'window/logMessage'.
Params: {"type":1,"message":"2020/01/09 10:26:41 no diagnostics: context canceled\n\tURI = file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go"}

[Trace - 10:26:41.798 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go","version":1,"diagnostics":[]}

Then I add a a inside func main():

package main

func main() {
a
}

That triggers a didChange to gopls, and gopls responds with Version 2 of the diagnostics:

[Trace - 10:27:08.079 AM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"version":2,"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go"},"contentChanges":[{"range":{"start":{"line":3,"character":0},"end":{"line":4,"character":0}},"text":"a\n"}]}

[Trace - 10:27:08.085 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go","version":2,"diagnostics":[{"range":{"start":{"line":3,"character":0},"end":{"line":3,"character":1}},"severity":1,"source":"compiler","message":"undeclared name: a"}]}

I remove the a again, triggering another didChange (Version 3) to gopls and at this point I get Version 1 back again:

[Trace - 10:27:28.647 AM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"version":3,"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go"},"contentChanges":[{"range":{"start":{"line":3,"character":0},"end":{"line":4,"character":0}},"text":"\n"}]}

[Trace - 10:27:28.653 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go","version":1,"diagnostics":[]}

I re-add a at the same position again, and get Version 2 of the diagnostics:

[Trace - 10:27:34.830 AM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"version":4,"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go"},"contentChanges":[{"range":{"start":{"line":3,"character":0},"end":{"line":4,"character":0}},"text":"a\n"}]}

[Trace - 10:27:34.835 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go","version":2,"diagnostics":[{"range":{"start":{"line":3,"character":0},"end":{"line":3,"character":1}},"severity":1,"source":"compiler","message":"undeclared name: a"}]}

Repeat:

[Trace - 10:27:45.058 AM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"version":5,"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go"},"contentChanges":[{"range":{"start":{"line":3,"character":0},"end":{"line":4,"character":0}},"text":"\n"}]}

[Trace - 10:27:45.066 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go","version":1,"diagnostics":[]}

[Trace - 10:27:52.450 AM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"version":6,"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go"},"contentChanges":[{"range":{"start":{"line":3,"character":0},"end":{"line":4,"character":0}},"text":"a\n"}]}

[Trace - 10:27:52.452 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go","version":2,"diagnostics":[{"range":{"start":{"line":3,"character":0},"end":{"line":3,"character":1}},"severity":1,"source":"compiler","message":"undeclared name: a"}]}

[Trace - 10:28:03.645 AM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"version":7,"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go"},"contentChanges":[{"range":{"start":{"line":3,"character":0},"end":{"line":4,"character":0}},"text":"\n"}]}

[Trace - 10:28:03.652 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go","version":1,"diagnostics":[]}

If I then add b instead, gopls responds with the correct version:

[Trace - 10:28:12.194 AM] Sending notification 'textDocument/didChange'.
Params: {"textDocument":{"version":8,"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go"},"contentChanges":[{"range":{"start":{"line":3,"character":0},"end":{"line":4,"character":0}},"text":"b\n"}]}

[Trace - 10:28:12.197 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/tmp.kvOApBCf/main.go","version":8,"diagnostics":[{"range":{"start":{"line":3,"character":0},"end":{"line":3,"character":1}},"severity":1,"source":"compiler","message":"undeclared name: b"}]}

What did you expect to see?

A new version each time that corresponds to the didChange.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jan 9, 2020

As @leitzler has just linked, when we fixed gopls to ignore non-new diagnostics (per #36452 (comment)) we immediately ran into this issue.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 9, 2020

Thanks for reporting this issue! I think this issue must have existed for a while. It's definitely a caching issue, and we haven't noticed it in VS Code because VS Code will actually go back to old file versions if they are the same. For example, if a file's version 1 contains "foo", and then version 2 contains "fo", and then the next version goes back to "foo", VS Code will send version 1 for that second didChange. govim isn't doing that, which is why this error gets picked up. I'll take a look.

@leitzler

This comment has been minimized.

Copy link
Contributor Author

@leitzler leitzler commented Jan 10, 2020

Just a note, after https://golang.org/cl/213820/ was merged (tested with golang.org/x/tools/gopls v0.1.8-0.20200110042803-e2f26524b78c) old diagnostics aren't sent any longer.

That has impact on this issue as it is now a bit more severe when gopls never sends diagnostics that it has sent before. Most notable is that gopls won't clear diagnostics (send empty diagnostics) after doing it the first time.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jan 10, 2020

I'm fairly certain we're still seeing this problem as of a7a6caa82ab2

Starting with:

package main

func main() {
}

I then edit to take the file to:

package main

func main() {
        a
}

Then delete the a to take the file back to:

package main

func main() {
        
}

In my manual testing just now, this either leaves gopls in a state where it thinks:

  1. there are no errors in the file (despite the a being an error)
  2. there is always an error in the file (despite there being no a)

A gopls log file where I saw case 1 is attached:
fail.log

Note the last publishing of diagnostics is:

Params: {"uri":"file:///home/myitcv/gostuff/src/github.com/myitcv/playground/main.go","version":4,"diagnostics":[]}

Despite the fact that after that there are versions of the file, namely 5, 7 and 9, where there is again an error in the file (albeit the same error that was first introduced in version 3)

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 13, 2020

Change https://golang.org/cl/214418 mentions this issue: internal/lsp: associate error messages with URIs, not file identities

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 13, 2020

Change https://golang.org/cl/214422 mentions this issue: internal/lsp: use latest file versions in diagnostics

@leitzler

This comment has been minimized.

Copy link
Contributor Author

@leitzler leitzler commented Jan 14, 2020

Tested with golang.org/x/tools/gopls v0.1.8-0.20200114052453-d31a08c2edf2 and the original issue remains, reopen?
My bad, works like a charm.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jan 14, 2020

Just to confirm, per @leitzler's edit to that previous message, everything appears to be working as far as this test is concerned. Thanks for the fix!

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.