Per-property super accessors in async functions. #26707

mprobst · 2018-08-28T10:47:20Z

TypeScript must hoist accessors for super properties when converting
async method bodies to the __awaiter pattern for targets before
ES2016.

Previously, TypeScript would reify all property accesses into element
accesses, i.e. convert the property name into a string parameter and
pass it to super[...]. That breaks optimizers like Closure Compiler or
Uglify in advanced mode, when property renaming is enabled, as it mixes
quoted and un-quoted property access (super['x'] vs just x at the
declaration site).

This change creates a specific accessor for each property access on
super, which fixes the quoted/unquoted confusion. It keeps the generic
accessor for element access statements.

Fixes #21088.

mprobst · 2018-08-28T12:07:40Z

Might be worth noting: this change disables the early exit in the esnext & ES2017 transformers, here:
dcf2f7e#diff-b0af6bd9d1d8b206a6b9ac13af6deab3L56

It'd be possible to special case further and use a specific visitor when traversing an async method's body. I'm not sure it's worth the complication - any advice?

The other thing I'm not 100% clear on is the scope for the substitutedSuperAccessors array. It's currently per transformer invocation. It might be possible to remove it by moving to a property on the node, but this seems to match the general design better.

AlCalzone · 2018-08-28T12:39:41Z

tests/baselines/reference/asyncMethodWithSuperConflict_es6.js

        return __awaiter(this, void 0, void 0, function* () {
            const _super = null;
            const f = () => { };
            // call with property access
-            _super_1("x").value.call(this);
+            _super_x().value.call(this);


Maybe I'm missing something, but looking at the way _super_x is defined in line 91 (an object), shouldn't this be

_super_x.value.call(this); // ^ no parentheses

instead?

Good catch, thanks!

I've fixed this by moving to the implementation that @rbuckton suggested in #21088, i.e. creating a single object _superProps that contains configured properties for each named property.

src/compiler/transformers/es2017.ts

src/compiler/checker.ts

src/compiler/transformers/es2017.ts

mprobst · 2018-09-10T09:24:02Z

@rbuckton please take another look.

mprobst · 2018-10-02T12:34:37Z

Friendly ping.

src/compiler/checker.ts

src/compiler/transformers/es2017.ts

sandersn

Looks good except for a couple of typos in comments.

rbuckton

Minor change needed for tests.

tests/baselines/reference/asyncMethodWithSuperConflict_es6.js

TypeScript must hoist accessors for super properties when converting async method bodies to the `__awaiter` pattern for targets before ES2016. Previously, TypeScript would reify all property accesses into element accesses, i.e. convert the property name into a string parameter and pass it to `super[...]`. That breaks optimizers like Closure Compiler or Uglify in advanced mode, when property renaming is enabled, as it mixes quoted and un-quoted property access (`super['x']` vs just `x` at the declaration site). This change creates a variable `_superProps` that contains accessors for each property accessed on super within the async method. This allows accessing the properties by name (instead of quoted string), which fixes the quoted/unquoted confusion. The change keeps the generic accessor for element access statements to match quoting behaviour. Fixes microsoft#21088.

* rename _super to _superIndex and _superProps to _super. * reinstate early exit for transformers by marking super accesses as esnext/es2017 in `binder.ts`. * adjust comment in `checker.ts` to new emit.

mprobst · 2018-10-03T14:00:10Z

Done, please take another look.

sandersn

I think you added a few git submodules under src/ by mistake.

Change-Id: I30af09343446126ba73ed40199ecc3f0ed515b3e

mprobst · 2018-10-04T06:09:25Z

Fixed. Must have been VS Code doing something in my working directory? Odd.

sandersn · 2018-10-05T22:42:52Z

@rbuckton Does it look good to you? If so, you can merge, or I will after the weekend.

rbuckton · 2018-10-06T00:20:17Z

Looks good!

mprobst · 2018-10-06T14:40:23Z

Thanks for the review!

AlCalzone reviewed Aug 28, 2018

View reviewed changes

mprobst force-pushed the async-super-rename-safe branch 2 times, most recently from 4034cf9 to bce7cfd Compare August 29, 2018 12:03

RyanCavanaugh requested a review from rbuckton August 29, 2018 20:55

rbuckton suggested changes Aug 29, 2018

View reviewed changes

mprobst force-pushed the async-super-rename-safe branch from bce7cfd to f784d8a Compare August 31, 2018 09:03

RyanCavanaugh added this to the Community milestone Sep 17, 2018

sandersn self-requested a review October 2, 2018 16:59

sandersn reviewed Oct 2, 2018

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

sandersn reviewed Oct 2, 2018

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

sandersn reviewed Oct 2, 2018

View reviewed changes

src/compiler/transformers/es2017.ts Outdated Show resolved Hide resolved

sandersn requested changes Oct 2, 2018

View reviewed changes

rbuckton suggested changes Oct 2, 2018

View reviewed changes

tests/baselines/reference/asyncMethodWithSuperConflict_es6.js Outdated Show resolved Hide resolved

mprobst added 3 commits October 3, 2018 15:46

Fixes for review comments.

e618d75

* rename _super to _superIndex and _superProps to _super. * reinstate early exit for transformers by marking super accesses as esnext/es2017 in `binder.ts`. * adjust comment in `checker.ts` to new emit.

fix comments

e58ffcf

mprobst force-pushed the async-super-rename-safe branch 2 times, most recently from c7e8a99 to 57d922a Compare October 3, 2018 13:51

sandersn requested changes Oct 3, 2018

View reviewed changes

Rename to _superIndex to test conflict

539c455

Change-Id: I30af09343446126ba73ed40199ecc3f0ed515b3e

mprobst force-pushed the async-super-rename-safe branch from 57d922a to 539c455 Compare October 4, 2018 06:08

sandersn approved these changes Oct 5, 2018

View reviewed changes

rbuckton approved these changes Oct 6, 2018

View reviewed changes

rbuckton merged commit 85a3475 into microsoft:master Oct 6, 2018

gabrielbiga mentioned this pull request Feb 23, 2019

Wrong async scope resolution #30066

Closed

mprobst deleted the async-super-rename-safe branch March 9, 2020 08:29

Per-property super accessors in async functions. #26707

Per-property super accessors in async functions. #26707

Uh oh!

Conversation

mprobst commented Aug 28, 2018

Uh oh!

mprobst commented Aug 28, 2018

Uh oh!

AlCalzone Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprobst Aug 29, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mprobst commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mprobst commented Oct 2, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

rbuckton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mprobst commented Oct 3, 2018

Uh oh!

sandersn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprobst commented Oct 4, 2018

Uh oh!

sandersn commented Oct 5, 2018

Uh oh!

rbuckton commented Oct 6, 2018

Uh oh!

mprobst commented Oct 6, 2018 via email

Uh oh!

Uh oh!

AlCalzone Aug 28, 2018 •

edited

Loading

mprobst commented Sep 10, 2018 •

edited

Loading

sandersn left a comment •

edited

Loading