Merge #82324 #83214

82324: Add `loopvarcapture` linter r=srosenberg a=renatolabs

This PR introduces a new linter: `loopvarcapture`. It reports
uses of loop variables captured by reference in Go routines or `defer`
statements, a common source of data races [1].

`govet` currently has a similar linter [2]; however, that project
prioritizes having no false positives at the expense of allowing
false negatives. 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. 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.

For instance, the following code will be identified by the linter:

```go
for i, obj := range collection {
    go func() {
        // ...
        myFunc(i) // variable 'i' captured by reference
        // ...
    }()

    showProgress := func() {
        fmt.Printf("finished processing: %#v\n", obj)
    }

    go func() {
        defer showProgress() // function 'showProgress' captures loop variable 'obj' by reference
        // ...
    }()
}
```

To fix such cases, the code is expected to not use the loop variable in the
closure, as in the examples below:

```go
for i, obj := range collection {
    i := i // create copy of index variable
    go func() {
        // ...
        myFunc(i) // this is OK -- using loop's copy of the index variable
        // ...
    }()

    showProgress := func(obj MyStruct) {
        fmt.Printf("finished processing: %#v\n", obj)
    }

    go func(obj MyStruct) {
        defer showProgress(obj) // this is OK -- passing loop variable as parameter to the closure
        // ...
    }(obj)
}
```

Developers are still able to use their own judgement and disable this
linter in specific instances by using a `nolint` comment.

The commits after the first one fix violations found by this new linter in multiple
packages; all of the violations happened in test files. In most cases, the fixes
did not actually solve data races because most of our unit tests run
sequentially. There was a minor roachtest bug fix related to progress
logging.

Apologies for updating this many packages in one PR; the majority of the
changes were mechanical and don't require much review by the different teams.
Reviewing of the first commit should be mostly done by `@cockroachdb/test-eng.`

[1] A Study of Real-World Data Races in Golang: https://arxiv.org/pdf/2204.00764.pdf
[2] https://github.com/golangci/govet/blob/44ddbe260190d79165f4150b828650780405d801/rangeloop.go#L36

83214: kv: Fix test to correctly test the SystemClass r=ajwerner a=andrewbaptist

Previously this test had several issues that are all addressed.
First the system range it was writing to only had a single replica, so
even if it was correctly written to, a network partition wouldn't test
anything. Additionally, the address it was attempting to write to was
a lease change, which is not in the system range. The reason this worked
previously is that there was an early exist from the change lease code
if the old and new leaseholders were the same, so it didn't actually
attempt to write to the leaseholder name at all.

This change addresses all this, but writing to a key in the liveness
range and first replicating the liveness range to all nodes.

Release note: None

Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Andrew Baptist <[email protected]>

BUILD.bazel

@@ -189,6 +189,7 @@ nogo(
             "//pkg/testutils/lint/passes/grpcstatuswithdetails",
             "//pkg/testutils/lint/passes/hash",
             "//pkg/testutils/lint/passes/leaktestcall",
+            "//pkg/testutils/lint/passes/loopvarcapture",
             "//pkg/testutils/lint/passes/nilness",
             "//pkg/testutils/lint/passes/nocopy",
             "//pkg/testutils/lint/passes/returncheck",

pkg/BUILD.bazel

@@ -437,6 +437,7 @@ ALL_TESTS = [
     "//pkg/testutils/lint/passes/forbiddenmethod:forbiddenmethod_test",
     "//pkg/testutils/lint/passes/hash:hash_test",
     "//pkg/testutils/lint/passes/leaktestcall:leaktestcall_test",
+    "//pkg/testutils/lint/passes/loopvarcapture:loopvarcapture_test",
     "//pkg/testutils/lint/passes/nilness:nilness_test",
     "//pkg/testutils/lint/passes/nocopy:nocopy_test",
     "//pkg/testutils/lint/passes/passesutil:passesutil_test",

pkg/bench/bench_test.go

@@ -221,12 +221,12 @@ func BenchmarkTableResolution(b *testing.B) {
 	for _, createTempTables := range []bool{false, true} {
 		b.Run(fmt.Sprintf("temp_schema_exists:%t", createTempTables), func(b *testing.B) {
 			benchmarkCockroach(b, func(b *testing.B, db *sqlutils.SQLRunner) {
-				defer func() {
+				defer func(createTempTables bool) {
 					db.Exec(b, `DROP TABLE IF EXISTS bench.tbl`)
 					if createTempTables {
 						db.Exec(b, `DROP TABLE IF EXISTS bench.pg_temp.temp_tbl`)
 					}
-				}()
+				}(createTempTables)
 
 				db.Exec(b, `
 			USE bench;
@@ -301,13 +301,13 @@ func runBenchmarkInsert(b *testing.B, db *sqlutils.SQLRunner, count int) {
 func runBenchmarkInsertFK(b *testing.B, db *sqlutils.SQLRunner, count int) {
 	for _, nFks := range []int{1, 5, 10} {
 		b.Run(fmt.Sprintf("nFks=%d", nFks), func(b *testing.B) {
-			defer func() {
+			defer func(nFks int) {
 				dropStmt := "DROP TABLE IF EXISTS bench.insert"
 				for i := 0; i < nFks; i++ {
 					dropStmt += fmt.Sprintf(",bench.fk%d", i)
 				}
 				db.Exec(b, dropStmt)
-			}()
+			}(nFks)
 
 			for i := 0; i < nFks; i++ {
 				db.Exec(b, fmt.Sprintf(`CREATE TABLE bench.fk%d (k INT PRIMARY KEY)`, i))

pkg/ccl/backupccl/backup_test.go

@@ -2686,18 +2686,18 @@ CREATE TYPE d.greeting AS ENUM ('hello', 'howdy', 'hi');
 
 			// Start ALTER TYPE statement(s) that will block.
 			for _, query := range tc.queries {
-				go func(query string) {
+				go func(query string, totalQueries int) {
 					// Note we don't use sqlDB.Exec here because we can't Fatal from within a goroutine.
 					if _, err := sqlDB.DB.ExecContext(context.Background(), query); err != nil {
 						t.Error(err)
 					}
 					mu.Lock()
 					numTypeChangesFinished++
-					if numTypeChangesFinished == len(tc.queries) {
+					if numTypeChangesFinished == totalQueries {
 						close(typeChangesFinished)
 					}
 					mu.Unlock()
-				}(query)
+				}(query, len(tc.queries))
 			}
 
 			// Wait on the type changes to start.
@@ -7296,16 +7296,16 @@ func TestClientDisconnect(t *testing.T) {
 			done := make(chan struct{})
 			ctxToCancel, cancel := context.WithCancel(ctx)
 			defer cancel()
-			go func() {
+			go func(command string) {
 				defer close(done)
 				connCfg, err := pgx.ParseConfig(pgURL.String())
 				assert.NoError(t, err)
 				db, err := pgx.ConnectConfig(ctx, connCfg)
 				assert.NoError(t, err)
 				defer func() { assert.NoError(t, db.Close(ctx)) }()
-				_, err = db.Exec(ctxToCancel, testCase.jobCommand)
+				_, err = db.Exec(ctxToCancel, command)
 				assert.Equal(t, context.Canceled, errors.Unwrap(err))
-			}()
+			}(testCase.jobCommand)
 
 			// Wait for the job to start.
 			var jobID string

pkg/ccl/changefeedccl/changefeed_test.go

@@ -5913,18 +5913,18 @@ func TestChangefeedOnlyInitialScanCSV(t *testing.T) {
 						actualMessages = append(actualMessages, string(m.Value))
 					}
 				})
-				defer func() {
+				defer func(expectedPayload []string) {
 					closeFeed(t, feed)
 					sqlDB.Exec(t, `DROP TABLE foo`)
 					sqlDB.Exec(t, `DROP TABLE bar`)
 					_ = g.Wait()
-					require.Equal(t, len(testData.expectedPayload), len(actualMessages))
-					sort.Strings(testData.expectedPayload)
+					require.Equal(t, len(expectedPayload), len(actualMessages))
+					sort.Strings(expectedPayload)
 					sort.Strings(actualMessages)
-					for i := range testData.expectedPayload {
-						require.Equal(t, testData.expectedPayload[i], actualMessages[i])
+					for i := range expectedPayload {
+						require.Equal(t, expectedPayload[i], actualMessages[i])
 					}
-				}()
+				}(testData.expectedPayload)
 
 				jobFeed := feed.(cdctest.EnterpriseTestFeed)
 				require.NoError(t, jobFeed.WaitForStatus(func(s jobs.Status) bool {

pkg/ccl/multiregionccl/region_test.go

@@ -149,12 +149,12 @@ CREATE DATABASE db WITH PRIMARY REGION "us-east1" REGIONS "us-east2", "us-east3"
 CREATE TABLE db.rbr () LOCALITY REGIONAL BY ROW`)
 			require.NoError(t, err)
 
-			go func() {
-				if _, err := sqlDB.Exec(tc.firstOp); err != nil {
+			go func(firstOp string) {
+				if _, err := sqlDB.Exec(firstOp); err != nil {
 					t.Error(err)
 				}
 				close(firstOpFinished)
-			}()
+			}(tc.firstOp)
 
 			// Wait for the first operation to reach the type schema changer.
 			<-firstOpStarted
@@ -288,12 +288,12 @@ CREATE TABLE db.rbr(k INT PRIMARY KEY, v INT NOT NULL) LOCALITY REGIONAL BY ROW;
 `)
 				require.NoError(t, err)
 
-				go func() {
+				go func(cmd string, shouldSucceed bool) {
 					defer func() {
 						close(typeChangeFinished)
 					}()
-					_, err := sqlDB.Exec(regionAlterCmd.cmd)
-					if regionAlterCmd.shouldSucceed {
+					_, err := sqlDB.Exec(cmd)
+					if shouldSucceed {
 						if err != nil {
 							t.Errorf("expected success, got %v", err)
 						}
@@ -302,7 +302,7 @@ CREATE TABLE db.rbr(k INT PRIMARY KEY, v INT NOT NULL) LOCALITY REGIONAL BY ROW;
 							t.Errorf("expected error boom, found %v", err)
 						}
 					}
-				}()
+				}(regionAlterCmd.cmd, regionAlterCmd.shouldSucceed)
 
 				<-typeChangeStarted
 
@@ -405,14 +405,14 @@ CREATE TABLE db.global () LOCALITY GLOBAL;`)
 			require.NoError(t, err)
 			_, err = sqlDB.Exec(`SET CLUSTER SETTING sql.defaults.multiregion_placement_policy.enabled = true;`)
 			require.NoError(t, err)
-			go func() {
+			go func(regionOp string) {
 				defer func() {
 					close(regionOpFinished)
 				}()
 
-				_, err := sqlDB.Exec(tc.regionOp)
+				_, err := sqlDB.Exec(regionOp)
 				require.NoError(t, err)
-			}()
+			}(tc.regionOp)
 
 			<-regionOpStarted
 			_, err = sqlDB.Exec(tc.placementOp)
@@ -883,15 +883,15 @@ INSERT INTO db.rbr VALUES (1,1),(2,2),(3,3);
 `)
 				require.NoError(t, err)
 
-				go func() {
+				go func(cmd string) {
 					defer func() {
 						close(typeChangeFinished)
 					}()
-					_, err := sqlDBBackup.Exec(regionAlterCmd.cmd)
+					_, err := sqlDBBackup.Exec(cmd)
 					if err != nil {
-						t.Errorf("expected success, got %v when executing %s", err, regionAlterCmd.cmd)
+						t.Errorf("expected success, got %v when executing %s", err, cmd)
 					}
-				}()
+				}(regionAlterCmd.cmd)
 
 				<-typeChangeStarted
 

pkg/ccl/multiregionccl/regional_by_row_test.go

@@ -831,10 +831,10 @@ USE t;
 				// Perform the alter table command asynchronously; this will be interrupted.
 				rbrErrCh := make(chan error, 1)
 				performInterrupt = true
-				go func() {
-					_, err := sqlDB.Exec(rbrChange.cmd)
+				go func(cmd string) {
+					_, err := sqlDB.Exec(cmd)
 					rbrErrCh <- err
-				}()
+				}(rbrChange.cmd)
 
 				// Wait for the backfill to start.
 				<-interruptStartCh
@@ -884,10 +884,10 @@ USE t;
 				performInterrupt = true
 
 				regionChangeErr := make(chan error, 1)
-				go func() {
-					_, err := sqlDB.Exec(regionChange.cmd)
+				go func(cmd string) {
+					_, err := sqlDB.Exec(cmd)
 					regionChangeErr <- err
-				}()
+				}(regionChange.cmd)
 
 				// Wait for the enum change to start.
 				<-interruptStartCh

pkg/ccl/sqlproxyccl/conn_migration_test.go

@@ -768,12 +768,12 @@ func TestWaitForShowTransferState(t *testing.T) {
 			defer client.Close()
 
 			doneCh := make(chan struct{})
-			go func() {
-				for _, m := range tc.sendSequence {
+			go func(sequence []pgproto3.BackendMessage) {
+				for _, m := range sequence {
 					writeServerMsg(server, m)
 				}
 				close(doneCh)
-			}()
+			}(tc.sendSequence)
 
 			msgCh := make(chan pgproto3.BackendMessage, 10)
 			go func() {
@@ -968,12 +968,12 @@ func TestRunAndWaitForDeserializeSession(t *testing.T) {
 			defer serverProxy.Close()
 			defer server.Close()
 			doneCh := make(chan struct{})
-			go func() {
-				for _, m := range tc.sendSequence {
+			go func(sequence []pgproto3.BackendMessage) {
+				for _, m := range sequence {
 					writeServerMsg(server, m)
 				}
 				close(doneCh)
-			}()
+			}(tc.sendSequence)
 
 			msgCh := make(chan pgproto3.FrontendMessage, 1)
 			go func() {

pkg/cmd/roachtest/tests/cancel.go

@@ -71,8 +71,9 @@ func registerCancel(r registry.Registry) {
 				// Any error regarding the cancellation (or of its absence) will
 				// be sent on errCh.
 				errCh := make(chan error, 1)
-				go func(query string) {
+				go func(queryNum int) {
 					defer close(errCh)
+					query := tpch.QueriesByNumber[queryNum]
 					t.L().Printf("executing q%d\n", queryNum)
 					sem <- struct{}{}
 					close(sem)
@@ -87,7 +88,7 @@ func registerCancel(r registry.Registry) {
 							errCh <- errors.Wrap(err, "unexpected error")
 						}
 					}
-				}(tpch.QueriesByNumber[queryNum])
+				}(queryNum)
 
 				// Wait for the query-runner goroutine to start.
 				<-sem

pkg/cmd/roachvet/BUILD.bazel

@@ -12,6 +12,7 @@ go_library(
         "//pkg/testutils/lint/passes/forbiddenmethod",
         "//pkg/testutils/lint/passes/hash",
         "//pkg/testutils/lint/passes/leaktestcall",
+        "//pkg/testutils/lint/passes/loopvarcapture",
         "//pkg/testutils/lint/passes/nilness",
         "//pkg/testutils/lint/passes/nocopy",
         "//pkg/testutils/lint/passes/returnerrcheck",
@@ -28,7 +29,6 @@ go_library(
         "@org_golang_x_tools//go/analysis/passes/copylock",
         "@org_golang_x_tools//go/analysis/passes/errorsas",
         "@org_golang_x_tools//go/analysis/passes/httpresponse",
-        "@org_golang_x_tools//go/analysis/passes/loopclosure",
         "@org_golang_x_tools//go/analysis/passes/lostcancel",
         "@org_golang_x_tools//go/analysis/passes/nilfunc",
         "@org_golang_x_tools//go/analysis/passes/printf",

pkg/cmd/roachvet/main.go

@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/forbiddenmethod"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/hash"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/leaktestcall"
+	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/loopvarcapture"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/nilness"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/nocopy"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/returnerrcheck"
@@ -36,7 +37,6 @@ import (
 	"golang.org/x/tools/go/analysis/passes/copylock"
 	"golang.org/x/tools/go/analysis/passes/errorsas"
 	"golang.org/x/tools/go/analysis/passes/httpresponse"
-	"golang.org/x/tools/go/analysis/passes/loopclosure"
 	"golang.org/x/tools/go/analysis/passes/lostcancel"
 	"golang.org/x/tools/go/analysis/passes/nilfunc"
 	"golang.org/x/tools/go/analysis/passes/printf"
@@ -67,6 +67,7 @@ func main() {
 		errcmp.Analyzer,
 		nilness.Analyzer,
 		errwrap.Analyzer,
+		loopvarcapture.Analyzer,
 	)
 
 	// Standard go vet analyzers:
@@ -81,7 +82,8 @@ func main() {
 		copylock.Analyzer,
 		errorsas.Analyzer,
 		httpresponse.Analyzer,
-		loopclosure.Analyzer,
+		// loopclosure.Analyzer,
+		// loopclosure is superseded by 'loopvarcapture'
 		lostcancel.Analyzer,
 		nilfunc.Analyzer,
 		printf.Analyzer,

pkg/kv/client_test.go

@@ -166,9 +166,9 @@ func TestClientRetryNonTxn(t *testing.T) {
 				// We must try the non-txn put or get in a goroutine because
 				// it might have to retry and will only succeed immediately in
 				// the event we can push.
-				go func() {
+				go func(i int, args roachpb.Request) {
 					var err error
-					if _, ok := test.args.(*roachpb.GetRequest); ok {
+					if _, ok := args.(*roachpb.GetRequest); ok {
 						_, err = db.Get(nonTxnCtx, key)
 					} else {
 						err = db.Put(nonTxnCtx, key, "value")
@@ -179,8 +179,8 @@ func TestClientRetryNonTxn(t *testing.T) {
 					}
 					doneCall <- errors.Wrapf(
 						err, "%d: expected success on non-txn call to %s",
-						i, test.args.Method())
-				}()
+						i, args.Method())
+				}(i, test.args)
 				// Block until the non-transactional client has pushed us at
 				// least once.
 				testutils.SucceedsSoon(t, func() error {

pkg/kv/kvclient/rangecache/range_cache_test.go

@@ -887,19 +887,18 @@ func TestRangeCacheHandleDoubleSplit(t *testing.T) {
 					blocked = ch
 				}
 
-				go func(ctx context.Context) {
+				go func(ctx context.Context, reverseScan bool) {
 					defer wg.Done()
 					var desc *roachpb.RangeDescriptor
 					// Each request goes to a different key.
 					var err error
 					ctx, getRecAndFinish := tracing.ContextWithRecordingSpan(ctx, db.cache.tracer, "test")
 					defer getRecAndFinish()
 					tok, err := db.cache.lookupInternal(
-						ctx, key, oldToken,
-						tc.reverseScan)
+						ctx, key, oldToken, reverseScan)
 					require.NoError(t, err)
 					desc = tok.Desc()
-					if tc.reverseScan {
+					if reverseScan {
 						if !desc.ContainsKeyInverted(key) {
 							t.Errorf("desc %s does not contain exclusive end key %s", desc, key)
 						}
@@ -916,7 +915,7 @@ func TestRangeCacheHandleDoubleSplit(t *testing.T) {
 								key, expLog, rec)
 						}
 					}
-				}(ctx)
+				}(ctx, tc.reverseScan)
 
 				// If we're expecting this request to block, wait for that.
 				if blocked != nil {