refactor: identify AST nodes by NodeId instead of Span/Address by IWANABETHATGUY · Pull Request #9609 · rolldown/rolldown

IWANABETHATGUY · 2026-05-30T09:01:18Z

Summary

Replaces the two ad-hoc cross-pass AST-node identity keys — oxc Span (deduped by PreProcessor) and arena Address — with oxc's post-semantic NodeId for every side table threaded from scan → link → finalize:

EcmaView::imports, dummy_record_set, new_url_references, this_expr_replace_map
MemberExprRef::node_id / LinkingMetadata::resolved_member_expr_refs
DynamicImportExprInfo::node_id / EntryPoint::related_stmt_infos
cross-module-optimization state (side-effect-free calls, unreachable dynamic imports)

Cross-module tables are keyed by (ModuleIdx, NodeId) because node ids are unique only within a single AST. Span is retained purely as source-location metadata (diagnostics, source maps, generated replacement spans); import records now carry an explicit importer_span so the TLA import-chain diagnostic no longer needs a reverse lookup over the (formerly span-keyed) imports map.

This removes the implicit "spans are stable and unique within a module" contract along with the PreProcessor synthetic-span workaround: finalizer-generated nodes keep NodeId::DUMMY and can no longer collide with scan-time records.

Notes

The incremental-cache (make_copy) and HMR paths finalize a clone of the scanned AST; they rely on clone_in_with_semantic_ids (via EcmaAst::clone_with_another_arena) to preserve node ids across the arena boundary, where plain clone_in would reset them to NodeId::DUMMY.
meta/design/ast-mutation.md is rewritten to document the new NodeId contract.

🤖 Generated with Claude Code

IWANABETHATGUY · 2026-05-30T09:01:31Z

refactor: identify AST nodes by NodeId instead of Span/Address #9609 👈 (View in Graphite)
main

How to use the Graphite Merge Queue

Add the label graphite: merge-when-ready to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

netlify · 2026-05-30T09:02:25Z

✅ Deploy Preview for rolldown-rs canceled.

Name	Link
🔨 Latest commit	`44cb85c`
🔍 Latest deploy log	https://app.netlify.com/projects/rolldown-rs/deploys/6a1aaecb57946700088403a6

IWANABETHATGUY · 2026-06-04T04:57:56Z

@overlookmotel Thanks for the review. However, I still prefer using NodeId to identify nodes, for the following reasons:

Address is more error-prone. I know it is safe to use in most cases, but it can still potentially introduce bugs when unstable_address is involved.
Address comparison is indeed faster than NodeId comparison, but for Rolldown this should be trivial and probably not even observable, since AST traversal is only a small part of the overall pipeline.
If we keep using Address in some places, we would end up with two ways to identify a node, and three ways to inspect a node if we also count Span, which is used for diagnostics. This would make the code a bit messy.

Overall, I would prefer to use NodeId consistently for all AST node identification.

codspeed-hq · 2026-06-04T05:05:44Z

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks¹

_{Comparing chore/node-id-migration (44cb85c) with main (645ed62)}

10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

overlookmotel · 2026-06-04T11:55:49Z

OK, fair enough. I tend towards prioritising perf myself, but obviously simplicity has large advantages - your rationale makes total sense.

My understanding of where we're at with NodeId in Oxc is:

AST which has come out of Semantic: all AST nodes have unique NodeIds defined.
New AST nodes created in transformer / minifier: have dummy NodeId (0).

We need to fix the 2nd, but it wasn't on immediate roadmap. We'll likely need to have 2 x AstBuilders with/without NodeId creation. If it's important to Rolldown, we can bump it up the priority list.

This PR: What I haven't figured out yet is whether it's going to hit problems due to the dummy NodeIds post-transform. I'd need to explore Rolldown (with an LLM) and get my head around the overall flow. I'm keen to do that - I've felt for a long time that I need to understand Rolldown much better - and this could be a good opportunity.

So... would you like to wait for me to do that and then review this PR (likely next week)? Or would you like to assess yourselves and get this merged earlier?

Sorry, I had hoped to get into it properly this week, but... other stuff has been going on.

IWANABETHATGUY · 2026-06-04T13:36:42Z

OK, fair enough. I tend towards prioritising perf myself, but obviously simplicity has large advantages - your rationale makes total sense.

My understanding of where we're at with NodeId in Oxc is:

AST which has come out of Semantic: all AST nodes have unique NodeIds defined.

New AST nodes created in transformer / minifier: have dummy NodeId (0).

We need to fix the 2nd, but it wasn't on immediate roadmap. We'll likely need to have 2 x AstBuilders with/without NodeId creation. If it's important to Rolldown, we can bump it up the priority list.

This PR: What I haven't figured out yet is whether it's going to hit problems due to the dummy NodeIds post-transform. I'd need to explore Rolldown (with an LLM) and get my head around the overall flow. I'm keen to do that - I've felt for a long time that I need to understand Rolldown much better - and this could be a good opportunity.

So... would you like to wait for me to do that and then review this PR (likely next week)? Or would you like to assess yourselves and get this merged earlier?

Sorry, I had hoped to get into it properly this week, but... other stuff has been going on.

No rush. I’ll wait until you finish the review before merging it.

## Summary Adds `meta/design/ast-construction.md` documenting how rolldown should construct oxc AST, and cross-links it from `ast-mutation.md`. The convention: - **Generic nodes → oxc's `AstBuilder` directly** (handle named `ast`). - **Rolldown-specific patterns → an `AstBuilderExt` extension trait**, methods prefixed `make_` and named after the operation (e.g. `make_to_esm_wrapper`), mirroring oxc's builder signature style. The call site self-identifies (bare node name = oxc; `make_*` = rolldown) and avoids inherent-vs-trait method shadowing. - **Build programmatically by default; parsing source is an exception.** Direct construction has no runtime cost; parsing pays lexing+parsing overhead every build. Authoring code as JS and parsing it (`EcmaCompiler::parse`) is reserved for a large fixed blob — in practice just the runtime module. - The `AstSnippet` facade is retired over time (its author already noted the name was a compromise for `AstBuilder`). ## Background oxc made `AstBuilder` the single sanctioned construction path (`#[non_exhaustive]` on `NodeId`-bearing nodes, oxc#23046, handled in #9670) and is redesigning it further (oxc#23043, which cites #9609). This doc aligns rolldown with that direction instead of growing a divergent local layer. ## Notes - **Docs-only; no code changes.** - The `## Plan` section is **temporary** and will be removed once the migration it describes is implemented. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

hyf0 · 2026-06-10T12:45:06Z

So... would you like to wait for me to do that and then review this PR (likely next week)? Or would you like to assess yourselves and get this merged earlier?

@overlookmotel

There isn't really a hurry for this, but I also get sometimes we want things to get too pefect to be done. If you could finish the review in time that's good, if you can't that's fine too. On the other hands, merging this would be also a way of getting feedbacks for the node id design. Will plan to merge this after this weeks' release, so we get a full week time to revert this PR if you find some significant downsides.

I wanna push this forward, because I'm looking at the some internal improvement/refactor. I want the future refactor can build on top of the new basis. It might not be perfect, but we still want to fix bugs on top of the new basis instead of the old one.

github-actions Bot assigned IWANABETHATGUY May 30, 2026

IWANABETHATGUY force-pushed the chore/node-id-migration branch from 698e338 to 77574a0 Compare May 30, 2026 09:19

u

5a92de4

IWANABETHATGUY force-pushed the chore/node-id-migration branch from 77574a0 to 5a92de4 Compare May 30, 2026 09:31

[autofix.ci] apply automated fixes

44cb85c

IWANABETHATGUY changed the title u refactor: identify AST nodes by NodeId instead of Span/Address May 30, 2026

overlookmotel reviewed Jun 1, 2026

View reviewed changes

Comment thread crates/rolldown/src/ast_scanner/impl_visit.rs

IWANABETHATGUY marked this pull request as ready for review June 4, 2026 04:59

IWANABETHATGUY requested review from hyf0 and overlookmotel June 4, 2026 04:59

IWANABETHATGUY marked this pull request as draft June 4, 2026 13:37

overlookmotel mentioned this pull request Jun 7, 2026

Redesign AstBuilder oxc-project/oxc#23043

Open

hyf0 mentioned this pull request Jun 8, 2026

docs(design): plan to unify all internal AST construction #9673

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: identify AST nodes by NodeId instead of Span/Address#9609

refactor: identify AST nodes by NodeId instead of Span/Address#9609
IWANABETHATGUY wants to merge 2 commits into
mainfrom
chore/node-id-migration

IWANABETHATGUY commented May 30, 2026 •

edited

Loading

Uh oh!

IWANABETHATGUY commented May 30, 2026

Uh oh!

netlify Bot commented May 30, 2026 •

edited

Loading

Uh oh!

Uh oh!

IWANABETHATGUY commented Jun 4, 2026

Uh oh!

codspeed-hq Bot commented Jun 4, 2026

Uh oh!

overlookmotel commented Jun 4, 2026 •

edited

Loading

Uh oh!

IWANABETHATGUY commented Jun 4, 2026

Uh oh!

hyf0 commented Jun 10, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

IWANABETHATGUY commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notes

Uh oh!

IWANABETHATGUY commented May 30, 2026

How to use the Graphite Merge Queue

Uh oh!

netlify Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for rolldown-rs canceled.

Uh oh!

Uh oh!

IWANABETHATGUY commented Jun 4, 2026

Uh oh!

codspeed-hq Bot commented Jun 4, 2026

Merging this PR will not alter performance

Footnotes

Uh oh!

overlookmotel commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IWANABETHATGUY commented Jun 4, 2026

Uh oh!

hyf0 commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

IWANABETHATGUY commented May 30, 2026 •

edited

Loading

netlify Bot commented May 30, 2026 •

edited

Loading

overlookmotel commented Jun 4, 2026 •

edited

Loading

hyf0 commented Jun 10, 2026 •

edited

Loading