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

cmd/vet: Should warn if not all function arguments are used #17748

Closed
k3a opened this issue Nov 2, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@k3a
Copy link

commented Nov 2, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.7.3 darwin/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOROOT="/usr/local/Cellar/go/1.7.3/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7.3/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/jz/d17tbxmd46sblngzv_ncs1000000z8/T/go-build885041694=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

func (c Context) QueryParam(key string) string {
	return c.Request.URL.Query().Get("token")
}

What did you expect to see?

govet complaining that 'key' argument is not used.

What did you see instead?

no complains at all :)

@ALTree

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

See #7892

@k3a

This comment has been minimized.

Copy link
Author

commented Nov 2, 2016

Ah, haven't noticed it has been discussed already. I still think it would be a very useful feature (java and C# reports that as a warning) but I understand the problem with implementing some interface methods partly and getting way too much useless reports.

What about having a rulefile for specifying what should be reported and what not? Like tslint does have for example https://palantir.github.io/tslint/rules/

Or some way to silence the false positives like: _ = key ?

@ALTree

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

What about having a rulefile for specifying what should be reported and what not? Like tslint does have for example https://palantir.github.io/tslint/rules/

I believe that traditionally rules that do not satisfy the three vet criteria (Correctness, Frequency, Precision) have been outright rejected, instead of being added but as an option as you suggest. Vet is meant to be a reliable tool that requires no configuration.

Or some way to silence the false positives like: _ = key ?

"Few false positives" means "few false positives", and not "false positives are okay as long as they can be silenced in some way". Almost every false positive can be silenced with the addition of some ugly code; I don't think this is desirable.

Those are just my personal opinions; but given that this was rejected in the past I'm not sure it will be reconsidered now.

@k3a

This comment has been minimized.

Copy link
Author

commented Nov 2, 2016

It's a matter of opinion. One could say we have arguments in functions because we need them in the internal function algorithm and say, that declaring function argument and not using it is a bad practice (like importing a package and not referencing it).

I understand that one may occasionally need to have unused arguments to maintain backward API compatibility, but in those cases they can explicitly reference them by "_ = ..." like we do for unused imports.

govet could have saved me from making this mistake, unfortunately it only helped me once, when I was learning go and wrote a structure tag wrongly.

It would be helpful to know the opinion of more contributors.

@k3a k3a changed the title cmd/vet: Should warn if not all supplied function arguments are used cmd/vet: Should warn if not all function arguments are used Nov 2, 2016

@josharian

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

Rob decided against this in #7892, and he is the ultimate arbiter of what goes into cmd/vet, so I going to close this.

I agree with this particular decision; I don't see a clear way to address the many false positives such a check would generate.

@josharian josharian closed this Nov 3, 2016

@golang golang locked and limited conversation to collaborators Nov 3, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.