go-tools
go-tools copied to clipboard
dominikh
staticcheck: false positive in SA4000 for `<-ch || <-ch`
The code if <-ch || <-ch was used in a test to check that two calls to some function, that sent a value to ch, didn't return true. We could run into the same issue with function calls.
This is a tough one. It's obviously a false positive, and valid code, but not flagging any such channel receives (or even function calls) would hide many more true positives than it would prevent false positives.
Hi @dominikh, thanks for keeping track of this. I just encountered this (in the slightly different form <-ch && <-ch) and I was thinking of working on a fix.
It's obviously a false positive, and valid code, but not flagging any such channel receives (or even function calls) would hide many more true positives than it would prevent false positives.
Just for my understanding, what do you mean here? While a function can be referentially transparent and evaluating whether f() && f() makes sense or not definitely seems tough, reading from a channel sounds like inherently meant to be impure, like reading from a mutable queue. Do you have some example in mind where <-ch || <- ch could be a true positive? I can think of cases where you purposefully create a channel that always returns the same value, but it seems relatively unlikely that that would be used in conjunction with this pattern.
I'm new to Go, so happy to stand corrected before I start working on something way more complex than I thought.
@stefanobaghino The purpose of SA4000 is really to detect typos and similar accidents. b && b isn't useful. We don't know what the author meant to say, it might've been just b, or b && otherB, among others. The same goes for the channel example. Maybe it wasn't meant to receive twice, or maybe it wasn't meant to receive from the same channel both times. More often than not, <-ch || <-ch really wasn't meant to receive twice from ch.
It'd be trivial to avoid the false positive by not emitting the diagnostic for channel receives. But doing so would cause us to miss many actual bugs.
It'd be trivial to avoid the false positive by not emitting the diagnostic for channel receives. But doing so would cause us to miss many actual bugs.
Is it possible to use a different checker for this case that is subjective, then, so that it can more easily be skipped by those looking for something that detects certain problems instead of potential problems? We have valid code doing this intentionally, and I'd rather not have to grep out the specific error to determine if we have a regression somewhere.
How many instances of that pattern do you have? Is ignoring the specific instances (via https://staticcheck.dev/docs/configuration/#line-based-linter-directives) not viable?
Only one. As a matter of principal, I don't add annotations in my code in order to make tooling happy. Tooling should work correctly with the code as written, not the other way around.