Skip to content

Update async await transpilation to avoid captures of super #3103

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

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions src/com/google/javascript/jscomp/RewriteAsyncFunctions.java
Original file line number Diff line number Diff line change
Expand Up @@ -243,10 +243,35 @@ private void convertAsyncFunction(NodeTraversal t, LexicalContext functionContex
newBody.addChildToBack(IR.constNode(IR.name(ASYNC_ARGUMENTS), IR.name("arguments")));
NodeUtil.addFeatureToScript(t.getCurrentFile(), Feature.CONST_DECLARATIONS);
}
boolean needsSuperTranspilation = compiler.getOptions().needsTranspilationFrom(FeatureSet.ES6);
for (String replacedMethodName : functionContext.replacedSuperProperties) {
// MS Edge 17 cannot properly capture references to "super" in an arrow function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any link we could insert here pointing to the MS Edge bug?
We'd really like to be able to track when the MS Edge bug gets fixed, or if it's already fixed and we're just stuck because of installed browsers not getting the fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created one and added the link

// If we are not transpiling classes, switch to using Object.getPrototypeOf(this.constructor)
// as a replacement for super.
// If we are transpiling classes, the super reference will be handled elsewhere.
Node superReference;
if (needsSuperTranspilation) {
superReference = IR.superNode();
} else if (originalFunction.getParent().isStaticMember()) {
// static super: Object.getPrototypeOf(this);
superReference =
IR.call(IR.getprop(IR.name("Object"), IR.string("getPrototypeOf")), IR.thisNode());
} else {
// instance super: Object.getPrototypeOf(this.constructor).prototype
superReference =
IR.getprop(
IR.call(
IR.getprop(IR.name("Object"), IR.string("getPrototypeOf")),
IR.getprop(IR.thisNode(), IR.string("constructor"))),
IR.string("prototype"));
}

// const super$get$x = () => super.x;
Node arrowFunction = IR.arrowFunction(
IR.name(""), IR.paramList(), IR.getprop(IR.superNode(), IR.string(replacedMethodName)));
Node arrowFunction =
IR.arrowFunction(
IR.name(""),
IR.paramList(),
IR.getprop(superReference, IR.string(replacedMethodName)));
compiler.reportChangeToChangeScope(arrowFunction);
NodeUtil.addFeatureToScript(t.getCurrentFile(), Feature.ARROW_FUNCTIONS);

Expand Down
23 changes: 22 additions & 1 deletion src/com/google/javascript/jscomp/RewriteAsyncIteration.java
Original file line number Diff line number Diff line change
Expand Up @@ -557,17 +557,38 @@ private void prependTempVarDeclarations(LexicalContext ctx, NodeTraversal t) {
IR.constNode(IR.name(argumentsVarName), IR.name("arguments"))
.useSourceInfoFromForTree(block));
}
boolean needsSuperTranspilation = compiler.getOptions().needsTranspilationFrom(FeatureSet.ES6);
for (String replacedMethodName : thisSuperArgsCtx.usedSuperProperties) {
// { // prefixBlock
// const $jscomp$asyncIter$this = this;
// const $jscomp$asyncIter$arguments = arguments;
// const $jscomp$asyncIter$super$get$x = () => super.x;
// }
//
// MS Edge 17 cannot properly capture references to "super" in an arrow function.
// If we are not transpiling classes, switch to using Object.getPrototypeOf(this.constructor)
// as a replacement for super.
// If we are transpiling classes, the super reference will be handled elsewhere.
Node superReference;
if (needsSuperTranspilation) {
superReference = IR.superNode();
} else if (ctx.function.getParent().isStaticMember()) {
// static super: Object.getPrototypeOf(this.constructor);
superReference =
IR.call(
IR.getprop(IR.name("Object"), IR.string("getPrototypeOf")),
IR.getprop(IR.thisNode(), IR.string("constructor")));
} else {
// instance super: Object.getPrototypeOf(this)
superReference =
IR.call(IR.getprop(IR.name("Object"), IR.string("getPrototypeOf")), IR.thisNode());
}

Node arrowFunction =
IR.arrowFunction(
IR.name(""),
IR.paramList(),
IR.getprop(IR.superNode(), IR.string(replacedMethodName)));
IR.getprop(superReference, IR.string(replacedMethodName)));
compiler.reportChangeToChangeScope(arrowFunction);
NodeUtil.addFeatureToScript(t.getCurrentFile(), Feature.ARROW_FUNCTIONS);
String superReplacementName = superPropGetterPrefix + replacedMethodName;
Expand Down
6 changes: 4 additions & 2 deletions test/com/google/javascript/jscomp/IntegrationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -3988,7 +3988,8 @@ public void testAsyncFunctionSuper() {
"}",
"class Baz extends Foo {",
" bar() {",
" const $jscomp$async$this = this, $jscomp$async$super$get$bar = () => super.bar;",
" const $jscomp$async$this = this, $jscomp$async$super$get$bar =",
" () => Object.getPrototypeOf(this.constructor).prototype.bar;",
" return $jscomp.asyncExecutePromiseGeneratorFunction(function*() {",
" yield Promise.resolve();",
" $jscomp$async$super$get$bar().call($jscomp$async$this);",
Expand Down Expand Up @@ -4041,7 +4042,8 @@ public void testAsyncIterationSuper() {
"class Baz extends Foo {",
" bar() {",
" const $jscomp$asyncIter$this = this,",
" $jscomp$asyncIter$super$get$bar = () => super.bar;",
" $jscomp$asyncIter$super$get$bar =",
" () => Object.getPrototypeOf(this.constructor).prototype.bar;",
" return new $jscomp.AsyncGeneratorWrapper(function*() {",
" $jscomp$asyncIter$super$get$bar().call($jscomp$asyncIter$this).next();",
" }());",
Expand Down
67 changes: 67 additions & 0 deletions test/com/google/javascript/jscomp/RewriteAsyncFunctionsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,73 @@ public void testInnerSuperReference() {
"}"));
}

@Test
public void testInnerSuperCallEs2015Out() {
setLanguageOut(LanguageMode.ECMASCRIPT_2015);
test(
lines(
"class A {",
" m() {",
" return this;",
" }",
"}",
"class X extends A {",
" async m() {",
" return super.m();",
" }",
"}"),
lines(
"class A {",
" m() {",
" return this;",
" }",
"}",
"class X extends A {",
" m() {",
" const $jscomp$async$this = this;",
" const $jscomp$async$super$get$m =",
" () => Object.getPrototypeOf(this.constructor).prototype.m;",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we just get the prototype directly from this?

() => Object.getPrototypeOf(this).m;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not. See the comments in the following snippet:

class Base {
  static foo() { console.log('base foo'); }
  bar() { console.log('base bar'); }
}

class Child extends Base {
  static foo() {
    console.log(Object.getPrototypeOf(this), Object.getPrototypeOf(this).foo);
    Object.getPrototypeOf(this).foo.call(this);
  }

  bar() {
    console.log(Object.getPrototypeOf(this), Object.getPrototypeOf(this).bar);
    Object.getPrototypeOf(this).bar(); // logs out "base bar" appropriately
    Object.getPrototypeOf(this).bar.call(this); // infinitely loops and locks up the Chrome tab!
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see my mistake.
I meant to say Object.getPrototypeOf(Object.getPrototypeOf(this))

I wonder which is the safer choice in the face of code that mucks around with prototypes?
Is it more likely that this.constructor isn't pointing at the right thing or that an additional object has been inserted into the prototype chain?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed this with @concavelenz
We agree that it seems more correct and slightly safer to call Object.getPrototypeOf() twice to get up to the superclass prototype rather than relying on this.constructor.

Would you be willing to make that change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course - done.

" return $jscomp.asyncExecutePromiseGeneratorFunction(",
" function* () {",
" return $jscomp$async$super$get$m().call($jscomp$async$this);",
" });",
" }",
"}"));
}

@Test
public void testInnerSuperCallStaticEs2015Out() {
setLanguageOut(LanguageMode.ECMASCRIPT_2015);
test(
lines(
"class A {",
" static m() {",
" return this;",
" }",
"}",
"class X extends A {",
" static async m() {",
" return super.m();",
" }",
"}"),
lines(
"class A {",
" static m() {",
" return this;",
" }",
"}",
"class X extends A {",
" static m() {",
" const $jscomp$async$this = this;",
" const $jscomp$async$super$get$m = () => Object.getPrototypeOf(this).m;",
" return $jscomp.asyncExecutePromiseGeneratorFunction(",
" function* () {",
" return $jscomp$async$super$get$m().call($jscomp$async$this);",
" });",
" }",
"}"));
}

@Test
public void testNestedArrowFunctionUsingThis() {
test(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,8 @@ public void testInnerSuperReferenceInAsyncGenerator() {
"}",
"class X extends A {",
" m() {",
" const $jscomp$asyncIter$super$get$m = () => super.m;",
" const $jscomp$asyncIter$super$get$m =",
" () => Object.getPrototypeOf(this.constructor).prototype.m;",
" return new $jscomp.AsyncGeneratorWrapper(",
" function* () {",
" const tmp = $jscomp$asyncIter$super$get$m();",
Expand Down