-
Notifications
You must be signed in to change notification settings - Fork 0
Description
oxc-project/oxc#3890 converted AstBuilder
to be generated with a codegen. Great!
However, I feel AstBuilder
's API is a bit unpleasant and confusing. I think this is probably inevitable for any type with so many methods.
At its worst, we have:
fn array_expression(...) -> ArrayExpression<'a>
fn alloc_array_expression(...) -> Box<'a, ArrayExpression<'a>>
fn expression_array(...) -> Expression<'a>
fn expression_from_array(...) -> Expression<'a>
This is confusing. array_expression
or expression_array
?
How about we split up builder to be methods on types themselves? i.e.:
Replace:
// ArrayExpression
let arr_expr = self.ast.array_expression(span, elements, comma);
// Box<ArrayExpression>
let boxed_arr_expr = self.ast.alloc_array_expression(span, elements, comma);
// Expression
let expr = self.ast.expression_array(span, elements, comma);
// Expression
let expr = self.ast.expression_from_array(boxed_arr_expr);
with:
// ArrayExpression
let arr_expr = ArrayExpression::new(self, span, elements, comma);
// Box<ArrayExpression>
let boxed_arr_expr = ArrayExpression::boxed(self, span, elements, comma);
// Expression
let expr = ArrayExpression::expression(self, span, elements, comma);
// Expression
let expr = Expression::ArrayExpression(boxed_arr_expr);
// or...
let expr = ArrayExpression::expression_from(self, arr_expr);
The advantage, as I see it, is that the type name ArrayExpression
is always present. This makes it clearer when reading the code what type is being created.
In an IDE with syntax highlighting, I find the type names "jump out" a bit more and make it easier to understand:

NB: These methods take self
rather than self.ast
. We can achieve this compact syntax by making all methods take impl Builder
where Builder
is a trait which is implemented for all types which have access to AstBuilder
e.g. ParserImpl
, TransformCtx
, TraverseCtx
.