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: cache diagnostics, don't re-send if unchanged #32443

Open
myitcv opened this issue Jun 5, 2019 · 5 comments
Assignees
Labels
Milestone

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Jun 5, 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/"
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-build939912546=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://github.com/myitcv/govim/blob/multiple_gopls_diagnostics/cmd/govim/testdata/diagnostics.txt defines a test that looks to assert the number of diagnostic callbacks for various files.

The setup is very simple (see the txtar at the end of the script)

The sequence is then:

  • Open main1.go
  • Add blank line above line 1
  • Set line 1 to be the comment // Hello
  • Save file

What did you expect to see?

As the last two lines of the testscript assert, this should result in:

  • 3 x diagnostics for main1.go: initial load + first change + second change
  • 1 x diagnostic for main2.go: initial load (after which no changes are made to main2.go or any related file that cause the diagnostics for main2.go to change)

What did you see instead?

  • 3 x diagnostics for main1.go: initial load + first change + second change
  • 3 x diagnostics for main2.go: initial load + first change to main1.go + second change to main1.go

Therefore, two superfluous diagnostic notifications for main2.go (because the diagnostics for main2.go have not changed, i.e. they remain zero.

Arguably, there should be no notifications for main1.go either because the diagnostics for that file have not changed either, but there may be some requirement on diagnostics being sent for files that have been changed?


cc @stamblerre @ianthehat


Edit: update to reflect the fact this problem persists with golang.org/x/tools v0.0.0-20190611222205-d73e1c7e250b

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 5, 2019

There is actually a more serious bug here beyond the superfluous diagnostics; we aren't actually receiving diagnostics for files where it matters, at least not reliably.

In the scenario described in govim/govim#280, we are only getting diagnostic updates for a subset of the 100 files. For example, for a given edit to foo1.go, I only received 38 PublishDiagnostics notifications.

Critically, we sometimes fail to get diagnostic updates for the one file we are editing.

This gives the impression that, from the user's perspective, things have stalled. i.e. Vim's quickfix window shows an error from a number of edits ago (the intervening published diagnostics have not been received by govim).

@myitcv myitcv changed the title x/tools/cmd/gopls: superfluous diagnostics sent x/tools/cmd/gopls: incomplete/superfluous diagnostics sent Jun 5, 2019
@myitcv myitcv changed the title x/tools/cmd/gopls: incomplete/superfluous diagnostics sent x/tools/cmd/gopls: incomplete and superfluous diagnostics sent Jun 5, 2019
myitcv added a commit to govim/govim that referenced this issue Jun 5, 2019
This helps to throttle the number of changes we try to push through to
the quickfix window.

However, golang/go#32443 remains an issue in
as much as incomplete diagnostics are sent to the client. i.e. we can't
be sure to receive the most recent diagnostics for the file which we are
editing (and in which we know, for example, we are making errors).
myitcv added a commit to govim/govim that referenced this issue Jun 5, 2019
This helps to throttle the number of changes we try to push through to
the quickfix window.

However, golang/go#32443 remains an issue in
as much as incomplete diagnostics are sent to the client. i.e. we can't
be sure to receive the most recent diagnostics for the file which we are
editing (and in which we know, for example, we are making errors).
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 10, 2019

Just noting a Slack conversation here, where there was some suggestion that this issue might be related to the "no room in queue" errors that others have been seeing. FWIW, govim uses internal JSON RPC and LSP client implementations.

@myitcv myitcv changed the title x/tools/cmd/gopls: incomplete and superfluous diagnostics sent x/tools/cmd/gopls: superfluous diagnostics sent Jun 12, 2019
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 12, 2019

Updated my two previous comments to reflect the fact that diagnostic results from gopls were being dropped because, per #32467 (comment), the default for the JSON RPC client is to do just that:

This seems to happen because the defaultMessageBufferSize is set to 20 and RejectIfOverloaded is set to true for both client and server conns in jsonrpc2.

https://github.com/golang/tools/blob/master/internal/lsp/protocol/protocol.go#L14
https://github.com/golang/tools/blob/master/internal/jsonrpc2/jsonrpc2.go#L28

I think increasing the default to a more-reasonable 128, or just disabling RejectIfOverloaded should fix this issue.

Hence govim/govim@e9bdd97 fixed this for govim.

So this issue is now just about the superfluous diagnostics being sent from gopls to the client, per the original description.

@stamblerre stamblerre changed the title x/tools/cmd/gopls: superfluous diagnostics sent x/tools/cmd/gopls: cache diagnostics, don't re-send if unchanged Jun 29, 2019
@stamblerre stamblerre changed the title x/tools/cmd/gopls: cache diagnostics, don't re-send if unchanged x/tools/gopls: cache diagnostics, don't re-send if unchanged Jul 2, 2019
@icexin

This comment has been minimized.

Copy link

@icexin icexin commented Jul 30, 2019

There is a similar problem in emacs. If there are too many files in a package, when editing one of the files, each time you add a character, you will get the diagnostics of all the files under the package, causing the emacs to be very stuck and almost impossible to edit.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Aug 8, 2019

This is likely one of the causes of #33531.

@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Aug 26, 2019
myitcv added a commit to govim/govim that referenced this issue Sep 2, 2019
Currently, because of golang/go#32443 we can
unnecessarily update the quickfix window with diagnostics. What's more,
when we update the quickfix list we lose the selected index; the first
row is selected when the quickfix list is reset.

This presents something of a problem when jumping to a quickfix error in
a file that is not currently open, where that error is not the first in
the quickfix list. Because jumping to that error means we trigger a
didOpen, which triggers all diagnostics to be sent again (even though
they haven't changed), which causes the first row to be selected again.
The user, understandably, get's very confused.

So for now, we use reflect.DeepEqual to de-dupe the diagnostics we
receive from gopls, pending a fix to
golang/go#32443. We also keep track of the
last set error diagnostics, so that when setting new diagnostics we can
reselect the currently selected row. Or, if that row no longer exists,
select the first row. This changes the times that setqflist is called.
Hence, fix up tests which previously asserted based on this behaviour.

As part of this change we also retrigger clearing/population of the
quickfix list (and signs) as the relevant config changes.

Also slightly optimise the sign placement code to return early if signs
are disabled.
myitcv added a commit to govim/govim that referenced this issue Sep 2, 2019
Currently, because of golang/go#32443 we can
unnecessarily update the quickfix window with diagnostics. What's more,
when we update the quickfix list we lose the selected index; the first
row is selected when the quickfix list is reset.

This presents something of a problem when jumping to a quickfix error in
a file that is not currently open, where that error is not the first in
the quickfix list. Because jumping to that error means we trigger a
didOpen, which triggers all diagnostics to be sent again (even though
they haven't changed), which causes the first row to be selected again.
The user, understandably, get's very confused.

So for now, we use reflect.DeepEqual to de-dupe the diagnostics we
receive from gopls, pending a fix to
golang/go#32443. We also keep track of the
last set error diagnostics, so that when setting new diagnostics we can
reselect the currently selected row. Or, if that row no longer exists,
select the first row. This changes the times that setqflist is called.
Hence, fix up tests which previously asserted based on this behaviour.

As part of this change we also retrigger clearing/population of the
quickfix list (and signs) as the relevant config changes.

Also slightly optimise the sign placement code to return early if signs
are disabled.
myitcv added a commit to govim/govim that referenced this issue Sep 4, 2019
Currently, because of golang/go#32443 we can
unnecessarily update the quickfix window with diagnostics. What's more,
when we update the quickfix list we lose the selected index; the first
row is selected when the quickfix list is reset.

This presents something of a problem when jumping to a quickfix error in
a file that is not currently open, where that error is not the first in
the quickfix list. Because jumping to that error means we trigger a
didOpen, which triggers all diagnostics to be sent again (even though
they haven't changed), which causes the first row to be selected again.
The user, understandably, get's very confused.

So for now, we use reflect.DeepEqual to de-dupe the diagnostics we
receive from gopls, pending a fix to
golang/go#32443. We also keep track of the
last set error diagnostics, so that when setting new diagnostics we can
reselect the currently selected row. Or, if that row no longer exists,
select the first row. This changes the times that setqflist is called.
Hence, fix up tests which previously asserted based on this behaviour.

As part of this change we also retrigger clearing/population of the
quickfix list (and signs) as the relevant config changes.

Also slightly optimise the sign placement code to return early if signs
are disabled.
myitcv added a commit to govim/govim that referenced this issue Sep 4, 2019
Currently, because of golang/go#32443 we can
unnecessarily update the quickfix window with diagnostics. What's more,
when we update the quickfix list we lose the selected index; the first
row is selected when the quickfix list is reset.

This presents something of a problem when jumping to a quickfix error in
a file that is not currently open, where that error is not the first in
the quickfix list. Because jumping to that error means we trigger a
didOpen, which triggers all diagnostics to be sent again (even though
they haven't changed), which causes the first row to be selected again.
The user, understandably, get's very confused.

So for now, we use reflect.DeepEqual to de-dupe the diagnostics we
receive from gopls, pending a fix to
golang/go#32443. We also keep track of the
last set error diagnostics, so that when setting new diagnostics we can
reselect the currently selected row. Or, if that row no longer exists,
select the first row. This changes the times that setqflist is called.
Hence, fix up tests which previously asserted based on this behaviour.

As part of this change we also retrigger clearing/population of the
quickfix list (and signs) as the relevant config changes.

Also slightly optimise the sign placement code to return early if signs
are disabled.
myitcv added a commit to govim/govim that referenced this issue Sep 4, 2019
Currently, because of golang/go#32443 we can
unnecessarily update the quickfix window with diagnostics. What's more,
when we update the quickfix list we lose the selected index; the first
row is selected when the quickfix list is reset.

This presents something of a problem when jumping to a quickfix error in
a file that is not currently open, where that error is not the first in
the quickfix list. Because jumping to that error means we trigger a
didOpen, which triggers all diagnostics to be sent again (even though
they haven't changed), which causes the first row to be selected again.
The user, understandably, get's very confused.

So for now, we use reflect.DeepEqual to de-dupe the diagnostics we
receive from gopls, pending a fix to
golang/go#32443. We also keep track of the
last set error diagnostics, so that when setting new diagnostics we can
reselect the currently selected row. Or, if that row no longer exists,
select the first row. This changes the times that setqflist is called.
Hence, fix up tests which previously asserted based on this behaviour.

As part of this change we also retrigger clearing/population of the
quickfix list (and signs) as the relevant config changes.

Also slightly optimise the sign placement code to return early if signs
are disabled.
@stamblerre stamblerre self-assigned this Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.