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

[timothy-king]

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]

Related #66156. This is inspired by case 3.

[adonovan]

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]

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.

[zigo101]

@adonovan

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.

This is not always true. This kind of code might be correct before Go 1.22 if synchronizations are done well for all the 3 places. But since Go 1.22, it becomes impossible to do synchronization for the third implicit use(x). (And before Go 1.22, the third implicit use(x) often doesn't exist at all.)