Skip to content

refactor: remove AstFactory in favor of new_* construction traits#10353

Merged
graphite-app[bot] merged 1 commit into
mainfrom
refactor/remove-ast-factory
Jul 19, 2026
Merged

refactor: remove AstFactory in favor of new_* construction traits#10353
graphite-app[bot] merged 1 commit into
mainfrom
refactor/remove-ast-factory

Conversation

@Boshen

@Boshen Boshen commented Jul 19, 2026

Copy link
Copy Markdown
Member

Removes the AstFactory newtype (rolldown's wrapper over oxc's AstBuilder) and moves rolldown's recurring AST constructions onto per-node-type extension traits, matching oxc's own methods-on-types construction shape. Rolldown now holds and passes oxc's AstBuilder directly.

What

  • AstFactory's ~28 inherent make_* methods become new_* associated functions on *FactoryExt traits — ExpressionFactoryExt, StatementFactoryExt, MemberExpressionFactoryExt, CallExpressionFactoryExt, BindingIdentifierFactoryExt, IdentifierNameFactoryExt, ObjectPropertyKindFactoryExt, ClassElementFactoryExt — each generic over B: GetAstBuilder<'ast> + GetAllocator<'ast> and taking the builder as its last argument (e.g. Expression::new_id_ref_expr(SPAN, name, builder)).
  • BindingPatternExt / BindingPropertyExt are made generic over the builder the same way, decoupling them from the deleted type.
  • Every holder struct's ast_factory: AstFactory field becomes ast_builder: AstBuilder; call sites go from self.ast_factory.make_x(..) to Type::new_x(.., &self.ast_builder).
  • internal-docs/ast-construction/implementation.md is updated to describe the new convention.

Why

  • Matches oxc. Construction lives on the AST types for both oxc and rolldown nodes, builder passed last.
  • Builder-last unblocks &mut construction. Passing the builder as an argument rather than a self receiver is what lets oxc later switch builder methods to &mut AstBuilder — dropping the Cells from Allocator (a hot-path win) and enabling stateful builders that auto-assign unique NodeIds.
  • Statefulness stays reintroducible. Because every constructor is generic over the builder, a future stateful wrapper (impl GetAstBuilder + GetAllocator) can be threaded through with zero call-site changes.

Notes vs the proposal

Two intentional differences from the sketch in the issue:

  • Method names keep the operation suffix (new_id_ref_expr, not new_id_ref); the new_* names are operation-based (new_keep_name_call), disjoint from oxc's variant-based constructors (new_call_expression), so none collide.
  • Holder structs keep an ast_builder field and pass &self.ast_builder, rather than implementing GetAstBuilder themselves to pass self. The borrow-check benefit still holds via argument evaluation order; the shorter self call syntax is a possible follow-up.

Closes #10328

@netlify

netlify Bot commented Jul 19, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 3c72b5f
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/6a5ceb53ea4b6e00083bb712

@codspeed-hq

codspeed-hq Bot commented Jul 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing refactor/remove-ast-factory (1584232) with main (75d8860)

Open in CodSpeed

Footnotes

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

shulaoda commented Jul 19, 2026

Copy link
Copy Markdown
Member

Merge activity

  • Jul 19, 3:20 PM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jul 19, 3:20 PM UTC: shulaoda added this pull request to the Graphite merge queue.
  • Jul 19, 3:25 PM UTC: Merged by the Graphite merge queue.

…0353)

Removes the `AstFactory` newtype (rolldown's wrapper over oxc's `AstBuilder`) and moves rolldown's recurring AST constructions onto per-node-type extension traits, matching oxc's own methods-on-types construction shape. Rolldown now holds and passes oxc's `AstBuilder` directly.

## What

- `AstFactory`'s ~28 inherent `make_*` methods become `new_*` associated functions on `*FactoryExt` traits — `ExpressionFactoryExt`, `StatementFactoryExt`, `MemberExpressionFactoryExt`, `CallExpressionFactoryExt`, `BindingIdentifierFactoryExt`, `IdentifierNameFactoryExt`, `ObjectPropertyKindFactoryExt`, `ClassElementFactoryExt` — each generic over `B: GetAstBuilder<'ast> + GetAllocator<'ast>` and taking the builder as its **last** argument (e.g. `Expression::new_id_ref_expr(SPAN, name, builder)`).
- `BindingPatternExt` / `BindingPropertyExt` are made generic over the builder the same way, decoupling them from the deleted type.
- Every holder struct's `ast_factory: AstFactory` field becomes `ast_builder: AstBuilder`; call sites go from `self.ast_factory.make_x(..)` to `Type::new_x(.., &self.ast_builder)`.
- `internal-docs/ast-construction/implementation.md` is updated to describe the new convention.

## Why

- **Matches oxc.** Construction lives on the AST types for both oxc and rolldown nodes, builder passed last.
- **Builder-last unblocks `&mut` construction.** Passing the builder as an argument rather than a `self` receiver is what lets oxc later switch builder methods to `&mut AstBuilder` — dropping the `Cell`s from `Allocator` (a hot-path win) and enabling stateful builders that auto-assign unique `NodeId`s.
- **Statefulness stays reintroducible.** Because every constructor is generic over the builder, a future stateful wrapper (`impl GetAstBuilder + GetAllocator`) can be threaded through with zero call-site changes.

## Notes vs the proposal

Two intentional differences from the sketch in the issue:

- Method names keep the operation suffix (`new_id_ref_expr`, not `new_id_ref`); the `new_*` names are operation-based (`new_keep_name_call`), disjoint from oxc's variant-based constructors (`new_call_expression`), so none collide.
- Holder structs keep an `ast_builder` field and pass `&self.ast_builder`, rather than implementing `GetAstBuilder` themselves to pass `self`. The borrow-check benefit still holds via argument evaluation order; the shorter `self` call syntax is a possible follow-up.

Closes #10328
@graphite-app
graphite-app Bot force-pushed the refactor/remove-ast-factory branch from 1584232 to 3c72b5f Compare July 19, 2026 15:20
@graphite-app
graphite-app Bot merged commit 3c72b5f into main Jul 19, 2026
34 checks passed
@graphite-app
graphite-app Bot deleted the refactor/remove-ast-factory branch July 19, 2026 15:25
graphite-app Bot pushed a commit that referenced this pull request Jul 24, 2026
Follow-on after #10018 and #10353. Pure refactor.

Implement `GetAstBuilder` and `GetAllocator` on visitor/pass/context types which hold an `AstBuilder`.

AST builder methods take any `&B where B: GetAstBuilder`, so this allows removing a lot of repetitious boilerplate code at call sites.

Before:

```rust
Expression::new_string_literal(
  SPAN,
  Str::from_str_in(id, &self.ast_builder),
  None,
  &self.ast_builder,
)
```

After:

```rust
Expression::new_string_literal(SPAN, Str::from_str_in(id, self), None, self)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: Remove AstFactory

2 participants