From 3904accf4c02c38d6aab8bf0ce700cb811a8eaa7 Mon Sep 17 00:00:00 2001 From: Alex Santamaura Date: Thu, 10 Aug 2023 17:00:04 -0400 Subject: [PATCH] lint: add new deferunlockcheck pass This commit updates the previous iteration of the linter to only trigger the report if we exceed a certain number of manual unlocks. This is due to the fact that while some cases of manual unlocks are ok (and are fine passing via the nolint) there are others which should be fixed. Discerning which of those that need to be fixed and then fixing them will require considerable effort and will likely not be completed in the short term. Having the linter report over a number of manual locks will enforce future behaviour to either defer unlocks or apply the nolint. Release note: None --- pkg/BUILD.bazel | 2 - .../lint/passes/deferunlockcheck/BUILD.bazel | 18 +----- .../deferunlockcheck/deferunlockcheck.go | 14 ++++- .../deferunlockcheck/deferunlockcheck_test.go | 34 ----------- .../deferunlockcheck/testdata/src/a/a.go | 58 ------------------- .../cockroach/pkg/util/syncutil/mutex_sync.go | 21 ------- 6 files changed, 14 insertions(+), 133 deletions(-) delete mode 100644 pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck_test.go delete mode 100644 pkg/testutils/lint/passes/deferunlockcheck/testdata/src/a/a.go delete mode 100644 pkg/testutils/lint/passes/deferunlockcheck/testdata/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync.go diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 01bcb8c767fb..91108241de2e 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -589,7 +589,6 @@ 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", @@ -2151,7 +2150,6 @@ GO_TARGETS = [ "//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", diff --git a/pkg/testutils/lint/passes/deferunlockcheck/BUILD.bazel b/pkg/testutils/lint/passes/deferunlockcheck/BUILD.bazel index 96831507e7ff..990050b47da8 100644 --- a/pkg/testutils/lint/passes/deferunlockcheck/BUILD.bazel +++ b/pkg/testutils/lint/passes/deferunlockcheck/BUILD.bazel @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") +load("@io_bazel_rules_go//go:def.bzl", "go_library") go_library( name = "deferunlockcheck", @@ -12,19 +12,3 @@ go_library( "@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", - ], -) diff --git a/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go b/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go index 4b04889d32f7..169ac4db1b3e 100644 --- a/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go +++ b/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck.go @@ -24,6 +24,9 @@ const Doc = "checks that usages of mutex Unlock() are deferred." const noLintName = "deferunlock" +// Please try to avoid updating this value, use the nolint comment instead. +const unlocksLimit = 1037 + // Analyzer is an analysis pass that checks for mutex unlocks which // aren't deferred. var Analyzer = &analysis.Analyzer{ @@ -40,6 +43,7 @@ func run(pass *analysis.Pass) (interface{}, error) { filter := []ast.Node{ (*ast.ExprStmt)(nil), } + var unlocksFound int astInspector.Preorder(filter, func(n ast.Node) { stmt := n.(*ast.ExprStmt) expr, ok := stmt.X.(*ast.CallExpr) @@ -50,8 +54,16 @@ func run(pass *analysis.Pass) (interface{}, error) { if !ok { return } + // Increase the count if we found a violation. 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) + unlocksFound++ + } + // If we exceed the limit then the reports trigger. + if unlocksFound > unlocksLimit { + pass.Report(analysis.Diagnostic{ + Pos: n.Pos(), + Message: "Too many Unlocks() not being deferred, check code changes (//nolint:deferunlock to pass)", + }) } }) diff --git a/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck_test.go b/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck_test.go deleted file mode 100644 index ccdabf5b9e88..000000000000 --- a/pkg/testutils/lint/passes/deferunlockcheck/deferunlockcheck_test.go +++ /dev/null @@ -1,34 +0,0 @@ -// 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") -} diff --git a/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/a/a.go b/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/a/a.go deleted file mode 100644 index 0297007d25d7..000000000000 --- a/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/a/a.go +++ /dev/null @@ -1,58 +0,0 @@ -// 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() -} diff --git a/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync.go b/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync.go deleted file mode 100644 index a7066e32dad3..000000000000 --- a/pkg/testutils/lint/passes/deferunlockcheck/testdata/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync.go +++ /dev/null @@ -1,21 +0,0 @@ -// 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 -}