Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TakeIn trait to replace AstBuilder::move_* methods #5888

Open
overlookmotel opened this issue Aug 31, 2024 · 2 comments
Open

TakeIn trait to replace AstBuilder::move_* methods #5888

overlookmotel opened this issue Aug 31, 2024 · 2 comments

Comments

@overlookmotel
Copy link
Collaborator

overlookmotel commented Aug 31, 2024

Currently AstBuilder has move_expression, move_statement, move_assignment_target, move_declaration and move_vec methods.

Combine them into a single AstBuilder::take method with a new Take trait:

pub trait Take<'a> {
    fn replacement(ast: AstBuilder<'a>) -> Self;
}

impl<'a> Take<'a> for Expression<'a> {
    fn replacement(ast: AstBuilder<'a>) -> Self {
        ast.expression_null_literal(oxc_span::SPAN)
    }
}

impl<'a, T> Take<'a> for Vec<'a, T> {
    fn replacement(ast: AstBuilder<'a>) -> Self {
        ast.vec()
    }
}

// etc

impl<'a> AstBuilder<'a> {
    pub fn take<T: Take<'a>>(self, value: &mut T) -> T {
        std::mem::replace(value, T::replacement(self))
    }
}

Usage:

let expr: &mut Expression = get_mut_expression_somehow();
let owned_expr: Expression = ast.take(expr);

NB: Reason why Take::replacement takes an AstBuilder<'a> instead of an &'a Allocator is to be forwards compatible with when all AST nodes need an ID and AstBuilder is stateful.

Related to oxc-project/backlog#101.

@overlookmotel
Copy link
Collaborator Author

overlookmotel commented Sep 19, 2024

Actually, since purpose of Take is to create dummy nodes which are only temporarily inserted into AST, they should probably have dummy NodeIds (i.e. 0).

So Take methods can take &Allocator instead of AstBuilder.

If we can avoid an allocation side effect for at least some Take methods (oxc-project/backlog#104 (comment)) then using NodeId::DUMMY which also has no side effect (doesn't increment node ID counter), makes it more likely that compiler can understand that writing the dummy node into AST can be elided, since that operation has no side effects and the dummy node is replaced again shortly after.

@overlookmotel
Copy link
Collaborator Author

overlookmotel commented Sep 19, 2024

As part of adding node IDs to AST types, we added #[non_exhaustive] to all AST types. But it had to be reverted (#5885) because of Rolldown's TakeIn trait, which is basically the same as what I've suggested here.

https://github.com/rolldown/rolldown/blob/main/crates/rolldown_ecmascript/src/allocator_helpers/take_in/impl_for_oxc_ast.rs

If we implement our own TakeIn trait, we can:

  • Remove TakeIn trait from Rolldown, and switch Rolldown over to using oxc_ast::TakeIn.
  • Apply #[non_exhaustive] to AST types again.

Or, if we don't make node_id fields public (#5689 (comment)), that will have same effect as #[non_exhaustive].

We can generate TakeIn with AST tools via #[generate_derive(TakeIn)] (I assume this wouldn't be too tricky, right @rzvxa?)

Probably we should only implement TakeIn on a small subset of types

  1. to avoid generating reams of unnecessary code.
  2. because some types are much cheaper to create dummys for than others - generating a dummy for some types involves multiple allocations. So it's preferable to push people towards the cheaper dummy types.

@overlookmotel overlookmotel changed the title Take trait to replace AstBuilder::move_* methods TakeIn trait to replace AstBuilder::move_* methods Sep 19, 2024
@overlookmotel overlookmotel transferred this issue from oxc-project/backlog Sep 19, 2024
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

No branches or pull requests

1 participant