diff --git a/cmd/vet/bool.go b/cmd/vet/bool.go new file mode 100644 index 00000000..e28c2588 --- /dev/null +++ b/cmd/vet/bool.go @@ -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 + } +} diff --git a/cmd/vet/doc.go b/cmd/vet/doc.go index 3b3e5ae7..2b7e1432 100644 --- a/cmd/vet/doc.go +++ b/cmd/vet/doc.go @@ -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 diff --git a/cmd/vet/testdata/bool.go b/cmd/vet/testdata/bool.go new file mode 100644 index 00000000..af6cc011 --- /dev/null +++ b/cmd/vet/testdata/bool.go @@ -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" +}