refactor(linters): consolidate duplicated AST/context helpers into internal/astutil#43649
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the linter suite by consolidating repeated AST/type helper logic into pkg/linters/internal/astutil and updating multiple analyzers to use the shared helpers, with accompanying unit tests for the new shared functions.
Changes:
- Added shared AST/type utilities in
pkg/linters/internal/astutil(enclosing func type, context parameter detection,os.*call detection, comparison-op flipping). - Migrated several linters to use
astutilhelpers and removed their duplicated local implementations. - Expanded
astutilunit tests to cover the newly centralized helper functions.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/internal/astutil/astutil.go | Adds centralized AST/type helper functions used across linters. |
| pkg/linters/internal/astutil/astutil_test.go | Adds unit coverage for the new shared helper functions. |
| pkg/linters/timesleepnocontext/timesleepnocontext.go | Switches to shared astutil context/function helpers; removes local copies. |
| pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go | Switches to shared astutil context/function helpers; removes local copies. |
| pkg/linters/httpnoctx/httpnoctx.go | Switches to shared astutil context/function helpers; removes local copies. |
| pkg/linters/ctxbackground/ctxbackground.go | Switches to shared astutil context/function helpers; removes local copies. |
| pkg/linters/osgetenvlibrary/osgetenvlibrary.go | Reuses shared astutil.CalledOSFunc for os.Getenv/os.LookupEnv detection. |
| pkg/linters/ossetenvlibrary/ossetenvlibrary.go | Reuses shared astutil.CalledOSFunc for os.Setenv/os.Unsetenv detection. |
| pkg/linters/lenstringzero/lenstringzero.go | Reuses shared astutil.FlipComparisonOp to normalize comparisons. |
| pkg/linters/stringsindexcontains/stringsindexcontains.go | Reuses shared astutil.FlipComparisonOp to normalize comparisons. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Low
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| if got := FlipComparisonOp(tt.in); got != tt.want { | ||
| t.Fatalf("FlipComparisonOp(%v) = %v, want %v", tt.in, got, tt.want) | ||
| } | ||
| }) | ||
| } |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (246 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
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
|
🧪 Test Quality Sentinel Report❌ Test Quality Score: 91/100 — Excellent
📊 Metrics (8 tests)
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey The PR is well-scoped, the description clearly explains the motivation and what changed, and the expanded
|
![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 /codebase-design and /tdd — 4 targeted issues raised, all minor. The refactor itself is clean and well-motivated.
📋 Key Themes & Highlights
Key Themes
- Leaky abstraction in
httpnoctx: ThectxTypeparameter onhasContextInEnclosingFuncand the redundantfnType.Params == nilguard expose implementation details that the shared helpers already encapsulate. - Test coverage gaps on new helpers: The
_-named parameter edge case inContextParamNameand the*ast.Identcall form inCalledOSFuncare untested paths. - Monolithic test function:
TestContextHelperscombines two independent concerns; splitting into table-driven subtests improves failure locality.
Positive Highlights
- ✅ Excellent reduction of 271 lines of duplicated boilerplate across 8 packages — zero behavioral delta
- ✅ Shared helpers add proper nil guards (
pass == nil,pass.Pkg == nil,pass.TypesInfo == nil) that several originals lacked - ✅
CalledOSFuncvariadicallowedNamesdesign cleanly replaces per-packageif name != ... && name != ...chains - ✅
FlipComparisonOptests follow the table-driven pattern witht.Parallel()— the right model for the other helpers too - ✅ New
slices.Containsusage is idiomatic Go 1.21+
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 72.4 AIC · ⌖ 8.25 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/linters/httpnoctx/httpnoctx.go:144
[/codebase-design] The fnType.Params == nil guard here is redundant — ContextParamName already handles a nil Params field internally (astutil.go line 84). Keeping it leaks implementation details of the helper into the caller and will drift if the helper's contract ever tightens.
<details>
<summary>💡 Suggested fix</summary>
Drop the Params arm from the nil check:
// before
if fnType == nil || fnType.Params == nil {
continue
}
// after
if fnType == nil {
continue
}
`…
</details>
<details><summary>pkg/linters/httpnoctx/httpnoctx.go:137</summary>
**[/codebase-design]** `ctxType types.Type` is passed into `hasContextInEnclosingFunc` only to guard against a nil early exit (line 138–140), but `ContextParamName` re-derives the context type internally. This creates a shallow module interface — the caller must know enough to pre-fetch and pass `ctxType`, but the callee duplicates the lookup anyway.
<details>
<summary>💡 Suggested fix</summary>
Remove `ctxType` from the signature and drop the early-exit nil guard:
```go
// signature before
f…
</details>
<details><summary>pkg/linters/internal/astutil/astutil_test.go:171</summary>
**[/tdd]** `TestContextHelpers` is a monolithic test function that exercises `ContextContextType` and `ContextParamName` in a single body. When either fails the first assertion, the second scenario is never reached, and failure messages don't indicate which case broke.
<details>
<summary>💡 Suggested structure</summary>
Split into focused subtests:
```go
func TestContextContextType(t *testing.T) {
t.Parallel()
t.Run("with_context_import", func(t *testing.T) { ... })
t.Run("without…
</details>
<details><summary>pkg/linters/internal/astutil/astutil_test.go:223</summary>
**[/tdd]** `TestCalledOSFunc` tests the `*ast.SelectorExpr` call form but never exercises the `*ast.Ident` branch (direct function call without a package qualifier, e.g. a dot-import). That branch in `CalledOSFunc` (astutil.go line 141) is reachable and has no regression protection.
<details>
<summary>💡 Suggested addition</summary>
```go
// add after existing selector test
identIdent := ast.NewIdent("Getenv")
passIdent := &analysis.Pass{
TypesInfo: &types.Info{
Uses: map[*ast.Ide…
</details>
There was a problem hiding this comment.
Review: refactor(linters): consolidate duplicated AST/context helpers into internal/astutil
Good clean-up — 271 lines deleted, duplication eliminated across 8 analyzers, and new helpers are better-guarded than their local counterparts (nil checks on pass, pass.Pkg, pass.TypesInfo).
Two non-blocking suggestions:
-
httpnoctx: leakedctxTypeparameter —hasContextInEnclosingFuncstill acceptsctxType types.Typebut after migration the only role of that parameter is a nil guard; the actual matching is fully delegated toContextParamName. This leaves a misleading signature and causesContextContextTypeto be called twice. The parameter (and its call-site assignment at line 47) can be safely removed. -
TestContextHelpers: missing blank-param coverage —ContextParamNameexplicitly skips"_"-named parameters but the test doesn't exercise that path. Easy to add one extra assertion usingmakePassWithFuncType(true, "_").
Both items are refactoring opportunities rather than bugs — the migrated behavior is correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 115.4 AIC · ⌖ 8.46 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
pkg/linters/httpnoctx/httpnoctx.go:137
After the migration to astutil.ContextParamName, the ctxType types.Type parameter is now only used for a nil guard, while all actual context matching is delegated to ContextParamName which re-calls ContextContextType internally. This creates two issues:
ContextContextType(pass)is called twice perhttp.NewRequestcheck (once eagerly at line 47, once insideContextParamName).- The parameter signature is misleading — it implies the caller controls the context type, but it's ig…
pkg/linters/internal/astutil/astutil_test.go:221
TestContextHelpers doesn't cover the blank parameter name path in ContextParamName. The implementation explicitly skips "_"-named parameters (see astutil.go: if name.Name != "_"), but there is no test asserting that a function with only _ context.Context returns ("", false).
Consider adding a case using makePassWithFuncType(true, "_") and asserting ok == false:
passBlank, fnTypeBlank := makePassWithFuncType(true, "_")
if _, ok := ContextParamName(passBlank, fnTypeBlank…
</details>
There was a problem hiding this comment.
Consolidation is solid — one real regression and two test issues need fixing
The deduplication intent is correct and the shared helpers are well-structured. One issue in httpnoctx is a genuine correctness-of-API + performance regression introduced by the refactor; two test concerns are worth addressing before merge.
🔍 Findings summary
Blocking (medium):
httpnoctx.gohasContextInEnclosingFunc:ctxType types.Typeparameter is now a dead parameter — only its nil check on line 138 is used;ContextParamNameredundantly callsContextContextType(O(imports)) once per enclosing-function per call site instead of once per pass. The API implies the pre-computed type is being used for matching, but it is not.
Non-blocking (medium):
astutil_test.goTestCalledOSFunc: in-place mutation ofpass.TypesInfo.Usesto switch test scenarios is a latent data race if subtests are ever parallelised, inconsistent with thet.Parallel()pattern throughout this file.
Non-blocking (low):
astutil_test.goTestContextHelpers: blank-identifier (_) context parameter path inContextParamNameis never exercised; the explicit skip on line 97 ofastutil.gogoes untested.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 183.7 AIC · ⌖ 5.46 AIC · ⊞ 5.4K
Comment /review to run again
| return true | ||
| } | ||
| } | ||
| if _, ok := astutil.ContextParamName(pass, fnType); ok { |
There was a problem hiding this comment.
Vestigial ctxType parameter causes redundant import scanning on every loop iteration.
After this refactor, hasContextInEnclosingFunc still accepts ctxType types.Type and uses it only for the nil early-exit on line 138. But ContextParamName internally calls ContextContextType again on every invocation inside the loop. The original code computed ctxType once in run() — O(imports). After this change, ContextContextType is called O(call_sites × nesting_depth × imports).
💡 Suggested fix
Either drop the parameter and compute ctxType once inside the function, or add a helper that accepts a pre-computed type to avoid repeated import iteration:
func hasContextInEnclosingFunc(pass *analysis.Pass, cursor inspector.Cursor) bool {
ctxType := astutil.ContextContextType(pass)
if ctxType == nil {
return false
}
for enclosing := range cursor.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) {
fnType := astutil.EnclosingFuncType(enclosing.Node())
if fnType == nil || fnType.Params == nil {
continue
}
if _, ok := astutil.ContextParamName(pass, fnType); ok {
return true
}
}
return false
}The ctxType value computed at line 47 of run() is now passed into this function but only used for the nil guard — it no longer participates in the actual type comparison at all, making the parameter misleading.
| t.Fatal("CalledOSFunc() = ok=true for non-allowed name, want false") | ||
| } | ||
|
|
||
| pass.TypesInfo.Uses[selIdent] = otherFunc |
There was a problem hiding this comment.
Shared state mutation between test scenarios is a latent data race.
pass.TypesInfo.Uses[selIdent] = otherFunc mutates the shared map to switch between test scenarios within a single non-parallel test function. If this test is ever refactored into parallel subtests (consistent with the t.Parallel() pattern used throughout this file), the concurrent map writes will trigger the race detector.
💡 Suggested fix
Construct independent pass and call objects per scenario rather than mutating shared state:
makeCall := func(fn types.Object) (*analysis.Pass, *ast.CallExpr) {
id := ast.NewIdent("Getenv")
p := &analysis.Pass{
TypesInfo: &types.Info{
Uses: map[*ast.Ident]types.Object{id: fn},
},
}
c := &ast.CallExpr{Fun: &ast.SelectorExpr{X: ast.NewIdent("os"), Sel: id}}
return p, c
}
osPass, osCall := makeCall(osFunc)
if fn, ok := CalledOSFunc(osPass, osCall, "Getenv", "LookupEnv"); !ok || fn != osFunc { ... }
otherPass, otherCall := makeCall(otherFunc)
if _, ok := CalledOSFunc(otherPass, otherCall); ok { ... }| if got := ContextContextType(passWithContext); got == nil { | ||
| t.Fatal("ContextContextType() = nil, want context.Context type") | ||
| } | ||
| name, ok := ContextParamName(passWithContext, fnTypeWithContext) |
There was a problem hiding this comment.
ContextParamName blank-identifier branch (_ param name) is untested.
astutil.go line 97 explicitly skips blank-named parameters (if name.Name != "_"), returning ("", false) to signal no usable context param. This is a meaningful edge case — a function signature like func(context.Context, error) with a blank context receiver should not yield a param name. The test only exercises the happy path (ctx) and the no-context-import path; no case calls ContextParamName with a _-named parameter.
💡 Suggested addition
// Blank-named context param should not match.
passBlank, fnTypeBlank := makePassWithFuncType(true, "_")
if name, ok := ContextParamName(passBlank, fnTypeBlank); ok {
t.Fatalf("ContextParamName() = (%q, true) for blank param, want (\"\", false)", name)
}
|
@copilot please run the Unresolved review feedback to address:
Once those are addressed, rerun checks and update the PR if anything is still stale.
|
- Add //go:build !integration tag to astutil_test.go - Add blank-identifier param coverage to TestContextHelpers - Add direct *ast.Ident coverage to TestCalledOSFunc - Remove dead ctxType param from hasContextInEnclosingFunc in httpnoctx.go (ContextParamName already resolves context type internally) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all the unresolved feedback in commit
Local validation ( |
|
@copilot please run the Please also ensure the ADR link is present in the PR body before handoff. Run: https://github.com/github/gh-aw/actions/runs/28764674525
|
The linter suite had repeated helper implementations across analyzers (
enclosingFuncType, context-type/param resolution, OS-call detection, and comparison-op flipping), creating avoidable drift risk. This PR centralizes those helpers inpkg/linters/internal/astutiland rewires affected analyzers to consume the shared implementations.Shared helper consolidation
pkg/linters/internal/astutil:EnclosingFuncTypeContextContextTypeContextParamNameCalledOSFuncFlipComparisonOpAnalyzer migration to shared utilities
execcommandwithoutcontexthttpnoctxtimesleepnocontextctxbackgroundosgetenvlibraryossetenvlibrarylenstringzerostringsindexcontainsContext helper naming consistency
ctxbackgroundnow uses the shared context helper path, eliminating localcontextTypenaming divergence.Shared helper unit coverage
astutiltests to cover new shared functions and their edge cases (func-type extraction, context lookup/param selection, OS function matching, operator flipping).