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

proposal: cmd/vet: warn when the iterator variable on 3-clause for loops where is written to in a go statement after 1.22 #66388

Open
timothy-king opened this issue Mar 18, 2024 · 3 comments
Labels
Milestone

Comments

@timothy-king
Copy link
Contributor

Proposal Details

The proposal is to extend the loopclosure checker to additionally warn with a variables in a 3-clause for loop is written to after Go 1.22. A part of the new semantics is to copy the iterator variable at the end of the for loop. This is a READ that can race with writes from other goroutines.

Example:

for i := 0; i < N; i ++ {
  go func() {
    i *= 10  // warn "loop variable i written by func literal"
    println(i)
  }()
}

Criteria to use:

  1. There is a 3-clause for loop declaring a variable v.
  2. It is in a file with GoVersion >= 1.22.
  3. The 'last' instruction in the loop body is a go statement.
  4. The function executed is a function literal.
  5. The function literal contains an assignment (or IncDecStmt).
  6. The assignment updates the loop variable according to the following recursive definition in some left hand side:
  • Updates(id) holds when id refers to the variable,
  • Updates(e.X) holds when Updates(e),
  • Updates(e[g]) holds when Updates(e),
  • Updates(*e) holds when Updates(e),
  • and otherwise Updates(e) does not hold.

Additionally:

  • See loopclosure forEachLastStmt for the operational definition of 'last'.
  • In addition to go statements this would be reported for errgroup.Go and t.Run() when t.Parrallel is present. It will not be reported for defer statements.

To discuss: The requirements give at the moment dereference pointers. This is not sufficient to ensure a race has occurred. Just that a race is overwhelmingly likely. This seems to be in the spirit of the existing loopclosure check. Curious whether others agree or think we should not report if there is indirection.

@timothy-king
Copy link
Contributor Author

Related #66156. This is inspired by case 3.

@adonovan
Copy link
Member

This seems like an oddly specific version of a static race checker that looks for patterns of the form:

   x = f()
   go func() {
       x = g()
   }()
   use(x)

What makes go1.22 for loops special is that the use is implicit. I suppose the concern is that one might use a mutex around the first two references to x but not be cognizant of the third one.

I think we need evidence that this is a real problem: this kind of code has always been wrong, it's just the nature of the bug has changed. We may not have such evidence for a while.

@timothy-king
Copy link
Contributor Author

Waiting for evidence or clearer demand instead of speculating seems reasonable to me.

Another potential cause is transitioning an existing range loop to a 3-clause for loop when >=go1.22. That can go from safe to racing too. That is more speculation though.

@ianlancetaylor ianlancetaylor changed the title cmd/vet: warn when the iterator variable on 3-clause for loops where is written to in a go statement after 1.22 proposal: cmd/vet: warn when the iterator variable on 3-clause for loops where is written to in a go statement after 1.22 Mar 27, 2024
@gopherbot gopherbot added this to the Proposal milestone Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

3 participants