x/tools/gopls/internal/analysis/modernize: rangeint: transformation unsound when loop body has effects on iterated array

[jba]

Go version

x/tools/gopls v0.18.1

Output of go env in your module/workspace:

irrelevant

What did you do?

This program prints 1, 2, 3 4:

func main() {
	s := []int{1, 2, 3}
	for i := 0; i < len(s); i++ {
		fmt.Println(s[i])
		if s[i] == 2 {
			s = append(s, 4)
		}
	}
}

What did you see happen?

Gopls wants to modernize the for loop to for i := range len(s).
Since the range expression is evaluated only once instead of each time through the loop, the semantics change, and the program prints 1, 2 3.

What did you expect to see?

Behavior unchanged.

[adonovan]

Thanks for reporting. (Curious: was this an encountered bug or an a priori deduction?)

Pretty much all our uses of equalSyntax in modernizers have some kind of exploitable edge case like this. Depending on how strict we want to be about aliasing and side effects, we may need to downgrade a number of modernizers if we are to achieve our goal of "first do no harm". Seeing how many subtle bugs we have had so far even among the easy cases, I am starting to wonder whether (continuing the medical analogy) modernizers are mere cosmetic surgery, a procedure of no medical benefit that is not without risk, and whether batch modernization is ever something we should recommend.

The benefits may be clearer in cases of interactive modernization, when the user is already critically studying the code (to stretch the metaphor, a form of informed consent), or in the LLM-inference feedback loop, where the user has indicated a penchant for backstreet brain surgery. ;-)

[jba]

Found it during actual use.

[findleyr]

Milestoning for next minor.

[adonovan]

This bug caused a bad transformation in the regexp package too:

func (m *machine) step(runq, nextq *queue, pos, nextPos int, c rune, nextCond *lazyFlag) {
	longest := m.re.longest
	for j := 0; j < len(runq.dense); j++ { // <--- don't use for/range: the length changes!
...
				runq.dense = runq.dense[:0]
	}

Fortunately it was caught by tests.

[adonovan]

To be strictly correct--and that is the standard on which we find ourselves converging for modernizers--we would need to disable the transformation to range s unless we can prove that len(s) is loop invariant. In practice that means s must be a local variable that is assigned once.

[jba]

for a slice: is assigned once and doesn't escape
for a map: is assigned once, doesn't escape, and isn't index-assigned, deleted from or cleared in the loop body
also, for both: no unsafe manipulation of the header

(All modernizers should probably punt if unsafe is imported.)

[adonovan]

for a map: is assigned once, doesn't escape, and isn't index-assigned, deleted from or cleared in the loop body

rangeint doesn't touch maps, though I suppose it could replace for i := 0; i < len(m); i++ with for range when i is unneeded. But you give a good reason why we shouldn't try; noted.

also, for both: no unsafe manipulation of the header

Unsafe access would require &slice, which would trigger the address-taken check, so I don't think it requires any further check.

[gopherbot]

Change https://go.dev/cl/660435 mentions this issue: gopls/internal/analysis/modernize: rangeint: respect side effects