AstBuilder pattern

The pattern for Oxc's new version of AstBuilder is:

• All builder methods are implemented on AST types themselves.
• They all take a &B where B: GetAstBuilder.
• GetAstBuilder is implemented on all structs which hold an AstBuilder e.g. ParserImpl, TraverseCtx.

impl<'a> UnaryExpression<'a> {
    pub fn new<B: GetAstBuilder<'a>>(
        span: Span,
        operator: UnaryOperator,
        argument: Expression<'a>,
        builder: &B,
    ) -> Self {
        let builder = builder.builder();
        UnaryExpression { node_id: Cell::new(builder.node_id()), span, operator, argument }
    }
}

The B: GetAstBuilder generic shortens call sites - you can pass self rather than e.g. &self.ast_builder:

// `self` here is e.g. `&TraverseCtx`
let unary_expr = UnaryExpression::new(span, argument, self);

Proposal

I propose to migrate AstFactory to the same pattern.

Before:

self.ast_factory.make_id_ref_expr(SPAN, &binding_name)

After:

Expression::new_id_ref(SPAN, &binding_name, self)

All methods which are currently methods of AstFactory would become e.g.:

pub trait ExpressionMethods<'ast> {
    fn new_id_ref<B: GetAstBuilder<'ast>>(span: Span, name: &str, builder: &B) -> Expression<'ast> {
        Expression::new_identifier(span, Str::from_str_in(name, builder), builder)
    }
}

impl<'ast> ExpressionMethods<'ast> for Expression<'ast> {}

Having to use traits is annoying, but it's the only way Rolldown can implement methods on Oxc's foreign types.

We can, however, make it painless at call sites. API users would just add at top of file:

use rolldown_ecmascript_utils::ast_factory::*;

Once this change is made, AstFactory can be deleted altogether - Rolldown would just use Oxc's AstBuilder directly.

Why?

The main reasons we went with the new design of AstBuilder in Oxc are:

1. Enables making AstBuilder stateful to support automatically assigning unique NodeIds to all AST nodes.
2. Shorter syntax - pass self, not self.ctx.ast_builder.
3. Passing self as last param avoids borrow-checker problems. Patterns like this pass borrow-check where they wouldn't before:

// `generate_id` takes `&mut self`

// New pattern - this compiles fine
let id = Expression::new_identifier(span, self.generate_id(), self);

// Old pattern - borrow-check refuses (`self` already borrowed)
let id = self.ast_builder.identifier(span, self.generate_id());

IMO it's important to make assignment of unique NodeIds consistent and automatic across both Oxc and Rolldown, so that NodeIds can be used reliably as unique identifiers. This proposed change to AstFactory would make this goal easier to achieve.

Further changes to come

There is a further change I'd like to make to AstBuilder. It'd be ideal if methods took a &mut AstBuilder not &AstBuilder. This would allow us to make AstBuilder stateful without having to use interior mutability (Cells), which would be both a performance gain and a simplification.

That change should work with few code changes to either Oxc or Rolldown if we use the new methods-on-AST-types pattern, but would be really unpleasant with the old pattern - borrow-checker will go bananas.

Alternative

If Rolldown team strongly feel that you want to keep AstFactory as it is, we could work around its limitations and achieve unique NodeIds.

However, we'd either have to lose the "further changes to come" idea, or make a lot of tortuous changes to Rolldown to work around borrow-checker problems.

Who does it?

If you accept my proposal, I would be very happy to make a PR for the changes. The codemod we used to migrate Oxc over to new AstBuilder could be easily adapted to do this migration too.