x/tools/gopls: unusedparams should ignore unused params prefixed with an underscore

[vanackere]

gopls version

golang.org/x/tools/gopls v0.10.1
    golang.org/x/tools/gopls@v0.10.1 h1:JoHe17pdZ8Vsa24/GUO8iTVTKPh0EOBiWpPop7XJybI=

What did you do?

unusedparams correctly warns about unused parameters, but the only way to remove the warning appears to either remove the parameter name completely, or replace it with an underscore.

What did you expect to see?

In a lot of cases the parameter name has a meaning and is useful for code maintenance (let's say for example a boolean parameter that is necessary to implement some interface but unused in the particular implementation case), it would be much more userfriendly to have parameters prefixed by an underscore also ignored by unusedparams (either by default or with a config option).
Conversely, if this were to be implemented, it would be nice to have unusedparams emit a warning if a parameter prefixed by an underscore is actually used within the function ;-)

[someview]

Not only so, ignore this param when complie method:

// ignore the input
func SayHello(_ int) {
}

[findleyr]

Hi, thanks for the issue. However, I don't think we should make this change.

Using a parameter name _ is a clear signal that the parameter is not meant to be used (as it cannot be used). However, _foo is a perfectly valid parameter name, and as a general rule I don't think analyzers should be adding their own conventions like this.

Put differently: while this convention may be convenient in your code base, it might suppress diagnostics in another code base where the developer is not aware of this convention. Putting this burden of awareness on all users is not worth the marginal benefit it provides.

I think comments are a cleaner way to add documentation.

So we can't do this, sorry. However, I agree that the unusedparam analyzer is too noisy. I personally don't use it regularly -- instead I just run it every once in a while to inspect its results. That's a UX problem that we need to solve, but I don't think this is a viable solution.

[vanackere]

Hi,

I respectfully disagree with your analysis, prefixing unused parameters with an underscore in a very common way of doing things in the js / typescript world for example, and this convention was also adopted by other go linters previously so I don't think this is a strange idea at all, nor that it would surprise people (and also please prove me wrong but having read a lot of external go code, I think a code base using parameters names prefixed by an underscore is really uncommon).

As I said, forcing users to replace all unused parameters names by a single _ actually makes the code worse to read an maintain, and that's a thing I'm concerned with, I've already had a developers removing parameter names from our code base to make the warning disappear, but the code readability was actually hurt by those changes, since the unused parameters were actually necessary to implement external interfaces and the name are actually useful...

I still think this should be the default, but at least I'd like to make the behaviour configurable. Could you please reopen this issue ?

[findleyr]

I want to emphasize that it is not sufficient for this change to the analyzer to be useful. It must be so useful that it is worthwhile incurring additional cognitive overhead on all users of this analyzer, to understand this new convention. In this case, I think the most idiomatic way of documenting blank-named parameters is with a comment (e.g. "// Implements the Foo interface"), and so I don't think this new convention will be immediately obvious to users.

prefixing unused parameters with an underscore in a very common way of doing things in the js / typescript world for example

Again, even if it were common in Go, that wouldn't necessarily justify coupling that convention with the behavior of an analyzer. But as you say, prefixing names with parameters is uncommon in Go. Having an analyzer convention that is inconsistent with idiomatic Go code is something we should avoid.

As I said, forcing users to replace all unused parameters names by a single _ actually makes the code worse to read an maintain, and that's a thing I'm concerned with, I've already had a developers removing parameter names from our code base to make the warning disappear, but the code readability was actually hurt by those changes, since the unused parameters were actually necessary to implement external interfaces and the name are actually useful...

This is the reason why the unusedparams analyzer is not enabled by default. I think it has too many false positives to ever be enabled by default, and we should be pursuing more robust forms of dead-code analysis, or a better UX for presenting unused parameters.

I fully agree that there is a problem with this analyzer, but I don't agree that new convention/configuration is the solution.

I still think this should be the default, but at least I'd like to make the behaviour configurable. Could you please reopen this issue ?

I can definitely reopen the issue for further discussion. However, I will caveat that we are unlikely to accept this change, even as configuration. Sorry, but we have to be defensive against adding new complexity and new configuration. In fact, quite the opposite: we are on a mission to reduce our configuration surface area.

To me, this discussion is a strong argument in favor of #59869.

CC @adonovan @timothy-king, who help maintain our static analysis tools.

[vanackere]

Indeed I was not aware of #59869 but it would be very nice to have configurable analyzers specific for each code base.

Regarding the unusedparams analyser, you said :

I think it has too many false positives to ever be enabled by default

Well if your diagnostic is correct and the intent is to keep the current behaviour with no configuration possible then I's suggest to simply delete it from gopls... As of why it is enabled by default in my VSCode, I have no idea, maybe because I'm using the nightly version of gopls ?

Finally I'd had a quick look and the documentation at the start of gopls/internal/lsp/analysis/unusedparams/unusedparams.go states :

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 are underscored
- functions in test files
- functions with empty bodies or those with just a return stmt

For me - but I may be wrong since I'm not a native english speaker - underscored means "starts with an underscore" and it would mean that the current behavior is indeed an oversight / bug that I'd be glad to fix / send a PR (this is just a one-line change for the simple fix, and not much harder to ensure underscore-prefixed parameters are not used)

[timothy-king]

I may be wrong since I'm not a native english speaker - underscored means "starts with an underscore"

Well I am a native english speaker, and I would agree with the interpretation that "underscored" does include _foo in context. The implementation is actually checking if i.Name == "_", which has a special meaning in go (https://go.dev/ref/spec#Blank_identifier). So I think the documentation should be fixed to reflect the implementation. Something like parameters that do not have a name or has the name '_' (blank identifier).

"// Implements the Foo interface"

Another option is give the method name too in the comment "// Implements the Fooer.Bar method." if you feel like replacing the parameter name with '_' makes a method signature harder to read. Slightly redundant but IMO points more directly at why the signature includes an unused parameter.

[findleyr]

Thanks, I agree that this documentation is incorrect. I'll put this into the gopls@v0.13.0 milestone to make a decision about the analyzer behavior.

This is an example where we need a framework for making decisions about configuration, so I filed #61001 to discuss such a framework.

[gopherbot]

Change https://go.dev/cl/524835 mentions this issue: gopls/internal/lsp/analysis/unusedparams: document the blank identifier