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>
diff --git a/internal/lsp/analysis/unusedvariable/testdata/src/assign/a.go b/internal/lsp/analysis/unusedvariable/testdata/src/assign/a.go
new file mode 100644
index 0000000..eccfe14
--- /dev/null
+++ b/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
+}
diff --git a/internal/lsp/analysis/unusedvariable/testdata/src/assign/a.go.golden b/internal/lsp/analysis/unusedvariable/testdata/src/assign/a.go.golden
new file mode 100644
index 0000000..8d6e561
--- /dev/null
+++ b/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
+}
diff --git a/internal/lsp/analysis/unusedvariable/testdata/src/decl/a.go b/internal/lsp/analysis/unusedvariable/testdata/src/decl/a.go
new file mode 100644
index 0000000..024e49d
--- /dev/null
+++ b/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)
+}
diff --git a/internal/lsp/analysis/unusedvariable/testdata/src/decl/a.go.golden b/internal/lsp/analysis/unusedvariable/testdata/src/decl/a.go.golden
new file mode 100644
index 0000000..a589a47
--- /dev/null
+++ b/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)
+}
diff --git a/internal/lsp/analysis/unusedvariable/unusedvariable.go b/internal/lsp/analysis/unusedvariable/unusedvariable.go
new file mode 100644
index 0000000..47564f1
--- /dev/null
+++ b/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
+}
diff --git a/internal/lsp/analysis/unusedvariable/unusedvariable_test.go b/internal/lsp/analysis/unusedvariable/unusedvariable_test.go
new file mode 100644
index 0000000..e6d7c02
--- /dev/null
+++ b/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")
+ })
+}
diff --git a/internal/lsp/source/api_json.go b/internal/lsp/source/api_json.go
index 4e2183c..2493da2 100755
--- a/internal/lsp/source/api_json.go
+++ b/internal/lsp/source/api_json.go
@@ -434,6 +434,11 @@
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",
Default: "true",
@@ -1014,6 +1019,10 @@
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",
Default: true,
diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go
index 5da14eb..09401bc 100644
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@@ -61,6 +61,7 @@
"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 @@
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 @@
Fix: UndeclaredName,
Enabled: true,
},
+ unusedvariable.Analyzer.Name: {
+ Analyzer: unusedvariable.Analyzer,
+ Enabled: false,
+ },
}
}
