From 320b0cdd77f1ea997acc9bf1fbb8dafbbdddca6c Mon Sep 17 00:00:00 2001 From: =?utf8?q?Daniel=20Mart=C3=AD?= Date: Sun, 8 Oct 2017 18:29:45 +0100 Subject: [PATCH] cmd/vet: skip self-assigns with side effects MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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í TryBot-Result: Gobot Gobot Reviewed-by: Rob Pike --- src/cmd/vet/assign.go | 3 +++ src/cmd/vet/testdata/assign.go | 15 ++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/cmd/vet/assign.go b/src/cmd/vet/assign.go index 54c1ae1fdc..bfa5b30329 100644 --- 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 32ba8683c1..6140ad4db8 100644 --- 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 } -- 2.48.1