Skip to content

Simplify deferred function, class, and accessor checking #6083

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.

Sign up for GitHub

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 2 commits into from
Dec 15, 2015
Merged

Simplify deferred function, class, and accessor checking #6083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b44f6e4
Simplify deferred checking of function, class, and accessor bodies.
ahejlsberg Dec 12, 2015
91b9343
Accepting new baselines
ahejlsberg Dec 12, 2015
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
155 changes: 40 additions & 115 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ namespace ts {

let jsxElementClassType: Type;

let deferredNodes: Node[];
Copy link
Member

Choose a reason for hiding this comment

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

deferredCheckNodes and leave a decent comment about why these need to be deferred until the end.

Copy link
Member

Choose a reason for hiding this comment

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

Are you ever going to not have deferred nodes for most purposes? Seems like you'd have more overhead on a large project from checking over and over whether to defer than to create a one-time array allocation.

Copy link
Member Author

Choose a reason for hiding this comment

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

The deferredNodes array is used only when doing a full type check. When called from the language service (i.e. when "helicoptering in") we don't use it.


const tupleTypes: Map<TupleType> = {};
const unionTypes: Map<UnionType> = {};
const intersectionTypes: Map<IntersectionType> = {};
Expand Down Expand Up @@ -10123,6 +10125,7 @@ namespace ts {

if (!contextChecked) {
checkSignatureDeclaration(node);
checkNodeDeferred(node);
}
}
}
Expand All @@ -10135,7 +10138,7 @@ namespace ts {
return type;
}

function checkFunctionExpressionOrObjectLiteralMethodBody(node: ArrowFunction | FunctionExpression | MethodDeclaration) {
function checkFunctionExpressionOrObjectLiteralMethodDeferred(node: ArrowFunction | FunctionExpression | MethodDeclaration) {
Debug.assert(node.kind !== SyntaxKind.MethodDeclaration || isObjectLiteralMethod(node));

const isAsync = isAsyncFunctionLike(node);
Expand Down Expand Up @@ -10178,8 +10181,6 @@ namespace ts {
checkTypeAssignableTo(exprType, returnOrPromisedType, node.body);
}
}

checkFunctionAndClassExpressionBodies(node.body);
}
}
}
Expand Down Expand Up @@ -11428,13 +11429,13 @@ namespace ts {
if (node.parent.kind !== SyntaxKind.ObjectLiteralExpression) {
checkSourceElement(node.body);
}
else {
checkNodeDeferred(node);
}
}

function checkObjectLiteralAccessorBody(node: AccessorDeclaration) {
if (node.body) {
checkSourceElement(node.body);
checkFunctionAndClassExpressionBodies(node.body);
}
function checkAccessorDeferred(node: AccessorDeclaration) {
checkSourceElement(node.body);
}

function checkMissingDeclaration(node: Node) {
Expand Down Expand Up @@ -12373,11 +12374,7 @@ namespace ts {
if (node.kind === SyntaxKind.Block) {
checkGrammarStatementInAmbientContext(node);
}

forEach(node.statements, checkSourceElement);
if (isFunctionBlock(node) || node.kind === SyntaxKind.ModuleBlock) {
checkFunctionAndClassExpressionBodies(node);
}
}

function checkCollisionWithArgumentsInGeneratedCode(node: SignatureDeclaration) {
Expand Down Expand Up @@ -13406,15 +13403,19 @@ namespace ts {

function checkClassExpression(node: ClassExpression): Type {
checkClassLikeDeclaration(node);
checkNodeDeferred(node);
return getTypeOfSymbol(getSymbolOfNode(node));
}

function checkClassExpressionDeferred(node: ClassExpression) {
forEach(node.members, checkSourceElement);
}

function checkClassDeclaration(node: ClassDeclaration) {
if (!node.name && !(node.flags & NodeFlags.Default)) {
grammarErrorOnFirstToken(node, Diagnostics.A_class_declaration_without_the_default_modifier_must_have_a_name);
}
checkClassLikeDeclaration(node);

forEach(node.members, checkSourceElement);
}

Expand Down Expand Up @@ -14478,107 +14479,29 @@ namespace ts {
// Here, performing a full type check of the body of the function expression whilst in the process of
// determining the type of foo would cause foo to be given type any because of the recursive reference.
// Delaying the type check of the body ensures foo has been assigned a type.
Copy link
Member

Choose a reason for hiding this comment

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

Move the comment up to the definition of deferredCheckNodes and reword the comment appropriately.

function checkFunctionAndClassExpressionBodies(node: Node): void {
switch (node.kind) {
case SyntaxKind.FunctionExpression:
case SyntaxKind.ArrowFunction:
forEach((<FunctionLikeDeclaration>node).parameters, checkFunctionAndClassExpressionBodies);
checkFunctionExpressionOrObjectLiteralMethodBody(<FunctionExpression>node);
break;
case SyntaxKind.ClassExpression:
forEach((<ClassExpression>node).members, checkSourceElement);
forEachChild(node, checkFunctionAndClassExpressionBodies);
break;
case SyntaxKind.MethodDeclaration:
case SyntaxKind.MethodSignature:
forEach(node.decorators, checkFunctionAndClassExpressionBodies);
forEach((<MethodDeclaration>node).parameters, checkFunctionAndClassExpressionBodies);
if (isObjectLiteralMethod(node)) {
checkFunctionExpressionOrObjectLiteralMethodBody(node);
}
break;
case SyntaxKind.Constructor:
case SyntaxKind.FunctionDeclaration:
forEach((<FunctionLikeDeclaration>node).parameters, checkFunctionAndClassExpressionBodies);
break;
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
forEach((<FunctionLikeDeclaration>node).parameters, checkFunctionAndClassExpressionBodies);
if (node.parent.kind === SyntaxKind.ObjectLiteralExpression) {
checkObjectLiteralAccessorBody(<AccessorDeclaration>node);
}
break;
case SyntaxKind.WithStatement:
checkFunctionAndClassExpressionBodies((<WithStatement>node).expression);
break;
case SyntaxKind.Decorator:
case SyntaxKind.Parameter:
case SyntaxKind.PropertyDeclaration:
case SyntaxKind.PropertySignature:
case SyntaxKind.ObjectBindingPattern:
case SyntaxKind.ArrayBindingPattern:
case SyntaxKind.BindingElement:
case SyntaxKind.ArrayLiteralExpression:
case SyntaxKind.ObjectLiteralExpression:
case SyntaxKind.PropertyAssignment:
case SyntaxKind.PropertyAccessExpression:
case SyntaxKind.ElementAccessExpression:
case SyntaxKind.CallExpression:
case SyntaxKind.NewExpression:
case SyntaxKind.TaggedTemplateExpression:
case SyntaxKind.TemplateExpression:
case SyntaxKind.TemplateSpan:
case SyntaxKind.TypeAssertionExpression:
case SyntaxKind.AsExpression:
case SyntaxKind.ParenthesizedExpression:
case SyntaxKind.TypeOfExpression:
case SyntaxKind.VoidExpression:
case SyntaxKind.AwaitExpression:
case SyntaxKind.DeleteExpression:
case SyntaxKind.PrefixUnaryExpression:
case SyntaxKind.PostfixUnaryExpression:
case SyntaxKind.BinaryExpression:
case SyntaxKind.ConditionalExpression:
case SyntaxKind.SpreadElementExpression:
case SyntaxKind.YieldExpression:
case SyntaxKind.Block:
case SyntaxKind.ModuleBlock:
case SyntaxKind.VariableStatement:
case SyntaxKind.ExpressionStatement:
case SyntaxKind.IfStatement:
case SyntaxKind.DoStatement:
case SyntaxKind.WhileStatement:
case SyntaxKind.ForStatement:
case SyntaxKind.ForInStatement:
case SyntaxKind.ForOfStatement:
case SyntaxKind.ContinueStatement:
case SyntaxKind.BreakStatement:
case SyntaxKind.ReturnStatement:
case SyntaxKind.SwitchStatement:
case SyntaxKind.CaseBlock:
case SyntaxKind.CaseClause:
case SyntaxKind.DefaultClause:
case SyntaxKind.LabeledStatement:
case SyntaxKind.ThrowStatement:
case SyntaxKind.TryStatement:
case SyntaxKind.CatchClause:
case SyntaxKind.VariableDeclaration:
case SyntaxKind.VariableDeclarationList:
case SyntaxKind.ClassDeclaration:
case SyntaxKind.HeritageClause:
case SyntaxKind.ExpressionWithTypeArguments:
case SyntaxKind.EnumDeclaration:
case SyntaxKind.EnumMember:
case SyntaxKind.ExportAssignment:
case SyntaxKind.SourceFile:
case SyntaxKind.JsxExpression:
case SyntaxKind.JsxElement:
case SyntaxKind.JsxSelfClosingElement:
case SyntaxKind.JsxAttribute:
case SyntaxKind.JsxSpreadAttribute:
case SyntaxKind.JsxOpeningElement:
forEachChild(node, checkFunctionAndClassExpressionBodies);
break;
function checkNodeDeferred(node: Node) {
if (deferredNodes) {
deferredNodes.push(node);
}
}

function checkDeferredNodes() {
for (const node of deferredNodes) {
switch (node.kind) {
case SyntaxKind.FunctionExpression:
case SyntaxKind.ArrowFunction:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.MethodSignature:
checkFunctionExpressionOrObjectLiteralMethodDeferred(<FunctionExpression>node);
break;
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
checkAccessorDeferred(<AccessorDeclaration>node);
break;
case SyntaxKind.ClassExpression:
checkClassExpressionDeferred(<ClassExpression>node);
break;
}
}
}

Expand Down Expand Up @@ -14613,8 +14536,10 @@ namespace ts {
emitAwaiter = false;
potentialThisCollisions.length = 0;

deferredNodes = [];
Copy link
Member

Choose a reason for hiding this comment

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

I can't see what's above this because it's above the 11K mark, but does that mean that this is unconditionally initialized? If so, you can remove the initialization check in checkNodeDeferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it is not unconditional. See my earlier comment.

forEach(node.statements, checkSourceElement);
checkFunctionAndClassExpressionBodies(node);
checkDeferredNodes();
deferredNodes = undefined;

if (isExternalOrCommonJsModule(node)) {
checkExternalModuleExports(node);
Expand Down
11 changes: 1 addition & 10 deletions tests/baselines/reference/multiLinePropertyAccessAndArrowFunctionIndent1.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,11 @@
tests/cases/compiler/multiLinePropertyAccessAndArrowFunctionIndent1.ts(1,1): error TS1108: A 'return' statement can only be used within a function body.
tests/cases/compiler/multiLinePropertyAccessAndArrowFunctionIndent1.ts(1,18): error TS2304: Cannot find name 'role'.
tests/cases/compiler/multiLinePropertyAccessAndArrowFunctionIndent1.ts(2,18): error TS2304: Cannot find name 'Role'.
tests/cases/compiler/multiLinePropertyAccessAndArrowFunctionIndent1.ts(4,26): error TS2503: Cannot find namespace 'ng'.


==== tests/cases/compiler/multiLinePropertyAccessAndArrowFunctionIndent1.ts (4 errors) ====
==== tests/cases/compiler/multiLinePropertyAccessAndArrowFunctionIndent1.ts (1 errors) ====
return this.edit(role)
~~~~~~
!!! error TS1108: A 'return' statement can only be used within a function body.
~~~~
!!! error TS2304: Cannot find name 'role'.
.then((role: Role) =>
~~~~
!!! error TS2304: Cannot find name 'Role'.
this.roleService.add(role)
.then((data: ng.IHttpPromiseCallbackArg<Role>) => data.data));
~~
!!! error TS2503: Cannot find namespace 'ng'.

3 changes: 1 addition & 2 deletions tests/baselines/reference/multiLinePropertyAccessAndArrowFunctionIndent1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@ return this.edit(role)


//// [multiLinePropertyAccessAndArrowFunctionIndent1.js]
var _this = this;
return this.edit(role)
.then(function (role) {
return _this.roleService.add(role)
return this.roleService.add(role)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmz, is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is intended. The return statement is an error so we don't do any further checks on the expression. It will never execute anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah oke! Thanks for the clarification 👍

.then(function (data) { return data.data; });
});
5 changes: 1 addition & 4 deletions tests/baselines/reference/parserStatementIsNotAMemberVariableDeclaration1.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserStatementIsNotAMemberVariableDeclaration1.ts(1,1): error TS1108: A 'return' statement can only be used within a function body.
tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserStatementIsNotAMemberVariableDeclaration1.ts(6,5): error TS2304: Cannot find name 'private'.


==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserStatementIsNotAMemberVariableDeclaration1.ts (2 errors) ====
==== tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserStatementIsNotAMemberVariableDeclaration1.ts (1 errors) ====
return {
~~~~~~
!!! error TS1108: A 'return' statement can only be used within a function body.
Expand All @@ -11,8 +10,6 @@ tests/cases/conformance/parser/ecmascript5/ErrorRecovery/parserStatementIsNotAMe

// 'private' should not be considered a member variable here.
private[key] = value;
~~~~~~~
!!! error TS2304: Cannot find name 'private'.

}

Expand Down