Improve upper bound finalization in VM and fix #25122.

Add regression test.

[email protected]

Review URL: https://codereview.chromium.org/1513493002 .

Author: crelier

runtime/lib/mirrors.cc

@@ -1206,6 +1206,11 @@ DEFINE_NATIVE_ENTRY(LibraryMirror_members, 2) {
 DEFINE_NATIVE_ENTRY(ClassMirror_type_variables, 1) {
   GET_NON_NULL_NATIVE_ARGUMENT(MirrorReference, ref, arguments->NativeArgAt(0));
   const Class& klass = Class::Handle(ref.GetClassReferent());
+  const Error& error = Error::Handle(zone, klass.EnsureIsFinalized(thread));
+  if (!error.IsNull()) {
+    Exceptions::PropagateError(error);
+    UNREACHABLE();
+  }
   return CreateTypeVariableList(klass);
 }
 

runtime/vm/class_finalizer.cc

@@ -1269,7 +1269,8 @@ void ClassFinalizer::FinalizeUpperBounds(const Class& cls) {
   for (intptr_t i = 0; i < num_type_params; i++) {
     type_param ^= type_params.TypeAt(i);
     bound = type_param.bound();
-    if (bound.IsFinalized() || bound.IsBeingFinalized()) {
+    // Bound may be finalized, but not canonical yet.
+    if (bound.IsCanonical() || bound.IsBeingFinalized()) {
       // A bound involved in F-bounded quantification may form a cycle.
       continue;
     }
@@ -2231,7 +2232,10 @@ void ClassFinalizer::FinalizeTypesInClass(const Class& cls) {
   FinalizeTypeParameters(cls);  // May change super type.
   super_class = cls.SuperClass();
   ASSERT(super_class.IsNull() || super_class.is_type_finalized());
-  ResolveUpperBounds(cls);
+  // Only resolving rather than finalizing the upper bounds here would result in
+  // instantiated type parameters of the super type to temporarily have
+  // unfinalized bounds. It is more efficient to finalize them early.
+  FinalizeUpperBounds(cls);
   // Finalize super type.
   AbstractType& super_type = AbstractType::Handle(cls.super_type());
   if (!super_type.IsNull()) {
@@ -2344,6 +2348,7 @@ void ClassFinalizer::FinalizeTypesInClass(const Class& cls) {
 void ClassFinalizer::FinalizeClass(const Class& cls) {
   Thread* thread = Thread::Current();
   HANDLESCOPE(thread);
+  ASSERT(cls.is_type_finalized());
   if (cls.is_finalized()) {
     return;
   }

runtime/vm/object.cc

@@ -3787,11 +3787,11 @@ bool Class::TypeTestNonRecursive(const Class& cls,
     }
     // Check for reflexivity.
     if (thsi.raw() == other.raw()) {
-      const intptr_t num_type_args = thsi.NumTypeArguments();
-      if (num_type_args == 0) {
+      const intptr_t num_type_params = thsi.NumTypeParameters();
+      if (num_type_params == 0) {
         return true;
       }
-      const intptr_t num_type_params = thsi.NumTypeParameters();
+      const intptr_t num_type_args = thsi.NumTypeArguments();
       const intptr_t from_index = num_type_args - num_type_params;
       // Since we do not truncate the type argument vector of a subclass (see
       // below), we only check a subvector of the proper length.
@@ -15630,8 +15630,8 @@ bool AbstractType::TypeTest(TypeTestKind test_kind,
                             const AbstractType& other,
                             Error* bound_error,
                             Heap::Space space) const {
-  ASSERT(IsResolved());
-  ASSERT(other.IsResolved());
+  ASSERT(IsFinalized());
+  ASSERT(other.IsFinalized());
   if (IsMalformed() || other.IsMalformed()) {
     // Malformed types involved in subtype tests should be handled specially
     // by the caller. Malformed types should only be encountered here in a
@@ -15685,6 +15685,15 @@ bool AbstractType::TypeTest(TypeTestKind test_kind,
       }
     }
     const AbstractType& bound = AbstractType::Handle(type_param.bound());
+    // We may be checking bounds at finalization time and can encounter
+    // a still unfinalized bound.
+    if (!bound.IsFinalized() && !bound.IsBeingFinalized()) {
+      ClassFinalizer::FinalizeType(
+          Class::Handle(type_param.parameterized_class()),
+          bound,
+          ClassFinalizer::kCanonicalize);
+      type_param.set_bound(bound);
+    }
     if (bound.IsMoreSpecificThan(other, bound_error)) {
       return true;
     }

tests/language/language_dart2js.status

@@ -53,6 +53,7 @@ mixin_super_2_test: CompileTimeError # Issue 23773
 mixin_super_bound2_test: CompileTimeError # Issue 23773
 mixin_super_use_test: CompileTimeError # Issue 23773
 mixin_superclass_test: CompileTimeError # Issue 23773
+regress_25122_test: Crash # Issue 25181
 
 ref_before_declaration_test/00: MissingCompileTimeError
 ref_before_declaration_test/01: MissingCompileTimeError

tests/language/regress_25122_test.dart

@@ -0,0 +1,10 @@
+// Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+main() {}
+
+class AbstractListMember<E, M extends AbstractListMember<E, M>> {}
+
+class RepoListMember<M extends RepoListMember<M>>
+    extends AbstractListMember<String, M> {}