cmd/vet: type conversions never have side effects

Make the hasSideEffects func use type information to see if a CallExpr
is a type conversion or not. In case it is, there cannot be any side
effects.

Now that vet always has type information, we can afford to use it here.
Update the tests and remove the TODO there too.

Change-Id: I74fdacf830aedf2371e67ba833802c414178caf1
Reviewed-on: https://go-review.googlesource.com/79536
Reviewed-by: Robert Griesemer <gri@golang.org>

diff --git a/src/cmd/vet/assign.go b/src/cmd/vet/assign.go
index bfa5b3032938e22eb37dffa586c1b4699bf80152..223e80d400710b243e6981530bd01bcf799040e2 100644 (file)
--- 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 07c2a93dffa3c2218d86244f1d8f2fed90b4aa88..31e81ec4bf1cea5ad9efe334a6673f97984cd0c8 100644 (file)
--- 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 af6cc011dd60c4d95fcdae7400945b4c943d1e67..bada13ae0dc2bb810703dfb79caddadab7f1326a 100644 (file)
--- 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