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

cmd/vet: check for duplicate input to some binary ops #15586

Open
josharian opened this issue May 6, 2016 · 9 comments
Open

cmd/vet: check for duplicate input to some binary ops #15586

josharian opened this issue May 6, 2016 · 9 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@josharian
Copy link
Contributor

[moved from #15570]

@dominikh's staticcheck found some bugs in the standard library -- see #15570. This issue is to consider whether it's worth adding a vet check along the same lines.

The check would be to look for expressions of the form (x BOP x), where:

  • x is not of type float
  • BOP is one of: & && | || == != - / % ^ &^
  • x is a side-effect free expression (see the boolean conditions check)

These expressions are either redundant or have a constant value (with some very rare exceptions, like division and the smallest negative integer), which indicates that they are probably a mistake, and in any case would be better written in another way.

cc @robpike for opinions

cc @valyala in case you are interested in playing with more vet checks :)

@dominikh
Copy link
Member

dominikh commented May 6, 2016

The following operators as well: > >= < <=

@robpike
Copy link
Contributor

robpike commented May 7, 2016

Only 4 examples in the whole tree would argue against.

@dominikh
Copy link
Member

dominikh commented May 7, 2016

Some data points from staticcheck:

  • I checked 213690 packages. That's excluding any packages in /vendor/ (and Godep's _workspace), but includes forks, and some people vendor code in /internal/
  • In total, there were 1709 matches
  • After filtering out obvious false positives (reflect and errors tests in Go) as well as some Go interpreter project that was forked a number of times, there are 1181 matches left
  • After checking about 200 random samples, the vast majority are real bugs, primarily in tests.
  • staticcheck also considers function calls that might have side effects, which is how it found the bugs in the net package's tests.
    • A tiny amount (<10) of matches in all of the 1181 matches were false positives due to this. The other matches with function calls were legitimate bugs.
    • However, the majority of all matches didn't include function calls, other than the builtin len

@robpike
Copy link
Contributor

robpike commented May 7, 2016

You report a false positive rate of over 40%. That is high.

@dominikh
Copy link
Member

dominikh commented May 7, 2016

The issue with those false positives is that they were hundreds of tests like these:

// Test Complex128 == Complex128
func TestCheckBinaryTypedExprComplex128EqlComplex128(t *testing.T) {
    env := makeEnv()

    expectConst(t, `complex128(0xffffffff + 0xffffffff * 1i) == complex128(0xffffffff + 0xffffffff * 1i)`, env, complex128(0xffffffff + 0xffffffff * 1i) == complex128(0xffffffff + 0xffffffff * 1i), reflect.TypeOf(complex128(0xffffffff + 0xffffffff * 1i) == complex128(0xffffffff + 0xffffffff * 1i)))
}

(from github.com/rocky/go-eval/checkbinaryexpr_typed_gen_test.go)

It's part of a Go interpreter project, with hundreds of code-gened tests like those, and the project had several forks.

If you ignore that particular project, you're left with about 20 false positives among all matches. Those are in generated code, in tests of reflect and errors and in some libraries that ensure that interface values are comparable.

@valyala
Copy link
Contributor

valyala commented May 23, 2016

@josharian , see the draft CL.

@gopherbot
Copy link

CL https://golang.org/cl/23350 mentions this issue.

@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 6, 2016
@rsc rsc modified the milestones: Go1.9, Go1.8 Nov 3, 2016
@mvdan
Copy link
Member

mvdan commented Jun 16, 2017

This is marked NeedsDecision and it seems like noone is working on it - should we postpone to 1.10?

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jun 16, 2017
@bradfitz
Copy link
Contributor

Done.

We try to get to NeedsDecision labels in our proposal review meetings, but I guess we never made it here. :-/

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

10 participants