vet: point out unused function arguments

[gopherbot]

by tobias.schwab@dynport.de:

When applying the "extract method pattern" I quite often forget to actually
use extracted arguments and keep the hardcoded values in the the extracted function
which leads to errors.

I think that for not exported functions at least vet should complain about unused
function arguments. My personal opinion is that I would treat unused arguments of not
exported functions the same as unused variables (and cause a compiler error).

For exported functions things are a bit different because of API stability reasons. A
similar workaround to handling unused variables ("_ = unusedArgument" at the
beginning of the function) would be possible though and also enable documenting (via
comment) why a specific arguments is not used.

[cznic]

Comment 1:

""""
My personal opinion is that I would treat unused arguments of not exported functions the
same as unused variables (and cause a compiler error).
""""
Methods implementing an interface it's completely ok that some implementations of a
particular interface are legitimately never using some of the arguments. Consider
type Fooer interface {
        Foo()
        CloseTempFile(AndRemoveIt bool)
}
An all memory implementation of Fooer has no use of the AndRemoveIt argument.
Now consider that also plain functions can be used in the same, or almost the same way
as interface implementing types. Either passed as a callback or converted to an
interface type (like http.HandlerFunc) and then again, unused arguments might be
perfectly legitimate.
IOW, making unused args, even when only for exported function, a compile error is IMO
not an option.

[griesemer]

Comment 2:

We are not making language changes; especially not changes that would invalidate
existing code.
Treating this as a request for potential future vet functionality; changed title
accordingly. Handing over to r.

Labels changed: added repo-tools.

Owner changed to @robpike.

[robpike]

Comment 3:

The issue here is eliminating false positives. The request is fine but if vet complains
too often incorrectly, the check is not valuable.
If it is a method, whether internal or exported, it might satisfy some interface and
there's no practical way to know which one because interfaces are not bound to the
package.
func (t or T) Read(p buf) (int, error) { return 0, io.EOF }
satisfies io.Reader, which means the parameter cannot be removed. Plus it might make
perfect sense to document that p is unused. Thus complaining about unused parameters to
methods will generate too many false positives.
The check cannot be made to work reliably for methods. What about functions?
If it is an exported function, there could be many reasons that the parameter exists but
is unused; for instance, it might be historical but now irrelevant. Again, too many
false positives.
If it is an internal function, we could provide this test usefully, I believe, but there
are very few such things, on average, plus the question explicitly asked about methods,
not functions.
We therefore conclude that vet cannot provide a low-enough noise signal for enough cases
to make this worthwhile.
Marking this WontFix.

Status changed to WontFix.