-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
spec: less error-prone loop variable scoping #60078
Comments
The biggest concern I could suggest would be the compatibility promise, and the go.mod stuff addresses that. When I started learning Go, I found a page which purported to be a list of common Go pitfalls. It said "pitfalls", plural, but in fact, this was the only one. That has been my experience of using the language for the last few years; this is the pitfall in Go programming. Do it. Doooo eeeeet. |
It may be pedantic, but packages in modules with |
Personally, I strongly oppose to apply this proposal to The proposal docs mentions that the semantic-equivalent form of for i := 0; i < 3; i++ {
prints = append(prints, func() { println(i) })
} is like {
i_outer := 0
first := true
for {
i := i_outer
if first {
first = false
} else {
i++
}
if !(i < 3) {
break
}
prints = append(prints, func() { println(i) })
i_outer = i
}
} Sorry, this adds too much magical implicitness and makes it is hard to make a clear explanation for the new semantic. Obeying users' intuitions is important. This is why I support applying this proposal to [update]: I don't think the so-called "accidental" bugs in using var ids []*int
for i := 0; i < 10; i++ {
ids = append(ids, &i)
} is totally the same as the bug in var ids []*int
var i int
for i = 0; i < 10; i++ {
ids = append(ids, &i)
} In both, I think most people think there is only one instance of the variable |
@go101 That is not the code as mentioned in the proposal; you have added an extra outer loop where the proposal only had a block. |
@zikaeroh corrected now. It doesn't affect the opinion. |
@go101 to be clear, you're only showing the "after" part of that kind of expansion, as far as this proposal is concerned. Currently, the equivalent expansion would be {
i := 0 // with proposal this is i_outer
first := true
for {
// with proposal, there's an i := i_outer here
if first {
first = false
} else {
i++
}
if !(i < 3) {
break
}
prints = append(prints, func() { println(i) })
// with proposal, there's an i_outer = i here
}
} i.e. the proposal is to add |
@Merovius Yes, that was a small mistake which has been corrected. It doesn't affect the whole opinion. |
Overall I am very much for this proposal. However @go101 did bring up a valid point about I would just want somebody to confirm the behavior of the following loop under the new semantics: for i := 0; i < 5; i++ {
fmt.Println(i)
i += 1
} Under the new semantics I believe it would print: 0, 1, 2, 3, 4 instead of: 0, 2, 4. This might be a little unexpected to users who do not know this history, and the bugs associated with capturing the loop variable. To reiterate (unintentional pun), I think the proposal is beneficial to Go but this is my only worry. |
@davidmdm The change does affect the behaviors of some use cases. |
@davidmdm I haven't seen any sort of convincing argument, why the three-clause Meanwhile "having two loop constructs which behave extremely differently in relation to short variable declarations is confusing and hard to explain" seems obvious and hard to refute to me. YMMV, of course. |
@Merovius for (let i = 0; i < 5; i++) {
console.log(i)
i++
} This will print the sequence 0, 2, 4. The same goes for python, and most other high level languages. Go would be unique (maybe, I won't pretend to know most languages...) in this regard. I am still for it, because it solves subtle bugs with regards to closures and addressability that I have fallen prey to before. For example: for i := 0; i < 5; i++ {
fmt.Println(i)
i += 1 // go vet: ineffective assignment -> iteration variable moving out of scope
} |
@davidmdm Maybe you missed it, but the proposal has the "transformed" code copy the final state of the loop variable to the beginning of the next loop, meaning that the above should also print ”0, 2, 4", and that {
i_outer := 0
first := true
for {
i := i_outer
if first {
first = false
} else {
i++
}
if !(i < 5) {
break
}
fmt.Println(i)
i += 1
i_outer = i
}
} The only semantic change this proposal should have is to do with capturing the loop variable by address or by a closure. |
@davidmdm You misunderstand the new semantics. They are basically the same as the Javascript let form you mentioned. The loop you showed does not change meaning. This kind of loop is explicitly called out at the end of the https://go.googlesource.com/proposal/+/master/design/60078-loopvar.md#language-specification section. ^F for "Note that in both 3-clause..." I would encourage you to try any programs you like on your own computer, using gotip with GOEXPERIMENT=loopvar.
|
Regarding 3-clause loops, three points: If we don't fix 3-clause for loops, the footgun remains. If we leave 3-clause loops alone, then every time you change a loop from 3-clause to range or back, you have to stop and reason about the possible capture difference you might be introducing. When you are doing that kind of conversion, you should have to think about whether the iteration visits the same things and then be able to stop there. Real-world evidence all points to this change fixing some 3-clause for loops and breaking none. I freely admit that it is possible to write 3-clause for loops that behave differently in the new semantics, just as it is possible to write range loops that behave differently, such as this one by Tim King:
As Merovius put it so well, if you saw this in real life, the surprise would be not that the code behaves differently with the new loop semantics but that it was written this way at all. Focusing on artificial examples misses the point, which is that in actual programs, when per-iteration vs per-loop does matter, 99% of the time users expect per-iteration. Here are three real examples of 3-clause loop mistakes that made it into a repo and had to be corrected (I found these by searching git histories for diff hunks consisting only of added
(Of course, searching git histories misses all the buggy loops that were debugged before being checked in.) Having just gone through all the test failures inside Google caused by enabling the new semantics globally, I can tell you that 3-clause loops were a minority of root causes of test failures, but they did exist, and all of them clearly expected per-iteration semantics. The test failures happened because the test was incorrectly passing, like That's some of the real-world evidence in favor of changing 3-clause loops. I have looked, and I found no evidence in favor of not changing them. If you want to make the case for not changing them, the way to do that would be to provide real-world examples of code that breaks with the new semantics. You might start by running your own tests against gotip with the loop variable change enabled (instructions here). Evidence outweighs gut reaction. I sympathize with the gut reaction from @go101 and @davidmdm here. I remember the first time I saw 3-clause for loops with per-iteration variable semantics, in an early talk about Dart around 2009 or so. My own gut reaction was that I was certain it was a terrible idea that couldn't possibly work, too strange to merit serious consideration. But as they say, certainty is just an emotion. The evidence we've built up over more than a decade of experience with Go seems to prove conclusively that Dart was right (as is JavaScript's for-let) and Go was wrong – I was wrong. Live and learn. |
Number one frustration with how lambdas interact with loops in Go and Node.js. This is actually a safety concern. On the one hand, declaring lambdas inside of a loop is usually a performance loss, compared to declaring the lambda before or after the loop. On the other hand, when you need to curry some specific iteration state, then it is annoying to have to use additional boilerplate there. 100% code coverage is not guaranteed to catch this glitch, by the way. This is a rather subtle quirk dating back to how C does its dirty work. If the developer has insufficient tests, then many downstream problems can occur. i can think of no useful program that desires only the last iteration state to be curried into all the lambdas. Let's adopt this fix ASAP. |
This is 100% a good idea and worth doing, but I worry we're down playing the idea "only packages in a module with a go.mod that says go 1.22 would get the new loop semantics". I was trying to find more about this in @rsc 's video and link (may have missed it there's a lot to catch up on). Tools like renovatebot and dependabot are almost required for open source projects. Many configure these tools to auto update minor (and patch) version dependencies (of course after tests run) and even auto merge (there could be dozens of these packages to keep up to date). My mental model is to never expect the line -go 1.21
+go 1.22 to be something that could break my existing code since it is a minor change. These tools by default auto update this line. What is the correct mental model for seeing the above diff? Is there an assumption beyond optimizations (sort ordering changing). Maybe the bar is "beyond reasonable doubt" for behavior changes, while I thought the previous bar was much more deterministic. Maybe it's off topic or overblown, but I don't know what I should think when I review renovatebot diffs in the future that do patch changes to the "go" line in the mod file. Maybe there should be an official-ish recommendation of |
The correct mental model should be that you are running a newer version of a dependency and you should test that your software continues to work as expected with that newer dependency. This is true for any dependency, not just the Go line. If you are using foo/bar v1.2.3 and dependabot bumps it to v1.2.4 or v1.3.0, the fact that it's still v1 (not a different major version) means the author intends that there are no breaking changes. But maybe the author fixed a bug, and your code accidentally depends on the buggy behavior. Or maybe the author introduced a bug by accident, and your program is the one that discovers the bug. Intending not to break you is different from not breaking you. The same is true for the go line. If you see the go version change, then the Go team intends that there are no breaking changes for you. But maybe we fixed a bug you depend on, or maybe we introduced a bug by accident. In literally every instance of a loopvar-induced failure I've seen, "fixed a bug you depend on" is an accurate description. We absolutely understand that intending not to break you is different from not breaking you. When we do break you, we want you to have the right tools to make that as painless as possible. That's why the loopvar change is keyed on the go line, and it's also why other compatible-but-possibly-breaking changes will have GODEBUGs with defaults keyed on the go line, as of Go 1.21 (see this doc). Circling back to dependabot, if dependabot proposes a change, any change at all, it would be a mistake to accept it solely because "it's only a version number increment". The only way dependabot is safe is if (1) the change passes all your tests, and (2) you have very good test coverage. If that's already true for your module dependencies, then you'll have no problem with go line bumps either. |
This proposal has been added to the active column of the proposals project |
Let me re-state my opinion: I support making the change for The current semantic of The current semantic of traditional It is natural for me to think the iteration variables func foo(f func(*K, *V)) {
for k, v := range c {
// If we think the iteration variables k and v are declared in
// each loop step, then the following k and v are exactly
// the iteration variables k and v.
f(&k, &v)
}
} On the other hand, it is natural for me to think the iteration variables func bar(f func(*K, *V)) {
for k, v := next(); ; k, v = next() {
// By the proposed change, the following k and v are implicitly
// declared and they are not the above iteration variables k and v.
// This is too counter-intuitive.
f(&k, &v)
}
} That is all. Obeying intuition is so important for a language. A thousand arguments to break the rule are still pale. Adding magic implicitness is a big no-no (esp. for Go, a language promoting explicitness). Doing this will break many people's expectations. In fact, after I read all the arguments in related threads to break the rule, I found none of them are
Sorry, if the change is made on traditional Currently, I have found two new footguns caused by this: The first: // The loop prints true now. The semantic change will make it print false.
for i, p := 0, (*int)(nil); p == nil; print(p == &i) {
p = &i
} The second: // Now the value "a" will not duplicated at each loop step.
// The semantic change will make "a" be duplicated (twice)
// at each loop step.
func foo(constFunc func(*[100000]int)) {
for a, i := [100000]int{}, 0; i < len(a); i++ {
constFunc(&a)
}
} That is two surprises for many people. Several people said the two cases are artificial or "intentionally confusing programs". I'm speechless on such non-serious attitudes. I can't believe that the bar to keep backward-compatibility is down to such a low level. The two code pieces are just two demonstrations. They are valid code, it is hard to say no code like these are used in practice.
It is unfair to compare Go with JS (I'm not familar with Dart, so I'm not sure if this is also true for Dart). Go is quite different from JS, Go supports pointers and large-size types/values, which makes the use cases using Go loops much more diverse. This is evident in the above two new footgun examples.
I don't think this is not a problem for a qualified Go programmer. It is unnecessary to make the pointless consistency. The two kinds of loops are different in nature and a qualified Go programmer should realize the result. In fact, making them behave differently is a great feature so that Go programmers have more choices for different scenarios. Even if the argument is valid, then similar situations will happen if the proposed change is applied to traditional // 1
for v := f(); g(); h(&v) {
... // no continue here
}
// 2
for v := f(); g(); {
... // no continue here
h(&v)
}
Firstly, I think maybe what you have looked are still not enough. Second, I also don't see universal and strong desires in favor of changing them. And we need to make it clear whether or not the causes of the following two bugs are the same. var ids []*int
for i := 0; i < 10; i++ {
ids = append(ids, &i)
} and var ids []*int
var i int
for i = 0; i < 10; i++ {
ids = append(ids, &i)
} Finally, in my opinion, mixing the changes to the two kinds of loops is not a good idea. I suggest to split this proposal into two, one for I have noticed that this proposal has been added to the active column and will be reviewed. I'm curious that why do this in such a hurry. Personally, I think the current analysis work is still not sufficient. Shouldn't it be done after the experimental phase is almost over? |
@go101 I think most participants in the conversation understand your opinion. That doesn't mean they can't disagree with it.
To me, the fact that your arguments are based on extremely contrived code to show anything about "intuition" makes them very weak. I still, to this day, have not formed an opinion on what any of your examples "should" do, because I think the only correct response to encountering them in the wild isn't to try to puzzle through and explain their behavior, but to remove them. They are bad code. "This change would make extremely bad and obtuse code counter-intuitive" is a non-starter of an argument to me. That code is counter-intuitive, no matter what it does.
We are accepting breaking backwards compatibility with this proposal. Even with the part you explicitly endorse. So, clearly this is not the issue. You are making an argument based on supposed "counter-intuitive semantics". And for that, yes, the fact that your examples are contrived is extremely salient. |
This proposal had a discussion and a separate issue discussing adding the experiment flag. Both of which you are perfectly aware of, as you've participated in both. So, pretending that there hasn't been extensive conversation about this so far is simply disingenuous. Meanwhile, being added to the active column simply means that this proposal is in the active discussion phase. Nothing more. That is, it's not a "likely accept", or anything. There are proposals which spent many months in the active phase. So, again, pretending that there is any "hurry" implied by adding it to the list of active proposals is wrong. That being said, the response to both the discussion and this proposal have been overwhelmingly positive. Note that this issue accumulated 230 👍 in two days, and only 6 👎. The emoji-votes on the original discussion are even more unanimously and overwhelmingly positive. There also have been few proposals in the past so thoroughly evidenced and backed by actual data. So, yes, just because you don't like it, doesn't mean we shouldn't do this soon, based on the long discussion phase and extremely positive feedback we had so far. |
@go101, your follow-up post repeatedly uses words like “intuitive” and “natural” and “reasonable” and “expectations” as if they are universal absolutes. To me, the new semantics are intuitive and natural and reasonable and match my expectations better than the old semantics. We are going to have to agree to disagree about how the new semantics make us feel. I want to be clear: instead of focusing on feelings, we are going to make this decision based on evidence. The evidence we have presented in favor of making the change is our experience running the new semantics in a code base with many million lines of Go code. The evidence you have presented is a few hypothetical programs that look nothing like any Go program I have ever seen in actual practice. The evidence in favor of the change is – objectively and scientifically – far stronger. About your hypotheticals, you wrote:
On the contrary, it is easy for me to say that no code like those is used in practice, or at least that it is exceedingly rare. Because, again, we are already running many millions of lines of Go code using the new semantics and have encountered zero instances of code that broke due to the new semantics of 3-clause for loops. The single instance where correct code arguably broke was a range loop. The bar is not “never anywhere”. The bar is “exceedingly rare”, and the evidence we have gathered absolutely establishes that. I have invited you to look for real-world evidence supporting your position and explained how to go about doing that. If you would like to gather real evidence and share it with us, please do. But otherwise, please stop posting the same arguments over and over. Thank you. |
Change https://go.dev/cl/518815 mentions this issue: |
With #60078 accepted, we expect Go 1.22 will have different for loop semantics than Go 1.19 did. Go 1.19 is already unsupported, but add a check anyway, just to help catch some mistakes and usage of old Go toolchains beyond their end-of-support. Note that Go 1.19 can keep being used indefinitely with pre-Go 1.22 code. This change only makes it refuse to build code that says it needs Go 1.22 semantics, because Go 1.19 does not provide those. Cherry-pick of the change from the Go 1.20 branch. For #60078. Change-Id: I75118d6fbd0cc08a6bc309aca54c389a255ba7dc Reviewed-on: https://go-review.googlesource.com/c/go/+/518675 Reviewed-by: Bryan Mills <bcmills@google.com> Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-on: https://go-review.googlesource.com/c/go/+/518815 Auto-Submit: Russ Cox <rsc@golang.org> TryBot-Bypass: Russ Cox <rsc@golang.org>
I must say @go101's argument makes total sense. I have the following information to defend for 3-clause for loops (hereinafter "
|
It's funny. I got totally caught by surprise with this issue. Thankfully, I never actually ran into it but my assumption has always been that loop variables are closure-safe and retain their value on each iteration. IMHO if your program breaks by this change, it was buggy to begin with. So by all means, do go ahead @rsc |
Change https://go.dev/cl/532580 mentions this issue: |
As someone whose codebase contains lots and lots of for-range loops that cannot all be individually unit-tested, my concern isn't with 'contrived' code. Contrived examples are there so that effects can de demonstrated; in real life, it's usually abstraction and the escaping of variables into downstream code that produce the same problems, and are much harder to detect. And they're types of domain interaction that aren't as likely to exist within the base sdk, nor within the scope of a very focused codebase such as Encrypt's. The risk of breakage doesn't seem especially high, it's the risk of unnoticed breakage. And it's not just a matter of unit tests, just think of all the sdk unit tests that you say were shown to be wrong when adopting the change. At least as for-range loops go, it seems it would have been trivial to just keep the old code running as is by tying the new behavior to a new keyword, e.g. Otoh, I fully understand not wanting to multiply operators or keywords, lest we end up in C++ turmoil. One thing I think could help would be -as someone suggested- a migration tool that would not only show which loops would be affected by the change, but also provide a rewriting that would keep the old behaviour (by declaring vars outside the for, is that it? how does that work with for-range?). For me, that would solve the problem entirely, or almost. The last time I used the flag, I don't recall seeing anything towards that. |
@entonio Adding a new keyword like |
@entonio Last time I looked at it, the k8s code base consisted of huge quantities of for range loops. They aren't individually unit-tested either. Yet IIRC so far running the exisiting tests revealed no detectable problem. If you have followed the discussion then you might have noticed mentioning of code scans for misuse of for range variables. The following discussion should also have sufficiently answered your own argument: only contrived examples were presented, but no real code. I really don't get your writing about abstraction and skds here. To me, this doesn't bring any new reasoning here. In your logic, even the slightest change in code generation anywhere else must be dangerous, with all those abstractions and sdks? |
@entonio You may wish to read the section Transitional Support Tooling in the design doc. This describes how to get the data one needs from the compiler to create such a tool. It would be a decent amount of work, but that is the starting point. |
Not weighing in one way or another on this specific proposal, but I have an observation that may be useful. IIRC, most implementations of closures in Lisp implementations treated values of outer lexically scoped variables and references to them differently. That is to say, referencing a loop variable by value stores its current value in the closure environment when the closure is created. Referencing a loop variable by reference (which is what the current implementation seems to be doing in all cases) is storing a pointer to the variable in the closure environment. Clearly, doing the latter in a loop would make this a race-condition factory, but in cases where the closure only needs the values of an outer lexical context, it would avoid this issue completely. I don't know anything about how they are implemented in Go, nor whether such an approach is relevant or feasible. Perhaps if someone here has some insight or knows someone on the go team with more information, it might be worth considering figuring out if that might be a better and more general solution. The compiler should be able to handle the distinction between the two in the generated code, without wondering if it would break things. (Edit: It also is completely insensitive to the looping context (e.g. for/range).) |
Now that Go 1.22 is released, I think we can close this as done. |
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:
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 andTestAllEven
has returned. By the time theif
statement in the closure executes, the loop is done, andv
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: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):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 saysgo 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 ingo.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 thego
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 newloopclosure
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 likeTestAllEvenBuggy
, 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:
Building the program with
-d=loopvar=2
reports all the affected loops: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=
: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:
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.
The text was updated successfully, but these errors were encountered: