Skip to content

Standardize two linters on Cursor traversal and add shared astutil.Root #42719

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Jul 1, 2026
+30 −27
Merged

Standardize two linters on Cursor traversal and add shared astutil.Root #42719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
823cbee
Initial plan
Copilot Jul 1, 2026
cbc76ba
Add astutil.Root and migrate two linters to Cursor traversal
Copilot Jul 1, 2026
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
21 changes: 8 additions & 13 deletions pkg/linters/httpstatuscode/httpstatuscode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,34 +91,29 @@ var httpStatusNames = map[int]string{
}

func run(pass *analysis.Pass) (any, error) {
insp, err := astutil.Inspector(pass)
root, err := astutil.Root(pass)
if err != nil {
return nil, err
}
noLintLinesByFile := nolint.BuildLineIndex(pass, "httpstatuscode")

nodeFilter := []ast.Node{
(*ast.BinaryExpr)(nil),
(*ast.SwitchStmt)(nil),
}

insp.Preorder(nodeFilter, func(n ast.Node) {
switch node := n.(type) {
for cur := range root.Preorder((*ast.BinaryExpr)(nil), (*ast.SwitchStmt)(nil)) {
switch node := cur.Node().(type) {
case *ast.BinaryExpr:
if node.Op != token.EQL && node.Op != token.NEQ {
return
continue
}
lit, other := extractStatusLiteral(node)
if lit == nil {
return
continue
}
if !isHTTPStatusContext(pass, other) {
return
continue
}
checkAndReport(pass, lit, noLintLinesByFile)
case *ast.SwitchStmt:
if node.Tag == nil || !isHTTPStatusContext(pass, node.Tag) {
return
continue
}
for _, s := range node.Body.List {
cc, ok := s.(*ast.CaseClause)
Expand All @@ -134,7 +129,7 @@ func run(pass *analysis.Pass) (any, error) {
}
}
}
})
}

return nil, nil
}
Expand Down
10 changes: 10 additions & 0 deletions pkg/linters/internal/astutil/astutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,16 @@ func Inspector(pass *analysis.Pass) (*inspector.Inspector, error) {
return insp, nil
}

// 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) {

@github-actions github-actions Bot Jul 1, 2026

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.

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:

  1. Remove Root() and keep one convention — callers write insp.Root() themselves (one extra call, zero ambiguity).
  2. 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.

@github-actions github-actions Bot Jul 1, 2026

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.

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

insp, err := Inspector(pass)
if err != nil {
return inspector.Cursor{}, err
}

@github-actions github-actions Bot Jul 1, 2026

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.

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.

return insp.Root(), nil

@github-actions github-actions Bot Jul 1, 2026

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] 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.

}

// NodeText formats node as Go source text using go/printer.
func NodeText(fset *token.FileSet, node ast.Node) string {
var buf bytes.Buffer
Expand Down
26 changes: 12 additions & 14 deletions pkg/linters/sortslice/sortslice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,47 +25,45 @@ var Analyzer = &analysis.Analyzer{
}

func run(pass *analysis.Pass) (any, error) {
insp, err := astutil.Inspector(pass)
root, err := astutil.Root(pass)
if err != nil {
return nil, err
}
noLintLinesByFile := nolint.BuildLineIndex(pass, "sortslice")

nodeFilter := []ast.Node{(*ast.CallExpr)(nil)}

insp.Preorder(nodeFilter, func(n ast.Node) {
call, ok := n.(*ast.CallExpr)
for cur := range root.Preorder((*ast.CallExpr)(nil)) {
call, ok := cur.Node().(*ast.CallExpr)
if !ok {

@github-actions github-actions Bot Jul 1, 2026

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.

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.

@github-actions github-actions Bot Jul 1, 2026

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.

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

return
continue

@github-actions github-actions Bot Jul 1, 2026

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.

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.

}

pos := pass.Fset.PositionFor(call.Pos(), false)
if filecheck.IsTestFile(pos.Filename) {
return
continue
}
if nolint.HasDirective(pos, noLintLinesByFile) {
return
continue
}

sel, ok := call.Fun.(*ast.SelectorExpr)
if !ok {
return
continue
}
pkgIdent, ok := sel.X.(*ast.Ident)
if !ok {
return
continue
}
if pass.TypesInfo == nil {
return
continue
}
obj := pass.TypesInfo.ObjectOf(pkgIdent)
// ObjectOf can be nil when type information is incomplete.
if obj == nil {
return
continue
}
pkgName, ok := obj.(*types.PkgName)
if !ok || pkgName.Imported().Path() != "sort" {
return
continue
}

switch sel.Sel.Name {
Expand All @@ -75,7 +73,7 @@ func run(pass *analysis.Pass) (any, error) {
case "SliceStable":
pass.ReportRangef(call, "sort.SliceStable is not type-safe; use slices.SortStableFunc instead")
}
})
}

return nil, nil
}
Loading