Temporarily restore ad hoc Future.then inference

Temporarily restore ad hoc Future.then inference

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

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

[Leaf, 2017-02-02 21:15:01]

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

[Leaf, 2017-02-02 21:15:01]

Still testing this out in g3, but wanted to get started on the review
simultaneously.  Note that this is mostly restoring code from here:

https://codereview.chromium.org/2647833002/

with some small additional logic to restrict it to methods which don't use the
FutureOr return type.

[Jennifer Messerly, 2017-02-03 18:31:51]

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

[Jennifer Messerly, 2017-02-03 18:31:52]

Hi Leaf,

Did you consider another way to tackle this: handle it in member inference? We
could see that Future.then is being overridden, and fill in FutureOr<T> in the
return type. That would be a lot less ad hoc/more forwards compatible. Wanted to
throw that out there as an idea, LMK if I can help implement it.

- Jenny

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

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:5221: * TODO(leafp): Eliminate this
when code is switched to using FutureOr
nit: this should go outside of the doc comment

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6091: // Pretend the return type of
Future<T>.then<S> first parameter is
this comment is obsolete now, I think... should say something like "introduce
FutureOr<T> for backwards compatibility if it was missing in old code"

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

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1984:
argTypes[0].isDartAsyncFutureOr &&
I'm not sure this check will be satisfied? it looks like it's matching Dart code
like this:

    FutureOr<T> futureOr;
    MyFuture<T> myFuture;

    myFuture.then(futureOr); // this is going to be a type error anyway


... in other words, is this entire if-block dead code?

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1988: if (firstArgType
is FunctionType) {
if firstArgType was a FutureOr, then it's not a FunctionType

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1994: DartType
paramReturnType = fnType.typeFormals[0].type;
rather than this line and the following if statement, I think you could do this
instead:

DartType paramReturnType = resolver.createFutureOr(fnType.typeFormals[0].type);


... that should also have less effect once folks begin to use FutureOr<T> in
their code.

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

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1908: //    checkFile(
remove commented out test?

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:2004: void
solo_test_futureUnion_downwards_deprecated() {
remove solo_

[Leaf, 2017-02-03 21:28:03]

On 2017/02/03 18:31:52, Jennifer Messerly wrote:
> Hi Leaf,
> 
> Did you consider another way to tackle this: handle it in member inference? We
> could see that Future.then is being overridden, and fill in FutureOr<T> in the
> return type. That would be a lot less ad hoc/more forwards compatible. Wanted
to
> throw that out there as an idea, LMK if I can help implement it.

We actually did this by hand in a few places (by just removing the type from the
overridden method parameter in the source code), but it also adds new errors in
because it makes MyFuture.then(functionReturningDynamic) into an error where it
wasn't before.

[Jennifer Messerly, 2017-02-03 21:28:56]

On 2017/02/03 21:28:03, Leaf wrote:
> On 2017/02/03 18:31:52, Jennifer Messerly wrote:
> > Hi Leaf,
> > 
> > Did you consider another way to tackle this: handle it in member inference?
We
> > could see that Future.then is being overridden, and fill in FutureOr<T> in
the
> > return type. That would be a lot less ad hoc/more forwards compatible.
Wanted
> to
> > throw that out there as an idea, LMK if I can help implement it.
> 
> We actually did this by hand in a few places (by just removing the type from
the
> overridden method parameter in the source code), but it also adds new errors
in
> because it makes MyFuture.then(functionReturningDynamic) into an error where
it
> wasn't before.

oh wow! makes sense.

that's probably an error at runtime in DDC tho?

[Leaf, 2017-02-03 21:28:57]

Thanks for the comments, that was a ugly.  PTAL.

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

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:5221: * TODO(leafp): Eliminate this
when code is switched to using FutureOr
On 2017/02/03 18:31:51, Jennifer Messerly wrote:
> nit: this should go outside of the doc comment

Done.

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6091: // Pretend the return type of
Future<T>.then<S> first parameter is
On 2017/02/03 18:31:51, Jennifer Messerly wrote:
> this comment is obsolete now, I think... should say something like "introduce
> FutureOr<T> for backwards compatibility if it was missing in old code"

Done.

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

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1984:
argTypes[0].isDartAsyncFutureOr &&
On 2017/02/03 18:31:51, Jennifer Messerly wrote:
> I'm not sure this check will be satisfied? it looks like it's matching Dart
code
> like this:
> 
>     FutureOr<T> futureOr;
>     MyFuture<T> myFuture;
> 
>     myFuture.then(futureOr); // this is going to be a type error anyway
> 
> 
> ... in other words, is this entire if-block dead code?

Yeesh.  Totally borked.  I guess the downwards inference was what did all the
heavy lifting, since this part clearly wasn't kicking in...

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1988: if (firstArgType
is FunctionType) {
On 2017/02/03 18:31:52, Jennifer Messerly wrote:
> if firstArgType was a FutureOr, then it's not a FunctionType

Done.

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1994: DartType
paramReturnType = fnType.typeFormals[0].type;
On 2017/02/03 18:31:51, Jennifer Messerly wrote:
> rather than this line and the following if statement, I think you could do
this
> instead:
> 
> DartType paramReturnType =
resolver.createFutureOr(fnType.typeFormals[0].type);
> 
> 
> ... that should also have less effect once folks begin to use FutureOr<T> in
> their code.

Done.

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

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:1908: //    checkFile(
On 2017/02/03 18:31:52, Jennifer Messerly wrote:
> remove commented out test?

or uncomment it.  :}  Done.

https://codereview.chromium.org/2676633005/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/inferred_type_test.dart:2004: void
solo_test_futureUnion_downwards_deprecated() {
On 2017/02/03 18:31:52, Jennifer Messerly wrote:
> remove solo_

yah think?  geez.

[Leaf, 2017-02-03 21:31:54]

> oh wow! makes sense.
> 
> that's probably an error at runtime in DDC tho?

I don't think so, if I'm understanding correctly.  Basically, the old treatment
was that Future.then expected a T -> dynamic, and would cast the result
internally.  It was only inference that treated it as T -> S | Future<S>.  So as
long as the functionReturningDynamic returned a correctly typed value, it would
work.

[Jennifer Messerly, 2017-02-03 21:33:07]

On 2017/02/03 21:31:54, Leaf wrote:
> > oh wow! makes sense.
> > 
> > that's probably an error at runtime in DDC tho?
> 
> I don't think so, if I'm understanding correctly.  Basically, the old
treatment
> was that Future.then expected a T -> dynamic, and would cast the result
> internally.  It was only inference that treated it as T -> S | Future<S>.  So
as
> long as the functionReturningDynamic returned a correctly typed value, it
would
> work.

ohh ... but now it won't work right? Since we require the return type to be
FutureOr<T> in the signature?

[Jennifer Messerly, 2017-02-03 21:33:50]

lgtm

[Brian Wilkerson, 2017-02-03 21:35:02]

lgtm

[Leaf, 2017-02-03 21:52:27]

Description was changed from

==========
Temporarily restore ad hoc Future.then inference

BUG=
==========

to

==========
Temporarily restore ad hoc Future.then inference

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

Committed:
https://github.com/dart-lang/sdk/commit/15527f136f984af2707ef52969ea5391aa6b49fc
==========

[Leaf, 2017-02-03 21:52:28]

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