Repository navigation
Add checks specifically for code review #1102
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.
Activity
ainar-g commentedon Oct 18, 2021
If making sure that people aren't just blindly enabling stuff in
staticcheckis a goal then perhaps the new set of checks should only be available from a new tool?staticcomplain:-D ?(Opinions below.)
I feel like including it in
goplsmay be a bit too much, especially if we consider them more false-positive-prone.I also don't think that trying out checks there is a good idea, since that could potentially mean that a lot of good checks won't be activated for people who haven't yet (or don't plant to) set up the code review infrastructure. Although “demoting” false-positive-prone checks to that new category/tool seems fine.
Also, I think that each check should have an explanation and some examples showing when the code in question actually is fine.
(I don't have any strong opinions regarding the diffs and I don't know enough about GH Actions to contribute meaningfully to the glue.)
dominikh commentedon Oct 18, 2021
Love the name. More seriously: we'll use the same binary so that we can reuse the same cache. However, enabling the checks will require an extra flag, and
-checks=allwill not enable them.Yes, we probably can't do it. But that's unfortunate, because that means people will only learn about some bugs until they submit a pull request. And solo developers who push directly to master (cough, me) won't benefit from the checks at all. But I suppose it's on them to integrate the checks with their workflows. Perhaps some pre-commit hooks or something that emulates the code review experience. Or just run the tool manually...
It's not like new checks would be "stuck" in code review for years to come. More likely one or two releases, and only for checks where we're not sure if they can cause false positives.
Absolutely. (Related: #939)
dominikh commentedon Oct 19, 2021
Related: #999
ainar-g commentedon Oct 19, 2021
I'm not familiar with the details of the cache, but that will certainly create some confusion, since
allwould not actually meanallanymore. It's like C compilers with their-Wall,-Wextra, and-Weverythingmess. Nothing critical, but prepare to explain that to confused people filing issues like:And I don't even want to think about what happens once this functionality gets integrated into
golangci-lintand similar “megacheckers”.I mean, that depends on how much of a glue we're talking about. I'm pretty sure I could write something that just diffs the output with that of the merge base in POSIX shell, but that would be very error prone, since the precise locations in code will differ. Perhaps, a simpler-but-more-flawed local solution would be to compare only the total number of issues per module?
Depends on whether you mean “patch releases”, “minor releases”, or “major releases”, because if you meant the last one, then it literally means “one or two years”, heh.
And that also raises the question of how exactly the false-positiveness is decided. The lack of complains from users, unfortunately, does not necessarily mean the lack of issues.
dominikh commentedon Oct 19, 2021
You make a good point, and I may have underestimated the potential for confusion.
To be clear, the actual diffing of diagnostics to only produce the relevant ones will be provided by Staticcheck itself (likely as a standalone utility that processes Staticcheck's JSON output and
git-diffoutput.)The required glue for a shell user would be to run Staticcheck on two versions, and pass the outputs, together with
git-diffoutput, to that tool.For a hosted code review system, the remaining glue would be making the right API calls to turn diagnostics into review comments.
Even Staticcheck with its slow release cycles has about two major releases per year. Anyway, we don't need to decide this now, it was more of a passing thought.
Same way I currently find most issues with existing checks: regularly searching through GitHub issues and commits, watching people interact with the tool and complain about it. For this feature, it might even be possible to automate it: find all PRs with review comments made by Staticcheck, see how many of them were ignored.
ainar-g commentedon Oct 20, 2021
That sounds relatively elementary. In shell pseudocode, off the top of my head:
Other than that, I don't think I have any more feedback. Waiting to try it out!
sluongng commentedon Feb 23, 2022
This would be such a wonderful feature to use with https://github.com/reviewdog/reviewdog 👍
(I have no affiliation with that project)
jvmatl commentedon May 31, 2023
I wasn't aware of this proposal until I submitted #1409 -- I very much like the idea of a the 'diff' capability to show diagnostics that are only relevant at merge time, but, my $0.02:
It's not clear to me from the description, if the intent is for this 'code-review' category to ONLY be available in this 'diff' mode, but I hope not. As a user with a large existing codebase, I would definitely want the ability to run all those false-positive-prone checks against my code and make my own decisions about what is a false-positive, the goal being to establish the baseline assumption that "what is in already in git has been reviewed and all non-false-positives (true positives?) have been addressed, so we only need to look at the diffs"
sort of as a corollary to my first point: making these kinds of tests available is useful functionality even without the diff mode, and a lot of the difficult design decisions and tricky code will be related to the diff management (tracking things that moved, how can I integrate with git to diff two branches, how do we integrate with various CI tools, etc.) -- It seems to me it would make sense to break this into two issues:
dominikh commentedon May 31, 2023
We'll not encourage using it outside of a code review context, but there will most likely at least be a debug flag to run the new checks, and nobody is stopping you from diffing HEAD against the first commit in your history. I don't want to make it too easy, because that will inevitably result in people being lazy and always running the checks, in inappropriate contexts.
I don't want this to turn into a category of noisy checks, and as such I really don't want to offer it before the diffing has been implemented.
jvmatl commentedon May 31, 2023
Fair enough - it's your project, so obviously your call. As long as there's an accessible method to run all the tests against existing code (e.g. your example of diffing against first commit) that totally works for me.
seiyab commentedon Mar 30, 2024
Can I refer to issues labeled "aggressive" for getting idea for my open project?
( https://github.com/seiyab/gost )
dominikh commentedon Mar 30, 2024
@seiyab Sure.