go/analysis/passes/assign: correctly handle parallel assignments

Fixes golang/go#74013

Change-Id: I40a4832ccb180f0f72ae9a63e6638d81a1446c7e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/679435
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: kwjw

go/analysis/passes/assign/assign.go

@@ -9,11 +9,11 @@ package assign
 
 import (
 	_ "embed"
-	"fmt"
 	"go/ast"
 	"go/token"
 	"go/types"
 	"reflect"
+	"strings"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
@@ -48,31 +48,84 @@ func run(pass *analysis.Pass) (any, error) {
 			// If LHS and RHS have different cardinality, they can't be the same.
 			return
 		}
+
+		// Delete redundant LHS, RHS pairs, taking care
+		// to include intervening commas.
+		var (
+			exprs                    []string // expressions appearing on both sides (x = x)
+			edits                    []analysis.TextEdit
+			runStartLHS, runStartRHS token.Pos // non-zero => within a run
+		)
 		for i, lhs := range stmt.Lhs {
 			rhs := stmt.Rhs[i]
-			if analysisutil.HasSideEffects(pass.TypesInfo, lhs) ||
-				analysisutil.HasSideEffects(pass.TypesInfo, rhs) ||
-				isMapIndex(pass.TypesInfo, lhs) {
-				continue // expressions may not be equal
-			}
-			if reflect.TypeOf(lhs) != reflect.TypeOf(rhs) {
-				continue // short-circuit the heavy-weight gofmt check
+			isSelfAssign := false
+			var le string
+
+			if !analysisutil.HasSideEffects(pass.TypesInfo, lhs) &&
+				!analysisutil.HasSideEffects(pass.TypesInfo, rhs) &&
+				!isMapIndex(pass.TypesInfo, lhs) &&
+				reflect.TypeOf(lhs) == reflect.TypeOf(rhs) { // short-circuit the heavy-weight gofmt check
+
+				le = analysisinternal.Format(pass.Fset, lhs)
+				re := analysisinternal.Format(pass.Fset, rhs)
+				if le == re {
+					isSelfAssign = true
+				}
 			}
-			le := analysisinternal.Format(pass.Fset, lhs)
-			re := analysisinternal.Format(pass.Fset, rhs)
-			if le == re {
-				pass.Report(analysis.Diagnostic{
-					Pos: stmt.Pos(), Message: fmt.Sprintf("self-assignment of %s to %s", re, le),
-					SuggestedFixes: []analysis.SuggestedFix{{
-						Message: "Remove self-assignment",
-						TextEdits: []analysis.TextEdit{{
-							Pos: stmt.Pos(),
-							End: stmt.End(),
-						}}},
-					},
-				})
+
+			if isSelfAssign {
+				exprs = append(exprs, le)
+				if !runStartLHS.IsValid() {
+					// Start of a new run of self-assignments.
+					if i > 0 {
+						runStartLHS = stmt.Lhs[i-1].End()
+						runStartRHS = stmt.Rhs[i-1].End()
+					} else {
+						runStartLHS = lhs.Pos()
+						runStartRHS = rhs.Pos()
+					}
+				}
+			} else if runStartLHS.IsValid() {
+				// End of a run of self-assignments.
+				endLHS, endRHS := stmt.Lhs[i-1].End(), stmt.Rhs[i-1].End()
+				if runStartLHS == stmt.Lhs[0].Pos() {
+					endLHS, endRHS = lhs.Pos(), rhs.Pos()
+				}
+				edits = append(edits,
+					analysis.TextEdit{Pos: runStartLHS, End: endLHS},
+					analysis.TextEdit{Pos: runStartRHS, End: endRHS},
+				)
+				runStartLHS, runStartRHS = 0, 0
 			}
 		}
+
+		// If a run of self-assignments continues to the end of the statement, close it.
+		if runStartLHS.IsValid() {
+			last := len(stmt.Lhs) - 1
+			edits = append(edits,
+				analysis.TextEdit{Pos: runStartLHS, End: stmt.Lhs[last].End()},
+				analysis.TextEdit{Pos: runStartRHS, End: stmt.Rhs[last].End()},
+			)
+		}
+
+		if len(exprs) == 0 {
+			return
+		}
+
+		if len(exprs) == len(stmt.Lhs) {
+			// If every part of the statement is a self-assignment,
+			// remove the whole statement.
+			edits = []analysis.TextEdit{{Pos: stmt.Pos(), End: stmt.End()}}
+		}
+
+		pass.Report(analysis.Diagnostic{
+			Pos:     stmt.Pos(),
+			Message: "self-assignment of " + strings.Join(exprs, ", "),
+			SuggestedFixes: []analysis.SuggestedFix{{
+				Message:   "Remove self-assignment",
+				TextEdits: edits,
+			}},
+		})
 	})
 
 	return nil, nil

go/analysis/passes/assign/testdata/src/a/a.go

@@ -15,11 +15,23 @@ type ST struct {
 
 func (s *ST) SetX(x int, ch chan int) {
 	// Accidental self-assignment; it should be "s.x = x"
-	x = x // want "self-assignment of x to x"
+	x = x // want "self-assignment of x"
 	// Another mistake
-	s.x = s.x // want "self-assignment of s.x to s.x"
+	s.x = s.x // want "self-assignment of s.x"
 
-	s.l[0] = s.l[0] // want "self-assignment of s.l.0. to s.l.0."
+	s.l[0] = s.l[0] // want "self-assignment of s.l.0."
+
+	// Report self-assignment to x but preserve the actual assignment to s.x
+	x, s.x = x, 1 // want "self-assignment of x"
+	s.x, x = 1, x // want "self-assignment of x"
+
+	// Delete multiple self-assignment
+	x, s.x = x, s.x                       // want "self-assignment of x, s.x"
+	s.l[0], x, s.x = 1, x, s.x            // want "self-assignment of x, s.x"
+	x, s.l[0], s.x = x, 1, s.x            // want "self-assignment of x, s.x"
+	x, s.x, s.l[0] = x, s.x, 1            // want "self-assignment of x, s.x"
+	s.l[0], x, s.x, s.l[1] = 1, x, s.x, 1 // want "self-assignment of x, s.x"
+	x, s.l[0], s.l[1], s.x = x, 1, 1, s.x // want "self-assignment of x, s.x"
 
 	// Bail on any potential side effects to avoid false positives
 	s.l[num()] = s.l[num()]

go/analysis/passes/assign/testdata/src/a/a.go.golden

@@ -15,11 +15,23 @@ type ST struct {
 
 func (s *ST) SetX(x int, ch chan int) {
 	// Accidental self-assignment; it should be "s.x = x"
-	// want "self-assignment of x to x"
+	// want "self-assignment of x"
 	// Another mistake
-	// want "self-assignment of s.x to s.x"
+	// want "self-assignment of s.x"
 
-	// want "self-assignment of s.l.0. to s.l.0."
+	// want "self-assignment of s.l.0."
+
+	// Report self-assignment to x but preserve the actual assignment to s.x
+	s.x = 1 // want "self-assignment of x"
+	s.x = 1 // want "self-assignment of x"
+
+	// Delete multiple self-assignment
+	// want "self-assignment of x, s.x"
+	s.l[0] = 1            // want "self-assignment of x, s.x"
+	s.l[0] = 1            // want "self-assignment of x, s.x"
+	s.l[0] = 1            // want "self-assignment of x, s.x"
+	s.l[0], s.l[1] = 1, 1 // want "self-assignment of x, s.x"
+	s.l[0], s.l[1] = 1, 1 // want "self-assignment of x, s.x"
 
 	// Bail on any potential side effects to avoid false positives
 	s.l[num()] = s.l[num()]

go/analysis/passes/assign/testdata/src/typeparams/typeparams.go

@@ -15,11 +15,11 @@ type ST[T interface{ ~int }] struct {
 
 func (s *ST[T]) SetX(x T, ch chan T) {
 	// Accidental self-assignment; it should be "s.x = x"
-	x = x // want "self-assignment of x to x"
+	x = x // want "self-assignment of x"
 	// Another mistake
-	s.x = s.x // want "self-assignment of s.x to s.x"
+	s.x = s.x // want "self-assignment of s.x"
 
-	s.l[0] = s.l[0] // want "self-assignment of s.l.0. to s.l.0."
+	s.l[0] = s.l[0] // want "self-assignment of s.l.0."
 
 	// Bail on any potential side effects to avoid false positives
 	s.l[num()] = s.l[num()]

go/analysis/passes/assign/testdata/src/typeparams/typeparams.go.golden

@@ -15,11 +15,11 @@ type ST[T interface{ ~int }] struct {
 
 func (s *ST[T]) SetX(x T, ch chan T) {
 	// Accidental self-assignment; it should be "s.x = x"
-	// want "self-assignment of x to x"
+	// want "self-assignment of x"
 	// Another mistake
-	// want "self-assignment of s.x to s.x"
+	// want "self-assignment of s.x"
 
-	// want "self-assignment of s.l.0. to s.l.0."
+	// want "self-assignment of s.l.0."
 
 	// Bail on any potential side effects to avoid false positives
 	s.l[num()] = s.l[num()]

go/analysis/unitchecker/unitchecker_test.go

@@ -97,7 +97,7 @@ func _() {
 ([/._\-a-zA-Z0-9]+[\\/]fake[\\/])?b/b.go:7:11: call of MyFunc123\(...\)
 `
 	const wantC = `# golang.org/fake/c
-([/._\-a-zA-Z0-9]+[\\/]fake[\\/])?c/c.go:5:5: self-assignment of i to i
+([/._\-a-zA-Z0-9]+[\\/]fake[\\/])?c/c.go:5:5: self-assignment of i
 `
 	const wantAJSON = `# golang.org/fake/a
 \{
@@ -130,7 +130,7 @@ func _() {
 		"assign": \[
 			\{
 				"posn": "([/._\-a-zA-Z0-9]+[\\/]fake[\\/])?c/c.go:5:5",
-				"message": "self-assignment of i to i",
+				"message": "self-assignment of i",
 				"suggested_fixes": \[
 					\{
 						"message": "Remove self-assignment",