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

Support 'for...of' loops in ES6 #2164

Merged
merged 30 commits into from
Mar 2, 2015
Merged

Support 'for...of' loops in ES6 #2164

merged 30 commits into from
Mar 2, 2015

Conversation

JsonFreeman
Copy link
Contributor

The syntax and emit for for...of loops was already done, so this PR handles the type check aspect. It also introduces the concept of iterators in TypeScript.

If you have a statement of the form

for (var v of X) { }

or

for (v of X) { }

The main workhorse is getIteratedType in the checker, which will take the type of X, and get the type that it's an iterator of. That will then become the type of v. It does some error reporting along the way too.

Also in this PR is support for destructuring on the left hand side of the of keyword, an explicit disallowing of destructuring in a for...in statement, and the notion that Iterables can now contextually type array literals.

Pending is the support for spreading an Iterable, and array destructuring of an Iterable. Downlevel of for...of is also pending.

@@ -5198,20 +5202,20 @@ module ts {
if (container && container.parent && container.parent.kind === SyntaxKind.ClassDeclaration) {
if (container.flags & NodeFlags.Static) {
canUseSuperExpression =
container.kind === SyntaxKind.MethodDeclaration ||
Copy link
Contributor

Choose a reason for hiding this comment

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

this indent should remain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, I do not understand why this keeps happening.

checkGrammarForGenerator(node);
checkGrammarDisallowedModifiersInBlockOrObjectLiteralExpression(node) ||
checkGrammarFunctionName(node.name) ||
checkGrammarForGenerator(node);
Copy link
Member

Choose a reason for hiding this comment

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

I'm split between aesthetics and convention 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.

Reverting (none of this should have changed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean I'm keeping it, sorry. I like the new way better.

@DanielRosenwasser
Copy link
Member

13.6.4.1 of the ES6 Spec Draft:

  • It is a Syntax Error if the BoundNames of ForDeclaration contains "let".
  • It is a Syntax Error if any element of the BoundNames of ForDeclaration also occurs in the VarDeclaredNames of Statement.
  • It is a Syntax Error if the BoundNames of ForDeclaration contains any duplicate entries.

The latter two might be handled by @vladima; do we have a test for the first? We should be testing all three.



==== tests/cases/conformance/es6/for-ofStatements/for-of32.ts (1 errors) ====
for (var v of v) { }
Copy link
Member

Choose a reason for hiding this comment

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

Also a great test. However, I realized that the semantics of this if you used a let binding could be different.

let v = [1,2,3,4,5]
for (let v of v) {
    console.log(v);
}

If I try this out in Traceur, I get each element echoed to my console. I'm not necessarily sure whether this is the expected behavior since I'm not as familiar with this part of the spec. Doesn't look like there exists a test for this, so let's add one and make sure we have the right semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I'll try it out, thanks! I'll also test the three bullets you mentioned above, and see what happens. It is possible they will have to be fixed more holistically with regular variable declarations if they are broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm reading the spec section 13.6.4.12, ForIn/OfHeadEvaluation, and for this let v of v case, it looks like v is in a temporal dead zone. I'm not very familiar with these, but I guess that means referencing v in that position would be an error. I may simply open a bug for this, but I want to double check what the compiler is supposed to do for a case like that. I did notice that in the case of let v = v;, we treat it as having type any, so at least this is consistent.

Copy link
Member

Choose a reason for hiding this comment

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

I would assume it means that v as declared in the for-of statement is not usable from within the expression, so an outer-defined v would not be shadowed (and thus, still usable) at that point. Let's discuss tomorrow.

@JsonFreeman
Copy link
Contributor Author

@DanielRosenwasser regarding 13.6.4.1 of the ES6 Spec Draft:

I'm adding tests for all 3 bullets. The first and third bullet seem to be working. The second bullet is not working. That is, if you have

for (let x of []) {
    var x;
}

we do not give an error. I presume the idea here is that because ES6 has introduced a new lexical environment, they can give an error when any inner scope declares a var that gets shadowed by this new lexical scope. I will open a bug for this, but I want to talk to @vladima before doing anything, as it looks like we are also missing this error for catch blocks and regular for-loops as well. Also, we do get an error upon initializing the variable.

@DanielRosenwasser
Copy link
Member

Make a test like

for (var let of [1,2,3,4]) {
    // stuff!
}

after that 👍

JsonFreeman added a commit that referenced this pull request Mar 2, 2015
Support 'for...of' loops in ES6
@JsonFreeman JsonFreeman merged commit d6045e4 into master Mar 2, 2015
@JsonFreeman JsonFreeman deleted the for-ofES6 branch March 2, 2015 23:47
@DickvdBrink DickvdBrink mentioned this pull request Mar 8, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants