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: noisy results for the undeclaredname analyzer in code with parse errors #59888

Closed
findleyr opened this issue Apr 28, 2023 · 11 comments
Assignees
Labels
gopls/incremental gopls Issues related to the Go language server, gopls. release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@findleyr
Copy link
Contributor

Reported by @heschi: the new analysis driver in gopls@v0.12.0-pre.1 results in very noisy analysis for code that contains parse errors, due to the undeclaredname analyzer.
image

We should not run this analyzer (or any analyzer, AFAICT) if there are parse errors.

CC @adonovan

@findleyr findleyr added this to the gopls/v0.12.0 milestone Apr 28, 2023
@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 Apr 28, 2023
@adonovan
Copy link
Member

adonovan commented May 3, 2023

The underlying issue here is poor error recovery in go/parser; see #58833

Running the analyzer only in the presence of zero parse errors, or a small number, or a set of errors whose locations suggest a narrow "blast radius", seems like a good practical step.

@heschi
Copy link
Contributor

heschi commented May 3, 2023

I must be missing something, because every time I say this nobody bites, but I'm going to say it again for the record :)

I don't understand why the type checking in the analyzer behaves differently than the type checking gopls does for itself. Aren't they doing essentially the exact same thing? How is the analyzer seeing errors syntactically earlier in the file than the gopls type check run?

(A wild guess: is the file being fixed and the analyzers run on the original content?)

@findleyr
Copy link
Contributor Author

findleyr commented May 3, 2023

@heschi why do you think the analyzer type checker is behaving differently? This seems clearly to me like a bug introduced when we refactored analysis: we now run analyzers in the presence of parse errors, when we didn't before.

(A wild guess: is the file being fixed and the analyzers run on the original content?)

They both use the same parsing logic.

@heschi
Copy link
Contributor

heschi commented May 3, 2023

Er, because the analyzer is reporting a bazillion errors and the "compiler" source only has one...no? Maybe this is the part I'm missing.

@adonovan
Copy link
Member

adonovan commented May 3, 2023

What's happening here is:

  • the parser encounters a type error at "io". That's the red squiggly.
  • during recovery it consumes a bunch of tokens and produces a bad tree.
  • the type checker generates an enormous number of spurious "undeclared" errors from the bad tree.
  • the type-error analyzers in gopls try to augment those errors with quick fixes, but in doing so end up "owning" the errors. That's why the type errors have gold squigglies.

The ultimate fix is in the parser, but we could also do one or more of

  • make the type checker more cautious about bad trees;
  • make gopls' type-error analyzers less gung ho in the face of bad trees; and
  • keep the red (compiler) not gold (analysis) state for those errors.

@findleyr
Copy link
Contributor Author

findleyr commented May 3, 2023

Er, because the analyzer is reporting a bazillion errors and the "compiler" source only has one...no? Maybe this is the part I'm missing.

This is a type error analyzer. We don't show type errors when there are parse errors, but we are running analysis, now. That's why these errors are showing up with 'undeclaredname' source, rather than 'compiler' source.

@heschi
Copy link
Contributor

heschi commented May 3, 2023

Thanks both, I get it now. I probably wrote the code that's claiming the errors. Sorry :)

@findleyr
Copy link
Contributor Author

findleyr commented May 3, 2023

@adonovan I think the problem is slightly more subtle: if we weren't suppressing the type error, the diagnostics would be merged into the 'compiler' diagnostic. But we do suppress the type-checker errors, so there is nothing to merge with.

@adonovan adonovan self-assigned this May 3, 2023
@gopherbot
Copy link

Change https://go.dev/cl/492295 mentions this issue: gopls/internal/lsp/analysis: skip type-error analyzers on bad nodes

@gopherbot
Copy link

Change https://go.dev/cl/492988 mentions this issue: gopls/internal/regtest/marker: migrate @diag to new marker tests

@gopherbot
Copy link

Change https://go.dev/cl/493616 mentions this issue: gopls/internal/lsp/cache: skip type errors after parse errors

gopherbot pushed a commit to golang/tools that referenced this issue May 9, 2023
This CL contains the result of a relentlessly unsatisfactory
several hours trying to migrate the @diag markers to the new
framework.

Success was achieved for bad_test, generated, generator, noparse,
rundespiteerrors, undeclared. Some trickier cases remain.

Observations:
- tests without go.mod files result in packages named after
  the absolute directory path. Is this intentional?
- the new @diag markers ignore the kind and severity fields.
  Should we restore them?
- new @diag markers are quite particular about ranges.
- IIUC, the (old) no_diagnostics special case was unnecessary
  since the framework checks got=want not got>want. Deleted.

This was supposed to be an easy preparatory step before
fixing the issue below.

Updates golang/go#59888

Change-Id: I5cd9c37804f1fd627186c03f5b5d4c24d336a285
Reviewed-on: https://go-review.googlesource.com/c/tools/+/492988
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls/incremental gopls Issues related to the Go language server, gopls. release-blocker 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