Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/linters/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (
"github.com/github/gh-aw/pkg/linters/logfatallibrary"
"github.com/github/gh-aw/pkg/linters/manualmutexunlock"
"github.com/github/gh-aw/pkg/linters/mapdeletecheck"
"github.com/github/gh-aw/pkg/linters/nilctxpassed"
"github.com/github/gh-aw/pkg/linters/osexitinlibrary"
"github.com/github/gh-aw/pkg/linters/osgetenvlibrary"
"github.com/github/gh-aw/pkg/linters/ossetenvlibrary"
Expand Down Expand Up @@ -93,6 +94,7 @@ func main() {
logfatallibrary.Analyzer,
manualmutexunlock.Analyzer,
mapdeletecheck.Analyzer,
nilctxpassed.Analyzer,
osexitinlibrary.Analyzer,
osgetenvlibrary.Analyzer,
ossetenvlibrary.Analyzer,
Expand Down
50 changes: 50 additions & 0 deletions docs/adr/45799-add-nilctxpassed-linter.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# ADR-45799: Add nilctxpassed Static Analyzer to Catch nil context.Context Arguments

**Date**: 2026-07-15
**Status**: Draft
**Deciders**: pelikhan, copilot-swe-agent

---

### Context

Passing `nil` as a `context.Context` argument compiles cleanly in Go because `nil` is a valid zero value for any interface type, including `context.Context`. However, any call to a method on a `nil` context (e.g., `ctx.Deadline()`, `ctx.Done()`, `ctx.Value()`) causes a runtime panic that is difficult to diagnose. The existing linter suite (`pkg/linters/`) already uses the `golang.org/x/tools/go/analysis` framework and shared helpers (`astutil`, `nolint`, `filecheck`) to catch similar classes of bugs at analysis time. Adding `nilctxpassed` closes the gap between "compiles successfully" and "safe to run" for a common mistake pattern.

### Decision

We will add a new `nilctxpassed` analyzer (`pkg/linters/nilctxpassed/nilctxpassed.go`) that inspects every call expression in non-generated Go source files, resolves the callee's `*types.Signature`, and reports a diagnostic whenever the predeclared `nil` (confirmed via `*types.Nil`) is passed in a position whose parameter type is identical to `context.Context`. The analyzer supports variadic positions and respects `//nolint:nilctxpassed` suppression. It is registered in the `cmd/linters/main.go` multichecker binary alongside all existing analyzers.

### Alternatives Considered

#### Alternative 1: Rely on the Go type system alone

Go's compiler rejects most non-interface mismatches, but `nil` is assignable to any interface type including `context.Context`. No compiler error or warning is emitted when a developer writes `f(nil)` where `f` accepts `context.Context`. This alternative provides zero protection against the pattern at build time, so it was rejected.

#### Alternative 2: Runtime guard / defensive nil checks inside every context-accepting function

Each function that accepts a `context.Context` could panic or return an error immediately if `ctx == nil`. This defensive check is idiomatic in some codebases but requires manual discipline across every function signature, adds per-call overhead, produces less actionable error messages than a linter diagnostic with source location, and does not scale to third-party code that does not own the function definitions. It was rejected in favour of a single-point static check that enforces the constraint uniformly.

#### Alternative 3: Use an off-the-shelf linter (e.g., `noctx` from `golangci-lint`)

Existing third-party linters target related but distinct problems (e.g., HTTP requests made without a context). None of the commonly used linters in `golangci-lint` precisely flag nil-as-context-argument using type-identity checks. Additionally, the project's linter suite is a custom multichecker binary that requires analyzers to be written using the same internal helpers (`astutil`, `nolint`, `filecheck`) for consistent nolint suppression and generated-file skipping. Adopting a third-party linter would mean a different suppression mechanism and would not integrate with the shared `nolint.Analyzer` dependency. A bespoke analyzer was preferred.

### Consequences

#### Positive
- Eliminates an entire class of runtime panic (`nil` context) by making it a build-time error, catching bugs before they reach staging or production.
- Follows the established pattern of the linter suite: same `go/analysis` framework, same shared helpers, same `//nolint:<linter>` suppression mechanism — no new concepts for contributors to learn.
- The `analysistest`-based test suite provides deterministic, reproducible verification of both flagged and non-flagged cases without runtime execution.
- Generated files are automatically excluded from analysis via the existing `filecheck.Analyzer` dependency.

#### Negative
- Adds one more analysis pass to the multichecker binary, marginally increasing total analysis time (proportional to the number of call expressions in the codebase).
- The `//nolint:nilctxpassed` escape valve could be misused to suppress legitimate diagnostics in cases where `nil` is intentionally passed (e.g., tests that explicitly exercise nil-context handling). Teams must review nolint suppressions in code review.
- The analyzer only catches the predeclared `nil` identifier (confirmed via `*types.Nil`); a typed `var ctx context.Context` (whose zero value is also nil) would not be flagged, leaving a gap for that pattern.

#### Neutral
- The linter count in `pkg/linters/doc.go` and the spec test count increment from 44 to 45; the spec test `documentedAnalyzers()` list and README table must be kept in sync when adding future analyzers.
- The binary at `linters` (tracked in version control) was updated as part of this change.

---

*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*
Binary file modified linters
Binary file not shown.
2 changes: 2 additions & 0 deletions pkg/linters/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ This package currently provides custom Go analyzers in the following subpackages
- `logfatallibrary` — reports `log.Fatal`, `log.Fatalf`, and `log.Fatalln` calls in library packages (`pkg/*`) where they implicitly call `os.Exit` and bypass deferred cleanup.
- `mapdeletecheck` — reports redundant map membership checks before `delete(m, k)` calls since `delete` is already a no-op for missing keys.
- `manualmutexunlock` — reports non-deferred mutex `Unlock()` calls that can lead to deadlocks on early returns or panics.
- `nilctxpassed` — reports function calls where `nil` is passed as a `context.Context` argument; the correct idioms are `context.Background()` or `context.TODO()`.
- `osgetenvlibrary` — reports `os.Getenv` calls in library packages (`pkg/*`) where environment access should be injected.
- `osexitinlibrary` — reports `os.Exit` calls in library packages (`pkg/*`) where process termination should be delegated to `cmd/*` entry points.
- `ossetenvlibrary` — reports `os.Setenv` calls in library packages (`pkg/*`) where side effects should be isolated.
Expand Down Expand Up @@ -90,6 +91,7 @@ This package currently provides custom Go analyzers in the following subpackages
| `logfatallibrary` | Custom `go/analysis` analyzer that flags `log.Fatal`, `log.Fatalf`, and `log.Fatalln` calls in library packages where they implicitly call `os.Exit` and bypass deferred cleanup |
| `mapdeletecheck` | Custom `go/analysis` analyzer that flags redundant map membership checks before `delete(m, k)` calls since `delete` is a no-op for missing keys |
| `manualmutexunlock` | Custom `go/analysis` analyzer that flags mutex `Unlock()` calls that are not deferred |
| `nilctxpassed` | Custom `go/analysis` analyzer that flags function calls where `nil` is passed as a `context.Context` argument |
| `osgetenvlibrary` | Custom `go/analysis` analyzer that flags `os.Getenv` usage in library packages |
| `osexitinlibrary` | Custom `go/analysis` analyzer that flags `os.Exit` usage in library packages |
| `ossetenvlibrary` | Custom `go/analysis` analyzer that flags `os.Setenv` usage in library packages |
Expand Down
3 changes: 2 additions & 1 deletion pkg/linters/doc.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Package linters is a namespace for gh-aw's custom Go analysis linters.
//
// All 44 active analyzers:
// All 45 active analyzers:
//
// - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...)
// - bytescomparestring — flags string(a) == string(b) and string(a) != string(b) comparisons where a and b are []byte values and recommends bytes.Equal for clearer intent
Expand All @@ -25,6 +25,7 @@
// - lenstringsplit — flags len(strings.Split(s, sep)) with a non-empty separator that should use strings.Count(s, sep)+1
// - lenstringzero — flags len(s) == 0 / len(s) != 0 on string values that should use s == "" / s != ""
// - manualmutexunlock — flags non-deferred mutex Unlock() calls
// - nilctxpassed — flags function calls where nil is passed as a context.Context argument
// - osexitinlibrary — flags os.Exit calls in library packages
// - osgetenvlibrary — flags os.Getenv calls in library packages
// - ossetenvlibrary — flags os.Setenv calls in library packages
Expand Down
138 changes: 138 additions & 0 deletions pkg/linters/nilctxpassed/nilctxpassed.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
// Package nilctxpassed implements a Go analysis linter that flags function
// calls where nil is passed as a context.Context argument.
package nilctxpassed

import (
"go/ast"
"go/types"

"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/inspect"

"github.com/github/gh-aw/pkg/linters/internal/astutil"
"github.com/github/gh-aw/pkg/linters/internal/filecheck"
"github.com/github/gh-aw/pkg/linters/internal/nolint"
)

// Analyzer is the nil-context-passed analysis pass.
var Analyzer = &analysis.Analyzer{
Name: "nilctxpassed",
Doc: "reports function calls where nil is passed as a context.Context argument",
URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/nilctxpassed",
Requires: []*analysis.Analyzer{inspect.Analyzer, nolint.Analyzer, filecheck.Analyzer},
Run: run,
}

func run(pass *analysis.Pass) (any, error) {
insp, err := astutil.Inspector(pass)
if err != nil {
return nil, err
}
noLintIndex, err := nolint.Index(pass)
if err != nil {
return nil, err
}
generatedFiles, err := filecheck.Index(pass)
if err != nil {
return nil, err
}

for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) {
call, ok := cur.Node().(*ast.CallExpr)
if !ok {
continue
}

callPos := pass.Fset.PositionFor(call.Pos(), false)
if filecheck.ShouldSkipFilename(callPos.Filename, generatedFiles) {
continue
}

sig := calleeSignature(pass, call)
if sig == nil {
continue
}

params := sig.Params()
for i, arg := range call.Args {
var paramType types.Type
if sig.Variadic() && params.Len() > 0 && i >= params.Len()-1 {
// Variadic: the last param is a slice; check its element type.
sliceType, ok := params.At(params.Len() - 1).Type().(*types.Slice)
if !ok {
continue
}
paramType = sliceType.Elem()
} else if i < params.Len() {
paramType = params.At(i).Type()
} else {
continue
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

if !isContextContext(paramType) {
continue
}

if !isBuiltinNil(pass, arg) {
continue
}

argPos := pass.Fset.PositionFor(arg.Pos(), false)
if nolint.HasDirectiveForLinter(argPos, noLintIndex, "nilctxpassed") {
continue
}

pass.Report(analysis.Diagnostic{
Pos: arg.Pos(),
End: arg.End(),
Message: "nil passed as context.Context; use context.Background() or context.TODO() instead",
})
}
}

return nil, nil
}

// isContextContext reports whether t is the context.Context interface type,
// identified by inspecting the named type directly rather than relying on the
// current package importing "context". This catches cases where the analyzed
// package passes nil to a context.Context parameter from an external function
// without importing the context package itself.
func isContextContext(t types.Type) bool {
named, ok := t.(*types.Named)
if !ok {
return false
}
obj := named.Obj()
return obj.Pkg() != nil && obj.Pkg().Path() == "context" && obj.Name() == "Context"
}

// calleeSignature returns the *types.Signature of the callee if available.
func calleeSignature(pass *analysis.Pass, call *ast.CallExpr) *types.Signature {
if pass.TypesInfo == nil {
return nil
}
t := pass.TypesInfo.TypeOf(call.Fun)
if t == nil {
return nil
}
sig, ok := t.Underlying().(*types.Signature)
if !ok {
return nil
}
return sig
}

// isBuiltinNil reports whether expr is the predeclared nil identifier.
func isBuiltinNil(pass *analysis.Pass, expr ast.Expr) bool {
ident, ok := expr.(*ast.Ident)
if !ok || ident.Name != "nil" {
return false
}
if pass.TypesInfo == nil {
return false
}
obj := pass.TypesInfo.Uses[ident]
_, ok = obj.(*types.Nil)
return ok
}
16 changes: 16 additions & 0 deletions pkg/linters/nilctxpassed/nilctxpassed_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//go:build !integration

package nilctxpassed_test

import (
"testing"

"golang.org/x/tools/go/analysis/analysistest"

"github.com/github/gh-aw/pkg/linters/nilctxpassed"
)

func TestNilCtxPassed(t *testing.T) {
testdata := analysistest.TestData()
analysistest.Run(t, testdata, nilctxpassed.Analyzer, "nilctxpassed", "nilctxpassed_noctx")
}
11 changes: 11 additions & 0 deletions pkg/linters/nilctxpassed/testdata/src/ctxhelper/ctxhelper.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Package ctxhelper provides helper functions used by nilctxpassed test
// fixtures to exercise calls from packages that do not directly import context.
package ctxhelper

import "context"

// TakesCtx accepts a context.Context parameter.
func TakesCtx(ctx context.Context) {}

// TakesOtherPointer accepts a *int parameter (not context.Context).
func TakesOtherPointer(p *int) {}
70 changes: 70 additions & 0 deletions pkg/linters/nilctxpassed/testdata/src/nilctxpassed/nilctxpassed.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package nilctxpassed

import (
"context"
)
Comment on lines +1 to +5

func takesCtx(ctx context.Context) {}

func takesCtxAndOther(ctx context.Context, n int) {}

func takesOtherAndCtx(n int, ctx context.Context) {}

func takesVariadicCtx(args ...context.Context) {}

type myKey struct{}

// flagged: nil passed as context.Context
func BadNilContext() {
takesCtx(nil) // want `nil passed as context\.Context; use context\.Background\(\) or context\.TODO\(\) instead`
}

// flagged: nil in first of two params
func BadNilFirstParam() {
takesCtxAndOther(nil, 42) // want `nil passed as context\.Context; use context\.Background\(\) or context\.TODO\(\) instead`
}

// flagged: nil in second positional context param
func BadNilSecondParam() {
takesOtherAndCtx(42, nil) // want `nil passed as context\.Context; use context\.Background\(\) or context\.TODO\(\) instead`
}

// flagged: nil as the only variadic context arg
func BadNilVariadic() {
takesVariadicCtx(nil) // want `nil passed as context\.Context; use context\.Background\(\) or context\.TODO\(\) instead`
}

// not flagged: proper context values
func GoodContextBackground() {
takesCtx(context.Background())
}

func GoodContextTODO() {
takesCtx(context.TODO())
}

// not flagged: non-context nil (e.g. error interface)
func takesError(err error) {}

func GoodNilError() {
takesError(nil)
}

// not flagged: a local context variable passed normally is fine
func GoodLocalCtx() {
ctx := context.Background()
takesCtx(ctx)
}

// not flagged: nolint suppresses the diagnostic (same line)
func NolintSuppressed() {
takesCtx(nil) //nolint:nilctxpassed
}

// not flagged: nolint on the nil arg line suppresses in a multiline call
func NolintSuppressedMultiline() {
takesCtxAndOther(
nil, //nolint:nilctxpassed
42,
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Package nilctxpassed_noctx tests the nilctxpassed analyzer for the case
// where the analyzed package does NOT directly import "context" but still
// passes nil to a context.Context parameter via an external function.
package nilctxpassed_noctx

import "ctxhelper"

// BadNoDirectImport passes nil to a context.Context param without importing context.
func BadNoDirectImport() {
ctxhelper.TakesCtx(nil) // want `nil passed as context\.Context; use context\.Background\(\) or context\.TODO\(\) instead`
}

// GoodNilNonContext passes nil to a non-context pointer param — not flagged.
func GoodNilNonContext() {
ctxhelper.TakesOtherPointer(nil)
}
6 changes: 4 additions & 2 deletions pkg/linters/spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/github/gh-aw/pkg/linters/lenstringsplit"
"github.com/github/gh-aw/pkg/linters/lenstringzero"
"github.com/github/gh-aw/pkg/linters/manualmutexunlock"
"github.com/github/gh-aw/pkg/linters/nilctxpassed"
"github.com/github/gh-aw/pkg/linters/osexitinlibrary"
"github.com/github/gh-aw/pkg/linters/ossetenvlibrary"
panicinlibrarycode "github.com/github/gh-aw/pkg/linters/panic-in-library-code"
Expand Down Expand Up @@ -66,7 +67,7 @@ type docAnalyzer struct {
}

// documentedAnalyzers returns the analyzer subpackages documented in the README
// "Public API > Subpackages" table. The README documents 40 analyzers
// "Public API > Subpackages" table. The README documents 41 analyzers
// subpackages (the non-analyzer `internal` helper subpackage is excluded because
// it exposes no Analyzer).
//
Expand All @@ -75,7 +76,7 @@ type docAnalyzer struct {
// appendbytestring, contextcancelnotdeferred, ctxbackground, deferinloop, errorfwrapv, excessivefuncparams, errormessage,
// errortypeassertion, errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf,
// hardcodedfilepath, httpnoctx, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero,
// manualmutexunlock, osexitinlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib,
// manualmutexunlock, nilctxpassed, osexitinlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib,
// regexpcompileinfunction, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, ssljson,
// strconvparseignorederror, stringreplaceminusone, stringscountcontains, stringsindexcontains, timeafterleak, timesleepnocontext,
// tolowerequalfold, uncheckedtypeassertion, wgdonenotdeferred, writebytestring
Expand All @@ -101,6 +102,7 @@ func documentedAnalyzers() []docAnalyzer {
{"lenstringsplit", lenstringsplit.Analyzer},
{"lenstringzero", lenstringzero.Analyzer},
{"manualmutexunlock", manualmutexunlock.Analyzer},
{"nilctxpassed", nilctxpassed.Analyzer},
{"osexitinlibrary", osexitinlibrary.Analyzer},
{"ossetenvlibrary", ossetenvlibrary.Analyzer},
{"panic-in-library-code", panicinlibrarycode.Analyzer},
Expand Down
Loading