[linter-miner] Add manual-mutex-unlock linter to detect non-deferred mutex unlocks#34091
Conversation
Reports mutex Unlock() calls that are not deferred, which can lead to deadlocks if a panic or early return occurs between Lock() and Unlock(). Evidence: 12+ instances found in codebase including: - pkg/cli/compile_watch.go:189 (manual unlocks at lines 204, 209) - pkg/console/spinner.go:130-138, 159-168 - pkg/cli/docker_images.go:149-151 The linter detects both sync.Mutex and sync.RWMutex violations and provides clear diagnostics for manual unlock patterns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. Code review completed - comprehensive analysis done, no blocking issues found |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom Go go/analysis linter (manualmutexunlock) to the project’s linter runner to detect mutex unlock patterns that are not deferred, with accompanying analysistest-based fixtures/tests.
Changes:
- Added
pkg/linters/manualmutexunlockanalyzer implementation and unit tests. - Added analysistest fixtures under
pkg/linters/manualmutexunlock/testdata. - Registered the analyzer in
cmd/linters/main.go.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/manualmutexunlock/manualmutexunlock.go | New analyzer that tracks mutex Lock/RLock and flags manual Unlock/RUnlock without a defer. |
| pkg/linters/manualmutexunlock/manualmutexunlock_test.go | Runs the analyzer via analysistest.Run. |
| pkg/linters/manualmutexunlock/testdata/src/manualmutexunlock/manualmutexunlock.go | Test fixtures covering good/bad lock/unlock patterns. |
| cmd/linters/main.go | Registers the new analyzer with the multichecker. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/linters/manualmutexunlock/manualmutexunlock.go:173
- getLockCallObj/getUnlockCallObj only handle receivers that are *ast.Ident (e.g.,
mu.Lock()), so the analyzer will miss common patterns likes.mu.Lock()/pullState.mu.Lock()where the receiver expression is a selector. Consider extracting the underlying types.Object from selector receivers too (e.g., via pass.TypesInfo.Selections fors.mu, or by handling *ast.SelectorExpr/*ast.StarExpr recursively) so the linter actually covers struct fields and package-level vars used via selectors.
// 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
}
ident, ok := sel.X.(*ast.Ident)
if !ok {
return nil
}
// Check if the receiver is a mutex type
obj := pass.TypesInfo.ObjectOf(ident)
if obj == nil {
return nil
}
if !isMutexType(pass.TypesInfo.TypeOf(ident)) {
return nil
}
return obj
}
// 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
}
ident, ok := sel.X.(*ast.Ident)
if !ok {
return nil
}
// Check if the receiver is a mutex type
obj := pass.TypesInfo.ObjectOf(ident)
if obj == nil {
return nil
}
if !isMutexType(pass.TypesInfo.TypeOf(ident)) {
return nil
}
return obj
}
pkg/linters/manualmutexunlock/manualmutexunlock.go:108
- The diagnostic message hard-codes "Unlock()"/"Lock()" even when the pattern is
RLock()/RUnlock(), which makes reports confusing for RWMutex read locks. Consider making the message generic (lock/unlock) or selecting wording based on whether the call was Lock vs RLock so it matches the actual API being used.
// 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",
})
}
- Files reviewed: 4/4 changed files
- Comments generated: 2
| // New lock call - initialize or update state | ||
| if _, exists := mutexVars[obj]; !exists { | ||
| mutexVars[obj] = &mutexVarState{ | ||
| lockPos: call.Pos(), | ||
| } | ||
| } else { | ||
| // Reset state for new lock on same variable | ||
| mutexVars[obj] = &mutexVarState{ | ||
| lockPos: call.Pos(), | ||
| } | ||
| } |
| // ... do work ... | ||
|
|
||
| mu2.Unlock() | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — requesting changes to improve test coverage and diagnostic quality.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps ([/tdd]): Missing edge cases for conditional unlocks, struct field mutexes, and re-lock patterns
- Scope documentation ([/zoom-out]): Unclear whether struct field mutexes (
s.mu.Lock()) are intentionally excluded - Diagnostic positioning ([/zoom-out]): Error messages point to
Lock()lines instead of the problematicUnlock()calls
Positive Highlights
✅ Clean implementation: Type-safe tracking using types.Object for mutex variables
✅ Good coverage of basics: Tests validate core patterns (Mutex, RWMutex, multiple mutexes)
✅ Proper integration: Correctly registered in cmd/linters/main.go and follows linter conventions
✅ Clear documentation: Package comment explains the safety concern
🔍 Review Summary by Skill
[/tdd] Test-Driven Development
The test suite covers happy and unhappy paths for simple mutex patterns, but misses critical edge cases:
- Missing: Conditional unlock scenarios (defer declared after a conditional unlock path)
- Missing: Re-lock patterns (multiple
Lock()calls on the same variable) - Missing: Struct field mutexes (
s.mu.Lock()) — these appear in the PR's evidence section - Missing: Goroutine independence tests
Without these tests, it's unclear whether the linter handles real-world patterns or only synthetic examples.
[/zoom-out] Architectural Context
The linter only handles direct variable references (mu.Lock()). From the PR description, the codebase has patterns like:
pkg/cli/compile_watch.go:189—debounceMu.Lock()pkg/console/spinner.go:130-138— likely struct field mutexes
Current implementation skips s.mu, (*ptr), and receiver mutexes due to:
ident, ok := sel.X.(*ast.Ident) // Only matches simple identifiersThis scope limitation should be either:
- Documented explicitly in tests (add test cases showing what's excluded)
- Expanded to handle struct field mutexes (more valuable but more work)
User experience concern: Diagnostics point to Lock() lines, but developers need to fix Unlock() lines. Consider tracking unlock positions for better error messages.
Next Steps: Address test coverage gaps (especially struct field mutexes and conditional unlocks) and consider improving diagnostic positioning. These changes will make the linter more robust and developer-friendly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 735.2K
| } | ||
|
|
||
| // Look for mutex Lock() calls | ||
| if exprStmt, ok := node.(*ast.ExprStmt); ok { |
There was a problem hiding this comment.
[/tdd] Missing edge case: nested if or switch statements with conditional unlocks.
💡 Why this matters
The current implementation only tracks whether a mutex has any manual unlock and any defer. It doesn't verify that the defer comes before the manual unlock in execution order.
Consider this false negative:
func conditional() {
var mu sync.Mutex
mu.Lock()
if someCondition {
mu.Unlock() // Manual unlock in branch
return
}
defer mu.Unlock() // Defer comes after conditional unlock
// ... work ...
}The linter won't flag this because hasDefer = true and hasManualUnlock = true, but the defer doesn't protect the early return path.
Suggestion: Add a test case for conditional unlocks with early returns to document current behavior or enhance the linter to detect this pattern.
| // ... do work ... | ||
| } | ||
|
|
||
| // Wrong: multiple locks, one without defer |
There was a problem hiding this comment.
[/tdd] Add test case: re-locking the same mutex should reset tracking state.
💡 Re-lock pattern test
The implementation resets mutexVars[obj] when it encounters a new Lock() on the same variable (lines 68-71 in the main file). This behavior should have explicit test coverage:
// Correct: re-lock with defer after first unlock
func GoodRelockPattern() {
var mu sync.Mutex
mu.Lock()
defer mu.Unlock()
// ... work ...
mu.Unlock()
mu.Lock() // Second lock — should not be flagged if followed by defer
defer mu.Unlock()
// ... more work ...
}
// Wrong: second lock without defer
func BadRelockPattern() {
var mu sync.Mutex
mu.Lock()
defer mu.Unlock()
mu.Unlock()
mu.Lock() // want `mutex Unlock\(\) should be deferred...`
// ... work ...
mu.Unlock()
}Without this test, the re-lock reset logic is untested.
| // 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 { |
There was a problem hiding this comment.
[/zoom-out] Potential false negatives: pointer receivers and struct field mutexes.
💡 Broader pattern analysis
The linter only handles direct variable references (mu.Lock()). Real-world mutex usage often involves:
- Struct field mutexes:
s.mu.Lock()/s.mu.Unlock() - Pointer indirection:
(*mutexPtr).Lock() - Method receivers:
func (s *Server) handler() { s.mu.Lock(); ... }
From the PR description, the codebase has patterns like:
pkg/cli/compile_watch.go:189—debounceMu.Lock()pkg/console/spinner.go:130-138— likely struct field mutexes
Current implementation:
ident, ok := sel.X.(*ast.Ident) // Only matches simple identifiers
if !ok {
return nil // Skips s.mu, (*ptr), etc.
}Suggestion: Add test cases for these patterns to document what's in scope vs. explicitly out of scope for this linter.
|
|
||
| return true | ||
| }) | ||
|
|
There was a problem hiding this comment.
[/zoom-out] Reporting loop loses individual unlock positions — diagnostics point to Lock() instead of Unlock().
💡 User experience concern
When the linter reports a diagnostic, it uses state.lockPos (the position of Lock()), not the position of the problematic Unlock() call:
pass.Report(analysis.Diagnostic{
Pos: state.lockPos, // Points to mu.Lock()
Message: "mutex Unlock() should be deferred...",
})Developer experience issue: The error points to the Lock() line, but the fix is at the Unlock() line. Developers expect the diagnostic to point where the action is needed.
Example output:
file.go:42: mutex Unlock() should be deferred...
But line 42 is mu.Lock() — the unlock is on line 50.
Suggestion: Track unlockPos in mutexVarState and report diagnostics at the unlock location, or report both positions in the message:
Message: fmt.Sprintf("mutex Unlock() at line %d should be deferred (Lock at line %d)", unlockLine, lockLine),| "github.com/github/gh-aw/pkg/linters/manualmutexunlock" | ||
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] Test suite lacks negative cases and edge case coverage.
💡 Missing test scenarios
The test relies entirely on analysistest.Run() with fixture files. Strong test coverage should include:
- Struct field mutexes:
type Server struct { mu sync.Mutex }→s.mu.Lock(); s.mu.Unlock() - Pointer receivers: Methods with
func (s *Server) handle() { s.mu.Lock(); ... } - Conditional unlock edge case: Defer declared after a conditional manual unlock
- Re-lock patterns: Multiple
Lock()calls on the same variable in one function - Goroutines:
go func() { mu.Lock(); defer mu.Unlock() }()should not interfere
Current test file only validates:
- ✅ Basic good/bad patterns
- ✅ RWMutex variants
- ✅ Multiple mutexes
Suggestion: Add test cases for the patterns listed above, especially struct field mutexes which appear in the PR's evidence section.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (306 new lines under 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References:
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Test Coverage AnalysisThe test uses the
Coverage assessment: ✅ Comprehensive
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. Why This Test Scores 100/100
References:
|
|
|
|
|
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — I reviewed and addressed the actionable review feedback in commit
|
Summary
Adds a new Go static analysis linter
manualmutexunlockthat detectssync.Mutexandsync.RWMutexunlock calls not protected bydefer, reducing the risk of lock leaks from early returns or panics.Changes
Added Files
types.Objectto identify manual unlock callsgolang.org/x/tools/go/analysis/analysistestsync.Mutexandsync.RWMutexModified Files
manualmutexunlockanalyzer in the linter registryTechnical Details
Implementation Approach
types.Objectidentity to track mutex variables across lock/unlock callsast.DeferStmtinspectionsync.Mutexandsync.RWMutextypes (Lock/Unlock and RLock/RUnlock)obj.mu.Unlock()) to correctly identify the mutex variableDiagnostic Output
Reports violations with position information pointing to non-deferred unlock calls, enabling integration with standard Go tooling workflows.
Impact Assessment
Commit History
703236018— Merge branch 'main' into linter-miner/manual-mutex-unlock-d7cb2952dc6202d11484c62ed— Fix manualmutexunlock state overwrite and add selector fixturesc6ab3cb67— Add draft ADR-34091 for manual-mutex-unlock linter4599e2c4c— Add manual-mutex-unlock linter to detect non-deferred mutex unlocksFiles Changed