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

Jump to bottom

x/tools/gopls: cache diagnostics, don't re-send if unchanged #32443

Closed
myitcv opened this issue Jun 5, 2019 · 6 comments
Closed

x/tools/gopls: cache diagnostics, don't re-send if unchanged #32443

myitcv opened this issue Jun 5, 2019 · 6 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
gopls/v0.4.0

Comments

@myitcv
Copy link
Member

myitcv commented Jun 5, 2019 •
edited

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 myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Jun 5, 2019
@myitcv myitcv mentioned this issue Jun 5, 2019
Closed
@gopherbot gopherbot added this to the Unreleased milestone Jun 5, 2019
@myitcv myitcv mentioned this issue Jun 5, 2019
Closed
@myitcv
Copy link
Member Author

myitcv commented Jun 5, 2019 •
edited

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
@myitcv
cmd/govim: de-dupe PublishDiagnostics notifications from gopls
62e29fd 
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 myitcv mentioned this issue Jun 5, 2019
Merged
myitcv added a commit to govim/govim that referenced this issue Jun 5, 2019
@myitcv
cmd/govim: de-dupe PublishDiagnostics notifications from gopls (#282)
c71ed08 
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).
@leitzler leitzler mentioned this issue Jun 8, 2019
Closed
@myitcv
Copy link
Member Author

myitcv commented Jun 10, 2019 •
edited

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
Copy link
Member Author

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 added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels 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
Copy link

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.

@myitcv myitcv mentioned this issue Jul 30, 2019
Open
@stamblerre
Copy link
Contributor

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
@myitcv
cmd/govim: de-dupe quickfix diagnostics and retain selected index
b7a5b93 
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
@myitcv
cmd/govim: de-dupe quickfix diagnostics and retain selected index
a1b104e 
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
@myitcv
cmd/govim: de-dupe quickfix diagnostics and retain selected index
b3eb367 
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
@myitcv
cmd/govim: de-dupe quickfix diagnostics and retain selected index
be38c6a 
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
@myitcv
cmd/govim: de-dupe quickfix diagnostics and retain selected index (#476)
956c1eb 
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
@gopherbot
Copy link

Change https://golang.org/cl/208261 mentions this issue: internal/lsp: don't resend diagnostics if they are unchanged

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.4.0 Jul 22, 2020
@golang golang locked and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
gopls/v0.4.0
Development

No branches or pull requests
4 participants
@icexin @myitcv @stamblerre @gopherbot