Downwards closure inference

Downwards closure inference.  This implements a basic form of downwards closure inference.  It only handles expression functions (=> e), and it does not try to narrow the type of parameters.

BUG=
R=vsm@google.com

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

[Leaf, 2015-03-31 23:41:25]

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

[Leaf, 2015-03-31 23:41:34]

https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
File test/checker/inferred_type_test.dart (right):

https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
test/checker/inferred_type_test.dart:1243: String mk(String info) => '''
Renamed this to info, as suggest in comments to an earlier CL

https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
test/checker/inferred_type_test.dart:1344: String mk(String info) => '''
Moved the code into main with scoping, since the variable reuse was bogus at the
top level.

[vsm, 2015-04-01 00:14:19]

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

https://codereview.chromium.org/1050703002/diff/1/lib/src/checker/rules.dart#...
lib/src/checker/rules.dart:730: if (returnT.isDynamic) return false;
Why do we disallow this?

In this case,

typedef Foo(int x);

Foo f = (x) => x;

should we infer the closure as int->int?

https://codereview.chromium.org/1050703002/diff/1/lib/src/checker/rules.dart#...
lib/src/checker/rules.dart:747: return rules.isSubTypeOf(e.staticType, t);
Should we have changed e.element.returnType if the inference failed?

https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
File test/checker/inferred_type_test.dart (right):

https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
test/checker/inferred_type_test.dart:1491: Function2<int, String> l2 =
/*severe:StaticTypeError*/(x) => 3;
What would we do with:

Function2<int, int> l4 = (x) => x;

and 

Function2<int, String> l5 = (x) => x; // error?

Or:

Function2<int, String> l6 = (x) => x.substring(3); // error?

https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
test/checker/inferred_type_test.dart:1503: }, inferDownwards: true);
Perhaps turn closure wrapping off for this test?

https://codereview.chromium.org/1050703002/diff/1/test/codegen/expect/sunflow...
File test/codegen/expect/sunflower/sunflower.js (right):

https://codereview.chromium.org/1050703002/diff/1/test/codegen/expect/sunflow...
test/codegen/expect/sunflower/sunflower.js:32:
exports.slider.addEventListener('change', dart.as((e) => draw(),
dom.EventListener));
Did we infer this?  We shouldn't need a cast at runtime, right?

[vsm, 2015-04-01 17:34:00]

On 2015/04/01 00:14:19, vsm wrote:
> https://codereview.chromium.org/1050703002/diff/1/lib/src/checker/rules.dart
> File lib/src/checker/rules.dart (right):
> 
>
https://codereview.chromium.org/1050703002/diff/1/lib/src/checker/rules.dart#...
> lib/src/checker/rules.dart:730: if (returnT.isDynamic) return false;
> Why do we disallow this?
> 
> In this case,
> 
> typedef Foo(int x);
> 
> Foo f = (x) => x;
> 
> should we infer the closure as int->int?
> 
>
https://codereview.chromium.org/1050703002/diff/1/lib/src/checker/rules.dart#...
> lib/src/checker/rules.dart:747: return rules.isSubTypeOf(e.staticType, t);
> Should we have changed e.element.returnType if the inference failed?
> 
>
https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
> File test/checker/inferred_type_test.dart (right):
> 
>
https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
> test/checker/inferred_type_test.dart:1491: Function2<int, String> l2 =
> /*severe:StaticTypeError*/(x) => 3;
> What would we do with:
> 
> Function2<int, int> l4 = (x) => x;
> 
> and 
> 
> Function2<int, String> l5 = (x) => x; // error?
> 
> Or:
> 
> Function2<int, String> l6 = (x) => x.substring(3); // error?
> 
>
https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
> test/checker/inferred_type_test.dart:1503: }, inferDownwards: true);
> Perhaps turn closure wrapping off for this test?
> 
>
https://codereview.chromium.org/1050703002/diff/1/test/codegen/expect/sunflow...
> File test/codegen/expect/sunflower/sunflower.js (right):
> 
>
https://codereview.chromium.org/1050703002/diff/1/test/codegen/expect/sunflow...
> test/codegen/expect/sunflower/sunflower.js:32:
> exports.slider.addEventListener('change', dart.as((e) => draw(),
> dom.EventListener));
> Did we infer this?  We shouldn't need a cast at runtime, right?

Some cases on Angular this doesn't appear to infer (but should?):

warning: line 569, column 14 of package:path/src/context.dart:
[InferableClosure] (arg) => arg == null ? "null" : '"$arg"' ((dynamic) → String)
will need runtime check to cast to type (String) → dynamic
        .map((arg) => arg == null ? "null" : '"$arg"')

warning: line 109, column 22 of
package:stack_trace/src/stack_zone_specification.dart: [InferableClosure]
(error, stackTrace, sink) {if (stackTrace == null) stackTrace = new
Trace.current(); if (stackTrace is! Chain && _chains[stackTrace] == null)
{_chains[stackTrace] = node;} sink.addError(error, stackTrace);} ((dynamic,
dynamic, dynamic) → dynamic) will need runtime check to cast to type (Object,
StackTrace, EventSink<dynamic>) → void
        handleError: (error, stackTrace, sink) {
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: line 131, column 37 of package:stack_trace/src/chain.dart:
[InferableClosure] (trace) => new Trace.parseFriendly(trace) ((dynamic) → Trace)
will need runtime check to cast to type (String) → dynamic
    new Chain(chain.split(_gap).map((trace) => new Trace.parseFriendly(trace)));
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: line 158, column 9 of package:stack_trace/src/chain.dart:
[InferableClosure] (trace) => trace.foldFrames(predicate, terse: terse)
((dynamic) → dynamic) will need runtime check to cast to type (Trace) → dynamic
        (trace) => trace.foldFrames(predicate, terse: terse));
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Leaf, 2015-04-03 00:51:06]

> Some cases on Angular this doesn't appear to infer (but should?):
> 
> warning: line 569, column 14 of package:path/src/context.dart:
> [InferableClosure] (arg) => arg == null ? "null" : '"$arg"' ((dynamic) →
String)
> will need runtime check to cast to type (String) → dynamic
>         .map((arg) => arg == null ? "null" : '"$arg"')

This is a side effect of the fuzzy types  Top -> String <: String -> Top, but
Bot -> String </: String -> Top.

Inference on the return type can't help here - it's already more specific than
the context requires.

Inference on the argument type would help, not doing it yet.

Treating literals as having the non-fuzzy arrow type would fix this as well.

> 
> warning: line 109, column 22 of
> package:stack_trace/src/stack_zone_specification.dart: [InferableClosure]
> (error, stackTrace, sink) {if (stackTrace == null) stackTrace = new
> Trace.current(); if (stackTrace is! Chain && _chains[stackTrace] == null)
> {_chains[stackTrace] = node;} sink.addError(error, stackTrace);} ((dynamic,
> dynamic, dynamic) → dynamic) will need runtime check to cast to type (Object,
> StackTrace, EventSink<dynamic>) → void
>         handleError: (error, stackTrace, sink) {
>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Fuzzy types again, and also, I don't handle block functions yet.

> warning: line 131, column 37 of package:stack_trace/src/chain.dart:
> [InferableClosure] (trace) => new Trace.parseFriendly(trace) ((dynamic) →
Trace)
> will need runtime check to cast to type (String) → dynamic
>     new Chain(chain.split(_gap).map((trace) => new
Trace.parseFriendly(trace)));

Same story as the first one.  

>                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> warning: line 158, column 9 of package:stack_trace/src/chain.dart:
> [InferableClosure] (trace) => trace.foldFrames(predicate, terse: terse)
> ((dynamic) → dynamic) will need runtime check to cast to type (Trace) →
dynamic
>         (trace) => trace.foldFrames(predicate, terse: terse));
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Again.

[Leaf, 2015-04-03 00:51:55]

PTAL.

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

https://codereview.chromium.org/1050703002/diff/1/lib/src/checker/rules.dart#...
lib/src/checker/rules.dart:730: if (returnT.isDynamic) return false;
On 2015/04/01 00:14:19, vsm wrote:
> Why do we disallow this?
> 
> In this case,
> 
> typedef Foo(int x);
> 
> Foo f = (x) => x;
> 
> should we infer the closure as int->int?

Eventually, yes.  As it stands, no.  I don't improve on the argument types of
the closures, since doing so requires fully re-checking the body.  

So if returnT is dynamic, inference (in this limited implementation) cannot
improve the type.

https://codereview.chromium.org/1050703002/diff/1/lib/src/checker/rules.dart#...
lib/src/checker/rules.dart:747: return rules.isSubTypeOf(e.staticType, t);
On 2015/04/01 00:14:19, vsm wrote:
> Should we have changed e.element.returnType if the inference failed?

Probably not.  Probably doesn't matter, but still not a good idea.  Fixed.

https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
File test/checker/inferred_type_test.dart (right):

https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
test/checker/inferred_type_test.dart:1491: Function2<int, String> l2 =
/*severe:StaticTypeError*/(x) => 3;
On 2015/04/01 00:14:19, vsm wrote:
> What would we do with:
> 
> Function2<int, int> l4 = (x) => x;
> 
> and 
> 
> Function2<int, String> l5 = (x) => x; // error?
> 
> Or:
> 
> Function2<int, String> l6 = (x) => x.substring(3); // error?

The first will be inferred once we push types to the argument as well.  The
latter two should be errors (currently they will show up as Inferable).

Added these to the test.

https://codereview.chromium.org/1050703002/diff/1/test/checker/inferred_type_...
test/checker/inferred_type_test.dart:1503: }, inferDownwards: true);
On 2015/04/01 00:14:19, vsm wrote:
> Perhaps turn closure wrapping off for this test?

Done.

https://codereview.chromium.org/1050703002/diff/1/test/codegen/expect/sunflow...
File test/codegen/expect/sunflower/sunflower.js (right):

https://codereview.chromium.org/1050703002/diff/1/test/codegen/expect/sunflow...
test/codegen/expect/sunflower/sunflower.js:32:
exports.slider.addEventListener('change', dart.as((e) => draw(),
dom.EventListener));
On 2015/04/01 00:14:19, vsm wrote:
> Did we infer this?  We shouldn't need a cast at runtime, right?

The JS codegen currently turns most conversions (including these inferred type
ones) into casts.

[vsm, 2015-04-03 12:35:58]

lgtm

[Leaf, 2015-04-03 16:17:35]

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