|
We propose to change for loop variables declared with := from one-instance-per-loop to one-instance-per-iteration. This change would apply only to packages in modules that explicitly declare a new enough Go version in the go.mod file, so all existing code is unaffected. Changing the loop semantics would prevent unintended sharing in per-iteration closures and goroutines (see this entry in the Go FAQ for one explanation). We expect this change to fix many subtly broken for loops in new code, as well as in old code as it is updated to the newer Go version. There was an earlier pre-proposal discussion of this idea at #56010.
For example, consider a loop like the one in this test:
func TestAllEven(t *testing.T) {
testCases := []int{0, 2, 4, 6}
for _, v := range testCases {
t.Run("sub", func(t *testing.T) {
t.Parallel()
if v&1 != 0 {
t.Fatal("odd v", v)
}
})
}
}
This test aims to check that all the test cases are even, but it doesn't check them all. The problem is that t.Parallel stops the closure and lets the loop continue, and then it runs all the closures in parallel when the loop is over and TestAllEven has returned. By the time the if statement in the closure executes, the loop is done, and v has its final iteration value, 6. All four subtests now continue executing in parallel, and they all check that 6 is even, instead of checking each of the test cases.
Most Go developers are familiar with this mistake and know the answer: add v := v to the loop body:
func TestAllEven(t *testing.T) {
testCases := []int{0, 2, 4, 6}
for _, v := range testCases {
v := v // MAKE ITERATION VALUE SHARING BUG GO AWAY
t.Run("sub", func(t *testing.T) {
t.Parallel()
if v&1 != 0 {
t.Fatal("odd v", v)
}
})
}
}
Changing the loop semantics would in essence insert this kind of v := v statement for every for loop variable declared with :=. It would fix this loop and many others to do what the author clearly intends.
A subtler variation is when the code says testCases := []int{1, 2, 4, 6} (note the non-even test case 1):
func TestAllEvenBuggy(t *testing.T) {
testCases := []int{1, 2, 4, 6}
for _, v := range testCases {
t.Run("sub", func(t *testing.T) {
t.Parallel()
if v&1 != 0 {
t.Fatal("odd v", v)
}
})
}
}
TestAllEvenBuggy passes today, because the test is only checking 6, four times. Changing the loop semantics would still fix the loop to do what the author clearly intended, but it would break the test, because the test is passing incorrectly. So there is the potential to cause problems for users.
Because of this potential for problems, the new loop semantics would only apply in Go modules that have opted in to the release with the new loops. If that was Go 1.22, then only packages in a module with a go.mod that says go 1.22 would get the new loop semantics. Packages in other modules, including packages in dependencies, would get the old semantics. This would guarantee that all existing Go code keeps working the same as it does today, even when compiling with a new toolchain. People only need to debug loop changes when they opt in to the new semantics in go.mod. This approach is in keeping with our backwards and forwards compatibility work, #56986 and #57001, specifically the principle that toolchain upgrades preserve the behavior of old code, and compatibility is based on the go line.
If this proposal is accepted, users will need additional tooling support for a successful transition. That would come primarily in two forms: a compiler mode that reports affected loops, and a debugging tool that identifies the specific loops whose changed compilation is responsible for causing a test failure. There is a demo of the tooling support at the end of this comment.
The tooling support is important and necessary, but we expect it to be needed only rarely. Analysis and conversion of Google's own Go source code found that only about 1 package test per 8,000 started failing due to the new semantics, and the bug was essentially always in the test itself, like in TestAllEvenBuggy. In contrast, the new loopclosure vet analysis that shipped in Go 1.20 flagged definite bugs in 1 test per 400. The rest stayed passing with their bugs fixed by the new semantics. That is, in Google's code base, about 5% of the tests that contain this kind of sharing mistake were like TestAllEvenBuggy, exposed as buggy by the new semantics. The other 95% of the tests with this kind of mistake still passed when they started testing what they intended to. Of all the test failures, only one was caused by a loop variable semantic test change in non-test code. That code was very low-level and could not tolerate a new allocation in the loop. The design document has details. This evidence suggests that changing the semantics is usually a no-op, and when it’s not, it fixes buggy code far more often than it breaks correct code.
Based on the preliminary discussion the further work summarized here, @dr2chase and I propose that we make the change in an appropriate future Go version, perhaps Go 1.22 if the stars align, and otherwise a later version.
More details can be found in the design document.
Update, May 10 2023: Note that the change will not break the common idiom of changing a loop variable in the body of a 3-clause for loops. See this comment for details and links before commenting about 3-clause for loops. Thanks.
Tooling Demonstration
To demonstrate the tooling we would provide to support a successful transition, here is a complete example of a test that passes today but fails with the new loop semantics:
% cat x_test.go
package x
import "testing"
func Test(t *testing.T) {
testCases := []int{1, 2, 4, 6}
for _, v := range testCases {
t.Run("sub", func(t *testing.T) {
t.Parallel()
if v&1 != 0 {
t.Fatal("odd v", v)
}
})
}
for _, v := range testCases {
t.Run("sub", func(t *testing.T) {
t.Log(v)
})
}
for _, v := range testCases {
t.Run("sub", func(t *testing.T) {
t.Parallel()
if v&1 != 0 {
t.Fatal("odd v", v)
}
})
}
}
%
Building the program with -d=loopvar=2 reports all the affected loops:
% GOEXPERIMENT=loopvar go test -gcflags=all=-d=loopvar=2 x_test.go
# runtime
go/src/runtime/proc.go:1815:7: loop variable freem now per-iteration, stack-allocated
go/src/runtime/proc.go:3149:7: loop variable enum now per-iteration, stack-allocated
go/src/runtime/mgcmark.go:810:6: loop variable d now per-iteration, stack-allocated
go/src/runtime/traceback.go:623:7: loop variable iu now per-iteration, stack-allocated
go/src/runtime/traceback.go:943:7: loop variable iu now per-iteration, stack-allocated
# runtime/pprof
go/src/runtime/pprof/pprof.go:386:9: loop variable r now per-iteration, heap-allocated
go/src/runtime/pprof/proto.go:363:6: loop variable e now per-iteration, stack-allocated
go/src/runtime/pprof/proto.go:612:9: loop variable frame now per-iteration, heap-allocated
go/src/runtime/pprof/protomem.go:29:9: loop variable r now per-iteration, heap-allocated
# command-line-arguments [command-line-arguments.test]
./x_test.go:7:9: loop variable v now per-iteration, stack-allocated
./x_test.go:15:9: loop variable v now per-iteration, stack-allocated
./x_test.go:20:9: loop variable v now per-iteration, stack-allocated
# internal/fuzz
go/src/internal/fuzz/fuzz.go:432:9: loop variable e now per-iteration, stack-allocated
--- FAIL: Test (0.00s)
--- FAIL: Test/sub (0.00s)
x_test.go:11: odd v 1
--- FAIL: Test/sub#08 (0.00s)
x_test.go:24: odd v 1
FAIL
FAIL command-line-arguments 0.199s
FAIL
%
Note that some loops are in the Go standard library. Those are unlikely to be the cause, so we can limit the diagnostics to the current package by dropping all=:
% GOEXPERIMENT=loopvar go test -gcflags=-d=loopvar=2 x_test.go
# command-line-arguments [command-line-arguments.test]
./x_test.go:7:9: loop variable v now per-iteration, stack-allocated
./x_test.go:15:9: loop variable v now per-iteration, stack-allocated
./x_test.go:20:9: loop variable v now per-iteration, stack-allocated
--- FAIL: Test (0.00s)
--- FAIL: Test/sub (0.00s)
x_test.go:11: odd v 1
--- FAIL: Test/sub#08 (0.00s)
x_test.go:24: odd v 1
FAIL
FAIL command-line-arguments 0.119s
FAIL
%
That trims the diagnostic output, but it still leaves us to check all our loops. In this trivial example, 2 of 3 are buggy, but in a real program there are fewer needles and more haystack. To solve that problem, we can use a dynamic tool that reruns a test, varying which loops compile with the new semantics, to identify the specific loops that cause the failure:
% bisect -loopvar go test x_test.go
bisect: checking target with all changes disabled
bisect: run: go test -gcflags=all=-d=loopvar=1,loopvarhash=n x_test.go... ok (13 matches)
bisect: run: go test -gcflags=all=-d=loopvar=1,loopvarhash=n x_test.go... ok (13 matches)
bisect: checking target with all changes enabled
bisect: run: go test -gcflags=all=-d=loopvar=1,loopvarhash=y x_test.go... FAIL (13 matches)
bisect: run: go test -gcflags=all=-d=loopvar=1,loopvarhash=y x_test.go... FAIL (13 matches)
bisect: target succeeds with no changes, fails with all changes
bisect: searching for minimal set of changes to enable to cause failure
bisect: run: go test -gcflags=all=-d=loopvar=1,loopvarhash=+0 x_test.go... ok (7 matches)
bisect: run: go test -gcflags=all=-d=loopvar=1,loopvarhash=+0 x_test.go... ok (7 matches)
bisect: run: go test -gcflags=all=-d=loopvar=1,loopvarhash=+1 x_test.go... FAIL (6 matches)
bisect: run: go test -gcflags=all=-d=loopvar=1,loopvarhash=+1 x_test.go... FAIL (6 matches)
bisect: run: go test -gcflags=all=-d=loopvar=1,loopvarhash=+01 x_test.go... FAIL (3 matches)
bisect: run: go test -gcflags=all=-d=loopvar=1,loopvarhash=+01 x_test.go... FAIL (3 matches)
...
bisect: FOUND failing change set
--- change set #1 (enabling changes causes failure)
./x_test.go:7:9
---
...
bisect: FOUND failing change set
--- change set #2 (enabling changes causes failure)
./x_test.go:20:9
---
...
bisect: target succeeds with all remaining changes enabled
%
Now we know to spend our attention on the loops at lines 7 and 20, which are in fact the buggy ones. (The loop at line 15 has been cleared of suspicion.) Bisect uses an adaptation of binary search that can identify the relevant loop in a relatively small number of trials, making it useful even on very large, very complicated tests that run for a long time.
|
