Downward inference

Downward inference

This is a first cut at downwards inference. The interaction with InferableLiteral etc isn't well sorted out yet, so while downwards inference on nested expressions works, there will currently be spurious warnings.

BUG=
R=vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/9cd58524e96dfe74b6779949f8979f83a429a48e

[Leaf, 2015-03-27 00:26:54]

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

[vsm, 2015-03-27 18:35:35]

very cool!  first set of comments.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:565: ////////// Called for each map literal which
gets inferred /////
"map literal" -> "new / const expression"

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:571: if (e is Conversion) return
inferExpression(e.node, t);
Should we really be inferring into a Conversion?  t may be weaker than e.toT. 
E.g.,:

dynamic x;
List<List<num>> l = [[1,2,3], <int>[x]];

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:575: if (e is NamedExpression) return
_inferNamedExpression(e, t);
Not sure I understand what inference we need here.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:581: // Assume t1 <: t2, and t1 = I<dynamic, ...,
dynamic>
Should I interpret dynamic as bottom here?  We typically do not for generics.

[vsm, 2015-03-27 21:20:59]

Another round.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:611: // two (if it exists).
I wonder if we get enough utility out of allowing general permutations.  If the
type parameters don't trivially line up, we could just return false.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:678: // TODO(leafp): generalize to handle named
parameters
s/named/optional/ ?

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:698: fType.namedParameterTypes.length > 0)) return
false;
This should be fType.optionalParameterTypes here I think.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/codegen/reify_c...
File lib/src/codegen/reify_coercions.dart (right):

https://codereview.chromium.org/1038213003/diff/20001/lib/src/codegen/reify_c...
lib/src/codegen/reify_coercions.dart:64: cName.type = typeName;
Does e.staticType require updating?

https://codereview.chromium.org/1038213003/diff/20001/lib/src/info.dart
File lib/src/info.dart (right):

https://codereview.chromium.org/1038213003/diff/20001/lib/src/info.dart#newco...
lib/src/info.dart:416: class InferredTypeExact extends InferredTypeBase {
Perhaps call this InferredTypeAllocation for clarity / consistency.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/options.dart
File lib/src/options.dart (right):

https://codereview.chromium.org/1038213003/diff/20001/lib/src/options.dart#ne...
lib/src/options.dart:320: help: 'Infer types downwards from local context',
defaultsTo: true)
s/true/RulesOptions.inferDownwardsDefault/

https://codereview.chromium.org/1038213003/diff/20001/test/checker/inferred_t...
File test/checker/inferred_type_test.dart (right):

https://codereview.chromium.org/1038213003/diff/20001/test/checker/inferred_t...
test/checker/inferred_type_test.dart:1184: String mk(String error) => '''
minor nit: error->info since it's not an error in the default case

https://codereview.chromium.org/1038213003/diff/20001/test/checker/inferred_t...
test/checker/inferred_type_test.dart:1276: List<int> l3 =
/*warning:InferableLiteral*/["hello", 3];
Perhaps some tests with variables / expressions inside the literal?

[Leaf, 2015-03-30 23:26:00]

Implemented support for named args on constructors, addressed comments.  PTAL.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:565: ////////// Called for each map literal which
gets inferred /////
On 2015/03/27 18:35:35, vsm wrote:
> "map literal" -> "new / const expression"

Done.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:571: if (e is Conversion) return
inferExpression(e.node, t);
On 2015/03/27 18:35:35, vsm wrote:
> Should we really be inferring into a Conversion?  t may be weaker than e.toT. 
> E.g.,:
> 
> dynamic x;
> List<List<num>> l = [[1,2,3], <int>[x]];

This shouldn't happen during type checking as it stands.  This code is re-run
during coercion reification though, to recreate the appropriate types, and in
that case we want to just ignore conversions.  This should go away when I
integrate this code more closely into the checker.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:575: if (e is NamedExpression) return
_inferNamedExpression(e, t);
On 2015/03/27 18:35:35, vsm wrote:
> Not sure I understand what inference we need here.

If you have a named expression (in principle anywhere, in practice a named
argument) then you need to push the type down through the named expression to
the underlying value.  Example:

class A<T> {
  List<T> x;
  A({this.x});
}

List<int> = new A(x: [3]);

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:581: // Assume t1 <: t2, and t1 = I<dynamic, ...,
dynamic>
On 2015/03/27 18:35:35, vsm wrote:
> Should I interpret dynamic as bottom here?  We typically do not for generics.

Poorly worded comment, rephrased.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:611: // two (if it exists).
On 2015/03/27 21:20:59, vsm wrote:
> I wonder if we get enough utility out of allowing general permutations.  If
the
> type parameters don't trivially line up, we could just return false.

There are examples where we benefit from at least allowing sub-setting: see for
example line 212 here
https://codereview.chromium.org/1038213003/diff/40001/test/dart_codegen/expec...
. 

I kind of feel like permutation is worth supporting - it seems surprising if you
happen to permute type arguments in a subclass and suddenly don't get inference.

We probably don't get much out of allowing duplication, but it doesn't add much
complexity.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:678: // TODO(leafp): generalize to handle named
parameters
On 2015/03/27 21:20:59, vsm wrote:
> s/named/optional/ ?

Went ahead and implemented.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:698: fType.namedParameterTypes.length > 0)) return
false;
On 2015/03/27 21:20:59, vsm wrote:
> This should be fType.optionalParameterTypes here I think.

Done.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/codegen/reify_c...
File lib/src/codegen/reify_coercions.dart (right):

https://codereview.chromium.org/1038213003/diff/20001/lib/src/codegen/reify_c...
lib/src/codegen/reify_coercions.dart:64: cName.type = typeName;
On 2015/03/27 21:20:59, vsm wrote:
> Does e.staticType require updating?

Done.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/info.dart
File lib/src/info.dart (right):

https://codereview.chromium.org/1038213003/diff/20001/lib/src/info.dart#newco...
lib/src/info.dart:416: class InferredTypeExact extends InferredTypeBase {
On 2015/03/27 21:20:59, vsm wrote:
> Perhaps call this InferredTypeAllocation for clarity / consistency.

Done.

https://codereview.chromium.org/1038213003/diff/20001/lib/src/options.dart
File lib/src/options.dart (right):

https://codereview.chromium.org/1038213003/diff/20001/lib/src/options.dart#ne...
lib/src/options.dart:320: help: 'Infer types downwards from local context',
defaultsTo: true)
On 2015/03/27 21:20:59, vsm wrote:
> s/true/RulesOptions.inferDownwardsDefault/

Done.

[vsm, 2015-03-31 17:11:28]

lgtm

https://codereview.chromium.org/1038213003/diff/60001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1038213003/diff/60001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:559: ////////// Called for each list literal which
gets inferred /////
Nit: dart style is '///' as prefix:
https://www.dartlang.org/articles/doc-comment-guidelines/

https://codereview.chromium.org/1038213003/diff/60001/test/checker/inferred_t...
File test/checker/inferred_type_test.dart (right):

https://codereview.chromium.org/1038213003/diff/60001/test/checker/inferred_t...
test/checker/inferred_type_test.dart:1246: A<int, String> a4 =
/*warning:InferableAllocation*/new A<int, dynamic>(3, "hello");
" ... should be severe:StaticTypeError" here and below?

[Leaf, 2015-03-31 20:24:30]

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

[Leaf, 2015-04-03 19:24:52]

Looks like I forgot to publish these comment replies.

https://codereview.chromium.org/1038213003/diff/60001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1038213003/diff/60001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:559: ////////// Called for each list literal which
gets inferred /////
On 2015/03/31 17:11:28, vsm wrote:
> Nit: dart style is '///' as prefix:
> https://www.dartlang.org/articles/doc-comment-guidelines/

Done.

https://codereview.chromium.org/1038213003/diff/60001/test/checker/inferred_t...
File test/checker/inferred_type_test.dart (right):

https://codereview.chromium.org/1038213003/diff/60001/test/checker/inferred_t...
test/checker/inferred_type_test.dart:1246: A<int, String> a4 =
/*warning:InferableAllocation*/new A<int, dynamic>(3, "hello");
On 2015/03/31 17:11:28, vsm wrote:
> " ... should be severe:StaticTypeError" here and below?

Yes.  I was leaving this to address separately, but really no reason to delay. 
I've now made everything that would have been an InferableLiteral or
InferableAllocation into a StaticTypeError