Skip to content

ES6 transpilation of async + super call moves properties from unquoted->quoted #21088

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
mprobst opened this issue Jan 9, 2018 · 10 comments · Fixed by #26707
Closed

ES6 transpilation of async + super call moves properties from unquoted->quoted #21088

mprobst opened this issue Jan 9, 2018 · 10 comments · Fixed by #26707
Labels
Bug A bug in TypeScript Domain: Transforms Relates to the public transform API Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented.
Milestone

Comments

@mprobst
Copy link
Contributor

mprobst commented Jan 9, 2018

TypeScript Version: Version 2.7.0-dev.20180109

Code

class A {
  test() {}
}

class B extends A {
  async test() {
    return super.test();
  }
}

Expected behavior:

TypeScript should not emit code that accesses properties by string name, as this breaks optimizers such as Closure Compiler or Uglify with property renaming enabled.

Actual behavior:

class A {
    test() { }
}
class B extends A {
    test() {
        const _super = name => super[name];
        return __awaiter(this, void 0, void 0, function* () {
            return _super("test").call(this);
        });
    }
}

_super("test") boils down to super['test'], which breaks when test was renamed by an optimizing compiler.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 9, 2018

So what is the proposal here? this was the best solution we came up with for the super access that does not completely massacre the use code.

@mhegazy mhegazy added the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Jan 9, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Jan 9, 2018

//cc @rbuckton

@mhegazy mhegazy added Bug A bug in TypeScript Domain: Transforms Relates to the public transform API labels Jan 9, 2018
@scriby
Copy link

scriby commented Jan 9, 2018

const _super = super;
return __awaiter(this, void 0, void 0, function* () {
  return _super.test.call(this);
});

I am sure I am missing something, but why doesn't that one work?

@rbuckton
Copy link
Contributor

rbuckton commented Jan 9, 2018

@scriby This isn't valid javascript, unfortunately. super cannot be captured like an identifier. The only valid uses of super are a super-constructor call (i.e. super()), super-property access (i.e. super.test), or super-element access (i.e. super["test"]). We chose to use super-element access here as it allows us to parameterize super properties using super-element access in an arrow function closure.

The only way to address your concern would be to inject a wrapper for every super-property access in an async function:

// TypeScript
class B extends A {
    async test() {
      // get
      const w = super.w;

      // set
      super.x = 1;

      // destructuring
      ({ y: super.y } = { y: 2 });

      // call
      super.z();
    }
}

// JavaScript
class B extends A {
    test() {
        const _super = Object.create(null, {
          w: { get: () => super.w },
          x: { set: (value) => super.x = value },
          y: { set: (value) => super.y = value },
          z: { get: () => super.z }
        });
        return __awaiter(this, void 0, void 0, function* () {
            // get
            const w = _super.w;

            // set
            _super.x = 1;

            // destructuring
            ({ y: _super.y } = { y: 2 });

            // call
            _super.z.call(this);
        });
    }
}

@scriby
Copy link

scriby commented Jan 9, 2018

So this one would work?

const _test = super.test;
return __awaiter(this, void 0, void 0, function* () {
  return _test.call(this);
});

@rbuckton
Copy link
Contributor

rbuckton commented Jan 16, 2018

@scriby, not if super.test is a getter with side-effects, as the side-effects happen in the wrong place. Consider:

class Base {
  testCounter = 0;
  get test() {
    this.testCounter++;
    return () => this.testCounter;
  }
}

class Sub extends Base {
  async fn() {
    console.log(super.test());
    console.log(super.test());
  }
}

You would expect it to print:

1
2

But with the above proposal it would print:

1
1

@scriby
Copy link

scriby commented Jan 16, 2018

Nice test case. I'll just keep guessing at it until something sticks :)

const _test = () => super.test;
return __awaiter(this, void 0, void 0, function* () {
  return _test().call(this);
});

@rbuckton
Copy link
Contributor

This is effectively the same as my proposal above, except that my proposal logically groups all of these helpers together and allows for the use of super.x as a binding target in a destructuring assignment:

const _super = Object.create(null, {
  w: { get: () => super.w },
  x: { set: (value) => super.x = value },
  y: { set: (value) => super.y = value },
  z: { get: () => super.z }
});

@scriby
Copy link

scriby commented Jan 17, 2018

Ah sorry, I should have read that more carefully. Looks great!

@mhegazy mhegazy added this to the Future milestone Apr 12, 2018
@calebegg
Copy link

No idea how palatable this is, but another option would be:

class A {
    test(arg) { }
}
class B extends A {
    test() {
        const _super = name => super[name];
        return __awaiter(this, void 0, void 0, function* () {
            return _super({test: [this, someArg]});
        });
    }
}

where _super then uses const [key, args] = Object.entries(obj)[0]; super[key].call(...args) instead of a string directly. Closure and Uglify would then know to rename 'test' in both places, and they'd get renamed to the same string, by design.

mprobst added a commit to mprobst/TypeScript that referenced this issue Aug 28, 2018
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 microsoft#21088.
mprobst added a commit to mprobst/TypeScript that referenced this issue Aug 29, 2018
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.
mprobst added a commit to mprobst/TypeScript that referenced this issue Aug 29, 2018
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.
mprobst added a commit to mprobst/TypeScript that referenced this issue Aug 31, 2018
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.
mprobst added a commit to mprobst/TypeScript that referenced this issue Oct 3, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Transforms Relates to the public transform API Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants