diff --git a/cmd/linters/main.go b/cmd/linters/main.go index ccc733d9574..5f81884700b 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -22,6 +22,7 @@ import ( "github.com/github/gh-aw/pkg/linters/excessivefuncparams" "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" "github.com/github/gh-aw/pkg/linters/largefunc" + "github.com/github/gh-aw/pkg/linters/manualmutexunlock" "github.com/github/gh-aw/pkg/linters/osexitinlibrary" "github.com/github/gh-aw/pkg/linters/rawloginlib" "github.com/github/gh-aw/pkg/linters/regexpcompileinfunction" @@ -36,6 +37,7 @@ func main() { excessivefuncparams.Analyzer, fileclosenotdeferred.Analyzer, largefunc.Analyzer, + manualmutexunlock.Analyzer, osexitinlibrary.Analyzer, rawloginlib.Analyzer, regexpcompileinfunction.Analyzer, diff --git a/docs/adr/34091-add-manual-mutex-unlock-linter.md b/docs/adr/34091-add-manual-mutex-unlock-linter.md new file mode 100644 index 00000000000..d45f869fedc --- /dev/null +++ b/docs/adr/34091-add-manual-mutex-unlock-linter.md @@ -0,0 +1,85 @@ +# ADR-34091: Add manual-mutex-unlock Linter + +**Date**: 2026-05-22 +**Status**: Draft +**Deciders**: Unknown + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +An automated scan of the codebase (linter-miner run #15) found 12+ instances where `sync.Mutex` / `sync.RWMutex` instances are unlocked manually instead of via `defer mu.Unlock()` — including `pkg/cli/compile_watch.go:189` (with manual unlocks at 204 and 209), multiple sites in `pkg/console/spinner.go:130-168`, and `pkg/cli/docker_images.go:149-151`. Manual unlock is a well-known Go anti-pattern: a panic or early return between `Lock()` and `Unlock()` leaves the mutex held and can deadlock the entire goroutine pool. The repository already houses a family of small, focused, in-house analyzers under `pkg/linters/` (e.g., `fileclosenotdeferred`, `largefunc`, `regexpcompileinfunction`) registered through `cmd/linters/main.go`, so the established convention is to add another analyzer rather than rely on review discipline or external tooling. + +### Decision + +We will add a new static-analysis linter, `manualmutexunlock`, that flags functions where a `sync.Mutex` or `sync.RWMutex` is unlocked via a non-deferred `Unlock()` / `RUnlock()` call. The linter lives under `pkg/linters/manualmutexunlock/`, is registered in `cmd/linters/main.go` alongside the existing analyzers, walks each `*ast.FuncDecl` body (excluding nested `*ast.FuncLit` closures to avoid false positives) tracking per-variable state keyed by `types.Object` (to correctly handle variable shadowing), and reports the lock position when a variable has a manual `Unlock()` / `RUnlock()` and no matching `defer`. Test files are excluded via the shared `pkg/linters/internal/filecheck.IsTestFile` helper. The implementation mirrors the structure of ADR-33834 (`fileclosenotdeferred`) for consistency. + +### Alternatives Considered + +#### Alternative 1: Fix the 12+ known instances and rely on review + +Patch each flagged call site to use `defer mu.Unlock()` and trust reviewers to catch new instances. Rejected because the same anti-pattern was present in 12+ independent sites across `pkg/cli/` and `pkg/console/`, indicating that human review alone has not been sufficient. A mechanical check on every PR is cheaper than reviewer attention and cannot be forgotten as the codebase grows. + +#### Alternative 2: Use a third-party linter (e.g., `gocritic`, `staticcheck`'s SA-rules) + +General-purpose Go linters offer mutex-related rules with broader coverage. Rejected to stay consistent with the project's convention of small, focused, in-house analyzers under `pkg/linters/`, each as its own package with custom logic. Pulling in an external linter for a single rule introduces a new dependency surface, inconsistent rule configuration, and noise from rules the project has not opted into. + +#### Alternative 3: Combine with a broader "deferred cleanup" linter + +Bundle this rule with `fileclosenotdeferred` and any future checks (e.g., `http.Response.Body` close, `context.CancelFunc` invocation) into one analyzer that flags any "resource acquired but cleanup not deferred" pattern. Rejected because the existing convention is one analyzer per rule, which keeps each rule's `Analyzer.Doc` URL narrow, independently disable-able, and easy to extend without coupling unrelated checks. The two linters share a near-identical structural pattern, but coupling them would make it harder to suppress one without the other. + +### Consequences + +#### Positive +- New non-deferred `Unlock()` / `RUnlock()` patterns introduced after merge are caught by `make golint-custom` and fail in CI rather than landing on `main`. +- The linter follows the same `pkg/linters//` layout, `Analyzer` shape, and `testdata` convention as the sibling analyzers (notably the sibling `fileclosenotdeferred` from ADR-33834), so contributors can extend it without learning a new pattern. +- Creates incentive to clean up the 12+ pre-existing manual-unlock sites in `pkg/cli/` and `pkg/console/` to maintain a clean linter signal. + +#### Negative +- Detection is structural and intentionally narrow: it matches only direct calls `.Lock()` / `.RLock()` / `.Unlock()` / `.RUnlock()` where ``'s static type is `sync.Mutex` / `sync.RWMutex` (or a pointer thereto). It will miss mutexes accessed via field selectors (`s.mu.Lock()`), mutexes returned from helper functions, and locks released through wrapper methods. False negatives are accepted in exchange for low false-positive rate. +- The 12+ pre-existing violations are not fixed in this PR, so the linter cannot be made a blocking CI gate without follow-up work or suppression. +- Adds one more analyzer to the registry, marginally increasing `cmd/linters/main.go` compile and run time. + +#### Neutral +- Test files are deliberately excluded via `filecheck.IsTestFile`, matching the convention used by the sibling linters. Test fixtures may legitimately unlock mutexes inline to exercise concurrency paths. +- The diagnostic reports at the lock position, not the manual-unlock position, so the warning points the reader to the place where `defer` should be inserted. +- Re-locking the same variable after a prior unlock is handled by resetting per-variable state on the new `Lock()` call, so cyclical lock/unlock patterns inside a single function are tolerated provided each lock has a matching defer. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Linter Behaviour + +1. The analyzer **MUST** be exported as `manualmutexunlock.Analyzer` with `Name` equal to `"manualmutexunlock"`. +2. The analyzer **MUST** report a diagnostic for every local variable whose static type is `sync.Mutex`, `sync.RWMutex`, or a pointer to either, when `Unlock()` or `RUnlock()` is invoked on that variable without a matching `defer` statement in the same `*ast.FuncDecl` body following a `Lock()` or `RLock()` call. +3. The analyzer **MUST NOT** report a diagnostic when the mutex variable's `Unlock()` / `RUnlock()` is invoked via a `defer` statement anywhere in the same function body for the corresponding lock acquisition. +4. The analyzer **MUST NOT** descend into nested `*ast.FuncLit` literals; closures are treated as independent scopes. +5. The analyzer **MUST NOT** report a diagnostic when the containing file is a Go test file as determined by `pkg/linters/internal/filecheck.IsTestFile`. +6. The diagnostic `Pos` **MUST** be the position of the originating `Lock()` / `RLock()` call, not the position of the manual `Unlock()` / `RUnlock()`. +7. The diagnostic `Message` **SHOULD** read `"mutex Unlock() should be deferred immediately after Lock() to prevent deadlocks on panic or early return"` so downstream tooling can match on a stable string. +8. The analyzer **MUST** declare `inspect.Analyzer` in its `Requires` list. +9. Mutex variable identity **MUST** be tracked by `types.Object` (not by identifier name) so that variable shadowing and same-named variables in different scopes are distinguished correctly. + +### Registration + +1. The analyzer **MUST** be registered in `cmd/linters/main.go` via the `multichecker.Main` argument list alongside the existing custom analyzers. +2. The package import in `cmd/linters/main.go` **MUST** use the path `github.com/github/gh-aw/pkg/linters/manualmutexunlock`. + +### Package Layout + +1. The linter source **MUST** live under `pkg/linters/manualmutexunlock/`. +2. Test fixtures **MUST** live under `pkg/linters/manualmutexunlock/testdata/src/manualmutexunlock/` and **MUST** use `// want` comments compatible with `golang.org/x/tools/go/analysis/analysistest`. +3. The test fixtures **MUST** include at least one positive case (manual `Unlock()` flagged), one positive case for `sync.RWMutex` (`RUnlock()` flagged), one negative case (`defer mu.Unlock()` not flagged), and one multi-mutex case where one mutex is correctly deferred and another is not. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26311406138) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/linters/manualmutexunlock/manualmutexunlock.go b/pkg/linters/manualmutexunlock/manualmutexunlock.go new file mode 100644 index 00000000000..f46b54dbf7d --- /dev/null +++ b/pkg/linters/manualmutexunlock/manualmutexunlock.go @@ -0,0 +1,183 @@ +// Package manualmutexunlock implements a Go analysis linter that flags +// mutex Unlock() calls that are not deferred, which can lead to deadlocks +// if a panic or early return occurs between Lock() and Unlock(). +package manualmutexunlock + +import ( + "go/ast" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + + "github.com/github/gh-aw/pkg/linters/internal/filecheck" +) + +// Analyzer is the manual-mutex-unlock analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "manualmutexunlock", + Doc: "reports mutex Unlock() calls that are not deferred", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/manualmutexunlock", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.FuncDecl)(nil), + } + + insp.Preorder(nodeFilter, func(n ast.Node) { + fn, ok := n.(*ast.FuncDecl) + if !ok || fn.Body == nil { + return + } + + pos := pass.Fset.PositionFor(fn.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + return + } + + // Track mutex variables: types.Object -> *mutexVarState (lock position, hasDefer, hasManualUnlock) + mutexVars := make(map[types.Object]*mutexVarState) + + // Walk all statements in the function body + ast.Inspect(fn.Body, func(node ast.Node) bool { + if node == nil { + return false + } + + // Do not descend into function literals — closures are independent + if _, ok := node.(*ast.FuncLit); ok { + return false + } + + // Look for mutex Lock() calls + if exprStmt, ok := node.(*ast.ExprStmt); ok { + if call, ok := exprStmt.X.(*ast.CallExpr); ok { + if obj := getLockCallObj(pass, call); obj != nil { + // If this mutex was already tracked from a prior lock on the same + // binding, report any unresolved violation before overwriting state. + if prev, exists := mutexVars[obj]; exists && prev.hasManualUnlock && !prev.hasDefer { + pass.Report(analysis.Diagnostic{ + Pos: prev.lockPos, + Message: "mutex Unlock() should be deferred immediately after Lock() to prevent deadlocks on panic or early return", + }) + } + mutexVars[obj] = &mutexVarState{ + lockPos: call.Pos(), + } + } + } + } + + // Look for defer mu.Unlock() + if deferStmt, ok := node.(*ast.DeferStmt); ok { + if obj := getUnlockCallObj(pass, deferStmt.Call); obj != nil { + if state, found := mutexVars[obj]; found { + state.hasDefer = true + } + } + } + + // Look for non-deferred mu.Unlock() in expression statements + if exprStmt, ok := node.(*ast.ExprStmt); ok { + if call, ok := exprStmt.X.(*ast.CallExpr); ok { + if obj := getUnlockCallObj(pass, call); obj != nil { + if state, found := mutexVars[obj]; found { + state.hasManualUnlock = true + } + } + } + } + + return true + }) + + // Report mutexes with manual unlock but no defer + for _, state := range mutexVars { + if state.hasManualUnlock && !state.hasDefer { + pass.Report(analysis.Diagnostic{ + Pos: state.lockPos, + Message: "mutex Unlock() should be deferred immediately after Lock() to prevent deadlocks on panic or early return", + }) + } + } + }) + + return nil, nil +} + +type mutexVarState struct { + lockPos token.Pos + hasDefer bool + hasManualUnlock bool +} + +// getLockCallObj returns the types.Object for the receiver if call is like mu.Lock() or mu.RLock() +func getLockCallObj(pass *analysis.Pass, call *ast.CallExpr) types.Object { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return nil + } + if sel.Sel.Name != "Lock" && sel.Sel.Name != "RLock" { + return nil + } + return getMutexReceiverObj(pass, sel.X) +} + +// getUnlockCallObj returns the types.Object for the receiver if call is like mu.Unlock() or mu.RUnlock() +func getUnlockCallObj(pass *analysis.Pass, call *ast.CallExpr) types.Object { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return nil + } + if sel.Sel.Name != "Unlock" && sel.Sel.Name != "RUnlock" { + return nil + } + return getMutexReceiverObj(pass, sel.X) +} + +func getMutexReceiverObj(pass *analysis.Pass, recv ast.Expr) types.Object { + if !isMutexType(pass.TypesInfo.TypeOf(recv)) { + return nil + } + + switch r := recv.(type) { + case *ast.Ident: + return pass.TypesInfo.ObjectOf(r) + case *ast.SelectorExpr: + if sel := pass.TypesInfo.Selections[r]; sel != nil { + return sel.Obj() + } + } + return nil +} + +// isMutexType returns true if t is sync.Mutex, sync.RWMutex, or a pointer to one +func isMutexType(t types.Type) bool { + if t == nil { + return false + } + + // Handle pointer types + if ptr, ok := t.(*types.Pointer); ok { + t = ptr.Elem() + } + + named, ok := t.(*types.Named) + if !ok { + return false + } + + obj := named.Obj() + if obj == nil || obj.Pkg() == nil { + return false + } + + return obj.Pkg().Path() == "sync" && (obj.Name() == "Mutex" || obj.Name() == "RWMutex") +} diff --git a/pkg/linters/manualmutexunlock/manualmutexunlock_test.go b/pkg/linters/manualmutexunlock/manualmutexunlock_test.go new file mode 100644 index 00000000000..b9841c1c3d3 --- /dev/null +++ b/pkg/linters/manualmutexunlock/manualmutexunlock_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package manualmutexunlock_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/manualmutexunlock" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, manualmutexunlock.Analyzer, "manualmutexunlock") +} diff --git a/pkg/linters/manualmutexunlock/testdata/src/manualmutexunlock/manualmutexunlock.go b/pkg/linters/manualmutexunlock/testdata/src/manualmutexunlock/manualmutexunlock.go new file mode 100644 index 00000000000..8e323647052 --- /dev/null +++ b/pkg/linters/manualmutexunlock/testdata/src/manualmutexunlock/manualmutexunlock.go @@ -0,0 +1,114 @@ +package manualmutexunlock + +import ( + "sync" +) + +// Correct: defer unlock immediately after lock +func GoodMutexPattern() { + var mu sync.Mutex + mu.Lock() + defer mu.Unlock() + + // ... do work ... +} + +// Wrong: manual unlock without defer - should be flagged +func BadMutexPattern() { + var mu sync.Mutex + mu.Lock() // want `mutex Unlock\(\) should be deferred immediately after Lock\(\) to prevent deadlocks on panic or early return` + + // ... do work ... + + mu.Unlock() +} + +// Correct: RWMutex with defer +func GoodRWMutexPattern() { + var mu sync.RWMutex + mu.RLock() + defer mu.RUnlock() + + // ... do work ... +} + +// Wrong: RWMutex without defer - should be flagged +func BadRWMutexPattern() { + var mu sync.RWMutex + mu.RLock() // want `mutex Unlock\(\) should be deferred immediately after Lock\(\) to prevent deadlocks on panic or early return` + + // ... do work ... + + mu.RUnlock() +} + +// Wrong: write lock without defer - should be flagged +func BadRWMutexWriteLock() { + var mu sync.RWMutex + mu.Lock() // want `mutex Unlock\(\) should be deferred immediately after Lock\(\) to prevent deadlocks on panic or early return` + + // ... do work ... + + mu.Unlock() +} + +// Correct: nested function with defer +func GoodNestedPattern() { + var mu sync.Mutex + mu.Lock() + defer mu.Unlock() + + func() { + // This is a closure, analyzed separately + }() +} + +// Correct: multiple locks with defers +func GoodMultipleMutexes() { + var mu1 sync.Mutex + var mu2 sync.Mutex + + mu1.Lock() + defer mu1.Unlock() + + mu2.Lock() + defer mu2.Unlock() + + // ... do work ... +} + +// Wrong: multiple locks, one without defer +func BadMultipleMutexes() { + var mu1 sync.Mutex + var mu2 sync.Mutex + + mu1.Lock() + defer mu1.Unlock() + + mu2.Lock() // want `mutex Unlock\(\) should be deferred immediately after Lock\(\) to prevent deadlocks on panic or early return` + + // ... do work ... + + mu2.Unlock() +} + +type guarded struct { + mu sync.Mutex +} + +// Wrong: selector-based mutex receiver without defer - should be flagged +func BadSelectorPattern() { + var g guarded + g.mu.Lock() // want `mutex Unlock\(\) should be deferred immediately after Lock\(\) to prevent deadlocks on panic or early return` + g.mu.Unlock() +} + +// Wrong: repeated lock on same mutex should still report earlier unresolved violation +func BadRepeatedLockBeforeGood() { + var mu sync.Mutex + mu.Lock() // want `mutex Unlock\(\) should be deferred immediately after Lock\(\) to prevent deadlocks on panic or early return` + mu.Unlock() + + mu.Lock() + defer mu.Unlock() +}