cmd/compile: add GOEXPERIMENT=loopvar #57969

rsc · 2023-01-24T01:41:37Z

@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).

gopherbot · 2023-01-25T18:12:58Z

Change https://go.dev/cl/411904 mentions this issue: cmd/compile: experimental loop iterator capture semantics change

rsc · 2023-02-01T19:02:07Z

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

go101 · 2023-02-02T04:34:11Z

Could it be clarified more:

Does this apply to both for-range and for;;?
Does this ignore all go.mod files?

dr2chase · 2023-02-02T17:20:09Z

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?)

rsc · 2023-02-09T02:27:58Z

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

go101 · 2023-02-10T16:52:20Z

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.

Merovius · 2023-02-10T17:04:48Z

@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.

go101 · 2023-02-11T01:55:58Z

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.

go101 · 2023-02-11T16:52:44Z

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)
	}
}

Merovius · 2023-02-11T17:47:41Z

@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.

go101 · 2023-02-12T01:23:11Z

:)

See #1927 (comment) We can remove the reassignment after [this experiment](golang/go#57969) is concluded.

Adds: GOEXPERIMENT=loopvar (expected way of invoking) -d=loopvar={-1,0,1,2,11,12} (for per-package control and/or logging) -d=loopvarhash=... (for hash debugging) loopvar=11,12 are for testing, benchmarking, and debugging. If enabled,for loops of the form `for x,y := range thing`, if x and/or y are addressed or captured by a closure, are transformed by renaming x/y to a temporary and prepending an assignment to the body of the loop x := tmp_x. This changes the loop semantics by making each iteration's instance of x be distinct from the others (currently they are all aliased, and when this matters, it is almost always a bug). 3-range with captured iteration variables are also transformed, though it is a more complex transformation. "Optimized" to do a simpler transformation for 3-clause for where the increment is empty. (Prior optimization of address-taking under Return disabled, because it was incorrect; returns can have loops for children. Restored in a later CL.) Includes support for -d=loopvarhash=<binary string> intended for use with hash search and GOCOMPILEDEBUG=loopvarhash=<binary string> (use `gossahash -e loopvarhash command-that-fails`). Minor feature upgrades to hash-triggered features; clients can specify that file-position hashes use only the most-inline position, and/or that they use only the basenames of source files (not the full directory path). Most-inlined is the right choice for debugging loop-iteration change once the semantics are linked to the package across inlining; basename-only makes it tractable to write tests (which, otherwise, depend on the full pathname of the source file and thus vary). Updates #57969. Change-Id: I180a51a3f8d4173f6210c861f10de23de8a1b1db Reviewed-on: https://go-review.googlesource.com/c/go/+/411904 Reviewed-by: Matthew Dempsky <mdempsky@google.com> Run-TryBot: David Chase <drchase@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

dr2chase · 2023-03-06T21:38:14Z

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 .

The addition of WaitGroup.Go in the standard library has been repeatedly proposed and rejected. See golang/go#18022, golang/go#23538, and golang/go#39863 In summary, the argument for WaitGroup.Go is that it avoids bugs like: go func() { wg.Add(1) defer wg.Done() ... }() where the increment happens after execution (not before) and also (to a lesser degree) because: wg.Go(func() { ... }) is shorter and more readble. The argument against WaitGroup.Go is that the provided function takes no arguments and so inputs and outputs must closed over by the provided function. The most common race bug for goroutines is that the caller forgot to capture the loop iteration variable, so this pattern may make it easier to be accidentally racy. However, that is changing with golang/go#57969. In my experience the probability of race bugs due to the former still outwighs the latter, but I have no concrete evidence to prove it. The existence of errgroup.Group.Go and frequent utility of the method at least proves that this is a workable pattern and the possibility of accidental races do not appear to manifest as frequently as feared. A reason *not* to use errgroup.Group everywhere is that there are many situations where it doesn't make sense for the goroutine to return an error since the error is handled in a different mechanism (e.g., logged and ignored, formatted and printed to the frontend, etc.). While you can use errgroup.Group by always returning nil, the fact that you *can* return nil makes it easy to accidentally return an error when nothing is checking the return of group.Wait. This is not a hypothetical problem, but something that has bitten us in usages that was only using errgroup.Group without intending to use the error reporting part of it. Thus, add a (yet another) variant of WaitGroup here that is identical to sync.WaitGroup, but with an extra method. Signed-off-by: Joe Tsai <joetsai@digital-static.net>

go101 · 2023-03-13T12:19:59Z

Well done. All the 3 examples in #57969 (comment) get verified.

thediveo · 2023-03-25T11:54:26Z

@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?

go101 · 2023-03-25T13:48:38Z

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?

:)

The addition of WaitGroup.Go in the standard library has been repeatedly proposed and rejected. See golang/go#18022, golang/go#23538, and golang/go#39863 In summary, the argument for WaitGroup.Go is that it avoids bugs like: go func() { wg.Add(1) defer wg.Done() ... }() where the increment happens after execution (not before) and also (to a lesser degree) because: wg.Go(func() { ... }) is shorter and more readble. The argument against WaitGroup.Go is that the provided function takes no arguments and so inputs and outputs must closed over by the provided function. The most common race bug for goroutines is that the caller forgot to capture the loop iteration variable, so this pattern may make it easier to be accidentally racy. However, that is changing with golang/go#57969. In my experience the probability of race bugs due to the former still outwighs the latter, but I have no concrete evidence to prove it. The existence of errgroup.Group.Go and frequent utility of the method at least proves that this is a workable pattern and the possibility of accidental races do not appear to manifest as frequently as feared. A reason *not* to use errgroup.Group everywhere is that there are many situations where it doesn't make sense for the goroutine to return an error since the error is handled in a different mechanism (e.g., logged and ignored, formatted and printed to the frontend, etc.). While you can use errgroup.Group by always returning nil, the fact that you *can* return nil makes it easy to accidentally return an error when nothing is checking the return of group.Wait. This is not a hypothetical problem, but something that has bitten us in usages that was only using errgroup.Group without intending to use the error reporting part of it. Thus, add a (yet another) variant of WaitGroup here that is identical to sync.WaitGroup, but with an extra method. Signed-off-by: Joe Tsai <joetsai@digital-static.net>

gopherbot added this to the Proposal milestone Jan 24, 2023

gopherbot added the Proposal label Jan 24, 2023

thanm mentioned this issue Jan 31, 2023

Go compiler and runtime meeting notes #43930

Open

rsc added the Proposal-FinalCommentPeriod label Feb 1, 2023

rsc changed the title ~~proposal: cmd/compile: add GOEXPERIMENT=loopvar~~ cmd/compile: add GOEXPERIMENT=loopvar Feb 9, 2023

rsc modified the milestones: Proposal, Backlog Feb 9, 2023

rsc added Proposal-Accepted and removed Proposal-FinalCommentPeriod labels Feb 9, 2023

DrJosh9000 mentioned this issue Feb 22, 2023

Implement Async Pipeline Upload Flow buildkite/agent#1927

Merged

triarius added a commit to buildkite/agent that referenced this issue Feb 24, 2023

Eliminate a named type and a loop closure in tests

d2f79e1

See #1927 (comment) We can remove the reassignment after [this experiment](golang/go#57969) is concluded.

triarius added a commit to buildkite/agent that referenced this issue Feb 27, 2023

Eliminate a named type and a loop closure in tests

77e6b92

See #1927 (comment) We can remove the reassignment after [this experiment](golang/go#57969) is concluded.

dr2chase closed this as completed Mar 6, 2023

dsnet mentioned this issue Mar 7, 2023

syncs: add WaitGroup wrapper tailscale/tailscale#7481

Merged

go101 mentioned this issue May 10, 2023

spec: less error-prone loop variable scoping #60078

Closed

dmitshur modified the milestones: Backlog, Go1.21 Jun 4, 2023

stoksc mentioned this issue Jul 12, 2023

chore: k8s pod & node informer actor refactor [DET-9597] determined-ai/determined#7182

Merged

4 tasks

mdempsky mentioned this issue Aug 28, 2023

cmd/compile: closure func name changes when inlining #60324

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: add GOEXPERIMENT=loopvar #57969

cmd/compile: add GOEXPERIMENT=loopvar #57969

rsc commented Jan 24, 2023

gopherbot commented Jan 25, 2023

rsc commented Feb 1, 2023

go101 commented Feb 2, 2023

dr2chase commented Feb 2, 2023

rsc commented Feb 9, 2023

go101 commented Feb 10, 2023 •

edited

Merovius commented Feb 10, 2023 •

edited

go101 commented Feb 11, 2023 •

edited

go101 commented Feb 11, 2023 •

edited

Merovius commented Feb 11, 2023

go101 commented Feb 12, 2023

dr2chase commented Mar 6, 2023

go101 commented Mar 13, 2023

thediveo commented Mar 25, 2023 •

edited

go101 commented Mar 25, 2023 •

edited

cmd/compile: add GOEXPERIMENT=loopvar #57969

cmd/compile: add GOEXPERIMENT=loopvar #57969

Comments

rsc commented Jan 24, 2023

gopherbot commented Jan 25, 2023

rsc commented Feb 1, 2023

go101 commented Feb 2, 2023

dr2chase commented Feb 2, 2023

rsc commented Feb 9, 2023

go101 commented Feb 10, 2023 • edited

Merovius commented Feb 10, 2023 • edited

go101 commented Feb 11, 2023 • edited

go101 commented Feb 11, 2023 • edited

Merovius commented Feb 11, 2023

go101 commented Feb 12, 2023

dr2chase commented Mar 6, 2023

go101 commented Mar 13, 2023

thediveo commented Mar 25, 2023 • edited

go101 commented Mar 25, 2023 • edited

go101 commented Feb 10, 2023 •

edited

Merovius commented Feb 10, 2023 •

edited

go101 commented Feb 11, 2023 •

edited

go101 commented Feb 11, 2023 •

edited

thediveo commented Mar 25, 2023 •

edited

go101 commented Mar 25, 2023 •

edited