Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: add GOEXPERIMENT=loopvar #57969

Closed
rsc opened this issue Jan 24, 2023 · 15 comments
Closed

cmd/compile: add GOEXPERIMENT=loopvar #57969

rsc opened this issue Jan 24, 2023 · 15 comments

Comments

@rsc
Copy link
Contributor

rsc commented Jan 24, 2023

@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 gopherbot added this to the Proposal milestone Jan 24, 2023
@gopherbot
Copy link
Contributor

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

@rsc
Copy link
Contributor Author

rsc commented Feb 1, 2023

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

@zigo101
Copy link

zigo101 commented Feb 2, 2023

Could it be clarified more:

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

@dr2chase
Copy link
Contributor

dr2chase commented Feb 2, 2023

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
Copy link
Contributor Author

rsc commented Feb 9, 2023

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

@rsc rsc changed the title proposal: cmd/compile: add GOEXPERIMENT=loopvar cmd/compile: add GOEXPERIMENT=loopvar Feb 9, 2023
@rsc rsc modified the milestones: Proposal, Backlog Feb 9, 2023
@zigo101
Copy link

zigo101 commented Feb 10, 2023

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
Copy link
Contributor

Merovius commented Feb 10, 2023

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

@zigo101
Copy link

zigo101 commented Feb 11, 2023

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.

@zigo101
Copy link

zigo101 commented Feb 11, 2023

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
Copy link
Contributor

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

@zigo101
Copy link

zigo101 commented Feb 12, 2023

:)

triarius added a commit to buildkite/agent that referenced this issue Feb 24, 2023
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
See #1927 (comment)

We can remove the reassignment after [this experiment](golang/go#57969) is concluded.
gopherbot pushed a commit that referenced this issue Mar 6, 2023
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
Copy link
Contributor

dr2chase commented Mar 6, 2023

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 .

@dr2chase dr2chase closed this as completed Mar 6, 2023
dsnet added a commit to tailscale/tailscale that referenced this issue Mar 7, 2023
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>
dsnet added a commit to tailscale/tailscale that referenced this issue Mar 7, 2023
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>
dsnet added a commit to tailscale/tailscale that referenced this issue Mar 9, 2023
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>
@zigo101
Copy link

zigo101 commented Mar 13, 2023

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

@thediveo
Copy link

thediveo commented Mar 25, 2023

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

@zigo101
Copy link

zigo101 commented Mar 25, 2023

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?

:)

darksip pushed a commit to darksip/tailscale that referenced this issue Apr 3, 2023
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>
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Jun 4, 2023
@golang golang locked and limited conversation to collaborators Jun 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants