Standardize two linters on Cursor traversal and add shared astutil.Root#42719
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
astutil.Root
There was a problem hiding this comment.
Pull request overview
This PR continues the repository’s x/tools modernization by moving two Go analyzers from legacy Inspector.Preorder callback traversal to the newer Cursor-based traversal API, and centralizes the common “get inspector root cursor” logic behind a shared helper.
Changes:
- Added
astutil.Root(pass)to return aninspector.Cursorwith a shared error path for missing/invalidinspectresults. - Migrated
sortsliceto iterateroot.Preorder((*ast.CallExpr)(nil))while preserving existing filtering and diagnostics. - Migrated
httpstatuscodeto iterateroot.Preorder((*ast.BinaryExpr)(nil), (*ast.SwitchStmt)(nil))while preserving existing diagnostic behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/internal/astutil/astutil.go | Adds shared Root(*analysis.Pass) helper that returns the inspector root cursor (via existing Inspector(pass) extraction). |
| pkg/linters/sortslice/sortslice.go | Replaces callback-based traversal with cursor iteration over *ast.CallExpr nodes. |
| pkg/linters/httpstatuscode/httpstatuscode.go | Replaces callback-based traversal with cursor iteration over *ast.BinaryExpr and *ast.SwitchStmt nodes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42719 does not have the 'implementation' label and has only 30 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Non-blocking observations
The Cursor migration is mechanically correct — no behavioral regressions identified. All continue/return replacements are semantically equivalent, and both linters preserve their diagnostic logic.
Findings (non-blocking)
1. Dead type-assertion guard in sortslice.go — cur.Node().(*ast.CallExpr) after Preorder((*ast.CallExpr)(nil)) is guaranteed to succeed; the !ok branch is unreachable dead code copied mechanically from the old callback style. Use the single-value assertion form.
2. Zero-value inspector.Cursor{} on error in Root — Unlike the nil-pointer returned by Inspector, a zero Cursor may silently produce zero iterations when the error is accidentally ignored, masking bugs rather than causing an obvious panic. Minimal godoc addition or a panic on unexpected type would keep the fail-fast contract.
🔎 Code quality review by PR Code Quality Reviewer · 87.9 AIC · ⌖ 9.81 AIC · ⊞ 1.6K
Comment /review to run again
| call, ok := n.(*ast.CallExpr) | ||
| for cur := range root.Preorder((*ast.CallExpr)(nil)) { | ||
| call, ok := cur.Node().(*ast.CallExpr) | ||
| if !ok { |
There was a problem hiding this comment.
Dead type-assertion guard: cur.Node().(*ast.CallExpr) always succeeds here and the !ok branch is unreachable dead code.
💡 Detail
root.Preorder((*ast.CallExpr)(nil)) passes *ast.CallExpr as the sole filter type, so the inspector package guarantees every yielded cursor holds exactly a *ast.CallExpr. The two-value form call, ok := cur.Node().(*ast.CallExpr) will therefore always set ok = true.
The if !ok { continue } guard is dead code — a mechanical carry-over from the old callback style.
Recommended fix:
// Single-value assertion is safe — guaranteed by Preorder filter:
call := cur.Node().(*ast.CallExpr)Leaving the dead branch misleads reviewers into thinking it is reachable, and silently becomes a masked bug if the filter type is later changed without updating the assertion.
| insp, err := Inspector(pass) | ||
| if err != nil { | ||
| return inspector.Cursor{}, err | ||
| } |
There was a problem hiding this comment.
Weaker error sentinel for value-type return: returning inspector.Cursor{} on error is harder to detect than the nil pointer returned by Inspector.
💡 Detail
Inspector returns (*inspector.Inspector, error); a caller that ignores the error gets a nil pointer that will immediately panic on first use — a loud, fast failure.
Root returns (inspector.Cursor, error); a caller that ignores the error gets a zero inspector.Cursor{}. Depending on the inspector library's behaviour for the zero value, this will either silently produce zero iterations (masking the bug entirely) or panic with a less diagnostic stack trace.
Consider documenting the zero-value behaviour explicitly, or panicking eagerly on error to match the fail-fast contract of the surrounding helpers:
// Option A — document the risk in the godoc:
// If the error is non-nil, the returned Cursor is the zero value and
// must not be used; iterating over it will produce zero results or panic.
// Option B — keep only Inspector exposed publicly and let callers call .Root() themselves,
// preserving the nil-pointer sentinel for the error path.|
|
||
| // Root extracts the inspector root cursor from pass.ResultOf. | ||
| // It returns an error if the inspect result has an unexpected type. | ||
| func Root(pass *analysis.Pass) (inspector.Cursor, error) { |
There was a problem hiding this comment.
Convention fork with no migration plan: Root() creates a second pattern for obtaining an AST cursor alongside the one already used by all 9 existing Cursor-style linters.
💡 Detail
All 9 other linters that already use Cursor traversal do this:
insp, err := astutil.Inspector(pass)
if err != nil { return nil, err }
// ...
for cur := range insp.Root().Preorder(...) { ... }This PR introduces Root() and migrates exactly 2 linters to it, leaving 9 linters on the old pattern. The codebase now has two conventions for the same one-liner with no documented preference.
The helper also doesn't fully replace Inspector(): any linter needing WithStack or other *inspector.Inspector methods still needs Inspector() directly, so Root() can't be universally adopted anyway.
Options:
- Remove
Root()and keep one convention — callers writeinsp.Root()themselves (one extra call, zero ambiguity). - Migrate all 11 linters to
Root()in this PR so the new convention is consistent from the start.
Option 1 is lower risk. Option 2 requires touching 9 more files.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — no blocking issues, three targeted suggestions.
📋 Key Themes & Highlights
Key Themes
- Dead-code guard in
sortslice:cur.Node().(*ast.CallExpr)is filtered byPreorder, so the!okbranch can never trigger — it should use a plain (non-ok) assertion. - Incomplete pattern migration:
astutil.Rootis a good helper, but four existing linters still callastutil.Inspector(pass)+.Root()directly (deferinloop,timesleepnocontext,execcommandwithoutcontext,panic-in-library-code). Two coexisting patterns reduce navigability. - Missing unit test for
Root: All otherastutilhelpers have direct unit tests;Root's error path is only implicitly covered by analyser-level tests.
Positive Highlights
- Clean migration from callback to iterator style — the
returntocontinuemapping is correct throughout. astutil.Rootdoc comment is clear and matches the style ofInspector.- Net line reduction while improving readability — good trade-off.
- No diagnostic behaviour changed; existing
analysistesttest data validates correctness.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 70.5 AIC · ⌖ 6.89 AIC · ⊞ 6.6K
Comment /matt to run again
| call, ok := n.(*ast.CallExpr) | ||
| for cur := range root.Preorder((*ast.CallExpr)(nil)) { | ||
| call, ok := cur.Node().(*ast.CallExpr) | ||
| if !ok { |
There was a problem hiding this comment.
[/codebase-design] The !ok guard is dead code — Preorder((*ast.CallExpr)(nil)) guarantees only *ast.CallExpr nodes are yielded, so the type assertion will never fail.
💡 Suggested simplification
Replace:
call, ok := cur.Node().(*ast.CallExpr)
if !ok {
continue
}With:
call := cur.Node().(*ast.CallExpr)The node-type filter passed to Preorder is a compile-time guarantee that cur.Node() returns exactly the filtered type. Keeping the ok branch misleads readers into thinking the assertion could fail. The same pattern applies in httpstatuscode.go for the cur.Node() type-switch — though there the switch default is simply a no-op, not dead code per se.
@copilot please address this.
|
|
||
| // Root extracts the inspector root cursor from pass.ResultOf. | ||
| // It returns an error if the inspect result has an unexpected type. | ||
| func Root(pass *analysis.Pass) (inspector.Cursor, error) { |
There was a problem hiding this comment.
[/codebase-design] Root is a thin two-liner that delegates straight to Inspector + .Root(). Consider whether this helper justifies its own named function in the public surface.
💡 Context
The helper provides real value by centralising the error-handling pattern and giving call sites a single import. However, several existing linters (deferinloop, timesleepnocontext, execcommandwithoutcontext, panic-in-library-code) already call astutil.Inspector(pass) and then chain .Root() directly — they were not migrated in this PR. As a result, two equivalent patterns now coexist in the codebase:
// pattern A (existing)
insp, err := astutil.Inspector(pass)
for cur := range insp.Root().Preorder(...) {
// pattern B (new, this PR)
root, err := astutil.Root(pass)
for cur := range root.Preorder(...) {To make Root worthwhile, the remaining linters should be migrated too — or a note should be added to CONTEXT.md / the PR description explaining that follow-up migration is planned.
@copilot please address this.
| if err != nil { | ||
| return inspector.Cursor{}, err | ||
| } | ||
| return insp.Root(), nil |
There was a problem hiding this comment.
[/tdd] astutil.Root has no direct unit test — only Inspector is implicitly exercised through analyzer tests. An explicit unit test would lock in the error-path behaviour.
💡 Suggested test (alongside existing `TestIsPkgSelector`)
func TestRoot_InvalidResult(t *testing.T) {
t.Parallel()
pass := &analysis.Pass{
ResultOf: map[*analysis.Analyzer]any{
inspect.Analyzer: "not an inspector",
},
}
_, err := Root(pass)
if err == nil {
t.Fatal("expected error for unexpected result type, got nil")
}
}This mirrors the existing pattern for Inspector and ensures the error message is preserved through the delegation layer.
@copilot please address this.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
Review: Standardize two linters on Cursor traversal and add shared astutil.Root
Overall this is a clean, well-executed refactor. The astutil.Root() helper is a natural extension of astutil.Inspector() and the migration from callback-style Preorder to range-based Cursor iteration is correct for both linters.
One non-blocking issue worth cleaning up:
Dead code guard in sortslice.go (and pre-existing in others)
call, ok := cur.Node().(*ast.CallExpr)
if !ok {
continue
}When iterating with root.Preorder((*ast.CallExpr)(nil)), the type filter guarantees every yielded cursor holds exactly an *ast.CallExpr — the !ok branch is never reachable. The same dead-code guard was copied from the pre-existing timeafterleak.go / deferinloop.go callers. Simplifying to a direct assertion would be cleaner and signals to the reader that the filter provides that guarantee.
Other observations
httpstatuscode.go: Theswitch node := cur.Node().(type)idiom is correct here because two node types are filtered — no dead code.- Existing callers like
timeafterleak,ctxbackground,deferinloop, andhttpnoctxstill use theinsp.Root().Preorder()inline pattern rather thanastutil.Root(). A follow-up PR to migrate those would make the codebase fully consistent. - No test for
astutil.Root(), but this matches the existing coverage posture forastutil.Inspector().
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.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 81.7 AIC · ⌖ 6.99 AIC · ⊞ 4.9K
| call, ok := cur.Node().(*ast.CallExpr) | ||
| if !ok { | ||
| return | ||
| continue |
There was a problem hiding this comment.
The !ok guard (lines 35–37) is unreachable dead code: root.Preorder((*ast.CallExpr)(nil)) only ever yields *ast.CallExpr nodes, so the type assertion always succeeds and the continue is never taken.
Consider simplifying to a direct assertion:
for cur := range root.Preorder((*ast.CallExpr)(nil)) {
call := cur.Node().(*ast.CallExpr)
// ...
}(The same pattern exists pre-existing in timeafterleak.go and deferinloop.go — a follow-up to clean those up would make all Cursor-based linters consistent.)
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
This change continues the x/tools alignment work by moving selected analyzers off legacy
Inspector.Preordercallbacks to the recommendedCursortraversal style. It also introduces a shared helper so Cursor-based analyzers use one extraction/error path.Summary
astutil.Root(pass)helper returninginspector.Cursor.sortsliceandhttpstatuscodefrom callback-based traversal to Cursor iteration, preserving existing diagnostic behavior.Changes
pkg/linters/internal/astutil/astutil.gopkg/linters/sortslice/sortslice.goinsp.Preorder(nodeFilter, func(n ast.Node) {...})withfor cur := range root.Preorder((*ast.CallExpr)(nil)) { ... }pkg/linters/httpstatuscode/httpstatuscode.goRepresentative pattern