fix(transformer/arrow-functions): _this = this should be inserted after super call expression

[Dunqing]

related: #7792

This PR doesn't contain fixing the async arrow function in super(), which is difficult for our architecture. I just found that esbuild's implementation is quite simpler! https://esbuild.github.io/try/#dAAwLjI0LjAALS10YXJnZXQ9ZXMyMDE2AGNsYXNzIEMgZXh0ZW5kcyBTIHsKICBjb25zdHJ1Y3RvcigpIHsKICAgIHN1cGVyKGFzeW5jICgpID0+IHRoaXMpOwogICAgYXN5bmMoKSA9PiB7fQogIH0KfQ

[Dunqing]

• fix(transformer/arrow-functions): _this = this should be inserted after super call expression #8024 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

[codspeed-hq]

CodSpeed Performance Report

Merging #8024 will not alter performance

_{Comparing 12-20-fix_transformer_arrow-functions__this_this_should_be_inserted_after_super_call_expression (c0bf5b9) with main (7aebed0)}

Summary

✅ 29 untouched benchmarks

[Dunqing]

Unwrap failed here, we need to find all possible statements, and this may not be a good implementation

[overlookmotel]

You can use Ancestor::is_via_statement for this.

Note it doesn't tell you if that ancestor is a statement, but whether its child is a statement.

So if you are visiting a Function, it could be either an expression or a statement. ctx.parent().is_via_statement() will tell you.

To find the closest statement, loop through ancestors, and when ancestor.is_via_statement() returns true, then the previous ancestor you passed is the statement.

Put another way, you have to walk 1 step further up the tree than the node you're looking for.

The naming is_via_* is weird, and the fact that you have to look at the ancestor above is confusing as hell. So it's a terrible API! But it does do what you need.

[overlookmotel]

#8031 renames this method to Ancestor::is_parent_of_statement. I think that's clearer.

[Dunqing]

I just found that we have these methods!

[Dunqing]

@overlookmotel Can you take a look at this PR, I am not sure this is a good direction to fix it. I want to change everything according to esbuild's implementation

[overlookmotel]

I'm not sure this approach based on Address works.

super() can be after the async function:

class C extends S {
  constructor() {
    let f = async () => this;
    super();
  }
}

Babel REPL

It can also be used an expression, rather than a statement:

class C extends S {
  constructor() {
    let f = async () => this;
    let x = foo(super(), f());
  }
}

Babel REPL

I think the most simple approach involves a double-visit:

• When exiting a function, check if that function is the constructor of a class with a super-class.
• If so, and a _this var is required, visit the constructor body again and either:
  1. If super() first appears as a top level StatementExpression, insert _this = this; statement after super().
  2. Otherwise, replace all super() calls with (super(), _this = this).

This is what the visitor in class properties that I mentioned in #7792 (comment) does, so you could just copy the code.

[overlookmotel]

Concerning ESBuild's version:

Yes it's simpler to implement, but I think less performant at runtime than Babel's version. In Babel's version, there's only 1 call to _asyncToGenerator when the async function is defined. But in ESBuild's version, __async gets called and constructs a new generator function every time you call the async function.

I don't know for sure, but that sounds more expensive (more garbage collection).

I think we could do our own version which is better than both! Maybe something like this:

Input:

class C extends S {
  constructor() {
    super();
    let f = async () => this;
  }
}

Output:

class C extends S {
  constructor() {
    var _getThis = () => this;
    super();
    let f = oxcHelpers.asyncToGenerator(function*() { return this; }, _getThis, "f");
  }
}

(the last param takes care of naming the function correctly, instead of Babel's extra function wrappers)

But... I suggest we follow Babel for now, make our implementation correct, and look at optimizing output later. There's many places we can improve on Babel in lots of transforms.

[overlookmotel]

PS: I've made a small improvement to the super()-replacer visitor in #8028.

[Dunqing]

That's a trade-off for us considering the different implementations. When I am a developer for the transformer, I prefer a
way easy to implement save my hair!

[Dunqing]

To clarify, so in this case, you prefer to double-visited? I have no strong opinion about this. Just think of this way is easier to implement, but cannot solve super(async () => {}).

[overlookmotel]

I think double-visit is the best way, yes.

I think it'll be able to handle the super(async () => this) case too.

We could later try and optimize it for common case where super() is a top level statement, by storing the Address of super() during traversal, and then you know exactly where to insert _this = this without another visit.

But:

1. I'm not actually sure if that'll be a perf gain or not, because you have to do a lot more checking for "is this node super()?", when most of the time the answer is "no". And when the class constructor doesn't contain an async arrow function (most won't), then that's wasted work.
2. That won't handle all the edge cases anyway.
3. Make it correct first, then optimize.