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

Type checking 'for...of' in ES3/5 #2308

Merged
merged 9 commits into from
Mar 12, 2015
Merged
73 changes: 51 additions & 22 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1869,7 +1869,11 @@ module ts {
return anyType;
}
if (declaration.parent.parent.kind === SyntaxKind.ForOfStatement) {
return getTypeForVariableDeclarationInForOfStatement(<ForOfStatement>declaration.parent.parent);
// checkRightHandSideOfForOf will return undefined if the for-of expression type was
// missing properties/signatures required to get its iteratedType (like
// [Symbol.iterator] or next). This may be because we accessed properties from anyType,
// or it may have led to an error inside getIteratedType.
return checkRightHandSideOfForOf((<ForOfStatement>declaration.parent.parent).expression) || anyType;
}
if (isBindingPattern(declaration.parent)) {
return getTypeForBindingElement(<BindingElement>declaration);
Expand Down Expand Up @@ -8777,25 +8781,19 @@ module ts {
}

function checkForOfStatement(node: ForOfStatement): void {
if (languageVersion < ScriptTarget.ES6) {
grammarErrorOnFirstToken(node, Diagnostics.for_of_statements_are_only_available_when_targeting_ECMAScript_6_or_higher);
return;
}

checkGrammarForInOrForOfStatement(node)

// Check the LHS and RHS
// If the LHS is a declaration, just check it as a variable declaration, which will in turn check the RHS
// via getTypeForVariableDeclarationInForOfStatement.
// via checkRightHandSideOfForOf.
// If the LHS is an expression, check the LHS, as a destructuring assignment or as a reference.
// Then check that the RHS is assignable to it.
if (node.initializer.kind === SyntaxKind.VariableDeclarationList) {
checkForInOrForOfVariableDeclaration(node);
}
else {
var varExpr = <Expression>node.initializer;
var rightType = checkExpression(node.expression);
var iteratedType = checkIteratedType(rightType, node.expression);
var iteratedType = checkRightHandSideOfForOf(node.expression);

// There may be a destructuring assignment on the left side
if (varExpr.kind === SyntaxKind.ArrayLiteralExpression || varExpr.kind === SyntaxKind.ObjectLiteralExpression) {
Expand Down Expand Up @@ -8877,18 +8875,11 @@ module ts {
}
}

function getTypeForVariableDeclarationInForOfStatement(forOfStatement: ForOfStatement): Type {
// Temporarily return 'any' below ES6
if (languageVersion < ScriptTarget.ES6) {
return anyType;
}

// iteratedType will be undefined if the for-of expression type was missing properties/signatures
// required to get its iteratedType (like [Symbol.iterator] or next). This may be
// because we accessed properties from anyType, or it may have led to an error inside
// getIteratedType.
var expressionType = getTypeOfExpression(forOfStatement.expression);
return checkIteratedType(expressionType, forOfStatement.expression) || anyType;
function checkRightHandSideOfForOf(rhsExpression: Expression): Type {
var expressionType = getTypeOfExpression(rhsExpression);
return languageVersion >= ScriptTarget.ES6
? checkIteratedType(expressionType, rhsExpression)
: checkElementTypeOfArrayOrString(expressionType, rhsExpression);
}

/**
Expand Down Expand Up @@ -8987,6 +8978,45 @@ module ts {
}
}

function checkElementTypeOfArrayOrString(arrayOrStringType: Type, expressionForError: Expression): Type {
Debug.assert(languageVersion < ScriptTarget.ES6);
var isJustString = allConstituentTypesHaveKind(arrayOrStringType, TypeFlags.StringLike);
Copy link
Contributor

Choose a reason for hiding this comment

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

isNotUnjustString


// Check isJustString because removeTypesFromUnionType will only remove types if it doesn't result
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this behavior should be in removeTypesFromUnionType. i.e. have a flag like 'allowAllTypesToBeRemoved'. It's very weird that 'removeTypesFromUnionType' will choose to not actually remove the type, and that you have to work around that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think you are right. I was actually thinking about that when I did this. I can add a parameter called allowEmptyUnionResult.

// in an emptyObjectType. In this case, we actually do want the emptyObjectType.
var arrayType = isJustString ? emptyObjectType : removeTypesFromUnionType(arrayOrStringType, TypeFlags.StringLike, /*isTypeOfKind*/ true);
var hasStringConstituent = arrayOrStringType !== emptyObjectType && arrayOrStringType !== arrayType;

var reportedError = false;
if (hasStringConstituent && languageVersion < ScriptTarget.ES5) {
error(expressionForError, Diagnostics.Using_a_string_in_a_for_of_statement_is_only_supported_in_ECMAScript_5_and_higher);
reportedError = true;
}

if (isJustString) {
return stringType;
}

if (!isArrayLikeType(arrayType)) {
if (!reportedError) {
error(expressionForError, Diagnostics.Type_0_is_not_an_array_type, typeToString(arrayType));
Copy link
Contributor

Choose a reason for hiding this comment

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

SHould this be 'is not an array or string type'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe... but if you give the type string | number, it will say that type number is not an array type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yet, if you give the type number, it should say that number is not a string or array type.

}
return hasStringConstituent ? stringType : unknownType;
}

var arrayElementType = getIndexTypeOfType(arrayType, IndexKind.Number) || unknownType;
if (hasStringConstituent) {
// This is just an optimization for the case where arrayOrStringType is string | string[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add, at the top of this function, the types of types you want this fnction to be able to handle, and what final type will be returned?

Note: this function never seems to return 'undefined'. But i thought you used || earlier to default to hte 'any' type of you got 'undefined' back from this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I will add a summary of what the result should be, as well as some examples.

if (arrayElementType.flags & TypeFlags.StringLike) {
return stringType;
}

return getUnionType([arrayElementType, stringType]);
}

return arrayElementType;
}

function checkBreakOrContinueStatement(node: BreakOrContinueStatement) {
// Grammar checking
checkGrammarStatementInAmbientContext(node) || checkGrammarBreakOrContinueStatement(node);
Expand Down Expand Up @@ -10931,7 +10961,6 @@ module ts {
}

function isUnknownIdentifier(location: Node, name: string): boolean {
// Do not call resolveName on a synthesized node!
Debug.assert(!nodeIsSynthesized(location), "isUnknownIdentifier called with a synthesized location");
return !resolveName(location, name, SymbolFlags.Value, /*nodeNotFoundMessage*/ undefined, /*nameArg*/ undefined) &&
!hasProperty(getGeneratedNamesForSourceFile(getSourceFile(location)), name);
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/diagnosticInformationMap.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,6 @@ module ts {
Property_0_does_not_exist_on_const_enum_1: { code: 2479, category: DiagnosticCategory.Error, key: "Property '{0}' does not exist on 'const' enum '{1}'." },
let_is_not_allowed_to_be_used_as_a_name_in_let_or_const_declarations: { code: 2480, category: DiagnosticCategory.Error, key: "'let' is not allowed to be used as a name in 'let' or 'const' declarations." },
Cannot_initialize_outer_scoped_variable_0_in_the_same_scope_as_block_scoped_declaration_1: { code: 2481, category: DiagnosticCategory.Error, key: "Cannot initialize outer scoped variable '{0}' in the same scope as block scoped declaration '{1}'." },
for_of_statements_are_only_available_when_targeting_ECMAScript_6_or_higher: { code: 2482, category: DiagnosticCategory.Error, key: "'for...of' statements are only available when targeting ECMAScript 6 or higher." },
The_left_hand_side_of_a_for_of_statement_cannot_use_a_type_annotation: { code: 2483, category: DiagnosticCategory.Error, key: "The left-hand side of a 'for...of' statement cannot use a type annotation." },
Export_declaration_conflicts_with_exported_declaration_of_0: { code: 2484, category: DiagnosticCategory.Error, key: "Export declaration conflicts with exported declaration of '{0}'" },
The_left_hand_side_of_a_for_of_statement_cannot_be_a_previously_defined_constant: { code: 2485, category: DiagnosticCategory.Error, key: "The left-hand side of a 'for...of' statement cannot be a previously defined constant." },
Expand All @@ -339,6 +338,7 @@ module ts {
The_left_hand_side_of_a_for_in_statement_cannot_be_a_destructuring_pattern: { code: 2491, category: DiagnosticCategory.Error, key: "The left-hand side of a 'for...in' statement cannot be a destructuring pattern." },
Cannot_redeclare_identifier_0_in_catch_clause: { code: 2492, category: DiagnosticCategory.Error, key: "Cannot redeclare identifier '{0}' in catch clause" },
Tuple_type_0_with_length_1_cannot_be_assigned_to_tuple_with_length_2: { code: 2493, category: DiagnosticCategory.Error, key: "Tuple type '{0}' with length '{1}' cannot be assigned to tuple with length '{2}'." },
Using_a_string_in_a_for_of_statement_is_only_supported_in_ECMAScript_5_and_higher: { code: 2494, category: DiagnosticCategory.Error, key: "Using a string in a 'for...of' statement is only supported in ECMAScript 5 and higher." },
Import_declaration_0_is_using_private_name_1: { code: 4000, category: DiagnosticCategory.Error, key: "Import declaration '{0}' is using private name '{1}'." },
Type_parameter_0_of_exported_class_has_or_is_using_private_name_1: { code: 4002, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported class has or is using private name '{1}'." },
Type_parameter_0_of_exported_interface_has_or_is_using_private_name_1: { code: 4004, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported interface has or is using private name '{1}'." },
Expand Down
8 changes: 4 additions & 4 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1299,10 +1299,6 @@
"category": "Error",
"code": 2481
},
"'for...of' statements are only available when targeting ECMAScript 6 or higher.": {
"category": "Error",
"code": 2482
},
"The left-hand side of a 'for...of' statement cannot use a type annotation.": {
"category": "Error",
"code": 2483
Expand Down Expand Up @@ -1347,6 +1343,10 @@
"category": "Error",
"code": 2493
},
"Using a string in a 'for...of' statement is only supported in ECMAScript 5 and higher.": {
"category": "Error",
"code": 2494
},

"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
Expand Down
7 changes: 7 additions & 0 deletions tests/baselines/reference/ES3For-ofTypeCheck1.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
tests/cases/conformance/statements/for-ofStatements/ES3For-ofTypeCheck1.ts(1,15): error TS2494: Using a string in a 'for...of' statement is only supported in ECMAScript 5 and higher.


==== tests/cases/conformance/statements/for-ofStatements/ES3For-ofTypeCheck1.ts (1 errors) ====
for (var v of "") { }
~~
!!! error TS2494: Using a string in a 'for...of' statement is only supported in ECMAScript 5 and higher.
7 changes: 7 additions & 0 deletions tests/baselines/reference/ES3For-ofTypeCheck1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//// [ES3For-ofTypeCheck1.ts]
for (var v of "") { }

//// [ES3For-ofTypeCheck1.js]
for (var _i = 0, _a = ""; _i < _a.length; _i++) {
var v = _a[_i];
}
9 changes: 9 additions & 0 deletions tests/baselines/reference/ES3For-ofTypeCheck2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//// [ES3For-ofTypeCheck2.ts]
for (var v of [true]) { }

//// [ES3For-ofTypeCheck2.js]
for (var _i = 0, _a = [
true
]; _i < _a.length; _i++) {
var v = _a[_i];
}
5 changes: 5 additions & 0 deletions tests/baselines/reference/ES3For-ofTypeCheck2.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
=== tests/cases/conformance/statements/for-ofStatements/ES3For-ofTypeCheck2.ts ===
for (var v of [true]) { }
>v : boolean
>[true] : boolean[]

8 changes: 8 additions & 0 deletions tests/baselines/reference/ES3For-ofTypeCheck4.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
tests/cases/conformance/statements/for-ofStatements/ES3For-ofTypeCheck4.ts(2,17): error TS2494: Using a string in a 'for...of' statement is only supported in ECMAScript 5 and higher.


==== tests/cases/conformance/statements/for-ofStatements/ES3For-ofTypeCheck4.ts (1 errors) ====
var union: string | string[];
for (const v of union) { }
~~~~~
!!! error TS2494: Using a string in a 'for...of' statement is only supported in ECMAScript 5 and higher.
9 changes: 9 additions & 0 deletions tests/baselines/reference/ES3For-ofTypeCheck4.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//// [ES3For-ofTypeCheck4.ts]
var union: string | string[];
for (const v of union) { }

//// [ES3For-ofTypeCheck4.js]
var union;
for (var _i = 0; _i < union.length; _i++) {
var v = union[_i];
}
9 changes: 9 additions & 0 deletions tests/baselines/reference/ES3For-ofTypeCheck6.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//// [ES3For-ofTypeCheck6.ts]
var union: string[] | number[];
for (var v of union) { }

//// [ES3For-ofTypeCheck6.js]
var union;
for (var _i = 0; _i < union.length; _i++) {
var v = union[_i];
}
8 changes: 8 additions & 0 deletions tests/baselines/reference/ES3For-ofTypeCheck6.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
=== tests/cases/conformance/statements/for-ofStatements/ES3For-ofTypeCheck6.ts ===
var union: string[] | number[];
>union : string[] | number[]

for (var v of union) { }
>v : string | number
>union : string[] | number[]

6 changes: 3 additions & 3 deletions tests/baselines/reference/ES5For-of1.errors.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
tests/cases/conformance/statements/for-ofStatements/ES5For-of1.ts(1,1): error TS2482: 'for...of' statements are only available when targeting ECMAScript 6 or higher.
tests/cases/conformance/statements/for-ofStatements/ES5For-of1.ts(2,5): error TS2304: Cannot find name 'console'.


==== tests/cases/conformance/statements/for-ofStatements/ES5For-of1.ts (1 errors) ====
for (var v of ['a', 'b', 'c']) {
~~~
!!! error TS2482: 'for...of' statements are only available when targeting ECMAScript 6 or higher.
console.log(v);
~~~~~~~
!!! error TS2304: Cannot find name 'console'.
}
13 changes: 0 additions & 13 deletions tests/baselines/reference/ES5For-of10.errors.txt

This file was deleted.

29 changes: 29 additions & 0 deletions tests/baselines/reference/ES5For-of10.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
=== tests/cases/conformance/statements/for-ofStatements/ES5For-of10.ts ===
function foo() {
>foo : () => { x: number; }

return { x: 0 };
>{ x: 0 } : { x: number; }
>x : number
}
for (foo().x of []) {
>foo().x : number
>foo() : { x: number; }
>foo : () => { x: number; }
>x : number
>[] : undefined[]

for (foo().x of [])
>foo().x : number
>foo() : { x: number; }
>foo : () => { x: number; }
>x : number
>[] : undefined[]

var p = foo().x;
>p : number
>foo().x : number
>foo() : { x: number; }
>foo : () => { x: number; }
>x : number
}
8 changes: 0 additions & 8 deletions tests/baselines/reference/ES5For-of11.errors.txt

This file was deleted.

8 changes: 8 additions & 0 deletions tests/baselines/reference/ES5For-of11.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
=== tests/cases/conformance/statements/for-ofStatements/ES5For-of11.ts ===
var v;
>v : any

for (v of []) { }
>v : any
>[] : undefined[]

8 changes: 4 additions & 4 deletions tests/baselines/reference/ES5For-of12.errors.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
tests/cases/conformance/statements/for-ofStatements/ES5For-of12.ts(1,1): error TS2482: 'for...of' statements are only available when targeting ECMAScript 6 or higher.
tests/cases/conformance/statements/for-ofStatements/ES5For-of12.ts(1,7): error TS2364: Invalid left-hand side of assignment expression.


==== tests/cases/conformance/statements/for-ofStatements/ES5For-of12.ts (1 errors) ====
for ([""] of []) { }
~~~
!!! error TS2482: 'for...of' statements are only available when targeting ECMAScript 6 or higher.
for ([""] of [[""]]) { }
~~
!!! error TS2364: Invalid left-hand side of assignment expression.
8 changes: 6 additions & 2 deletions tests/baselines/reference/ES5For-of12.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
//// [ES5For-of12.ts]
for ([""] of []) { }
for ([""] of [[""]]) { }

//// [ES5For-of12.js]
for (var _i = 0, _a = []; _i < _a.length; _i++) {
for (var _i = 0, _a = [
[
""
]
]; _i < _a.length; _i++) {
"" = _a[_i][0];
}
9 changes: 0 additions & 9 deletions tests/baselines/reference/ES5For-of13.errors.txt

This file was deleted.

9 changes: 9 additions & 0 deletions tests/baselines/reference/ES5For-of13.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
=== tests/cases/conformance/statements/for-ofStatements/ES5For-of13.ts ===
for (let v of ['a', 'b', 'c']) {
>v : string
>['a', 'b', 'c'] : string[]

var x = v;
>x : string
>v : string
}
9 changes: 0 additions & 9 deletions tests/baselines/reference/ES5For-of14.errors.txt

This file was deleted.

9 changes: 9 additions & 0 deletions tests/baselines/reference/ES5For-of14.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
=== tests/cases/conformance/statements/for-ofStatements/ES5For-of14.ts ===
for (const v of []) {
>v : any
>[] : undefined[]

var x = v;
>x : any
>v : any
}
12 changes: 0 additions & 12 deletions tests/baselines/reference/ES5For-of15.errors.txt

This file was deleted.

Loading