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

Comments

Projects
None yet
9 participants
@josharian
Contributor

josharian commented May 6, 2016

[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 :)

@josharian josharian added this to the Go1.8 milestone May 6, 2016

@dominikh

This comment has been minimized.

Member

dominikh commented May 6, 2016

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

@robpike

This comment has been minimized.

Contributor

robpike commented May 7, 2016

Only 4 examples in the whole tree would argue against.

@dominikh

This comment has been minimized.

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

This comment has been minimized.

Contributor

robpike commented May 7, 2016

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

@dominikh

This comment has been minimized.

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

This comment has been minimized.

Contributor

valyala commented May 23, 2016

@gopherbot

This comment has been minimized.

gopherbot commented May 23, 2016

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

@mvdan

This comment has been minimized.

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

This comment has been minimized.

Member

bradfitz commented Jun 16, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment