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: various analyzers panic #42857

Closed
pjweinb opened this issue Nov 27, 2020 · 3 comments
Closed

x/tools/gopls: various analyzers panic #42857

pjweinb opened this issue Nov 27, 2020 · 3 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.

Comments

@pjweinb
Copy link

pjweinb commented Nov 27, 2020

With gopls at tip, and go either at 1.15.1 or at tip, internal/lsp/testdata/semantic/a.go causes several analyzers to panic.

a.go contains code that will not build, and some analyzers panic on a type assertion where the types look like []interface{}.(string).

gopls will run analyzers on code that does not build, so it can test proposed fixes,
that is, in cache/analysis.go:325:

		data.err = errors.Errorf("analysis skipped due to errors in package: %v", pkg.GetErrors())
		return data
}

the test cannot be replaced by if len(pgk.GetErrors()) > 0, because then the tests for proposed fixes fail.

Most of the tests for analyzers won't run on files that don't build (nor will the tests in internal/lsp/source) so one could argue that these analyzers, at least, aren't intended to work on such code. Choices seem to be:

  • Put up with panic messages from gopls (bad user experience)
  • Fix the analyzers one by one (whack-a-mole)
  • For code that does not build, only run the analyzers that propose fixes (contributions welcome)
  • Do a more careful job at analysis.go:325 in deciding whether to run analyzers (contributions welcome)
  • Fix a.go so it doesn't trigger panics (push the problem off into the future)
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Nov 27, 2020
@gopherbot gopherbot added this to the Unreleased milestone Nov 27, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/273766 mentions this issue: go/analysis: avoid panic in ifaceassart

gopherbot pushed a commit to golang/tools that referenced this issue Nov 30, 2020
Presently ifaceassert.go panics if given asserts whose
types reduce to
[]interface{}.(string)
which is illegal Go. Its tests won't even run on such code, but
gopls will happily invoke it. This CL adds a test for nil.

See golang/go#42857

Change-Id: I2791f4bd0b58559e65e6590822ac8f4123989273
Reviewed-on: https://go-review.googlesource.com/c/tools/+/273766
Reviewed-by: Michael Matloob <[email protected]>
Trust: Peter Weinberger <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/420538 mentions this issue: internal/lsp/cache: invalidate analysis results on packages.Load

gopherbot pushed a commit to golang/tools that referenced this issue Oct 11, 2022
Most invalidation happens in snapshot.clone, but to be safe we were also
invalidating package data when new metadata is set via packages.Load. At
the time, I wasn't sure why this was necessary.

Now I understand: with experimentalUseInvalidMetadata it is possible
that we re-compute invalidated data using stale metadata in between the
initial clone and the subsequent reload. I noticed that it was also
possible to have stale analysis results after the Load results arrive.

Fix this by invalidating analysis results on Load, in addition to
packages. Factor out invalidation into a new helper method.

Since we believe this may fix analyzer panics, re-enable strict handling
of analysis panics during tests.

For golang/go#56035
For golang/go#54762
For golang/go#42857

Change-Id: I8c28e0700f8c16c58df4ecf60f6127b1c05d6dc5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420538
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
@adonovan
Copy link
Member

The analysis driver was rewritten in CL 443099 and related work, and we took a zero tolerance approach to analyzer panics, so I think this is fixed.

@golang golang locked and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants