go/analysis/passes/modernize: reflecttypefor: remove orphaned var

This CL fixes a bug in reflecttypefor that caused it to leave
a declaration for a local variable that was no longer used,
which is not legal.  Now, it deletes the variable:

- var zero T
- reflect.TypeOf(zero)
+ reflect.TypeFor[T]()

The bookkeeping is done by the new refactor.DeleteUnusedVars function.

Fixes golang/go#75767

Change-Id: If51af1500a63c968098b4a518b941315e826fcbf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/710656
Reviewed-by: Madeline Kalil <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: shuang cui <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>

Author: adonovan

go/analysis/passes/modernize/modernize_test.go

@@ -10,6 +10,7 @@ import (
 	. "golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/go/analysis/passes/modernize"
 	"golang.org/x/tools/internal/goplsexport"
+	"golang.org/x/tools/internal/testenv"
 )
 
 func TestAppendClipped(t *testing.T) {
@@ -61,6 +62,7 @@ func TestRangeInt(t *testing.T) {
 }
 
 func TestReflectTypeFor(t *testing.T) {
+	testenv.NeedsGo1Point(t, 25) // requires go1.25 types.Var.Kind
 	RunWithSuggestedFixes(t, TestData(), modernize.ReflectTypeForAnalyzer, "reflecttypefor")
 }
 

go/analysis/passes/modernize/reflect.go

@@ -18,6 +18,7 @@ import (
 	"golang.org/x/tools/internal/analysisinternal/generated"
 	typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex"
 	"golang.org/x/tools/internal/astutil"
+	"golang.org/x/tools/internal/refactor"
 	"golang.org/x/tools/internal/typesinternal"
 	"golang.org/x/tools/internal/typesinternal/typeindex"
 	"golang.org/x/tools/internal/versions"
@@ -66,16 +67,15 @@ func reflecttypefor(pass *analysis.Pass) (any, error) {
 				obj := typeutil.Callee(info, call2)
 				if typesinternal.IsMethodNamed(obj, "reflect", "Type", "Elem") {
 					if ptr, ok := t.(*types.Pointer); ok {
-						// Have: reflect.TypeOf(...*T value...).Elem()
-						// => reflect.TypeFor[T]()
+						// Have: TypeOf(expr).Elem() where expr : *T
 						t = ptr.Elem()
-						edits = []analysis.TextEdit{
-							{
-								// delete .Elem()
-								Pos: call.End(),
-								End: call2.End(),
-							},
-						}
+						// reflect.TypeOf(expr).Elem()
+						//                     -------
+						// reflect.TypeOf(expr)
+						edits = []analysis.TextEdit{{
+							Pos: call.End(),
+							End: call2.End(),
+						}}
 					}
 				}
 			}
@@ -92,6 +92,7 @@ func reflecttypefor(pass *analysis.Pass) (any, error) {
 		if versions.Before(info.FileVersions[file], "go1.22") {
 			continue // TypeFor requires go1.22
 		}
+		tokFile := pass.Fset.File(file.Pos())
 
 		// Format the type as valid Go syntax.
 		// TODO(adonovan): FileQualifier needs to respect
@@ -105,6 +106,14 @@ func reflecttypefor(pass *analysis.Pass) (any, error) {
 			continue // e.g. reflect was dot-imported
 		}
 
+		// If the call argument contains the last use
+		// of a variable, as in:
+		//	var zero T
+		//	reflect.TypeOf(zero)
+		// remove the declaration of that variable.
+		curArg0 := curCall.ChildAt(edge.CallExpr_Args, 0)
+		edits = append(edits, refactor.DeleteUnusedVars(index, info, tokFile, curArg0)...)
+
 		pass.Report(analysis.Diagnostic{
 			Pos:     call.Fun.Pos(),
 			End:     call.Fun.End(),

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

@@ -19,3 +19,13 @@ var (
 	_ = reflect.TypeOf(time.Time{})       // want "reflect.TypeOf call can be simplified using TypeFor"
 	_ = reflect.TypeOf(time.Duration(0))  // want "reflect.TypeOf call can be simplified using TypeFor"
 )
+
+// Eliminate local var if we deleted its last use.
+func _() {
+	var zero string
+	_ = reflect.TypeOf(zero) // want "reflect.TypeOf call can be simplified using TypeFor"
+
+	var z2 string
+	_ = reflect.TypeOf(z2) // want "reflect.TypeOf call can be simplified using TypeFor"
+	_ = z2                 // z2 has multiple uses
+}

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

@@ -20,3 +20,11 @@ var (
 	_ = reflect.TypeFor[time.Duration]() // want "reflect.TypeOf call can be simplified using TypeFor"
 )
 
+// Eliminate local var if we deleted its last use.
+func _() {
+	_ = reflect.TypeFor[string]() // want "reflect.TypeOf call can be simplified using TypeFor"
+
+	var z2 string
+	_ = reflect.TypeFor[string]() // want "reflect.TypeOf call can be simplified using TypeFor"
+	_ = z2                 // z2 has multiple uses
+}

internal/refactor/delete.go

@@ -18,6 +18,7 @@ import (
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/internal/astutil"
 	"golang.org/x/tools/internal/typesinternal"
+	"golang.org/x/tools/internal/typesinternal/typeindex"
 )
 
 // DeleteVar returns edits to delete the declaration of a variable
@@ -431,3 +432,34 @@ Outer:
 	}
 	return []analysis.TextEdit{edit}
 }
+
+// DeleteUnusedVars computes the edits required to delete the
+// declarations of any local variables whose last uses are in the
+// curDelend subtree, which is about to be deleted.
+func DeleteUnusedVars(index *typeindex.Index, info *types.Info, tokFile *token.File, curDelend inspector.Cursor) []analysis.TextEdit {
+	// TODO(adonovan): we might want to generalize this by
+	// splitting the two phases below, so that we can gather
+	// across a whole sequence of deletions then finally compute the
+	// set of variables that are no longer wanted.
+
+	// Count number of deletions of each var.
+	delcount := make(map[*types.Var]int)
+	for curId := range curDelend.Preorder((*ast.Ident)(nil)) {
+		id := curId.Node().(*ast.Ident)
+		if v, ok := info.Uses[id].(*types.Var); ok &&
+			typesinternal.GetVarKind(v) == typesinternal.LocalVar { // always false before go1.25
+			delcount[v]++
+		}
+	}
+
+	// Delete declaration of each var that became unused.
+	var edits []analysis.TextEdit
+	for v, count := range delcount {
+		if len(slices.Collect(index.Uses(v))) == count {
+			if curDefId, ok := index.Def(v); ok {
+				edits = append(edits, DeleteVar(tokFile, info, curDefId)...)
+			}
+		}
+	}
+	return edits
+}