fix #28642, handling of top and bottom types in DDC

fix #28642, handling of top and bottom types in DDC
in particular: void, Null, and combinations with FutureOr<T>

R=leafp@google.com

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

[Jennifer Messerly, 2017-02-06 20:38:11]

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

[Jennifer Messerly, 2017-02-06 20:39:03]

Description was changed from

==========
fix handling of top and bottom types in DDC
in particular: void, Null, and combinations with FutureOr<T>
==========

to

==========
fix #28642, handling of top and bottom types in DDC
in particular: void, Null, and combinations with FutureOr<T>
==========

[Leaf, 2017-02-06 21:11:59]

Can you try out the fix I discussed below?  If that fixes the expectation
changes that I called out in the tests, then LGTM.  Otherwise let's discuss
further.

https://codereview.chromium.org/2671113002/diff/20001/pkg/dev_compiler/test/b...
File pkg/dev_compiler/test/browser/runtime_tests.js (right):

https://codereview.chromium.org/2671113002/diff/20001/pkg/dev_compiler/test/b...
pkg/dev_compiler/test/browser/runtime_tests.js:945: // ... skip the (infinite)
rest of the top types :D
Probably for the best...

https://codereview.chromium.org/2671113002/diff/20001/pkg/dev_compiler/test/b...
pkg/dev_compiler/test/browser/runtime_tests.js:1062:
maybe(functionType(dart.void, [int]), functionType(int, [dyn]));
I'm worried about these changes, this could break code since we will throw an
error.

https://codereview.chromium.org/2671113002/diff/20001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (left):

https://codereview.chromium.org/2671113002/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:666: return ret2
=== $dynamic || ret2 === $FutureOr;
For now, can you keep this test?  Deleting it is what is causing the expectation
change in the runtime tests, and I'm concerned that this will break tests or
code.  This line allows us to return false when we test whether a function
returning void is a function returning int.  We actually do agree with the VM on
this, so it's safe to return false.  Otherwise we fall through to the next
check, where we return null, since we don't know whether were testing void
against int (safe, we agree with the VM) or Object against int (unsafe, we
disagree with the VM).

[Jennifer Messerly, 2017-02-06 22:50:12]

okay I'll rerun with the _isTop check and land if that works for you. I'm going
to be inconsistently around for the rest of today & gone tomorrow so feel free
to revert/change on top of this/whatever else you need.

https://codereview.chromium.org/2671113002/diff/20001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (left):

https://codereview.chromium.org/2671113002/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:666: return ret2
=== $dynamic || ret2 === $FutureOr;
On 2017/02/06 21:11:59, Leaf wrote:
> For now, can you keep this test?  Deleting it is what is causing the
expectation
> change in the runtime tests, and I'm concerned that this will break tests or
> code.  This line allows us to return false when we test whether a function
> returning void is a function returning int.  We actually do agree with the VM
on
> this, so it's safe to return false.  Otherwise we fall through to the next
> check, where we return null, since we don't know whether were testing void
> against int (safe, we agree with the VM) or Object against int (unsafe, we
> disagree with the VM).

okay, sure, it should be _isTop(ret2) then?

[Leaf, 2017-02-06 22:55:54]

works for me.

https://codereview.chromium.org/2671113002/diff/20001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart (left):

https://codereview.chromium.org/2671113002/diff/20001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart:666: return ret2
=== $dynamic || ret2 === $FutureOr;
On 2017/02/06 22:50:12, Jennifer Messerly wrote:
> On 2017/02/06 21:11:59, Leaf wrote:
> > For now, can you keep this test?  Deleting it is what is causing the
> expectation
> > change in the runtime tests, and I'm concerned that this will break tests or
> > code.  This line allows us to return false when we test whether a function
> > returning void is a function returning int.  We actually do agree with the
VM
> on
> > this, so it's safe to return false.  Otherwise we fall through to the next
> > check, where we return null, since we don't know whether were testing void
> > against int (safe, we agree with the VM) or Object against int (unsafe, we
> > disagree with the VM).
> 
> okay, sure, it should be _isTop(ret2) then?

Yep, that would be the best fix.

[Jennifer Messerly, 2017-02-06 22:56:21]

Description was changed from

==========
fix #28642, handling of top and bottom types in DDC
in particular: void, Null, and combinations with FutureOr<T>
==========

to

==========
fix #28642, handling of top and bottom types in DDC
in particular: void, Null, and combinations with FutureOr<T>

R=leafp@google.com

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

[Jennifer Messerly, 2017-02-06 22:56:22]

Committed patchset #3 (id:40001) manually as
5817ba47e26469fa276c3a87bfdbcd6996894b96 (presubmit successful).