cmd/compile: add GOEXPERIMENT=loopvar
rsc opened this issue · 15 comments
@dr2chase has been working on an implementation of the for loop scoping change in discussion #56010. Before we get to a formal proposal, it would be good to make the code available for people to try in their own code. I propose we add the change behind a GOEXPERIMENT=loopvar setting, so that developers can judge the effect in their own code, including in the Go 1.21 release (by setting the environment variable and recompiling).
Change https://go.dev/cl/411904 mentions this issue: cmd/compile: experimental loop iterator capture semantics change
Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group
Could it be clarified more:
- Does this apply to both
for-rangeandfor;;? - Does this ignore all
go.modfiles?
Applies to both for-range and for;;, using the fancy Javascript rewrite so that side-effects to the "unshared" iteration variable that occur during (*) the serial execution of the loop body are picked up by the next iteration of the loop. (*) any side effect observed at the completion of that iteration of the loop, whether serial, synchronized, or racy+lucky.
The current state of the CL is that it will ignore go.mod files, but there is also per-package control for turning it off (-gcflags=whatever_package=-d-loopvar=-1, on ...loopvar=1, or accepting whatever is the default for the compiler or build ...loopvar=0). My model/plan at this stage is that I want to test this on as much code as possible, and in practice so far it works fine, so the risk from applying the change to entire applications is pretty low. Involving module version will just get in the way of running this on lots of code. I don't want people to be conservative, I want them to test it.
That said, there is also a hashed-search tool for repeatedly running a failing application with the change enabled in different places to search for any place where the change is a problem. I'm still tweaking that, and it also interacts with what I think is a bug in how source positions are recorded for names in inlined functions, but the idea would be if you have a command some command that go builds then fails to run then you would type lvh-search some command that go builds then fails to run and in a relatively short time (fewer than 10 executions of the failing command, most likely) it will tell you exactly where the failure-causing loop is. Right now lvh-search is spelled gossahash -e loopvarhash, and it works like this:
gossahash -e loopvarhash go run .
Trying go args=[run .], env=[GOCOMPILEDEBUG=loopvarhash=1]
Trying go args=[run .], env=[GOCOMPILEDEBUG=loopvarhash=0]
go failed (5 distinct triggers): exit status 1
Trying go args=[run .], env=[GOCOMPILEDEBUG=loopvarhash=00]
go failed (3 distinct triggers): exit status 1
Trying go args=[run .], env=[GOCOMPILEDEBUG=loopvarhash=000]
go failed (2 distinct triggers): exit status 1
Trying go args=[run .], env=[GOCOMPILEDEBUG=loopvarhash=1000]
go failed (1 distinct triggers): exit status 1
Review GSHS_LAST_FAIL.0.log for failing run
FINISHED, suggest this command line for debugging:
GOCOMPILEDEBUG=loopvarhash=1000 go run .
Problem is at /blahblahblah/src/cmd/compile/internal/loopvar/testdata/inlines/main.go:27:11
I.e., I want people to go into this feeling lucky, but if they're not lucky, I want them to find the problem in a few minutes with little-to-no mental effort. It's binary search, so finding the problem in 2500 loops takes only twice as long as finding the problem in a program with 50 loops (the Go compiler itself has 10,000-ish loops, but only about 50 that potentially leak their iteration variable).
What I expect (based on testing within Google) is that if you have a problem, it will be in a unit test, because the usual case for this has been with parallel subtest execution inside a loop with a captured iteration variable. When this happens, the pre-change behavior is to only run the last test in the series, and if it passes, you might never notice that there was a problem. If there is a problem in the not-run tests, the new behavior will uncover it (and it has done this). The hash search is a good match for this, since go test builds, then runs, so if you have a failing test, it is a simple matter of lvh-search go test -run TestThatFails . and it should take you directly there without wasting your time wondering which loop it is or if it is somewhere in your non-test code.
One tricky bit is whether the package enable/disable will work across inlines; that will need to happen by modules if/when we make the change, and that CL is already written and tested. Writing this all down has rubber-ducked me into thinking that the answer is "yes, the choice should follow across inlines"; if someone is trying this and encounters a problem in some module that they import but not their own code (i.e., it is in the module cache, readonly), I don't want them to be forced to download that code, modify it, and edit their application's go.mod to replace the import with its local version just to finish the testing. If they can disable it in the broken package on the command line, and that disable follows inlines, then they can more easily finish testing.
The other "tool" (perhaps just a script) I am trying to put together would combine information about where the transformation was applied (-gcflags=whatever_package=-d=loopvar=2) with coverage information so that it's possible to know that every transformed loop was tested. The new coverage in 1.20 can be used on entire applications, so this extends to more than just testing (though of course all Go programmers obtain 100% code coverage from their unit tests, right?)
No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group
After reading all comments about the change on 3-clause for-loops, I still think it is over-calibrated to apply the change on them.
It brings some implicit code, which actually breaks Go convention and increases the difficulty to explain their semantics.
@rsc mentioned a bug caused by the current semantic, I would argue that the bug is caused by carelessness.
IMHO, the semantic of
// way 1
for d := 3; d < len(terms); d++ {
...
}should be the same as (or more similar to)
// way 2
{
var d int
for d = 3; d < len(terms); d++ {
...
}
}instead of the proposed change
{
var d int
pd = &d
for d = 3; d < len(terms); d++ {
func() {
d := d // it is hardly to say this line is unnatural
defer func() {
*pd = d // but this line is too unnatural
}()
...
}()
}
}One reason is for good intuition.
Another reason is the change will avoid the bug when using way 1, but will not when using way 2. This is a new inconsistency.
@go101 Every bug is caused by carelessness. The question is how easy we should make it to be careless.
And you are correct, that the change will not avoid the bug in the latter code you posted, but that's not code anyone would write. That is, under this change the two snippets you posted are not semantically equivalent - just like they are not with the equivalent range loop constructs. You didn't really make a case for why they should be equivalent, or why they should be equivalent for the three-clause loop, but it's fine for them to not be equivalent for range loops.
I don't have very strong opinions on whether or not the three-clause loop should fall under the new semantics, but I do think there is an obvious case to be made that it's clearer and easier to understand if both loop constructs work the same.
Okay, I just express my opinion to state the change is counter-intuitive and unnecessary. It might make an (unexpected) consistency, but it will also create another inconsistency at the same time.
If the change is applied to for;;, then it looks the following program will print 01, which might surprise many people.
package main
func main() {
var f func() bool
f = func() bool {
return true
}
for a := 0; f(); a++ {
print(a)
f = func() bool {
return a < 1
}
}
}[Edit]: another example, which prints true now, but will print false if the change is applied. The implicit code is quite magic.
package main
func main() {
for i, p := 0, (*int)(nil); p == nil; print(p == &i) {
p = &i
}
}And a variation (the loop will ext soon now, but will never if the change is applied):
for i, p := 0, new(int); p != &i; {
p = &i
}[Edit 2]: The third example:
func foo(constFunc func(*[10000]int)) {
for a, i := [10000]int{}, 0; i < len(a); i++ { // will a be copied at each loop step? And twice?
go constFunc(&a)
}
}@go101 "People might write intentionally confusing programs" isn't a very good argument, IMO. And to be clear, I feel like I'd be surprised by the output even under the current semantics. I'd basically be surprised by its existence.
:)
Done, see https://go.dev/cl/411904, with follow-on https://go.dev/cl/463595 to track inlining.
There's an optimization still in the pipeline that is not yet submitted, but not necessary for the basic semantics: https://go.dev/cl/472355 .
Well done. All the 3 examples in #57969 (comment) get verified.
@go101 what was the clause or expectation you were verifying? From the thread I unfortunately cannot see what your examples were to show, but this has been pointed out before. Forgive me in case I've missed your clarification somehow above.
All in all I find your for ;; example to be rather the point in case for dealing with it in the same way as the for range loop. Before your example that was probably to show the opposite I was undecided.
In any case, the for ;; examples look very artifical to me, but then you might have seen such code in wild where I yet have to come across such code?
For example 1 and 2, it is backward compatibility breaking.
For example 3, it is a serious performance degradation.
The change of for;; is much different from the change of for range.
The former introduces implicit variable declarations, and the implicitness is a magic level one.
In any case, the for ;; examples look very artifical to me, but then you might have seen such code in wild where I yet have to come across such code?
:)