From: Daniel Martí <mvdan@mvdan.cc>
Date: Wed, 22 Nov 2017 21:00:36 +0000 (+0000)
Subject: cmd/vet: type conversions never have side effects
X-Git-Tag: go1.11beta1~1423
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=c55505bae23940a0e253a9ea6f25577c7123c6c0;p=gostls13.git

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