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: no diagnostics received for initially empty files (sometimes) #39646

Open
myitcv opened this issue Jun 17, 2020 · 14 comments
Open

x/tools/gopls: no diagnostics received for initially empty files (sometimes) #39646

myitcv opened this issue Jun 17, 2020 · 14 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Jun 17, 2020

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

$ go version
go version devel +d286e61b67 Mon Jun 15 23:29:23 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200617042924-7f3f4b10a808
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20200617042924-7f3f4b10a808

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=""
GOMODCACHE="/home/myitcv/gostuff/pkg/mod"
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-build538522998=/tmp/go-build -gno-record-gcc-switches"

What did you do?

govim has a test that verifies we receive expected diagnostics in the case where files are initially empty on disk, but subsequently get populated with content in the editor.

https://github.com/govim/govim/blob/e2a1802131f331832d667736aa27f023636f0c3a/cmd/govim/testdata/scenario_default/quickfix_empty_files.txt

Recently (apologies, I can't be any more specific than saying "in the last 2-3 weeks) we have started to see, and continue to see, a small number of flakes in this test.

In a normal run of this test we expect a set of diagnostics; when we see these flakes, we don't see any diagnostics.

Here is a gopls log file from a passing test run. Notice the DidChange notifications, as well as the PublishDiagnostics notifications: gopls.log

And here is the gopls log from a failing test run. Notice the DidChange notifications, but no PublishDiagnostics notifications: gopls.log

What did you expect to see?

A consistently passing test.

What did you see instead?

Flakes as described above.


cc @stamblerre @findleyr

FYI @leitzler

@myitcv
Copy link
Member Author

@myitcv myitcv commented Jun 25, 2020

FYI we're still seeing ~1 failure per day (in a build matrix of 12 entries) in the govim gopls@master build runs.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 25, 2020

Thanks for the report! I am going to convert this into a gopls regression test and see if I can reproduce the issue.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 26, 2020

Change https://golang.org/cl/240063 mentions this issue: internal/lsp: add a new regtest to reproduce golang/go#39646

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 26, 2020

I tried to reproduce the test as carefully as possible in my CL, but I haven't been able to trigger the failure. The issue is that the new files are getting treated like command-line-arguments packages, which should only happen when they are empty. I wonder if we will see similar flakes on the builders.

In the logs, I see command-line-arguments in the packages.Load at snapshot 3, which is the point at which p/p.go should be in a good state. I don't see any packages.Load results for snapshot 1, which is when you might expect a command-line-arguments result. One more thing to try would be adding an analogous go/packages test.

gopherbot pushed a commit to golang/tools that referenced this issue Jun 26, 2020
Change-Id: I51d8c66a83ecae1c8fc1f39c0e90a03a732c263b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/240063
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 26, 2020

Change https://golang.org/cl/240098 mentions this issue: internal/lsp: reproduce and fix golang/go#39646

akupila added a commit to akupila/govim that referenced this issue Jul 6, 2020
* gopls/internal/hooks, internal/lsp/source: only match full words in link regexes 0cc1aa72
* internal/lsp: support opening single files f01a4bec
* internal/lsp/regtest: add regression tests for on-disk file changes ea7be8d7
* gopls/doc/troubleshooting.md: update the gopls output channel name 7cb253f4
* internal/lsp/source: speed up completion candidate formatting 1837592e
* internal/lsp/cmd: change pre-filled issue header for gopls bug 9a0e0698
* internal/lsp: decorate error message from workspace/configuration a32c0cb1
* go/packages: move all overlay tests into overlay_test.go c138986d
* internal/lsp: remove VS Code-specific completion hack b2d8b033
* internal/lsp/cache: handle a few possible panics in PackageStats 4bdfe1a3
* internal/analysisinternal: prevent fillstruct panic on nil package aa3d5013
* internal/lsp/source: add a new symbolStyle configuration option aa94e735
* internal/lsp: fully qualify workspace Symbol matches 84cfedeb
* internal/lsp: add a new regtest to reproduce golang/go#39646 bcbc01e0
* internal/lsp/fake: explicitly set GOPACKAGESDRIVER=off in regtests 6506e20d
* internal/lsp: watch all files in the module and replace target fadf93ff
leitzler pushed a commit to govim/govim that referenced this issue Jul 6, 2020
* gopls/internal/hooks, internal/lsp/source: only match full words in link regexes 0cc1aa72
* internal/lsp: support opening single files f01a4bec
* internal/lsp/regtest: add regression tests for on-disk file changes ea7be8d7
* gopls/doc/troubleshooting.md: update the gopls output channel name 7cb253f4
* internal/lsp/source: speed up completion candidate formatting 1837592e
* internal/lsp/cmd: change pre-filled issue header for gopls bug 9a0e0698
* internal/lsp: decorate error message from workspace/configuration a32c0cb1
* go/packages: move all overlay tests into overlay_test.go c138986d
* internal/lsp: remove VS Code-specific completion hack b2d8b033
* internal/lsp/cache: handle a few possible panics in PackageStats 4bdfe1a3
* internal/analysisinternal: prevent fillstruct panic on nil package aa3d5013
* internal/lsp/source: add a new symbolStyle configuration option aa94e735
* internal/lsp: fully qualify workspace Symbol matches 84cfedeb
* internal/lsp: add a new regtest to reproduce golang/go#39646 bcbc01e0
* internal/lsp/fake: explicitly set GOPACKAGESDRIVER=off in regtests 6506e20d
* internal/lsp: watch all files in the module and replace target fadf93ff
@stamblerre stamblerre modified the milestones: gopls/v0.5.0, gopls/v0.4.4 Jul 12, 2020
leitzler added a commit to govim/govim that referenced this issue Jul 16, 2020
* internal/lsp/lsprpc: improvements to daemon logging f2c07d7d
* internal/lsp/regtest: simpler way to invert options cf799cae
* internal/lsp: don't keep track of closed overlays 43ed9469
* internal/lsp: fix error in CL 242457 e66011cb
* internal/lsp: clean up the code lens code a8f9df4c
* internal/lsp: refactor generate code lens code c80dc571
* go/packages: add x test overlay test 130c9f19
* internal/lsp: add an importShortcut configuration ddb87c8c
* internal/lsp: handle nil pointer in PackageStats 4cea8971
* internal/lsp/source: handle nil pointer in newBuiltinSignature b42590c1
* internal/lsp: avoid panic caused by assuming file ends with newline 9048b464
* internal/lsp: always show signature as the top line in hover 6acd2ab8
* gopls, internal/lsp: support an extra formatting hook for gofumpt 62a0bb78
* internal/lsp: add changeMethods logic to rename check f8240f79
* internal/lsp/source: add Vendor to list of supported commands 01425d70
* internal/analysisinternal: do not initialize {Get,Set}TypeErrors f739c553
* internal/lsp: watch go.{mod,sum} files, as well as Go files fd294ab1
* internal/lsp/cmd: add a -vv flag for higher verbosity 7342f973
* internal/lsp, go/packages: reproduce and fix golang/go#39646 f1c4188a
* internal/lsp: add new go.mod requires to files with unused require 125cc70a
* internal/lsp: fix rename with Windows line endings 55a0fde5
* internal/lsp/regtest: standardize on 20s timeout e327e101
* internal/lsp/regtest: use a common directory for regtest sandboxes e404ca24
* internal/lsp/regtest: remove stray short timeout df98bc6d
* internal/lsp/regtest: add a regtest for golang/go#39296 3544e8c9
* internal/lsp/cache: handle missing mod file 31b9a748
* internal/lsp: check if analysis enabled in convenience fixes 7370b034
* internal/lsp/regtest: await IWL before running tests 134513de
* internal/lsp/source: disable fillstruct by default d5a74533
* internal/lsp/cache: fix race in RunProcessEnvFunc 065b96d3
* internal/lsp: extract highlighted selection to variable 9c9572d6
* internal/imports: require valid options, move LocalPrefix up 416e8f4f
* internal/lsp/analysis/fillstruct: support anonymous structs 9e0a013e
* cmd/goyacc: print newlines more consistently b22de682
* all: update dependencies in go.mod file 682c4542
* internal/lsp/source: handle nil pointer in comment completions 95bc2bdf
leitzler added a commit to govim/govim that referenced this issue Jul 17, 2020
* internal/lsp: add a configuration to enable/disable links in hover 6ddee643
* internal/lsp: log errors for compute fix edits instead of returning d518495e
* internal/lsp: refactor go.mod diagnostics to simplify the API 58eba7e7
* internal/lsp/lsprpc: improvements to daemon logging f2c07d7d
* internal/lsp/regtest: simpler way to invert options cf799cae
* internal/lsp: don't keep track of closed overlays 43ed9469
* internal/lsp: fix error in CL 242457 e66011cb
* internal/lsp: clean up the code lens code a8f9df4c
* internal/lsp: refactor generate code lens code c80dc571
* go/packages: add x test overlay test 130c9f19
* internal/lsp: add an importShortcut configuration ddb87c8c
* internal/lsp: handle nil pointer in PackageStats 4cea8971
* internal/lsp/source: handle nil pointer in newBuiltinSignature b42590c1
* internal/lsp: avoid panic caused by assuming file ends with newline 9048b464
* internal/lsp: always show signature as the top line in hover 6acd2ab8
* gopls, internal/lsp: support an extra formatting hook for gofumpt 62a0bb78
* internal/lsp: add changeMethods logic to rename check f8240f79
* internal/lsp/source: add Vendor to list of supported commands 01425d70
* internal/analysisinternal: do not initialize {Get,Set}TypeErrors f739c553
* internal/lsp: watch go.{mod,sum} files, as well as Go files fd294ab1
* internal/lsp/cmd: add a -vv flag for higher verbosity 7342f973
* internal/lsp, go/packages: reproduce and fix golang/go#39646 f1c4188a
* internal/lsp: add new go.mod requires to files with unused require 125cc70a
* internal/lsp: fix rename with Windows line endings 55a0fde5
* internal/lsp/regtest: standardize on 20s timeout e327e101
* internal/lsp/regtest: use a common directory for regtest sandboxes e404ca24
* internal/lsp/regtest: remove stray short timeout df98bc6d
* internal/lsp/regtest: add a regtest for golang/go#39296 3544e8c9
* internal/lsp/cache: handle missing mod file 31b9a748
* internal/lsp: check if analysis enabled in convenience fixes 7370b034
* internal/lsp/regtest: await IWL before running tests 134513de
* internal/lsp/source: disable fillstruct by default d5a74533
* internal/lsp/cache: fix race in RunProcessEnvFunc 065b96d3
* internal/lsp: extract highlighted selection to variable 9c9572d6
* internal/imports: require valid options, move LocalPrefix up 416e8f4f
* internal/lsp/analysis/fillstruct: support anonymous structs 9e0a013e
* cmd/goyacc: print newlines more consistently b22de682
* all: update dependencies in go.mod file 682c4542
* internal/lsp/source: handle nil pointer in comment completions 95bc2bdf
leitzler added a commit to govim/govim that referenced this issue Jul 17, 2020
* internal/lsp: add a configuration to enable/disable links in hover 6ddee643
* internal/lsp: log errors for compute fix edits instead of returning d518495e
* internal/lsp: refactor go.mod diagnostics to simplify the API 58eba7e7
* internal/lsp/lsprpc: improvements to daemon logging f2c07d7d
* internal/lsp/regtest: simpler way to invert options cf799cae
* internal/lsp: don't keep track of closed overlays 43ed9469
* internal/lsp: fix error in CL 242457 e66011cb
* internal/lsp: clean up the code lens code a8f9df4c
* internal/lsp: refactor generate code lens code c80dc571
* go/packages: add x test overlay test 130c9f19
* internal/lsp: add an importShortcut configuration ddb87c8c
* internal/lsp: handle nil pointer in PackageStats 4cea8971
* internal/lsp/source: handle nil pointer in newBuiltinSignature b42590c1
* internal/lsp: avoid panic caused by assuming file ends with newline 9048b464
* internal/lsp: always show signature as the top line in hover 6acd2ab8
* gopls, internal/lsp: support an extra formatting hook for gofumpt 62a0bb78
* internal/lsp: add changeMethods logic to rename check f8240f79
* internal/lsp/source: add Vendor to list of supported commands 01425d70
* internal/analysisinternal: do not initialize {Get,Set}TypeErrors f739c553
* internal/lsp: watch go.{mod,sum} files, as well as Go files fd294ab1
* internal/lsp/cmd: add a -vv flag for higher verbosity 7342f973
* internal/lsp, go/packages: reproduce and fix golang/go#39646 f1c4188a
* internal/lsp: add new go.mod requires to files with unused require 125cc70a
* internal/lsp: fix rename with Windows line endings 55a0fde5
* internal/lsp/regtest: standardize on 20s timeout e327e101
* internal/lsp/regtest: use a common directory for regtest sandboxes e404ca24
* internal/lsp/regtest: remove stray short timeout df98bc6d
* internal/lsp/regtest: add a regtest for golang/go#39296 3544e8c9
* internal/lsp/cache: handle missing mod file 31b9a748
* internal/lsp: check if analysis enabled in convenience fixes 7370b034
* internal/lsp/regtest: await IWL before running tests 134513de
* internal/lsp/source: disable fillstruct by default d5a74533
* internal/lsp/cache: fix race in RunProcessEnvFunc 065b96d3
* internal/lsp: extract highlighted selection to variable 9c9572d6
* internal/imports: require valid options, move LocalPrefix up 416e8f4f
* internal/lsp/analysis/fillstruct: support anonymous structs 9e0a013e
* cmd/goyacc: print newlines more consistently b22de682
* all: update dependencies in go.mod file 682c4542
* internal/lsp/source: handle nil pointer in comment completions 95bc2bdf
@myitcv
Copy link
Member Author

@myitcv myitcv commented Jul 30, 2020

We've just seen this again using 6467de6f59a7. Same situation as described before, logs as follows:

@myitcv myitcv reopened this Jul 30, 2020
@stamblerre stamblerre modified the milestones: gopls/v0.4.4, gopls/v1.0.0 Jul 30, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 9, 2020

@myitcv: Sorry for the delay on this. Are you still seeing this with gopls at master?

@myitcv
Copy link
Member Author

@myitcv myitcv commented Sep 10, 2020

@stamblerre - I've just submitted govim/govim@c8c4a6f to verify.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Sep 10, 2020

This appears to still be flakey using 44a2922940c2: we had two flakes in https://github.com/govim/govim/actions/runs/247768908

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 10, 2020

Is there a way to see the gopls logs associated with these failures?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 10, 2020

Note to self--from the earlier logs, the pass case has packages.Loads at snapshots 0, 6, and 12, whereas fail also has a load at snapshot 3. The regression test currently loads at 0, 3, 6, 9, and 12.

@leitzler
Copy link
Contributor

@leitzler leitzler commented Sep 10, 2020

Is there a way to see the gopls logs associated with these failures?

They are available in the artifacts, for example (looking at the run Paul linked above) the artifact file:

  • ubuntu-latest_go1.13.12_vim_v8.2.0931.zip

With the failed test found in the archive as:
Test TestScripts/scenario_default/quickfix_empty_files =>
Log govim/cmd/govim/scenario_default/script-quickfix_empty_files/_tmp/gopls.log

@stamblerre stamblerre added this to Needs Triage in vscode-go: gopls by default Nov 10, 2020
@stamblerre stamblerre moved this from Needs Triage to Critical in vscode-go: gopls by default Nov 10, 2020
@stamblerre stamblerre moved this from Critical to Needs Triage in vscode-go: gopls by default Nov 11, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 11, 2020

I expect that these test failures were caused by the go/packages overlay support, but there is now native support for overlays in 1.16.

@leitzler, @myitcv: Do you know if this test is fixed with Go 1.16?

@stamblerre stamblerre moved this from Needs Triage to In progress in vscode-go: gopls by default Nov 11, 2020
@myitcv
Copy link
Member Author

@myitcv myitcv commented Nov 12, 2020

We will setup a tip instance in our build matrix and try it out.

@stamblerre stamblerre removed this from the gopls/vscode-go milestone Nov 22, 2020
@stamblerre stamblerre removed this from In progress in vscode-go: gopls by default Nov 22, 2020
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.

4 participants