go/analysis/passes/modernize: forvar: handle "if v := v; cond {"

Some analyzers' (especially stditerator's) fixes result
in redundant v := v assignments, often as the "init" part of
an if statement. This change causes the forvar analyzer to
clean them up when the if statement is the sole statement
of the loop body.

+ test

For golang/go#76241
For golang/go#76240

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

Author: adonovan

go/analysis/passes/modernize/forvar.go

@@ -38,48 +38,74 @@ var ForVarAnalyzer = &analysis.Analyzer{
 // because the Go specfilesUsingGoVersionsays "The variable used by each subsequent iteration
 // is declared implicitly before executing the post statement and initialized to the
 // value of the previous iteration's variable at that moment.")
+//
+// Variant: same thing in an IfStmt.Init, when the IfStmt is the sole
+// loop body statement:
+//
+//	for _, x := range foo {
+//		if x := x; cond { ... }
+//	}
+//
+// (The restriction is necessary to avoid potential problems arising
+// from merging two distinct variables.)
+//
+// This analyzer is synergistic with stditerators,
+// which may create redundant "x := x" statements.
 func forvar(pass *analysis.Pass) (any, error) {
 	for curFile := range filesUsingGoVersion(pass, versions.Go1_22) {
 		for curLoop := range curFile.Preorder((*ast.RangeStmt)(nil)) {
 			loop := curLoop.Node().(*ast.RangeStmt)
 			if loop.Tok != token.DEFINE {
 				continue
 			}
-			isLoopVarRedecl := func(assign *ast.AssignStmt) bool {
-				for i, lhs := range assign.Lhs {
-					if !(astutil.EqualSyntax(lhs, assign.Rhs[i]) &&
-						(astutil.EqualSyntax(lhs, loop.Key) || astutil.EqualSyntax(lhs, loop.Value))) {
-						return false
+			isLoopVarRedecl := func(stmt ast.Stmt) bool {
+				if assign, ok := stmt.(*ast.AssignStmt); ok &&
+					assign.Tok == token.DEFINE &&
+					len(assign.Lhs) == len(assign.Rhs) {
+
+					for i, lhs := range assign.Lhs {
+						if !(astutil.EqualSyntax(lhs, assign.Rhs[i]) &&
+							(astutil.EqualSyntax(lhs, loop.Key) ||
+								astutil.EqualSyntax(lhs, loop.Value))) {
+							return false
+						}
 					}
+					return true
 				}
-				return true
+				return false
 			}
 			// Have: for k, v := range x { stmts }
 			//
 			// Delete the prefix of stmts that are
 			// of the form k := k; v := v; k, v := k, v; v, k := v, k.
 			for _, stmt := range loop.Body.List {
-				if assign, ok := stmt.(*ast.AssignStmt); ok &&
-					assign.Tok == token.DEFINE &&
-					len(assign.Lhs) == len(assign.Rhs) &&
-					isLoopVarRedecl(assign) {
-
-					curStmt, _ := curLoop.FindNode(stmt)
-					edits := refactor.DeleteStmt(pass.Fset.File(stmt.Pos()), curStmt)
-					if len(edits) > 0 {
-						pass.Report(analysis.Diagnostic{
-							Pos:     stmt.Pos(),
-							End:     stmt.End(),
-							Message: "copying variable is unneeded",
-							SuggestedFixes: []analysis.SuggestedFix{{
-								Message:   "Remove unneeded redeclaration",
-								TextEdits: edits,
-							}},
-						})
-					}
+				if isLoopVarRedecl(stmt) {
+					// { x := x; ... }
+					//   ------
+				} else if ifstmt, ok := stmt.(*ast.IfStmt); ok &&
+					ifstmt.Init != nil &&
+					len(loop.Body.List) == 1 && // must be sole statement in loop body
+					isLoopVarRedecl(ifstmt.Init) {
+					// if x := x; cond {
+					//    ------
+					stmt = ifstmt.Init
 				} else {
 					break // stop at first other statement
 				}
+
+				curStmt, _ := curLoop.FindNode(stmt)
+				edits := refactor.DeleteStmt(pass.Fset.File(stmt.Pos()), curStmt)
+				if len(edits) > 0 {
+					pass.Report(analysis.Diagnostic{
+						Pos:     stmt.Pos(),
+						End:     stmt.End(),
+						Message: "copying variable is unneeded",
+						SuggestedFixes: []analysis.SuggestedFix{{
+							Message:   "Remove unneeded redeclaration",
+							TextEdits: edits,
+						}},
+					})
+				}
 			}
 		}
 	}

go/analysis/passes/modernize/testdata/src/forvar/forvar.go

@@ -36,6 +36,12 @@ func _(m map[int]int, s []int) {
 		/* hi */ i := i // want "copying variable is unneeded"
 		go f(i)
 	}
+	for v := range m {
+		if v := v; true { // want "copying variable is unneeded"
+			print(v)
+		}
+	}
+
 	// nope
 	var i, k, v int
 
@@ -84,6 +90,12 @@ func _(m map[int]int, s []int) {
 		i := i + 1
 		go f(i)
 	}
+	for v := range m {
+		if v := v; true { // nope, would merge distinct outer and inner variables v
+			print(v)
+		}
+		print(v)
+	}
 }
 
 func f(n int) {}

go/analysis/passes/modernize/testdata/src/forvar/forvar.go.golden

@@ -27,6 +27,12 @@ func _(m map[int]int, s []int) {
 	for i := range s {
 		go f(i)
 	}
+	for v := range m {
+		if true { // want "copying variable is unneeded"
+			print(v)
+		}
+	}
+
 	// nope
 	var i, k, v int
 
@@ -75,6 +81,12 @@ func _(m map[int]int, s []int) {
 		i := i + 1
 		go f(i)
 	}
+	for v := range m {
+		if v := v; true { // nope, would merge distinct outer and inner variables v
+			print(v)
+		}
+		print(v)
+	}
 }
 
 func f(n int) {}