Disable the linter if loop variable is used after the loop

[qdm12]

If you have a loop where you use the loop variable after the loop, for example:

var i int
for i = 0; i < 1; i++ {
  fmt.Println(i) // prints 0
}

// Then use the value of i after the loop
fmt.Println(i) // prints 1

If you replace it with an intrange without too much thought, you get

var i int
for i = range 1 {
  fmt.Println(i) // prints 0
}

// Then use the value of i after the loop
fmt.Println(i) // prints 0

This got caught in one of my tests, and it really does worry me I would miss such non obvious cases.
This is rather worrisome and prone to human error, it is critical to implement a disable rule in your linter, if the linter finds the loop variable is used after the for loop notably.

[ckaznocha]

Hi @qdm12,

Thanks for the report! This is an interesting one. I think it's not recommended to react to the feedback from any linter without too much thought! 🤪

I'm going to update your examples to use values other than 1 because I think using 1 kind of masks an off by one issue.

var i int
for i = 0; i < 3; i++ {
	fmt.Println(i) // prints 0, 1, 2
}

// Then use the value of i after the loop
fmt.Println(i) // prints 3

var i int
for i = range 3 {
	fmt.Println(i) // prints 0, 1, 2
}

// Then use the value of i after the loop
fmt.Println(i) // prints 2

Now we can see that using the range loop in this style does update i as expected, its just off by one because the first example causes a final post increment on the last iteration.

I think the linter is correct that this code can be updated even though the original code is kind of odd. It just can't be updated without a bit thought 🤔

For example, it could be updated to add a final post increment after the loop to replicate the original logic:

var i int
for i = range 3 {
	fmt.Println(i) // prints 0, 1, 2
}

i++

// Then use the value of i after the loop
fmt.Println(i) // prints 3

Or better yet, since you know what the final value of i will be before even going into the loop, just use that:

n := 3
for i := range n {
	fmt.Println(i) // prints 0, 1, 2
}

// Then use the value of n after the loop
fmt.Println(n) // prints 3

Go Playground with all examples

This would be a dangerous thing to autofix but since the tool doesn't autofix these I don't think I see an issue yet.

There is some discussion about user defined exclusions in #31. Unless I'm missing something here, I'll probably close this one in favor of that one.

[qdm12]

I think it's not recommended to react to the feedback from any linter without too much thought

Definitely, but that was really an non obvious case to think about. I really thought intrange would behave the same, especially given the linter suggesting the replacement.

I think the linter is correct that this code can be updated even though the original code is kind of odd.

It's not that odd, in that case I was keeping track of the number of tries and there is an if check after the loop checking if we reached the max number of tries i == n.

The approaches you mention to workaround it don't work if you have if+break within the loop, you would need to check i == n - 1 instead of i == n after the loop, I think that's the only fix.

This would be a dangerous thing to autofix but since the tool doesn't autofix these I don't think I see an issue yet.

Ehh I'm pretty sure I wouldn't be the only one wanting to migrate to intrange without realizing this intrange tiny change of behavior. I wouldn't have a unit test covering the code, this could have catastrophic consequences and is also tedious to debug. My point is, even knowing the problem now, I still think it's easy to get bitten by this especially since your linter suggests the replacement.

Again, a fix would be to NOT suggest using intrange for code using the loop variable AFTER the loop (rare-ish cases like mine). Or at the very least suggest it but with a different message pointing out the last value of the loop variable won't be the 'max'.