feat(linters): add nilctxpassed analyzer#45799
Conversation
Adds a new nilctxpassed analyzer that detects function calls where nil is passed as a context.Context argument. The correct idioms are context.Background() or context.TODO() instead. - pkg/linters/nilctxpassed/nilctxpassed.go: analyzer implementation - pkg/linters/nilctxpassed/nilctxpassed_test.go: analysistest-based test - pkg/linters/nilctxpassed/testdata/src/nilctxpassed/nilctxpassed.go: fixture - cmd/linters/main.go: register in multichecker binary - pkg/linters/README.md: documentation entry - pkg/linters/doc.go: update count (44 → 45) and list - pkg/linters/spec_test.go: register in documented analyzers Closes #45795 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new Go go/analysis analyzer (nilctxpassed) to detect calls that pass the predeclared nil to parameters typed as context.Context, and wires it into the linter binary and public documentation/spec.
Changes:
- Introduces
pkg/linters/nilctxpassedanalyzer and ananalysistestsuite with fixture code. - Registers the analyzer in
cmd/lintersmultichecker. - Updates linter package documentation and spec assertions to include the new analyzer.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/nilctxpassed/nilctxpassed.go | Implements the nilctxpassed analyzer logic and diagnostics |
| pkg/linters/nilctxpassed/nilctxpassed_test.go | Adds analysistest harness for the new analyzer |
| pkg/linters/nilctxpassed/testdata/src/nilctxpassed/nilctxpassed.go | Provides fixture cases for diagnostics and suppression |
| cmd/linters/main.go | Registers nilctxpassed in the multichecker driver |
| pkg/linters/README.md | Documents the new analyzer in the public subpackages list/table |
| pkg/linters/doc.go | Updates overall analyzer list/count and includes nilctxpassed |
| pkg/linters/spec_test.go | Updates spec expectations to include the new documented analyzer |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/8 changed files
- Comments generated: 2
- Review effort level: Low
| ctxType := astutil.ContextContextType(pass) | ||
| if ctxType == nil { | ||
| return nil, nil | ||
| } |
| package nilctxpassed | ||
|
|
||
| import ( | ||
| "context" | ||
| ) |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (2 tests)
Verdict
Coverage highlights:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Documents the decision to add a static analyzer that catches nil passed as context.Context at analysis time rather than runtime. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (213 new lines in 📄 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
📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![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 — requesting changes on two correctness/coverage gaps.
📋 Key Themes & Highlights
Key Issues
- False-negative blind spot:
astutil.ContextContextTypereturnsnilwhen the analyzed package doesn't importcontextitself, causing the analyzer to silently skip all calls in that package. This is a real correctness bug — packages that call e.g.http.NewRequestWithContext(nil,...)without a directcontextimport won't be analyzed. - Missing test coverage: No test for calling via a function variable (
f := takesCtx; f(nil)), leaving thecalleeSignaturecode path without regression protection.
Positive Highlights
- ✅ Clean structure following established analyzer patterns (
astutil,nolint,filecheck) - ✅ Correct variadic parameter handling
- ✅
isBuiltinNilusingtypes.Nilis precise — avoids false positives on typed nils - ✅ Comprehensive happy-path test cases
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 32.8 AIC · ⌖ 4.55 AIC · ⊞ 6.7K
Comment /matt to run again
| ctxType := astutil.ContextContextType(pass) | ||
| if ctxType == nil { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
[/tdd] The early-exit on ctxType == nil produces false negatives when the analyzed package doesn't itself import context — a package can call http.NewRequestWithContext(nil, ...) without importing context and the analyzer silently skips it.
💡 Suggested fix
Look up the context.Context type by walking the type-checker's universe or via a direct package lookup rather than relying on astutil.ContextContextType which depends on the current package's imports:
// Instead of bailing when the local package doesn't import context,
// resolve the type from the type-checker's scope:
ctxIface := lookupContextType(pass.Pkg)
// where lookupContextType searches imported packages transitivelyAlternatively, check every parameter against the named type context.Context using its package path ("context") so the check is import-independent.
@copilot please address this.
| } else { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] There is no test case for calling through a function variable or method value — e.g. f := takesCtx; f(nil). The calleeSignature path handles this correctly via TypesInfo.TypeOf, but without a test this regression surface is uncovered.
💡 Suggested test case
Add to testdata:
func BadNilFuncVar() {
f := takesCtx
f(nil) // want `nil passed as context\.Context...`
}@copilot please address this.
There was a problem hiding this comment.
Review: nilctxpassed analyzer
The implementation is well-structured and follows the established linter patterns. Two issues warrant attention (already flagged inline):
Blocking concern — ContextContextType returns nil when the analyzed package does not directly import context, causing an early exit at line 43. Packages that pass nil to context.Context parameters from external packages (e.g. net/http.NewRequestWithContext) without importing context themselves will produce no diagnostics — a common real-world bug pattern the analyzer should catch.
Suggested fix: rather than relying on the current package's imports, resolve the context.Context type by iterating all transitively-imported packages, or compare by interface identity (method set) rather than types.Identical.
Test gap — The testdata only exercises packages that import context directly. A test using net/http.NewRequestWithContext(nil, ...) without a context import would expose the false-negative above.
No other issues found in registration, documentation, or the nolint/variadic handling.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 26 AIC · ⌖ 4.34 AIC · ⊞ 5K
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness bugs and a crash path must be fixed before merge.
Blocking issues
-
ctxType == nilearly exit silently disables the linter for the most common real-world scenario (flagged by prior review):ContextContextTypeonly walks direct imports; any package calling an external function that acceptscontext.Contextwithout itself importing"context"will produce zero diagnostics. This covers the majority of real call sites (e.g.http.NewRequestWithContext(nil, ...)). -
nolint suppression checked at
call.Pos()instead ofarg.Pos()(new): In multi-line calls,//nolint:nilctxpassedon thenilargument line is silently ignored. The linter emits a false-positive and developers cannot suppress it. Inverse is also true: a nolint on the call-expression opening line blanket-suppresses all argument diagnostics. Fix: moveHasDirectiveForLinterinside the argument loop usingarg.Pos(). -
params.At(params.Len()-1)with no guard againstparams.Len() == 0(new): Whensig.Variadic()is true on a zero-parameter signature (cgo stubs, synthetic types), this panics with an index-out-of-range. A simpleif params.Len() == 0 { continue }guard is all that is needed.
🔎 Code quality review by PR Code Quality Reviewer · 58.9 AIC · ⌖ 4.79 AIC · ⊞ 5.6K
Comment /review to run again
| if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { | ||
| continue | ||
| } | ||
| if nolint.HasDirectiveForLinter(pos, noLintIndex, "nilctxpassed") { |
There was a problem hiding this comment.
nolint suppression is checked at call.Pos() but the diagnostic is reported at arg.Pos() — in a multi-line call, //nolint:nilctxpassed on the nil argument line is silently ignored.
💡 Details and fix
Current code checks position at the call expression start:
pos := pass.Fset.PositionFor(call.Pos(), false)
if nolint.HasDirectiveForLinter(pos, noLintIndex, "nilctxpassed") {
continue
}For a multi-line call like:
foo(
nil, (nolint/redacted):nilctxpassed
)call.Pos() is on the foo( line — the directive on the nil line is never found, so the diagnostic fires despite suppression. Worse, a //nolint:nilctxpassed on the foo( line will blanket-suppress diagnostics for all args on later lines.
Fix: move the nolint check inside the argument loop using arg.Pos():
for i, arg := range call.Args {
// ...(type checks)...
argPos := pass.Fset.PositionFor(arg.Pos(), false)
if nolint.HasDirectiveForLinter(argPos, noLintIndex, "nilctxpassed") {
continue
}
pass.Report(...)
}The file-skip check can remain at call.Pos() since file membership cannot change within a call.
| params := sig.Params() | ||
| for i, arg := range call.Args { | ||
| var paramType types.Type | ||
| if sig.Variadic() && i >= params.Len()-1 { |
There was a problem hiding this comment.
Potential panic: params.At(params.Len()-1) is called without guarding against params.Len() == 0 when sig.Variadic() is true on a malformed or cgo-generated signature.
💡 Details and fix
sig.Variadic() == true means the function type is variadic, but nothing in the Go type-checker guarantees params.Len() >= 1 in all corner cases (e.g. cgo-generated stubs, intentionally malformed AST input during testing). Computing params.Len()-1 on an empty tuple yields -1, and params.At(-1) panics.
Fix: add a guard before the slice-type lookup:
if sig.Variadic() && i >= params.Len()-1 {
if params.Len() == 0 {
continue
}
sliceType, ok := params.At(params.Len() - 1).Type().(*types.Slice)
...
}| takesCtx(ctx) | ||
| } | ||
|
|
||
| // not flagged: nolint suppresses the diagnostic |
There was a problem hiding this comment.
The nolint test only covers a single-line, single-argument call — the broken multi-line case is entirely untested, meaning the position-mismatch bug described in the main file goes undetected by this test suite.
💡 What to add
Add a test case where the //nolint:nilctxpassed directive is on the same line as the nil argument in a multi-line call:
// not flagged: nolint on the nil arg line in a multiline call
func NolintMultilineSuppressed() {
takesCtxAndOther(
nil, (nolint/redacted):nilctxpassed
42,
)
}Without this test, the nolint-at-call-pos vs nolint-at-arg-pos bug passes CI silently.
|
Run: https://github.com/github/gh-aw/actions/runs/29444107527
|
…int position, guard variadic zero-params - Replace ContextContextType early-return with isContextContext() that walks each parameter's named type directly (Pkg().Path()=="context" && Name()=="Context"), eliminating the false-negative when the analyzed package does not directly import "context". - Add params.Len()>0 guard before params.Len()-1 to prevent a panic on a theoretically zero-param variadic signature. - Move nolint suppression check to arg-level (arg.Pos()) so that a //nolint:nilctxpassed comment on the nil argument's line correctly suppresses the diagnostic in multiline calls. - Add testdata/src/ctxhelper helper package and testdata/src/nilctxpassed_noctx fixture that calls ctxhelper.TakesCtx(nil) without importing "context", with a // want annotation that exercises the regression path. - Add NolintSuppressedMultiline fixture case to verify per-arg nolint suppression in multiline calls. - Update TestNilCtxPassed to run analysis on both packages. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All four issues addressed in commit
|
|
Run: https://github.com/github/gh-aw/actions/runs/29447716606
|
All blocking review issues are confirmed fully addressed in the pushed commits. Summary: Blocking review (all addressed in commit
ADR — Local validation — |
|
@copilot please run the
|
PR is ready for maintainer review. Local validation confirms:
All blocking review issues were addressed in commit Hand-off: CI must be re-triggered by a maintainer (close/reopen PR or |
|
🎉 This pull request is included in a new release. Release: |
Passing
nilas acontext.Contextcompiles cleanly but panics at runtime when the callee calls any method on the context. Adds a newnilctxpassedanalyzer to catch this at analysis time.What changed
pkg/linters/nilctxpassed/nilctxpassed.go— New analyzer. For each call expression, resolves the callee's*types.Signature, checks each parameter that acceptscontext.Context, and flags arguments that are the predeclarednil(confirmed via*types.Nil). Handles variadic positions; respects//nolint:nilctxpassedand skips generated files.pkg/linters/nilctxpassed/nilctxpassed_test.go+testdata/—analysistest-based coverage: nil in first/second/variadic positions, propercontext.Background()/context.TODO()(no flag), nil for non-context interfaces (no flag), nolint suppression.cmd/linters/main.go— Registers the analyzer in the multichecker binary.pkg/linters/README.md,doc.go,spec_test.go— Documentation and spec updated (count 44 → 45).Example