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

Update async await transpilation to avoid captures of super #3103

Closed

Conversation

ChadKillingsworth
Copy link
Collaborator

Closes #3101

MS Edge 17 does not properly honor captures of super in arrow functions. Avoid this pattern when transpiling async/await.

@brad4d
Copy link
Contributor

brad4d commented Oct 11, 2018

I don't think we can make this change.
I believe we initially tried just saving the value of super.foo, but ran into runtime failures because the value of super.foo changed between the point where we'd saved it and the point where we needed to evaluate it.

Is there some workaround for the MS edge bug we could generate here?
What does it do when you reference super() inside an arrow function?
Does an exception get thrown or is the value "wrong" in some way?

@ChadKillingsworth
Copy link
Collaborator Author

What does it do when you reference super() inside an arrow function? Does an exception get thrown or is the value "wrong" in some way?

In an arrow function in Edge, it looks like super === this. There is no exception - just an infinite loop of recursive calls. It's nasty.

Is there some workaround for the MS edge bug we could generate here?

I can keep looking for other options. One option is to replace the reference to super with the extends clause name. But that would also then require Es6RewriteClassExtendsExpressions for transpilation even during ES2015 out.

@ChadKillingsworth ChadKillingsworth force-pushed the msedge-capture-super branch 3 times, most recently from d2925ef to ef49982 Compare October 12, 2018 21:26
@ChadKillingsworth
Copy link
Collaborator Author

@brad4d I think I may have found a workable solution: Use Object.getPrototypeOf.

I only switch to this technique if classes are not being transpiled. This avoids a dependence on Object.getPrototypeOf for IE < 10.

@ChadKillingsworth
Copy link
Collaborator Author

I tested this on both Chrome and MS Edge in a project where I'm trying to land ES6 out.

…ing ES2015+ output.

MS Edge 17 does not properly capture references to `super` in an arrow function.
Closes google#3101
" 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.

@brad4d
Copy link
Contributor

brad4d commented Oct 15, 2018

@ChadKillingsworth
I like your solution. Very clean.

Is there some reason why we can't just do Object.getPrototypeOf(this) for the non-static case?
See also comment on code.

@ChadKillingsworth
Copy link
Collaborator Author

Classes and super references are tricky to transpile. I was able to simplify static references to super though.

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

@brad4d
Copy link
Contributor

brad4d commented Oct 16, 2018

Imported and sent for internal review and testing.

@blickly blickly closed this in 79891f0 Oct 19, 2018
@ChadKillingsworth ChadKillingsworth deleted the msedge-capture-super branch October 20, 2018 15:05
copybara-service bot pushed a commit that referenced this pull request Oct 27, 2021
…peOf`

This is in effect a rollback of the functional change in #3103

Preserving the `super.function()` syntax is more spec-compliant in some cases,
and MS Edge 17 is incredibly rare these days.

PiperOrigin-RevId: 406004773
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants