gopls/internal/analysis/unusedfunc: use refactor.Delete* operators

This CL causes unusedfunc to make use of the new library of
deletion operators, making it simpler while also handling
a variety of edge cases such as n:n and n:1 assignments,
side effects on the RHS, and trailing comments.

Change-Id: I13be5092f294684b9434ae77fa5764a96d42bacf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/711360
Reviewed-by: Peter Weinberger <pjw@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>

Author: adonovan

gopls/internal/analysis/unusedfunc/testdata/basic.txtar

@@ -77,8 +77,27 @@ const (
 const (
 	constOne       = 1
 	unusedConstTwo = constOne // want `const "unusedConstTwo" is unused`
+	_, unusedConstThree = 0, 3 // want `const "unusedConstThree" is unused`
+	unusedConstFour, _ = 4, 0 // want `const "unusedConstFour" is unused`
+	unusedConstFive, UsedConst6 = 4, 6 // want `const "unusedConstFive" is unused`
 )
 
+// -- vars --
+
+var unusedVar = 1 // want `var "unusedVar" is unused`
+
+var (
+	unusedVar2 = 1 // want `var "unusedVar2" is unused`
+)
+
+var (
+	usedVar int
+	unusedVar3 = usedVar // want `var "unusedVar3" is unused`
+	_, unusedVar4, _ = Triple() // want `var "unusedVar4" is unused`
+)
+
+func Triple() (int, int, int)
+
 -- a/a.go.golden --
 package a
 
@@ -112,8 +131,6 @@ type _ interface{ dynamic() }
 
 type ExportedType2 int
 
-// want `type "unusedUnexportedType2" is unused`
-
 type (
 	one int
 )
@@ -122,16 +139,22 @@ type (
 
 type g[T any] int
 
-// want `method "method" is unused`
-
 // -- constants --
 
-// want `const "unusedConst" is unused`
-
 const (
 	unusedEnum = iota
 )
 
 const (
 	constOne = 1
+	UsedConst6 = 6 // want `const "unusedConstFive" is unused`
 )
+
+// -- vars --
+
+var (
+	usedVar int
+	_, _, _ = Triple() // want `var "unusedVar4" is unused`
+)
+
+func Triple() (int, int, int)

gopls/internal/analysis/unusedfunc/unusedfunc.go

@@ -20,6 +20,7 @@ import (
 	typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex"
 	"golang.org/x/tools/internal/astutil"
 	"golang.org/x/tools/internal/packagepath"
+	"golang.org/x/tools/internal/refactor"
 	"golang.org/x/tools/internal/typesinternal/typeindex"
 )
 
@@ -98,7 +99,7 @@ func run(pass *analysis.Pass) (any, error) {
 
 	// checkUnused reports a diagnostic if the object declared at id
 	// is unexported and unused. References within curSelf are ignored.
-	checkUnused := func(noun string, id *ast.Ident, node ast.Node, curSelf inspector.Cursor) {
+	checkUnused := func(noun string, id *ast.Ident, curSelf inspector.Cursor, delete func() []analysis.TextEdit) {
 		// Exported functions may be called from other packages.
 		if id.IsExported() {
 			return
@@ -118,28 +119,13 @@ func run(pass *analysis.Pass) (any, error) {
 			}
 		}
 
-		// Expand to include leading doc comment.
-		pos := node.Pos()
-		if doc := astutil.DocComment(node); doc != nil {
-			pos = doc.Pos()
-		}
-
-		// Expand to include trailing line  comment.
-		end := node.End()
-		if doc := eolComment(node); doc != nil {
-			end = doc.End()
-		}
-
 		pass.Report(analysis.Diagnostic{
 			Pos:     id.Pos(),
 			End:     id.End(),
 			Message: fmt.Sprintf("%s %q is unused", noun, id.Name),
 			SuggestedFixes: []analysis.SuggestedFix{{
-				Message: fmt.Sprintf("Delete %s %q", noun, id.Name),
-				TextEdits: []analysis.TextEdit{{
-					Pos: pos,
-					End: end,
-				}},
+				Message:   fmt.Sprintf("Delete %s %q", noun, id.Name),
+				TextEdits: delete(),
 			}},
 		})
 	}
@@ -159,6 +145,7 @@ func run(pass *analysis.Pass) (any, error) {
 		if ast.IsGenerated(file) {
 			continue // skip generated files
 		}
+		tokFile := pass.Fset.File(file.Pos())
 
 	nextDecl:
 		for i := range file.Decls {
@@ -197,22 +184,22 @@ func run(pass *analysis.Pass) (any, error) {
 				}
 
 				noun := cond(decl.Recv == nil, "function", "method")
-				checkUnused(noun, decl.Name, decl, curDecl)
+				checkUnused(noun, decl.Name, curDecl, func() []analysis.TextEdit {
+					return refactor.DeleteDecl(tokFile, curDecl)
+				})
 
 			case *ast.GenDecl:
-				// Instead of deleting a spec in a singleton decl,
-				// delete the whole decl.
-				singleton := len(decl.Specs) == 1
-
 				switch decl.Tok {
 				case token.TYPE:
 					for i, spec := range decl.Specs {
 						var (
 							spec    = spec.(*ast.TypeSpec)
 							id      = spec.Name
-							curSelf = curDecl.ChildAt(edge.GenDecl_Specs, i)
+							curSpec = curDecl.ChildAt(edge.GenDecl_Specs, i)
 						)
-						checkUnused("type", id, cond[ast.Node](singleton, decl, spec), curSelf)
+						checkUnused("type", id, curSpec, func() []analysis.TextEdit {
+							return refactor.DeleteSpec(tokFile, curSpec)
+						})
 					}
 
 				case token.CONST, token.VAR:
@@ -224,13 +211,12 @@ func run(pass *analysis.Pass) (any, error) {
 						spec := spec.(*ast.ValueSpec)
 						curSpec := curDecl.ChildAt(edge.GenDecl_Specs, i)
 
-						// Ignore n:n and n:1 assignments for now.
-						// TODO(adonovan): support these cases.
-						if len(spec.Names) != 1 {
-							continue
+						for j, id := range spec.Names {
+							checkUnused(decl.Tok.String(), id, curSpec, func() []analysis.TextEdit {
+								curId := curSpec.ChildAt(edge.ValueSpec_Names, j)
+								return refactor.DeleteVar(tokFile, pass.TypesInfo, curId)
+							})
 						}
-						id := spec.Names[0]
-						checkUnused(decl.Tok.String(), id, cond[ast.Node](singleton, decl, spec), curSpec)
 					}
 				}
 			}
@@ -240,22 +226,6 @@ func run(pass *analysis.Pass) (any, error) {
 	return nil, nil
 }
 
-func eolComment(n ast.Node) *ast.CommentGroup {
-	// TODO(adonovan): support:
-	//    func f() {...} // comment
-	switch n := n.(type) {
-	case *ast.GenDecl:
-		if !n.TokPos.IsValid() && len(n.Specs) == 1 {
-			return eolComment(n.Specs[0])
-		}
-	case *ast.ValueSpec:
-		return n.Comment
-	case *ast.TypeSpec:
-		return n.Comment
-	}
-	return nil
-}
-
 func cond[T any](cond bool, t, f T) T {
 	if cond {
 		return t

gopls/internal/test/integration/diagnostics/analysis_test.go

@@ -108,7 +108,7 @@ package main
 import _ "embed"
 
 //go:embed foo.txt
-var foo, bar string
+var foo, Bar string
 
 func main() {
 	_ = foo

gopls/internal/test/marker/testdata/mcptools/file_diagnostics.txt

@@ -29,7 +29,7 @@ Fix:
  package main
 
 -func foo() {} //@loc(foo, "foo")
-+ //@loc(foo, "foo")
++
 
  //@mcptool("go_file_diagnostics", `{"file":"$WORKDIR/a/main.go"}`, output=unused)
  //@diag(foo, re"unused")

internal/refactor/delete.go

@@ -21,8 +21,8 @@ import (
 	"golang.org/x/tools/internal/typesinternal/typeindex"
 )
 
-// DeleteVar returns edits to delete the declaration of a variable
-// whose defining identifier is curId.
+// DeleteVar returns edits to delete the declaration of a variable or
+// constant whose defining identifier is curId.
 //
 // It handles variants including:
 // - GenDecl > ValueSpec versus AssignStmt;
@@ -46,6 +46,9 @@ func DeleteVar(tokFile *token.File, info *types.Info, curId inspector.Cursor) []
 	return nil
 }
 
+// deleteVarFromValueSpec returns edits to delete the declaration of a
+// variable or constant within a ValueSpec.
+//
 // Precondition: curId is Ident beneath ValueSpec.Names beneath GenDecl.
 //
 // See also [deleteVarFromAssignStmt], which has parallel structure.
@@ -98,7 +101,7 @@ func deleteVarFromValueSpec(tokFile *token.File, info *types.Info, curIdent insp
 		}}
 	}
 
-	// If the assignment is 1:1 and the RHS has no effects,
+	// If the assignment is n:n and the RHS has no effects,
 	// we can delete the LHS and its corresponding RHS.
 	if len(spec.Names) == len(spec.Values) &&
 		typesinternal.NoEffects(info, spec.Values[index]) {
@@ -240,12 +243,12 @@ func deleteVarFromAssignStmt(tokFile *token.File, info *types.Info, curIdent ins
 	return edits
 }
 
-// DeleteSpec returns edits to delete the ValueSpec identified by curSpec.
+// DeleteSpec returns edits to delete the {Type,Value}Spec identified by curSpec.
 //
 // TODO(adonovan): add test suite. Test for consts as well.
 func DeleteSpec(tokFile *token.File, curSpec inspector.Cursor) []analysis.TextEdit {
 	var (
-		spec    = curSpec.Node().(*ast.ValueSpec)
+		spec    = curSpec.Node().(ast.Spec)
 		curDecl = curSpec.Parent()
 		decl    = curDecl.Node().(*ast.GenDecl)
 	)
@@ -259,14 +262,14 @@ func DeleteSpec(tokFile *token.File, curSpec inspector.Cursor) []analysis.TextEd
 	// Delete the spec and its comments.
 	_, index := curSpec.ParentEdge() // index of ValueSpec within GenDecl.Specs
 	pos, end := spec.Pos(), spec.End()
-	if spec.Doc != nil {
-		pos = spec.Doc.Pos() // leading comment
+	if doc := astutil.DocComment(spec); doc != nil {
+		pos = doc.Pos() // leading comment
 	}
 	if index == len(decl.Specs)-1 {
 		// Delete final spec.
-		if spec.Comment != nil {
+		if c := eolComment(spec); c != nil {
 			//  var (v int // comment \n)
-			end = spec.Comment.End()
+			end = c.End()
 		}
 	} else {
 		// Delete non-final spec.
@@ -463,3 +466,19 @@ func DeleteUnusedVars(index *typeindex.Index, info *types.Info, tokFile *token.F
 	}
 	return edits
 }
+
+func eolComment(n ast.Node) *ast.CommentGroup {
+	// TODO(adonovan): support:
+	//    func f() {...} // comment
+	switch n := n.(type) {
+	case *ast.GenDecl:
+		if !n.TokPos.IsValid() && len(n.Specs) == 1 {
+			return eolComment(n.Specs[0])
+		}
+	case *ast.ValueSpec:
+		return n.Comment
+	case *ast.TypeSpec:
+		return n.Comment
+	}
+	return nil
+}