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: duplicate initial diagnostics sent for a file containing an error #36243

Closed
myitcv opened this issue Dec 21, 2019 · 13 comments
Closed

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Dec 21, 2019

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

@gopherbot gopherbot added this to the Unreleased milestone Dec 21, 2019
@gopherbot gopherbot added the Tools label Dec 21, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Dec 23, 2019

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.

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

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Dec 28, 2019

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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Dec 30, 2019

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

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Dec 31, 2019

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 myitcv closed this Dec 31, 2019
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Dec 31, 2019

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 myitcv reopened this Dec 31, 2019
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Dec 31, 2019

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

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 1, 2020

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

myitcv added a commit to myitcvforks/govim that referenced this issue Jan 1, 2020
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
myitcv added a commit to myitcvforks/govim that referenced this issue Jan 1, 2020
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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 2, 2020

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

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 2, 2020

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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 2, 2020

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

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 3, 2020

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?

myitcv added a commit to govim/govim that referenced this issue Jan 4, 2020
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
myitcv added a commit to govim/govim that referenced this issue Jan 5, 2020
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
myitcv added a commit to govim/govim that referenced this issue Jan 5, 2020
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
myitcv added a commit to govim/govim that referenced this issue Jan 5, 2020
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
myitcv added a commit to govim/govim that referenced this issue Jan 5, 2020
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
myitcv added a commit to govim/govim that referenced this issue Jan 5, 2020
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 a commit to govim/govim that referenced this issue Jan 6, 2020
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 a commit to govim/govim that referenced this issue Jan 6, 2020
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 a commit to govim/govim that referenced this issue Jan 6, 2020
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 a commit to govim/govim that referenced this issue Jan 6, 2020
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 a commit to govim/govim that referenced this issue Jan 6, 2020
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 a commit to govim/govim that referenced this issue Jan 6, 2020
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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 6, 2020

@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 added a commit to govim/govim that referenced this issue Jan 7, 2020
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 a commit to govim/govim that referenced this issue Jan 7, 2020
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

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 7, 2020

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.

@myitcv myitcv closed this Jan 7, 2020
myitcv added a commit to govim/govim that referenced this issue Jan 7, 2020
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 a commit to govim/govim that referenced this issue Jan 7, 2020
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 a commit to govim/govim that referenced this issue Jan 8, 2020
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 a commit to govim/govim that referenced this issue Jan 8, 2020
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 a commit to govim/govim that referenced this issue Jan 8, 2020
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 a commit to govim/govim that referenced this issue Jan 8, 2020
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 a commit to govim/govim that referenced this issue Jan 8, 2020
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.
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.

None yet
3 participants
You can’t perform that action at this time.