This change is 

Take or leave this feedback: I wish this thing was just a tad more sophisticated: I wish it would accept it in the case where the variable is never written to again. In practice that's a tad complicated. I guess you'd want to see if the variable were captured in a different closure and to see if its address were taken and then generally looking for assignments. In the vast majority of the changes you have to make here, the variables are never changed after their initial assignment.

All that being said, nice job writing a linter. The analysis framework is cool. @cockroachdb/sql-schema code LGTM

This linter, on the other hand, represents the
opinion that loop variables should not be captured by reference in Go
routines even when it's safe to do so.

This linter's opinion does not correspond to mine :).

That behavior is confusing and
concurrency added to related code over time could lead to the
introduction of data races, potentially manifesting as bugs in the
product or flakiness in the tests. These issues are hard to debug and
take a lot of developer time.

I checked a couple of the cases touched by this PR, and in all of them adding some sort of new concurrency would cause all sorts of problems unrelated to the loop variable. It's very common to have iterations of a loop that use goroutines internally, and wait for all these goroutines to finish before moving to the next iteration. In fact, I think you'll find this is the common case. I don't want this linter in the way of this pattern.

So I for one think I'm against this linter. Tell me the numbers again: how many bugs were actually fixed here, versus how many false positives?

We already have too many linters; let's think twice before adding more.

If this does go forward after all, at the very least let's fix the "violations" by making a copy of the variable in question inside the loop and have the closure capture the copy (or by using the index-only range iteration), rather than introducing parameters to the closures.

Also, reorder the new linter commit at the end, so that the tests always pass (for bisections for example).

Take or leave this feedback: I wish this thing was just a tad more sophisticated: I wish it would accept it in the case where the variable is never written to again.

@ajwerner Not sure I understand your comment here. Why would it matter whether the variable is written to again? This linter only applies to loop variables (which are "assigned" in each iteration of the loop). As long as there's a chance that the Go routine will outlive the iteration where it was created, there's a data race.

Let me know if I'm missing the point you were making.

@andreimatei thank you for your feedback 👍 My opinion is that loop variables being captured by reference in Go routines increase the chances of data races even if it's safe to do so at the time they were introduced. At the same time, I also understand your point that the pattern of creating a Go routine and waiting for it to finish in the same iteration is a common pattern, and I didn't want this linter to get too much in the way of that.

In fact, I think you'll find this is the common case. I don't want this linter in the way of this pattern.

This is indeed the common case as I mentioned in the commit messages and PR description. The majority (if not all) of the changes here don't fix data races that exist at this point in time either because the tests run sequentially or because there's extra synchronization involved.

I wanted to point out that I saw this linter not as a guaranteed data-race detector, but as an indicator of a potentially dangerous pattern. IMO (and as substantiated by the paper linked in the PR description), this behaviour of closures capturing variables in scope by reference is not obvious to most people and lead to subtle bugs. This can happen even when it looks like we're waiting for the Go routines to finish at the end of the loop, like below:

for _, tc := range array {
  // do things
  wg.Add(len(tc.queries))
  for _, query := range tc.queries {
    go func() {
      defer wg.Done()
      doSomethingWithQuery(query)
    }()
  }

  wg.Wait()
  // etc
}

Maybe the issue is obvious when looking at the code above because we have our minds set to look for that pattern and beause the code is not complex enough; in more realistic scenarios, I think it's easy to miss the data race there.

All of that said, I'm open to hearing everyone's opinions and if most people think it's better to not have the linter, I'm fine with that as well. 👍

Your comment also gave me an idea to change the linter so that it will recognize "obvious" synchronization happening in the loop. The linter could not report issues if one of the following is true:

• the closure sends to a channel that is then read after in the same loop
• the closure calls Done on a wait group that is then Waited in the same loop.

I prototyped this change, and the vast majority of test cases changed here would not need to be changed if we implemented it like that. The linter will not be able to detect more sophisticated synchronization mechanisms, but I think this gets us a long way.

Happy to hear thoughts and opinions on this approach or on anything else here. 🙂

I'm +1 on this. Capturing loop variables is a frequent and subtle footgun, especially for newcomers to Go. The upside here definitely outweighs the downside.

Canceled.

bors r=srosenberg

Build succeeded:

• GitHub CI (Cockroach)

This seems to have triggered a false positive on a random PR in bors:
https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_UnitTests/5548391?showRootCauses=true&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true

pkg/server/api_v2_sql.go:411:59: loop variable 'stmtIdx' captured by reference, and may hold an undesirable value by the time the deferred function is called

The PR in question is:
https://reviewable.io/reviews/cockroachdb/cockroach/79663#-N0XWFS5CidRv8Q3C4t1

Line 411 is retErr = errors.Wrapf(retErr, "executing stmt %d", stmtIdx+1).

I think there's no goroutines at play there, just two nested anonymous functions. As far as I can see, at least.

The PR in question is: https://reviewable.io/reviews/cockroachdb/cockroach/79663#-N0XWFS5CidRv8Q3C4t1

Line 411 is retErr = errors.Wrapf(retErr, "executing stmt %d", stmtIdx+1).

I think there's no goroutines at play there, just two nested anonymous functions. As far as I can see, at least.

It's captured inside the defer statement which is subject to the same rules as goroutines. If this is benign, the author can suppress it via,

//nolint:loopvarcapture

But look at the code. The defer does not outlive the loop iteration; it's inside a function called inline.

But look at the code. The defer does not outlive the loop iteration; it's inside a function called inline.

That's true; this defer is always executed sequentially. @renatolabs Thoughts on whether it's easy to allow these cases? i.e., exclude defer when it's declared inside an applied closure?

Personally, I would refactor to this form,

stmtErr := func(int i) (retErr error) {
   ...
   txnRes.Statement = i
   ...
   defer func() {
        ...
        retErr = errors.Wrapf(retErr, "executing stmt %d", i)
        ...
   }()
   ...
}(stmtIdx+1)

Not only does it improve readability by removing the common subexpression (stmtIdx+1), its (operational) semantics are much more lucid, imo. It's not immediately obvious that the deeply nested defer is executed on each iteration; maybe it's just me, but I had to stare at the code for a good 30+ seconds to prove that it's free of unwanted side-effects.

I'm aware that the linter's treatment of defer can be improved in cases where it's contained in a closure within the loop; this was on my list. Thanks for reporting, I'll take a look at it soon.

Personally, I would refactor to this form,

This rewrite is worse, in my opinion. You're introducing an argument (i) for which you have to scroll down two screens to see what it's assigned to. That's part of the reason why I was against this linter - it encourages doing this, which can be harmful to readability.

@renatolabs given that this false-positive was triggered by, I believe, literally the first bors batch after merging the linter, can I convince you to revert it until you fix it (if the fix is not imminent)? The use of anonymous functions called inline for the explicit purpose of putting a defer in them is a useful and common Go idiom, and I think we definitely don't want to flag it.

The use of anonymous functions called inline for the explicit purpose of putting a defer in them is a useful and common Go idiom

Yep, that's true. For example, I just merged PR #83083 that added a dozen of those.

The use of anonymous functions called inline for the explicit purpose of putting a defer in them is a useful and common Go idiom

Yep, that's true. For example, I just merged PR #83083 that added a dozen of those.

This is a good example from that PR,

// Close releases the resources used by the IndexBackfiller.
func (ib *IndexBackfiller) Close(ctx context.Context) {
	if ib.mon != nil {
		func() {
			ib.muBoundAccount.Lock()
			defer ib.muBoundAccount.Unlock()
			ib.muBoundAccount.boundAccount.Close(ctx)
		}()
		ib.mon.Stop(ctx)
	}
}

There's a PR up to handle the situation discussed here: #83418.

Thank you!