go/analysis/passes/modernize: stditerators: even better name heuristic

This CL causes stditerators to generate a fresh name only when
the preferred name is already declared _and_ is free in
(referenced within) the loop body. This eliminates most cases
where a fresh (ugly) name is needed.

(Most places that use FreshName should do something similar,
though some care will be required to figure out the correct
abstraction.)

Fixes golang/go#76240
Updates golang/go#76241

Change-Id: Ia7a3332ef49e3d4f2934e9f85215b9373226084a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/719321
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Author: adonovan

go/analysis/passes/modernize/errorsastype.go

@@ -125,6 +125,8 @@ func errorsastype(pass *analysis.Pass) (any, error) {
 		errtype := types.TypeString(v.Type(), qual)
 
 		// Choose a name for the "ok" variable.
+		// TODO(adonovan): this pattern also appears in stditerators,
+		// and is wanted elsewhere; factor.
 		okName := "ok"
 		if okVar := lookup(info, curCall, "ok"); okVar != nil {
 			// The name 'ok' is already declared, but

go/analysis/passes/modernize/stditerators.go

@@ -163,7 +163,23 @@ func stditerators(pass *analysis.Pass) (any, error) {
 			}
 
 			loop := curBody.Parent().Node()
-			return refactor.FreshName(info.Scopes[loop], loop.Pos(), row.elemname), nil
+
+			// Choose a fresh name only if
+			// (a) the preferred name is already declared here, and
+			// (b) there are references to it from the loop body.
+			// TODO(adonovan): this pattern also appears in errorsastype,
+			// and is wanted elsewhere; factor.
+			name := row.elemname
+			if v := lookup(info, curBody, name); v != nil {
+				// is it free in body?
+				for curUse := range index.Uses(v) {
+					if curBody.Contains(curUse) {
+						name = refactor.FreshName(info.Scopes[loop], loop.Pos(), name)
+						break
+					}
+				}
+			}
+			return name, nil
 		}
 
 		// Process each call of x.Len().

go/analysis/passes/modernize/stringscut.go

@@ -191,6 +191,7 @@ func stringscut(pass *analysis.Pass) (any, error) {
 
 			scope := iObj.Parent()
 			var (
+				// TODO(adonovan): avoid FreshName when not needed; see errorsastype.
 				okVarName     = refactor.FreshName(scope, iIdent.Pos(), "ok")
 				beforeVarName = refactor.FreshName(scope, iIdent.Pos(), "before")
 				afterVarName  = refactor.FreshName(scope, iIdent.Pos(), "after")

go/analysis/passes/modernize/stringscutprefix.go

@@ -201,6 +201,7 @@ func stringscutprefix(pass *analysis.Pass) (any, error) {
 
 					if astutil.EqualSyntax(lhs, bin.X) && astutil.EqualSyntax(call.Args[0], bin.Y) ||
 						(astutil.EqualSyntax(lhs, bin.Y) && astutil.EqualSyntax(call.Args[0], bin.X)) {
+						// TODO(adonovan): avoid FreshName when not needed; see errorsastype.
 						okVarName := refactor.FreshName(info.Scopes[ifStmt], ifStmt.Pos(), "ok")
 						// Have one of:
 						//   if rest := TrimPrefix(s, prefix); rest != s { (ditto Suffix)

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

@@ -13,10 +13,14 @@ func _(scope *types.Scope) {
 		print(scope.Child(i))
 	}
 	{
-		const child = 0                            // shadowing of preferred name at def
+		// tests of shadowing of preferred name at def
+		const child = 0
 		for i := 0; i < scope.NumChildren(); i++ { // want "NumChildren/Child loop can simplified using Scope.Children iteration"
 			print(scope.Child(i))
 		}
+		for i := 0; i < scope.NumChildren(); i++ { // want "NumChildren/Child loop can simplified using Scope.Children iteration"
+			print(scope.Child(i), child)
+		}
 	}
 	{
 		for i := 0; i < scope.NumChildren(); i++ {
@@ -65,4 +69,22 @@ func _(tuple *types.Tuple) {
 		bar := tuple.At(i)
 		print(bar)
 	}
+	{
+		// The name v is already declared, but not
+		// used in the loop, so we can use it again.
+		v := 1
+		print(v)
+
+		for i := 0; i < tuple.Len(); i++ { // want "Len/At loop can simplified using Tuple.Variables iteration"
+			print(tuple.At(i))
+		}
+	}
+	{
+		// The name v is used from the loop, so
+		// we must choose a fresh name.
+		v := 1
+		for i := 0; i < tuple.Len(); i++ { // want "Len/At loop can simplified using Tuple.Variables iteration"
+			print(tuple.At(i), v)
+		}
+	}
 }

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

@@ -13,9 +13,13 @@ func _(scope *types.Scope) {
 		print(child)
 	}
 	{
-		const child = 0                        // shadowing of preferred name at def
+		// tests of shadowing of preferred name at def
+		const child = 0
+		for child := range scope.Children() { // want "NumChildren/Child loop can simplified using Scope.Children iteration"
+			print(child)
+		}
 		for child0 := range scope.Children() { // want "NumChildren/Child loop can simplified using Scope.Children iteration"
-			print(child0)
+			print(child0, child)
 		}
 	}
 	{
@@ -65,4 +69,22 @@ func _(tuple *types.Tuple) {
 		bar := foo
 		print(bar)
 	}
+	{
+		// The name v is already declared, but not
+		// used in the loop, so we can use it again.
+		v := 1
+		print(v)
+
+		for v := range tuple.Variables() { // want "Len/At loop can simplified using Tuple.Variables iteration"
+			print(v)
+		}
+	}
+	{
+		// The name v is used from the loop, so
+		// we must choose a fresh name.
+		v := 1
+		for v0 := range tuple.Variables() { // want "Len/At loop can simplified using Tuple.Variables iteration"
+			print(v0, v)
+		}
+	}
 }

internal/refactor/refactor.go

@@ -17,6 +17,11 @@ import (
 
 // FreshName returns the name of an identifier that is undefined
 // at the specified position, based on the preferred name.
+//
+// TODO(adonovan): refine this to choose a fresh name only when there
+// would be a conflict with the existing declaration: it's fine to
+// redeclare a name in a narrower scope so long as there are no free
+// references to the outer name from within the narrower scope.
 func FreshName(scope *types.Scope, pos token.Pos, preferred string) string {
 	newName := preferred
 	for i := 0; ; i++ {