Strong mode foreach inference. Infer the type of declared identifiers

Strong mode foreach inference.  Infer the type of declared identifiers
in foreach blocks.

BUG=
R=brianwilkerson@google.com, paulberry@google.com

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

[Leaf, 2015-08-28 20:32:30]

leafp@google.com changed reviewers:
+ brianwilkerson@google.com, paulberry@google.com

[Paul Berry, 2015-08-28 20:51:10]

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

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1243: var parent =
node.parent as ForEachStatement;
Isn't this going to blow up when visiting declared identifiers that don't appear
in ForEach statements?  Maybe I'm missing something.

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1248: var iteratedType
= _findIteratedType(exprType);
We need to also check whether this is an "await for" statement, in which case
type inference is different because the type of exprType is expected to be a
Stream<T>.

[Brian Wilkerson, 2015-08-28 20:59:33]

LGTM. Your call on whether to address comments in this CL or a follow-on CL.

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

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1218: var typeArguments
= type.typeArguments;
nit: in the 'analyzer' package, we always provide a type annotation, including
'void' on methods.

(We also keep our methods sorted, but that's going to be hard for you to do
unless you're using a tool that supports it.)

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1248: var iteratedType
= _findIteratedType(exprType);
This doesn't support the 'await for' case (see
ResolverVisitor.visitForEachStatementInScope).

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/resolver_test.dart:13762: code,
typeProvider.intType, typeProvider.intType);
I might be misreading the test, but it looks like we're expecting 'v' to have a
propagated type that is equal to the static type. I don't know that it matters
in practice, but we generally only keep a propagated type if it's more specific
than the static type. I'd like to understand why the propagated type isn't null
here.

The propagated type is computed in ResolverVisitor.visitForEachStatementInScope.
I'm guessing that we just need to disable that code when strong mode is enabled.

[Leaf, 2015-09-01 21:21:02]

PTAL.

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

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1218: var typeArguments
= type.typeArguments;
On 2015/08/28 20:59:33, Brian Wilkerson wrote:
> nit: in the 'analyzer' package, we always provide a type annotation, including
> 'void' on methods.
> 
> (We also keep our methods sorted, but that's going to be hard for you to do
> unless you're using a tool that supports it.)

Done.  What tool do you use?  I poked around Intellij a bit, but couldn't find
anything obvious.

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1243: var parent =
node.parent as ForEachStatement;
On 2015/08/28 20:51:10, Paul Berry wrote:
> Isn't this going to blow up when visiting declared identifiers that don't
appear
> in ForEach statements?  Maybe I'm missing something.

As far as I can tell, the only place that the analyzer ast generates
DeclaredIdentifiers is in foreach statements, but I've added a guard just to be
safe.

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1248: var iteratedType
= _findIteratedType(exprType);
On 2015/08/28 20:59:33, Brian Wilkerson wrote:
> This doesn't support the 'await for' case (see
> ResolverVisitor.visitForEachStatementInScope).

Done.

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/resolver_test.dart:13762: code,
typeProvider.intType, typeProvider.intType);
On 2015/08/28 20:59:33, Brian Wilkerson wrote:
> I might be misreading the test, but it looks like we're expecting 'v' to have
a
> propagated type that is equal to the static type. I don't know that it matters
> in practice, but we generally only keep a propagated type if it's more
specific
> than the static type. I'd like to understand why the propagated type isn't
null
> here.
> 
> The propagated type is computed in
ResolverVisitor.visitForEachStatementInScope.
> I'm guessing that we just need to disable that code when strong mode is
enabled.

I changed visitForEachStatementInScope as discussed to only record the
propagated type if it is better.

[Paul Berry, 2015-09-01 21:34:01]

lgtm assuming null-aware operator issue is addressed.

https://codereview.chromium.org/1323513005/diff/60001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1323513005/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1421: return
_find(type.superclass) ??
Unfortunately, we can't use null-aware operators in analyzer yet, because there
are still people making use of analyzer that haven't upgraded to Dart 1.12.0.

[Brian Wilkerson, 2015-09-01 21:37:29]

LGTM

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

https://codereview.chromium.org/1323513005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1243: var parent =
node.parent as ForEachStatement;
> As far as I can tell, the only place that the analyzer ast generates
> DeclaredIdentifiers is in foreach statements, but I've added a guard just to
be
> safe.

True, but we might re-use the class if the syntax of the language changes.

https://codereview.chromium.org/1323513005/diff/60001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1323513005/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1421: return
_find(type.superclass) ??
Unless we update our SDK dependencies (in puspec), we can't use null-aware
operators yet.

https://codereview.chromium.org/1323513005/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1663: DartType
iteratedType = _findIteratedType(exprType, targetType);
Add a TODO comment or implement the 'async' case.

[Leaf, 2015-09-01 23:11:32]

PTAL.  Eliminated ??, added test for "await for", and made _findIteratedType
defensive against cycles in the type hierarchy.

https://codereview.chromium.org/1323513005/diff/60001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1323513005/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1421: return
_find(type.superclass) ??
On 2015/09/01 21:34:00, Paul Berry wrote:
> Unfortunately, we can't use null-aware operators in analyzer yet, because
there
> are still people making use of analyzer that haven't upgraded to Dart 1.12.0.

Done.

https://codereview.chromium.org/1323513005/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1663: DartType
iteratedType = _findIteratedType(exprType, targetType);
On 2015/09/01 21:37:29, Brian Wilkerson wrote:
> Add a TODO comment or implement the 'async' case.

This is handled by the choice of targetType above.  Added a test for this.

[Paul Berry, 2015-09-01 23:20:20]

https://codereview.chromium.org/1323513005/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1323513005/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1403: HashSet<DartType>
visitedTypes = new HashSet<DartType>();
visitedTypes should be a set of class elements, not a set of types.  Otherwise
we would go into an infinite loop with code like:

class A<T> extends A<List<T>> {}

See for example how this is handled in InterfaceTypeImpl.isMoreSpecificThan()

[Brian Wilkerson, 2015-09-01 23:21:33]

LGTM

[Leaf, 2015-09-01 23:50:25]

One more once?  Changed cycle detection over to use class elements.

https://codereview.chromium.org/1323513005/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1323513005/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1403: HashSet<DartType>
visitedTypes = new HashSet<DartType>();
On 2015/09/01 23:20:19, Paul Berry wrote:
> visitedTypes should be a set of class elements, not a set of types.  Otherwise
> we would go into an infinite loop with code like:
> 
> class A<T> extends A<List<T>> {}
> 
> See for example how this is handled in InterfaceTypeImpl.isMoreSpecificThan()

Done.

[Paul Berry, 2015-09-02 00:04:04]

lgtm

[Leaf, 2015-09-02 00:38:05]

Committed patchset #6 (id:100001) manually as
f90c09df4822f95bfd4c4e239bd0f139d8c31457 (presubmit successful).