Reify implicit casts for FutureOr

Reify implicit casts for FutureOr

Before landing, two issues to resolve:
(1) We appear to allow sideways casts now without static error - e.g.,

List<Object> -> FutureOr<List<String>>

Is that intended?  If so, it breaks an assumption that implicit casts
are from supertype to subtype.

(2) LUB and FutureOr

It appears that the LUB of, e.g., String and Future<String> is Object.
See fixes in tests to record casts back to FutureOr.

Fixes #29251

R=leafp@google.com

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

[vsm, 2017-04-05 15:52:59]

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

https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:958: return
(/*info:DOWN_CAST_COMPOSITE*/(1 > 0) ? new Future<String>.value('hello') :
"world");
FYI - this is a cast from Object to FutureOr<String> :-(

[Leaf, 2017-04-05 20:47:27]

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

[Leaf, 2017-04-05 20:47:27]

https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong/checker.dart (right):

https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/checker.dart:940: // Unwrap FutureOr union to
check for a cast.
This will solve the async issue, but will also generate downcasts for other
cases that are sideways casts.  I don't think this is a problem, in fact I'm not
sure why we care about eliding sideways casts.

https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/checker.dart:951: return null;
Is there any reason not to just generate the cast even when it's a sideways
cast?  Then you could just get rid of the new code above.

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

https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:958: return
(/*info:DOWN_CAST_COMPOSITE*/(1 > 0) ? new Future<String>.value('hello') :
"world");
On 2017/04/05 15:52:59, vsm wrote:
> FYI - this is a cast from Object to FutureOr<String> :-(

The fact that the LUB is Object is WAI.  The consensus on the language team was
not to make LUB produce FutureOr.

Treating this as a cast to FutureOr<String> is, I believe, a valid
implementation strategy but is not the only valid strategy.

[vsm, 2017-04-07 22:23:08]

PTAL

https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong/checker.dart (right):

https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/checker.dart:940: // Unwrap FutureOr union to
check for a cast.
On 2017/04/05 20:47:27, Leaf wrote:
> This will solve the async issue, but will also generate downcasts for other
> cases that are sideways casts.  I don't think this is a problem, in fact I'm
not
> sure why we care about eliding sideways casts.

I've cleaned this up to just use assignability.

https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/checker.dart:951: return null;
On 2017/04/05 20:47:27, Leaf wrote:
> Is there any reason not to just generate the cast even when it's a sideways
> cast?  Then you could just get rid of the new code above.

Injecting the cast here spams the error list with a ton of hints (and a big diff
in the tests).

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

https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:958: return
(/*info:DOWN_CAST_COMPOSITE*/(1 > 0) ? new Future<String>.value('hello') :
"world");
On 2017/04/05 20:47:27, Leaf wrote:
> On 2017/04/05 15:52:59, vsm wrote:
> > FYI - this is a cast from Object to FutureOr<String> :-(
> 
> The fact that the LUB is Object is WAI.  The consensus on the language team
was
> not to make LUB produce FutureOr.
> 
> Treating this as a cast to FutureOr<String> is, I believe, a valid
> implementation strategy but is not the only valid strategy.

Makes sense.  Yes, it'll work - just unnecessary casts.

[vsm, 2017-04-07 22:31:46]

On 2017/04/07 22:23:08, vsm wrote:
> PTAL
> 
>
https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
> File pkg/analyzer/lib/src/task/strong/checker.dart (right):
> 
>
https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
> pkg/analyzer/lib/src/task/strong/checker.dart:940: // Unwrap FutureOr union to
> check for a cast.
> On 2017/04/05 20:47:27, Leaf wrote:
> > This will solve the async issue, but will also generate downcasts for other
> > cases that are sideways casts.  I don't think this is a problem, in fact I'm
> not
> > sure why we care about eliding sideways casts.
> 
> I've cleaned this up to just use assignability.
> 
>
https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
> pkg/analyzer/lib/src/task/strong/checker.dart:951: return null;
> On 2017/04/05 20:47:27, Leaf wrote:
> > Is there any reason not to just generate the cast even when it's a sideways
> > cast?  Then you could just get rid of the new code above.
> 
> Injecting the cast here spams the error list with a ton of hints (and a big
diff
> in the tests).
> 
>
https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/test/src/task/...
> File pkg/analyzer/test/src/task/strong/checker_test.dart (right):
> 
>
https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/test/src/task/...
> pkg/analyzer/test/src/task/strong/checker_test.dart:958: return
> (/*info:DOWN_CAST_COMPOSITE*/(1 > 0) ? new Future<String>.value('hello') :
> "world");
> On 2017/04/05 20:47:27, Leaf wrote:
> > On 2017/04/05 15:52:59, vsm wrote:
> > > FYI - this is a cast from Object to FutureOr<String> :-(
> > 
> > The fact that the LUB is Object is WAI.  The consensus on the language team
> was
> > not to make LUB produce FutureOr.
> > 
> > Treating this as a cast to FutureOr<String> is, I believe, a valid
> > implementation strategy but is not the only valid strategy.
> 
> Makes sense.  Yes, it'll work - just unnecessary casts.

Actually, this isn't quite right - let me make another pass.

[vsm, 2017-04-07 22:52:31]

On 2017/04/07 22:31:46, vsm wrote:
> On 2017/04/07 22:23:08, vsm wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
> > File pkg/analyzer/lib/src/task/strong/checker.dart (right):
> > 
> >
>
https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
> > pkg/analyzer/lib/src/task/strong/checker.dart:940: // Unwrap FutureOr union
to
> > check for a cast.
> > On 2017/04/05 20:47:27, Leaf wrote:
> > > This will solve the async issue, but will also generate downcasts for
other
> > > cases that are sideways casts.  I don't think this is a problem, in fact
I'm
> > not
> > > sure why we care about eliding sideways casts.
> > 
> > I've cleaned this up to just use assignability.
> > 
> >
>
https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/lib/src/task/s...
> > pkg/analyzer/lib/src/task/strong/checker.dart:951: return null;
> > On 2017/04/05 20:47:27, Leaf wrote:
> > > Is there any reason not to just generate the cast even when it's a
sideways
> > > cast?  Then you could just get rid of the new code above.
> > 
> > Injecting the cast here spams the error list with a ton of hints (and a big
> diff
> > in the tests).
> > 
> >
>
https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/test/src/task/...
> > File pkg/analyzer/test/src/task/strong/checker_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/2801683004/diff/1/pkg/analyzer/test/src/task/...
> > pkg/analyzer/test/src/task/strong/checker_test.dart:958: return
> > (/*info:DOWN_CAST_COMPOSITE*/(1 > 0) ? new Future<String>.value('hello') :
> > "world");
> > On 2017/04/05 20:47:27, Leaf wrote:
> > > On 2017/04/05 15:52:59, vsm wrote:
> > > > FYI - this is a cast from Object to FutureOr<String> :-(
> > > 
> > > The fact that the LUB is Object is WAI.  The consensus on the language
team
> > was
> > > not to make LUB produce FutureOr.
> > > 
> > > Treating this as a cast to FutureOr<String> is, I believe, a valid
> > > implementation strategy but is not the only valid strategy.
> > 
> > Makes sense.  Yes, it'll work - just unnecessary casts.
> 
> Actually, this isn't quite right - let me make another pass.

Ok, PTAL

I had to add the special case back in, as the async return case induces a cast
between unassignable types.

[Leaf, 2017-04-07 23:11:57]

lgtm

[vsm, 2017-04-08 00:33:44]

Description was changed from

==========
Reify implicit casts for FutureOr

Before landing, two issues to resolve:
(1) We appear to allow sideways casts now without static error - e.g.,

List<Object> -> FutureOr<List<String>>

Is that intended?  If so, it breaks an assumption that implicit casts
are from supertype to subtype.

(2) LUB and FutureOr

It appears that the LUB of, e.g., String and Future<String> is Object.
See fixes in tests to record casts back to FutureOr.

Fixes #29251
==========

to

==========
Reify implicit casts for FutureOr

Before landing, two issues to resolve:
(1) We appear to allow sideways casts now without static error - e.g.,

List<Object> -> FutureOr<List<String>>

Is that intended?  If so, it breaks an assumption that implicit casts
are from supertype to subtype.

(2) LUB and FutureOr

It appears that the LUB of, e.g., String and Future<String> is Object.
See fixes in tests to record casts back to FutureOr.

Fixes #29251

R=leafp@google.com

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

[vsm, 2017-04-08 00:33:45]

Committed patchset #3 (id:20001) manually as
bc3089838cd3fb1e3e84dacd2807b0cc89682d78 (presubmit successful).