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

traverse: port scope.push API from Babel #5049

Closed
Dunqing opened this issue Aug 21, 2024 · 6 comments
Closed

traverse: port scope.push API from Babel #5049

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

Comments

@Dunqing
Copy link
Member

Dunqing commented Aug 21, 2024

Babel's implementation is here

Why need this API?

We need to combine the variable declarators required by different plugins to be inserted at the top of the statements.

Related tests:

  • logical-assignment/null-coalescing/input.js
-var _obj$x, _obj$x2, _deep$obj, _deep$obj$x, _deep$obj2, _deep$obj2$x, _key, _obj$_key, _key2, _obj$_key2, _deep$obj3, _key3, _deep$obj3$_key, _deep$obj4, _key4, _deep$obj4$_key;
+var _obj$x, _obj$x2, _deep$obj$x, _deep$obj2$x, _obj$_key, _obj$_key2, _deep$obj3$_key, _deep$obj4$_key;
+var _deep$obj, _deep$obj2, _key, _key2, _deep$obj3, _key3, _deep$obj4, _key4;
@Dunqing Dunqing added C-bug Category - Bug C-enhancement Category - New feature or request A-transformer Area - Transformer / Transpiler and removed C-bug Category - Bug labels Aug 21, 2024
@Dunqing
Copy link
Member Author

Dunqing commented Aug 21, 2024

When we have this API, we can remove the following code. This implementation appears in many plugins

fn enter_statements(&mut self, _stmts: &mut Vec<'a, Statement<'a>>, ctx: &mut TraverseCtx<'a>) {
self.var_declarations.push(ctx.ast.vec());
}
fn exit_statements(
&mut self,
statements: &mut Vec<'a, Statement<'a>>,
ctx: &mut TraverseCtx<'a>,
) {
if let Some(declarations) = self.var_declarations.pop() {
if declarations.is_empty() {
return;
}
let variable = ctx.ast.alloc_variable_declaration(
SPAN,
VariableDeclarationKind::Var,
declarations,
false,
);
statements.insert(0, Statement::VariableDeclaration(variable));
}
}

@overlookmotel
Copy link
Contributor

overlookmotel commented Aug 22, 2024

Yes, agreed we need this. Aside from the failing test, the repeated exit_statements pattern is inefficient.

I'm not sure which is best way to implement it though. Ideally it wouldn't be a method on TraverseCtx because it's not really related to traversal. But it's shared state between different transforms, which we don't have a facility for at present. Hmm...

By the way, Babel's implementation is inefficient:

// Input
{ obj.a &&= 1; }
{ obj.b &&= 2; }
// Output
{
  var _obj;
  (_obj = obj).a && (_obj.a = 1);
}
{
  var _obj2;
  (_obj2 = obj).b && (_obj2.b = 2);
}

Babel REPL

But as they are var statements, they could be hoisted up and combined:

var _obj, _obj2;
{ (_obj = obj).a && (_obj.a = 1); }
{ (_obj2 = obj).b && (_obj2.b = 2); }

This is more compact output (though minifier would probably do that hoisting anyway) but also requires maintaining an entry in var_declarations for every block, when it could be only top level / function blocks.

As a futher optimization, where the temp var is used only briefly, a single binding could be reused multiple times:

var _temp;
{ (_temp = obj).a && (_temp.a = 1); }
{ (_temp = obj).b && (_temp.b = 2); }

(again, maybe the minifier will be able to optimize the code down to that anyway)

@overlookmotel
Copy link
Contributor

We can use SparseStack (introduced in #5940).

The problem with putting this in TransformCtx is that then it needs to be wrapped in a RefCell which will be somewhat costly, as it's accessed often.

Maybe should parameterize TraverseCtx so it can hold arbitrary state:

pub trait Traverse<'a, State> {
    fn enter_statements(&mut self, stmts: &mut Vec<'a, Statement<'a>>, ctx: &mut TraverseCtx<'a, State>) {}
    fn exit_statements(&mut self, stmts: &mut Vec<'a, Statement<'a>>, ctx: &mut TraverseCtx<'a, State>) {}
    // etc...
}

pub struct TraverseCtx<'a, State> {
    pub ancestry: TraverseAncestry<'a>,
    pub scoping: TraverseScoping,
    pub ast: AstBuilder<'a>,
    pub state: State,
}

struct TransformerState<'a> {
    var_declarations: SparseStack<Vec<'a, VariableDeclarator<'a>>>,
}

type TransCtx<'a> = TraverseCtx<'a, TransformerState<'a>>;

struct CommonTransform;

impl<'a> Traverse<'a, TransformerState<'a>> for CommonTransform {
    fn enter_statements(&mut self, _stmts: &mut Vec<'a, Statement<'a>>, ctx: &mut TransCtx<'a>) {
        ctx.state.var_declarations.push(None);
    }

    fn exit_statements(&mut self, stmts: &mut Vec<'a, Statement<'a>>, ctx: &mut TransCtx<'a>) {
        if let Some(declarations) = ctx.state.var_declarations.pop() {
            // Insert `var` declaration statement
        }
    }
}

To do:

  1. Put it in TransformCtx first, in a RefCell.
  2. Move to TraverseCtx and see if it makes a difference to perf.

Dunqing pushed a commit that referenced this issue Sep 30, 2024
First step towards #5049.

Various transforms need to add a `var` statement at top of enclosing statement block.

e.g.:

```js
// Input
a ??= b;
```

```js
// Output
var _a;
(_a = a) !== null && _a !== void 0 ? _a : (a = b);
```

Each of these transforms previously maintained it's own stack and added `var` statements individually.

Share this functionality in a "common" utility transform which maintains a single stack to serve them all.
@overlookmotel
Copy link
Contributor

overlookmotel commented Oct 5, 2024

#6170 fixed the failing test mentioned at top of this issue.

Only thing remaining is for arrow functions transform to also use VarDeclarations for inserting var _this = this; statements.

oxc-project/backlog#144 covers moving TransformCtx into TraverseCtx to avoid the RefCell.

@overlookmotel
Copy link
Contributor

@Dunqing I think arrow functions transform now uses VarDeclarations for adding statements, right? If so, I think we can close this issue.

@Dunqing
Copy link
Member Author

Dunqing commented Jan 13, 2025

Yes, It is done!

@Dunqing Dunqing closed this as completed Jan 13, 2025
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

3 participants