From 3598d4340d8b125e5366ef834ce13b14cb9923d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Crelier?= Date: Thu, 6 Feb 2020 01:45:29 +0000 Subject: [PATCH] [VM/nnbd] Consider nullability of type parameters in function type tests. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This also fixes issue #40259. Change-Id: I2e603e927e98270bb24b726444490993e6360f97 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134572 Commit-Queue: Régis Crelier Reviewed-by: Liam Appelbe Reviewed-by: Alexander Markov --- runtime/lib/object.cc | 6 +- runtime/vm/class_finalizer.cc | 8 +- runtime/vm/compiler/backend/il.cc | 4 +- .../compiler/frontend/flow_graph_builder.cc | 4 +- runtime/vm/object.cc | 117 +++++++++++------- runtime/vm/object.h | 31 +++-- 6 files changed, 108 insertions(+), 62 deletions(-) diff --git a/runtime/lib/object.cc b/runtime/lib/object.cc index 5fbc5e1ae43..85e286356b6 100644 --- a/runtime/lib/object.cc +++ b/runtime/lib/object.cc @@ -110,7 +110,7 @@ DEFINE_NATIVE_ENTRY(Object_haveSameRuntimeType, 0, 2) { const AbstractType& right_type = AbstractType::Handle(right.GetType(Heap::kNew)); return Bool::Get( - left_type.IsEquivalent(right_type, /* syntactically = */ true)) + left_type.IsEquivalent(right_type, TypeEquality::kSyntactical)) .raw(); } @@ -129,7 +129,7 @@ DEFINE_NATIVE_ENTRY(Object_haveSameRuntimeType, 0, 2) { const intptr_t num_type_params = cls.NumTypeParameters(); return Bool::Get(left_type_arguments.IsSubvectorEquivalent( right_type_arguments, num_type_args - num_type_params, - num_type_params, /* syntactically = */ true)) + num_type_params, TypeEquality::kSyntactical)) .raw(); } @@ -205,7 +205,7 @@ DEFINE_NATIVE_ENTRY(Type_equality, 0, 2) { if (type.raw() == other.raw()) { return Bool::True().raw(); } - return Bool::Get(type.IsEquivalent(other, /* syntactically = */ true)).raw(); + return Bool::Get(type.IsEquivalent(other, TypeEquality::kSyntactical)).raw(); } DEFINE_NATIVE_ENTRY(Internal_inquireIs64Bit, 0, 0) { diff --git a/runtime/vm/class_finalizer.cc b/runtime/vm/class_finalizer.cc index 5f72da18620..c6a5e03b5ea 100644 --- a/runtime/vm/class_finalizer.cc +++ b/runtime/vm/class_finalizer.cc @@ -378,9 +378,11 @@ void ClassFinalizer::CheckRecursiveType(const Class& cls, if ((pending_type.raw() != type.raw()) && pending_type.IsType() && (pending_type.type_class() == type_cls.raw())) { pending_arguments = pending_type.arguments(); + // By using TypeEquality::kIgnoreNullability, we throw a wider net and + // may reject more problematic declarations. if (!pending_arguments.IsSubvectorEquivalent( arguments, first_type_param, num_type_params, - /* syntactically = */ true) && + TypeEquality::kIgnoreNullability) && !pending_arguments.IsSubvectorInstantiated(first_type_param, num_type_params)) { const TypeArguments& instantiated_arguments = TypeArguments::Handle( @@ -394,9 +396,11 @@ void ClassFinalizer::CheckRecursiveType(const Class& cls, NNBDMode::kLegacyLib, Object::null_type_arguments(), Object::null_type_arguments(), kNoneFree, NULL, Heap::kNew)); + // By using TypeEquality::kIgnoreNullability, we throw a wider net and + // may reject more problematic declarations. if (!instantiated_pending_arguments.IsSubvectorEquivalent( instantiated_arguments, first_type_param, num_type_params, - /* syntactically = */ true)) { + TypeEquality::kIgnoreNullability)) { const String& type_name = String::Handle(zone, type.Name()); ReportError(cls, type.token_pos(), "illegal recursive type '%s'", type_name.ToCString()); diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc index 9d2846efa54..2fcc1811f85 100644 --- a/runtime/vm/compiler/backend/il.cc +++ b/runtime/vm/compiler/backend/il.cc @@ -387,7 +387,9 @@ bool HierarchyInfo::CanUseSubtypeRangeCheckFor(const AbstractType& type) { // arguments are not "dynamic" but instantiated-to-bounds. const Type& rare_type = Type::Handle(zone, Type::RawCast(type_class.RareType())); - if (!rare_type.IsEquivalent(type, /* syntactically = */ true)) { + // TODO(regis): Revisit the usage of TypeEquality::kSyntactical when + // implementing strong mode. + if (!rare_type.IsEquivalent(type, TypeEquality::kSyntactical)) { return false; } } diff --git a/runtime/vm/compiler/frontend/flow_graph_builder.cc b/runtime/vm/compiler/frontend/flow_graph_builder.cc index c10afcddfc9..9d8855f5702 100644 --- a/runtime/vm/compiler/frontend/flow_graph_builder.cc +++ b/runtime/vm/compiler/frontend/flow_graph_builder.cc @@ -380,7 +380,9 @@ bool SimpleInstanceOfType(const AbstractType& type) { // type arguments, then we can still use the _simpleInstanceOf // implementation (see also runtime/lib/object.cc:Object_SimpleInstanceOf). const auto& rare_type = AbstractType::Handle(type_class.RareType()); - return rare_type.IsEquivalent(type, /* syntactically = */ true); + // TODO(regis): Revisit the usage of TypeEquality::kSyntactical when + // implementing strong mode. + return rare_type.IsEquivalent(type, TypeEquality::kSyntactical); } // Finally a simple class for instance of checking. diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index 03c278cc139..bb16eb83802 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -5625,7 +5625,7 @@ void TypeArguments::PrintSubvectorName(intptr_t from_index, bool TypeArguments::IsSubvectorEquivalent(const TypeArguments& other, intptr_t from_index, intptr_t len, - bool syntactically, + TypeEquality kind, TrailPtr trail) const { if (this->raw() == other.raw()) { return true; @@ -5643,7 +5643,7 @@ bool TypeArguments::IsSubvectorEquivalent(const TypeArguments& other, type = TypeAt(i); other_type = other.TypeAt(i); // Still unfinalized vectors should not be considered equivalent. - if (type.IsNull() || !type.IsEquivalent(other_type, syntactically, trail)) { + if (type.IsNull() || !type.IsEquivalent(other_type, kind, trail)) { return false; } } @@ -7721,7 +7721,8 @@ bool Function::IsContravariantParameter(NNBDMode mode, return other_param_type.IsSubtypeOf(mode, param_type, space); } -bool Function::HasSameTypeParametersAndBounds(const Function& other) const { +bool Function::HasSameTypeParametersAndBounds(const Function& other, + TypeEquality kind) const { Thread* thread = Thread::Current(); Zone* zone = thread->zone(); @@ -7747,10 +7748,7 @@ bool Function::HasSameTypeParametersAndBounds(const Function& other) const { ASSERT(bound.IsFinalized()); other_bound = other_type_param.bound(); ASSERT(other_bound.IsFinalized()); - // TODO(dartbug.com/40259): Treat top types as equivalent and disregard - // nullability in weak mode. - const bool syntactically = !FLAG_strong_non_nullable_type_checks; - if (!bound.IsEquivalent(other_bound, syntactically)) { + if (!bound.IsEquivalent(other_bound, kind)) { return false; } } @@ -7785,7 +7783,8 @@ bool Function::IsSubtypeOf(NNBDMode mode, return false; } // Check the type parameters and bounds of generic functions. - if (!HasSameTypeParametersAndBounds(other)) { + if (!HasSameTypeParametersAndBounds(other, + TypeEquality::kSubtypeNullability)) { return false; } Thread* thread = Thread::Current(); @@ -17982,7 +17981,7 @@ void AbstractType::SetIsBeingFinalized() const { } bool AbstractType::IsEquivalent(const Instance& other, - bool syntactically, + TypeEquality kind, TrailPtr trail) const { // AbstractType is an abstract class. UNREACHABLE(); @@ -18422,7 +18421,8 @@ bool AbstractType::IsSubtypeOf(NNBDMode mode, const TypeParameter& type_param = TypeParameter::Cast(*this); if (other.IsTypeParameter()) { const TypeParameter& other_type_param = TypeParameter::Cast(other); - if (type_param.Equals(other_type_param)) { + if (type_param.IsEquivalent(other_type_param, + TypeEquality::kSubtypeNullability)) { return true; } if (type_param.IsFunctionTypeParameter() && @@ -18441,6 +18441,10 @@ bool AbstractType::IsSubtypeOf(NNBDMode mode, const int other_offset = other_sig_fun.NumParentTypeParameters(); if (type_param.index() - offset == other_type_param.index() - other_offset) { + if (FLAG_strong_non_nullable_type_checks && type_param.IsNullable() && + other_type_param.IsNonNullable()) { + return false; + } return true; } } @@ -18843,7 +18847,7 @@ RawAbstractType* Type::InstantiateFrom( } bool Type::IsEquivalent(const Instance& other, - bool syntactically, + TypeEquality kind, TrailPtr trail) const { ASSERT(!IsNull()); if (raw() == other.raw()) { @@ -18854,7 +18858,7 @@ bool Type::IsEquivalent(const Instance& other, const AbstractType& other_ref_type = AbstractType::Handle(TypeRef::Cast(other).type()); ASSERT(!other_ref_type.IsTypeRef()); - return IsEquivalent(other_ref_type, syntactically, trail); + return IsEquivalent(other_ref_type, kind, trail); } if (!other.IsType()) { return false; @@ -18866,18 +18870,30 @@ bool Type::IsEquivalent(const Instance& other, if (type_class_id() != other_type.type_class_id()) { return false; } - Nullability this_type_nullability = nullability(); - Nullability other_type_nullability = other_type.nullability(); - if (syntactically) { - if (this_type_nullability == Nullability::kLegacy) { - this_type_nullability = Nullability::kNonNullable; + if (kind != TypeEquality::kIgnoreNullability) { + Nullability this_type_nullability = nullability(); + Nullability other_type_nullability = other_type.nullability(); + if (kind == TypeEquality::kSubtypeNullability) { + if (FLAG_strong_non_nullable_type_checks && + this_type_nullability == Nullability::kNullable && + other_type_nullability == Nullability::kNonNullable) { + return false; + } + } else { + if (kind == TypeEquality::kSyntactical) { + if (this_type_nullability == Nullability::kLegacy) { + this_type_nullability = Nullability::kNonNullable; + } + if (other_type_nullability == Nullability::kLegacy) { + other_type_nullability = Nullability::kNonNullable; + } + } else { + ASSERT(kind == TypeEquality::kCanonical); + } + if (this_type_nullability != other_type_nullability) { + return false; + } } - if (other_type_nullability == Nullability::kLegacy) { - other_type_nullability = Nullability::kNonNullable; - } - } - if (this_type_nullability != other_type_nullability) { - return false; } if (!IsFinalized() || !other_type.IsFinalized()) { return false; // Too early to decide if equal. @@ -18909,8 +18925,8 @@ bool Type::IsEquivalent(const Instance& other, return false; } } else if (!type_args.IsSubvectorEquivalent(other_type_args, from_index, - num_type_params, - syntactically, trail)) { + num_type_params, kind, + trail)) { return false; } #ifdef DEBUG @@ -18926,7 +18942,7 @@ bool Type::IsEquivalent(const Instance& other, for (intptr_t i = 0; i < from_index; i++) { type_arg = type_args.TypeAt(i); other_type_arg = other_type_args.TypeAt(i); - ASSERT(type_arg.IsEquivalent(other_type_arg, syntactically, trail)); + ASSERT(type_arg.IsEquivalent(other_type_arg, kind, trail)); } } #endif @@ -18947,7 +18963,7 @@ bool Type::IsEquivalent(const Instance& other, // Compare function type parameters and their bounds. // Check the type parameters and bounds of generic functions. - if (!sig_fun.HasSameTypeParametersAndBounds(other_sig_fun)) { + if (!sig_fun.HasSameTypeParametersAndBounds(other_sig_fun, kind)) { return false; } @@ -18989,7 +19005,7 @@ bool Type::IsEquivalent(const Instance& other, for (intptr_t i = 0; i < num_params; i++) { param_type = sig_fun.ParameterTypeAt(i); other_param_type = other_sig_fun.ParameterTypeAt(i); - if (!param_type.IsEquivalent(other_param_type, syntactically)) { + if (!param_type.IsEquivalent(other_param_type, kind)) { return false; } } @@ -19354,7 +19370,7 @@ bool TypeRef::IsInstantiated(Genericity genericity, } bool TypeRef::IsEquivalent(const Instance& other, - bool syntactically, + TypeEquality kind, TrailPtr trail) const { if (raw() == other.raw()) { return true; @@ -19366,8 +19382,7 @@ bool TypeRef::IsEquivalent(const Instance& other, return true; } const AbstractType& ref_type = AbstractType::Handle(type()); - return !ref_type.IsNull() && - ref_type.IsEquivalent(other, syntactically, trail); + return !ref_type.IsNull() && ref_type.IsEquivalent(other, kind, trail); } RawTypeRef* TypeRef::InstantiateFrom( @@ -19535,7 +19550,7 @@ bool TypeParameter::IsInstantiated(Genericity genericity, } bool TypeParameter::IsEquivalent(const Instance& other, - bool syntactically, + TypeEquality kind, TrailPtr trail) const { if (raw() == other.raw()) { return true; @@ -19545,7 +19560,7 @@ bool TypeParameter::IsEquivalent(const Instance& other, const AbstractType& other_ref_type = AbstractType::Handle(TypeRef::Cast(other).type()); ASSERT(!other_ref_type.IsTypeRef()); - return IsEquivalent(other_ref_type, syntactically, trail); + return IsEquivalent(other_ref_type, kind, trail); } if (!other.IsTypeParameter()) { return false; @@ -19558,18 +19573,32 @@ bool TypeParameter::IsEquivalent(const Instance& other, if (parameterized_function() != other_type_param.parameterized_function()) { return false; } - Nullability this_type_param_nullability = nullability(); - Nullability other_type_param_nullability = other_type_param.nullability(); - if (syntactically) { - if (this_type_param_nullability == Nullability::kLegacy) { - this_type_param_nullability = Nullability::kNonNullable; + if (kind != TypeEquality::kIgnoreNullability) { + Nullability this_type_param_nullability = nullability(); + Nullability other_type_param_nullability = other_type_param.nullability(); + if (kind == TypeEquality::kSubtypeNullability) { + if (FLAG_strong_non_nullable_type_checks && + this_type_param_nullability == Nullability::kNullable && + other_type_param_nullability == Nullability::kNonNullable) { + return false; + } + } else { + if (kind == TypeEquality::kSyntactical) { + if (this_type_param_nullability == Nullability::kLegacy || + this_type_param_nullability == Nullability::kUndetermined) { + this_type_param_nullability = Nullability::kNonNullable; + } + if (other_type_param_nullability == Nullability::kLegacy || + other_type_param_nullability == Nullability::kUndetermined) { + other_type_param_nullability = Nullability::kNonNullable; + } + } else { + ASSERT(kind == TypeEquality::kCanonical); + } + if (this_type_param_nullability != other_type_param_nullability) { + return false; + } } - if (other_type_param_nullability == Nullability::kLegacy) { - other_type_param_nullability = Nullability::kNonNullable; - } - } - if (this_type_param_nullability != other_type_param_nullability) { - return false; } if (IsFinalized() == other_type_param.IsFinalized()) { return (index() == other_type_param.index()); diff --git a/runtime/vm/object.h b/runtime/vm/object.h index 86b29895825..6b7c8bb4550 100644 --- a/runtime/vm/object.h +++ b/runtime/vm/object.h @@ -874,6 +874,14 @@ enum class Nullability : int8_t { kLegacy = 3, }; +// Equality kind between types. +enum class TypeEquality { + kCanonical = 0, + kSyntactical = 1, + kSubtypeNullability = 2, + kIgnoreNullability = 3, +}; + // The NNBDMode is passed to routines performing type reification and/or subtype // tests. The mode reflects the opted-in status of the library performing type // reification and/or subtype tests. @@ -2514,7 +2522,8 @@ class Function : public Object { // Returns true if this function has the same number of type parameters with // equal bounds as the other function. Type parameter names are ignored. - bool HasSameTypeParametersAndBounds(const Function& other) const; + bool HasSameTypeParametersAndBounds(const Function& other, + TypeEquality kind) const; // Return the number of type parameters declared in parent generic functions. intptr_t NumParentTypeParameters() const; @@ -7031,19 +7040,19 @@ class TypeArguments : public Instance { // Check if the vectors are equal (they may be null). bool Equals(const TypeArguments& other) const { return IsSubvectorEquivalent(other, 0, IsNull() ? 0 : Length(), - /* syntactically = */ false); + TypeEquality::kCanonical); } bool IsEquivalent(const TypeArguments& other, - bool syntactically, + TypeEquality kind, TrailPtr trail = NULL) const { - return IsSubvectorEquivalent(other, 0, IsNull() ? 0 : Length(), - syntactically, trail); + return IsSubvectorEquivalent(other, 0, IsNull() ? 0 : Length(), kind, + trail); } bool IsSubvectorEquivalent(const TypeArguments& other, intptr_t from_index, intptr_t len, - bool syntactically, + TypeEquality kind, TrailPtr trail = NULL) const; // Check if the vector is instantiated (it must not be null). @@ -7233,10 +7242,10 @@ class AbstractType : public Instance { } virtual uint32_t CanonicalizeHash() const { return Hash(); } virtual bool Equals(const Instance& other) const { - return IsEquivalent(other, /* syntactically = */ false); + return IsEquivalent(other, TypeEquality::kCanonical); } virtual bool IsEquivalent(const Instance& other, - bool syntactically, + TypeEquality kind, TrailPtr trail = NULL) const; virtual bool IsRecursive() const; @@ -7482,7 +7491,7 @@ class Type : public AbstractType { intptr_t num_free_fun_type_params = kAllFree, TrailPtr trail = NULL) const; virtual bool IsEquivalent(const Instance& other, - bool syntactically, + TypeEquality kind, TrailPtr trail = NULL) const; virtual bool IsRecursive() const; @@ -7647,7 +7656,7 @@ class TypeRef : public AbstractType { intptr_t num_free_fun_type_params = kAllFree, TrailPtr trail = NULL) const; virtual bool IsEquivalent(const Instance& other, - bool syntactically, + TypeEquality kind, TrailPtr trail = NULL) const; virtual bool IsRecursive() const { return true; } virtual bool IsFunctionType() const { @@ -7731,7 +7740,7 @@ class TypeParameter : public AbstractType { intptr_t num_free_fun_type_params = kAllFree, TrailPtr trail = NULL) const; virtual bool IsEquivalent(const Instance& other, - bool syntactically, + TypeEquality kind, TrailPtr trail = NULL) const; virtual bool IsRecursive() const { return false; } virtual RawAbstractType* InstantiateFrom(