mirror of
https://github.com/golang/go
synced 2024-11-05 11:56:12 -07:00
go.tools/cmd/vet: detect stupid boolean conditions
This CL introduces two vet checks. Statistics and code below are from a recent 50mb corpus of public code.
1. Check for redundant conjunctions and disjunctions. This check caught 26 instances, of which 20 were clearly copy/paste bugs and 6 appeared to be mere duplication. A typical example:
if xResolution < 0 || xResolution < 0 {
panic("SetSize(): width < 0 || height < 0")
}
2. Check for expressions of the form 'x != c1 || x != c2' or 'x == c1 && x == c2', with c1 and c2 constant expressions. This check caught 16 instances, of which all were bugs. A typical example:
if rf.uri.Scheme != "http" || rf.uri.Scheme != "ftp" {
rf.uri.Scheme = "file"
}
Fixes golang/go#7622.
LGTM=rsc, r
R=golang-codereviews, jscrockett01, r, gri, rsc
CC=golang-codereviews
https://golang.org/cl/98120043
This commit is contained in:
Josh Bleecher Snyder
parent
ccb18ed35d
commit
7da5f193b7
185
cmd/vet/bool.go
Normal file
185
cmd/vet/bool.go
Normal file
@ -0,0 +1,185 @@
|
||||
// Copyright 2014 The Go Authors. All rights reserved.
|
||||
// Use of this source code is governed by a BSD-style
|
||||
// license that can be found in the LICENSE file.
|
||||
|
||||
// This file contains boolean condition tests.
|
||||
|
||||
package main
|
||||
|
||||
import (
|
||||
"go/ast"
|
||||
"go/token"
|
||||
)
|
||||
|
||||
func init() {
|
||||
register("bool",
|
||||
"check for mistakes involving boolean operators",
|
||||
checkBool,
|
||||
binaryExpr)
|
||||
}
|
||||
|
||||
func checkBool(f *File, n ast.Node) {
|
||||
e := n.(*ast.BinaryExpr)
|
||||
|
||||
var op boolOp
|
||||
switch e.Op {
|
||||
case token.LOR:
|
||||
op = or
|
||||
case token.LAND:
|
||||
op = and
|
||||
default:
|
||||
return
|
||||
}
|
||||
|
||||
comm := op.commutativeSets(e)
|
||||
for _, exprs := range comm {
|
||||
op.checkRedundant(f, exprs)
|
||||
op.checkSuspect(f, exprs)
|
||||
}
|
||||
}
|
||||
|
||||
type boolOp struct {
|
||||
name string
|
||||
tok token.Token // token corresponding to this operator
|
||||
badEq token.Token // token corresponding to the equality test that should not be used with this operator
|
||||
}
|
||||
|
||||
var (
|
||||
or = boolOp{"or", token.LOR, token.NEQ}
|
||||
and = boolOp{"and", token.LAND, token.EQL}
|
||||
)
|
||||
|
||||
// commutativeSets returns all side effect free sets of
|
||||
// expressions in e that are connected by op.
|
||||
// For example, given 'a || b || f() || c || d' with the or op,
|
||||
// commutativeSets returns {{b, a}, {d, c}}.
|
||||
func (op boolOp) commutativeSets(e *ast.BinaryExpr) [][]ast.Expr {
|
||||
exprs := op.split(e)
|
||||
|
||||
// Partition the slice of expressions into commutative sets.
|
||||
i := 0
|
||||
var sets [][]ast.Expr
|
||||
for j := 0; j <= len(exprs); j++ {
|
||||
if j == len(exprs) || hasSideEffects(exprs[j]) {
|
||||
if i < j {
|
||||
sets = append(sets, exprs[i:j])
|
||||
}
|
||||
i = j + 1
|
||||
}
|
||||
}
|
||||
|
||||
return sets
|
||||
}
|
||||
|
||||
// checkRedundant checks for expressions of the form
|
||||
// e && e
|
||||
// e || e
|
||||
// Exprs must contain only side effect free expressions.
|
||||
func (op boolOp) checkRedundant(f *File, exprs []ast.Expr) {
|
||||
seen := make(map[string]bool)
|
||||
for _, e := range exprs {
|
||||
efmt := f.gofmt(e)
|
||||
if seen[efmt] {
|
||||
f.Badf(e.Pos(), "redundant %s: %s %s %s", op.name, efmt, op.tok, efmt)
|
||||
} else {
|
||||
seen[efmt] = true
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// checkSuspect checks for expressions of the form
|
||||
// x != c1 || x != c2
|
||||
// x == c1 && x == c2
|
||||
// where c1 and c2 are constant expressions.
|
||||
// If c1 and c2 are the same then it's redundant;
|
||||
// if c1 and c2 are different then it's always true or always false.
|
||||
// Exprs must contain only side effect free expressions.
|
||||
func (op boolOp) checkSuspect(f *File, exprs []ast.Expr) {
|
||||
// seen maps from expressions 'x' to equality expressions 'x != c'.
|
||||
seen := make(map[string]string)
|
||||
|
||||
for _, e := range exprs {
|
||||
bin, ok := e.(*ast.BinaryExpr)
|
||||
if !ok || bin.Op != op.badEq {
|
||||
continue
|
||||
}
|
||||
|
||||
// In order to avoid false positives, restrict to cases
|
||||
// in which one of the operands is constant. We're then
|
||||
// interested in the other operand.
|
||||
// In the rare case in which both operands are constant
|
||||
// (e.g. runtime.GOOS and "windows"), we'll only catch
|
||||
// mistakes if the LHS is repeated, which is how most
|
||||
// code is written.
|
||||
var x ast.Expr
|
||||
switch {
|
||||
case f.pkg.types[bin.Y].Value != nil:
|
||||
x = bin.X
|
||||
case f.pkg.types[bin.X].Value != nil:
|
||||
x = bin.Y
|
||||
default:
|
||||
continue
|
||||
}
|
||||
|
||||
// e is of the form 'x != c' or 'x == c'.
|
||||
xfmt := f.gofmt(x)
|
||||
efmt := f.gofmt(e)
|
||||
if prev, found := seen[xfmt]; found {
|
||||
// checkRedundant handles the case in which efmt == prev.
|
||||
if efmt != prev {
|
||||
f.Badf(e.Pos(), "suspect %s: %s %s %s", op.name, efmt, op.tok, prev)
|
||||
}
|
||||
} else {
|
||||
seen[xfmt] = efmt
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// hasSideEffects reports whether evaluation of e has side effects.
|
||||
func hasSideEffects(e ast.Expr) bool {
|
||||
safe := true
|
||||
ast.Inspect(e, func(node ast.Node) bool {
|
||||
switch n := node.(type) {
|
||||
// Using CallExpr here will catch conversions
|
||||
// as well as function and method invocations.
|
||||
// We'll live with the false negatives for now.
|
||||
case *ast.CallExpr:
|
||||
safe = false
|
||||
return false
|
||||
case *ast.UnaryExpr:
|
||||
if n.Op == token.ARROW {
|
||||
safe = false
|
||||
return false
|
||||
}
|
||||
}
|
||||
return true
|
||||
})
|
||||
return !safe
|
||||
}
|
||||
|
||||
// split returns a slice of all subexpressions in e that are connected by op.
|
||||
// For example, given 'a || (b || c) || d' with the or op,
|
||||
// split returns []{d, c, b, a}.
|
||||
func (op boolOp) split(e ast.Expr) (exprs []ast.Expr) {
|
||||
for {
|
||||
e = unparen(e)
|
||||
if b, ok := e.(*ast.BinaryExpr); ok && b.Op == op.tok {
|
||||
exprs = append(exprs, op.split(b.Y)...)
|
||||
e = b.X
|
||||
} else {
|
||||
exprs = append(exprs, e)
|
||||
break
|
||||
}
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
func unparen(e ast.Expr) ast.Expr {
|
||||
for {
|
||||
p, ok := e.(*ast.ParenExpr)
|
||||
if !ok {
|
||||
return e
|
||||
}
|
||||
e = p.X
|
||||
}
|
||||
}
|
||||
@ -98,6 +98,12 @@ Flag: -atomic
|
||||
|
||||
Common mistaken usages of the sync/atomic package.
|
||||
|
||||
Boolean conditions
|
||||
|
||||
Flag: -bool
|
||||
|
||||
Mistakes involving boolean operators.
|
||||
|
||||
Build tags
|
||||
|
||||
Flag: -buildtags
|
||||
|
||||
113
cmd/vet/testdata/bool.go
vendored
Normal file
113
cmd/vet/testdata/bool.go
vendored
Normal file
@ -0,0 +1,113 @@
|
||||
// Copyright 2014 The Go Authors. All rights reserved.
|
||||
// Use of this source code is governed by a BSD-style
|
||||
// license that can be found in the LICENSE file.
|
||||
|
||||
// This file contains tests for the bool checker.
|
||||
|
||||
package testdata
|
||||
|
||||
import "io"
|
||||
|
||||
func RatherStupidConditions() {
|
||||
var f, g func() int
|
||||
if f() == 0 || f() == 0 { // OK f might have side effects
|
||||
}
|
||||
if v, w := f(), g(); v == w || v == w { // ERROR "redundant or: v == w || v == w"
|
||||
}
|
||||
_ = f == nil || f == nil // ERROR "redundant or: f == nil || f == nil"
|
||||
|
||||
_ = i == byte(1) || i == byte(1) // TODO conversions are treated as if they may have side effects
|
||||
|
||||
var c chan int
|
||||
_ = 0 == <-c || 0 == <-c // OK subsequent receives may yield different values
|
||||
for i, j := <-c, <-c; i == j || i == j; i, j = <-c, <-c { // ERROR "redundant or: i == j || i == j"
|
||||
}
|
||||
|
||||
var i, j, k int
|
||||
_ = i+1 == 1 || i+1 == 1 // ERROR "redundant or: i\+1 == 1 || i\+1 == 1"
|
||||
_ = i == 1 || j+1 == i || i == 1 // ERROR "redundant or: i == 1 || i == 1"
|
||||
|
||||
_ = i == 1 || i == 1 || f() == 1 // ERROR "redundant or: i == 1 || i == 1"
|
||||
_ = i == 1 || f() == 1 || i == 1 // OK f may alter i as a side effect
|
||||
_ = f() == 1 || i == 1 || i == 1 // ERROR "redundant or: i == 1 || i == 1"
|
||||
|
||||
// Test partition edge cases
|
||||
_ = f() == 1 || i == 1 || i == 1 || j == 1 // ERROR "redundant or: i == 1 || i == 1"
|
||||
_ = f() == 1 || j == 1 || i == 1 || i == 1 // ERROR "redundant or: i == 1 || i == 1"
|
||||
_ = i == 1 || f() == 1 || i == 1 || i == 1 // ERROR "redundant or: i == 1 || i == 1"
|
||||
_ = i == 1 || i == 1 || f() == 1 || i == 1 // ERROR "redundant or: i == 1 || i == 1"
|
||||
_ = i == 1 || i == 1 || j == 1 || f() == 1 // ERROR "redundant or: i == 1 || i == 1"
|
||||
_ = j == 1 || i == 1 || i == 1 || f() == 1 // ERROR "redundant or: i == 1 || i == 1"
|
||||
_ = i == 1 || f() == 1 || f() == 1 || i == 1
|
||||
|
||||
_ = i == 1 || (i == 1 || i == 2) // ERROR "redundant or: i == 1 || i == 1"
|
||||
_ = i == 1 || (f() == 1 || i == 1) // OK f may alter i as a side effect
|
||||
_ = i == 1 || (i == 1 || f() == 1) // ERROR "redundant or: i == 1 || i == 1"
|
||||
_ = i == 1 || (i == 2 || (i == 1 || i == 3)) // ERROR "redundant or: i == 1 || i == 1"
|
||||
|
||||
var a, b bool
|
||||
_ = i == 1 || (a || (i == 1 || b)) // ERROR "redundant or: i == 1 || i == 1"
|
||||
|
||||
// Check that all redundant ors are flagged
|
||||
_ = j == 0 ||
|
||||
i == 1 ||
|
||||
f() == 1 ||
|
||||
j == 0 || // ERROR "redundant or: j == 0 || j == 0"
|
||||
i == 1 || // ERROR "redundant or: i == 1 || i == 1"
|
||||
i == 1 || // ERROR "redundant or: i == 1 || i == 1"
|
||||
i == 1 ||
|
||||
j == 0 ||
|
||||
k == 0
|
||||
|
||||
_ = i == 1*2*3 || i == 1*2*3 // ERROR "redundant or: i == 1\*2\*3 || i == 1\*2\*3"
|
||||
|
||||
// These test that redundant, suspect expressions do not trigger multiple errors.
|
||||
_ = i != 0 || i != 0 // ERROR "redundant or: i != 0 || i != 0"
|
||||
_ = i == 0 && i == 0 // ERROR "redundant and: i == 0 && i == 0"
|
||||
|
||||
// and is dual to or; check the basics and
|
||||
// let the or tests pull the rest of the weight.
|
||||
_ = 0 != <-c && 0 != <-c // OK subsequent receives may yield different values
|
||||
_ = f() != 0 && f() != 0 // OK f might have side effects
|
||||
_ = f != nil && f != nil // ERROR "redundant and: f != nil && f != nil"
|
||||
_ = i != 1 && i != 1 && f() != 1 // ERROR "redundant and: i != 1 && i != 1"
|
||||
_ = i != 1 && f() != 1 && i != 1 // OK f may alter i as a side effect
|
||||
_ = f() != 1 && i != 1 && i != 1 // ERROR "redundant and: i != 1 && i != 1"
|
||||
}
|
||||
|
||||
func RoyallySuspectConditions() {
|
||||
var i, j int
|
||||
|
||||
_ = i == 0 || i == 1 // OK
|
||||
_ = i != 0 || i != 1 // ERROR "suspect or: i != 0 || i != 1"
|
||||
_ = i != 0 || 1 != i // ERROR "suspect or: i != 0 || 1 != i"
|
||||
_ = 0 != i || 1 != i // ERROR "suspect or: 0 != i || 1 != i"
|
||||
_ = 0 != i || i != 1 // ERROR "suspect or: 0 != i || i != 1"
|
||||
|
||||
_ = (0 != i) || i != 1 // ERROR "suspect or: 0 != i || i != 1"
|
||||
|
||||
_ = i+3 != 7 || j+5 == 0 || i+3 != 9 // ERROR "suspect or: i\+3 != 7 || i\+3 != 9"
|
||||
|
||||
_ = i != 0 || j == 0 || i != 1 // ERROR "suspect or: i != 0 || i != 1"
|
||||
|
||||
_ = i != 0 || i != 1<<4 // ERROR "suspect or: i != 0 || i != 1<<4"
|
||||
|
||||
_ = i != 0 || j != 0
|
||||
_ = 0 != i || 0 != j
|
||||
|
||||
var s string
|
||||
_ = s != "one" || s != "the other" // ERROR "suspect or: s != .one. || s != .the other."
|
||||
|
||||
_ = "et" != "alii" || "et" != "cetera" // ERROR "suspect or: .et. != .alii. || .et. != .cetera."
|
||||
_ = "me gustas" != "tu" || "le gustas" != "tu" // OK we could catch this case, but it's not worth the code
|
||||
|
||||
var err error
|
||||
_ = err != nil || err != io.EOF // TODO catch this case?
|
||||
|
||||
// Sanity check and.
|
||||
_ = i != 0 && i != 1 // OK
|
||||
_ = i == 0 && i == 1 // ERROR "suspect and: i == 0 && i == 1"
|
||||
_ = i == 0 && 1 == i // ERROR "suspect and: i == 0 && 1 == i"
|
||||
_ = 0 == i && 1 == i // ERROR "suspect and: 0 == i && 1 == i"
|
||||
_ = 0 == i && i == 1 // ERROR "suspect and: 0 == i && i == 1"
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user