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 require unpacking both range variables in a for loop #21250

Closed
doriable opened this issue Aug 1, 2017 · 4 comments

Comments

Projects
None yet
5 participants
@doriable
Copy link

commented Aug 1, 2017

Problem

When iterating over a slice or a map using range, the programmer has the option of unpacking only the index, or both index and the value. In cases where only the value is needed, it is easy to forget to also unpack the index into a throwaway variable. In particular, there are two cases where the compiler cannot catch this mistake:

  1. The value type is the same as the index.
sum := 0
for n := range []int{5, 6} {
	sum += n
}
  1. The value type is different from the index type, but is used in a type-ambiguous context such as an empty interface.
func work(v interface{}) {
	...
}

for s := range []string{"a"} {
	work(s)
}

Proposed Solution

We are proposing that go vet require the unpacking of both values so the programmer must be explicit when using range. This will make mistakes like the above examples more obvious, for example:

sum := 0
for n, _ := []int{5, 6} {
	sum += n
}

Now it is clear that n is the index.

@ianlancetaylor ianlancetaylor changed the title go vet should require unpacking both range variables in a for loop cmd/vet: should require unpacking both range variables in a for loop Aug 1, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2017

I think that would issue a warning on too much existing, correct, code. See cmd/vet/README for the criteria for vet checks.

@mvdan

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

I agree with @ianlancetaylor. Perhaps you should propose this as a Go2 language change.

@doriable

This comment has been minimized.

Copy link
Author

commented Aug 3, 2017

That's fair, and I appreciate the feedback, thank you! I was wondering if the following scenario is worth discussing:
In the case of:

nums := []int{5, 6}
for n := range nums {
    // ...
}

whether or not n is being used as an index (e.g. nums[n]) in the loop.

I'm unsure if this should be something that is checked by the compiler for Go2 or if this would fulfill the correctness/frequency/precision criteria for go vet. However, I do think this can be a very difficult bug to detect without some tooling and Go programmers would benefit from this situation being flagged.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2017

Correct code and incorrect code are basically indistinguishable in this case. The way to find this bug is to run the code and have a test, not to make vet guess which is meant.

@rsc rsc closed this Oct 31, 2017

@golang golang locked and limited conversation to collaborators Oct 31, 2018

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.