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

[myitcv]

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

[stamblerre]

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]

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]

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]

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]

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]

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]

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

[stamblerre]

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]

...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]

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]

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?

[stamblerre]

@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.

[myitcv]

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.