Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proposal: Go 2: error on unused function parameters #39118

Closed
Denton-L opened this issue May 17, 2020 · 14 comments
Closed

proposal: Go 2: error on unused function parameters #39118

Denton-L opened this issue May 17, 2020 · 14 comments
Labels
FrozenDueToAge LanguageChange Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Milestone
Proposal

Comments

@Denton-L
Copy link

Currently, if there are any unused local variables in a function, it will fail to compile with something like x declared but not used. However, the same is not true for any unused function parameters. The following is perfectly valid Go code:

package main

func f(f int) {}

func main() {
	f(0)
}

Proposal

Disallow unused parameters in functions. In case an unused variable is desired, it will have to be written as _, just like other unused variables. f() in the above would be rewritten as

func f(_ int) {}

Discussion

Although failing to use a parameter is not always a bug, in the case where it's unintentionally done, it is just as bad as failing to use a local variable.

There is one case where an unused parameter is not a bug. This is when a function is written to satisfy an interface and one of the parameters is actually not needed. It would be better to force programmers to explicitly mark the parameter as unused via _ so that they are consciously choosing not to use it.

One argument against this is that the parameter's name, even if unused, serves as implicit documentation as to the purpose of the parameter. However, I don't think that this is necessarily true. If a developer were using the function as part of an interface, the interface would've documented the purpose of the parameter via its name, otherwise it wasn't important anyway.

If a developer were using the function directly on a concrete type, then they would not need to know the purpose of the unused variable at all since it's, well, unused. Explicitly marking this with _ would allow the user to clearly see this from the function signature. Thus, it would alleviate any (albeit small) cognitive overhead about deciding what arg to pass and just specify a zero-value.
@gopherbot gopherbot added this to the Proposal milestone May 17, 2020
@seankhliao
Copy link
Member

see also #7892

@mvdan
Copy link
Member

mvdan commented May 18, 2020

I agree with previous comments that this would break far too much existing code.

Having said that, I agree that there is value in finding truly unused parameters. You can take a look at https://github.com/mvdan/unparam, which attempts the static analysis route.

@ianlancetaylor
Copy link
Contributor

Yeah, maybe this would have been a good idea early on, but at present it seems impossible. Even if we provide conversion tools, we can't realistically require thousands of Go programs to be changed. Or, to put it another way, if we are going to require thousands of Go programs to be changed, I think we need a much bigger benefit than this will give us.

@robpike
Copy link
Contributor

robpike commented May 19, 2020

When there is only one argument, you don't need to write

f(_ int)

You could just write

f(int)

I agree that the benefit is too small for the work involved now. One way to narrow the focus might be to add a vet test that reports, only when we know all the call sites, that a function parameter is unused. That might catch a few things at the cost of a dance being required when the function is being developed, analogous to assigning variables to _ when code is under development.

Even with that restriction, it's close to harmless. It's my feeling that a value that returns from a function matters much more than a variable being passed to one, as ignoring the former has been shown to adumbrate many bugs, but there is no evidence I've seen for an unused function parameter mattering much at all.

@rsc rsc added the v2 A language change or incompatible library change label Oct 29, 2020
@KZiemian
Copy link

KZiemian commented Jan 31, 2022 •
edited

I agree that pain in this situation isn't worth a gain. Fortunately Go is successful and this means there is a lot code written in Go. And cost of barking changes is the language probably n^2 in number of projects written in it.

At the same time I will write why I want to see this feature in Go 2. Even if I have little hope of Go 2 becoming reality. As Go FAQ says If it's worth complaining about, it's worth fixing in the code. And if it's not worth fixing, it's not worth mentioning. Unused variable is worth mentioning, so it is worth of fixing. For me unused parameter of the function is worth mentioning, so it is worth fixing, no less than unused variable.

This is one of the places that for me are not wrong per say, but they feel inconsistency with Go philosophy. But, this may be only my feeling.

@ianlancetaylor
Copy link
Contributor

We aren't going to be able to make this change. And we don't plan to release a breaking Go 2 in the foreseeable future. Therefore, this is a likely decline. Leaving open for four weeks for final comments.

@griesemer griesemer mentioned this issue Mar 2, 2022
Open
@Olshansk
Copy link

@ianlancetaylor I just came by this thread looking for the same functionality.

I understand the reasoning behind not implementing this directly, but I was wondering if you (or someone) has context into how difficult something like this would be to implement in staticcheck.io?

I haven't spent any time looking into this yet, but simply looking for a pointer (or someone telling me it's a bad idea) before I spend time on it.

@ianlancetaylor
Copy link
Contributor

I think it would be easy to implement in a static analyzer. (But it may still be a bad idea.)

@KZiemian
Copy link

Do you mean that implementing func(_ int) int as possible signature for function with unused argument can be a bad idea?

@ianlancetaylor
Copy link
Contributor

No, I mean that requiring programmers to always use _ if the parameter is unused may be a bad idea. There are many programming styles where that is not appropriate. But of course there are other programming styles where it is appropriate. It's up to you to decide what to do for your code. It's not a style that will be adopted in, for example, the Go standard library.

@Olshansk
Copy link

I think discussing whether it's a good or bad idea could go down a pretty big rabbit hole.

However, is there any opposition to having sta take a flag that says Produce warnings for unused function parameters that are not anonymous in the method header.

When time permits, I'll take a bit of a dive to see if the existing compiler/linter can be leveraged to make this a simple change, or if the scope is larger than I'd expect. If anyone has a pointer for a starting point, I'd appreciate it!

@ianlancetaylor
Copy link
Contributor

No change in consensus.

@Olshansk
Copy link

Olshansk commented Apr 6, 2022

Just wanted to follow up to see if any core go developers could at least point in the right direction of where these static checks take place.

@ianlancetaylor
Copy link
Contributor

There are a number of Go static analyzers. One of the most popular is https://staticcheck.io/.

@jmackie jmackie mentioned this issue Apr 29, 2022
Merged
@golang golang locked and limited conversation to collaborators Apr 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge LanguageChange Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
9 participants
@rsc @Olshansk @ianlancetaylor @mvdan @robpike @gopherbot @Denton-L @seankhliao @KZiemian