Merge cockroachdb#61608

61608: Add nilness lint pass and fix its findings r=miretskiy a=stevendanna

This adds a nilness lint pass that checks for errant uses of provably nil values.

This nilness analyzer was taken from `golang.org/x/tools` and then modified
to add checks for various `errors.Wrapf`-style functions where passing a `nil` value
can lead to a nil error to be returned when the author likely expected a non-nil error.

While most of the errors fixed here were found by the nilness linter's check for
"degenerate" if conditions, I've turned off that check for now as there are still some cases
to fix.

Release note: None

Co-authored-by: Steven Danna <[email protected]>

pkg/BUILD.bazel

@@ -265,6 +265,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/nilness:nilness_test",
     "//pkg/testutils/lint/passes/nocopy:nocopy_test",
     "//pkg/testutils/lint/passes/passesutil:passesutil_test",
     "//pkg/testutils/lint/passes/returnerrcheck:returnerrcheck_test",

pkg/cmd/roachtest/cluster.go

@@ -1698,7 +1698,7 @@ WHERE t.status NOT IN ('RANGE_CONSISTENT', 'RANGE_INDETERMINATE')`)
 	for rows.Next() {
 		var rangeID int32
 		var prettyKey, status, detail string
-		if scanErr := rows.Scan(&rangeID, &prettyKey, &status, &detail); err != nil {
+		if scanErr := rows.Scan(&rangeID, &prettyKey, &status, &detail); scanErr != nil {
 			return scanErr
 		}
 		finalErr = errors.CombineErrors(finalErr,

pkg/cmd/roachvet/BUILD.bazel

@@ -11,6 +11,7 @@ go_library(
         "//pkg/testutils/lint/passes/forbiddenmethod",
         "//pkg/testutils/lint/passes/hash",
         "//pkg/testutils/lint/passes/leaktestcall",
+        "//pkg/testutils/lint/passes/nilness",
         "//pkg/testutils/lint/passes/nocopy",
         "//pkg/testutils/lint/passes/returnerrcheck",
         "//pkg/testutils/lint/passes/timer",

pkg/cmd/roachvet/main.go

@@ -19,6 +19,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/nilness"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/nocopy"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/returnerrcheck"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/timer"
@@ -63,6 +64,7 @@ func main() {
 		unconvert.Analyzer,
 		fmtsafe.Analyzer,
 		errcmp.Analyzer,
+		nilness.CRDBAnalyzer,
 	)
 
 	// Standard go vet analyzers:

pkg/cmd/smithtest/main.go

@@ -188,9 +188,6 @@ func (s WorkerSetup) run(ctx context.Context, rnd *rand.Rand) error {
 				return errors.Wrap(err, "connector error")
 			}
 			db = gosql.OpenDB(connector)
-			if err != nil {
-				return errors.Wrap(err, "connect")
-			}
 			fmt.Println("connected to", match[1])
 			break
 		}

pkg/geo/geomfn/linestring.go

@@ -91,10 +91,6 @@ func LineLocatePoint(line geo.Geometry, point geo.Geometry) (float64, error) {
 	}
 
 	p := closestT.(*geom.Point)
-	if err != nil {
-		return 0, err
-	}
-
 	// build new line segment to the closest point we found
 	lineSegment := geom.NewLineString(geom.XY).MustSetCoords([]geom.Coord{lineString.Coord(0), p.Coords()})
 

pkg/jobs/job_scheduler.go

@@ -88,7 +88,7 @@ SELECT
 FROM %s S
 WHERE next_run < %s
 ORDER BY random()
-%s 
+%s
 FOR UPDATE`, env.SystemJobsTableName(), CreatedByScheduledJobs,
 		StatusSucceeded, StatusCanceled, StatusFailed,
 		env.ScheduledJobsTableName(), env.NowExpr(), limitClause)
@@ -313,7 +313,7 @@ func (s *jobScheduler) executeSchedules(
 			return s.processSchedule(ctx, schedule, numRunning, stats, txn)
 		}); processErr != nil {
 			if errors.HasType(processErr, (*savePointError)(nil)) {
-				return errors.Wrapf(err, "savepoint error for schedule %d", schedule.ScheduleID())
+				return errors.Wrapf(processErr, "savepoint error for schedule %d", schedule.ScheduleID())
 			}
 
 			// Failed to process schedule.

pkg/jobs/registry.go

@@ -1200,7 +1200,7 @@ func (r *Registry) stepThroughStateMachine(
 		jm.ResumeFailed.Inc(1)
 		if sErr := (*InvalidStatusError)(nil); errors.As(err, &sErr) {
 			if sErr.status != StatusCancelRequested && sErr.status != StatusPauseRequested {
-				return errors.NewAssertionErrorWithWrappedErrf(jobErr,
+				return errors.NewAssertionErrorWithWrappedErrf(sErr,
 					"job %d: unexpected status %s provided for a running job", job.ID(), sErr.status)
 			}
 			return sErr
@@ -1281,8 +1281,7 @@ func (r *Registry) stepThroughStateMachine(
 			errors.Wrapf(err, "job %d: cannot be reverted, manual cleanup may be required", job.ID()))
 	case StatusFailed:
 		if jobErr == nil {
-			return errors.NewAssertionErrorWithWrappedErrf(jobErr,
-				"job %d: has StatusFailed but no error was provided", job.ID())
+			return errors.AssertionFailedf("job %d: has StatusFailed but no error was provided", job.ID())
 		}
 		if err := job.failed(ctx, nil /* txn */, jobErr, nil /* fn */); err != nil {
 			// If we can't transactionally mark the job as failed then it will be

pkg/server/init.go

@@ -272,13 +272,11 @@ func (s *initServer) ServeAndWait(
 					return nil, false, err
 				}
 
-				if err != nil {
-					// We expect the join RPC to blindly retry on all
-					// "connection" errors save for one above. If we're
-					// here, we failed to initialize our first store after a
-					// successful join attempt.
-					return nil, false, errors.NewAssertionErrorWithWrappedErrf(err, "unexpected error: %v", err)
-				}
+				// We expect the join RPC to blindly retry on all
+				// "connection" errors save for one above. If we're
+				// here, we failed to initialize our first store after a
+				// successful join attempt.
+				return nil, false, errors.NewAssertionErrorWithWrappedErrf(err, "unexpected error: %v", err)
 			}
 
 			state := result.state

pkg/server/init_handshake.go

@@ -348,15 +348,13 @@ func (t *tlsInitHandshaker) getPeerCACert(
 		return nodeHostnameAndCA{}, err
 	}
 
-	// Read and validate server provided ack.
-	// HMAC(hostname + server CA public certificate, secretToken)
-	var msg nodeHostnameAndCA
-	if err != nil {
-		return nodeHostnameAndCA{}, err
-	}
 	if res.StatusCode != 200 {
 		return nodeHostnameAndCA{}, errors.Errorf("unexpected error returned from peer: HTTP %d", res.StatusCode)
 	}
+
+	// Read and validate server provided ack.
+	// HMAC(hostname + server CA public certificate, secretToken)
+	var msg nodeHostnameAndCA
 	if err := json.NewDecoder(res.Body).Decode(&msg); err != nil {
 		return nodeHostnameAndCA{}, err
 	}

pkg/sql/alter_sequence.go

@@ -74,9 +74,6 @@ func (n *alterSequenceNode) startExec(params runParams) error {
 	}
 	opts := desc.SequenceOpts
 	seqValueKey := params.p.ExecCfg().Codec.SequenceKey(uint32(desc.ID))
-	if err != nil {
-		return err
-	}
 
 	getSequenceValue := func() (int64, error) {
 		kv, err := params.p.txn.Get(params.ctx, seqValueKey)

pkg/sql/alter_table.go

@@ -620,9 +620,6 @@ func (n *alterTableNode) startExec(params runParams) error {
 				descriptorChanged = true
 			}
 
-			if err != nil {
-				return err
-			}
 			if err := params.p.removeColumnComment(params.ctx, n.tableDesc.ID, colToDrop.GetID()); err != nil {
 				return err
 			}

pkg/sql/opt/optbuilder/orderby.go

@@ -133,10 +133,6 @@ func (b *Builder) analyzeOrderByIndex(
 	for i, n := 0, index.KeyColumnCount(); i < n; i++ {
 		// Columns which are indexable are always orderable.
 		col := index.Column(i)
-		if err != nil {
-			panic(err)
-		}
-
 		desc := col.Descending
 
 		// DESC inverts the order of the index.

pkg/sql/pgwire/conn.go

@@ -288,7 +288,8 @@ func (c *conn) serveImpl(
 		// we only need the minimum to make pgx happy.
 		var err error
 		for param, value := range testingStatusReportParams {
-			if err := c.sendParamStatus(param, value); err != nil {
+			err = c.sendParamStatus(param, value)
+			if err != nil {
 				break
 			}
 		}

pkg/sql/sem/tree/create.go

@@ -1656,10 +1656,6 @@ func (o KVOptions) ToRoleOptions(
 				}
 			} else {
 				strFn, err := typeAsStringOrNull(ro.Value, op)
-				if err != nil {
-					return nil, err
-				}
-
 				if err != nil {
 					return nil, err
 				}

pkg/storage/cloudimpl/workload_storage.go

@@ -76,7 +76,7 @@ func makeWorkloadStorage(
 		}
 	}
 	if s.table.Name == `` {
-		return nil, errors.Wrapf(err, `unknown table %s for generator %s`, conf.Table, meta.Name)
+		return nil, errors.Errorf(`unknown table %s for generator %s`, conf.Table, meta.Name)
 	}
 	return s, nil
 }

pkg/testutils/lint/lint_test.go

@@ -2048,6 +2048,8 @@ func TestLint(t *testing.T) {
 			// roachtest is not collecting redactable logs so we don't care
 			// about printf hygiene there as much.
 			stream.GrepNot(`pkg/cmd/roachtest/log\.go:.*format argument is not a constant expression`),
+			// We purposefully produce nil dereferences in this file to test crash conditions
+			stream.GrepNot(`pkg/util/log/logcrash/crash_reporting_test\.go:.*nil dereference in type assertion`),
 		}
 
 		const vetTool = "roachvet"

pkg/testutils/lint/passes/nilness/BUILD.bazel

@@ -0,0 +1,24 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "nilness",
+    srcs = ["nilness.go"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/nilness",
+    visibility = ["//visibility:public"],
+    deps = [
+        "@org_golang_x_tools//go/analysis",
+        "@org_golang_x_tools//go/analysis/passes/buildssa",
+        "@org_golang_x_tools//go/ssa",
+    ],
+)
+
+go_test(
+    name = "nilness_test",
+    srcs = ["nilness_test.go"],
+    data = glob(["testdata/**"]),
+    tags = ["broken_in_bazel"],
+    deps = [
+        ":nilness",
+        "@org_golang_x_tools//go/analysis/analysistest",
+    ],
+)