8000 Check using "super" before "this" lexically by yuit · Pull Request #6860 · microsoft/TypeScript · GitHub
[go: up one dir, main page]

Skip to content

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

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

Merged
merged 11 commits into from
Feb 11, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Check using "super" before "this" lexically instead of using the
NodeCheckFlags

Remove "type-checking" way of checking if super is used before this.
Instead check using whether super occurs before this syntactically

Refactor the code

Dive down to get super call
  • Loading branch information
Kanchalai Tanglertsampan committed Feb 3, 2016
commit ed0962883b9711f0f4c324c3e904bd776c5c9305
99 changes: 61 additions & 38 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7262,17 +7262,65 @@ namespace ts {
}
}

function isSuperCallExpression(n: Node): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

This function already exists elsewhere, so you can remove this one.

return n.kind === SyntaxKind.CallExpression && (<CallExpression>n).expression.kind === SyntaxKind.SuperKeyword;
}

/**
* 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 {
Copy link
Member

Choose a reason for hiding this comment

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

getSuperCallInConstructor

function getContainingSuperCall(n: Node): Node {
Copy link
Member

Choose a reason for hiding this comment

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

findFirstSuperCall might be more appropriate. This certainly isn't the containing super call.

Also, move this out of the function, since it doesn't close over anything.

if (isSuperCallExpression(n)) {
return n;
}
else if (isFunctionLike(n)) {
return undefined;
}
return forEachChild(n, getContainingSuperCall);
}

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);
}
return links.superStatement;
}

/**
* Check if the given class-declaration extends null then return true.
* Otherwise, return false
* @param classDecl a class declaration to check if it extends null
*/
function isClassDeclarationExtendNull(classDecl: ClassDeclaration): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

classDeclarationExtendsNull

const classSymbol = getSymbolOfNode(classDecl);
const classInstanceType = <InterfaceType>getDeclaredTypeOfSymbol(classSymbol);
const baseConstructorType = getBaseConstructorTypeOfClass(classInstanceType);

return baseConstructorType === nullType;
}

function checkThisExpression(node: Node): Type {
// Stop at the first arrow function so that we can
// tell whether 'this' needs to be captured.
let container = getThisContainer(node, /* includeArrowFunctions */ true);
let needToCaptureLexicalThis = false;

if (container.kind === SyntaxKind.Constructor) {
const baseTypeNode = getClassExtendsHeritageClauseElement(<ClassLikeDeclaration>container.parent);
if (baseTypeNode && !(getNodeCheckFlags(container) & NodeCheckFlags.HasSeenSuperCall)) {
// 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);
const containingClassDecl = <ClassDeclaration>container.parent;
const baseTypeNode = getClassExtendsHeritageClauseElement(containingClassDecl);

// 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);
Copy link
Member

Choose a reason for hiding this comment

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

superCall

if (!superStatement || (superStatement && superStatement.pos > node.pos || superStatement.end > node.pos)) {
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the superStatement && because that'll already have been taken care of

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason the condition is superStatement.pos > node.pos || superStatement.end > node.pos?

Can it be

superExpression.pos >= node.end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not sufficient to just check superExpression.pos >= node.end. Consider

super(this). 

// This should be an error as well but if you just check superExpression.pos >= node.end then we will not report this error correctly

Though, I agree with simplifying the check to superCall.end > node.pos calling to super should be done before accessing this

// 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);
}
}
}

Expand Down Expand Up @@ -10219,14 +10267,11 @@ namespace ts {
checkGrammarTypeArguments(node, node.typeArguments) || checkGrammarArguments(node, node.arguments);

const signature = getResolvedSignature(node);
if (node.expression.kind === SyntaxKind.SuperKeyword) {
const containgFunction = getContainingFunction(node.expression);

if (containgFunction && containgFunction.kind === SyntaxKind.Constructor) {
getNodeLinks(containgFunction).flags |= NodeCheckFlags.HasSeenSuperCall;
}
if (node.expression.kind === SyntaxKind.SuperKeyword) {
return voidType;
}

if (node.kind === SyntaxKind.NewExpression) {
const declaration = signature.declaration;

Expand Down Expand Up @@ -11766,27 +11811,6 @@ namespace ts {
return;
}

function isSuperCallExpression(n: Node): boolean {
return n.kind === SyntaxKind.CallExpression && (<CallExpression>n).expression.kind === SyntaxKind.SuperKeyword;
}

function containsSuperCallAsComputedPropertyName(n: Declaration): boolean {
return n.name && containsSuperCall(n.name);
}

function containsSuperCall(n: Node): boolean {
if (isSuperCallExpression(n)) {
return true;
}
else if (isFunctionLike(n)) {
return false;
}
else if (isClassLike(n)) {
return forEach((<ClassLikeDeclaration>n).members, containsSuperCallAsComputedPropertyName);
}
return forEachChild(n, containsSuperCall);
}

function markThisReferencesAsErrors(n: Node): void {
if (n.kind === SyntaxKind.ThisKeyword) {
error(n, Diagnostics.this_cannot_be_referenced_in_current_location);
Expand All @@ -11803,16 +11827,14 @@ namespace ts {
}

// TS 1.0 spec (April 2014): 8.3.2
// Constructors of classes with no extends clause may not contain super calls, whereas
// constructors of derived classes must contain at least one super call somewhere in their function body.
// Constructors of classes with no extends clause and constructors of classes that extends null may not contain super calls,
// whereas constructors of derived classes must contain at least one super call somewhere in their function body.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't currently in the spec, so revert this and file a spec bug, while linking to this section in the code.

const containingClassDecl = <ClassDeclaration>node.parent;
if (getClassExtendsHeritageClauseElement(containingClassDecl)) {
const containingClassSymbol = getSymbolOfNode(containingClassDecl);
const containingClassInstanceType = <InterfaceType>getDeclaredTypeOfSymbol(containingClassSymbol);
const baseConstructorType = getBaseConstructorTypeOfClass(containingClassInstanceType);
const isClassExtendNull = isClassDeclarationExtendNull(containingClassDecl);

if (containsSuperCall(node.body)) {
if (baseConstructorType === nullType) {
if (getSuperStatementInConstructor(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.

}

Expand All @@ -11830,6 +11852,7 @@ namespace ts {
if (superCallShouldBeFirst) {
const statements = (<Block>node.body).statements;
let superCallStatement: ExpressionStatement;

for (const statement of statements) {
if (statement.kind === SyntaxKind.ExpressionStatement && isSuperCallExpression((<ExpressionStatement>statement).expression)) {
superCallStatement = <ExpressionStatement>statement;
Expand All @@ -11844,7 +11867,7 @@ namespace ts {
}
}
}
else if (baseConstructorType !== nullType) {
else if (!isClassExtendNull) {
error(node, Diagnostics.Constructors_for_derived_classes_must_contain_a_super_call);
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2068,9 +2068,8 @@ namespace ts {
LoopWithCapturedBlockScopedBinding = 0x00010000, // Loop that contains block scoped variable captured in closure
CapturedBlockScopedBinding = 0x00020000, // Block-scoped binding that is captured in some function
BlockScopedBindingInLoop = 0x00040000, // Block-scoped binding with declaration nested inside iteration statement
HasSeenSuperCall = 0x00080000, // Set during the binding when encounter 'super'
ClassWithBodyScopedClassBinding = 0x00100000, // Decorated class that contains a binding to itself inside of the class body.
BodyScopedClassBinding = 0x00200000, // Binding to a decorated class inside of the class's body.
ClassWithBodyScopedClassBinding = 0x0080000, // Decorated class that contains a binding to itself inside of the class body.
BodyScopedClassBinding = 0x00100000, // Binding to a decorated class inside of the class's body.
}

/* @internal */
Expand All @@ -2089,6 +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
Copy link
Member

Choose a reason for hiding this comment

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

superCall

}

export const enum TypeFlags {
Expand Down
0