gopls: skip unusedparams for generated files
The unusedparams analyzer makes a lot of noise for some generated files,
notably those generated by protoc-gen-go.
Since we can't actually edit generated files to fix this issue,
it seems better not to report it.
Fixes golang/go#71481
Change-Id: I4e73c74312dfea6ef4ce631cc029764519d6b809
Reviewed-on: https://go-review.googlesource.com/c/tools/+/645575
Reviewed-by: Sam Salisbury <samsalisbury@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Hongxiang Jiang <hxjiang@golang.org>
diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md
index 282a9f4..2749152 100644
--- a/gopls/doc/analyzers.md
+++ b/gopls/doc/analyzers.md
@@ -1000,6 +1000,8 @@
effects in the argument expressions; see
https://github.com/golang/tools/releases/tag/gopls%2Fv0.14.
+This analyzer ignores generated code.
+
Default: on.
Package documentation: [unusedparams](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedparams)
diff --git a/gopls/internal/analysis/unusedparams/doc.go b/gopls/internal/analysis/unusedparams/doc.go
index 07e43c0..16d318e 100644
--- a/gopls/internal/analysis/unusedparams/doc.go
+++ b/gopls/internal/analysis/unusedparams/doc.go
@@ -31,4 +31,6 @@
// arguments at call sites, while taking care to preserve any side
// effects in the argument expressions; see
// https://github.com/golang/tools/releases/tag/gopls%2Fv0.14.
+//
+// This analyzer ignores generated code.
package unusedparams
diff --git a/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/generatedcode.go b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/generatedcode.go
new file mode 100644
index 0000000..fdbe64d
--- /dev/null
+++ b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/generatedcode.go
@@ -0,0 +1,15 @@
+// Code generated with somegen DO NOT EDIT.
+//
+// Because this file is generated, there should be no diagnostics
+// reported for any unused parameters.
+
+package generatedcode
+
+// generatedInterface exists to ensure that the generated code
+// is considered when determining whether parameters are used
+// in non-generated code.
+type generatedInterface interface{ n(f bool) }
+
+func a(x bool) { println() }
+
+var v = func(x bool) { println() }
diff --git a/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/generatedcode.go.golden b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/generatedcode.go.golden
new file mode 100644
index 0000000..fdbe64d
--- /dev/null
+++ b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/generatedcode.go.golden
@@ -0,0 +1,15 @@
+// Code generated with somegen DO NOT EDIT.
+//
+// Because this file is generated, there should be no diagnostics
+// reported for any unused parameters.
+
+package generatedcode
+
+// generatedInterface exists to ensure that the generated code
+// is considered when determining whether parameters are used
+// in non-generated code.
+type generatedInterface interface{ n(f bool) }
+
+func a(x bool) { println() }
+
+var v = func(x bool) { println() }
diff --git a/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go
new file mode 100644
index 0000000..fe0ef94
--- /dev/null
+++ b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go
@@ -0,0 +1,20 @@
+package generatedcode
+
+// This file does not have the generated code comment.
+// It exists to ensure that generated code is considered
+// when determining whether or not function parameters
+// are used.
+
+type implementsGeneratedInterface struct{}
+
+// The f parameter should not be reported as unused,
+// because this method implements the parent interface defined
+// in the generated code.
+func (implementsGeneratedInterface) n(f bool) {
+ // The body must not be empty, otherwise unusedparams will
+ // not report the unused parameter regardles of the
+ // interface.
+ println()
+}
+
+func b(x bool) { println() } // want "unused parameter: x"
diff --git a/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go.golden b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go.golden
new file mode 100644
index 0000000..170dc85
--- /dev/null
+++ b/gopls/internal/analysis/unusedparams/testdata/src/generatedcode/nongeneratedcode.go.golden
@@ -0,0 +1,20 @@
+package generatedcode
+
+// This file does not have the generated code comment.
+// It exists to ensure that generated code is considered
+// when determining whether or not function parameters
+// are used.
+
+type implementsGeneratedInterface struct{}
+
+// The f parameter should not be reported as unused,
+// because this method implements the parent interface defined
+// in the generated code.
+func (implementsGeneratedInterface) n(f bool) {
+ // The body must not be empty, otherwise unusedparams will
+ // not report the unused parameter regardles of the
+ // interface.
+ println()
+}
+
+func b(_ bool) { println() } // want "unused parameter: x"
diff --git a/gopls/internal/analysis/unusedparams/unusedparams.go b/gopls/internal/analysis/unusedparams/unusedparams.go
index ff7fdc4..2986dfd 100644
--- a/gopls/internal/analysis/unusedparams/unusedparams.go
+++ b/gopls/internal/analysis/unusedparams/unusedparams.go
@@ -139,172 +139,193 @@
}
}
- // Check each non-address-taken function's parameters are all used.
- filter := []ast.Node{
- (*ast.FuncDecl)(nil),
- (*ast.FuncLit)(nil),
- }
- cursor.Root(inspect).Inspect(filter, func(c cursor.Cursor, push bool) bool {
- // (We always return true so that we visit nested FuncLits.)
-
+ // Inspect each file to see if it is generated.
+ //
+ // We do not want to report unused parameters in generated code itself,
+ // however we need to include generated code in the overall analysis as
+ // it may be calling functions in non-generated code.
+ files := []ast.Node{(*ast.File)(nil)}
+ cursor.Root(inspect).Inspect(files, func(c cursor.Cursor, push bool) bool {
if !push {
return true
}
- var (
- fn types.Object // function symbol (*Func, possibly *Var for a FuncLit)
- ftype *ast.FuncType
- body *ast.BlockStmt
- )
- switch n := c.Node().(type) {
- case *ast.FuncDecl:
- // We can't analyze non-Go functions.
- if n.Body == nil {
+ isGenerated := ast.IsGenerated(c.Node().(*ast.File))
+
+ // Descend into the file, check each non-address-taken function's parameters
+ // are all used.
+ funcs := []ast.Node{
+ (*ast.FuncDecl)(nil),
+ (*ast.FuncLit)(nil),
+ }
+ c.Inspect(funcs, func(c cursor.Cursor, push bool) bool {
+ // (We always return true so that we visit nested FuncLits.)
+ if !push {
return true
}
- // Ignore exported functions and methods: we
- // must assume they may be address-taken in
- // another package.
- if n.Name.IsExported() {
- return true
- }
+ var (
+ fn types.Object // function symbol (*Func, possibly *Var for a FuncLit)
+ ftype *ast.FuncType
+ body *ast.BlockStmt
+ )
+ switch n := c.Node().(type) {
+ case *ast.FuncDecl:
+ // We can't analyze non-Go functions.
+ if n.Body == nil {
+ return true
+ }
- // Ignore methods that match the name of any
- // interface method declared in this package,
- // as the method's signature may need to conform
- // to the interface.
- if n.Recv != nil && unexportedIMethodNames[n.Name.Name] {
- return true
- }
+ // Ignore exported functions and methods: we
+ // must assume they may be address-taken in
+ // another package.
+ if n.Name.IsExported() {
+ return true
+ }
- fn = pass.TypesInfo.Defs[n.Name].(*types.Func)
- ftype, body = n.Type, n.Body
+ // Ignore methods that match the name of any
+ // interface method declared in this package,
+ // as the method's signature may need to conform
+ // to the interface.
+ if n.Recv != nil && unexportedIMethodNames[n.Name.Name] {
+ return true
+ }
- case *ast.FuncLit:
- // Find the symbol for the variable (if any)
- // to which the FuncLit is bound.
- // (We don't bother to allow ParenExprs.)
- switch parent := c.Parent().Node().(type) {
- case *ast.AssignStmt:
- // f = func() {...}
- // f := func() {...}
- if e, idx := c.Edge(); e == edge.AssignStmt_Rhs {
- // Inv: n == AssignStmt.Rhs[idx]
- if id, ok := parent.Lhs[idx].(*ast.Ident); ok {
- fn = pass.TypesInfo.ObjectOf(id)
+ fn = pass.TypesInfo.Defs[n.Name].(*types.Func)
+ ftype, body = n.Type, n.Body
- // Edge case: f = func() {...}
- // should not count as a use.
- if pass.TypesInfo.Uses[id] != nil {
- usesOutsideCall[fn] = moreslices.Remove(usesOutsideCall[fn], id)
+ case *ast.FuncLit:
+ // Find the symbol for the variable (if any)
+ // to which the FuncLit is bound.
+ // (We don't bother to allow ParenExprs.)
+ switch parent := c.Parent().Node().(type) {
+ case *ast.AssignStmt:
+ // f = func() {...}
+ // f := func() {...}
+ if e, idx := c.Edge(); e == edge.AssignStmt_Rhs {
+ // Inv: n == AssignStmt.Rhs[idx]
+ if id, ok := parent.Lhs[idx].(*ast.Ident); ok {
+ fn = pass.TypesInfo.ObjectOf(id)
+
+ // Edge case: f = func() {...}
+ // should not count as a use.
+ if pass.TypesInfo.Uses[id] != nil {
+ usesOutsideCall[fn] = moreslices.Remove(usesOutsideCall[fn], id)
+ }
+
+ if fn == nil && id.Name == "_" {
+ // Edge case: _ = func() {...}
+ // has no local var. Fake one.
+ v := types.NewVar(id.Pos(), pass.Pkg, id.Name, pass.TypesInfo.TypeOf(n))
+ typesinternal.SetVarKind(v, typesinternal.LocalVar)
+ fn = v
+ }
}
+ }
- if fn == nil && id.Name == "_" {
- // Edge case: _ = func() {...}
- // has no local var. Fake one.
- v := types.NewVar(id.Pos(), pass.Pkg, id.Name, pass.TypesInfo.TypeOf(n))
- typesinternal.SetVarKind(v, typesinternal.LocalVar)
- fn = v
+ case *ast.ValueSpec:
+ // var f = func() { ... }
+ // (unless f is an exported package-level var)
+ for i, val := range parent.Values {
+ if val == n {
+ v := pass.TypesInfo.Defs[parent.Names[i]]
+ if !(v.Parent() == pass.Pkg.Scope() && v.Exported()) {
+ fn = v
+ }
+ break
}
}
}
- case *ast.ValueSpec:
- // var f = func() { ... }
- // (unless f is an exported package-level var)
- for i, val := range parent.Values {
- if val == n {
- v := pass.TypesInfo.Defs[parent.Names[i]]
- if !(v.Parent() == pass.Pkg.Scope() && v.Exported()) {
- fn = v
+ ftype, body = n.Type, n.Body
+ }
+
+ // Ignore address-taken functions and methods: unused
+ // parameters may be needed to conform to a func type.
+ if fn == nil || len(usesOutsideCall[fn]) > 0 {
+ return true
+ }
+
+ // If there are no parameters, there are no unused parameters.
+ if ftype.Params.NumFields() == 0 {
+ return true
+ }
+
+ // To reduce false positives, ignore functions with an
+ // empty or panic body.
+ //
+ // We choose not to ignore functions whose body is a
+ // single return statement (as earlier versions did)
+ // func f() { return }
+ // func f() { return g(...) }
+ // as we suspect that was just heuristic to reduce
+ // false positives in the earlier unsound algorithm.
+ switch len(body.List) {
+ case 0:
+ // Empty body. Although the parameter is
+ // unnecessary, it's pretty obvious to the
+ // reader that that's the case, so we allow it.
+ return true // func f() {}
+ case 1:
+ if stmt, ok := body.List[0].(*ast.ExprStmt); ok {
+ // We allow a panic body, as it is often a
+ // placeholder for a future implementation:
+ // func f() { panic(...) }
+ if call, ok := stmt.X.(*ast.CallExpr); ok {
+ if fun, ok := call.Fun.(*ast.Ident); ok && fun.Name == "panic" {
+ return true
}
- break
}
}
}
- ftype, body = n.Type, n.Body
- }
-
- // Ignore address-taken functions and methods: unused
- // parameters may be needed to conform to a func type.
- if fn == nil || len(usesOutsideCall[fn]) > 0 {
- return true
- }
-
- // If there are no parameters, there are no unused parameters.
- if ftype.Params.NumFields() == 0 {
- return true
- }
-
- // To reduce false positives, ignore functions with an
- // empty or panic body.
- //
- // We choose not to ignore functions whose body is a
- // single return statement (as earlier versions did)
- // func f() { return }
- // func f() { return g(...) }
- // as we suspect that was just heuristic to reduce
- // false positives in the earlier unsound algorithm.
- switch len(body.List) {
- case 0:
- // Empty body. Although the parameter is
- // unnecessary, it's pretty obvious to the
- // reader that that's the case, so we allow it.
- return true // func f() {}
- case 1:
- if stmt, ok := body.List[0].(*ast.ExprStmt); ok {
- // We allow a panic body, as it is often a
- // placeholder for a future implementation:
- // func f() { panic(...) }
- if call, ok := stmt.X.(*ast.CallExpr); ok {
- if fun, ok := call.Fun.(*ast.Ident); ok && fun.Name == "panic" {
- return true
- }
- }
+ // Don't report diagnostics on generated files.
+ if isGenerated {
+ return true
}
- }
- // Report each unused parameter.
- for _, field := range ftype.Params.List {
- for _, id := range field.Names {
- if id.Name == "_" {
- continue
- }
- param := pass.TypesInfo.Defs[id].(*types.Var)
- if !usedVars[param] {
- start, end := field.Pos(), field.End()
- if len(field.Names) > 1 {
- start, end = id.Pos(), id.End()
+ // Report each unused parameter.
+ for _, field := range ftype.Params.List {
+ for _, id := range field.Names {
+ if id.Name == "_" {
+ continue
}
- // This diagnostic carries both an edit-based fix to
- // rename the unused parameter, and a command-based fix
- // to remove it (see golang.RemoveUnusedParameter).
- pass.Report(analysis.Diagnostic{
- Pos: start,
- End: end,
- Message: fmt.Sprintf("unused parameter: %s", id.Name),
- Category: FixCategory,
- SuggestedFixes: []analysis.SuggestedFix{
- {
- Message: `Rename parameter to "_"`,
- TextEdits: []analysis.TextEdit{{
- Pos: id.Pos(),
- End: id.End(),
- NewText: []byte("_"),
- }},
+ param := pass.TypesInfo.Defs[id].(*types.Var)
+ if !usedVars[param] {
+ start, end := field.Pos(), field.End()
+ if len(field.Names) > 1 {
+ start, end = id.Pos(), id.End()
+ }
+
+ // This diagnostic carries both an edit-based fix to
+ // rename the unused parameter, and a command-based fix
+ // to remove it (see golang.RemoveUnusedParameter).
+ pass.Report(analysis.Diagnostic{
+ Pos: start,
+ End: end,
+ Message: fmt.Sprintf("unused parameter: %s", id.Name),
+ Category: FixCategory,
+ SuggestedFixes: []analysis.SuggestedFix{
+ {
+ Message: `Rename parameter to "_"`,
+ TextEdits: []analysis.TextEdit{{
+ Pos: id.Pos(),
+ End: id.End(),
+ NewText: []byte("_"),
+ }},
+ },
+ {
+ Message: fmt.Sprintf("Remove unused parameter %q", id.Name),
+ // No TextEdits => computed by gopls command
+ },
},
- {
- Message: fmt.Sprintf("Remove unused parameter %q", id.Name),
- // No TextEdits => computed by gopls command
- },
- },
- })
+ })
+ }
}
}
- }
+ return true
+ })
return true
})
return nil, nil
diff --git a/gopls/internal/analysis/unusedparams/unusedparams_test.go b/gopls/internal/analysis/unusedparams/unusedparams_test.go
index 1e2d885..e943c20 100644
--- a/gopls/internal/analysis/unusedparams/unusedparams_test.go
+++ b/gopls/internal/analysis/unusedparams/unusedparams_test.go
@@ -13,5 +13,5 @@
func Test(t *testing.T) {
testdata := analysistest.TestData()
- analysistest.RunWithSuggestedFixes(t, testdata, unusedparams.Analyzer, "a", "typeparams")
+ analysistest.RunWithSuggestedFixes(t, testdata, unusedparams.Analyzer, "a", "generatedcode", "typeparams")
}
diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json
index 83151ae..74a6159 100644
--- a/gopls/internal/doc/api.json
+++ b/gopls/internal/doc/api.json
@@ -635,7 +635,7 @@
},
{
"Name": "\"unusedparams\"",
- "Doc": "check for unused parameters of functions\n\nThe unusedparams analyzer checks functions to see if there are\nany parameters that are not being used.\n\nTo ensure soundness, it ignores:\n - \"address-taken\" functions, that is, functions that are used as\n a value rather than being called directly; their signatures may\n be required to conform to a func type.\n - exported functions or methods, since they may be address-taken\n in another package.\n - unexported methods whose name matches an interface method\n declared in the same package, since the method's signature\n may be required to conform to the interface type.\n - functions with empty bodies, or containing just a call to panic.\n - parameters that are unnamed, or named \"_\", the blank identifier.\n\nThe analyzer suggests a fix of replacing the parameter name by \"_\",\nbut in such cases a deeper fix can be obtained by invoking the\n\"Refactor: remove unused parameter\" code action, which will\neliminate the parameter entirely, along with all corresponding\narguments at call sites, while taking care to preserve any side\neffects in the argument expressions; see\nhttps://github.com/golang/tools/releases/tag/gopls%2Fv0.14.",
+ "Doc": "check for unused parameters of functions\n\nThe unusedparams analyzer checks functions to see if there are\nany parameters that are not being used.\n\nTo ensure soundness, it ignores:\n - \"address-taken\" functions, that is, functions that are used as\n a value rather than being called directly; their signatures may\n be required to conform to a func type.\n - exported functions or methods, since they may be address-taken\n in another package.\n - unexported methods whose name matches an interface method\n declared in the same package, since the method's signature\n may be required to conform to the interface type.\n - functions with empty bodies, or containing just a call to panic.\n - parameters that are unnamed, or named \"_\", the blank identifier.\n\nThe analyzer suggests a fix of replacing the parameter name by \"_\",\nbut in such cases a deeper fix can be obtained by invoking the\n\"Refactor: remove unused parameter\" code action, which will\neliminate the parameter entirely, along with all corresponding\narguments at call sites, while taking care to preserve any side\neffects in the argument expressions; see\nhttps://github.com/golang/tools/releases/tag/gopls%2Fv0.14.\n\nThis analyzer ignores generated code.",
"Default": "true"
},
{
@@ -1339,7 +1339,7 @@
},
{
"Name": "unusedparams",
- "Doc": "check for unused parameters of functions\n\nThe unusedparams analyzer checks functions to see if there are\nany parameters that are not being used.\n\nTo ensure soundness, it ignores:\n - \"address-taken\" functions, that is, functions that are used as\n a value rather than being called directly; their signatures may\n be required to conform to a func type.\n - exported functions or methods, since they may be address-taken\n in another package.\n - unexported methods whose name matches an interface method\n declared in the same package, since the method's signature\n may be required to conform to the interface type.\n - functions with empty bodies, or containing just a call to panic.\n - parameters that are unnamed, or named \"_\", the blank identifier.\n\nThe analyzer suggests a fix of replacing the parameter name by \"_\",\nbut in such cases a deeper fix can be obtained by invoking the\n\"Refactor: remove unused parameter\" code action, which will\neliminate the parameter entirely, along with all corresponding\narguments at call sites, while taking care to preserve any side\neffects in the argument expressions; see\nhttps://github.com/golang/tools/releases/tag/gopls%2Fv0.14.",
+ "Doc": "check for unused parameters of functions\n\nThe unusedparams analyzer checks functions to see if there are\nany parameters that are not being used.\n\nTo ensure soundness, it ignores:\n - \"address-taken\" functions, that is, functions that are used as\n a value rather than being called directly; their signatures may\n be required to conform to a func type.\n - exported functions or methods, since they may be address-taken\n in another package.\n - unexported methods whose name matches an interface method\n declared in the same package, since the method's signature\n may be required to conform to the interface type.\n - functions with empty bodies, or containing just a call to panic.\n - parameters that are unnamed, or named \"_\", the blank identifier.\n\nThe analyzer suggests a fix of replacing the parameter name by \"_\",\nbut in such cases a deeper fix can be obtained by invoking the\n\"Refactor: remove unused parameter\" code action, which will\neliminate the parameter entirely, along with all corresponding\narguments at call sites, while taking care to preserve any side\neffects in the argument expressions; see\nhttps://github.com/golang/tools/releases/tag/gopls%2Fv0.14.\n\nThis analyzer ignores generated code.",
"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedparams",
"Default": true
},
