refactor: remove AstFactory in favor of new_* construction traits#10353
Merged
Conversation
Boshen
requested review from
IWANABETHATGUY,
hyfdev,
sapphi-red and
shulaoda
as code owners
July 19, 2026 12:24
✅ Deploy Preview for rolldown-rs canceled.
|
Merging this PR will not alter performance
Comparing Footnotes
|
shulaoda
approved these changes
Jul 19, 2026
Member
Merge activity
|
…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
Bot
force-pushed
the
refactor/remove-ast-factory
branch
from
July 19, 2026 15:20
1584232 to
3c72b5f
Compare
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) ```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes the
AstFactorynewtype (rolldown's wrapper over oxc'sAstBuilder) 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'sAstBuilderdirectly.What
AstFactory's ~28 inherentmake_*methods becomenew_*associated functions on*FactoryExttraits —ExpressionFactoryExt,StatementFactoryExt,MemberExpressionFactoryExt,CallExpressionFactoryExt,BindingIdentifierFactoryExt,IdentifierNameFactoryExt,ObjectPropertyKindFactoryExt,ClassElementFactoryExt— each generic overB: GetAstBuilder<'ast> + GetAllocator<'ast>and taking the builder as its last argument (e.g.Expression::new_id_ref_expr(SPAN, name, builder)).BindingPatternExt/BindingPropertyExtare made generic over the builder the same way, decoupling them from the deleted type.ast_factory: AstFactoryfield becomesast_builder: AstBuilder; call sites go fromself.ast_factory.make_x(..)toType::new_x(.., &self.ast_builder).internal-docs/ast-construction/implementation.mdis updated to describe the new convention.Why
&mutconstruction. Passing the builder as an argument rather than aselfreceiver is what lets oxc later switch builder methods to&mut AstBuilder— dropping theCells fromAllocator(a hot-path win) and enabling stateful builders that auto-assign uniqueNodeIds.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:
new_id_ref_expr, notnew_id_ref); thenew_*names are operation-based (new_keep_name_call), disjoint from oxc's variant-based constructors (new_call_expression), so none collide.ast_builderfield and pass&self.ast_builder, rather than implementingGetAstBuilderthemselves to passself. The borrow-check benefit still holds via argument evaluation order; the shorterselfcall syntax is a possible follow-up.Closes #10328