cmd/vet: skip self-assigns with side effects

The existing logic for whether the left and right parts of an assignment
were equal only checked that the gofmt representation of the two was
equal. This only checks that the ASTs were equal.

However, that method is flawed. For example, if either of the
expressions contains a function call, the expressions may actually be
different even if their ASTs are the same. An obvious case is a func
call to math/rand to get a random integer, such as the one added in the
test.

If either of the expressions may have side effects, simply skip the
check. Reuse the logic from bool.go's hasSideEffects.

Fixes #22174.

Change-Id: Ied7f7543dc2bb8852e817230756c6d23bc801d90
Reviewed-on: https://go-review.googlesource.com/69116
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>

diff --git a/src/cmd/vet/assign.go b/src/cmd/vet/assign.go
index 54c1ae1fdc3f9142699a7a36bceb7bbed135b904..bfa5b3032938e22eb37dffa586c1b4699bf80152 100644 (file)
--- a/src/cmd/vet/assign.go
+++ b/src/cmd/vet/assign.go
@@ -37,6 +37,9 @@ func checkAssignStmt(f *File, node ast.Node) {
        }
        for i, lhs := range stmt.Lhs {
                rhs := stmt.Rhs[i]
+               if hasSideEffects(lhs) || hasSideEffects(rhs) {
+                       continue // expressions may not be equal
+               }
                if reflect.TypeOf(lhs) != reflect.TypeOf(rhs) {
                        continue // short-circuit the heavy-weight gofmt check
                }

diff --git a/src/cmd/vet/testdata/assign.go b/src/cmd/vet/testdata/assign.go
index 32ba8683c148522a7f2cfa618e6a661c470e3a0f..6140ad4db8cc0cd7e257556229dc74f69e27baee 100644 (file)
--- a/src/cmd/vet/testdata/assign.go
+++ b/src/cmd/vet/testdata/assign.go
@@ -6,13 +6,26 @@
 
 package testdata
 
+import "math/rand"
+
 type ST struct {
        x int
+       l []int
 }
 
-func (s *ST) SetX(x int) {
+func (s *ST) SetX(x int, ch chan int) {
        // Accidental self-assignment; it should be "s.x = x"
        x = x // ERROR "self-assignment of x to x"
        // Another mistake
        s.x = s.x // ERROR "self-assignment of s.x to s.x"
+
+       s.l[0] = s.l[0] // ERROR "self-assignment of s.l.0. to s.l.0."
+
+       // Bail on any potential side effects to avoid false positives
+       s.l[num()] = s.l[num()]
+       rng := rand.New(rand.NewSource(0))
+       s.l[rng.Intn(len(s.l))] = s.l[rng.Intn(len(s.l))]
+       s.l[<-ch] = s.l[<-ch]
 }
+
+func num() int { return 2 }