Better casting of for-in loops.

Better casting of for-in loops.

Generate a cast for each element, so that code like this can be
handled correctly:

  for (String s in <dynamic>["a", "b", "c"]) { ... }

Fixes #25916.

R=vsm@google.com

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

[Bob Nystrom, 2016-03-21 21:02:36]

rnystrom@google.com changed reviewers:
+ vsm@google.com

[Bob Nystrom, 2016-03-21 21:02:36]

There will be a little associated DDC work required to use this cast, but this
should handle the analyzer side of the issue. I'll file a second bug in DDC for
the JS side.

[Bob Nystrom, 2016-03-21 21:12:42]

On 2016/03/21 21:02:36, Bob Nystrom wrote:
> There will be a little associated DDC work required to use this cast, but this
> should handle the analyzer side of the issue. I'll file a second bug in DDC
for
> the JS side.

I think this existing bug works for tracking the DDC side:
https://github.com/dart-lang/dev_compiler/issues/166

[vsm, 2016-03-21 21:25:21]

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong/checker.dart (right):

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/checker.dart:336: // Find up the element type
of the sequence.
s/up//

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/checker.dart:347: if (iterableType.isDynamic) {
Should we just do checkAssignment(node.iterable, sequenceInterface)?  That will
cover the case where sequence has type Object as well.  The previous code
allowed that.

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/checker_test.dart (right):

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:106: for (int
/*info:DOWN_CAST_IMPLICIT*/i in <num>[]) {}
nice!

[Bob Nystrom, 2016-03-21 22:44:55]

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong/checker.dart (right):

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/checker.dart:336: // Find up the element type
of the sequence.
On 2016/03/21 21:25:20, vsm wrote:
> s/up//

Done.

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/checker.dart:347: if (iterableType.isDynamic) {
On 2016/03/21 21:25:20, vsm wrote:
> Should we just do checkAssignment(node.iterable, sequenceInterface)?  That
will
> cover the case where sequence has type Object as well.  The previous code
> allowed that.

I don't think we want that because we don't want a cast on the iterable itself
in cases like:

for (int i in <num>[]) { ... }

Casting <num>[] to Iterable<int> is redundant since we'll be adding a num->int
cast to i itself. Instead, we just want to do an isSubtypeOf() check to see if
the type is a supertype of Iterable. Did that and added some tests.

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/checker_test.dart (right):

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:106: for (int
/*info:DOWN_CAST_IMPLICIT*/i in <num>[]) {}
On 2016/03/21 21:25:20, vsm wrote:
> nice!

\o/

[vsm, 2016-03-21 22:50:36]

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong/checker.dart (right):

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/checker.dart:347: if (iterableType.isDynamic) {
On 2016/03/21 22:44:55, Bob Nystrom wrote:
> On 2016/03/21 21:25:20, vsm wrote:
> > Should we just do checkAssignment(node.iterable, sequenceInterface)?  That
> will
> > cover the case where sequence has type Object as well.  The previous code
> > allowed that.
> 
> I don't think we want that because we don't want a cast on the iterable itself
> in cases like:
> 
> for (int i in <num>[]) { ... }
> 
> Casting <num>[] to Iterable<int> is redundant since we'll be adding a num->int
> cast to i itself. Instead, we just want to do an isSubtypeOf() check to see if
> the type is a supertype of Iterable. Did that and added some tests.

It'd be a cast to Iterable, not Iterable<int> - i.e., using sequenceType
(instantiated with dynamic is that's needed here).

[Bob Nystrom, 2016-03-21 23:00:44]

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong/checker.dart (right):

https://codereview.chromium.org/1823793002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/checker.dart:347: if (iterableType.isDynamic) {
On 2016/03/21 22:50:35, vsm wrote:
> On 2016/03/21 22:44:55, Bob Nystrom wrote:
> > On 2016/03/21 21:25:20, vsm wrote:
> > > Should we just do checkAssignment(node.iterable, sequenceInterface)?  That
> > will
> > > cover the case where sequence has type Object as well.  The previous code
> > > allowed that.
> > 
> > I don't think we want that because we don't want a cast on the iterable
itself
> > in cases like:
> > 
> > for (int i in <num>[]) { ... }
> > 
> > Casting <num>[] to Iterable<int> is redundant since we'll be adding a
num->int
> > cast to i itself. Instead, we just want to do an isSubtypeOf() check to see
if
> > the type is a supertype of Iterable. Did that and added some tests.
> 
> It'd be a cast to Iterable, not Iterable<int> - i.e., using sequenceType
> (instantiated with dynamic is that's needed here).

OK, then we're thinking the same thing. That's what I did in the revised code. I
just didn't go through checkAssignment() because that will also report an error,
which isn't what we want here.

[vsm, 2016-03-21 23:06:08]

lgtm

Might make sense to modify checkAssignment to (optionally?) suppress the error
message.

[Bob Nystrom, 2016-03-21 23:14:23]

On 2016/03/21 23:06:08, vsm wrote:
> lgtm
> 
> Might make sense to modify checkAssignment to (optionally?) suppress the error
> message.

checkAssignment() just forwards to _checkDowncast() these days, and the latter
doesn't do much except for report an error. I could be wrong, but I don't think
there's much useful code to reuse here.

[vsm, 2016-03-21 23:30:01]

On 2016/03/21 23:14:23, Bob Nystrom wrote:
> On 2016/03/21 23:06:08, vsm wrote:
> > lgtm
> > 
> > Might make sense to modify checkAssignment to (optionally?) suppress the
error
> > message.
> 
> checkAssignment() just forwards to _checkDowncast() these days, and the latter
> doesn't do much except for report an error. I could be wrong, but I don't
think
> there's much useful code to reuse here.

ok, sounds good.

still lgtm.

[Bob Nystrom, 2016-03-21 23:37:32]

Description was changed from

==========
Better casting of for-in loops.

Generate a cast for each element, so that code like this can be
handled correctly:

  for (String s in <dynamic>["a", "b", "c"]) { ... }

Fixes #25916.
==========

to

==========
Better casting of for-in loops.

Generate a cast for each element, so that code like this can be
handled correctly:

  for (String s in <dynamic>["a", "b", "c"]) { ... }

Fixes #25916.

R=vsm@google.com

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

[Bob Nystrom, 2016-03-21 23:37:34]

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