Type check for-in statements.

Type check for-in statements.

This does nominal checking on the iterable expression used in a for-in
statement:

1. It must implement Iterable.
2. The type argument to Iterable must be assignable to the variable's
   type.

R=brianwilkerson@google.com, jmesserly@google.com

Committed: https://github.com/dart-lang/sdk/commit/cb7aa02116917259e72d53676e77f961698778fb

[Bob Nystrom, 2016-03-08 00:30:49]

rnystrom@google.com changed reviewers:
+ brianwilkerson@google.com, jmesserly@google.com

[Bob Nystrom, 2016-03-08 00:31:41]

https://codereview.chromium.org/1771153002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1771153002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/type_system.dart:490: * Searches the
superinterfaces of [type] for implementations of [genericType]
I moved this out of Resolver into here because I needed to be able to call it
from ErrorVerifier too (and it doesn't really have anything to do with
resolving).

Is this OK?

[Brian Wilkerson, 2016-03-08 01:12:12]

LGTM

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error_verifier.dart:5726: void
_checkForYieldOfInvalidType(
fyi: I don't know that it matters, but the original pattern for the helper
methods in this class was to return `true` if they reported an error. It was
(and probably still it) used in a couple of cases to prevent extraneous
follow-on errors. I don't have a problem changing the pattern.

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/static_type_warning_code_test.dart (right):

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/static_type_warning_code_test.dart:2117: void
test_forIn_downcast() {
Should any of these tests have different behavior when analyzing in strong-mode?

[Bob Nystrom, 2016-03-08 19:00:50]

Thanks!

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error_verifier.dart:5726: void
_checkForYieldOfInvalidType(
On 2016/03/08 01:12:12, Brian Wilkerson wrote:
> fyi: I don't know that it matters, but the original pattern for the helper
> methods in this class was to return `true` if they reported an error. It was
> (and probably still it) used in a couple of cases to prevent extraneous
> follow-on errors.

I'm not sure about the other methods, but this one was only called in one place
and that place didn't use the return value so I figured I might as well clean up
a bit of unused code while I was in here.

> I don't have a problem changing the pattern.

SGTM. I didn't look at the other "_check___" methods closely, but my hunch is
they mostly report errors themselves now and don't need a return value either,
though I didn't want to mix a bunch of extra clean-up changes in this CL.

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/static_type_warning_code_test.dart (right):

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/static_type_warning_code_test.dart:2117: void
test_forIn_downcast() {
On 2016/03/08 01:12:12, Brian Wilkerson wrote:
> Should any of these tests have different behavior when analyzing in
strong-mode?

Right now, I think you'll just get duplicate errors reported because strong mode
has the same type checking rules here. They don't show up here, of course,
because this test suite doesn't look for the strong mode errors. Strong mode was
already reporting errors on for-in loops, so the code for those already exists
and should be tested (I think it's:
https://github.com/dart-lang/sdk/blob/master/pkg/analyzer/test/src/task/stron...).

This patch is a step towards ultimately eliminating those duplicate errors. My
goal is to let ErrorVerifier be solely in charge of reporting all sideways cast
errors and omit those from strong mode. Currently, there is a hole in that only
strong mode reports sideways cast errors in for-in loops. This gets
ErrorVerifier to parity on that corner of the language.

[Jennifer Messerly, 2016-03-08 19:14:11]

LGTM

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error_verifier.dart:5687: SimpleIdentifier
variable = node.identifier != null
probably not worth it, but this could be:

    node?.identifier ?? node.loopVariable.identifier

(aside: I've always found the AST structure of ForEach a bit confusing...)

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error_verifier.dart:5697: String loopTypeName =
node.awaitKeyword != null
do you need this? Isn't this just: `loopType.name` ?

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error_verifier.dart:5703: // TODO(rnystrom): Move
this into mostSpecificTypeArgument()?
yeah, I think that makes sense.

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (left):

https://codereview.chromium.org/1771153002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:8855: List<DartType>
_findImplementedTypeArgument(DartType t1, InterfaceType t2) {
oh awesome! I was wondering when reading error verifier code if you managed to
reuse this logic :)

[Bob Nystrom, 2016-03-08 19:25:52]

Description was changed from

==========
Type check for-in statements.

This does nominal checking on the iterable expression used in a for-in
statement:

1. It must implement Iterable.
2. The type argument to Iterable must be assignable to the variable's
   type.
==========

to

==========
Type check for-in statements.

This does nominal checking on the iterable expression used in a for-in
statement:

1. It must implement Iterable.
2. The type argument to Iterable must be assignable to the variable's
   type.

R=brianwilkerson@google.com, jmesserly@google.com

Committed:
https://github.com/dart-lang/sdk/commit/cb7aa02116917259e72d53676e77f961698778fb
==========

[Bob Nystrom, 2016-03-08 19:25:53]

Committed patchset #2 (id:20001) manually as
cb7aa02116917259e72d53676e77f961698778fb (presubmit successful).