Skip to content

Avoid replacing last statement of derived constructors if 'this' is referenced.#12893

Merged
DanielRosenwasser merged 5 commits into
masterfrom
capturedThisFix
Dec 14, 2016
Merged

Avoid replacing last statement of derived constructors if 'this' is referenced.#12893
DanielRosenwasser merged 5 commits into
masterfrom
capturedThisFix

Conversation

@DanielRosenwasser
Copy link
Copy Markdown
Member

Fixes #12764.

Comment thread src/compiler/transformers/es2015.ts Outdated
// but only if the constructor itself doesn't use 'this' elsewhere.
if (superCallExpression
&& statementOffset === ctorStatements.length - 1
&& !(ctor.transformFlags & (TransformFlags.ContainsLexicalThis | TransformFlags.ContainsCapturedLexicalThis))) {
Copy link
Copy Markdown
Member Author

@DanielRosenwasser DanielRosenwasser Dec 13, 2016

Choose a reason for hiding this comment

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

Question: Does ContainsLexicalThisInComputedPropertyName make sense to check here? Does this apply to object literal computed properties?

@mhegazy @rbuckton

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, it's only used during aggregation to determine whether to promote ContainsLexicalThis in a computed property name so that it doesn't get stopped at a method or accessor.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The indentation of the if condition looks a bit odd here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So ContainsLexicalThis means merely that a this keyword was somewhere inside of the function. ContainsCapturedLexicalThis means that a this keyword was captured by an arrow function inside the function.

__extends(Derived, _super);
function Derived() {
return _super.call(this, function () { return _this; }) || this;
var _this = _super.call(this, function () { return _this; }) || this;
Copy link
Copy Markdown

@markboyall markboyall Dec 14, 2016

Choose a reason for hiding this comment

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

These baselines don't quite seem correct. Surely, _this should be declared and set to this first before the super call. That way, if the super call invokes the function, it'll still function as expected if the super does not return a new value for this.

Copy link
Copy Markdown
Member Author

@DanielRosenwasser DanielRosenwasser Dec 14, 2016

Choose a reason for hiding this comment

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

This is intentional - in ES2015 it is an error for this to be used before a call to super(...) completes.

This means that it shouldn't be an error to reference _this, but it should be an error to evaluate _this in a callback before the super(...) call returns.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ES2015 doesn't have a bunch of existing TypeScript code that evaluates this in a callback before the super() call returns.

Comment thread src/compiler/transformers/es2015.ts Outdated
// but only if the constructor itself doesn't use 'this' elsewhere.
if (superCallExpression
&& statementOffset === ctorStatements.length - 1
&& !(ctor.transformFlags & (TransformFlags.ContainsLexicalThis | TransformFlags.ContainsCapturedLexicalThis))) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The indentation of the if condition looks a bit odd here.

Comment thread src/compiler/transformers/es2015.ts Outdated
// but only if the constructor itself doesn't use 'this' elsewhere.
if (superCallExpression
&& statementOffset === ctorStatements.length - 1
&& !(ctor.transformFlags & (TransformFlags.ContainsLexicalThis | TransformFlags.ContainsCapturedLexicalThis))) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So ContainsLexicalThis means merely that a this keyword was somewhere inside of the function. ContainsCapturedLexicalThis means that a this keyword was captured by an arrow function inside the function.

@DanielRosenwasser DanielRosenwasser merged commit 8e94d84 into master Dec 14, 2016
@DanielRosenwasser DanielRosenwasser deleted the capturedThisFix branch December 14, 2016 21:31
@mhegazy mhegazy mentioned this pull request Dec 14, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants