refactor(linters): consolidate duplicated AST/context helpers into internal/astutil

docs/adr/43649-consolidate-linter-astutil-helpers.md

@@ -0,0 +1,46 @@
+# ADR-43649: Consolidate Duplicated AST/Context Helpers into internal/astutil
+
+**Date**: 2026-07-06
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+The `pkg/linters` suite contained multiple independent linter packages, each with its own private copies of the same small helper functions: `enclosingFuncType` (extract `*ast.FuncType` from a `FuncDecl` or `FuncLit`), `contextContextType` (resolve `context.Context` via type-checker imports), `contextParamName` (find the first `context.Context` parameter name in a function signature), `calledOSFunc` (check whether a call targets a named function in the `os` package), and `flipOp` / `flipComparisonOp` (flip comparison operators for normalization). At least eight analyzer packages (`execcommandwithoutcontext`, `httpnoctx`, `timesleepnocontext`, `ctxbackground`, `osgetenvlibrary`, `ossetenvlibrary`, `lenstringzero`, `stringsindexcontains`) maintained independent copies, creating a realistic drift risk: a correctness fix or a nil-guard addition applied to one copy could silently leave the others broken.
+
+### Decision
+
+We will centralize all shared AST and type-checker utility helpers into the existing `pkg/linters/internal/astutil` package and delete per-package copies. The consolidated helpers are exported as `EnclosingFuncType`, `ContextContextType`, `ContextParamName`, `CalledOSFunc` (redesigned with a variadic `allowedNames` filter for cleaner call sites), and `FlipComparisonOp`. Each affected linter is rewired to import and call the shared versions, and unit tests covering the new functions are added to `astutil_test.go`. The `internal` placement restricts access to packages within `pkg/linters/...`, which is the correct scope.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep Per-Package Copies with No Shared Abstraction
+
+Each analyzer would continue to own its local private helper. This is the status quo and requires zero refactoring effort. It was rejected because identical helper functions across eight-plus packages are a demonstrated maintenance hazard: divergent nil-guards and differing function-name filters were already present across packages, confirming that drift was occurring. There is no mechanism to enforce consistency without a single source of truth.
+
+#### Alternative 2: Code-Generate Per-Package Helpers from a Template
+
+A generator (e.g., using `go generate`) could stamp identical helper bodies into each package from a shared template, keeping copies in sync while preserving package-private visibility. This eliminates the behavioral drift risk without changing import paths. It was rejected because it introduces generator tooling complexity, requires each package's generated file to be regenerated whenever the template changes, and provides no advantage over a shared package for helpers that carry no package-specific state. The `internal` package approach is simpler and idiomatic in the Go standard library and `x/tools` ecosystem.
+
+### Consequences
+
+#### Positive
+- Single source of truth for shared helpers: a bug fix or a nil-guard addition needs to be made only once and takes effect across all linters immediately.
+- `CalledOSFunc` gains a variadic `allowedNames` parameter, making call sites more readable (`astutil.CalledOSFunc(pass, call, "Getenv", "LookupEnv")` vs. a hardcoded name comparison inside a local copy).
+- New linter authors can discover and reuse existing utilities rather than copy-pasting, lowering the per-linter authoring cost.
+- Expanded unit test coverage for `astutil` provides a regression harness for the shared utilities independent of any individual linter's test suite.
+
+#### Negative
+- The `internal` package boundary limits these helpers to packages under `pkg/linters/...`; code outside that tree (e.g., a future top-level analysis driver) cannot import them without restructuring.
+- Callers now take an indirect dependency on `pkg/linters/internal/astutil` — a change to a helper's signature requires updating every consumer in the same commit, rather than each package being independently evolvable.
+
+#### Neutral
+- No behavioral change is intended; existing linter diagnostics and suggested fixes should be identical before and after the migration.
+- The `astutil` package already existed and was in use; this PR extends it rather than introducing a new dependency.
+- Test coverage for the consolidated helpers lives in `astutil_test.go`, separate from individual linter testdata directories.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/linters/ctxbackground/ctxbackground.go

@@ -46,16 +46,11 @@ func run(pass *analysis.Pass) (any, error) {
 		}
 
 		for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) {
-			var ftype *ast.FuncType
-			switch fn := encl.Node().(type) {
-			case *ast.FuncDecl:
-				ftype = fn.Type
-			case *ast.FuncLit:
-				ftype = fn.Type
-			default:
+			ftype := astutil.EnclosingFuncType(encl.Node())
+			if ftype == nil {
 				continue
 			}
-			ctxParamName, ok := contextParamName(pass, ftype)
+			ctxParamName, ok := astutil.ContextParamName(pass, ftype)
 			if !ok {
 				break
 			}
@@ -103,44 +98,3 @@ func isContextBackgroundCall(pass *analysis.Pass, call *ast.CallExpr) bool {
 	}
 	return pkgName.Imported().Path() == "context"
 }
-
-// contextParamName returns the first non-blank context.Context parameter name.
-func contextParamName(pass *analysis.Pass, ftype *ast.FuncType) (string, bool) {
-	if ftype == nil || ftype.Params == nil {
-		return "", false
-	}
-	ctxType := contextType(pass)
-	if ctxType == nil {
-		return "", false
-	}
-	for _, field := range ftype.Params.List {
-		t := pass.TypesInfo.TypeOf(field.Type)
-		if t == nil {
-			continue
-		}
-		if !types.Identical(t, ctxType) {
-			continue
-		}
-		// At least one name must not be blank.
-		for _, name := range field.Names {
-			if name.Name != "_" {
-				return name.Name, true
-			}
-		}
-	}
-	return "", false
-}
-
-// contextType returns the types.Type for context.Context, or nil if the
-// package is not imported.
-func contextType(pass *analysis.Pass) types.Type {
-	for _, pkg := range pass.Pkg.Imports() {
-		if pkg.Path() == "context" {
-			obj := pkg.Scope().Lookup("Context")
-			if obj != nil {
-				return obj.Type()
-			}
-		}
-	}
-	return nil
-}

pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go

@@ -52,11 +52,11 @@ func run(pass *analysis.Pass) (any, error) {
 		}
 
 		for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) {
-			funcType := enclosingFuncType(encl.Node())
+			funcType := astutil.EnclosingFuncType(encl.Node())
 			if funcType == nil {
 				continue
 			}
-			ctxParamName, hasCtx := contextParamName(pass, funcType)
+			ctxParamName, hasCtx := astutil.ContextParamName(pass, funcType)
 			if !hasCtx {
 				continue
 			}
@@ -113,52 +113,3 @@ func execCommandSelector(pass *analysis.Pass, call *ast.CallExpr) (*ast.Selector
 	}
 	return sel, true
 }
-
-// contextParamName returns the name of the first context.Context parameter
-// in fn, and true, or "", false if none exists.
-func contextParamName(pass *analysis.Pass, fn *ast.FuncType) (string, bool) {
-	if fn == nil || fn.Params == nil {
-		return "", false
-	}
-	ctxType := contextContextType(pass)
-	if ctxType == nil {
-		return "", false
-	}
-	for _, field := range fn.Params.List {
-		t := pass.TypesInfo.TypeOf(field.Type)
-		if t == nil || !types.Identical(t, ctxType) {
-			continue
-		}
-		for _, name := range field.Names {
-			if name.Name != "_" {
-				return name.Name, true
-			}
-		}
-	}
-	return "", false
-}
-
-func enclosingFuncType(node ast.Node) *ast.FuncType {
-	switch fn := node.(type) {
-	case *ast.FuncDecl:
-		return fn.Type
-	case *ast.FuncLit:
-		return fn.Type
-	default:
-		return nil
-	}
-}
-
-// contextContextType returns the types.Type for context.Context, or nil if
-// the context package is not imported.
-func contextContextType(pass *analysis.Pass) types.Type {
-	for _, pkg := range pass.Pkg.Imports() {
-		if pkg.Path() == "context" {
-			obj := pkg.Scope().Lookup("Context")
-			if obj != nil {
-				return obj.Type()
-			}
-		}
-	}
-	return nil
-}

pkg/linters/httpnoctx/httpnoctx.go

@@ -44,7 +44,6 @@ func run(pass *analysis.Pass) (any, error) {
 		return nil, err
 	}
 	noLintLinesByFile := nolint.BuildLineIndex(pass, "httpnoctx")
-	ctxType := contextContextType(pass)
 
 	for cursor := range insp.Root().Preorder((*ast.CallExpr)(nil)) {
 		call, ok := cursor.Node().(*ast.CallExpr)
@@ -82,7 +81,7 @@ func run(pass *analysis.Pass) (any, error) {
 			}
 		}
 
-		if sel.Sel.Name == "NewRequest" && isHTTPPackage(pass, sel.X) && hasContextInEnclosingFunc(pass, cursor, ctxType) {
+		if sel.Sel.Name == "NewRequest" && isHTTPPackage(pass, sel.X) && hasContextInEnclosingFunc(pass, cursor) {
 			pass.ReportRangef(call,
 				"http.NewRequest does not propagate context; use http.NewRequestWithContext when context.Context is in scope",
 			)
@@ -134,56 +133,20 @@ func isHTTPPackage(pass *analysis.Pass, expr ast.Expr) bool {
 	return pkgName.Imported().Path() == "net/http"
 }
 
-func hasContextInEnclosingFunc(pass *analysis.Pass, cursor inspector.Cursor, ctxType types.Type) bool {
-	if ctxType == nil {
-		return false
-	}
-
+func hasContextInEnclosingFunc(pass *analysis.Pass, cursor inspector.Cursor) bool {
 	for enclosing := range cursor.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) {
-		fnType := enclosingFuncType(enclosing.Node())
+		fnType := astutil.EnclosingFuncType(enclosing.Node())
 		if fnType == nil || fnType.Params == nil {
 			continue
 		}
-
-		for _, field := range fnType.Params.List {
-			t := pass.TypesInfo.TypeOf(field.Type)
-			if t == nil || !types.Identical(t, ctxType) {
-				continue
-			}
-			for _, name := range field.Names {
-				if name.Name != "_" {
-					return true
-				}
-			}
+		if _, ok := astutil.ContextParamName(pass, fnType); ok {
+			return true
 		}
 	}
 
 	return false
 }
 
-func enclosingFuncType(node ast.Node) *ast.FuncType {
-	switch fn := node.(type) {
-	case *ast.FuncDecl:
-		return fn.Type
-	case *ast.FuncLit:
-		return fn.Type
-	default:
-		return nil
-	}
-}
-
-func contextContextType(pass *analysis.Pass) types.Type {
-	for _, pkg := range pass.Pkg.Imports() {
-		if pkg.Path() != "context" {
-			continue
-		}
-		if obj := pkg.Scope().Lookup("Context"); obj != nil {
-			return obj.Type()
-		}
-	}
-	return nil
-}
-
 func isHTTPDefaultClient(pass *analysis.Pass, expr ast.Expr) bool {
 	sel, ok := expr.(*ast.SelectorExpr)
 	if !ok || sel.Sel.Name != "DefaultClient" {

pkg/linters/internal/astutil/astutil.go

@@ -8,6 +8,7 @@ import (
 	"go/printer"
 	"go/token"
 	"go/types"
+	"slices"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
@@ -48,6 +49,59 @@ func IsStringLiteral(expr ast.Expr) bool {
 	return ok && lit.Kind == token.STRING
 }
 
+// EnclosingFuncType extracts a function type from a FuncDecl or FuncLit node.
+func EnclosingFuncType(node ast.Node) *ast.FuncType {
+	switch fn := node.(type) {
+	case *ast.FuncDecl:
+		return fn.Type
+	case *ast.FuncLit:
+		return fn.Type
+	default:
+		return nil
+	}
+}
+
+// ContextContextType returns the types.Type for context.Context, or nil if
+// the context package is not imported.
+func ContextContextType(pass *analysis.Pass) types.Type {
+	if pass == nil || pass.Pkg == nil {
+		return nil
+	}
+	for _, pkg := range pass.Pkg.Imports() {
+		if pkg.Path() == "context" {
+			obj := pkg.Scope().Lookup("Context")
+			if obj != nil {
+				return obj.Type()
+			}
+		}
+	}
+	return nil
+}
+
+// ContextParamName returns the name of the first context.Context parameter in
+// fn, and true, or "", false if none exists.
+func ContextParamName(pass *analysis.Pass, fn *ast.FuncType) (string, bool) {
+	if pass == nil || pass.TypesInfo == nil || fn == nil || fn.Params == nil {
+		return "", false
+	}
+	ctxType := ContextContextType(pass)
+	if ctxType == nil {
+		return "", false
+	}
+	for _, field := range fn.Params.List {
+		t := pass.TypesInfo.TypeOf(field.Type)
+		if t == nil || !types.Identical(t, ctxType) {
+			continue
+		}
+		for _, name := range field.Names {
+			if name.Name != "_" {
+				return name.Name, true
+			}
+		}
+	}
+	return "", false
+}
+
 // IsFmtErrorf reports whether call is a call to fmt.Errorf (including aliases).
 func IsFmtErrorf(pass *analysis.Pass, call *ast.CallExpr) bool {
 	sel, ok := call.Fun.(*ast.SelectorExpr)
@@ -72,6 +126,36 @@ func IsFmtErrorf(pass *analysis.Pass, call *ast.CallExpr) bool {
 	return pkgName.Imported().Path() == "fmt"
 }
 
+// CalledOSFunc reports whether call resolves to a function in package os. If
+// allowedNames are provided, the function name must match one of them.
+func CalledOSFunc(pass *analysis.Pass, call *ast.CallExpr, allowedNames ...string) (*types.Func, bool) {
+	if pass == nil || pass.TypesInfo == nil || call == nil {
+		return nil, false
+	}
+
+	var obj types.Object
+	switch fun := call.Fun.(type) {
+	case *ast.SelectorExpr:
+		obj = pass.TypesInfo.Uses[fun.Sel]
+	case *ast.Ident:
+		obj = pass.TypesInfo.Uses[fun]
+	default:
+		return nil, false
+	}
+
+	fn, ok := obj.(*types.Func)
+	if !ok || fn.Pkg() == nil || fn.Pkg().Path() != "os" {
+		return nil, false
+	}
+	if len(allowedNames) == 0 {
+		return fn, true
+	}
+	if slices.Contains(allowedNames, fn.Name()) {
+		return fn, true
+	}
+	return nil, false
+}
+
 // IsPkgSelector reports whether sel is a selector on an imported package with
 // the given import path.
 func IsPkgSelector(pass *analysis.Pass, sel *ast.SelectorExpr, pkgPath string) bool {
@@ -93,6 +177,23 @@ func IsPkgSelector(pass *analysis.Pass, sel *ast.SelectorExpr, pkgPath string) b
 	return pkgName.Imported().Path() == pkgPath
 }
 
+// FlipComparisonOp returns the comparison operator with left and right
+// operands swapped.
+func FlipComparisonOp(op token.Token) token.Token {
+	switch op {
+	case token.LSS:
+		return token.GTR
+	case token.GTR:
+		return token.LSS
+	case token.LEQ:
+		return token.GEQ
+	case token.GEQ:
+		return token.LEQ
+	default:
+		return op
+	}
+}
+
 // Inspector extracts the *inspector.Inspector from pass.ResultOf.
 // It returns an error if the result has an unexpected type.
 func Inspector(pass *analysis.Pass) (*inspector.Inspector, error) {