cmd/vet: should require unpacking both range variables in a for loop #21250

doriable · 2017-08-01T01:51:27Z

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:

The value type is the same as the index.

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

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.

The text was updated successfully, but these errors were encountered:

ianlancetaylor · 2017-08-01T02:38:46Z

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

mvdan · 2017-08-01T07:51:52Z

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

doriable · 2017-08-03T02:51:26Z

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 · 2017-10-31T17:08:28Z

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.

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

rsc closed this as completed Oct 31, 2017

golang locked and limited conversation to collaborators Oct 31, 2018

gopherbot added the FrozenDueToAge label Oct 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/vet: should require unpacking both range variables in a for loop #21250

cmd/vet: should require unpacking both range variables in a for loop #21250

doriable commented Aug 1, 2017

ianlancetaylor commented Aug 1, 2017

mvdan commented Aug 1, 2017

doriable commented Aug 3, 2017 •

edited

rsc commented Oct 31, 2017

Navigation Menu

cmd/vet: should require unpacking both range variables in a for loop #21250

cmd/vet: should require unpacking both range variables in a for loop #21250

Comments

doriable commented Aug 1, 2017

Problem

Proposed Solution

ianlancetaylor commented Aug 1, 2017

mvdan commented Aug 1, 2017

doriable commented Aug 3, 2017 • edited

rsc commented Oct 31, 2017

doriable commented Aug 3, 2017 •

edited