fix #28008, fix #28009 implement FutureOr<T>

fix #28008, fix #28009 implement FutureOr<T>

This implements FutureOr<T> in strong mode, otherwise it's ignored (treated as `dynamic`.

Also fixes strong mode's inference subtype function incorrectly treating `void` as a malformed type. This had the consequence of allowing `void` to be inferred as a type argument.

R=leafp@google.com, paulberry@google.com

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

[Paul Berry, 2017-01-23 16:42:14]

paulberry@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2017-01-23 16:42:15]

It looks like some summary work is required to support FutureOr<T>.  I'll work
on that and try to send you a CL ASAP that can be patched into this one.

[Paul Berry, 2017-01-23 19:31:46]

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

https://codereview.chromium.org/2647833002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:5268:
expect(y.type.toString(), 'void');
This change looks suspicious to me.  I didn't think it was supposed to be
allowed for a variable to have type `void`.  With this CL, this test fails when
invoked as part of summary logic, because the summary implementation of type
inference automatically coalesces any instance of `void` to `dynamic` (which I
still think is desirable behavior).

[Paul Berry, 2017-01-23 19:36:48]

On 2017/01/23 16:42:15, Paul Berry wrote:
> It looks like some summary work is required to support FutureOr<T>.  I'll work
> on that and try to send you a CL ASAP that can be patched into this one.

I believe this CL should be sufficient to add the necessary summary support:
https://codereview.chromium.org/2651773003/

[Jennifer Messerly, 2017-01-23 21:46:25]

(this change is not ready for review yet)

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

https://codereview.chromium.org/2647833002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:5268:
expect(y.type.toString(), 'void');
On 2017/01/23 19:31:46, Paul Berry wrote:
> This change looks suspicious to me.  I didn't think it was supposed to be
> allowed for a variable to have type `void`.  With this CL, this test fails
when
> invoked as part of summary logic, because the summary implementation of type
> inference automatically coalesces any instance of `void` to `dynamic` (which I
> still think is desirable behavior).

Yeah, this change is here for review to find out what the language team's
expectations are.

What happened: we had some broken subtyping logic for void that affect this
test. On fixing that logic, inferring a void typed variable became a
possibility. I don't know if that is desired or not. IIRC, we do desire void for
a type argument, but I don't know about an inferred variable type. So I may need
to figure out where to add code to handle that case, depending on what the
resolution of this is.

[Jennifer Messerly, 2017-01-23 23:59:56]

Description was changed from

==========
fix #28008, implement FutureOr<T> in Analyzer
also fixes #28009 for DDC
==========

to

==========
fix #28008, implement FutureOr<T> in Analyzer
also fixes #28009 for DDC
==========

[Jennifer Messerly, 2017-01-23 23:59:57]

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

[Jennifer Messerly, 2017-01-24 00:00:09]

Description was changed from

==========
fix #28008, implement FutureOr<T> in Analyzer
also fixes #28009 for DDC
==========

to

==========
fix #28008, implement FutureOr<T> in Analyzer, also fixes #28009 for DDC
==========

[Jennifer Messerly, 2017-01-24 00:03:23]

Description was changed from

==========
fix #28008, implement FutureOr<T> in Analyzer, also fixes #28009 for DDC
==========

to

==========
fix #28008, fix #28009 implement FutureOr<T>

This implements FutureOr<T> in strong mode, otherwise it's ignored (treated as
`dynamic`.

Also fixes strong mode's inference subtype function incorrectly treating `void`
as a malformed type. This had the consequence of allowing `void` to be inferred
as a type argument.
==========

[Jennifer Messerly, 2017-01-24 00:05:33]

Hi, this is ready for review.

After a chat with rnystrom@ and further investigation, I left in the change for
inferring void. Let me know about that. I can add code to inference to ignore
void.

It looks like we've always been able to infer `void` for variable types, FWIW.
The only change here is that generics can infer `void`. That change was the
result of a bug fix--we weren't skipping `void` for the right reason, we were
accidentally skipping it because we thought it was a malformed type (it lacks a
.element).

[Jennifer Messerly, 2017-01-24 01:03:47]

On 2017/01/24 00:05:33, Jennifer Messerly wrote:
> Hi, this is ready for review.
> 
> After a chat with rnystrom@ and further investigation, I left in the change
for
> inferring void. Let me know about that. I can add code to inference to ignore
> void.
> 
> It looks like we've always been able to infer `void` for variable types, FWIW.
> The only change here is that generics can infer `void`. That change was the
> result of a bug fix--we weren't skipping `void` for the right reason, we were
> accidentally skipping it because we thought it was a malformed type (it lacks
a
> .element).

Oh! I just noticed there are some language tests for FutureOr, they're passing
now. I will update the status file.

[Jennifer Messerly, 2017-01-24 01:21:22]

https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
File tests/lib/async/future_or_bad_type_test.dart (left):

https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
tests/lib/async/future_or_bad_type_test.dart:22: Expect.isTrue(499 is
FutureOr<A>);  /// 00: static type warning
I don't understand why this was expected to be a static warning. LMK if it makes
sense to you. I added another case that should be a warning.

[Paul Berry, 2017-01-24 13:55:42]

On 2017/01/24 00:05:33, Jennifer Messerly wrote:
> Hi, this is ready for review.
> 
> After a chat with rnystrom@ and further investigation, I left in the change
for
> inferring void. Let me know about that. I can add code to inference to ignore
> void.
> 
> It looks like we've always been able to infer `void` for variable types, FWIW.
> The only change here is that generics can infer `void`. That change was the
> result of a bug fix--we weren't skipping `void` for the right reason, we were
> accidentally skipping it because we thought it was a malformed type (it lacks
a
> .element).

Wow, you're right.  We have been getting this wrong in the summary type
inference logic for ages.  I'll send out a CL that fixes the summary inference
logic ASAP (you'll probably need it to land first in order to avoid test
failures with this CL).

[Paul Berry, 2017-01-24 17:45:13]

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1430: if (_strongMode
&& staticType.isObject) {
Why put this logic here and not in _typeSystem.getLeastUpperBound?  Putting it
here means that we'll have to replicate this logic in summary type inference;
putting it in _typeSystem.getLeastUpperBound would ensure that summary type
inference and non-summary type inference stay consistent.

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1535: if
(type.isDartAsyncFutureOr) {
I'm worried that this behavior hasn't been implemented in the summary type
inference logic yet.  But I don't recall seeing test failures.  Is this behavior
tested in pkg/analyzer/test/src/task/strong/inferred_type_test.dart?

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:834: // (Future<A> | A) <: t2
iff t2 <: Future<A> and t2 <: A.
I believe the code here is correct, but the comment doesn't match it.  Can we
please change it to say:

// given t1 is Future<A> | A, then:
// (Future<A> | A) <: t2 iff Future<A> <: t2 and A <: t2.

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/test/src/...
File pkg/analyzer/test/src/summary/resynthesize_ast_test.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/summary/resynthesize_ast_test.dart:706: void
test_voidReturnTypeSubtypesDynamic() =>
I'm guessing this override is no longer be necessary now that I've landed
https://codereview.chromium.org/2650943006/.

[Jennifer Messerly, 2017-01-24 18:58:20]

thanks! few thoughts

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1430: if (_strongMode
&& staticType.isObject) {
On 2017/01/24 17:45:13, Paul Berry wrote:
> Why put this logic here and not in _typeSystem.getLeastUpperBound?  Putting it
> here means that we'll have to replicate this logic in summary type inference;

yeah ... the problem tho is we have two implementations of resolution? I don't
know the best way to attack that. almost every type inference change hits it. I
would love to learn to fix the summary code, but I don't think it gets the same
coverage for strong mode (e.g. it doesn't seem to test that strong mode errors
were produced in inferred_type_test & checker_test?) ... and i don't really
understand how it works. Would be happy to discuss this larger issue though.

> putting it in _typeSystem.getLeastUpperBound would ensure that summary type
> inference and non-summary type inference stay consistent.

I don't know if we want to implement it in LUB for the language. The problem
with LUB is it's synthesizing types and used in many places.

Conditional expressions are a bit special, we've already checked the types for
both branches in strong mode, and we had a downwards context FutureOr<T>. So
this one is sound and matches the request in
https://github.com/dart-lang/sdk/issues/25368

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1535: if
(type.isDartAsyncFutureOr) {
On 2017/01/24 17:45:13, Paul Berry wrote:
> I'm worried that this behavior hasn't been implemented in the summary type
> inference logic yet. 

this is the same logic that was below, on line 2084 in the old code. It's just
moved here.
(_computeReturnTypeOfFunction only has one caller:
_inferLocalFunctionReturnType)

as to whether it was implemented previously in summary code, I don't know.

>  But I don't recall seeing test failures.  Is this behavior
> tested in pkg/analyzer/test/src/task/strong/inferred_type_test.dart?

yeah that's where most of its tests are

[Paul Berry, 2017-01-24 19:19:09]

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1430: if (_strongMode
&& staticType.isObject) {
On 2017/01/24 18:58:20, Jennifer Messerly wrote:
> On 2017/01/24 17:45:13, Paul Berry wrote:
> > Why put this logic here and not in _typeSystem.getLeastUpperBound?  Putting
it
> > here means that we'll have to replicate this logic in summary type
inference;
> 
> yeah ... the problem tho is we have two implementations of resolution? I don't
> know the best way to attack that.

Agreed.  It's really unfortunate that we have two implementations of resolution.
 As part of the front_end work, I am hoping to get us to the point where so much
code is shared that it's not really two implementations anymore, it's just two
slightly different code paths that lead to the same inference engine (one code
path for when we're inferring something that's visible as part of summaries, one
code path for when we're inferring a local variable).  But it's going to take
some time to get there.

> almost every type inference change hits it. I
> would love to learn to fix the summary code

I'm glad to help with this--the more people who understand the summary code the
better.

> but I don't think it gets the same
> coverage for strong mode (e.g. it doesn't seem to test that strong mode errors
> were produced in inferred_type_test & checker_test?) ...

The summary logic doesn't do error checking so there's no error checking to
validate.  What we do instead is we run each test in inferred_type_test through
both kinds of inference and verify that they produce the same set of types. 
IIRC, the reason checker_test isn't tested with summaries is that all of its
assertions are based on validating that the expected errors occur; since summary
logic doesn't do error checking, there's nothing to test.

If there's anything else missing from summary validation I'd love to talk about
it and get the test coverage up.

> and i don't really
> understand how it works. Would be happy to discuss this larger issue though.

That would be nice.  Let's schedule a meeting to talk about it.

> 
> > putting it in _typeSystem.getLeastUpperBound would ensure that summary type
> > inference and non-summary type inference stay consistent.
> 
> I don't know if we want to implement it in LUB for the language. The problem
> with LUB is it's synthesizing types and used in many places.

Ok, fair enough.  In that case, given the current architecture, I don't think we
can get around the need to duplicate this logic in the summary inference engine.

I'll look into doing that as soon as I can.

> 
> Conditional expressions are a bit special, we've already checked the types for
> both branches in strong mode, and we had a downwards context FutureOr<T>. So
> this one is sound and matches the request in
> https://github.com/dart-lang/sdk/issues/25368

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1535: if
(type.isDartAsyncFutureOr) {
On 2017/01/24 18:58:20, Jennifer Messerly wrote:
> On 2017/01/24 17:45:13, Paul Berry wrote:
> > I'm worried that this behavior hasn't been implemented in the summary type
> > inference logic yet. 
> 
> this is the same logic that was below, on line 2084 in the old code. It's just
> moved here.
> (_computeReturnTypeOfFunction only has one caller:
> _inferLocalFunctionReturnType)
> 
> as to whether it was implemented previously in summary code, I don't know.
> 
> >  But I don't recall seeing test failures.  Is this behavior
> > tested in pkg/analyzer/test/src/task/strong/inferred_type_test.dart?
> 
> yeah that's where most of its tests are

Ok, I didn't realize this was preexisting behavior.  In which case, if the
summary code isn't replicating this, it's a preexisting bug in summary code so
it shouldn't block this CL.  I'll investigate the summary code and follow up
with a CL (if necessary) as soon as I can.

[Paul Berry, 2017-01-24 20:14:50]

lgtm

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1430: if (_strongMode
&& staticType.isObject) {
On 2017/01/24 19:19:09, Paul Berry wrote:
> On 2017/01/24 18:58:20, Jennifer Messerly wrote:
> > On 2017/01/24 17:45:13, Paul Berry wrote:
> > > Why put this logic here and not in _typeSystem.getLeastUpperBound? 
Putting
> it
> > > here means that we'll have to replicate this logic in summary type
> inference;
> > 
> > yeah ... the problem tho is we have two implementations of resolution? I
don't
> > know the best way to attack that.
> 
> Agreed.  It's really unfortunate that we have two implementations of
resolution.
>  As part of the front_end work, I am hoping to get us to the point where so
much
> code is shared that it's not really two implementations anymore, it's just two
> slightly different code paths that lead to the same inference engine (one code
> path for when we're inferring something that's visible as part of summaries,
one
> code path for when we're inferring a local variable).  But it's going to take
> some time to get there.
> 
> > almost every type inference change hits it. I
> > would love to learn to fix the summary code
> 
> I'm glad to help with this--the more people who understand the summary code
the
> better.
> 
> > but I don't think it gets the same
> > coverage for strong mode (e.g. it doesn't seem to test that strong mode
errors
> > were produced in inferred_type_test & checker_test?) ...
> 
> The summary logic doesn't do error checking so there's no error checking to
> validate.  What we do instead is we run each test in inferred_type_test
through
> both kinds of inference and verify that they produce the same set of types. 
> IIRC, the reason checker_test isn't tested with summaries is that all of its
> assertions are based on validating that the expected errors occur; since
summary
> logic doesn't do error checking, there's nothing to test.
> 
> If there's anything else missing from summary validation I'd love to talk
about
> it and get the test coverage up.
> 
> > and i don't really
> > understand how it works. Would be happy to discuss this larger issue though.
> 
> That would be nice.  Let's schedule a meeting to talk about it.
> 
> > 
> > > putting it in _typeSystem.getLeastUpperBound would ensure that summary
type
> > > inference and non-summary type inference stay consistent.
> > 
> > I don't know if we want to implement it in LUB for the language. The problem
> > with LUB is it's synthesizing types and used in many places.
> 
> Ok, fair enough.  In that case, given the current architecture, I don't think
we
> can get around the need to duplicate this logic in the summary inference
engine.
> 
> I'll look into doing that as soon as I can.
> 
> > 
> > Conditional expressions are a bit special, we've already checked the types
for
> > both branches in strong mode, and we had a downwards context FutureOr<T>. So
> > this one is sound and matches the request in
> > https://github.com/dart-lang/sdk/issues/25368
> 

Ack!  I just re-read this and realized that it only applies in the case where
downward inference is in play (which never happens during summary inference). 
So forget everything I said about needing to duplicate this logic for summaries.
 Sorry for the noise.

[Leaf, 2017-01-24 21:56:27]

A few questions, but I think this is looking really good!

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6637: * If [type] is top (dynamic
or Object) then the resulting union type is
Can you give some examples of why you want to do this?  I don't immediately see
why it causes problems to have FutureOr<dynamic> floating around, and similarly
for FutureOr<Null>.

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6642: * For a similar reason
`Future<T> | Future<Future<T>>` is equivalent to just
I'd really like to avoid putting this in if at all possible.  What's the
motivation for doing this - it's not really part of the spec for this, and I
want to try to reduce our reliance on future flattening wherever possible.

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1429: // valid
FutureOr<T> as the downward context type for a conditional.
I'm not keen on putting this in just as a hack for FutureOr.  My take is that
this should be a separate feature, and should apply to all cases where we have a
downwards context.   Can we do this in a follow up CL, and make it apply to all
contexts?  This should also work for ?? and other related operators.

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:787: // The types are void,
dynamic, bottom, interface types, function types,
Update this comment?

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

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1737: var x =
f.then<Future<List<int>>>(/*info:INFERRED_TYPE_CLOSURE,error:ARGUMENT_TYPE_NOT_ASSIGNABLE*/(x)
=> []);
This is going to be a hard google3 roll.  We need to get the SDK updated to use
FutureOr, and then I'll need to prepare a CL that gets rid of all of the
.then<Future<T>> uses.  

The alternative would be to have one release which keeps future flattening here,
get rid of all of the uses of future flattening, then remove the flattening.

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:2504: => list.fold('',
/*info:INFERRED_TYPE_CLOSURE*/(x, y) =>
/*info:DYNAMIC_CAST,info:DYNAMIC_INVOKE*/x /*error:UNDEFINED_OPERATOR*/+
y.toString()));
What's going with this?  Why the regression?

https://codereview.chromium.org/2647833002/diff/120001/pkg/dev_compiler/tool/...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/dev_compiler/tool/...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:789: //
(Future<A> | A) <: t2 iff t2 <: Future<A> and t2 <: A.
should be Future<A> <: t2 and A <: t2

https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
File tests/lib/async/future_or_bad_type_test.dart (left):

https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
tests/lib/async/future_or_bad_type_test.dart:5: // In non strong-mode,
`FutureOr<T>` is dynamic, even if `T` doesn't exist.
There is a future_or_strong_tests.dart (or something similar) in this directory.
 It is not automatically run yet (I'm working on it), but I believe that you can
run it by hand.  It would be great to do so as a sanity check.

https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
tests/lib/async/future_or_bad_type_test.dart:22: Expect.isTrue(499 is
FutureOr<A>);  /// 00: static type warning
On 2017/01/24 01:21:22, Jennifer Messerly wrote:
> I don't understand why this was expected to be a static warning. LMK if it
makes
> sense to you. I added another case that should be a warning.

Can you ask Lasse what his intent was here?

[Jennifer Messerly, 2017-01-25 00:33:34]

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

[Jennifer Messerly, 2017-01-25 00:33:36]

Lasse, would you mind taking a look at the language test change? It's very
small.

Paul & Leaf: I think I got to all of your comments. Leaf PTAL and advise on the
FutureOr flattening question.

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6637: * If [type] is top (dynamic
or Object) then the resulting union type is
On 2017/01/24 21:56:26, Leaf wrote:
> Can you give some examples of why you want to do this?  I don't immediately
see
> why it causes problems to have FutureOr<dynamic> floating around, and
similarly
> for FutureOr<Null>.

This code already existed -- it was moved from "FutureUnionImpl.from". So, I'm
not really adding it. As much as I like to *fix-all-the-things* sometimes I
don't manage to clean up everything in one CL.

it looks like this function is only used for the downwards context of "await"
and "async" function's "return" statement.

I'll take a shot at removing this and/or simplifying

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:6642: * For a similar reason
`Future<T> | Future<Future<T>>` is equivalent to just
On 2017/01/24 21:56:26, Leaf wrote:
> I'd really like to avoid putting this in if at all possible.  What's the
> motivation for doing this - it's not really part of the spec for this, and I
> want to try to reduce our reliance on future flattening wherever possible.

(same comment as above -- this code already existed)

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1429: // valid
FutureOr<T> as the downward context type for a conditional.
On 2017/01/24 21:56:26, Leaf wrote:
> I'm not keen on putting this in just as a hack for FutureOr.  My take is that
> this should be a separate feature, and should apply to all cases where we have
a
> downwards context.   Can we do this in a follow up CL, and make it apply to
all
> contexts?  This should also work for ?? and other related operators.

Done.

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1430: if (_strongMode
&& staticType.isObject) {
On 2017/01/24 20:14:50, Paul Berry wrote:
> On 2017/01/24 19:19:09, Paul Berry wrote:
> > On 2017/01/24 18:58:20, Jennifer Messerly wrote:
> > > On 2017/01/24 17:45:13, Paul Berry wrote:
> > > > Why put this logic here and not in _typeSystem.getLeastUpperBound? 
> Putting
> > it
> > > > here means that we'll have to replicate this logic in summary type
> > inference;
> > > 
> > > yeah ... the problem tho is we have two implementations of resolution? I
> don't
> > > know the best way to attack that.
> > 
> > Agreed.  It's really unfortunate that we have two implementations of
> resolution.
> >  As part of the front_end work, I am hoping to get us to the point where so
> much
> > code is shared that it's not really two implementations anymore, it's just
two
> > slightly different code paths that lead to the same inference engine (one
code
> > path for when we're inferring something that's visible as part of summaries,
> one
> > code path for when we're inferring a local variable).  But it's going to
take
> > some time to get there.
> > 
> > > almost every type inference change hits it. I
> > > would love to learn to fix the summary code
> > 
> > I'm glad to help with this--the more people who understand the summary code
> the
> > better.
> > 
> > > but I don't think it gets the same
> > > coverage for strong mode (e.g. it doesn't seem to test that strong mode
> errors
> > > were produced in inferred_type_test & checker_test?) ...
> > 
> > The summary logic doesn't do error checking so there's no error checking to
> > validate.  What we do instead is we run each test in inferred_type_test
> through
> > both kinds of inference and verify that they produce the same set of types. 
> > IIRC, the reason checker_test isn't tested with summaries is that all of its
> > assertions are based on validating that the expected errors occur; since
> summary
> > logic doesn't do error checking, there's nothing to test.
> > 
> > If there's anything else missing from summary validation I'd love to talk
> about
> > it and get the test coverage up.
> > 
> > > and i don't really
> > > understand how it works. Would be happy to discuss this larger issue
though.
> > 
> > That would be nice.  Let's schedule a meeting to talk about it.
> > 
> > > 
> > > > putting it in _typeSystem.getLeastUpperBound would ensure that summary
> type
> > > > inference and non-summary type inference stay consistent.
> > > 
> > > I don't know if we want to implement it in LUB for the language. The
problem
> > > with LUB is it's synthesizing types and used in many places.
> > 
> > Ok, fair enough.  In that case, given the current architecture, I don't
think
> we
> > can get around the need to duplicate this logic in the summary inference
> engine.
> > 
> > I'll look into doing that as soon as I can.
> > 
> > > 
> > > Conditional expressions are a bit special, we've already checked the types
> for
> > > both branches in strong mode, and we had a downwards context FutureOr<T>.
So
> > > this one is sound and matches the request in
> > > https://github.com/dart-lang/sdk/issues/25368
> > 
> 
> Ack!  I just re-read this and realized that it only applies in the case where
> downward inference is in play (which never happens during summary inference). 
> So forget everything I said about needing to duplicate this logic for
summaries.
>  Sorry for the noise.

ahhh gotcha :)

yeah that explains why a lot of these downwards expression inference changes
don't need to affect summary generation. good to know!

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:787: // The types are void,
dynamic, bottom, interface types, function types,
On 2017/01/24 21:56:26, Leaf wrote:
> Update this comment?

Done.

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:834: // (Future<A> | A) <: t2
iff t2 <: Future<A> and t2 <: A.
On 2017/01/24 17:45:13, Paul Berry wrote:
> I believe the code here is correct, but the comment doesn't match it.  Can we
> please change it to say:
> 
> // given t1 is Future<A> | A, then:
> // (Future<A> | A) <: t2 iff Future<A> <: t2 and A <: t2.

Done.

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/test/src/...
File pkg/analyzer/test/src/summary/resynthesize_ast_test.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/summary/resynthesize_ast_test.dart:706: void
test_voidReturnTypeSubtypesDynamic() =>
On 2017/01/24 17:45:13, Paul Berry wrote:
> I'm guessing this override is no longer be necessary now that I've landed
> https://codereview.chromium.org/2650943006/.

can confirm. thank you very much! :)

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

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1737: var x =
f.then<Future<List<int>>>(/*info:INFERRED_TYPE_CLOSURE,error:ARGUMENT_TYPE_NOT_ASSIGNABLE*/(x)
=> []);
On 2017/01/24 21:56:27, Leaf wrote:
> This is going to be a hard google3 roll.  We need to get the SDK updated to
use
> FutureOr, and then I'll need to prepare a CL that gets rid of all of the
> .then<Future<T>> uses.  
> 
> The alternative would be to have one release which keeps future flattening
here,
> get rid of all of the uses of future flattening, then remove the flattening.  

yeah agreed.

A simple fix is to interpret FutureOr<T> as FutureOr< flatten(T) >. That's an
easy change. Let me know. Happy to add that!

https://codereview.chromium.org/2647833002/diff/120001/pkg/analyzer/test/src/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:2504: => list.fold('',
/*info:INFERRED_TYPE_CLOSURE*/(x, y) =>
/*info:DYNAMIC_CAST,info:DYNAMIC_INVOKE*/x /*error:UNDEFINED_OPERATOR*/+
y.toString()));
On 2017/01/24 21:56:27, Leaf wrote:
> What's going with this?  Why the regression?

Context type for "fold" is FutureOr<String>

The old code prevented FutureOr from ever being inferred (it wasn't a reified
type), so downwards inference for "fold" arbitrarily picked String. That made
the argument to "fold" `(String, String) -> String` in downwards inference.

Now, we push down FutureOr<String>. So the argument type is `(FutureOr<String>,
int) -> FutureOr<String>` ... and FutureOr<String> doesn't support "+" operator,
so we can't infer anything useful.

We can't infer "fold" reliably with our current inference. Even `?` wouldn't
save us here. We need to be able to use the first argument type (String) to help
us out, but we don't have that yet. C# style rules, where downwards inference
could take into account dependencies between known and unknown type parameters,
would work.

https://codereview.chromium.org/2647833002/diff/120001/pkg/dev_compiler/tool/...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (right):

https://codereview.chromium.org/2647833002/diff/120001/pkg/dev_compiler/tool/...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:789: //
(Future<A> | A) <: t2 iff t2 <: Future<A> and t2 <: A.
On 2017/01/24 21:56:27, Leaf wrote:
> should be Future<A> <: t2 and A <: t2

thank you. I managed to copy+paste this mistake in two places. At least the
actual code is correct :D

https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
File tests/lib/async/future_or_bad_type_test.dart (left):

https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
tests/lib/async/future_or_bad_type_test.dart:5: // In non strong-mode,
`FutureOr<T>` is dynamic, even if `T` doesn't exist.
On 2017/01/24 21:56:27, Leaf wrote:
> There is a future_or_strong_tests.dart (or something similar) in this
directory.
>  It is not automatically run yet (I'm working on it), but I believe that you
can
> run it by hand.  It would be great to do so as a sanity check.

I'll give it a try

https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
tests/lib/async/future_or_bad_type_test.dart:22: Expect.isTrue(499 is
FutureOr<A>);  /// 00: static type warning
On 2017/01/24 21:56:27, Leaf wrote:
> On 2017/01/24 01:21:22, Jennifer Messerly wrote:
> > I don't understand why this was expected to be a static warning. LMK if it
> makes
> > sense to you. I added another case that should be a warning.
> 
> Can you ask Lasse what his intent was here?

Sure, +lrn@ for thoughts?

[Jennifer Messerly, 2017-01-25 00:37:09]

https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
File tests/lib/async/future_or_bad_type_test.dart (left):

https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
tests/lib/async/future_or_bad_type_test.dart:5: // In non strong-mode,
`FutureOr<T>` is dynamic, even if `T` doesn't exist.
On 2017/01/25 00:33:36, Jennifer Messerly wrote:
> On 2017/01/24 21:56:27, Leaf wrote:
> > There is a future_or_strong_tests.dart (or something similar) in this
> directory.
> >  It is not automatically run yet (I'm working on it), but I believe that you
> can
> > run it by hand.  It would be great to do so as a sanity check.
> 
> I'll give it a try

./tools/test.py -m release --strong lib/async/future_or_strong_test
Test configuration: none_vm_release_x64_strong
[00:00 |  --% | +    1 | -    0]

[Jennifer Messerly, 2017-01-25 00:38:45]

On 2017/01/25 00:37:09, Jennifer Messerly wrote:
>
https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
> File tests/lib/async/future_or_bad_type_test.dart (left):
> 
>
https://codereview.chromium.org/2647833002/diff/120001/tests/lib/async/future...
> tests/lib/async/future_or_bad_type_test.dart:5: // In non strong-mode,
> `FutureOr<T>` is dynamic, even if `T` doesn't exist.
> On 2017/01/25 00:33:36, Jennifer Messerly wrote:
> > On 2017/01/24 21:56:27, Leaf wrote:
> > > There is a future_or_strong_tests.dart (or something similar) in this
> > directory.
> > >  It is not automatically run yet (I'm working on it), but I believe that
you
> > can
> > > run it by hand.  It would be great to do so as a sanity check.
> > 
> > I'll give it a try
> 
> ./tools/test.py -m release --strong lib/async/future_or_strong_test
> Test configuration: none_vm_release_x64_strong
> [00:00 |  --% | +    1 | -    0]

ah, I don't think that did anything. is this the right incantation?

$ ./tools/test.py -m release -r none -c dart2analyzer --strong
lib/async/future_or_strong_test
Test configuration: dart2analyzer_none_release_x64_strong
[00:02 | 100% | +    1 | -    0]

[Lasse Reichstein Nielsen, 2017-01-25 08:11:19]

https://codereview.chromium.org/2647833002/diff/160001/tests/lib/async/future...
File tests/lib/async/future_or_bad_type_test.dart (right):

https://codereview.chromium.org/2647833002/diff/160001/tests/lib/async/future...
tests/lib/async/future_or_bad_type_test.dart:24: Expect.isTrue(499 is
FutureOr<A, A>);              /// 01: static type warning
In Dart 1/spec mode, FutureOr<X> means dynamic, even statically (a List<int> is
statically assignable to List<FutureOr<String>>).

It's actually not clear to me whether malformed types should be a warning or
not, but it's  probably a good idea (and I should update the informal spec to
say so - and say anything about spec mode).

So, I agree that this is what we want. Treat `FutureOr` as a
one-generic-parameter type constructor which always constructs the type
`dynamic`.

[Paul Berry, 2017-01-25 13:24:02]

Thanks, this addresses the issues I raised.

Still lgtm (but of course we should wait for others' review before committing).

[Leaf, 2017-01-25 22:12:54]

jmesserly can you please merge this CL (tests) into yours:

https://codereview.chromium.org/2655133002

[Leaf, 2017-01-25 22:40:00]

lgtm

[Jennifer Messerly, 2017-01-25 23:32:35]

thanks everyone! landing now

[Jennifer Messerly, 2017-01-25 23:33:09]

Description was changed from

==========
fix #28008, fix #28009 implement FutureOr<T>

This implements FutureOr<T> in strong mode, otherwise it's ignored (treated as
`dynamic`.

Also fixes strong mode's inference subtype function incorrectly treating `void`
as a malformed type. This had the consequence of allowing `void` to be inferred
as a type argument.
==========

to

==========
fix #28008, fix #28009 implement FutureOr<T>

This implements FutureOr<T> in strong mode, otherwise it's ignored (treated as
`dynamic`.

Also fixes strong mode's inference subtype function incorrectly treating `void`
as a malformed type. This had the consequence of allowing `void` to be inferred
as a type argument.

R=leafp@google.com, paulberry@google.com

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

[Jennifer Messerly, 2017-01-25 23:33:10]

Committed patchset #12 (id:220001) manually as
0e00b3e5c83e4173d442bf287eb99d1dd12cb09b (presubmit successful).