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: guard against panics for analyzers that don't yet support generics #50691

Closed
findleyr opened this issue Jan 19, 2022 · 4 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Jan 19, 2022

While we are hopeful to update most analyzers used by gopls for 1.18, we need to guard against panics from analyzers that don't yet support generics.

A general idea is to filter the set of analyses we run on packages that use generics, removing any analyzers that are known to panic or give inaccurate results on generic code. The x/tools/go/analysis/passes/usesgenerics analyzer computes package facts intended to facilitate such filtering.

Ideally we we would do this filtering for users without any intervention required on their part. However, it's not great to just disable analyses silently: is there any way we can surface this action? Maybe we should include a window/showMessage notification the first time that any filtering occurs?

@findleyr findleyr added this to the gopls/v0.8.0 milestone Jan 19, 2022
@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 Jan 19, 2022
@findleyr
Copy link
Contributor Author

findleyr commented Feb 7, 2022

@dominikh, Ian did a bit of research recently, and found that (1) while any staticcheck analyzer that uses IR will panic, it doesn't seem to be problematic to simply recover: the panics cause neither performance issues nor corrupted state. For this reason, we're thinking of just letting it panic on generic code rather than eagerly guarding against the panic.

An argument against doing this would be if there were an analyzer that doesn't panic, but rather reports inaccurate diagnostics. We've seen this before in e.g. #50658. However, this type of behavior should not be related to SSA and is usually easy to fix. If we were to generate a avoid-list of such analyzers, it might be easier to just patch them rather than work around them. Are you aware of any staticcheck analyzers that report inaccurate diagnostics (but don't panic) on generic code? Thanks.

@findleyr
Copy link
Contributor Author

findleyr commented Feb 7, 2022

CC @timothy-king

@dominikh
Copy link
Member

dominikh commented Feb 7, 2022

@findleyr I'm currently auditing all checks in Staticcheck. So far I've only found false negatives, or slightly inaccurate messages (usually by referring to a named type by its base name, not the instantiated form). I'm tempted to expect no false positives, but I can't say for sure. I do plan on finishing the audit before the end of the month, though.

Similarly, I'm planning on failing gracefully in buildir instead of panicing (and of course I'm hoping to actually support type parameters in the IR in time for 1.18.) However, recovering from the panics should be fine in the meantime. I'm not aware of any global state that would get corrupted.

@zpavlinovic zpavlinovic added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Feb 8, 2022
@findleyr
Copy link
Contributor Author

@dominikh thanks for confirming. Closing as this is no longer actionable.

@golang golang locked and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) 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

5 participants