feat(transformer/async-to-generator): handle arrow-function correctly

[Dunqing]

No description provided.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[Dunqing]

• refactor(transformer): shorten imports #6643
• docs(transformer/async-to-generator): remove empty lines from doc comment #6642
• feat(transformer/async-to-generator): handle arrow-function correctly #6640 👈
• refactor(transformer/async-to-generator): use helper_call_expr #6634
• feat(transformer): pass TransformerCtx to async-to-generator plugin #6633
• refactor(transformer/async-to-generator): move internal methods below entry points #6632
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Dunqing and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #6640 will not alter performance

_{Comparing 10-17-feat_transformer_handle_arrow-function_correctly (a3dea9c) with main (390abca)}

Summary

✅ 30 untouched benchmarks

[overlookmotel]

I have some suggestions to shorten and simplify this transform. But let's merge first, and I'll write those suggestions up shortly.

[overlookmotel]

Merge activity

• Oct 17, 4:41 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Oct 17, 4:41 AM EDT: Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.
• Oct 17, 4:42 AM EDT: Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.
• Oct 17, 4:46 AM EDT: A user added this pull request to the Graphite merge queue.
• Oct 17, 4:53 AM EDT: A user merged this pull request with the Graphite merge queue.

[overlookmotel]

My suggestions for follow-ups (which you are free to ignore!):

Small things

• exit_expression: Style: Convert the 3-branch if / else if into a match.
• exit_statement: Style: Reduce indentation by starting function with let Statement::FunctionDeclaration(func) = stmt else { return };
• exit_statement: Function::id field is always Some for function declarations. Replace if let Some(id) = func.id.take() { with let id = func.id.take().unwrap(), and remove the else branch.
• exit_expression: No need to check body.r#async(). await cannot be used in a non-async function, so *body.r#async() is always true here.

Refactor

Instead of ctx.ancestry.ancestors().find_map() and then if in_async_function ..., use a loop and return to bail out if nothing to be done. e.g.:

// Exit if this `await` is top level, or in an async generator function
for ancestor in ctx.ancestors() {
    match ancestor {
        Ancestor::FunctionBody(body) => {
            if *body.generator() {
                return;
            }
            break;
        }
        // Arrow functions are never generators
        Ancestor::ArrowFunctionExpressionBody(_) => break,
        Ancestor::ProgramBody(_) => return,
        _ => {}
    }
}

// Convert the expression to yield
*expr = ctx.ast.expression_yield(
    SPAN,
    false,
    Some(ctx.ast.move_expression(&mut await_expr.argument)),
);

Personally I think this is a bit clearer, though it's a matter of taste. If you prefer how it is, feel free to ignore me.

Avoid constructing a new Function

transform_function doesn't need to construct a new Function, it can mutate the existing one.

• Take ownership of the Expresson / Statement in exit_expression and exit_statement via ctx.ast.move_expression / move_statement.
• Get the owned Box<Function> via this pattern.
• Pass Box<'a, Function<'a>> into transform_function.

Getting ownership by matching twice on expr being an Expression::Function is an ugly pattern, but the upside is (a) less operations and allocations and (b) simplify transform_function because only one move_* operation, instead of loads of little ones.

Note: I'm suggesting passing Box<Function> (not Function) into transform_function, because:

1. Box is only 8 bytes. You can avoid having a Function (104 bytes) on the stack.
2. Then you don't have to allocate the Function back into arena. Box<Function> already has it in arena, and you can mutate it in place.

[Dunqing]

@overlookmotel Good suggestion, I will keep it in mind!