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.

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

Jump to bottom

Improved checking of for-in statements #6379

Merged
merged 9 commits into from
Jan 8, 2016
Merged

Improved checking of for-in statements #6379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a05b1de
Change for-in iteration variable type from any to string
ahejlsberg Jan 6, 2016
75b4c50
Updating existing tests
ahejlsberg Jan 6, 2016
dbb4544
Adding new tests
ahejlsberg Jan 6, 2016
9ce5e64
Accepting new baselines
ahejlsberg Jan 6, 2016
120569a
Support for-in both with and without variable declaration
ahejlsberg Jan 7, 2016
1798024
Updating tests
ahejlsberg Jan 7, 2016
537fb4a
Merge branch 'master' into forInChecking
ahejlsberg Jan 7, 2016
3344a42
Check for numeric index signature instead of array-like type
ahejlsberg Jan 7, 2016
a0fcc0f
Accepting new baselines
ahejlsberg Jan 7, 2016
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
66 changes: 60 additions & 6 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2520,9 +2520,9 @@ namespace ts {

// Return the inferred type for a variable, parameter, or property declaration
function getTypeForVariableLikeDeclaration(declaration: VariableLikeDeclaration): Type {
// A variable declared in a for..in statement is always of type any
// A variable declared in a for..in statement is always of type string
if (declaration.parent.parent.kind === SyntaxKind.ForInStatement) {
return anyType;
return stringType;
}

if (declaration.parent.parent.kind === SyntaxKind.ForOfStatement) {
Expand Down Expand Up @@ -8563,6 +8563,56 @@ namespace ts {
return true;
}

/**
* Return the symbol of the for-in variable declared or referenced by the given for-in statement.
*/
function getForInVariableSymbol(node: ForInStatement): Symbol {
const initializer = node.initializer;
if (initializer.kind === SyntaxKind.VariableDeclarationList) {
const variable = (<VariableDeclarationList>initializer).declarations[0];
if (variable && !isBindingPattern(variable.name)) {
return getSymbolOfNode(variable);
}
}
else if (initializer.kind === SyntaxKind.Identifier) {
return getResolvedSymbol(<Identifier>initializer);
}
return undefined;
}

/**
* Return true if the given type is considered to have numeric property names.
*/
function hasNumericPropertyNames(type: Type) {
return getIndexTypeOfType(type, IndexKind.Number) && !getIndexTypeOfType(type, IndexKind.String);
}

/**
* Return true if given node is an expression consisting of an identifier (possibly parenthesized)
* that references a for-in variable for an object with numeric property names.
*/
function isForInVariableForNumericPropertyNames(expr: Expression) {
const e = skipParenthesizedNodes(expr);
if (e.kind === SyntaxKind.Identifier) {
const symbol = getResolvedSymbol(<Identifier>e);
if (symbol.flags & SymbolFlags.Variable) {
let child: Node = expr;
let node = expr.parent;
while (node) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to climb up? Can't you look up the variable declaration from the symbol and just check if the parent is a ForInStatement?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I realized it's partially because you might have multiple variable declarations. But in that case, you'll have gotten an error on inconsistent types between the two variables anyway, so maybe it doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Several reasons we need to climb up. Multiple declarations is one, making sure we're actually within a for-in statement is another, supporting for-in that references a variable instead of declaring one is a third.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha. Maybe you should add a comment in case anyone stumbles along and tries to "optimize" it in the future

if (node.kind === SyntaxKind.ForInStatement &&
child === (<ForInStatement>node).statement &&
getForInVariableSymbol(<ForInStatement>node) === symbol &&
hasNumericPropertyNames(checkExpression((<ForInStatement>node).expression))) {
return true;
}
child = node;
node = node.parent;
}
}
}
return false;
}

function checkIndexedAccess(node: ElementAccessExpression): Type {
// Grammar checking
if (!node.argumentExpression) {
Expand Down Expand Up @@ -8623,7 +8673,7 @@ namespace ts {
if (isTypeAnyOrAllConstituentTypesHaveKind(indexType, TypeFlags.StringLike | TypeFlags.NumberLike | TypeFlags.ESSymbol)) {

// Try to use a number indexer.
if (isTypeAnyOrAllConstituentTypesHaveKind(indexType, TypeFlags.NumberLike)) {
if (isTypeAnyOrAllConstituentTypesHaveKind(indexType, TypeFlags.NumberLike) || isForInVariableForNumericPropertyNames(node.argumentExpression)) {
const numberIndexType = getIndexTypeOfType(objectType, IndexKind.Number);
if (numberIndexType) {
return numberIndexType;
Expand All @@ -8638,7 +8688,9 @@ namespace ts {

// Fall back to any.
if (compilerOptions.noImplicitAny && !compilerOptions.suppressImplicitAnyIndexErrors && !isTypeAny(objectType)) {
error(node, Diagnostics.Index_signature_of_object_type_implicitly_has_an_any_type);
error(node, getIndexTypeOfType(objectType, IndexKind.Number) ?
Diagnostics.Element_implicitly_has_an_any_type_because_index_expression_is_not_of_type_number :
Diagnostics.Index_signature_of_object_type_implicitly_has_an_any_type);
}

return anyType;
Expand Down Expand Up @@ -12697,7 +12749,8 @@ namespace ts {
}
// For a binding pattern, validate the initializer and exit
if (isBindingPattern(node.name)) {
if (node.initializer) {
// Don't validate for-in initializer as it is already an error
if (node.initializer && node.parent.parent.kind !== SyntaxKind.ForInStatement) {
checkTypeAssignableTo(checkExpressionCached(node.initializer), getWidenedTypeForVariableLikeDeclaration(node), node, /*headMessage*/ undefined);
checkParameterInitializer(node);
}
Expand All @@ -12707,7 +12760,8 @@ namespace ts {
const type = getTypeOfVariableOrParameterOrProperty(symbol);
if (node === symbol.valueDeclaration) {
// Node is the primary declaration of the symbol, just validate the initializer
if (node.initializer) {
// Don't validate for-in initializer as it is already an error
if (node.initializer && node.parent.parent.kind !== SyntaxKind.ForInStatement) {
checkTypeAssignableTo(checkExpressionCached(node.initializer), type, node, /*headMessage*/ undefined);
checkParameterInitializer(node);
}
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2422,6 +2422,10 @@
"category": "Error",
"code": 7013
},
"Element implicitly has an 'any' type because index expression is not of type 'number'.": {
"category": "Error",
"code": 7015
},
"Property '{0}' implicitly has type 'any', because its 'set' accessor lacks a type annotation.": {
"category": "Error",
"code": 7016
Expand Down
28 changes: 14 additions & 14 deletions tests/baselines/reference/capturedLetConstInLoop1.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
=== tests/cases/compiler/capturedLetConstInLoop1.ts ===
//==== let
for (let x in {}) {
>x : any
>x : string
>{} : {}

(function() { return x});
>(function() { return x}) : () => any
>function() { return x} : () => any
>x : any
>(function() { return x}) : () => string
>function() { return x} : () => string
>x : string

(() => x);
>(() => x) : () => any
>() => x : () => any
>x : any
>(() => x) : () => string
>() => x : () => string
>x : string
}

for (let x of []) {
Expand Down Expand Up @@ -216,18 +216,18 @@ for (let y = 0; y < 1; ++y) {

//=========const
for (const x in {}) {
>x : any
>x : string
>{} : {}

(function() { return x});
>(function() { return x}) : () => any
>function() { return x} : () => any
>x : any
>(function() { return x}) : () => string
>function() { return x} : () => string
>x : string

(() => x);
>(() => x) : () => any
>() => x : () => any
>x : any
>(() => x) : () => string
>() => x : () => string
>x : string
}

for (const x of []) {
Expand Down
28 changes: 14 additions & 14 deletions tests/baselines/reference/capturedLetConstInLoop1_ES6.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
=== tests/cases/compiler/capturedLetConstInLoop1_ES6.ts ===
//==== let
for (let x in {}) {
>x : any
>x : string
>{} : {}

(function() { return x});
>(function() { return x}) : () => any
>function() { return x} : () => any
>x : any
>(function() { return x}) : () => string
>function() { return x} : () => string
>x : string

(() => x);
>(() => x) : () => any
>() => x : () => any
>x : any
>(() => x) : () => string
>() => x : () => string
>x : string
}

for (let x of []) {
Expand Down Expand Up @@ -216,18 +216,18 @@ for (let y = 0; y < 1; ++y) {

//=========const
for (const x in {}) {
>x : any
>x : string
>{} : {}

(function() { return x});
>(function() { return x}) : () => any
>function() { return x} : () => any
>x : any
>(function() { return x}) : () => string
>function() { return x} : () => string
>x : string

(() => x);
>(() => x) : () => any
>() => x : () => any
>x : any
>(() => x) : () => string
>() => x : () => string
>x : string
}

for (const x of []) {
Expand Down
36 changes: 18 additions & 18 deletions tests/baselines/reference/capturedLetConstInLoop2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ function foo0_1(x) {
>x : any

for (let x in []) {
>x : any
>x : string
>[] : undefined[]

let a = arguments.length;
Expand All @@ -47,17 +47,17 @@ function foo0_1(x) {
>length : number

(function() { return x + a });
>(function() { return x + a }) : () => any
>function() { return x + a } : () => any
>x + a : any
>x : any
>(function() { return x + a }) : () => string
>function() { return x + a } : () => string
>x + a : string
>x : string
>a : number

(() => x + a);
>(() => x + a) : () => any
>() => x + a : () => any
>x + a : any
>x : any
>(() => x + a) : () => string
>() => x + a : () => string
>x + a : string
>x : string
>a : number
}
}
Expand Down Expand Up @@ -400,7 +400,7 @@ function foo0_1_c(x) {
>x : any

for (const x in []) {
>x : any
>x : string
>[] : undefined[]

const a = arguments.length;
Expand All @@ -410,17 +410,17 @@ function foo0_1_c(x) {
>length : number

(function() { return x + a });
>(function() { return x + a }) : () => any
>function() { return x + a } : () => any
>x + a : any
>x : any
>(function() { return x + a }) : () => string
>function() { return x + a } : () => string
>x + a : string
>x : string
>a : number

(() => x + a);
>(() => x + a) : () => any
>() => x + a : () => any
>x + a : any
>x : any
>(() => x + a) : () => string
>() => x + a : () => string
>x + a : string
>x : string
>a : number
}
}
Expand Down
36 changes: 18 additions & 18 deletions tests/baselines/reference/capturedLetConstInLoop2_ES6.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ function foo0_1(x) {
>x : any

for (let x in []) {
>x : any
>x : string
>[] : undefined[]

let a = arguments.length;
Expand All @@ -46,17 +46,17 @@ function foo0_1(x) {
>length : number

(function() { return x + a });
>(function() { return x + a }) : () => any
>function() { return x + a } : () => any
>x + a : any
>x : any
>(function() { return x + a }) : () => string
>function() { return x + a } : () => string
>x + a : string
>x : string
>a : number

(() => x + a);
>(() => x + a) : () => any
>() => x + a : () => any
>x + a : any
>x : any
>(() => x + a) : () => string
>() => x + a : () => string
>x + a : string
>x : string
>a : number
}
}
Expand Down Expand Up @@ -399,7 +399,7 @@ function foo0_1_c(x) {
>x : any

for (const x in []) {
>x : any
>x : string
>[] : undefined[]

const a = arguments.length;
Expand All @@ -409,17 +409,17 @@ function foo0_1_c(x) {
>length : number

(function() { return x + a });
>(function() { return x + a }) : () => any
>function() { return x + a } : () => any
>x + a : any
>x : any
>(function() { return x + a }) : () => string
>function() { return x + a } : () => string
>x + a : string
>x : string
>a : number

(() => x + a);
>(() => x + a) : () => any
>() => x + a : () => any
>x + a : any
>x : any
>(() => x + a) : () => string
>() => x + a : () => string
>x + a : string
>x : string
>a : number
}
}
Expand Down
Loading