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

transformer: re-walk transformed nodes in exit_* methods #5381

Open
Dunqing opened this issue Sep 1, 2024 · 2 comments
Open

transformer: re-walk transformed nodes in exit_* methods #5381

Dunqing opened this issue Sep 1, 2024 · 2 comments
Assignees
Labels
A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request

Comments

@Dunqing
Copy link
Member

Dunqing commented Sep 1, 2024

Why do we need this?

Sometimes we may need the ability to re-walk transformed nodes in exit_* methods.

For example, when we transform Expression::ArrowFunctionExpression to Expression::CallExpression in exit_expression. The new CallExpression may contain some nodes that must be transformed.

Rough draft

The implementation that I thought looks like this:

    fn exit_expression(&mut self, expr: &mut Expression<'a>, ctx: &mut TraverseCtx<'a>) {
        let new_expr = if let Some(new_expr) = self.x1_react.transform_expression_on_exit(expr, ctx)
        {
            self.walk_expression(new_expr, ctx)
        } else {
            self.x3_es2015.exit_expression(expr, ctx)
        };

        *expr = new_expr;
    }

Now we only missing walk_xxxxx API

I can imagine that this might be quite complex to implement. 🤔

Disadvantages

This can also cause a serious impact, we may need to cache the already transformed nodes within the plugin to avoid re-transform them during a re-walk.

@Dunqing Dunqing added C-enhancement Category - New feature or request A-transformer Area - Transformer / Transpiler labels Sep 1, 2024
@overlookmotel overlookmotel self-assigned this Sep 3, 2024
@overlookmotel
Copy link
Collaborator

Argh! This is exactly what I was hoping we could avoid!

I was hoping we could avoid it by standardizing the order transforms execute in, so clashes become impossible/much less likely. This is kind of what I was getting at when I was talking the other week about adding observe_* methods which run after exit_*, so when a transform is gathering information about child nodes, it can do that in observe_* and it always sees the final state after all other transforms have run on that node.

But, yes, I agree we do have a problem here and we'll need a solution (of some kind).

In practice, do we have any transforms which aren't working at present because of this problem?

@Dunqing
Copy link
Member Author

Dunqing commented Sep 4, 2024

I am following up on #5366 to remove the duplicate logic in

/// Convert arrow function expression to normal arrow function
///
/// ```js
/// () => 1
/// ```
/// to
/// ```js
/// () => { return 1 }
/// ```
fn transform_arrow_function_to_block(
arrow: &mut ArrowFunctionExpression<'a>,
ctx: &mut TraverseCtx<'a>,
) {
if !arrow.expression {
return;
}
arrow.expression = false;
let Some(Statement::ExpressionStatement(statement)) = arrow.body.statements.pop() else {
unreachable!("arrow function body is never empty")
};
arrow
.body
.statements
.push(ctx.ast.statement_return(SPAN, Some(statement.unbox().expression)));
}
.

However, there is a problem: we added fallback logic in exit_arrow_function_expression. The refresh plugin transforms ArrowFunctionExpression in exit_expression. The issue arises because exit_arrow_function_expression is visited first, followed by exit_expression, causing the fallback logic not to cover it. I attempted to move the fallback logic to exit_expression, but I discovered that the refresh plugin transforms Expression::ArrowFunctionExpression to Expression::CallExpression, which still doesn't cover it even in exit_expression.

I hope I have explained it clearly. Nevertheless, in this scenario, we can resolve it by putting the logic into a utility function. This way, we can simply call the function when encountering the aforementioned case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants