Adds static type checking to the foreach loop.

Adds static type checking to the foreach loop.

Notice that the specification only states that the object being iterated over needs to implement Iterator<T> iterator() so this patch adheres to that.

BUG=http://code.google.com/p/dart/issues/detail?id=78 -

Committed: https://code.google.com/p/dart/source/detail?r=521

[karlklose, 2011-10-18 08:26:08]

LGTM.

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/Elements.java (right):

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:123:
@VisibleForTesting
Long line.

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:195:
this.typeProvider = typeProvider;
You could also store expectedIteratorType =
typeProvider.getIteratorType(variableType) here, instead of keeping the type
provider.

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:804:
typeOf(node.getIdentifier());
I would prefer the form

 Type variableType = ... ? ... 
                         : ...;

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:805: Type
iterableType = typeOf(node.getIterable());
Can you store node.getIterable() in a local variable and use it below?

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java
(right):

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java:756:
analyze("{ Array<String> strings = ['1','2','3']; for (String s in strings) {}
}");
Please use List instead of Array.

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/type/TypeTestCase.java (right):

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/type/TypeTestCase.java:87:
DartMethodDefinition iteratorMethod = DartMethodDefinition.create(new
DartIdentifier("iterator"),
Long line.

[fabiomfv, 2011-10-18 13:54:12]

LGTM!  few nits.

It would interesting to know what should be the fate from language folks of
iterable and interator() methods. it seems confusing to have an interface and
yet not being necessary (maybe we should remove iterable for good?)

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java
(right):

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java:755: //
Foreach tests
could you add a positive test to test to check when a 'custom' class does not
implement iterable and yet has a method iterator? (List<T> implements
collection<T> that impls iterable).

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java:756:
analyze("{ Array<String> strings = ['1','2','3']; for (String s in strings) {}
}");
shouldnt be list?

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java:757:
analyzeFail("{ Array<int> ints = [1,2,3]; for (String s in ints) {} }",
same.

[mmendez, 2011-10-18 18:35:37]

Fabio, I do agree that having Iterable looks weird -- I'll leave that to the
language guys to sort out.

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/Elements.java (right):

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:123:
@VisibleForTesting
On 2011/10/18 08:26:08, karlklose wrote:
> Long line.

Done.

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:195:
this.typeProvider = typeProvider;
On 2011/10/18 08:26:08, karlklose wrote:
> You could also store expectedIteratorType =
> typeProvider.getIteratorType(variableType) here, instead of keeping the type
> provider.

Done.

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:804:
typeOf(node.getIdentifier());
On 2011/10/18 08:26:08, karlklose wrote:
> I would prefer the form
> 
>  Type variableType = ... ? ... 
>                          : ...;

Done.

http://codereview.chromium.org/8322014/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:805: Type
iterableType = typeOf(node.getIterable());
On 2011/10/18 08:26:08, karlklose wrote:
> Can you store node.getIterable() in a local variable and use it below?

Done.

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java
(right):

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java:755: //
Foreach tests
On 2011/10/18 13:54:12, fabiomfv wrote:
> could you add a positive test to test to check when a 'custom' class does not
> implement iterable and yet has a method iterator? (List<T> implements
> collection<T> that impls iterable).

Done.  Added testForEachStatement.

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java:756:
analyze("{ Array<String> strings = ['1','2','3']; for (String s in strings) {}
}");
On 2011/10/18 13:54:12, fabiomfv wrote:
> shouldnt be list?

Done.  https://code.google.com/p/dart/source/detail?r=505 had not landed when I
sent this out.

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java:757:
analyzeFail("{ Array<int> ints = [1,2,3]; for (String s in ints) {} }",
On 2011/10/18 13:54:12, fabiomfv wrote:
> same.

Done.

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/type/TypeTestCase.java (right):

http://codereview.chromium.org/8322014/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/type/TypeTestCase.java:87:
DartMethodDefinition iteratorMethod = DartMethodDefinition.create(new
DartIdentifier("iterator"),
On 2011/10/18 08:26:08, karlklose wrote:
> Long line.

Done.