internal/lsp/analysis/unusedvariable: add analyzer

This analyzer suggests fixes for unused variable errors.
In declarations it will remove the whole statement if the offending
variable is the only one declared in that statement, otherwise it will
just delete the offending variable.
In assignments it will remove the whole statement if the offending
variable is the only one assigned in that statement, otherwise it will
rename the offending variable to `_`. If the assignment RHS contains a
statement that can cause a side effect (a function call or reading from
a channel), the assignment will be removed but RHS will be preserved.

Fixes golang/go#48975

Change-Id: I3850f1b0340cd5ae63249931df3a5403d8617080
Reviewed-on: https://go-review.googlesource.com/c/tools/+/394934
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>

gopls/doc/analyzers.md

@@ -657,6 +657,15 @@ func <>(inferred parameters) {
 
 **Enabled by default.**
 
+## **unusedvariable**
+
+check for unused variables
+
+The unusedvariable analyzer suggests fixes for unused variables errors.
+
+
+**Disabled by default. Enable it by setting `"analyses": {"unusedvariable": true}`.**
+
 ## **fillstruct**
 
 note incomplete struct initializations

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

@@ -0,0 +1,74 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package a
+
+import (
+	"fmt"
+	"os"
+)
+
+type A struct {
+	b int
+}
+
+func singleAssignment() {
+	v := "s" // want `v declared but not used`
+
+	s := []int{ // want `s declared but not used`
+		1,
+		2,
+	}
+
+	a := func(s string) bool { // want `a declared but not used`
+		return false
+	}
+
+	if 1 == 1 {
+		s := "v" // want `s declared but not used`
+	}
+
+	panic("I should survive")
+}
+
+func noOtherStmtsInBlock() {
+	v := "s" // want `v declared but not used`
+}
+
+func partOfMultiAssignment() {
+	f, err := os.Open("file") // want `f declared but not used`
+	panic(err)
+}
+
+func sideEffects(cBool chan bool, cInt chan int) {
+	b := <-c            // want `b declared but not used`
+	s := fmt.Sprint("") // want `s declared but not used`
+	a := A{             // want `a declared but not used`
+		b: func() int {
+			return 1
+		}(),
+	}
+	c := A{<-cInt}          // want `c declared but not used`
+	d := fInt() + <-cInt    // want `d declared but not used`
+	e := fBool() && <-cBool // want `e declared but not used`
+	f := map[int]int{       // want `f declared but not used`
+		fInt(): <-cInt,
+	}
+	g := []int{<-cInt}       // want `g declared but not used`
+	h := func(s string) {}   // want `h declared but not used`
+	i := func(s string) {}() // want `i declared but not used`
+}
+
+func commentAbove() {
+	// v is a variable
+	v := "s" // want `v declared but not used`
+}
+
+func fBool() bool {
+	return true
+}
+
+func fInt() int {
+	return 1
+}

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

@@ -0,0 +1,59 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package a
+
+import (
+	"fmt"
+	"os"
+)
+
+type A struct {
+	b int
+}
+
+func singleAssignment() {
+	if 1 == 1 {
+	}
+
+	panic("I should survive")
+}
+
+func noOtherStmtsInBlock() {
+}
+
+func partOfMultiAssignment() {
+	_, err := os.Open("file") // want `f declared but not used`
+	panic(err)
+}
+
+func sideEffects(cBool chan bool, cInt chan int) {
+	<-c            // want `b declared but not used`
+	fmt.Sprint("") // want `s declared but not used`
+	A{             // want `a declared but not used`
+		b: func() int {
+			return 1
+		}(),
+	}
+	A{<-cInt}          // want `c declared but not used`
+	fInt() + <-cInt    // want `d declared but not used`
+	fBool() && <-cBool // want `e declared but not used`
+	map[int]int{       // want `f declared but not used`
+		fInt(): <-cInt,
+	}
+	[]int{<-cInt}       // want `g declared but not used`
+	func(s string) {}() // want `i declared but not used`
+}
+
+func commentAbove() {
+	// v is a variable
+}
+
+func fBool() bool {
+	return true
+}
+
+func fInt() int {
+	return 1
+}

internal/lsp/analysis/unusedvariable/testdata/src/decl/a.go

@@ -0,0 +1,30 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package decl
+
+func a() {
+	var b, c bool // want `b declared but not used`
+	panic(c)
+
+	if 1 == 1 {
+		var s string // want `s declared but not used`
+	}
+}
+
+func b() {
+	// b is a variable
+	var b bool // want `b declared but not used`
+}
+
+func c() {
+	var (
+		d string
+
+		// some comment for c
+		c bool // want `c declared but not used`
+	)
+
+	panic(d)
+}

internal/lsp/analysis/unusedvariable/testdata/src/decl/a.go.golden

@@ -0,0 +1,24 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package decl
+
+func a() {
+	var c bool // want `b declared but not used`
+	panic(c)
+
+	if 1 == 1 {
+	}
+}
+
+func b() {
+	// b is a variable
+}
+
+func c() {
+	var (
+		d string
+	)
+	panic(d)
+}

internal/lsp/analysis/unusedvariable/unusedvariable.go

@@ -0,0 +1,300 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package unusedvariable defines an analyzer that checks for unused variables.
+package unusedvariable
+
+import (
+	"bytes"
+	"fmt"
+	"go/ast"
+	"go/format"
+	"go/token"
+	"go/types"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/ast/astutil"
+	"golang.org/x/tools/internal/analysisinternal"
+)
+
+const Doc = `check for unused variables
+
+The unusedvariable analyzer suggests fixes for unused variables errors.
+`
+
+var Analyzer = &analysis.Analyzer{
+	Name:             "unusedvariable",
+	Doc:              Doc,
+	Requires:         []*analysis.Analyzer{},
+	Run:              run,
+	RunDespiteErrors: true, // an unusedvariable diagnostic is a compile error
+}
+
+type fixesForError map[types.Error][]analysis.SuggestedFix
+
+const unusedVariableSuffix = " declared but not used"
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	for _, typeErr := range analysisinternal.GetTypeErrors(pass) {
+		if strings.HasSuffix(typeErr.Msg, unusedVariableSuffix) {
+			varName := strings.TrimSuffix(typeErr.Msg, unusedVariableSuffix)
+			err := runForError(pass, typeErr, varName)
+			if err != nil {
+				return nil, err
+			}
+		}
+	}
+
+	return nil, nil
+}
+
+func runForError(pass *analysis.Pass, err types.Error, name string) error {
+	var file *ast.File
+	for _, f := range pass.Files {
+		if f.Pos() <= err.Pos && err.Pos < f.End() {
+			file = f
+			break
+		}
+	}
+	if file == nil {
+		return nil
+	}
+
+	path, _ := astutil.PathEnclosingInterval(file, err.Pos, err.Pos)
+	if len(path) < 2 {
+		return nil
+	}
+
+	ident, ok := path[0].(*ast.Ident)
+	if !ok || ident.Name != name {
+		return nil
+	}
+
+	diag := analysis.Diagnostic{
+		Pos:     ident.Pos(),
+		End:     ident.End(),
+		Message: err.Msg,
+	}
+
+	for i := range path {
+		switch stmt := path[i].(type) {
+		case *ast.ValueSpec:
+			// Find GenDecl to which offending ValueSpec belongs.
+			if decl, ok := path[i+1].(*ast.GenDecl); ok {
+				fixes := removeVariableFromSpec(pass, path, stmt, decl, ident)
+				// fixes may be nil
+				if len(fixes) > 0 {
+					diag.SuggestedFixes = fixes
+					pass.Report(diag)
+				}
+			}
+
+		case *ast.AssignStmt:
+			if stmt.Tok != token.DEFINE {
+				continue
+			}
+
+			containsIdent := false
+			for _, expr := range stmt.Lhs {
+				if expr == ident {
+					containsIdent = true
+				}
+			}
+			if !containsIdent {
+				continue
+			}
+
+			fixes := removeVariableFromAssignment(pass, path, stmt, ident)
+			// fixes may be nil
+			if len(fixes) > 0 {
+				diag.SuggestedFixes = fixes
+				pass.Report(diag)
+			}
+		}
+	}
+
+	return nil
+}
+
+func removeVariableFromSpec(pass *analysis.Pass, path []ast.Node, stmt *ast.ValueSpec, decl *ast.GenDecl, ident *ast.Ident) []analysis.SuggestedFix {
+	newDecl := new(ast.GenDecl)
+	*newDecl = *decl
+	newDecl.Specs = nil
+
+	for _, spec := range decl.Specs {
+		if spec != stmt {
+			newDecl.Specs = append(newDecl.Specs, spec)
+			continue
+		}
+
+		newSpec := new(ast.ValueSpec)
+		*newSpec = *stmt
+		newSpec.Names = nil
+
+		for _, n := range stmt.Names {
+			if n != ident {
+				newSpec.Names = append(newSpec.Names, n)
+			}
+		}
+
+		if len(newSpec.Names) > 0 {
+			newDecl.Specs = append(newDecl.Specs, newSpec)
+		}
+	}
+
+	// decl.End() does not include any comments, so if a comment is present we
+	// need to account for it when we delete the statement
+	end := decl.End()
+	if stmt.Comment != nil && stmt.Comment.End() > end {
+		end = stmt.Comment.End()
+	}
+
+	// There are no other specs left in the declaration, the whole statement can
+	// be deleted
+	if len(newDecl.Specs) == 0 {
+		// Find parent DeclStmt and delete it
+		for _, node := range path {
+			if declStmt, ok := node.(*ast.DeclStmt); ok {
+				return []analysis.SuggestedFix{
+					{
+						Message:   suggestedFixMessage(ident.Name),
+						TextEdits: deleteStmtFromBlock(path, declStmt),
+					},
+				}
+			}
+		}
+	}
+
+	var b bytes.Buffer
+	if err := format.Node(&b, pass.Fset, newDecl); err != nil {
+		return nil
+	}
+
+	return []analysis.SuggestedFix{
+		{
+			Message: suggestedFixMessage(ident.Name),
+			TextEdits: []analysis.TextEdit{
+				{
+					Pos: decl.Pos(),
+					// Avoid adding a new empty line
+					End:     end + 1,
+					NewText: b.Bytes(),
+				},
+			},
+		},
+	}
+}
+
+func removeVariableFromAssignment(pass *analysis.Pass, 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
+		// on RHS
+		//
+		// RHS may have side effects, preserve RHS
+		if exprMayHaveSideEffects(stmt.Rhs[0]) {
+			// Delete until RHS
+			return []analysis.SuggestedFix{
+				{
+					Message: suggestedFixMessage(ident.Name),
+					TextEdits: []analysis.TextEdit{
+						{
+							Pos: ident.Pos(),
+							End: stmt.Rhs[0].Pos(),
+						},
+					},
+				},
+			}
+		}
+
+		// RHS does not have any side effects, delete the whole statement
+		return []analysis.SuggestedFix{
+			{
+				Message:   suggestedFixMessage(ident.Name),
+				TextEdits: deleteStmtFromBlock(path, stmt),
+			},
+		}
+	}
+
+	// Otherwise replace ident with `_`
+	return []analysis.SuggestedFix{
+		{
+			Message: suggestedFixMessage(ident.Name),
+			TextEdits: []analysis.TextEdit{
+				{
+					Pos:     ident.Pos(),
+					End:     ident.End(),
+					NewText: []byte("_"),
+				},
+			},
+		},
+	}
+}
+
+func suggestedFixMessage(name string) string {
+	return fmt.Sprintf("Remove variable %s", name)
+}
+
+func deleteStmtFromBlock(path []ast.Node, stmt ast.Stmt) []analysis.TextEdit {
+	// Find innermost enclosing BlockStmt.
+	var block *ast.BlockStmt
+	for i := range path {
+		if blockStmt, ok := path[i].(*ast.BlockStmt); ok {
+			block = blockStmt
+			break
+		}
+	}
+
+	nodeIndex := -1
+	for i, blockStmt := range block.List {
+		if blockStmt == stmt {
+			nodeIndex = i
+			break
+		}
+	}
+
+	// The statement we need to delete was not found in BlockStmt
+	if nodeIndex == -1 {
+		return nil
+	}
+
+	// Delete until the end of the block unless there is another statement after
+	// the one we are trying to delete
+	end := block.Rbrace
+	if nodeIndex < len(block.List)-1 {
+		end = block.List[nodeIndex+1].Pos()
+	}
+
+	return []analysis.TextEdit{
+		{
+			Pos: stmt.Pos(),
+			End: end,
+		},
+	}
+}
+
+// exprMayHaveSideEffects reports whether the expression may have side effects
+// (because it contains a function call or channel receive). We disregard
+// runtime panics as well written programs should not encounter them.
+func exprMayHaveSideEffects(expr ast.Expr) bool {
+	var mayHaveSideEffects bool
+	ast.Inspect(expr, func(n ast.Node) bool {
+		switch n := n.(type) {
+		case *ast.CallExpr: // possible function call
+			mayHaveSideEffects = true
+			return false
+		case *ast.UnaryExpr:
+			if n.Op == token.ARROW { // channel receive
+				mayHaveSideEffects = true
+				return false
+			}
+		case *ast.FuncLit:
+			return false // evaluating what's inside a FuncLit has no effect
+		}
+		return true
+	})
+
+	return mayHaveSideEffects
+}

internal/lsp/analysis/unusedvariable/unusedvariable_test.go

@@ -0,0 +1,24 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package unusedvariable_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/internal/lsp/analysis/unusedvariable"
+)
+
+func Test(t *testing.T) {
+	testdata := analysistest.TestData()
+
+	t.Run("decl", func(t *testing.T) {
+		analysistest.RunWithSuggestedFixes(t, testdata, unusedvariable.Analyzer, "decl")
+	})
+
+	t.Run("assign", func(t *testing.T) {
+		analysistest.RunWithSuggestedFixes(t, testdata, unusedvariable.Analyzer, "assign")
+	})
+}

internal/lsp/source/api_json.go

@@ -433,6 +433,11 @@ var GeneratedAPIJSON = &APIJSON{
 							Doc:     "suggested fixes for \"undeclared name: <>\"\n\nThis checker provides suggested fixes for type errors of the\ntype \"undeclared name: <>\". It will either insert a new statement,\nsuch as:\n\n\"<> := \"\n\nor a new function declaration, such as:\n\nfunc <>(inferred parameters) {\n\tpanic(\"implement me!\")\n}\n",
 							Default: "true",
 						},
+						{
+							Name:    "\"unusedvariable\"",
+							Doc:     "check for unused variables\n\nThe unusedvariable analyzer suggests fixes for unused variables errors.\n",
+							Default: "false",
+						},
 						{
 							Name:    "\"fillstruct\"",
 							Doc:     "note incomplete struct initializations\n\nThis analyzer provides diagnostics for any struct literals that do not have\nany fields initialized. Because the suggested fix for this analysis is\nexpensive to compute, callers should compute it separately, using the\nSuggestedFix function below.\n",
@@ -1013,6 +1018,10 @@ var GeneratedAPIJSON = &APIJSON{
 			Doc:     "suggested fixes for \"undeclared name: <>\"\n\nThis checker provides suggested fixes for type errors of the\ntype \"undeclared name: <>\". It will either insert a new statement,\nsuch as:\n\n\"<> := \"\n\nor a new function declaration, such as:\n\nfunc <>(inferred parameters) {\n\tpanic(\"implement me!\")\n}\n",
 			Default: true,
 		},
+		{
+			Name: "unusedvariable",
+			Doc:  "check for unused variables\n\nThe unusedvariable analyzer suggests fixes for unused variables errors.\n",
+		},
 		{
 			Name:    "fillstruct",
 			Doc:     "note incomplete struct initializations\n\nThis analyzer provides diagnostics for any struct literals that do not have\nany fields initialized. Because the suggested fix for this analysis is\nexpensive to compute, callers should compute it separately, using the\nSuggestedFix function below.\n",

internal/lsp/source/options.go

@@ -61,6 +61,7 @@ import (
 	"golang.org/x/tools/internal/lsp/analysis/stubmethods"
 	"golang.org/x/tools/internal/lsp/analysis/undeclaredname"
 	"golang.org/x/tools/internal/lsp/analysis/unusedparams"
+	"golang.org/x/tools/internal/lsp/analysis/unusedvariable"
 	"golang.org/x/tools/internal/lsp/analysis/useany"
 	"golang.org/x/tools/internal/lsp/command"
 	"golang.org/x/tools/internal/lsp/diff"
@@ -800,6 +801,9 @@ func (o *Options) enableAllExperimentMaps() {
 	if _, ok := o.Analyses[unusedparams.Analyzer.Name]; !ok {
 		o.Analyses[unusedparams.Analyzer.Name] = true
 	}
+	if _, ok := o.Analyses[unusedvariable.Analyzer.Name]; !ok {
+		o.Analyses[unusedvariable.Analyzer.Name] = true
+	}
 }
 
 func (o *Options) set(name string, value interface{}, seen map[string]struct{}) OptionResult {
@@ -1270,6 +1274,10 @@ func typeErrorAnalyzers() map[string]*Analyzer {
 			Fix:      UndeclaredName,
 			Enabled:  true,
 		},
+		unusedvariable.Analyzer.Name: {
+			Analyzer: unusedvariable.Analyzer,
+			Enabled:  false,
+		},
 	}
 }