gopls/internal/analysis: unusedvariable

The unused variable quickfix removes more than it should when there are
comments following the unused variable statement. This CL accounts for
comments when determing the end position of the text edit in the
quickfix.

Fixes golang/go#54373

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

Author: madelinekalil

gopls/doc/analyzers.md

@@ -1011,7 +1011,7 @@ Package documentation: [unusedresult](https://pkg.go.dev/golang.org/x/tools/go/a
 
 
 
-Default: off. Enable by setting `"analyses": {"unusedvariable": true}`.
+Default: on.
 
 Package documentation: [unusedvariable](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedvariable)
 

gopls/doc/release/v0.18.0.md

@@ -49,6 +49,10 @@ this fashion (or with `%s` for the port) is passed to `net.Dial` or a
 related function, and offers a fix to use `net.JoinHostPort`
 instead.
 
+## `unusedvariable` analyzer now on by default
+
+This analyzer suggests deleting the unused variable declaration.
+
 ## "Implementations" supports generics
 
 At long last, the "Go to Implementations" feature now fully supports

gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go

@@ -65,6 +65,17 @@ func commentAbove() {
 	v := "s" // want `declared (and|but) not used`
 }
 
+func commentBelow() {
+	v := "s" // want `declared (and|but) not used`
+	// v is a variable
+}
+
+func commentSpaceBelow() {
+	v := "s" // want `declared (and|but) not used`
+
+	// v is a variable
+}
+
 func fBool() bool {
 	return true
 }

gopls/internal/analysis/unusedvariable/testdata/src/assign/a.go.golden

@@ -50,6 +50,14 @@ func commentAbove() {
 	// v is a variable
 }
 
+func commentBelow() {
+	// v is a variable
+}
+
+func commentSpaceBelow() {
+	// v is a variable
+}
+
 func fBool() bool {
 	return true
 }

gopls/internal/analysis/unusedvariable/unusedvariable.go

@@ -17,6 +17,7 @@ import (
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/ast/astutil"
+	"golang.org/x/tools/gopls/internal/util/safetoken"
 )
 
 const Doc = `check for unused variables and suggest fixes`
@@ -37,7 +38,7 @@ var unusedVariableRegexp = []*regexp.Regexp{
 	regexp.MustCompile("^declared and not used: (.*)$"), // Go 1.23+
 }
 
-func run(pass *analysis.Pass) (interface{}, error) {
+func run(pass *analysis.Pass) (any, error) {
 	for _, typeErr := range pass.TypeErrors {
 		for _, re := range unusedVariableRegexp {
 			match := re.FindStringSubmatch(typeErr.Msg)
@@ -113,7 +114,7 @@ func runForError(pass *analysis.Pass, err types.Error, name string) error {
 				continue
 			}
 
-			fixes := removeVariableFromAssignment(path, stmt, ident)
+			fixes := removeVariableFromAssignment(pass.Fset, path, stmt, ident)
 			// fixes may be nil
 			if len(fixes) > 0 {
 				diag.SuggestedFixes = fixes
@@ -164,7 +165,7 @@ func removeVariableFromSpec(pass *analysis.Pass, path []ast.Node, stmt *ast.Valu
 		// Find parent DeclStmt and delete it
 		for _, node := range path {
 			if declStmt, ok := node.(*ast.DeclStmt); ok {
-				edits := deleteStmtFromBlock(path, declStmt)
+				edits := deleteStmtFromBlock(pass.Fset, path, declStmt)
 				if len(edits) == 0 {
 					return nil // can this happen?
 				}
@@ -198,7 +199,7 @@ func removeVariableFromSpec(pass *analysis.Pass, path []ast.Node, stmt *ast.Valu
 	}
 }
 
-func removeVariableFromAssignment(path []ast.Node, stmt *ast.AssignStmt, ident *ast.Ident) []analysis.SuggestedFix {
+func removeVariableFromAssignment(fset *token.FileSet, path []ast.Node, stmt *ast.AssignStmt, ident *ast.Ident) []analysis.SuggestedFix {
 	// The only variable in the assignment is unused
 	if len(stmt.Lhs) == 1 {
 		// If LHS has only one expression to be valid it has to have 1 expression
@@ -221,7 +222,7 @@ func removeVariableFromAssignment(path []ast.Node, stmt *ast.AssignStmt, ident *
 		}
 
 		// RHS does not have any side effects, delete the whole statement
-		edits := deleteStmtFromBlock(path, stmt)
+		edits := deleteStmtFromBlock(fset, path, stmt)
 		if len(edits) == 0 {
 			return nil // can this happen?
 		}
@@ -252,7 +253,7 @@ func suggestedFixMessage(name string) string {
 	return fmt.Sprintf("Remove variable %s", name)
 }
 
-func deleteStmtFromBlock(path []ast.Node, stmt ast.Stmt) []analysis.TextEdit {
+func deleteStmtFromBlock(fset *token.FileSet, path []ast.Node, stmt ast.Stmt) []analysis.TextEdit {
 	// Find innermost enclosing BlockStmt.
 	var block *ast.BlockStmt
 	for i := range path {
@@ -282,6 +283,31 @@ func deleteStmtFromBlock(path []ast.Node, stmt ast.Stmt) []analysis.TextEdit {
 		end = block.List[nodeIndex+1].Pos()
 	}
 
+	// Account for comments within the block containing the statement
+	// TODO(adonovan): when golang/go#20744 is addressed, query the AST
+	// directly for comments between stmt.End() and end. For now we
+	// must scan the entire file's comments (though we could binary search).
+	astFile := path[len(path)-1].(*ast.File)
+	currFile := fset.File(end)
+	stmtEndLine := safetoken.Line(currFile, stmt.End())
+outer:
+	for _, cg := range astFile.Comments {
+		for _, co := range cg.List {
+			if stmt.End() <= co.Pos() && co.Pos() <= end {
+				coLine := safetoken.Line(currFile, co.Pos())
+				// If a comment exists within the current block, after the unused variable statement,
+				// and before the next statement, we shouldn't delete it.
+				if coLine > stmtEndLine {
+					end = co.Pos()
+					break outer
+				}
+				if co.Pos() > end {
+					break outer
+				}
+			}
+		}
+	}
+
 	return []analysis.TextEdit{
 		{
 			Pos: stmt.Pos(),

gopls/internal/doc/api.json

@@ -608,7 +608,7 @@
 						{
 							"Name": "\"unusedvariable\"",
 							"Doc": "check for unused variables and suggest fixes",
-							"Default": "false"
+							"Default": "true"
 						},
 						{
 							"Name": "\"unusedwrite\"",
@@ -1267,7 +1267,7 @@
 			"Name": "unusedvariable",
 			"Doc": "check for unused variables and suggest fixes",
 			"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedvariable",
-			"Default": false
+			"Default": true
 		},
 		{
 			"Name": "unusedwrite",

gopls/internal/settings/analysis.go

@@ -197,7 +197,7 @@ func init() {
 		{analyzer: fillreturns.Analyzer},
 		{analyzer: nonewvars.Analyzer},
 		{analyzer: noresultvalues.Analyzer},
-		{analyzer: unusedvariable.Analyzer, nonDefault: true}, // not fully baked; see #54373
+		{analyzer: unusedvariable.Analyzer},
 	}
 	for _, analyzer := range analyzers {
 		DefaultAnalyzers[analyzer.analyzer.Name] = analyzer