-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Changes from 1 commit
a5cf7c1
76b6bff
ed09628
7e1088b
2dbc998
aff6775
1d4c1d1
47a2f3b
1cfce12
dc8e0cc
61e954b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7262,17 +7262,65 @@ namespace ts { | |
} | ||
} | ||
|
||
function isSuperCallExpression(n: Node): boolean { | ||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
function getContainingSuperCall(n: Node): Node { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if constructor does not have There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if (!superStatement || (superStatement && superStatement.pos > node.pos || superStatement.end > node.pos)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason the condition is Can it be
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is not sufficient to just check super(this). // This should be an error as well but if you just check Though, I agree with simplifying the check to |
||
// 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); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -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; | ||
|
||
|
@@ -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); | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
|
@@ -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; | ||
|
@@ -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); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 */ | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. superCall |
||
} | ||
|
||
export const enum TypeFlags { | ||
|
There was a problem hiding this comment.
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.