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/79536Reviewed-by: Robert Griesemer <gri@golang.org>

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) {

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:
+			// 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

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