x/tools/gopls/internal/analysis/modernize: rangeint: transformation unsound when loop variable is defined outside loop

[riannucci]

gopls version

Build info
----------
golang.org/x/tools/gopls v0.18.1
    golang.org/x/tools/[email protected] h1:2xJBNzdImS5u/kV/ZzqDLSvlBSeZX+pWY9uKVP7Pask=
    github.com/BurntSushi/[email protected] h1:pxW6RcqyfI9/kWtOwnv/G+AzdKuy2ZrqINhenH4HyNs=
    github.com/google/[email protected] h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
    golang.org/x/exp/[email protected] h1:1xaZTydL5Gsg78QharTwKfA9FY9CZ1VQj6D/AZEvHR0=
    golang.org/x/[email protected] h1:Zb7khfcRGKk+kqfxFaP5tZqCnDZMjC5VtUBs87Hr6QM=
    golang.org/x/[email protected] h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w=
    golang.org/x/[email protected] h1:L2k9GUV2TpQKVRGMjN94qfUMgUwOFimSQ6gipyJIjKw=
    golang.org/x/[email protected] h1:bofq7m3/HAFvbF51jz3Q9wLg3jkvSPuiZu/pD1XwgtM=
    golang.org/x/[email protected] h1:Ja/5gV5a9Vvho3p2NC/T2TtxhHjrWS/2DvCKMvA0a+Y=
    golang.org/x/[email protected] h1:NPGnvPOTgnjBc9HTaUx+nj+EaUYxl5SJOWqaDYGaFYw=
    honnef.co/go/[email protected] h1:4bH5o3b5ZULQ4UrBmP+63W9r7qIkqJClEA9ko5YKx+I=
    mvdan.cc/[email protected] h1:bg91ttqXmi9y2xawvkuMXyvAA/1ZGJqYAEGjXuP0JXU=
    mvdan.cc/xurls/[email protected] h1:lyBNOm8Wo71UknhUs4QTFUNNMyxy2JEIaKKo0RWOh+8=
go: go1.23.6

go env

Not applicable

What did you do?

Given a main.go:

package main

// longestPrefix finds the length of the shared prefix
// of two strings
func longestPrefix(k1, k2 string) int {
	var i int
	for i = 0; i < min(len(k1), len(k2)); i++ {
		if k1[i] != k2[i] {
			break
		}
	}
	return i
}

func main() {
	if longestPrefix("hello", "h") != 1 {
		panic("madness!!")
	}
}

You can run:

$ go run main.go
$ # OK
$ modernize -fix main.go
$ # loop now looks like "for i = range min(len(k1), len(k2)) {" with all other lines unchanged
$ go run main.go
panic: madness!!

goroutine 1 [running]:
main.main()
	/Users/username/main.go:17 +0x5c
exit status 2
$

(I encountered this while converting some old Go code - I accepted modernize's suggestion without thinking about it, but eventually traced the failing acceptance tests back to this transformation)

What did you see happen?

Modernize rewrote the loop to use range over int, except that this changes the loop's intended effect on the loop variable. Normally it would have had an additional increment in order to fail the i < min(len(k1), len(k2)) comparison, but that no longer happens.

I saw #72917 but this seemed subtlely different enough (and likely easy to recognize with a local syntax matcher vs looking at the loop body)

What did you expect to see?

Modernize should have left this loop alone (or - range over int should correctly affect the loop variable in this case, but not sure that this is feasible).

Editor and settings

Not applicable

Logs

Not applicable

[gabyhelp]

Related Issues

• x/tools/gopls/internal/analysis/modernize: rangeint generates correct but not idiomatic transformation #71725 (closed)
• x/tools/gopls/internal/analysis/modernize: rangeint should be silent when RHS of 'i < limit' depends on loop var #72726 (closed)
• x/tools/gopls/internal/analysis/modernize: rangeint: transformation unsound when loop body has effects on iterated array #72917
• x/tools/gopls: modernize misses modernization of 3-way "max" #72733
• x/tools/gopls: yet more modernizer bugs #71847 (closed)
• x/tools/gopls: No warning "loop variable xx captured by func literal". #60016 (closed)
• x/tools/gopls: modernizer max changes logic #71878 (closed)
• x/tools/gopls: loopclosure analysis runs on Go 1.22.0 #65910 (closed)
• x/tools/gopls: range over func without variable shows 'simplify range expression' warning #65799 (closed)
• x/tools/gopls/internal/analysis: add "modernizer" analyzers #70815 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[riannucci]

This looks very similar to the fix in https://go.dev/cl/650815 - I'll see if I can pull that (but the test case that was added implies that incrementing i in the loop body is necessary to trigger the bug, but it's not really in the example I gave above)

[riannucci]

FWIW go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest main.go still prints /Users/username/main.go:7:6: for loop can be modernized using range over int

[xieyuschen]

Hi @riannucci , the CL650815 you mentioned is that "The range expression x is evaluated before beginning the loop" mentioned in golang spec, so if you change it in the middle, it won't take effects as for...range won't evaluate it any more.

This issue looks like the assignment to i doesn't take effect, golang spec is a bit ambiguous and i don't know whether it's intended or not. @adonovan Hence, looks like it's not a modernize issue.

For example, if you use this code, it won't panic.

func longestPrefix(k1, k2 string) int {
	var i int
	for i = range min(len(k1), len(k2)) {
		if k1[i] != k2[i] {
			break
		}
		i++
	}
	return i
}

[adonovan]

This looks very similar to the fix in https://go.dev/cl/650815 - I'll see if I can pull that (but the test case that was added implies that incrementing i in the loop body is necessary to trigger the bug, but it's not really in the example I gave above)

That CL includes fixes and tests for two bugs:

• for i := 0; i < n; i++ -> for range n is invalid if the loop body contains i++
• var i int; for i = 0; i < n; i++ -> for range n removes uses of i that might make local var i unreferenced

The first of these is the same issue you reported here: the final state of variable i differs between for- and range-loops, so the transformation is unsound. Closing as dup.