Void is not required to be `null` anymore.

Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, kmillikin@google.com, lrn@google.com, regis@google.com

Committed: https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted: https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
Committed: https://github.com/dart-lang/sdk/commit/4e52c457a9ac412b6b0a3a350020998c7e2f7d9a
Reverted: https://github.com/dart-lang/sdk/commit/4b35d3995b2b273728751c19976ece0edca0eddc
Committed: https://github.com/dart-lang/sdk/commit/6caf9ef4439e20ea726d8a5383ed34d42b02c84b
Reverted: https://github.com/dart-lang/sdk/commit/30d9bf2ed3225f04c52d999a1a5c61e63a90578d
Committed: https://github.com/dart-lang/sdk/commit/8e01f69305c48ceabc53f4634f0f8a203dec15ca

[floitsch, 2017-02-27 14:58:43]

floitsch@google.com changed reviewers:
+ johnniwinther@google.com, lrn@google.com, regis@google.com

[regis, 2017-02-27 18:25:23]

I am only commenting on the VM changes.

Your modification in runtime/vm/flow_graph_builder.cc instructs the code
generator not to emit a type check if the destination type is void. However, it
does not actually update the type test with the new semantics. This should be
done in object.cc, Instance::IsInstanceOf(...), by returning true instead of
false if the destination type is void.

Also, inlined type test on all platforms need updating: see the various
FlowGraphCompiler::GenerateInlineInstanceof(...) for each platform.

You cannot assume null anymore in the case of void type in
flow_graph_type_propagator.cc, see CompileType::ToNullableCid() and also
CompileType::CanComputeIsInstanceOf(...). and also
CompileType::IsMoreSpecificThan(...) and also
AssertAssignableInstr::ComputeType() and also StaticCallInstr::ComputeType()

There is also the case where a function type is checked. See object.cc,
Function::TypeTest(...). Is now a function type returning void a subtype of a
function returning non-void? If so, the code needs updating too.

[floitsch, 2017-02-27 18:31:04]

On 2017/02/27 18:25:23, regis wrote:
> I am only commenting on the VM changes.
> 
> Your modification in runtime/vm/flow_graph_builder.cc instructs the code
> generator not to emit a type check if the destination type is void. However,
it
> does not actually update the type test with the new semantics. This should be
> done in object.cc, Instance::IsInstanceOf(...), by returning true instead of
> false if the destination type is void.
> 
> Also, inlined type test on all platforms need updating: see the various
> FlowGraphCompiler::GenerateInlineInstanceof(...) for each platform.
> 
> You cannot assume null anymore in the case of void type in
> flow_graph_type_propagator.cc, see CompileType::ToNullableCid() and also
> CompileType::CanComputeIsInstanceOf(...). and also
> CompileType::IsMoreSpecificThan(...) and also
> AssertAssignableInstr::ComputeType() and also StaticCallInstr::ComputeType()
> 
> There is also the case where a function type is checked. See object.cc,
> Function::TypeTest(...). Is now a function type returning void a subtype of a
> function returning non-void? If so, the code needs updating too.

Thanks. That's great feedback.

[floitsch, 2017-03-01 19:18:52]

PTAL.

@johnni and @lrn: ping.

[Lasse Reichstein Nielsen, 2017-03-02 07:21:49]

tests + changelog LGTM

https://codereview.chromium.org/2718513002/diff/100001/tests/language/void_ch...
File tests/language/void_check_test.dart (right):

https://codereview.chromium.org/2718513002/diff/100001/tests/language/void_ch...
tests/language/void_check_test.dart:13: return bar();
indentation is off.

[Lasse Reichstein Nielsen, 2017-03-02 07:21:50]

tests + changelog LGTM

[Johnni Winther, 2017-03-02 08:35:46]

lgtm

[floitsch, 2017-03-06 16:14:26]

Description was changed from

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.
==========

to

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
==========

[floitsch, 2017-03-06 16:15:21]

@regis: ping.
Do you have suggestions on how to test the non-obvious VM paths?

[regis, 2017-03-06 19:12:20]

On 2017/03/06 16:15:21, floitsch wrote:
> @regis: ping.
> Do you have suggestions on how to test the non-obvious VM paths?

It is quite difficult to test all paths in the VM and I do not have a magic
recipe. The problem is that various optimizations evolved over the years, making
some unoptimized paths redundant. Our test suite is pretty good though and I
would rely on it.

However, I am not so sure about this cl, as long as the more general issue with
void as discussed in a separate thread is resolved. I do think that the current
semantics could be preserved, while still allowing void functions to be
overridden.

[floitsch, 2017-03-09 16:48:55]

On 2017/03/06 19:12:20, regis wrote:
> On 2017/03/06 16:15:21, floitsch wrote:
> > @regis: ping.
> > Do you have suggestions on how to test the non-obvious VM paths?
> 
> It is quite difficult to test all paths in the VM and I do not have a magic
> recipe. The problem is that various optimizations evolved over the years,
making
> some unoptimized paths redundant. Our test suite is pretty good though and I
> would rely on it.
> 
> However, I am not so sure about this cl, as long as the more general issue
with
> void as discussed in a separate thread is resolved. I do think that the
current
> semantics could be preserved, while still allowing void functions to be
> overridden.

This CL fixes a problem we *currently* have. It is not gated by the other
changes we have planned for `void`. (They would also need this change, but that
is independent).

[floitsch, 2017-03-22 15:17:09]

On 2017/03/09 16:48:55, floitsch wrote:
> On 2017/03/06 19:12:20, regis wrote:
> > On 2017/03/06 16:15:21, floitsch wrote:
> > > @regis: ping.
> > > Do you have suggestions on how to test the non-obvious VM paths?
> > 
> > It is quite difficult to test all paths in the VM and I do not have a magic
> > recipe. The problem is that various optimizations evolved over the years,
> making
> > some unoptimized paths redundant. Our test suite is pretty good though and I
> > would rely on it.
> > 
> > However, I am not so sure about this cl, as long as the more general issue
> with
> > void as discussed in a separate thread is resolved. I do think that the
> current
> > semantics could be preserved, while still allowing void functions to be
> > overridden.
> 
> This CL fixes a problem we *currently* have. It is not gated by the other
> changes we have planned for `void`. (They would also need this change, but
that
> is independent).

Ping.

[regis, 2017-03-22 16:29:33]

https://codereview.chromium.org/2718513002/diff/100001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2718513002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:3790: if (other.IsDynamicClass()) {
|| other.IsVoidClass()   // and update the comment, but check my question below
first

https://codereview.chromium.org/2718513002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:3800: // However, DynamicType is not more specific than any
type.
If dynamic is not more specific than void, then ignore my comment above and move
the void test below with the ObjectType test.

https://codereview.chromium.org/2718513002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:3806: if (other.IsObjectClass()) {
if you did not handle void above, check here.

https://codereview.chromium.org/2718513002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:4822: if (!bound.IsObjectType() && !bound.IsDynamicType())
{
Is void allowed as an upper bound? If so, && !bound.IsVoidType()

https://codereview.chromium.org/2718513002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:4927: if (!bound.IsObjectType() && !bound.IsDynamicType())
{
ditto

https://codereview.chromium.org/2718513002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:15670: instantiated_other.IsDartFunctionType()) {
This may not yet be necessary, but it will as soon as we allow void as a type
argument:
|| instantiated_other.IsVoidType()

https://codereview.chromium.org/2718513002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:16466: if (other.IsObjectType() || other.IsDynamicType() ||
IsNullType()) {
|| other.IsVoidType()

https://codereview.chromium.org/2718513002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:18119: ASSERT(!upper_bound.IsObjectType() &&
!upper_bound.IsDynamicType());
&& !upper_bound.IsVoidType()

https://codereview.chromium.org/2718513002/diff/100001/tests/language/void_ch...
File tests/language/void_check_test.dart (right):

https://codereview.chromium.org/2718513002/diff/100001/tests/language/void_ch...
tests/language/void_check_test.dart:13: return bar();
This line hurts my eyes, and it is not because of the indentation. It is a
contrived example returning something from a void function.

[regis, 2017-03-22 16:33:48]

https://codereview.chromium.org/2718513002/diff/100001/tests/language/void_ch...
File tests/language/void_check_test.dart (right):

https://codereview.chromium.org/2718513002/diff/100001/tests/language/void_ch...
tests/language/void_check_test.dart:13: return bar();
On 2017/03/22 16:29:32, regis wrote:
> This line hurts my eyes, and it is not because of the indentation. It is a
> contrived example returning something from a void function.

Ignore this comment. It was saved as a draft and predates our discussion of last
week.

[floitsch, 2017-03-23 01:22:34]

Regis: Are you sure your comments are for this CL?
This CL doesn't introduce `void` as a generic type yet.
It just removes the check that `void` must be `null`. This change should not
require any changes in the subtyping code.

(I will keep your comments for future changes, when we do add `void` as a
generic type, but that's for another CL).

[regis, 2017-03-23 17:57:12]

On 2017/03/23 01:22:34, floitsch wrote:
> Regis: Are you sure your comments are for this CL?
> This CL doesn't introduce `void` as a generic type yet.
> It just removes the check that `void` must be `null`. This change should not
> require any changes in the subtyping code.
> 
> (I will keep your comments for future changes, when we do add `void` as a
> generic type, but that's for another CL).

Correct, as long as you do not yet allow `void` as a type argument or as a
regular type (of a field, local, upper bound, etc...), then you can ignore my
comments for now.

lgtm

[floitsch, 2017-05-03 15:31:30]

Description was changed from

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
==========

to

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

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

[floitsch, 2017-05-03 15:31:32]

Committed patchset #7 (id:120001) manually as
521dc6620fd2c4591aa975745c14b304428f17eb (presubmit successful).

[floitsch, 2017-05-03 15:37:54]

Description was changed from

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

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

to

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
==========

[floitsch, 2017-05-03 15:43:30]

Description was changed from

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
==========

to

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
Committed:
https://github.com/dart-lang/sdk/commit/4e52c457a9ac412b6b0a3a350020998c7e2f7d9a
==========

[floitsch, 2017-05-03 15:43:31]

Committed patchset #9 (id:160001) manually as
4e52c457a9ac412b6b0a3a350020998c7e2f7d9a (presubmit successful).

[floitsch, 2017-05-03 17:24:08]

Description was changed from

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
Committed:
https://github.com/dart-lang/sdk/commit/4e52c457a9ac412b6b0a3a350020998c7e2f7d9a
==========

to

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
Committed:
https://github.com/dart-lang/sdk/commit/4e52c457a9ac412b6b0a3a350020998c7e2f7d9a
Reverted:
https://github.com/dart-lang/sdk/commit/4b35d3995b2b273728751c19976ece0edca0eddc
==========

[floitsch, 2017-05-03 18:23:58]

@regis: could you please have another look at the VM changes.
I hit a crash in debug mode and had to fix a few lines.
The last patch-set contains the changes.

[regis, 2017-05-04 10:23:08]

On 2017/05/03 18:23:58, floitsch wrote:
> @regis: could you please have another look at the VM changes.
> I hit a crash in debug mode and had to fix a few lines.
> The last patch-set contains the changes.

The last patch set looks good.

[floitsch, 2017-05-04 14:27:59]

Description was changed from

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
Committed:
https://github.com/dart-lang/sdk/commit/4e52c457a9ac412b6b0a3a350020998c7e2f7d9a
Reverted:
https://github.com/dart-lang/sdk/commit/4b35d3995b2b273728751c19976ece0edca0eddc
==========

to

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
Committed:
https://github.com/dart-lang/sdk/commit/4e52c457a9ac412b6b0a3a350020998c7e2f7d9a
Reverted:
https://github.com/dart-lang/sdk/commit/4b35d3995b2b273728751c19976ece0edca0eddc
Committed:
https://github.com/dart-lang/sdk/commit/6caf9ef4439e20ea726d8a5383ed34d42b02c84b
==========

[floitsch, 2017-05-04 14:28:00]

Committed patchset #11 (id:200001) manually as
6caf9ef4439e20ea726d8a5383ed34d42b02c84b (presubmit successful).

[floitsch, 2017-05-06 16:40:11]

Description was changed from

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
Committed:
https://github.com/dart-lang/sdk/commit/4e52c457a9ac412b6b0a3a350020998c7e2f7d9a
Reverted:
https://github.com/dart-lang/sdk/commit/4b35d3995b2b273728751c19976ece0edca0eddc
Committed:
https://github.com/dart-lang/sdk/commit/6caf9ef4439e20ea726d8a5383ed34d42b02c84b
==========

to

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
Committed:
https://github.com/dart-lang/sdk/commit/4e52c457a9ac412b6b0a3a350020998c7e2f7d9a
Reverted:
https://github.com/dart-lang/sdk/commit/4b35d3995b2b273728751c19976ece0edca0eddc
Committed:
https://github.com/dart-lang/sdk/commit/6caf9ef4439e20ea726d8a5383ed34d42b02c84b
Reverted:
https://github.com/dart-lang/sdk/commit/30d9bf2ed3225f04c52d999a1a5c61e63a90578d
==========

[floitsch, 2017-05-06 17:47:09]

floitsch@google.com changed reviewers:
+ kmillikin@google.com

[floitsch, 2017-05-06 17:47:10]

Had to revert (again), because it now hit a line in the kernel code.

Kevin: please have a look at the change in the kernel_to_il.cc file. The change
is very simple, so I'm TBRing.

Also note that this CL now contains updates to the co19 status file from
https://codereview.chromium.org/2856863009.

[Kevin Millikin (Google), 2017-05-08 07:35:42]

Kernel changes LGTM.

[floitsch, 2017-05-08 09:31:44]

Description was changed from

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, lrn@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
Committed:
https://github.com/dart-lang/sdk/commit/4e52c457a9ac412b6b0a3a350020998c7e2f7d9a
Reverted:
https://github.com/dart-lang/sdk/commit/4b35d3995b2b273728751c19976ece0edca0eddc
Committed:
https://github.com/dart-lang/sdk/commit/6caf9ef4439e20ea726d8a5383ed34d42b02c84b
Reverted:
https://github.com/dart-lang/sdk/commit/30d9bf2ed3225f04c52d999a1a5c61e63a90578d
==========

to

==========
Void is not required to be `null` anymore.

In checked mode, `void` now accepts any value.

Fixes #28937.
Fixes #28938.

BUG= http://dartbug.com/28937
BUG= http://dartbug.com/28938
R=johnniwinther@google.com, kmillikin@google.com, lrn@google.com,
regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/521dc6620fd2c4591aa975745c14b304428f17eb
Reverted:
https://github.com/dart-lang/sdk/commit/a3c0cb65af4bbbdde756208dcccdcdf896902b5b
Committed:
https://github.com/dart-lang/sdk/commit/4e52c457a9ac412b6b0a3a350020998c7e2f7d9a
Reverted:
https://github.com/dart-lang/sdk/commit/4b35d3995b2b273728751c19976ece0edca0eddc
Committed:
https://github.com/dart-lang/sdk/commit/6caf9ef4439e20ea726d8a5383ed34d42b02c84b
Reverted:
https://github.com/dart-lang/sdk/commit/30d9bf2ed3225f04c52d999a1a5c61e63a90578d
Committed:
https://github.com/dart-lang/sdk/commit/8e01f69305c48ceabc53f4634f0f8a203dec15ca
==========

[floitsch, 2017-05-08 09:31:46]

Committed patchset #13 (id:240001) manually as
8e01f69305c48ceabc53f4634f0f8a203dec15ca (presubmit successful).