Skip to content
New issue

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

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

Already on GitHub? # to your account

proposal: Go 2: fix range loopclosure bug #37061

Closed
carnott-snap opened this issue Feb 5, 2020 · 1 comment
Closed

proposal: Go 2: fix range loopclosure bug #37061

carnott-snap opened this issue Feb 5, 2020 · 1 comment

Comments

@carnott-snap
Copy link

carnott-snap commented Feb 5, 2020

Would you consider yourself a novice, intermediate, or experienced Go programmer?

Intermediate to experienced.

What other languages do you have experience with?

  • Java/Kotlin
  • Rust
  • JavaScript/TypeScript
  • Python
  • Haskell

Would this change make Go easier or harder to learn, and why?

This change makes intuitive assumptions about how loops, closures, and goroutines work realised. It will add hassle and complexity for experienced users, but serves to benefit beginners.

Has this idea, or one like it, been proposed before?

I could not find github issues for this proposal, however since the issue is called out in the faq, it has likely been discussed before. My goal is to suggest this as a new breaking change, enabled via the modules go directive.

Who does this proposal help, and why?

This helps novice developers by making the language work how they expect loop variables to behave.

Is this change backward compatible?

No. The vast majority of code will be compatible, since the bug causes racy, unpredictable behaviour. That being said, somebody could have written a loop that intentionally takes advantage of the current behaviour, and that will break:

for i := range [...]struct{}{{},{},{},{}} {
        if i > 1 {
                break
        }
        go func() { fmt.Println(i) }()
}

Show example code before and after the change.

// before
for i := range [...]struct{}{{},{},{},{}} {
        i := i
        go func() { fmt.Println(u) }()
}
// after
for i := range [...]struct{}{{},{},{},{}} {
        go func() { fmt.Println(u) }()
}

What is the cost of this proposal? (Every language change has a cost).

This change may cause confusion for experienced users. It will impose a migration cost for a very small portion of the community currently using this behaviour. We will need to maintain compiler logic for two looping primitives, in perpetuity.

How many tools (such as vet, gopls, gofmt, goimports, etc.) would be affected?

Vet will be allowed to sunset the loopclosure vet for new versions of Go.

What is the compile time cost?

While this will add complexity to the compiler, it can be implemented as pure syntax sugar, so it should not be too expensive. If the AST generation is changed, the cost will be effectively nothing.

What is the run time cost?

Most customers already pay the runtime cost, so effectively nothing. Customers not using closures may see the cost of an extra initialisation per loop.

Can you describe a possible implementation?

I am not familiar enough with the internals of the compiler to make a concrete suggestion, but high level:

  • desugar for k, v := range iterable
    • into for { k, v := iterable[iteration]; /* loop body */ }
    • not var k KType; var v VType; for { k, v = iterable[iteration]; /* loop body */ }

Notes:

  • k, v can be replaced with a single variable and the pattern is the same.
  • := can be replaced with = if one really does want to reuse a single value for every iteration.
    • This allows for a way to migrate existing code that abuses/requires this behaviour. Similar to map randomness.
  • iterable[iteration] is a shorthand that maps to map[key], array[index], slice[index], channel entries, etc.

Do you have a prototype?

No.

How would the language spec change?

Much like map randomness, this is an implementation detail. It should not change the spec.

Orthogonality: how does this change interact or overlap with existing features?

This change should only affect how range loops work.

Is the goal of this change a performance improvement?

No.

Does this affect error handling?

No.

Is this about generics?

No.

@gopherbot gopherbot added this to the Proposal milestone Feb 5, 2020
@ianlancetaylor
Copy link
Member

Yes, this issue already exists. This is a dup of #20733.

@golang golang locked and limited conversation to collaborators Feb 4, 2021
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

No branches or pull requests

3 participants