oxc icon indicating copy to clipboard operation
oxc copied to clipboard

Add `AstBuilder::take` method

Open overlookmotel opened this issue 8 months ago • 1 comments

#10170 replaced all the AstBuilder::move_* methods with TakeIn::take_in.

- self.ast.move_expression(expr)
+ expr.take_in(self.ast.allocator)

Shorten this code by introducing an AstBuilder::take method:

- expr.take_in(self.ast.allocator)
+ self.ast.take(expr)

AstBuilder::take would use TakeIn trait internally.

impl<'a> AstBuilder<'a> {
    /// Take the AST node and substitute a dummy node in its place.
    pub fn take<T: TakeIn<'a>>(self, value: &mut T) -> T {
        value.take_in(self.allocator)
    }
}

overlookmotel avatar Apr 08 '25 11:04 overlookmotel

Actually... there's an alternative approach we could take which is also shorter.

Define a trait AllocatorAccessor, and implement it on everything which has access to allocator:

trait AllocatorAccessor<'a> {
    fn allocator(self) -> &'a Allocator;
}

impl<'a> AllocatorAccessor<'a> for &'a Allocator {
    fn allocator(self) -> &'a Allocator {
        self
    }
}

impl<'a> AllocatorAccessor<'a> for AstBuilder<'a> {
    fn allocator(self) -> &'a Allocator {
        self.allocator
    }
}

impl<'a> AllocatorAccessor<'a> for &TraverseCtx<'a> {
    fn allocator(self) -> &'a Allocator {
        self.ast.allocator
    }
}
pub trait TakeIn<'a>: Dummy<'a> {
    fn take_in<A: AllocatorAccessor<'a>>(&mut self, allocator_accessor: A) -> Self {
        let allocator = allocator_accessor.allocator();
        let dummy = Dummy::dummy(allocator);
        mem::replace(self, dummy)
    }
}

Usage:

- expr.take_in(self.ast.allocator)
+ expr.take_in(self)

What do you think @Dunqing? Which do you think is better/more readable?

overlookmotel avatar Apr 14 '25 21:04 overlookmotel

Using impl<'a> AllocatorAccessor<'a> for &TraverseCtx<'a>:

error[E0277]: the trait bound `&mut TraverseCtx<'a>: AllocatorAccessor<'_>` is not satisfied
   --> crates/oxc_transformer/src/lib.rs:531:68
    |
531 |                 let expression = Some(expr_stmt.expression.take_in(ctx));
    |                                                            ------- ^^^ the trait `AllocatorAccessor<'_>` is not implemented for `&mut TraverseCtx<'a>`
    |                                                            |
    |                                                            required by a bound introduced by this call
    |
    = help: the trait `AllocatorAccessor<'_>` is not implemented for `&mut TraverseCtx<'a>`
            but it is implemented for `&TraverseCtx<'_>`
    = note: `AllocatorAccessor<'_>` is implemented for `&TraverseCtx<'a>`, but not for `&mut TraverseCtx<'a>`
note: required by a bound in `take_in`
   --> /Users/boshen/oxc/oxc/crates/oxc_allocator/src/take_in.rs:9:19
    |
9   |     fn take_in<A: AllocatorAccessor<'a>>(&mut self, allocator_accessor: A) -> Self {
    |                   ^^^^^^^^^^^^^^^^^^^^^ required by this bound in `TakeIn::take_in`

Using impl<'a> AllocatorAccessor<'a> for &mut TraverseCtx<'a> {:

error[E0382]: use of moved value: `ctx`
   --> crates/oxc_transformer/src/lib.rs:532:25
    |
504 |         ctx: &mut TraverseCtx<'a>,
    |         --- move occurs because `ctx` has type `&mut TraverseCtx<'_>`, which does not implement the `Copy` trait
...
527 |             for stmt in statements.iter_mut().rev() {
    |             --------------------------------------- inside of this loop
...
531 |                 let expression = Some(expr_stmt.expression.take_in(ctx));
    |                                                                    --- value moved here
532 |                 *stmt = ctx.ast.statement_return(SPAN, expression);
    |                         ^^^^^^^ value used here after move
    |
help: consider creating a fresh reborrow of `ctx` here
    |
531 |                 let expression = Some(expr_stmt.expression.take_in(&mut *ctx));

It doesn't work for &mut self. Can only reduce to expr.take_in(self.ast) but not expr.take_in(self).

Boshen avatar May 20 '25 15:05 Boshen