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/compile: mention shadowing of predeclared identifiers in errors they cause #33567

Open
seebs opened this issue Aug 9, 2019 · 7 comments
Open
Labels
Milestone

Comments

@seebs
Copy link
Contributor

@seebs seebs commented Aug 9, 2019

(Related: #31064, #14494)

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

1.12, but N/A

Does this issue reproduce with the latest release?

I believe so?

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

N/A

What did you do?

Horrible monstrous things. Examples:

const iota = 0
type int struct{}
var nil = "haha"

Okay, fine, those are stupid. But you know what I've done unintentionally?

...
len := a -b
...
for i := 0; i < len(x); i++ {
...

and then get confused by the weird error message about len not being a function.

What did you expect to see?

I think redeclaring predeclared identifiers should be an error. Or, since that's probably impractical, it should be a thing in go vet.

What did you see instead?

Really weird and misleading error messages when I forget that something is a predeclared identifier. I've hit this most often with len, because that's a natural variable name and when I'm naming something that I'm usually not thinking about the function.

@gopherbot gopherbot added this to the Proposal milestone Aug 9, 2019
@gopherbot gopherbot added the Proposal label Aug 9, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Aug 9, 2019

See also #31064 and #21606.

@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Aug 9, 2019

This particular case of shadowing is no different in kind from the general issue. We should be able to close this in deference to the ones linked above.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 13, 2019

cmd/vet/README lists the requirements for a vet check.

The other two issues seem like clear declines to me on correctness/precision. They reject far too much good Go code. This proposal is different. It is unlikely to reject good Go code but seems unlikely to catch many bugs either, for the simple reason that hardly any Go programmers reuse predeclared identifiers. So it seems like this one fails on frequency.

There's still some correctness/precision argument too. Go has some obscure identifiers that are fine to reuse in your program, especially as a local variable. It's easy to see writing identifiers like imag or close or cap as a local variable in a short function and that's completely fine. It would be unfortunate for vet to reject that code.

Predeclared identifiers are not reserved words in Go precisely so that the set can expand without invalidating old programs and so that it can have some obscure entries without forcing developers to learn a list of "bad words" to avoid. Adding a vet check would undo much of that flexibility.

@seebs

This comment has been minimized.

Copy link
Contributor Author

@seebs seebs commented Nov 13, 2019

Empirically, usually if I unintentiontally shadow len, I find out very quickly, but the errors can be very confusing, because the real problem is "you shadowed a predeclared identifier and then tried to use the predeclared identifier without noticing this", and the diagnostic is always unrelated.

I wonder if it'd be sane/practical to track whether a given symbol shadows another symbol, and if so, and it's used in a diagnostic, point that out. That might be a saner approach that won't break real code, and would almost certainly be of use to developers who have been awake a bit too long.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 20, 2019

The other problem with trying to use vet to catch the error in the example at the top of this issue is that vet only works on code that compiles, and that code does not.

For this specific case it seems like cmd/compile and go/types should give a more helpful error message when the problem seems to be caused by shadowing. Will retitle.

@rsc rsc changed the title proposal: vet should catch shadowing of predeclared identifiers cmd/compile: mention shadowing of predeclared identifiers in errors they cause Nov 20, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 20, 2019

As a compiler error this no longer needs to be a proposal.

@rsc rsc added NeedsFix and removed Proposal labels Nov 20, 2019
@rsc rsc modified the milestones: Proposal, Go1.15 Nov 20, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 20, 2019

(Probably should happens in go/types too so it shows up in gopls etc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.