Revert "Void is not required to be null anymore."

This reverts commit 6caf9ef.

Commit breaks vm-kernel-linux-debug-x64-be.

Triggers asserts like
../../runtime/vm/flow_graph_compiler_x64.cc: 658: error: expected: dst_type.IsMalformedOrMalbounded() || (!dst_type.IsDynamicType() && !dst_type.IsObjectType() && !dst_type.IsVoidType())

Quick-check with python tools/test.py -cdartk -t120 --builder-tag no_ipv6 --vm-options --no-enable-malloc-hooks isolate

BUG=

Review-Url: https://codereview.chromium.org/2865603003 .

CHANGELOG.md

@@ -1,9 +1,6 @@
 ## 1.24.0
 
 ### Language
-* During a dynamic type check, `void` is not required to be `null` anymore.
-  In practice, this makes overriding `void` functions with non-`void` functions
-  safer.
 
 #### Strong Mode
 

pkg/compiler/lib/src/inferrer/type_graph_nodes.dart

@@ -1753,13 +1753,14 @@ TypeMask _narrowType(
     {bool isNullable: true}) {
   if (annotation.treatAsDynamic) return type;
   if (annotation.isObject) return type;
-  if (annotation.isVoid) return type;
   TypeMask otherType;
   if (annotation.isTypedef || annotation.isFunctionType) {
     otherType = closedWorld.commonMasks.functionType;
   } else if (annotation.isTypeVariable) {
     // TODO(ngeoffray): Narrow to bound.
     return type;
+  } else if (annotation.isVoid) {
+    otherType = closedWorld.commonMasks.nullType;
   } else {
     ResolutionInterfaceType interfaceType = annotation;
     otherType = new TypeMask.nonNullSubtype(interfaceType.element, closedWorld);

pkg/compiler/lib/src/inferrer/type_system.dart

@@ -282,7 +282,7 @@ class TypeSystem {
       TypeInformation type, ResolutionDartType annotation,
       {bool isNullable: true}) {
     if (annotation.treatAsDynamic) return type;
-    if (annotation.isVoid) return type;
+    if (annotation.isVoid) return nullType;
     if (annotation.element == closedWorld.commonElements.objectClass &&
         isNullable) {
       return type;

pkg/compiler/lib/src/js_backend/checked_mode_helpers.dart

@@ -117,6 +117,7 @@ class CheckedModeHelpers {
   /// All the checked mode helpers.
   static const List<CheckedModeHelper> helpers = const <CheckedModeHelper>[
     const MalformedCheckedModeHelper('checkMalformedType'),
+    const CheckedModeHelper('voidTypeCheck'),
     const CheckedModeHelper('stringTypeCast'),
     const CheckedModeHelper('stringTypeCheck'),
     const CheckedModeHelper('doubleTypeCast'),
@@ -202,6 +203,12 @@ class CheckedModeHelpers {
       return 'checkMalformedType';
     }
 
+    if (type.isVoid) {
+      assert(!typeCast); // Cannot cast to void.
+      if (nativeCheckOnly) return null;
+      return 'voidTypeCheck';
+    }
+
     if (type.isTypeVariable) {
       return typeCast
           ? 'subtypeOfRuntimeTypeCast'

pkg/compiler/lib/src/js_backend/impact_transformer.dart

@@ -363,7 +363,6 @@ class CodegenImpactTransformer {
 
   void onIsCheckForCodegen(DartType type, TransformedWorldImpact transformed) {
     if (type.isDynamic) return;
-    if (type.isVoid) return;
     type = type.unaliased;
     _impacts.typeCheck.registerImpact(transformed, _elementEnvironment);
 

pkg/compiler/lib/src/ssa/codegen.dart

@@ -2928,7 +2928,6 @@ class SsaCodeGenerator implements HVisitor, HBlockInformationVisitor {
     DartType type = node.typeExpression;
     assert(!type.isTypedef);
     assert(!type.isDynamic);
-    assert(!type.isVoid);
     if (type.isFunctionType) {
       // TODO(5022): We currently generate $isFunction checks for
       // function types.

pkg/compiler/lib/src/ssa/nodes.dart

@@ -1342,9 +1342,8 @@ abstract class HInstruction implements Spannable {
     assert(!type.isTypeVariable);
     assert(type.treatAsRaw || type.isFunctionType);
     if (type.isDynamic) return this;
-    if (type.isVoid) return this;
     if (type == closedWorld.commonElements.objectType) return this;
-    if (type.isFunctionType || type.isMalformed) {
+    if (type.isVoid || type.isFunctionType || type.isMalformed) {
       return new HTypeConversion(
           type, kind, closedWorld.commonMasks.dynamicType, this);
     }

runtime/vm/flow_graph_builder.cc

@@ -1259,9 +1259,8 @@ bool EffectGraphVisitor::CanSkipTypeCheck(TokenPosition token_pos,
     return false;
   }
 
-  // Any type is more specific than the dynamic type, the Object type, or void.
-  if (dst_type.IsDynamicType() || dst_type.IsObjectType() ||
-      dst_type.IsVoidType()) {
+  // Any type is more specific than the dynamic type and than the Object type.
+  if (dst_type.IsDynamicType() || dst_type.IsObjectType()) {
     return true;
   }
 

runtime/vm/flow_graph_compiler_arm.cc

@@ -532,6 +532,11 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateInlineInstanceof(
     Label* is_instance_lbl,
     Label* is_not_instance_lbl) {
   __ Comment("InlineInstanceof");
+  if (type.IsVoidType()) {
+    // A non-null value is returned from a void function, which will result in a
+    // type error. A null value is handled prior to executing this inline code.
+    return SubtypeTestCache::null();
+  }
   if (type.IsInstantiated()) {
     const Class& type_class = Class::ZoneHandle(zone(), type.type_class());
     // A class equality check is only applicable with a dst type (not a
@@ -574,7 +579,7 @@ void FlowGraphCompiler::GenerateInstanceOf(TokenPosition token_pos,
                                            const AbstractType& type,
                                            LocationSummary* locs) {
   ASSERT(type.IsFinalized() && !type.IsMalformed() && !type.IsMalbounded());
-  ASSERT(!type.IsObjectType() && !type.IsDynamicType() && !type.IsVoidType());
+  ASSERT(!type.IsObjectType() && !type.IsDynamicType());
   const Register kInstantiatorTypeArgumentsReg = R2;
   const Register kFunctionTypeArgumentsReg = R1;
   __ PushList((1 << kInstantiatorTypeArgumentsReg) |
@@ -653,8 +658,7 @@ void FlowGraphCompiler::GenerateAssertAssignable(TokenPosition token_pos,
   ASSERT(dst_type.IsFinalized());
   // Assignable check is skipped in FlowGraphBuilder, not here.
   ASSERT(dst_type.IsMalformedOrMalbounded() ||
-         (!dst_type.IsDynamicType() && !dst_type.IsObjectType() &&
-          !dst_type.IsVoidType()));
+         (!dst_type.IsDynamicType() && !dst_type.IsObjectType()));
   const Register kInstantiatorTypeArgumentsReg = R2;
   const Register kFunctionTypeArgumentsReg = R1;
   __ PushList((1 << kInstantiatorTypeArgumentsReg) |

runtime/vm/flow_graph_compiler_arm64.cc

@@ -524,6 +524,11 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateInlineInstanceof(
     Label* is_instance_lbl,
     Label* is_not_instance_lbl) {
   __ Comment("InlineInstanceof");
+  if (type.IsVoidType()) {
+    // A non-null value is returned from a void function, which will result in a
+    // type error. A null value is handled prior to executing this inline code.
+    return SubtypeTestCache::null();
+  }
   if (type.IsInstantiated()) {
     const Class& type_class = Class::ZoneHandle(zone(), type.type_class());
     // A class equality check is only applicable with a dst type (not a
@@ -566,7 +571,7 @@ void FlowGraphCompiler::GenerateInstanceOf(TokenPosition token_pos,
                                            const AbstractType& type,
                                            LocationSummary* locs) {
   ASSERT(type.IsFinalized() && !type.IsMalformed() && !type.IsMalbounded());
-  ASSERT(!type.IsObjectType() && !type.IsDynamicType() && !type.IsVoidType());
+  ASSERT(!type.IsObjectType() && !type.IsDynamicType());
   const Register kInstantiatorTypeArgumentsReg = R1;
   const Register kFunctionTypeArgumentsReg = R2;
   __ PushPair(kFunctionTypeArgumentsReg, kInstantiatorTypeArgumentsReg);
@@ -645,8 +650,7 @@ void FlowGraphCompiler::GenerateAssertAssignable(TokenPosition token_pos,
   ASSERT(dst_type.IsFinalized());
   // Assignable check is skipped in FlowGraphBuilder, not here.
   ASSERT(dst_type.IsMalformedOrMalbounded() ||
-         (!dst_type.IsDynamicType() && !dst_type.IsObjectType() &&
-          !dst_type.IsVoidType()));
+         (!dst_type.IsDynamicType() && !dst_type.IsObjectType()));
   const Register kInstantiatorTypeArgumentsReg = R1;
   const Register kFunctionTypeArgumentsReg = R2;
   __ PushPair(kFunctionTypeArgumentsReg, kInstantiatorTypeArgumentsReg);

runtime/vm/flow_graph_compiler_ia32.cc

@@ -538,6 +538,11 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateInlineInstanceof(
     Label* is_instance_lbl,
     Label* is_not_instance_lbl) {
   __ Comment("InlineInstanceof");
+  if (type.IsVoidType()) {
+    // A non-null value is returned from a void function, which will result in a
+    // type error. A null value is handled prior to executing this inline code.
+    return SubtypeTestCache::null();
+  }
   if (type.IsInstantiated()) {
     const Class& type_class = Class::ZoneHandle(zone(), type.type_class());
     // A class equality check is only applicable with a dst type (not a
@@ -580,7 +585,7 @@ void FlowGraphCompiler::GenerateInstanceOf(TokenPosition token_pos,
                                            const AbstractType& type,
                                            LocationSummary* locs) {
   ASSERT(type.IsFinalized() && !type.IsMalformedOrMalbounded());
-  ASSERT(!type.IsObjectType() && !type.IsDynamicType() && !type.IsVoidType());
+  ASSERT(!type.IsObjectType() && !type.IsDynamicType());
 
   __ pushl(EDX);  // Store instantiator type arguments.
   __ pushl(ECX);  // Store function type arguments.
@@ -661,8 +666,7 @@ void FlowGraphCompiler::GenerateAssertAssignable(TokenPosition token_pos,
   ASSERT(dst_type.IsFinalized());
   // Assignable check is skipped in FlowGraphBuilder, not here.
   ASSERT(dst_type.IsMalformedOrMalbounded() ||
-         (!dst_type.IsDynamicType() && !dst_type.IsObjectType() &&
-          !dst_type.IsVoidType()));
+         (!dst_type.IsDynamicType() && !dst_type.IsObjectType()));
   __ pushl(EDX);  // Store instantiator type arguments.
   __ pushl(ECX);  // Store function type arguments.
   // A null object is always assignable and is returned as result.

runtime/vm/flow_graph_compiler_mips.cc

@@ -520,6 +520,11 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateInlineInstanceof(
     Label* is_instance_lbl,
     Label* is_not_instance_lbl) {
   __ Comment("InlineInstanceof");
+  if (type.IsVoidType()) {
+    // A non-null value is returned from a void function, which will result in a
+    // type error. A null value is handled prior to executing this inline code.
+    return SubtypeTestCache::null();
+  }
   if (type.IsInstantiated()) {
     const Class& type_class = Class::ZoneHandle(zone(), type.type_class());
     // A class equality check is only applicable with a dst type (not a
@@ -562,7 +567,7 @@ void FlowGraphCompiler::GenerateInstanceOf(TokenPosition token_pos,
                                            const AbstractType& type,
                                            LocationSummary* locs) {
   ASSERT(type.IsFinalized() && !type.IsMalformed() && !type.IsMalbounded());
-  ASSERT(!type.IsObjectType() && !type.IsDynamicType() && !type.IsVoidType());
+  ASSERT(!type.IsObjectType() && !type.IsDynamicType());
 
   // Preserve instantiator type arguments (A1) and function type arguments (A2).
   __ addiu(SP, SP, Immediate(-2 * kWordSize));
@@ -648,8 +653,7 @@ void FlowGraphCompiler::GenerateAssertAssignable(TokenPosition token_pos,
   ASSERT(dst_type.IsFinalized());
   // Assignable check is skipped in FlowGraphBuilder, not here.
   ASSERT(dst_type.IsMalformedOrMalbounded() ||
-         (!dst_type.IsDynamicType() && !dst_type.IsObjectType() &&
-          !dst_type.IsVoidType()));
+         (!dst_type.IsDynamicType() && !dst_type.IsObjectType()));
 
   // Preserve instantiator type arguments (A1) and function type arguments (A2).
   __ addiu(SP, SP, Immediate(-2 * kWordSize));

runtime/vm/flow_graph_compiler_x64.cc

@@ -533,6 +533,11 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateInlineInstanceof(
     Label* is_instance_lbl,
     Label* is_not_instance_lbl) {
   __ Comment("InlineInstanceof");
+  if (type.IsVoidType()) {
+    // A non-null value is returned from a void function, which will result in a
+    // type error. A null value is handled prior to executing this inline code.
+    return SubtypeTestCache::null();
+  }
   if (type.IsInstantiated()) {
     const Class& type_class = Class::ZoneHandle(zone(), type.type_class());
     // A class equality check is only applicable with a dst type (not a
@@ -575,7 +580,7 @@ void FlowGraphCompiler::GenerateInstanceOf(TokenPosition token_pos,
                                            const AbstractType& type,
                                            LocationSummary* locs) {
   ASSERT(type.IsFinalized() && !type.IsMalformedOrMalbounded());
-  ASSERT(!type.IsObjectType() && !type.IsDynamicType() && !type.IsVoidType());
+  ASSERT(!type.IsObjectType() && !type.IsDynamicType());
 
   __ pushq(RDX);  // Store instantiator type arguments.
   __ pushq(RCX);  // Store function type arguments.
@@ -584,8 +589,8 @@ void FlowGraphCompiler::GenerateInstanceOf(TokenPosition token_pos,
   // If type is instantiated and non-parameterized, we can inline code
   // checking whether the tested instance is a Smi.
   if (type.IsInstantiated()) {
-    // A null object is only an instance of Null, Object, void and dynamic.
-    // Object void and dynamic have already been checked above (if the type is
+    // A null object is only an instance of Null, Object, and dynamic.
+    // Object and dynamic have already been checked above (if the type is
     // instantiated). So we can return false here if the instance is null,
     // unless the type is Null (and if the type is instantiated).
     // We can only inline this null check if the type is instantiated at compile
@@ -654,8 +659,7 @@ void FlowGraphCompiler::GenerateAssertAssignable(TokenPosition token_pos,
   ASSERT(dst_type.IsFinalized());
   // Assignable check is skipped in FlowGraphBuilder, not here.
   ASSERT(dst_type.IsMalformedOrMalbounded() ||
-         (!dst_type.IsDynamicType() && !dst_type.IsObjectType() &&
-          !dst_type.IsVoidType()));
+         (!dst_type.IsDynamicType() && !dst_type.IsObjectType()));
   __ pushq(RDX);  // Store instantiator type arguments.
   __ pushq(RCX);  // Store function type arguments.
   // A null object is always assignable and is returned as result.

runtime/vm/flow_graph_type_propagator.cc

@@ -597,7 +597,7 @@ intptr_t CompileType::ToNullableCid() {
     } else if (type_->IsMalformed()) {
       cid_ = kDynamicCid;
     } else if (type_->IsVoidType()) {
-      cid_ = kDynamicCid;
+      cid_ = kNullCid;
     } else if (type_->IsFunctionType() || type_->IsDartFunctionType()) {
       cid_ = kClosureCid;
     } else if (type_->HasResolvedTypeClass()) {
@@ -684,7 +684,7 @@ bool CompileType::CanComputeIsInstanceOf(const AbstractType& type,
     return false;
   }
 
-  if (type.IsDynamicType() || type.IsObjectType() || type.IsVoidType()) {
+  if (type.IsDynamicType() || type.IsObjectType()) {
     *is_instance = true;
     return true;
   }
@@ -696,6 +696,11 @@ bool CompileType::CanComputeIsInstanceOf(const AbstractType& type,
   // Consider the compile type of the value.
   const AbstractType& compile_type = *ToAbstractType();
 
+  // The compile-type of a value should never be void. The result of a void
+  // function must always be null, which was checked to be null at the return
+  // statement inside the function.
+  ASSERT(!compile_type.IsVoidType());
+
   if (compile_type.IsMalformedOrMalbounded()) {
     return false;
   }
@@ -710,6 +715,12 @@ bool CompileType::CanComputeIsInstanceOf(const AbstractType& type,
     return true;
   }
 
+  // A non-null value is not an instance of void.
+  if (type.IsVoidType()) {
+    *is_instance = IsNull();
+    return HasDecidableNullability();
+  }
+
   // If the value can be null then we can't eliminate the
   // check unless null is allowed.
   if (is_nullable_ && !is_nullable) {
@@ -726,6 +737,11 @@ bool CompileType::IsMoreSpecificThan(const AbstractType& other) {
     return false;
   }
 
+  if (other.IsVoidType()) {
+    // The only value assignable to void is null.
+    return IsNull();
+  }
+
   return ToAbstractType()->IsMoreSpecificThan(other, NULL, NULL, Heap::kOld);
 }
 
@@ -927,6 +943,11 @@ CompileType AssertAssignableInstr::ComputeType() const {
     return *value_type;
   }
 
+  if (dst_type().IsVoidType()) {
+    // The only value assignable to void is null.
+    return CompileType::Null();
+  }
+
   return CompileType::Create(value_type->ToCid(), dst_type());
 }
 
@@ -1017,9 +1038,13 @@ CompileType StaticCallInstr::ComputeType() const {
   }
 
   if (Isolate::Current()->type_checks()) {
+    // Void functions are known to return null, which is checked at the return
+    // from the function.
     const AbstractType& result_type =
         AbstractType::ZoneHandle(function().result_type());
-    return CompileType::FromAbstractType(result_type);
+    return CompileType::FromAbstractType(
+        result_type.IsVoidType() ? AbstractType::ZoneHandle(Type::NullType())
+                                 : result_type);
   }
 
   return CompileType::Dynamic();

runtime/vm/object.cc

@@ -3820,6 +3820,7 @@ bool Class::TypeTestNonRecursive(const Class& cls,
   Zone* zone = Thread::Current()->zone();
   Class& thsi = Class::Handle(zone, cls.raw());
   while (true) {
+    ASSERT(!thsi.IsVoidClass());
     // Check for DynamicType.
     // Each occurrence of DynamicType in type T is interpreted as the dynamic
     // type, a supertype of all types.
@@ -3838,15 +3839,10 @@ bool Class::TypeTestNonRecursive(const Class& cls,
       return test_kind == Class::kIsSubtypeOf;
     }
     // Check for ObjectType. Any type that is not NullType or DynamicType
-    // (already checked above), is more specific than ObjectType/VoidType.
-    if (other.IsObjectClass() || other.IsVoidClass()) {
+    // (already checked above), is more specific than ObjectType.
+    if (other.IsObjectClass()) {
       return true;
     }
-    // If other is neither Object, dynamic or void, then ObjectType/VoidType
-    // can't be a subtype of other.
-    if (thsi.IsObjectClass() || thsi.IsVoidClass()) {
-      return false;
-    }
     // Check for reflexivity.
     if (thsi.raw() == other.raw()) {
       const intptr_t num_type_params = thsi.NumTypeParameters();
@@ -15834,7 +15830,7 @@ bool Instance::IsInstanceOf(
   ASSERT(!other.IsMalformed());
   ASSERT(!other.IsMalbounded());
   if (other.IsVoidType()) {
-    return true;
+    return false;
   }
   Zone* zone = Thread::Current()->zone();
   const Class& cls = Class::Handle(zone, clazz());
@@ -16652,12 +16648,9 @@ bool AbstractType::TypeTest(TypeTestKind test_kind,
     return false;
   }
   // Any type is a subtype of (and is more specific than) Object and dynamic.
-  // As of Dart 1.24, void is dynamically treated like Object (except when
-  // comparing function-types).
   // As of Dart 1.5, the Null type is a subtype of (and is more specific than)
   // any type.
-  if (other.IsObjectType() || other.IsDynamicType() || other.IsVoidType() ||
-      IsNullType()) {
+  if (other.IsObjectType() || other.IsDynamicType() || IsNullType()) {
     return true;
   }
   Zone* zone = Thread::Current()->zone();