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

proposal: cmd/vet: warn about builtin assignment #42851

Closed
mengzhuo opened this issue Nov 27, 2020 · 4 comments
Closed

proposal: cmd/vet: warn about builtin assignment #42851

mengzhuo opened this issue Nov 27, 2020 · 4 comments
Labels
Milestone

Comments

@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Nov 27, 2020

This is a proposal that adding check on builtin assignment to vet.

For example:

var true = false
var nil = false
var false = 1

All are valid by the compiler and vet now.
We might warn about these kind of overwritten for clarification.

Go itself has ~200 overwritten

 grep -r --include "*.go" -E '\s(append|make|new|nil|string|copy|delete|len|cap|complex|imag|close|panic|recover|print|println|true|false|nil|bool) :?=' .  > ~/builtin_overwritten.log

wc -l ~/builtin_overwritten.log
281

builtin_overwritten.log

@gopherbot gopherbot added this to the Proposal milestone Nov 27, 2020
@gopherbot gopherbot added the Proposal label Nov 27, 2020
@davecheney
Copy link
Contributor

@davecheney davecheney commented Nov 27, 2020

What is the harm in using len or new as variables? If they are redefined they won't operate, in the current scope, as builtins so there should be little cause for concern that they are accidentally misused.

@mengzhuo
Copy link
Contributor Author

@mengzhuo mengzhuo commented Nov 27, 2020

What is the harm in using len or new as variables? If they are redefined they won't operate, in the current scope, as builtins so there should be little cause for concern that they are accidentally misused.

Yes, you are right about there is no harm.
However it makes code more clear that len,new,etc... are builtins instead of temporary varibles.
Just like vet checking argument types and numbers for fmt.Printf().

@martisch
Copy link
Contributor

@martisch martisch commented Nov 27, 2020

They are identifiers. Identifier shadowing is also allowed for other identifiers in Go.

However it makes code more clear that len,new,etc... are builtins instead of temporary varibles.

Vet is not a code style enforcer: "Vet's checks are about correctness, not style."
https://github.com/golang/go/tree/master/src/cmd/vet

As far as I understand the vet argument checking in fmt actually covers code that is considered an error by fmt. Reusing identifiers is not an error as shown by the standard library.

I think its fine if developers opt in to disallow shadowing some identifiers in style checkers on their codebase. Making it a defacto enforced style in Go seems a big hammer. If that path is taken of disallowing to reuse those identifiers (kind of a language change in disguise) it might just be better to make them compile errors. (I am not in favor of disallowing shadowing of builtin identifiers.)

@mengzhuo
Copy link
Contributor Author

@mengzhuo mengzhuo commented Nov 27, 2020

Fair point, let linter do their job since shadowing is not an error.

@mengzhuo mengzhuo closed this Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.