x/tools/gopls: duplicate initial diagnostics sent for a file containing an error #36243

myitcv · 2019-12-21T16:13:25Z

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

$ go version
go version devel +5c6f42773c Thu Dec 19 22:16:18 2019 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20191219230827-5e752206af05
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20191219230827-5e752206af05

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

What did you do?

Using govim, open main.go:

package main

blah

func main() {
}

Observe there are two identical (other than the version number) PublishDiagnostic notifications for main.go

fail.log

What did you expect to see?

Only one PublishDiagnostic notification for main.go because the diagnostics have not changed.

What did you see instead?

Per above.

This was spotted as part of govim/govim#637

cc @stamblerre

The text was updated successfully, but these errors were encountered:

stamblerre · 2019-12-23T19:34:10Z

Yeah, this is a result of the initial workspace load sending diagnostics for files, followed by the didOpen for the specific file. My reasoning was that we should probably always send a diagnostic for a file after you open it, even if you've already sent the diagnostic before. I'm not sure if that's necessarily correct, but it seemed reasonable to me.

myitcv · 2019-12-28T20:32:30Z

My reasoning was that we should probably always send a diagnostic for a file after you open it, even if you've already sent the diagnostic before.

Can you expand on why?

The client already has the diagnostics and so if they haven't changed, there's nothing to do no?

stamblerre · 2019-12-30T18:47:37Z

I think for me it's a combination of version information / the confirmation that the file has been opened. All prior diagnostics will have been sent for an unspecified version of the file, whereas when the client sends the first version on didOpen we will now have a version to reply with. For the gopls check command, we decided that this command is completed once we get a reply of diagnostics for the version of the file we specified, and I don't think that's an unreasonable use case.

myitcv · 2019-12-31T06:24:44Z

All prior diagnostics will have been sent for an unspecified version of the file, whereas when the client sends the first version on didOpen we will now have a version to reply with

👍 - that's a great point, had totally forgotten that.

we decided that this command is completed once we get a reply of diagnostics for the version of the file we specified

👍 - that also makes complete sense.

In which case, on the basis it doesn't make sense to send initial diagnostics for files where there are no errors (would also be a potentially large overhead too), I'll close this issue. i.e. we'll fix our expectations on the govim side.

Thanks very much for the detail.

cc @findleyr given our conversation by email

myitcv · 2019-12-31T15:34:52Z

Actually, re-opening to clarify one point. Seemingly the initial diagnostics do not include staticcheck errors.

@stamblerre can you clarify what we can expect in the initial diagnostics?

myitcv · 2019-12-31T16:58:39Z

One other slight vagary related to that last question: if there are two files, both of which only have go/analysis warnings, it seems that no initial diagnostics are sent, but also that on opening the first file, diagnostics (including the warnings) for both files are sent.

Getting some clarification here will help with the govim integration tests.

myitcv · 2020-01-01T07:23:57Z

I've separately raised #36340 for the go/analysis diagnostics vagaries.

This uber PR contains a number of changes to our testscript tests: * Drop all errlogmatch's looking for assertions where we really should not need them. Historically we needed these because the threading model of gopls was a bit broken. i.e. despite us notifying of a change to a file, unless we waited for diagnostics to be available then we couldn't guarantee that, for example, completions would be correct * Fix up all go.mod files to include go directives so that we don't fall foul of golang/go#36144 * Where we do require errlogmatch's, make sure we follow the existing semantics per golang/go#36243 and golang/go#36340 * Fix up some scripts so that we don't have initial errors in the file and hence don't need to soak up initial diagnostics * Use sleep $DEFAULT_ERRLOGMATCH_WAIT where we expect there to be no diagnostic errors * Only use errlogmatch on diagnostic publications where we need to know that a file has been opened, e.g. file watching, or where we need to soak up initial diagnostic notifications (see golang/go#36243) * Move all commented-out errlogmatch commands matching on the number of errors to the end of each script. We still can't enable these

stamblerre · 2020-01-02T17:28:26Z

We decided that the overhead of running analyses (which may include staticcheck) is too high, particularly because users may be opening their entire GOPATHs. Additionally, you may not necessarily want to see a staticcheck warning for a file in a package you aren't developing in. So the initial diagnostics will only contain compile errors, but when you open a file, all of the files in its package will be diagnosed and analyzed. Does that clarify things?

myitcv · 2020-01-02T18:37:40Z

...particularly because users may be opening their entire GOPATHs

I think this is an argument against doing anything to be honest. If someone opens up their entire GOPATH then they really won't want to see compile errors for all packages.

Additionally, you may not necessarily want to see a staticcheck warning for a file in a package you aren't developing in

I think the same argument applies to syntax and compile errors to be honest, particularly if you open up your entire GOPATH (or even a very large module for that matter)

So the initial diagnostics will only contain compile errors, but when you open a file, all of the files in its package will be diagnosed and analyzed

Just to clarify, the initial diagnostics will include syntax and compile errors for all packages under the root (directory if in GOPATH mode, main module if in module mode)... and the diagnostics for a given package will be updated to include analysis results when a file in said package is opened.

I understand there's a balance to be struck here with references/renaming. Could we actually do all the work of type checking these packages but not actually send any diagnostics? Isn't that what we really intend here?

stamblerre · 2020-01-02T20:55:31Z

I personally think that users would prefer to have compile errors for their workspace, particularly if we plan to show diagnostic errors for reverse dependencies. If you open a file and change it so that it breaks another file in the module, I think we would want to show that the user. And we can't be consistent with that unless we always show those types of errors, on start up.

myitcv · 2020-01-03T08:53:54Z

And we can't be consistent with that unless we always show those types of errors, on start up.

Not sure I understand how showing diagnostics on startup is related to (consistently) showing reverse dependency diagnostics. Please can you explain further?

This uber PR contains a number of changes to our testscript tests: * Drop all errlogmatch's looking for assertions where we really should not need them. Historically we needed these because the threading model of gopls was a bit broken. i.e. despite us notifying of a change to a file, unless we waited for diagnostics to be available then we couldn't guarantee that, for example, completions would be correct * Fix up all go.mod files to include go directives so that we don't fall foul of golang/go#36144 * Where we do require errlogmatch's, make sure we follow the existing semantics per golang/go#36243 and golang/go#36340 * Fix up some scripts so that we don't have initial errors in the file and hence don't need to soak up initial diagnostics * Use sleep $DEFAULT_ERRLOGMATCH_WAIT where we expect there to be no diagnostic errors * Only use errlogmatch on diagnostic publications where we need to know that a file has been opened, e.g. file watching, or where we need to soak up initial diagnostic notifications (see golang/go#36243) * Move all commented-out errlogmatch commands matching on the number of errors to the end of each script. We still can't enable these

This uber PR contains a number of changes to our testscript tests: * Drop all errlogmatch's looking for assertions where we really should not need them. Historically we needed these because the threading model of gopls was a bit broken. i.e. despite us notifying of a change to a file, unless we waited for diagnostics to be available then we couldn't guarantee that, for example, completions would be correct * Fix up all go.mod files to include go directives so that we don't fall foul of golang/go#36144 * Where we do require errlogmatch's, make sure we follow the existing semantics per golang/go#36243 and golang/go#36340 * Fix up some scripts so that we don't have initial errors in the file and hence don't need to soak up initial diagnostics * Use sleep $DEFAULT_ERRLOGMATCH_WAIT where we expect there to be no diagnostic errors * Only use errlogmatch on diagnostic publications where we need to know that a file has been opened, e.g. file watching, or where we need to soak up initial diagnostic notifications (see golang/go#36243) * Move all commented-out errlogmatch commands matching on the number of errors to the end of each script. We still can't enable these Add some initial tests that help to verify our expectations around diagnostics being published by gopls. Set GOPATH and GOCACHE consistently for the install testscript scripts to speed them up. Call t.Parallel in a couple of places to speed up the entire test run. Add a test (that is skipped) which capture the essence of golang/go#36144.

stamblerre · 2020-01-06T21:00:10Z

@myitcv: For me, it's really just about consistency. If we are going to show diagnostics for some un-opened packages in the workspace, then I think we should show them for all packages. Users may get confused if they see diagnostics pop up for reverse dependencies, since the link to the original package may not be immediately visible to them. Similarly, if find references and implementations are expected to work across all packages in the workspace, then I think it makes sense for diagnostics to work on all packages in the workspace.

As an example, imagine you have packages a and b in your workspace, where b depends on a. Only b is opened, but a has a compile error in it (say, an unused variable like var x int lingering in a function body). Then, say you change an exported function in b, such that there is now an error in a. If we don't show the original var x int diagnostic on start, we will now surface 2 diagnostics for a, one for the API change and one for the unused variable. I would imagine this could be a confusing user experience, since the unused variable has been there the whole time.

This uber PR contains a number of changes to our testscript tests: * Drop all errlogmatch's looking for assertions where we really should not need them. Historically we needed these because the threading model of gopls was a bit broken. i.e. despite us notifying of a change to a file, unless we waited for diagnostics to be available then we couldn't guarantee that, for example, completions would be correct * Fix up all go.mod files to include go directives so that we don't fall foul of golang/go#36144 * Where we do require errlogmatch's, make sure we follow the existing semantics per golang/go#36243 and golang/go#36340 * Fix up some scripts so that we don't have initial errors in the file and hence don't need to soak up initial diagnostics * Use sleep $DEFAULT_ERRLOGMATCH_WAIT where we expect there to be no diagnostic errors * Only use errlogmatch on diagnostic publications where we need to know that a file has been opened, e.g. file watching, or where we need to soak up initial diagnostic notifications (see golang/go#36243) * Move all commented-out errlogmatch commands matching on the number of errors to the end of each script. We still can't enable these Add some initial tests that help to verify our expectations around diagnostics being published by gopls. Set GOPATH and GOCACHE consistently for the install testscript scripts to speed them up. Call t.Parallel in a couple of places to speed up the entire test run. Add a test (that is skipped) which capture the essence of golang/go#36144.

myitcv · 2020-01-07T10:39:04Z

If we are going to show diagnostics for some un-opened packages in the workspace, then I think we should show them for all packages

Thanks, I very much see your point of view now.

It's definitely a balance; because if I open up a module/GOPATH with a view to specifically changing one file/package, but there are a whole load of other, unrelated errors in my diagnostics, I can't filter out the noise. I might still have 10+ diagnostic errors and yet I can compile, test and run the package I'm working on just fine.

But it's fair to say the situation I describe could equally come about if I opened a file in a package with errors and then the package I'm working on. i.e. we don't have any way of filtering the diagnostics.

Perhaps that's actually the problem I'm concerned about, which is quite clearly beyond the scope of this issue/discussion. I'll close this issue and raise a separate issue with discussion on that.

Thanks again for taking the time to talk this through.

This uber PR contains a number of changes to our testscript tests: * Drop all errlogmatch's looking for assertions where we really should not need them. Historically we needed these because the threading model of gopls was a bit broken. i.e. despite us notifying of a change to a file, unless we waited for diagnostics to be available then we couldn't guarantee that, for example, completions would be correct * Fix up all go.mod files to include go directives so that we don't fall foul of golang/go#36144 * Where we do require errlogmatch's, make sure we follow the existing semantics per golang/go#36243 and golang/go#36340 * Fix up some scripts so that we don't have initial errors in the file and hence don't need to soak up initial diagnostics * Use sleep $DEFAULT_ERRLOGMATCH_WAIT where we expect there to be no diagnostic errors * Only use errlogmatch on diagnostic publications where we need to know that a file has been opened, e.g. file watching, or where we need to soak up initial diagnostic notifications (see golang/go#36243) * Move all commented-out errlogmatch commands matching on the number of errors to the end of each script. We still can't enable these Add some initial tests that help to verify our expectations around diagnostics being published by gopls. Set GOPATH and GOCACHE consistently for the install testscript scripts to speed them up. Call t.Parallel in a couple of places to speed up the entire test run. Add a test (that is skipped) which capture the essence of golang/go#36144.

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 Dec 21, 2019

gopherbot added this to the Unreleased milestone Dec 21, 2019

gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Dec 21, 2019

stamblerre modified the milestones: Unreleased, gopls v1.0 Dec 23, 2019

myitcv closed this as completed Dec 31, 2019

myitcv reopened this Dec 31, 2019

myitcv mentioned this issue Dec 31, 2019

x/tools/gopls: (initial) diagnostics inconsistently sent/resent for go/analysis results #36340

Closed

myitcv closed this as completed Jan 7, 2020

myitcv mentioned this issue Jan 7, 2020

x/tools/gopls: unable to filter irrelevant diagnostics #36427

Closed

stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.4.0 Jul 22, 2020

golang locked and limited conversation to collaborators Jul 22, 2021

gopherbot added the FrozenDueToAge label Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/tools/gopls: duplicate initial diagnostics sent for a file containing an error #36243

x/tools/gopls: duplicate initial diagnostics sent for a file containing an error #36243

myitcv commented Dec 21, 2019

stamblerre commented Dec 23, 2019

myitcv commented Dec 28, 2019

stamblerre commented Dec 30, 2019

myitcv commented Dec 31, 2019

myitcv commented Dec 31, 2019

myitcv commented Dec 31, 2019

myitcv commented Jan 1, 2020

stamblerre commented Jan 2, 2020

myitcv commented Jan 2, 2020

stamblerre commented Jan 2, 2020

myitcv commented Jan 3, 2020

stamblerre commented Jan 6, 2020

myitcv commented Jan 7, 2020

x/tools/gopls: duplicate initial diagnostics sent for a file containing an error #36243

x/tools/gopls: duplicate initial diagnostics sent for a file containing an error #36243

Comments

myitcv commented Dec 21, 2019

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

stamblerre commented Dec 23, 2019

myitcv commented Dec 28, 2019

stamblerre commented Dec 30, 2019

myitcv commented Dec 31, 2019

myitcv commented Dec 31, 2019

myitcv commented Dec 31, 2019

myitcv commented Jan 1, 2020

stamblerre commented Jan 2, 2020

myitcv commented Jan 2, 2020

stamblerre commented Jan 2, 2020

myitcv commented Jan 3, 2020

stamblerre commented Jan 6, 2020

myitcv commented Jan 7, 2020

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

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