lint: add new deferunlocktest pass

This commit adds a new pass which checks for
`...Unlock()` expressions without `defer` which
could result in a lock being held indefinitely.

Resolves #105366

Release note: None

Author: Santamaura

BUILD.bazel

@@ -186,6 +186,7 @@ nogo(
             "@org_golang_x_tools//go/analysis/passes/unreachable:go_default_library",
             "@org_golang_x_tools//go/analysis/passes/unsafeptr:go_default_library",
             "@org_golang_x_tools//go/analysis/passes/unusedresult:go_default_library",
+            "//pkg/testutils/lint/passes/deferunlockcheck",
             "//pkg/testutils/lint/passes/descriptormarshal",
             "//pkg/testutils/lint/passes/errcheck",
             "//pkg/testutils/lint/passes/errcmp",

build/bazelutil/nogo_config.json

@@ -28,6 +28,12 @@
             "_test\\.go$": "tests"
         }
     },
+    "deferunlockcheck": {
+        "only_files": {
+            "cockroach/pkg/.*$": "first-party code",
+            "cockroach/bazel-out/.*/bin/pkg/.*$": "first-party code"
+        }
+    },
     "errcheck": {
         "exclude_files": {
             "pkg/.*\\.eg\\.go$": "generated code",

pkg/BUILD.bazel

@@ -593,6 +593,7 @@ ALL_TESTS = [
     "//pkg/testutils/fingerprintutils:fingerprintutils_test",
     "//pkg/testutils/floatcmp:floatcmp_test",
     "//pkg/testutils/keysutils:keysutils_test",
+    "//pkg/testutils/lint/passes/deferunlockcheck:deferunlockcheck_test",
     "//pkg/testutils/lint/passes/errcmp:errcmp_test",
     "//pkg/testutils/lint/passes/errwrap:errwrap_test",
     "//pkg/testutils/lint/passes/fmtsafe:fmtsafe_test",
@@ -2167,6 +2168,8 @@ GO_TARGETS = [
     "//pkg/testutils/keysutils:keysutils",
     "//pkg/testutils/keysutils:keysutils_test",
     "//pkg/testutils/kvclientutils:kvclientutils",
+    "//pkg/testutils/lint/passes/deferunlockcheck:deferunlockcheck",
+    "//pkg/testutils/lint/passes/deferunlockcheck:deferunlockcheck_test",
     "//pkg/testutils/lint/passes/descriptormarshal:descriptormarshal",
     "//pkg/testutils/lint/passes/errcheck:errcheck",
     "//pkg/testutils/lint/passes/errcmp:errcmp",

pkg/cmd/roachvet/BUILD.bazel

@@ -6,6 +6,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/cmd/roachvet",
     visibility = ["//visibility:private"],
     deps = [
+        "//pkg/testutils/lint/passes/deferunlockcheck",
         "//pkg/testutils/lint/passes/errcmp",
         "//pkg/testutils/lint/passes/errwrap",
         "//pkg/testutils/lint/passes/fmtsafe",

pkg/cmd/roachvet/main.go

@@ -14,6 +14,7 @@
 package main
 
 import (
+	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/deferunlockcheck"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/errcmp"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/errwrap"
 	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/fmtsafe"
@@ -70,6 +71,7 @@ func main() {
 		nilness.Analyzer,
 		errwrap.Analyzer,
 		loopvarcapture.Analyzer,
+		deferunlockcheck.Analyzer,
 	)
 
 	// Standard go vet analyzers:

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

@@ -0,0 +1,30 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "deferunlockcheck",
+    srcs = ["deferunlockcheck.go"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/deferunlockcheck",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/testutils/lint/passes/passesutil",
+        "@org_golang_x_tools//go/analysis",
+        "@org_golang_x_tools//go/analysis/passes/inspect",
+        "@org_golang_x_tools//go/ast/inspector",
+    ],
+)
+
+go_test(
+    name = "deferunlockcheck_test",
+    srcs = ["deferunlockcheck_test.go"],
+    args = ["-test.timeout=295s"],
+    data = glob(["testdata/**"]) + [
+        "@go_sdk//:files",
+    ],
+    deps = [
+        ":deferunlockcheck",
+        "//pkg/build/bazel",
+        "//pkg/testutils/datapathutils",
+        "//pkg/testutils/skip",
+        "@org_golang_x_tools//go/analysis/analysistest",
+    ],
+)

pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go

@@ -0,0 +1,59 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package deferunlockcheck
+
+import (
+	"go/ast"
+
+	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/passesutil"
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+// Doc documents this pass.
+const Doc = "checks that usages of mutex Unlock() are deferred."
+
+const noLintName = "deferunlock"
+
+// Analyzer is an analysis pass that checks for mutex unlocks which
+// aren't deferred.
+var Analyzer = &analysis.Analyzer{
+	Name:     "deferunlockcheck",
+	Doc:      Doc,
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	astInspector := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	// Note: defer ...Unlock() expressions are captured as ast.DeferStmt nodes so we don't have to worry about those results returning
+	// for ast.ExprStmt nodes.
+	filter := []ast.Node{
+		(*ast.ExprStmt)(nil),
+	}
+	astInspector.Preorder(filter, func(n ast.Node) {
+		stmt := n.(*ast.ExprStmt)
+		expr, ok := stmt.X.(*ast.CallExpr)
+		if !ok {
+			return
+		}
+		sel, ok := expr.Fun.(*ast.SelectorExpr)
+		if !ok {
+			return
+		}
+		if sel.Sel != nil && (sel.Sel.Name == "Unlock" || sel.Sel.Name == "RUnlock") && !passesutil.HasNolintComment(pass, n, noLintName) {
+			pass.Reportf(sel.Pos(), "Mutex %s not deferred", sel.Sel.Name)
+		}
+	})
+
+	return nil, nil
+}

pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck_test.go

@@ -0,0 +1,34 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package deferunlockcheck_test
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/build/bazel"
+	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/deferunlockcheck"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func init() {
+	if bazel.BuiltWithBazel() {
+		bazel.SetGoEnv()
+	}
+}
+
+func Test(t *testing.T) {
+	skip.UnderStress(t)
+	testdata := datapathutils.TestDataPath(t)
+	analysistest.TestData = func() string { return testdata }
+	analysistest.Run(t, testdata, deferunlockcheck.Analyzer, "a")
+}

pkg/testutils/lint/passes/deferunlockcheck/testdata/src/a/a.go

@@ -0,0 +1,58 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package a
+
+import (
+	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
+)
+
+type TestUnlockLint struct {
+	mu struct {
+		syncutil.Mutex
+	}
+}
+
+func init() {
+	var mut syncutil.Mutex
+	var rwmut syncutil.RWMutex
+	testUnlock := &TestUnlockLint{}
+
+	// Test the main use case.
+	// Should only capture Unlock()
+	testUnlock.mu.Lock()
+	testUnlock.mu.Unlock() // want `Mutex Unlock not deferred`
+	// This should pass.
+	defer testUnlock.mu.Unlock()
+
+	// Test within a function.
+	okFn := func() {
+		testUnlock.mu.Lock()
+		defer testUnlock.mu.Unlock()
+	}
+	failFn := func() {
+		testUnlock.mu.Lock()
+		testUnlock.mu.Unlock() // want `Mutex Unlock not deferred`
+	}
+	okFn()
+	failFn()
+
+	// Test mut variation.
+	defer mut.Unlock()
+	mut.Unlock() // want `Mutex Unlock not deferred`
+
+	// Test RUnlock
+	defer rwmut.RUnlock()
+	rwmut.RUnlock() // want `Mutex RUnlock not deferred`
+
+	// Test the no lint rule.
+	// nolint:deferunlock
+	testUnlock.mu.Unlock()
+}

pkg/testutils/lint/passes/deferunlockcheck/testdata/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync.go

@@ -0,0 +1,21 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package syncutil
+
+import "sync"
+
+type Mutex struct {
+	sync.Mutex
+}
+
+type RWMutex struct {
+	sync.RWMutex
+}