cmd/vet: assume that no builtin funcs are pure

That was the intention with the existing code, but it was buggy; builtin
functions aren't treated as values by types.TypeAndVal. Thus, we should
use the IsBuiltin method instead of IsValue.

Teaching vet what builtin funcs are pure is already being tracked as a
separate issue, #22851.

While at it, also add a test with methods, just to be sure that the
current logic doesn't break with that edge case either.

Fixes #25303.

Change-Id: Ic18402b22cceeabf76641c02f575b194b9a536cc
Reviewed-on: https://go-review.googlesource.com/112177
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

diff --git a/src/cmd/vet/bool.go b/src/cmd/vet/bool.go
index 67321c3df4cfba04562340497b2bbf19dc4cf207..e8b73175aaee135738e4479923c996400926d689 100644 (file)
--- a/src/cmd/vet/bool.go
+++ b/src/cmd/vet/bool.go
@@ -147,12 +147,18 @@ func hasSideEffects(f *File, e ast.Expr) bool {
                        // conversion as a *types.Named.
                        typVal := f.pkg.types[n.Fun]
                        _, isSig := typVal.Type.(*types.Signature)
-                       if typVal.IsValue() && isSig {
+                       switch {
+                       case typVal.IsValue() && isSig:
                                // If we have a value of unnamed signature type,
-                               // this CallExpr is a func call and not a type
-                               // conversion. Conservatively assume that all
-                               // function and method calls have side effects
-                               // for now.
+                               // this CallExpr is a non-builtin func call and
+                               // not a type conversion. Conservatively assume
+                               // that all function and method calls have side
+                               // effects for now.
+                               safe = false
+                               return false
+                       case typVal.IsBuiltin():
+                               // For now, conservatively assume that all
+                               // built-in functions have side effects.
                                safe = false
                                return false
                        }

diff --git a/src/cmd/vet/testdata/bool.go b/src/cmd/vet/testdata/bool.go
index be78caac18e51ae2f838fff924bf9ff36ebe5910..80c44d25ca38890e06e1c0933d6fd67007b48868 100644 (file)
--- a/src/cmd/vet/testdata/bool.go
+++ b/src/cmd/vet/testdata/bool.go
@@ -10,12 +10,18 @@ import "io"
 
 type T int
 
+func (t T) Foo() int { return int(t) }
+
 type FT func() int
 
+var S []int
+
 func RatherStupidConditions() {
        var f, g func() int
        if f() == 0 || f() == 0 { // OK f might have side effects
        }
+       var t T
+       _ = t.Foo() == 2 || t.Foo() == 2        // OK Foo 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"
@@ -25,9 +31,10 @@ func RatherStupidConditions() {
        _ = FT(f) == nil || FT(f) == nil // ERROR "redundant or: FT(f) == nil || FT(f) == nil"
 
        _ = (func() int)(f) == nil || (func() int)(f) == nil // ERROR "redundant or: (func() int)(f) == nil || (func() int)(f) == nil"
+       _ = append(S, 3) == nil || append(S, 3) == nil       // OK append has side effects
 
        var namedFuncVar FT
-       _ = namedFuncVar() == namedFuncVar() // OK; still func calls
+       _ = namedFuncVar() == namedFuncVar() // OK still func calls
 
        var c chan int
        _ = 0 == <-c || 0 == <-c                                  // OK subsequent receives may yield different values