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

Check using "super" before "this" lexically #6860

Merged
merged 11 commits into from
Feb 11, 2016
Prev Previous commit
Next Next commit
Address PR
Kanchalai Tanglertsampan committed Feb 3, 2016
commit 7e1088bb50d9c374a18af14388049fbcae080183
42 changes: 25 additions & 17 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
@@ -7266,28 +7266,28 @@ namespace ts {
return n.kind === SyntaxKind.CallExpression && (<CallExpression>n).expression.kind === SyntaxKind.SuperKeyword;
}

function findFirstSuperCall(n: Node): Node {
if (isSuperCallExpression(n)) {
return n;
}
else if (isFunctionLike(n)) {
return undefined;
}
return forEachChild(n, findFirstSuperCall);
}

/**
* Return a cached result if super-statement is already found.
* Otherwise, find a super statement in a given constructor function and cache the result in the node-links of the constructor
*
* @param constructor constructor-function to look for super statement
*/
function getSuperStatementInConstructor(constructor: ConstructorDeclaration): ExpressionStatement {
function getContainingSuperCall(n: Node): Node {
if (isSuperCallExpression(n)) {
return n;
}
else if (isFunctionLike(n)) {
return undefined;
}
return forEachChild(n, getContainingSuperCall);
}

function getSuperCallInConstructor(constructor: ConstructorDeclaration): ExpressionStatement {
Copy link
Member

Choose a reason for hiding this comment

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

The funny thing is that you don't actually use the returned value except to determine whether you've found a super call - so you should consider reducing your needs to NodeLinks.hasSuperCall or NodeLinks.superCall, but not both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used down below.

const links = getNodeLinks(constructor);
Copy link
Contributor

Choose a reason for hiding this comment

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

what if constructor does not have super call? does it mean that we'll repeat the traversal on every call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, unfortunately.

if (!links.superStatement) {
links.superStatement = <ExpressionStatement>getContainingSuperCall(constructor.body);
if (!links.superCall) {
links.superCall = <ExpressionStatement>findFirstSuperCall(constructor.body);
}
return links.superStatement;
return links.superCall;
}

/**
@@ -7316,8 +7316,16 @@ namespace ts {
// If a containing class does not have extends clause or the class extends null
// skip checking whether super statement is called before "this" accessing.
if (baseTypeNode && !isClassDeclarationExtendNull(containingClassDecl)) {
const superStatement = getSuperStatementInConstructor(<ConstructorDeclaration>container);
if (!superStatement || (superStatement && superStatement.pos > node.pos || superStatement.end > node.pos)) {
const superCall = getSuperCallInConstructor(<ConstructorDeclaration>container);

// We should give an error in the following cases:
// - No super-call
// - "this" is accessing before super-call.
// i.e super(this)
// this.x; super();
// We want to make sure that super-call is done before accessing "this" so that
// "this" is not accessed as a parameter of the super-call.
if (!superCall || superCall.end > node.pos) {
// In ES6, super inside constructor of class-declaration has to precede "this" accessing
error(node, Diagnostics.super_must_be_called_before_accessing_this_in_the_constructor_of_a_derived_class);
}
@@ -11833,7 +11841,7 @@ namespace ts {
if (getClassExtendsHeritageClauseElement(containingClassDecl)) {
const isClassExtendNull = isClassDeclarationExtendNull(containingClassDecl);

if (getSuperStatementInConstructor(node)) {
if (getSuperCallInConstructor(node)) {
if (isClassExtendNull) {
error(node, Diagnostics.A_constructor_cannot_contain_a_super_call_when_its_class_extends_null);
Copy link
Member

Choose a reason for hiding this comment

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

Currently you're erroring on the constructor node itself, which gives a pretty unpleasant experience (it errors on the entire body as well).

You should report an error on the super call that you were able to find.

}
2 changes: 1 addition & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
@@ -2088,7 +2088,7 @@ namespace ts {
importOnRightSide?: Symbol; // for import declarations - import that appear on the right side
jsxFlags?: JsxFlags; // flags for knowning what kind of element/attributes we're dealing with
resolvedJsxType?: Type; // resolved element attributes type of a JSX openinglike element
superStatement?: ExpressionStatement; // Cached super-statement found in the constructor. Used in checking whether super is called before this-accessing
superCall?: ExpressionStatement; // Cached first super-call found in the constructor. Used in checking whether super is called before this-accessing
Copy link
Member

Choose a reason for hiding this comment

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

You could just use hasProperty instead of having both of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hasProperty check property in the map

}

export const enum TypeFlags {