x/tools/gopls/internal/analysis/unusedparams: "no such param" error when applying fix

[adonovan]

gopls' unusedparams analyzer is spuriously reporting a diagnostic for an unused parameter in a func literal even when the func value is address taken, in this case by being assigned to a global variable of a particular func type:

var g func(x, y int)

func _() {
	g = func(x, y int) { // unused parameter: x
		print(y)
	}
}

The existing logic is supposed to recognize when a func lit is assigned to a func-typed var whose type it must conform to, but clearly it has a bug.

[adonovan]

Ah: aside from the assignment, all uses of var g (of which there are none) are in call position, so the function literal is considered not address-taken. (Adding a call g(1, 2) doesn't change this, but adding print(g) does.) I suppose this is consistent: the parameter could indeed be removed, but the change to do that may be more complex.

The problem is not due to g being a package-level variable; the same problem occurs with a local variable too:

func _() {
	var g func(x, y int)
	g = func(x, y int) { // unused parameter: x
		print(y)
	}
	g(1, 2)
}

In both cases, applying the fix fails with an error ("no param found"). I wondered if the problem was that removing the parameter x would require the type of var g to change, which could be a tricky edit. But even in the case where g is declared from the func lit using := like so, the quick fix fails:

func _() {
	g := func(x, y int) { // unused parameter: x
		print(y)
	}
	g(1, 2)
}

So I think the diagnostic is working as intended, but there are bugs in the fix.

[adonovan]

@findleyr What is the expected behavior of golang.removeParam in this case? Is it the responsibility of the caller (unusedparam, in effect) to ensure that the removal won't break existing assignments?

[adonovan]

What is the expected behavior of golang.removeParam in this case? Is it the responsibility of the caller (unusedparam, in effect) to ensure that the removal won't break existing assignments?

Answered offline: it's the caller's responsibility to ensure that removing the parameter and corresponding call arguments is safe; renameCall does not handle other (non-call) uses of the function.

[adonovan]

There's a fair amount of complexity in here:

• First unusedparam needs to distinguish between var f = func... and f := func..., which do not impose a constraint that the function literal conforms to the type of the var, from var f T = func... and f = func... which do (or may do).
• Second, it's not possible to write tests of the "remove param" fix in the unusedparam package itself, because this logic is part of the gopls ApplyFix command. But that uses a quite different code path from the unusedparam analyzer to offer the refactor.rewrite.removeUnusedParam code action based on the current selection, which must be in a FuncDecl (whereas unusedparam works with FuncLit too). So even if we fixed the problem in unusedparam, there's really no way to test it.

Needs some deeper de-muddling. Pushing to 22.