diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index fd65c3a2a9..861be1ca62 100644 --- a/gopls/doc/analyzers.md +++ b/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 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 0000000000..eccfe14c3a --- /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 0000000000..8d6e561fa6 --- /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 0000000000..024e49db9c --- /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 0000000000..a589a47af1 --- /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 0000000000..47564f1f15 --- /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 0000000000..e6d7c020f0 --- /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 4e2183cf4e..2493da25d8 100755 --- a/internal/lsp/source/api_json.go +++ b/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", diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 5da14ebfe9..09401bc6ef 100644 --- a/internal/lsp/source/options.go +++ b/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, + }, } }