fix #25487, infer block lambdas from return statements

fix #25487, infer block lambdas from return statements

R=leafp@google.com

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

[Jennifer Messerly, 2016-02-22 21:18:13]

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

[Leaf, 2016-02-23 02:17:47]

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:8496:
inferenceContext.addReturnType(e.staticType);
As it stands, I think that this will do the right thing for async functions that
return only futures, or return only values.  For async functions returning
Future<T> and T, I think it does a safe thing (Future<dynamic>) but I think we
could handle this case and return Future<T>.  Maybe a TODO?

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:8619: Object
visitYieldStatement(YieldStatement node) {
It seems like there's an analogous inference for yields.  Maybe a TODO?

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

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:472: DartType
inferredType = InferenceContext.getType(body);
It bothers me a little bit that this segment of code partially overlaps with the
original downwards inference code below.  I think (but could be wrong) that
these two can be folded together.  

This code path here will fire if we were doing downwards inference on a block
function body, or if we successfully did upwards inference.

The code path below will fire if we were doing downwards inference (for either
kind of body).  I believe that the second code path is entirely redundant in the
downwards inference case.  This seems like an infelicity in the existing code -
it basically redoes _resolver.matchFunctionTypeParameters to compute the
function type.  This should, I think, succeed iff the call to it in
resolver.visitFunctionExpression succeeded, and if that call succeeds, then we
should have that returnType == _computeReturnTypeOfFunction(body,
getType(body)).

So it really seems like we ought to be able to just fold these two together, and
do something uniform whenever we there is a type recorded on the body.  Does
this make sense?

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/test/src/t...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (right):

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1944: }
I'd like to be sure we cover this pretty well.  Can you please add tests for:

1) Inferred type is top
2) Inferred type is bottom
3) return null;
4) async functions, returning:
  a) All futures
  b) All values
  c) Futures and values
5) A sync* and async* example
6) An example where the downwards inference type is incompatible with the
upwards inference type.
7) Nested lambdas.

[Jennifer Messerly, 2016-02-23 17:41:46]

Wonderful comments! I'm going to get started on them. Here's my thoughts on how
to proceed, let me know if any of it sounds wrong, otherwise I'll get back to
you once I have the code ready :)

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:8496:
inferenceContext.addReturnType(e.staticType);
On 2016/02/23 02:17:47, Leaf wrote:
> As it stands, I think that this will do the right thing for async functions
that
> return only futures, or return only values.  For async functions returning
> Future<T> and T, I think it does a safe thing (Future<dynamic>) but I think we
> could handle this case and return Future<T>.  Maybe a TODO?

Nice catch. Yeah, it seems like we should interpret "T" here as "fatten(T)" in
an "async" body.

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:8619: Object
visitYieldStatement(YieldStatement node) {
On 2016/02/23 02:17:47, Leaf wrote:
> It seems like there's an analogous inference for yields.  Maybe a TODO?

Good catch. Completely forgot that sync*/async* could even be used in an
anonymous function. :)

I'm tempted to just handle it correctly. I think it should be about as much code
as writing the TODO.

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

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:472: DartType
inferredType = InferenceContext.getType(body);
On 2016/02/23 02:17:47, Leaf wrote:
> It bothers me a little bit that this segment of code partially overlaps with
the
> original downwards inference code below.  I think (but could be wrong) that
> these two can be folded together.  
> 
> This code path here will fire if we were doing downwards inference on a block
> function body, or if we successfully did upwards inference.
> 
> The code path below will fire if we were doing downwards inference (for either
> kind of body).  I believe that the second code path is entirely redundant in
the
> downwards inference case.  This seems like an infelicity in the existing code
-
> it basically redoes _resolver.matchFunctionTypeParameters to compute the
> function type.  This should, I think, succeed iff the call to it in
> resolver.visitFunctionExpression succeeded, and if that call succeeds, then we
> should have that returnType == _computeReturnTypeOfFunction(body,
> getType(body)).
> 
> So it really seems like we ought to be able to just fold these two together,
and
> do something uniform whenever we there is a type recorded on the body.  Does
> this make sense?

Yeah, makes sense. I was having the same thoughts but wasn't sure how much
refactoring to get into. It's funny that Resolver.visitFunctionExpression will
do almost the same logic as below, then call
"super.visitFunctionExpression(node);" which takes us right back into this
method (although we did visit children in the mean time, so it's not completely
useless). But yeah, there's got to be a way to simplify it. I'll play around
with it. I like your idea of just removing the redundant code below, that just
might work. :)

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/test/src/t...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (right):

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1944: }
On 2016/02/23 02:17:47, Leaf wrote:
> I'd like to be sure we cover this pretty well.  Can you please add tests for:
> 
> 1) Inferred type is top
> 2) Inferred type is bottom
> 3) return null;
> 4) async functions, returning:
>   a) All futures
>   b) All values
>   c) Futures and values
> 5) A sync* and async* example
> 6) An example where the downwards inference type is incompatible with the
> upwards inference type.
> 7) Nested lambdas.

Excellent list. Yeah I can do that.

[Jennifer Messerly, 2016-02-23 22:16:21]

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

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:472: DartType
inferredType = InferenceContext.getType(body);
On 2016/02/23 17:41:46, John Messerly wrote:
> On 2016/02/23 02:17:47, Leaf wrote:
> > It bothers me a little bit that this segment of code partially overlaps with
> the
> > original downwards inference code below.  I think (but could be wrong) that
> > these two can be folded together.  
> > 
> > This code path here will fire if we were doing downwards inference on a
block
> > function body, or if we successfully did upwards inference.
> > 
> > The code path below will fire if we were doing downwards inference (for
either
> > kind of body).  I believe that the second code path is entirely redundant in
> the
> > downwards inference case.  This seems like an infelicity in the existing
code
> -
> > it basically redoes _resolver.matchFunctionTypeParameters to compute the
> > function type.  This should, I think, succeed iff the call to it in
> > resolver.visitFunctionExpression succeeded, and if that call succeeds, then
we
> > should have that returnType == _computeReturnTypeOfFunction(body,
> > getType(body)).
> > 
> > So it really seems like we ought to be able to just fold these two together,
> and
> > do something uniform whenever we there is a type recorded on the body.  Does
> > this make sense?
> 
> Yeah, makes sense. I was having the same thoughts but wasn't sure how much
> refactoring to get into. It's funny that Resolver.visitFunctionExpression will
> do almost the same logic as below, then call
> "super.visitFunctionExpression(node);" which takes us right back into this
> method (although we did visit children in the mean time, so it's not
completely
> useless). But yeah, there's got to be a way to simplify it. I'll play around
> with it. I like your idea of just removing the redundant code below, that just
> might work. :)

Ah, I remembered one issue. This code isn't integrated with
_computeStaticReturnTypeOfFunctionExpression so we can record that inference
happened. I'll see about restructuring it.

[Jennifer Messerly, 2016-02-24 00:20:41]

Thanks Leaf, it's ready for re-review.

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:8496:
inferenceContext.addReturnType(e.staticType);
On 2016/02/23 17:41:46, John Messerly wrote:
> On 2016/02/23 02:17:47, Leaf wrote:
> > As it stands, I think that this will do the right thing for async functions
> that
> > return only futures, or return only values.  For async functions returning
> > Future<T> and T, I think it does a safe thing (Future<dynamic>) but I think
we
> > could handle this case and return Future<T>.  Maybe a TODO?
> 
> Nice catch. Yeah, it seems like we should interpret "T" here as "fatten(T)" in
> an "async" body.

Done.

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:8619: Object
visitYieldStatement(YieldStatement node) {
On 2016/02/23 17:41:46, John Messerly wrote:
> On 2016/02/23 02:17:47, Leaf wrote:
> > It seems like there's an analogous inference for yields.  Maybe a TODO?
> 
> Good catch. Completely forgot that sync*/async* could even be used in an
> anonymous function. :)
> 
> I'm tempted to just handle it correctly. I think it should be about as much
code
> as writing the TODO.

Done. This one was a little tricky, in particular we need some code similar to
flatten(T) for futures. I wasn't able to share quite as much as I would have
liked.

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

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:472: DartType
inferredType = InferenceContext.getType(body);
On 2016/02/23 22:16:21, John Messerly wrote:
> On 2016/02/23 17:41:46, John Messerly wrote:
> > On 2016/02/23 02:17:47, Leaf wrote:
> > > It bothers me a little bit that this segment of code partially overlaps
with
> > the
> > > original downwards inference code below.  I think (but could be wrong)
that
> > > these two can be folded together.  
> > > 
> > > This code path here will fire if we were doing downwards inference on a
> block
> > > function body, or if we successfully did upwards inference.
> > > 
> > > The code path below will fire if we were doing downwards inference (for
> either
> > > kind of body).  I believe that the second code path is entirely redundant
in
> > the
> > > downwards inference case.  This seems like an infelicity in the existing
> code
> > -
> > > it basically redoes _resolver.matchFunctionTypeParameters to compute the
> > > function type.  This should, I think, succeed iff the call to it in
> > > resolver.visitFunctionExpression succeeded, and if that call succeeds,
then
> we
> > > should have that returnType == _computeReturnTypeOfFunction(body,
> > > getType(body)).
> > > 
> > > So it really seems like we ought to be able to just fold these two
together,
> > and
> > > do something uniform whenever we there is a type recorded on the body. 
Does
> > > this make sense?
> > 
> > Yeah, makes sense. I was having the same thoughts but wasn't sure how much
> > refactoring to get into. It's funny that Resolver.visitFunctionExpression
will
> > do almost the same logic as below, then call
> > "super.visitFunctionExpression(node);" which takes us right back into this
> > method (although we did visit children in the mean time, so it's not
> completely
> > useless). But yeah, there's got to be a way to simplify it. I'll play around
> > with it. I like your idea of just removing the redundant code below, that
just
> > might work. :)
> 
> Ah, I remembered one issue. This code isn't integrated with
> _computeStaticReturnTypeOfFunctionExpression so we can record that inference
> happened. I'll see about restructuring it.

Done.

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/test/src/t...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (right):

https://codereview.chromium.org/1724543002/diff/40001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1944: }
On 2016/02/23 17:41:46, John Messerly wrote:
> On 2016/02/23 02:17:47, Leaf wrote:
> > I'd like to be sure we cover this pretty well.  Can you please add tests
for:
> > 
> > 1) Inferred type is top
> > 2) Inferred type is bottom
> > 3) return null;
> > 4) async functions, returning:
> >   a) All futures
> >   b) All values
> >   c) Futures and values
> > 5) A sync* and async* example
> > 6) An example where the downwards inference type is incompatible with the
> > upwards inference type.
> > 7) Nested lambdas.
> 
> Excellent list. Yeah I can do that.

Done.

[Leaf, 2016-02-24 15:38:40]

lgtm.  One comment, but fine with landing as is.  thanks!

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

https://codereview.chromium.org/1724543002/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:8718:
inferenceContext.matchTypes(rawType, declaredType);
I think this code should be changed to use your new _findImplementedTypeArgument
helper just as in visitYieldStatement above.  This arguably an abuse of
matchTypes here, or at least a larger hammer than is needed.  I'm pretty sure
that all this code is intended to do is what your new code does.  Also -
formatting?

https://codereview.chromium.org/1724543002/diff/80001/pkg/analyzer/test/src/t...
File pkg/analyzer/test/src/task/strong/inferred_type_test.dart (right):

https://codereview.chromium.org/1724543002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1925: 
Great tests, thanks!

[Jennifer Messerly, 2016-02-24 17:43:17]

Thanks again. Keen eyes as always!

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

https://codereview.chromium.org/1724543002/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:8718:
inferenceContext.matchTypes(rawType, declaredType);
On 2016/02/24 15:38:40, Leaf wrote:
> I think this code should be changed to use your new
_findImplementedTypeArgument
> helper just as in visitYieldStatement above.  This arguably an abuse of
> matchTypes here, or at least a larger hammer than is needed.  I'm pretty sure
> that all this code is intended to do is what your new code does.  Also -
> formatting?

Ah, thanks Leaf ... that makes so much more sense!

Originally I misunderstood what matchTypes does, and tried to reuse the code
here, factoring it out into its own method. But then it wouldn't handle a case
like:

    yield* [1, 2, 3] // List<int> matched against Iterable<> should give "int"

And then I looked into matchTypes and saw it was for the opposite kind of case,
when we're inferring a constructor from a context.

I'm not entirely sure that the new code would be right either -- it seems
potentially a bit too powerful as well. The new code is similar to flatten case
below for Futures, which I found has a bug:
https://github.com/dart-lang/sdk/issues/25854.

Based on that, I'm leaning towards looking into it in a follow up change.

But yeah I'll fix formatting. Funny that it helped us find some real problems in
the code. ;)

[Jennifer Messerly, 2016-02-24 18:00:00]

Description was changed from

==========
fix #25487, infer block lambdas from return statements
==========

to

==========
fix #25487, infer block lambdas from return statements

R=leafp@google.com

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

[Jennifer Messerly, 2016-02-24 18:00:06]

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