Skip to content

proposal: go/ast: add PreorderStack, a wrapper around ast.Inspect that maintains a stack #73319

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.

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

Open
adonovan opened this issue Apr 10, 2025 · 1 comment
Assignees
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Apr 10, 2025

Background: It is common when using ast.Inspect to need to record a stack of nodes. The easiest way to do this is to append to a slice when n is non-nil (an "enter" event) and to slice it when n is nil (a "leave" event):

var stack []ast.Node
ast.Inspect(root, func(n ast.Node) bool {
    if n != nil {
        stack = append(stack, n) // push
    } else {
        stack = stack[:len(stack)-1] // pop
    }
    ... use n ...
    return true
})

This pattern appears dozens of times throughout x/tools. However, it is subtly wrong: if the "...use n..." portion of the code executes return false, traversal of the subtree n is skipped, and in that case the user's function is never called with a corresponding nil to indicate the "leaving n" event; the stack gets misaligned. We make this mistake nearly every time we use a stack and "return false" in the same traversal.

Proposal: We propose to provide a wrapper function around ast.Inspect that constructs the stack (correctly), passing it to the user's callback.

// PreorderStack traverses the tree rooted at root,
// calling f before visiting each node.
//
// Each call to f provides the current node and traversal stack,
// consisting of the original value of stack appended with all nodes
// from root to n, excluding n itself. (This design allows calls
// to PreorderStack to be nested without double counting.)
//
// If f returns false, the traversal skips over that subtree. Unlike
// [ast.Inspect], no second call to f is made after visiting node n.
// In practice, the second call is nearly always used only to pop the
// stack, and it is surprisingly tricky to do this correctly; see
// https://go.dev/issue/73319.
func PreorderStack(root ast.Node, stack []ast.Node, f func(n ast.Node, stack []ast.Node) bool)
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Apr 10, 2025
@gopherbot gopherbot added this to the Unreleased milestone Apr 10, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/664635 mentions this issue: internal/astutil: PreorderStack: a safer ast.Inspect for stacks

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 10, 2025
@adonovan adonovan self-assigned this Apr 12, 2025
gopherbot pushed a commit to golang/tools that referenced this issue Apr 17, 2025
This CL defines PreorderStack, a safer function than ast.Inspect
for when you need to maintain a stack.

Beware, the stack that it produces does not include n itself--a
half-open interval--so that nested traversals compose correctly.

The CL also uses the new function in various places in x/tools
where appropriate; in some cases it was clearer to rewrite
using cursor.Cursor.

+ test

Updates golang/go#73319

Change-Id: I843122cdd49cc4af8a7318badd8c34389479a92a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/664635
Auto-Submit: Alan Donovan <[email protected]>
Commit-Queue: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@adonovan adonovan changed the title x/tools: provide a wrapper around ast.Inspect that maintains a stack proposal: go/ast: add PreorderStack, a wrapper around ast.Inspect that maintains a stack Apr 18, 2025
@adonovan adonovan moved this to Incoming in Proposals Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Projects
Status: Incoming
Development

No branches or pull requests

3 participants