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

Alternative API for getting TraverseCtx in #3260

Closed
overlookmotel opened this issue May 13, 2024 · 3 comments
Closed

Alternative API for getting TraverseCtx in #3260

overlookmotel opened this issue May 13, 2024 · 3 comments
Assignees
Labels
C-enhancement Category - New feature or request

Comments

@overlookmotel
Copy link
Collaborator

overlookmotel commented May 13, 2024

@Dunqing was asking in #3216 for a more ergonomic way to pass TraverseCtx around in transformers.

#3219 is one option. Here's another, which is more like Babel's Path API:

Instead of receiving AST node and TraverseCtx, you'd only get Path, and have to get the AST node from it:

impl<'a> Traverse<'a> for MyTransform {
  fn enter_binary_expression(&mut self, path: &mut Path) {
    let bin_expr: &mut BinaryExpression<'a> = path.node();
    // Do stuff with `bin_expr`
  }
}

Is that any better?

Personally I don't much like it - .node() is boilerplate for every single enter_* or exit_* method.

BUT if we want to support mutating upwards, we'd need an API something like that anyway. That'd work something like this:

impl<'a> Traverse<'a> for MyTransform {
  fn enter_binary_expression(&mut self, path: &mut Path) {
    match path.parent_mut() {
      AncestorMut::ExpressionStatement(expr_stmt) => {
        // `expr_stmt` is a `&mut ExpressionStatement` - you can mutate it
      }
    }
  }
}

With upwards mutation, the function params can't include a reference to the current node, as it would be UB to also get a &mut ref to parent at same time. Both current AST node and parent AST node have to both come from path, so path can enforce that you can only get one or the other at any time.

NB: I am not 100% sure upward mutation can be made safe, but I think it can with an API like the above.

@overlookmotel overlookmotel added the C-enhancement Category - New feature or request label May 13, 2024
@Dunqing
Copy link
Member

Dunqing commented May 14, 2024

I like this API.

Instead of receiving AST node and TraverseCtx, you'd only get Path, and have to get the AST node from it:

impl<'a> Traverse<'a> for MyTransform {
  fn enter_binary_expression(&mut self, path: &mut Path) {
    let bin_expr: &mut BinaryExpression<'a> = path.node();
    // Do stuff with `bin_expr`
  }
}

But your example looks incorrect. What do I get when I call path.node()?

@overlookmotel
Copy link
Collaborator Author

overlookmotel commented May 14, 2024

Path is a generic Path<'a, T>. I am hoping that Rust would allow eliding T, but maybe I'm wrong about that - maybe you can only elide lifetimes, not generic types.

With the full function signature, it would be:

impl<'a> Traverse<'a> for MyTransform {
  fn enter_binary_expression(&mut self, path: &mut Path<'a, BinaryExpression<'a>>) {
    let bin_expr = path.node();
    // Do stuff with `bin_expr`
  }
}

bin_expr is a &mut BinaryExpression<'a> - same as what gets passed as node param in current API.

@overlookmotel
Copy link
Collaborator Author

overlookmotel commented May 14, 2024

I can try and build it so we can evaluate it more clearly - should not take long to do that.

@Boshen Boshen closed this as completed Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category - New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants