x/tools/gopls: noisy results for the undeclaredname analyzer in code with parse errors

[findleyr]

Reported by @heschi: the new analysis driver in [email protected] results in very noisy analysis for code that contains parse errors, due to the undeclaredname analyzer.

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

CC @adonovan

[adonovan]

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]

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]

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

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]

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]

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]

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

[findleyr]

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

[gopherbot]

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

[gopherbot]

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

[gopherbot]

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