Description
Background
Currently, Staticcheck has two kinds of checks, differentiated by the contexts in which they can be used.
The first kind is the typical Staticcheck: a set of checks that have a very low rate of false-positives, used to find bugs and convoluted code, as well as enforce style rules. These checks can be used anywhere: CI, code review, in editors. For a healthy code base it is expected that Staticcheck doesn't report anything.
The second kind is the quickfix category. It contains checks that are only useful in editors, consisting primarily of highly optional refactorings. Most code bases will have a high number of possible quickfixes, and it wouldn't make sense to run these checks outside of editors.
New category
This issue tracks the addition of a third kind: checks that point out probable bugs, but which are not precise enough to be included in Staticcheck proper. Examples include comparing time.Time values with ==, which is sometimes correct, but usually isn't. Pointing out such questionable code can be helpful during code review, to guide a human's attention. However, said human will ultimately have to decide if the flagged code is indeed wrong. These checks may point out issues even in entirely bug-free code bases, and they shouldn't be used to fail CI or block pull requests from being merged.
The combination of more false positives and use in code review adds some new UX requirements. As a code base grows, it will amass more and more false positives. Obviously, users don't want to see the same irrelevant diagnostics over and over. For a given pull request, they only want to see new diagnostics that are relevant to that pull request. We shouldn't use our existing ignore directives. Those were designed to be used exceedingly rarely, not to be littered all over a code base.
A viable alternative is to run the checks twice: once against the merge base and once against the result of the merge. Only those diagnostics that are new for the merge result are relevant and should be reported. We will have to be slightly clever about the diffing, as old diagnostics can move around, both horizontally and vertically, and they shouldn't be misreported as new diagnostics. It should be possible to track offsets as they move.
Example UI
The UI is envisioned to roughly work as follows:
- The code review pipeline, such as GitHub Actions, runs Staticcheck twice, once on the merge base and once on the merge result.
- A special mode in Staticcheck takes the two outputs and produces a third output, the relevant diagnostics.
- Some platform-specific glue takes these diagnostics and surfaces them as code review comments. Notably, these diagnostics do not represent a hard "fail" of the PR.
- The author and the reviewers use the code review comments to guide their attention, marking invalid comments as resolved, and fixing valid ones.
- Eventually, no unresolved code review comments remain, and the change can be merged.
Initial work
As a first step, I've went through all previously rejected ideas for checks and reopened those that might be viable for this new category. They have been labeled with the aggressive label.
We should also go through all ideas that are currently labeled with needs-decision and evaluate whether they would better fit into the new category. Update: I went through some of them and retagged a subset of those, but in a lot of cases, I am not sure if they would fit into the new category. We have a small set of candidates that I am sure about, and I think I'll need more experience before I can decide on more checks.
Another potentially good source for checks is https://github.com/dgryski/semgrep-go
Open questions:
- Should these checks also run as part of gopls? If so, how can we make sure that false positives aren't distracting? Would it be enough to use the
Informationseverity? - Should the diff functionality be made available for all checks, or be restricted to the new checks?
- Who will write the glue? Where will it be hosted? Which systems can be supported?
- Can (and should) we (ab)use this new category to trial-run checks before including them in Staticcheck proper?
- Will GitHub (or any other system) let us propose fixes as part of review comments? Update: Yes. GitHub has code blocks of type
suggestion, which render specially and can be automatically applied.