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 <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

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,32 +24,64 @@ 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
 }
 
 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,32 +24,64 @@ 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
 }
 
 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
 }