diff --git a/src/cmd/vet/assign.go b/src/cmd/vet/assign.go index bfa5b30329..223e80d400 100644 --- a/src/cmd/vet/assign.go +++ b/src/cmd/vet/assign.go @@ -37,7 +37,7 @@ func checkAssignStmt(f *File, node ast.Node) { } for i, lhs := range stmt.Lhs { rhs := stmt.Rhs[i] - if hasSideEffects(lhs) || hasSideEffects(rhs) { + if hasSideEffects(f, lhs) || hasSideEffects(f, rhs) { continue // expressions may not be equal } if reflect.TypeOf(lhs) != reflect.TypeOf(rhs) { diff --git a/src/cmd/vet/bool.go b/src/cmd/vet/bool.go index 07c2a93dff..31e81ec4bf 100644 --- a/src/cmd/vet/bool.go +++ b/src/cmd/vet/bool.go @@ -9,6 +9,7 @@ package main import ( "go/ast" "go/token" + "go/types" ) func init() { @@ -31,7 +32,7 @@ func checkBool(f *File, n ast.Node) { return } - comm := op.commutativeSets(e) + comm := op.commutativeSets(f, e) for _, exprs := range comm { op.checkRedundant(f, exprs) op.checkSuspect(f, exprs) @@ -53,14 +54,14 @@ var ( // 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 { +func (op boolOp) commutativeSets(f *File, 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 j == len(exprs) || hasSideEffects(f, exprs[j]) { if i < j { sets = append(sets, exprs[i:j]) } @@ -136,16 +137,26 @@ func (op boolOp) checkSuspect(f *File, exprs []ast.Expr) { } // hasSideEffects reports whether evaluation of e has side effects. -func hasSideEffects(e ast.Expr) bool { +func hasSideEffects(f *File, 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 + // Don't call Type.Underlying(), since its lack + // lets us see the NamedFuncType(x) type + // conversion as a *types.Named. + _, ok := f.pkg.types[n.Fun].Type.(*types.Signature) + if ok { + // Conservatively assume that all function and + // method calls have side effects for + // now. This will include func type + // conversions, but it's ok given that + // this is the conservative side. + safe = false + return false + } + // It's a type conversion, which cannot + // have side effects. case *ast.UnaryExpr: if n.Op == token.ARROW { safe = false diff --git a/src/cmd/vet/testdata/bool.go b/src/cmd/vet/testdata/bool.go index af6cc011dd..bada13ae0d 100644 --- a/src/cmd/vet/testdata/bool.go +++ b/src/cmd/vet/testdata/bool.go @@ -8,6 +8,10 @@ package testdata import "io" +type T int + +type FT func() int + func RatherStupidConditions() { var f, g func() int if f() == 0 || f() == 0 { // OK f might have side effects @@ -16,7 +20,15 @@ func RatherStupidConditions() { } _ = 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 + _ = i == byte(1) || i == byte(1) // ERROR "redundant or: i == byte(1) || i == byte(1)" + _ = i == T(2) || i == T(2) // ERROR "redundant or: i == T(2) || i == T(2)" + _ = FT(f) == nil || FT(f) == nil // ERROR "redundant or: FT(f) == nil || FT(f) == nil" + + // TODO: distinguish from an actual func call + _ = (func() int)(f) == nil || (func() int)(f) == nil + + var namedFuncVar FT + _ = namedFuncVar() == namedFuncVar() // OK; still func calls var c chan int _ = 0 == <-c || 0 == <-c // OK subsequent receives may yield different values