gopls/internal/analysis/unusedparams: eliminate false positives

This change is a substantial rewrite of the unusedparams analyzer
designed to eliminate essentially all false positives, based on
the rule that unless we can see all uses of a function, we can't
tell whether it is safe to alter its signature.

The new algorithm computes the set of address-taken functions,
that is, functions that are referenced other than in the
CallExpr.Fun position of a function call. For soundness we must
assume that such values may be required to conform to some func
type, so we cannot alter their signature.

Exported functions and methods must also be treated
conservatively, as they may be address-taken in another package.
And unexported methods are ignored if they match the name of an
interface method declared in the same package, since they may be
required to conform to it.

Anonymous functions that are immediately assigned to a var are
treated like named functions, with the var symbol standing for
the function name.

Since the false positive rate is now close to zero, this change
also enables the analyzer by default in gopls.

Number of findings:

   Repo     Before	After    Comments
  x/tools       86         26	 100% true positives
      k8s     2457       1987    Only 14% are in non-generated
                                 files, and of those, all that
                                 I sampled were true positives.

Updates golang/go#64753

Change-Id: I35dfc116a97bb5e98a2b098047fce176f2c1a9d6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550355
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>

gopls/doc/analyzers.md

@@ -763,15 +763,29 @@ unusedparams: check for unused parameters of functions
 The unusedparams analyzer checks functions to see if there are
 any parameters that are not being used.
 
-To reduce false positives it ignores:
-- methods
-- parameters that do not have a name or have the name '_' (the blank identifier)
-- functions in test files
-- functions with empty bodies or those with just a return stmt
+To ensure soundness, it ignores:
+  - "address-taken" functions, that is, functions that are used as
+    a value rather than being called directly; their signatures may
+    be required to conform to a func type.
+  - exported functions or methods, since they may be address-taken
+    in another package.
+  - unexported methods whose name matches an interface method
+    declared in the same package, since the method's signature
+    may be required to conform to the interface type.
+  - functions with empty bodies, or containing just a call to panic.
+  - parameters that are unnamed, or named "_", the blank identifier.
+
+The analyzer suggests a fix of replacing the parameter name by "_",
+but in such cases a deeper fix can be obtained by invoking the
+"Refactor: remove unused parameter" code action, which will
+eliminate the parameter entirely, along with all corresponding
+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.
 
 [Full documentation](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedparams)
 
-**Disabled by default. Enable it by setting `"analyses": {"unusedparams": true}`.**
+**Enabled by default.**
 
 ## **unusedresult**
 

gopls/doc/settings.md

@@ -286,7 +286,7 @@ Example Usage:
 ...
 "analyses": {
   "unreachable": false, // Disable the unreachable analyzer.
-  "unusedparams": true  // Enable the unusedparams analyzer.
+  "unusedvariable": true  // Enable the unusedvariable analyzer.
 }
 ...
 ```

gopls/internal/analysis/unusedparams/cmd/main.go

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-// The stringintconv command runs the stringintconv analyzer.
+// The unusedparams command runs the unusedparams analyzer.
 package main
 
 import (

gopls/internal/analysis/unusedparams/doc.go

@@ -12,9 +12,23 @@
 // The unusedparams analyzer checks functions to see if there are
 // any parameters that are not being used.
 //
-// To reduce false positives it ignores:
-// - methods
-// - parameters that do not have a name or have the name '_' (the blank identifier)
-// - functions in test files
-// - functions with empty bodies or those with just a return stmt
+// To ensure soundness, it ignores:
+//   - "address-taken" functions, that is, functions that are used as
+//     a value rather than being called directly; their signatures may
+//     be required to conform to a func type.
+//   - exported functions or methods, since they may be address-taken
+//     in another package.
+//   - unexported methods whose name matches an interface method
+//     declared in the same package, since the method's signature
+//     may be required to conform to the interface type.
+//   - functions with empty bodies, or containing just a call to panic.
+//   - parameters that are unnamed, or named "_", the blank identifier.
+//
+// The analyzer suggests a fix of replacing the parameter name by "_",
+// but in such cases a deeper fix can be obtained by invoking the
+// "Refactor: remove unused parameter" code action, which will
+// eliminate the parameter entirely, along with all corresponding
+// 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.
 package unusedparams

gopls/internal/analysis/unusedparams/testdata/src/a/a.go

@@ -24,28 +24,28 @@ func (y *yuh) n(f bool) {
 	}
 }
 
-func a(i1 int, i2 int, i3 int) int { // want "potentially unused parameter: 'i2'"
+func a(i1 int, i2 int, i3 int) int { // want "unused parameter: i2"
 	i3 += i1
-	_ = func(z int) int { // want "potentially unused parameter: 'z'"
+	_ = func(z int) int { // want "unused parameter: z"
 		_ = 1
 		return 1
 	}
 	return i3
 }
 
-func b(c bytes.Buffer) { // want "potentially unused parameter: 'c'"
+func b(c bytes.Buffer) { // want "unused parameter: c"
 	_ = 1
 }
 
-func z(h http.ResponseWriter, _ *http.Request) { // want "potentially unused parameter: 'h'"
+func z(h http.ResponseWriter, _ *http.Request) { // no report: func z is address-taken
 	fmt.Println("Before")
 }
 
-func l(h http.Handler) http.Handler {
+func l(h http.Handler) http.Handler { // want "unused parameter: h"
 	return http.HandlerFunc(z)
 }
 
-func mult(a, b int) int { // want "potentially unused parameter: 'b'"
+func mult(a, b int) int { // want "unused parameter: b"
 	a += 1
 	return a
 }
@@ -53,3 +53,35 @@ func mult(a, b int) int { // want "potentially unused parameter: 'b'"
 func y(a int) {
 	panic("yo")
 }
+
+var _ = func(x int) {} // empty body: no diagnostic
+
+var _ = func(x int) { println() } // want "unused parameter: x"
+
+var (
+	calledGlobal       = func(x int) { println() } // want "unused parameter: x"
+	addressTakenGlobal = func(x int) { println() } // no report: function is address-taken
+)
+
+func _() {
+	calledGlobal(1)
+	println(addressTakenGlobal)
+}
+
+func Exported(unused int) {} // no finding: an exported function may be address-taken
+
+type T int
+
+func (T) m(f bool) { println() } // want "unused parameter: f"
+func (T) n(f bool) { println() } // no finding: n may match the interface method parent.n
+
+func _() {
+	var fib func(x, y int) int
+	fib = func(x, y int) int { // want "unused parameter: y"
+		if x < 2 {
+			return x
+		}
+		return fib(x-1, 123) + fib(x-2, 456)
+	}
+	fib(10, 42)
+}

gopls/internal/analysis/unusedparams/testdata/src/a/a.go.golden

@@ -24,28 +24,28 @@ func (y *yuh) n(f bool) {
 	}
 }
 
-func a(i1 int, _ int, i3 int) int { // want "potentially unused parameter: 'i2'"
+func a(i1 int, _ int, i3 int) int { // want "unused parameter: i2"
 	i3 += i1
-	_ = func(_ int) int { // want "potentially unused parameter: 'z'"
+	_ = func(_ int) int { // want "unused parameter: z"
 		_ = 1
 		return 1
 	}
 	return i3
 }
 
-func b(_ bytes.Buffer) { // want "potentially unused parameter: 'c'"
+func b(_ bytes.Buffer) { // want "unused parameter: c"
 	_ = 1
 }
 
-func z(_ http.ResponseWriter, _ *http.Request) { // want "potentially unused parameter: 'h'"
+func z(h http.ResponseWriter, _ *http.Request) { // no report: func z is address-taken
 	fmt.Println("Before")
 }
 
-func l(h http.Handler) http.Handler {
+func l(_ http.Handler) http.Handler { // want "unused parameter: h"
 	return http.HandlerFunc(z)
 }
 
-func mult(a, _ int) int { // want "potentially unused parameter: 'b'"
+func mult(a, _ int) int { // want "unused parameter: b"
 	a += 1
 	return a
 }
@@ -53,3 +53,35 @@ func mult(a, _ int) int { // want "potentially unused parameter: 'b'"
 func y(a int) {
 	panic("yo")
 }
+
+var _ = func(x int) {} // empty body: no diagnostic
+
+var _ = func(_ int) { println() } // want "unused parameter: x"
+
+var (
+	calledGlobal       = func(_ int) { println() } // want "unused parameter: x"
+	addressTakenGlobal = func(x int) { println() } // no report: function is address-taken
+)
+
+func _() {
+	calledGlobal(1)
+	println(addressTakenGlobal)
+}
+
+func Exported(unused int) {} // no finding: an exported function may be address-taken
+
+type T int
+
+func (T) m(_ bool) { println() } // want "unused parameter: f"
+func (T) n(f bool) { println() } // no finding: n may match the interface method parent.n
+
+func _() {
+	var fib func(x, y int) int
+	fib = func(x, _ int) int { // want "unused parameter: y"
+		if x < 2 {
+			return x
+		}
+		return fib(x-1, 123) + fib(x-2, 456)
+	}
+	fib(10, 42)
+}

gopls/internal/analysis/unusedparams/testdata/src/typeparams/typeparams.go

@@ -24,28 +24,28 @@ func (y *yuh[int]) n(f bool) {
 	}
 }
 
-func a[T comparable](i1 int, i2 T, i3 int) int { // want "potentially unused parameter: 'i2'"
+func a[T comparable](i1 int, i2 T, i3 int) int { // want "unused parameter: i2"
 	i3 += i1
-	_ = func(z int) int { // want "potentially unused parameter: 'z'"
+	_ = func(z int) int { // want "unused parameter: z"
 		_ = 1
 		return 1
 	}
 	return i3
 }
 
-func b[T any](c bytes.Buffer) { // want "potentially unused parameter: 'c'"
+func b[T any](c bytes.Buffer) { // want "unused parameter: c"
 	_ = 1
 }
 
-func z[T http.ResponseWriter](h T, _ *http.Request) { // want "potentially unused parameter: 'h'"
+func z[T http.ResponseWriter](h T, _ *http.Request) { // no report: func z is address-taken
 	fmt.Println("Before")
 }
 
-func l(h http.Handler) http.Handler {
+func l(h http.Handler) http.Handler { // want "unused parameter: h"
 	return http.HandlerFunc(z[http.ResponseWriter])
 }
 
-func mult(a, b int) int { // want "potentially unused parameter: 'b'"
+func mult(a, b int) int { // want "unused parameter: b"
 	a += 1
 	return a
 }

gopls/internal/analysis/unusedparams/testdata/src/typeparams/typeparams.go.golden

@@ -24,28 +24,28 @@ func (y *yuh[int]) n(f bool) {
 	}
 }
 
-func a[T comparable](i1 int, _ T, i3 int) int { // want "potentially unused parameter: 'i2'"
+func a[T comparable](i1 int, _ T, i3 int) int { // want "unused parameter: i2"
 	i3 += i1
-	_ = func(_ int) int { // want "potentially unused parameter: 'z'"
+	_ = func(_ int) int { // want "unused parameter: z"
 		_ = 1
 		return 1
 	}
 	return i3
 }
 
-func b[T any](_ bytes.Buffer) { // want "potentially unused parameter: 'c'"
+func b[T any](_ bytes.Buffer) { // want "unused parameter: c"
 	_ = 1
 }
 
-func z[T http.ResponseWriter](_ T, _ *http.Request) { // want "potentially unused parameter: 'h'"
+func z[T http.ResponseWriter](h T, _ *http.Request) { // no report: func z is address-taken
 	fmt.Println("Before")
 }
 
-func l(h http.Handler) http.Handler {
+func l(_ http.Handler) http.Handler { // want "unused parameter: h"
 	return http.HandlerFunc(z[http.ResponseWriter])
 }
 
-func mult(a, _ int) int { // want "potentially unused parameter: 'b'"
+func mult(a, _ int) int { // want "unused parameter: b"
 	a += 1
 	return a
 }

gopls/internal/analysis/unusedparams/unusedparams.go

@@ -9,151 +9,289 @@ import (
 	"fmt"
 	"go/ast"
 	"go/types"
-	"strings"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/gopls/internal/util/slices"
 	"golang.org/x/tools/internal/analysisinternal"
 )
 
 //go:embed doc.go
 var doc string
 
-var (
-	Analyzer = &analysis.Analyzer{
-		Name:     "unusedparams",
-		Doc:      analysisinternal.MustExtractDoc(doc, "unusedparams"),
-		Requires: []*analysis.Analyzer{inspect.Analyzer},
-		Run:      run,
-		URL:      "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedparams",
-	}
-	inspectLits     bool
-	inspectWrappers bool
-)
-
-func init() {
-	Analyzer.Flags.BoolVar(&inspectLits, "lits", true, "inspect function literals")
-	Analyzer.Flags.BoolVar(&inspectWrappers, "wrappers", false, "inspect functions whose body consists of a single return statement")
+var Analyzer = &analysis.Analyzer{
+	Name:     "unusedparams",
+	Doc:      analysisinternal.MustExtractDoc(doc, "unusedparams"),
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+	URL:      "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedparams",
 }
 
-type paramData struct {
-	field  *ast.Field
-	ident  *ast.Ident
-	typObj types.Object
-}
-
-func run(pass *analysis.Pass) (interface{}, error) {
+func run(pass *analysis.Pass) (any, error) {
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	nodeFilter := []ast.Node{
-		(*ast.FuncDecl)(nil),
-	}
-	if inspectLits {
-		nodeFilter = append(nodeFilter, (*ast.FuncLit)(nil))
-	}
 
-	inspect.Preorder(nodeFilter, func(n ast.Node) {
-		var fieldList *ast.FieldList
-		var body *ast.BlockStmt
+	// First find all "address-taken" functions.
+	// We must conservatively assume that their parameters
+	// are all required to conform to some signature.
+	//
+	// A named function is address-taken if it is somewhere
+	// used not in call position:
+	//
+	//	f(...)		// not address-taken
+	//      use(f)          // address-taken
+	//
+	// A literal function is address-taken if it is not
+	// immediately bound to a variable, or if that variable is
+	// used not in call position:
+	//
+	//    f := func() { ... }; f()     			used only in call position
+	//    var f func(); f = func() { ...f()... }; f()     	ditto
+	//    use(func() { ... })				address-taken
+	//
 
-		// Get the fieldList and body from the function node.
-		switch f := n.(type) {
-		case *ast.FuncDecl:
-			fieldList, body = f.Type.Params, f.Body
-			// TODO(golang/go#36602): add better handling for methods, if we enable methods
-			// we will get false positives if a struct is potentially implementing
-			// an interface.
-			if f.Recv != nil {
-				return
-			}
+	// Gather global information:
+	// - uses of functions not in call position
+	// - unexported interface methods
+	// - all referenced variables
 
-			// Ignore functions in _test.go files to reduce false positives.
-			if file := pass.Fset.File(n.Pos()); file != nil && strings.HasSuffix(file.Name(), "_test.go") {
-				return
-			}
-		case *ast.FuncLit:
-			fieldList, body = f.Type.Params, f.Body
+	usesOutsideCall := make(map[types.Object][]*ast.Ident)
+	unexportedIMethodNames := make(map[string]bool)
+	{
+		callPosn := make(map[*ast.Ident]bool) // all idents f appearing in f() calls
+		filter := []ast.Node{
+			(*ast.CallExpr)(nil),
+			(*ast.InterfaceType)(nil),
 		}
-		// If there are no arguments or the function is empty, then return.
-		if fieldList.NumFields() == 0 || body == nil || len(body.List) == 0 {
-			return
-		}
-
-		switch expr := body.List[0].(type) {
-		case *ast.ReturnStmt:
-			if !inspectWrappers {
-				// Ignore functions that only contain a return statement to reduce false positives.
-				return
-			}
-		case *ast.ExprStmt:
-			callExpr, ok := expr.X.(*ast.CallExpr)
-			if !ok || len(body.List) > 1 {
-				break
-			}
-			// Ignore functions that only contain a panic statement to reduce false positives.
-			if fun, ok := callExpr.Fun.(*ast.Ident); ok && fun.Name == "panic" {
-				return
-			}
-		}
-
-		// Get the useful data from each field.
-		params := make(map[string]*paramData)
-		unused := make(map[*paramData]bool)
-		for _, f := range fieldList.List {
-			for _, i := range f.Names {
-				if i.Name == "_" {
-					continue
+		inspect.Preorder(filter, func(n ast.Node) {
+			switch n := n.(type) {
+			case *ast.CallExpr:
+				// Strip off any generic instantiation.
+				fun := n.Fun
+				switch fun_ := fun.(type) {
+				case *ast.IndexExpr:
+					fun = fun_.X // f[T]()  (funcs[i]() is rejected below)
+				case *ast.IndexListExpr:
+					fun = fun_.X // f[K, V]()
 				}
-				params[i.Name] = &paramData{
-					field:  f,
-					ident:  i,
-					typObj: pass.TypesInfo.ObjectOf(i),
-				}
-				unused[params[i.Name]] = true
-			}
-		}
 
-		// Traverse through the body of the function and
-		// check to see which parameters are unused.
-		ast.Inspect(body, func(node ast.Node) bool {
-			n, ok := node.(*ast.Ident)
-			if !ok {
-				return true
+				// Find object:
+				// record non-exported function, method, or func-typed var.
+				var id *ast.Ident
+				switch fun := fun.(type) {
+				case *ast.Ident:
+					id = fun
+				case *ast.SelectorExpr:
+					id = fun.Sel
+				}
+				if id != nil && !id.IsExported() {
+					switch pass.TypesInfo.Uses[id].(type) {
+					case *types.Func, *types.Var:
+						callPosn[id] = true
+					}
+				}
+
+			case *ast.InterfaceType:
+				// Record the set of names of unexported interface methods.
+				// (It would be more precise to record signatures but
+				// generics makes it tricky, and this conservative
+				// heuristic is close enough.)
+				t := pass.TypesInfo.TypeOf(n).(*types.Interface)
+				for i := 0; i < t.NumExplicitMethods(); i++ {
+					m := t.ExplicitMethod(i)
+					if !m.Exported() && m.Name() != "_" {
+						unexportedIMethodNames[m.Name()] = true
+					}
+				}
 			}
-			param, ok := params[n.Name]
-			if !ok {
-				return false
-			}
-			if nObj := pass.TypesInfo.ObjectOf(n); nObj != param.typObj {
-				return false
-			}
-			delete(unused, param)
-			return false
 		})
 
-		// Create the reports for the unused parameters.
-		for u := range unused {
-			start, end := u.field.Pos(), u.field.End()
-			if len(u.field.Names) > 1 {
-				start, end = u.ident.Pos(), u.ident.End()
+		for id, obj := range pass.TypesInfo.Uses {
+			if !callPosn[id] {
+				// This includes "f = func() {...}", which we deal with below.
+				usesOutsideCall[obj] = append(usesOutsideCall[obj], id)
 			}
-			// TODO(golang/go#36602): Add suggested fixes to automatically
-			// remove the unused parameter from every use of this
-			// function.
-			pass.Report(analysis.Diagnostic{
-				Pos:     start,
-				End:     end,
-				Message: fmt.Sprintf("potentially unused parameter: '%s'", u.ident.Name),
-				SuggestedFixes: []analysis.SuggestedFix{{
-					Message: `Replace with "_"`,
-					TextEdits: []analysis.TextEdit{{
-						Pos:     u.ident.Pos(),
-						End:     u.ident.End(),
-						NewText: []byte("_"),
-					}},
-				}},
-			})
 		}
+	}
+
+	// Find all vars (notably parameters) that are used.
+	usedVars := make(map[*types.Var]bool)
+	for _, obj := range pass.TypesInfo.Uses {
+		if v, ok := obj.(*types.Var); ok {
+			if v.IsField() {
+				continue // no point gathering these
+			}
+			usedVars[v] = true
+		}
+	}
+
+	// Check each non-address-taken function's parameters are all used.
+	filter := []ast.Node{
+		(*ast.FuncDecl)(nil),
+		(*ast.FuncLit)(nil),
+	}
+	inspect.WithStack(filter, func(n ast.Node, push bool, stack []ast.Node) bool {
+		// (We always return true so that we visit nested FuncLits.)
+
+		if !push {
+			return true
+		}
+
+		var (
+			fn    types.Object // function symbol (*Func, possibly *Var for a FuncLit)
+			ftype *ast.FuncType
+			body  *ast.BlockStmt
+		)
+		switch n := n.(type) {
+		case *ast.FuncDecl:
+			// We can't analyze non-Go functions.
+			if n.Body == nil {
+				return true
+			}
+
+			// Ignore exported functions and methods: we
+			// must assume they may be address-taken in
+			// another package.
+			if n.Name.IsExported() {
+				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
+			}
+
+			fn = pass.TypesInfo.Defs[n.Name].(*types.Func)
+			ftype, body = n.Type, n.Body
+
+		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 := stack[len(stack)-2].(type) {
+			case *ast.AssignStmt:
+				// f  = func() {...}
+				// f := func() {...}
+				for i, rhs := range parent.Rhs {
+					if rhs == n {
+						if id, ok := parent.Lhs[i].(*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] = slices.Remove(usesOutsideCall[fn], id)
+							}
+
+							if fn == nil && id.Name == "_" {
+								// Edge case: _ = func() {...}
+								// has no var. Fake one.
+								fn = types.NewVar(id.Pos(), pass.Pkg, id.Name, pass.TypesInfo.TypeOf(n))
+							}
+						}
+						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
+						}
+						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
+					}
+				}
+			}
+		}
+
+		// 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()
+					}
+					// This diagnostic carries an edit-based fix to
+					// rename the parameter. Gopls's settings
+					// associate this analyzer with a command-based fix
+					// (RemoveUnusedParam, implemented by
+					// source.RemoveUnusedParameter), so the user
+					// will see a second Code Action offering to
+					// "Remove unused parameter x" (although currently
+					// the fix title is actually that of the diagnostic,
+					// i.e. "unused parameter: x", which is a poor UX.
+					// We should fix that bug in cache.toSourceDiagnostic).
+					pass.Report(analysis.Diagnostic{
+						Pos:     start,
+						End:     end,
+						Message: fmt.Sprintf("unused parameter: %s", id.Name),
+						SuggestedFixes: []analysis.SuggestedFix{{
+							Message: `Rename parameter to "_"`,
+							TextEdits: []analysis.TextEdit{{
+								Pos:     id.Pos(),
+								End:     id.End(),
+								NewText: []byte("_"),
+							}},
+						}},
+					})
+				}
+			}
+		}
+
+		return true
 	})
 	return nil, nil
 }

gopls/internal/lsp/cache/errors.go

@@ -312,6 +312,11 @@ func toSourceDiagnostic(srcAnalyzer *settings.Analyzer, gobDiag *gobDiagnostic)
 		// Accumulate command-based fixes computed on demand by
 		// (logic adjacent to) the analyzer.
 		if srcAnalyzer.Fix != "" {
+			// TODO(adonovan): this causes the fix to have the same title
+			// as the problem it fixes, which is confusing. For example,
+			// the fix for a diagnostic "unused parameter: x" should be
+			// titled "Remove parameter x". (The edit-based fixes above
+			// have sensible names provided by the originating analyzer.)
 			cmd, err := command.NewApplyFixCommand(gobDiag.Message, command.ApplyFixArgs{
 				URI:   gobDiag.Location.URI,
 				Range: gobDiag.Location.Range,

gopls/internal/lsp/source/fix.go

@@ -93,7 +93,7 @@ func ApplyFix(ctx context.Context, fix settings.Fix, snapshot *cache.Snapshot, f
 	return suggestedFixToEdits(ctx, snapshot, fixFset, suggestion)
 }
 
-// suggestedFixToEdits converts the suggestion's edits from analysis form into protocol form,
+// suggestedFixToEdits converts the suggestion's edits from analysis form into protocol form.
 func suggestedFixToEdits(ctx context.Context, snapshot *cache.Snapshot, fset *token.FileSet, suggestion *analysis.SuggestedFix) ([]protocol.TextDocumentEdit, error) {
 	editsPerFile := map[protocol.DocumentURI]*protocol.TextDocumentEdit{}
 	for _, edit := range suggestion.TextEdits {

gopls/internal/server/code_action.go

@@ -602,6 +602,9 @@ func refactorRewrite(snapshot *cache.Snapshot, pkg *cache.Package, pgf *source.P
 // This is true if:
 //   - [start, end) is contained within an unused field or parameter name
 //   - ... of a non-method function declaration.
+//
+// (Note that the unusedparam analyzer also computes this property, but
+// much more precisely, allowing it to report its findings as diagnostics.)
 func canRemoveParameter(pkg *cache.Package, pgf *source.ParsedGoFile, rng protocol.Range) bool {
 	info, err := source.FindParam(pgf, rng)
 	if err != nil {

gopls/internal/settings/api_json.go

@@ -214,7 +214,7 @@ var GeneratedAPIJSON = &APIJSON{
 			{
 				Name: "analyses",
 				Type: "map[string]bool",
-				Doc:  "analyses specify analyses that the user would like to enable or disable.\nA map of the names of analysis passes that should be enabled/disabled.\nA full list of analyzers that gopls uses can be found in\n[analyzers.md](https://github.com/golang/tools/blob/master/gopls/doc/analyzers.md).\n\nExample Usage:\n\n```json5\n...\n\"analyses\": {\n  \"unreachable\": false, // Disable the unreachable analyzer.\n  \"unusedparams\": true  // Enable the unusedparams analyzer.\n}\n...\n```\n",
+				Doc:  "analyses specify analyses that the user would like to enable or disable.\nA map of the names of analysis passes that should be enabled/disabled.\nA full list of analyzers that gopls uses can be found in\n[analyzers.md](https://github.com/golang/tools/blob/master/gopls/doc/analyzers.md).\n\nExample Usage:\n\n```json5\n...\n\"analyses\": {\n  \"unreachable\": false, // Disable the unreachable analyzer.\n  \"unusedvariable\": true  // Enable the unusedvariable analyzer.\n}\n...\n```\n",
 				EnumKeys: EnumKeys{
 					ValueType: "bool",
 					Keys: []EnumKey{
@@ -420,8 +420,8 @@ var GeneratedAPIJSON = &APIJSON{
 						},
 						{
 							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 reduce false positives it ignores:\n- methods\n- parameters that do not have a name or have the name '_' (the blank identifier)\n- functions in test files\n- functions with empty bodies or those with just a return stmt",
-							Default: "false",
+							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.",
+							Default: "true",
 						},
 						{
 							Name:    "\"unusedresult\"",
@@ -1209,9 +1209,10 @@ var GeneratedAPIJSON = &APIJSON{
 			Default: true,
 		},
 		{
-			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 reduce false positives it ignores:\n- methods\n- parameters that do not have a name or have the name '_' (the blank identifier)\n- functions in test files\n- functions with empty bodies or those with just a return stmt",
-			URL:  "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedparams",
+			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.",
+			URL:     "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedparams",
+			Default: true,
 		},
 		{
 			Name:    "unusedresult",

gopls/internal/settings/settings.go

@@ -327,7 +327,7 @@ type DiagnosticOptions struct {
 	// ...
 	// "analyses": {
 	//   "unreachable": false, // Disable the unreachable analyzer.
-	//   "unusedparams": true  // Enable the unusedparams analyzer.
+	//   "unusedvariable": true  // Enable the unusedvariable analyzer.
 	// }
 	// ...
 	// ```
@@ -828,9 +828,6 @@ func (o *Options) enableAllExperimentMaps() {
 	if _, ok := o.Codelenses[string(command.RunGovulncheck)]; !ok {
 		o.Codelenses[string(command.RunGovulncheck)] = true
 	}
-	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
 	}
@@ -1475,7 +1472,7 @@ func defaultAnalyzers() map[string]*Analyzer {
 		shadow.Analyzer.Name:           {Analyzer: shadow.Analyzer, Enabled: false},
 		sortslice.Analyzer.Name:        {Analyzer: sortslice.Analyzer, Enabled: true},
 		testinggoroutine.Analyzer.Name: {Analyzer: testinggoroutine.Analyzer, Enabled: true},
-		unusedparams.Analyzer.Name:     {Analyzer: unusedparams.Analyzer, Enabled: false},
+		unusedparams.Analyzer.Name:     {Analyzer: unusedparams.Analyzer, Enabled: true},
 		unusedwrite.Analyzer.Name:      {Analyzer: unusedwrite.Analyzer, Enabled: false},
 		useany.Analyzer.Name:           {Analyzer: useany.Analyzer, Enabled: false},
 		timeformat.Analyzer.Name:       {Analyzer: timeformat.Analyzer, Enabled: true},

gopls/internal/test/marker/testdata/codeaction/removeparam.txt

@@ -203,7 +203,7 @@ func _() {
 -- effects/effects.go --
 package effects
 
-func effects(x, y int) int { //@codeaction("y", "y", "refactor.rewrite", effects)
+func effects(x, y int) int { //@ diag("y", re"unused"), codeaction("y", "y", "refactor.rewrite", effects)
 	return x
 }
 
@@ -217,7 +217,7 @@ func _() {
 -- @effects/effects/effects.go --
 package effects
 
-func effects(x int) int { //@codeaction("y", "y", "refactor.rewrite", effects)
+func effects(x int) int { //@ diag("y", re"unused"), codeaction("y", "y", "refactor.rewrite", effects)
 	return x
 }
 

gopls/internal/test/marker/testdata/codelens/test.txt

@@ -22,7 +22,8 @@ func TestMain(m *testing.M) {} // no code lens for TestMain
 func TestFuncWithCodeLens(t *testing.T) { //@codelens(re"()func", "run test")
 }
 
-func thisShouldNotHaveACodeLens(t *testing.T) {
+func thisShouldNotHaveACodeLens(t *testing.T) { //@diag("t ", re"unused parameter")
+	println() // nonempty body => "unused parameter"
 }
 
 func BenchmarkFuncWithCodeLens(b *testing.B) { //@codelens(re"()func", "run benchmark")

gopls/internal/test/marker/testdata/rename/issue42134.txt

@@ -1,3 +1,5 @@
+Regression test for #42134,
+"rename fails to update doc comment for local variable of function type"
 
 -- 1.go --
 package issue42134
@@ -29,7 +31,7 @@ func _() {
 	fmt.Println(minNumber) //@rename("minNumber", "res", minNumberTores)
 }
 
-func min(a, b int) int { return a }
+func min(a, b int) int { return a + b }
 -- @minNumberTores/2.go --
 @@ -6 +6 @@
 -	// minNumber is a min number.

gopls/internal/util/slices/slices.go

@@ -65,3 +65,18 @@ func Grow[S ~[]E, E any](s S, n int) S {
 	}
 	return s
 }
+
+// Remove removes all values equal to elem from slice.
+//
+// The closest equivalent in the standard slices package is:
+//
+//	DeleteFunc(func(x T) bool { return x == elem })
+func Remove[T comparable](slice []T, elem T) []T {
+	out := slice[:0]
+	for _, v := range slice {
+		if v != elem {
+			out = append(out, v)
+		}
+	}
+	return out
+}