Second try: Fix resolution and canonicalization of typedefs and function types

in preparation of the new typedef syntax.

R=asiva@google.com

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

runtime/lib/mirrors.cc

@@ -852,7 +852,7 @@ DEFINE_NATIVE_ENTRY(DeclarationMirror_metadata, 1) {
   if (decl.IsClass()) {
     klass ^= decl.raw();
     library = klass.library();
-  } else if (decl.IsFunction()) {
+  } else if (decl.IsFunction() && !Function::Cast(decl).IsSignatureFunction()) {
     klass = Function::Cast(decl).origin();
     library = klass.library();
   } else if (decl.IsField()) {

runtime/vm/class_finalizer.cc

@@ -549,11 +549,34 @@ void ClassFinalizer::ResolveType(const Class& cls, const AbstractType& type) {
   // Resolve signature if function type.
   if (type.IsFunctionType()) {
     const Function& signature = Function::Handle(Type::Cast(type).signature());
-    const Class& scope_class = Class::Handle(type.type_class());
-    if (scope_class.IsTypedefClass()) {
-      ResolveSignature(scope_class, signature);
+    Type& signature_type = Type::Handle(signature.SignatureType());
+    if (signature_type.raw() != type.raw()) {
+      ResolveType(cls, signature_type);
     } else {
-      ResolveSignature(cls, signature);
+      const Class& scope_class = Class::Handle(type.type_class());
+      if (scope_class.IsTypedefClass()) {
+        ResolveSignature(scope_class, signature);
+      } else {
+        ResolveSignature(cls, signature);
+      }
+      if (signature.IsSignatureFunction()) {
+        // Drop fields that are not necessary anymore after resolution.
+        // The parent function, owner, and token position of a shared
+        // canonical function type are meaningless, since the canonical
+        // representent is picked arbitrarily.
+        signature.set_parent_function(Function::Handle());
+        // TODO(regis): As long as we support metadata in typedef signatures,
+        // we cannot reset these fields used to reparse a typedef.
+        // Note that the scope class of a typedef function type is always
+        // preserved as the typedef class (not reset to _Closure class), thereby
+        // preventing sharing of canonical function types between typedefs.
+        // Not being shared, these fields are therefore always meaningful for
+        // typedefs.
+        if (!scope_class.IsTypedefClass()) {
+          signature.set_owner(Object::Handle());
+          signature.set_token_pos(TokenPosition::kNoSource);
+        }
+      }
     }
   }
 }
@@ -2331,8 +2354,9 @@ void ClassFinalizer::FinalizeTypesInClass(const Class& cls) {
     cls.set_mixin(mixin_type);
   }
   if (cls.IsTypedefClass()) {
-    const Function& signature = Function::Handle(cls.signature_function());
+    Function& signature = Function::Handle(cls.signature_function());
     Type& type = Type::Handle(signature.SignatureType());
+    ASSERT(type.signature() == signature.raw());
 
     // Check for illegal self references.
     GrowableArray<intptr_t> visited_aliases;
@@ -2350,7 +2374,12 @@ void ClassFinalizer::FinalizeTypesInClass(const Class& cls) {
 
     // Resolve and finalize the signature type of this typedef.
     type ^= FinalizeType(cls, type, kCanonicalizeWellFormed);
+
+    // If a different canonical signature type is returned, update the signature
+    // function of the typedef.
+    signature = type.signature();
     signature.SetSignatureType(type);
+    cls.set_signature_function(signature);
 
     // Closure instances do not refer to this typedef as their class, so there
     // is no need to add this typedef class to the subclasses of _Closure.

runtime/vm/isolate_reload.cc

@@ -1596,6 +1596,9 @@ class MarkFunctionsForRecompilation : public ObjectVisitor {
     handle_ = obj;
     if (handle_.IsFunction()) {
       const Function& func = Function::Cast(handle_);
+      if (func.IsSignatureFunction()) {
+        return;
+      }
 
       // Switch to unoptimized code or the lazy compilation stub.
       func.SwitchToLazyCompiledUnoptimizedCode();

runtime/vm/kernel_to_il.cc

@@ -3953,7 +3953,7 @@ void DartTypeTranslator::VisitFunctionType(FunctionType* node) {
   // So we convert malformed return/parameter types to `dynamic`.
   TypeParameterScope scope(this, &node->type_parameters());
 
-  const Function& signature_function = Function::ZoneHandle(
+  Function& signature_function = Function::ZoneHandle(
       Z, Function::NewSignatureFunction(*active_class_->klass,
                                         TokenPosition::kNoSource));
 
@@ -4008,8 +4008,10 @@ void DartTypeTranslator::VisitFunctionType(FunctionType* node) {
   if (finalize_) {
     signature_type ^= ClassFinalizer::FinalizeType(
         *active_class_->klass, signature_type, ClassFinalizer::kCanonicalize);
+    // Do not refer to signature_function anymore, since it may have been
+    // replaced during canonicalization.
+    signature_function = Function::null();
   }
-  signature_function.SetSignatureType(signature_type);
 
   result_ = signature_type.raw();
 }

runtime/vm/object.cc

@@ -5563,6 +5563,7 @@ void Function::SetSignatureType(const Type& value) const {
   ASSERT(!obj.IsNull());
   if (IsSignatureFunction()) {
     SignatureData::Cast(obj).set_signature_type(value);
+    ASSERT(!value.IsCanonical() || (value.signature() == this->raw()));
   } else {
     ASSERT(IsClosureFunction());
     ClosureData::Cast(obj).set_signature_type(value);
@@ -5722,7 +5723,7 @@ void Function::set_name(const String& value) const {
 
 
 void Function::set_owner(const Object& value) const {
-  ASSERT(!value.IsNull());
+  ASSERT(!value.IsNull() || IsSignatureFunction());
   StorePointer(&raw_ptr()->owner_, value.raw());
 }
 
@@ -6948,6 +6949,10 @@ bool Function::HasInstantiatedSignature() const {
 
 
 RawClass* Function::Owner() const {
+  if (raw_ptr()->owner_ == Object::null()) {
+    ASSERT(IsSignatureFunction());
+    return Class::null();
+  }
   if (raw_ptr()->owner_->IsClass()) {
     return Class::RawCast(raw_ptr()->owner_);
   }
@@ -6958,6 +6963,10 @@ RawClass* Function::Owner() const {
 
 
 RawClass* Function::origin() const {
+  if (raw_ptr()->owner_ == Object::null()) {
+    ASSERT(IsSignatureFunction());
+    return Class::null();
+  }
   if (raw_ptr()->owner_->IsClass()) {
     return Class::RawCast(raw_ptr()->owner_);
   }
@@ -6982,6 +6991,10 @@ RawScript* Function::script() const {
     return Function::Handle(parent_function()).script();
   }
   const Object& obj = Object::Handle(raw_ptr()->owner_);
+  if (obj.IsNull()) {
+    ASSERT(IsSignatureFunction());
+    return Script::null();
+  }
   if (obj.IsClass()) {
     return Class::Cast(obj).script();
   }
@@ -7299,21 +7312,6 @@ void SignatureData::set_parent_function(const Function& value) const {
 
 void SignatureData::set_signature_type(const Type& value) const {
   StorePointer(&raw_ptr()->signature_type_, value.raw());
-// If the signature type is resolved, the parent function is not needed
-// anymore (type parameters may be declared by generic parent functions).
-// Keeping the parent function can unnecessarily pull more objects into a
-// snapshot. Also, the parent function is meaningless once the signature type
-// is canonicalized.
-
-// TODO(rmacnak): Keeping the parent function for unresolved signature types
-// is causing a tree shaking issue in AOT. Please, investigate.
-#if 0
-  if (value.IsResolved()) {
-    set_parent_function(Function::Handle());
-  }
-#else
-  set_parent_function(Function::Handle());
-#endif
 }
 
 
@@ -17129,6 +17127,7 @@ RawAbstractType* Type::CloneUnfinalized() const {
     const Class& owner = Class::Handle(zone, fun.Owner());
     Function& fun_clone = Function::Handle(
         zone, Function::NewSignatureFunction(owner, TokenPosition::kNoSource));
+    // TODO(regis): Handle cloning of a generic function type.
     AbstractType& type = AbstractType::Handle(zone, fun.result_type());
     type = type.CloneUnfinalized();
     fun_clone.set_result_type(type);
@@ -17145,6 +17144,7 @@ RawAbstractType* Type::CloneUnfinalized() const {
     }
     fun_clone.set_parameter_names(Array::Handle(zone, fun.parameter_names()));
     clone.set_signature(fun_clone);
+    fun_clone.SetSignatureType(clone);
   }
   clone.SetIsResolved();
   return clone.raw();
@@ -17341,6 +17341,10 @@ RawAbstractType* Type::Canonicalize(TrailPtr trail) const {
         }
         sig_fun.set_parameter_names(Array::Handle(zone, fun.parameter_names()));
         set_signature(sig_fun);
+        // Note that the signature type of the signature function may be
+        // different than the type being canonicalized.
+        // Consider F<int> being canonicalized, with F being a typedef and F<T>
+        // being its signature type.
       }
     }
 

runtime/vm/object.h

@@ -2993,6 +2993,7 @@ class Function : public Object {
   // RawFunction::VisitFunctionPointers accesses the private constructor of
   // Function.
   friend class RawFunction;
+  friend class ClassFinalizer;  // To reset parent_function.
 };
 
 

runtime/vm/object_service.cc

@@ -243,6 +243,11 @@ void PatchClass::PrintJSONImpl(JSONStream* stream, bool ref) const {
 static void AddFunctionServiceId(const JSONObject& jsobj,
                                  const Function& f,
                                  const Class& cls) {
+  if (cls.IsNull()) {
+    ASSERT(f.IsSignatureFunction());
+    jsobj.AddServiceId(f);
+    return;
+  }
   // Special kinds of functions use indices in their respective lists.
   intptr_t id = -1;
   const char* selector = NULL;
@@ -279,10 +284,13 @@ static void AddFunctionServiceId(const JSONObject& jsobj,
 
 void Function::PrintJSONImpl(JSONStream* stream, bool ref) const {
   Class& cls = Class::Handle(Owner());
-  ASSERT(!cls.IsNull());
-  Error& err = Error::Handle();
-  err ^= cls.EnsureIsFinalized(Thread::Current());
-  ASSERT(err.IsNull());
+  if (!cls.IsNull()) {
+    Error& err = Error::Handle();
+    err ^= cls.EnsureIsFinalized(Thread::Current());
+    ASSERT(err.IsNull());
+  } else {
+    ASSERT(IsSignatureFunction());
+  }
   JSONObject jsobj(stream);
   AddCommonObjectProperties(&jsobj, "Function", ref);
   AddFunctionServiceId(jsobj, *this, cls);
@@ -292,11 +300,13 @@ void Function::PrintJSONImpl(JSONStream* stream, bool ref) const {
   const Function& parent = Function::Handle(parent_function());
   if (!parent.IsNull()) {
     jsobj.AddProperty("owner", parent);
-  } else if (cls.IsTopLevel()) {
-    const Library& library = Library::Handle(cls.library());
-    jsobj.AddProperty("owner", library);
-  } else {
-    jsobj.AddProperty("owner", cls);
+  } else if (!cls.IsNull()) {
+    if (cls.IsTopLevel()) {
+      const Library& library = Library::Handle(cls.library());
+      jsobj.AddProperty("owner", library);
+    } else {
+      jsobj.AddProperty("owner", cls);
+    }
   }
 
   const char* kind_string = Function::KindToCString(kind());
@@ -1099,6 +1109,7 @@ void AbstractType::PrintJSONImpl(JSONStream* stream, bool ref) const {
 
 
 void Type::PrintJSONImpl(JSONStream* stream, bool ref) const {
+  // TODO(regis): Function types are not handled properly.
   JSONObject jsobj(stream);
   PrintSharedInstanceJSON(&jsobj, ref);
   jsobj.AddProperty("kind", "Type");

runtime/vm/parser.cc

@@ -2055,7 +2055,7 @@ void Parser::ParseFormalParameter(bool allow_explicit_default_value,
       // signature functions (except typedef signature functions), therefore
       // we do not need to keep the correct script via a patch class. Use the
       // actual current class as owner of the signature function.
-      const Function& signature_function =
+      Function& signature_function =
           Function::Handle(Z, Function::NewSignatureFunction(
                                   current_class(), TokenPosition::kNoSource));
       signature_function.set_parent_function(innermost_function());
@@ -2095,7 +2095,9 @@ void Parser::ParseFormalParameter(bool allow_explicit_default_value,
       if (!is_top_level_) {
         signature_type ^= ClassFinalizer::FinalizeType(
             current_class(), signature_type, ClassFinalizer::kCanonicalize);
-        signature_function.SetSignatureType(signature_type);
+        // Do not refer to signature_function anymore, since it may have been
+        // replaced during canonicalization.
+        signature_function = Function::null();
       }
       ASSERT(is_top_level_ || signature_type.IsFinalized());
       // A signature type itself cannot be malformed or malbounded, only its
@@ -11893,6 +11895,8 @@ AstNode* Parser::ParsePostfixExpr() {
 // Not all involved type classes may get resolved yet, but at least type
 // parameters will get resolved, thereby relieving the class
 // finalizer from resolving type parameters out of context.
+// TODO(regis): Refactor this code which is partially duplicated in the class
+// finalizer, paying attention to type parameter resolution and mixin library.
 void Parser::ResolveType(ClassFinalizer::FinalizationKind finalization,
                          AbstractType* type) {
   ASSERT(finalization >= ClassFinalizer::kResolveTypeParameters);
@@ -11970,6 +11974,12 @@ void Parser::ResolveType(ClassFinalizer::FinalizationKind finalization,
     if (!resolved_type_class.IsNull()) {
       // Replace unresolved class with resolved type class.
       parameterized_type.set_type_class(resolved_type_class);
+      // Promote type to a function type in case its type class is a typedef.
+      if (resolved_type_class.IsTypedefClass()) {
+        ASSERT(!parameterized_type.IsFunctionType());
+        parameterized_type.set_signature(
+            Function::Handle(Z, resolved_type_class.signature_function()));
+      }
     } else if (finalization >= ClassFinalizer::kCanonicalize) {
       ClassFinalizer::FinalizeMalformedType(
           Error::Handle(Z),  // No previous error.
@@ -11978,6 +11988,9 @@ void Parser::ResolveType(ClassFinalizer::FinalizationKind finalization,
       return;
     }
   }
+  if (finalization > ClassFinalizer::kResolveTypeParameters) {
+    type->SetIsResolved();
+  }
   // Resolve type arguments, if any.
   if (type->arguments() != TypeArguments::null()) {
     const TypeArguments& arguments =
@@ -11990,6 +12003,45 @@ void Parser::ResolveType(ClassFinalizer::FinalizationKind finalization,
       arguments.SetTypeAt(i, type_argument);
     }
   }
+  if (type->IsFunctionType()) {
+    const Function& signature =
+        Function::Handle(Z, Type::Cast(*type).signature());
+    Type& signature_type = Type::Handle(Z, signature.SignatureType());
+    if (signature_type.raw() != type->raw()) {
+      ResolveType(finalization, &signature_type);
+    } else {
+      AbstractType& type = AbstractType::Handle(signature.result_type());
+      ResolveType(finalization, &type);
+      signature.set_result_type(type);
+      const intptr_t num_parameters = signature.NumParameters();
+      for (intptr_t i = 0; i < num_parameters; i++) {
+        type = signature.ParameterTypeAt(i);
+        ResolveType(finalization, &type);
+        signature.SetParameterTypeAt(i, type);
+      }
+      if (signature.IsSignatureFunction()) {
+        // Drop fields that are not necessary anymore after resolution.
+        // The parent function, owner, and token position of a shared
+        // canonical function type are meaningless, since the canonical
+        // representent is picked arbitrarily.
+        signature.set_parent_function(Function::Handle(Z));
+        // TODO(regis): As long as we support metadata in typedef signatures,
+        // we cannot reset these fields used to reparse a typedef.
+        // Note that the scope class of a typedef function type is always
+        // preserved as the typedef class (not reset to _Closure class), thereby
+        // preventing sharing of canonical function types between typedefs.
+        // Not being shared, these fields are therefore always meaningful for
+        // typedefs.
+        if (type.HasResolvedTypeClass()) {
+          const Class& scope_class = Class::Handle(Z, type.type_class());
+          if (!scope_class.IsTypedefClass()) {
+            signature.set_owner(Object::Handle(Z));
+            signature.set_token_pos(TokenPosition::kNoSource);
+          }
+        }
+      }
+    }
+  }
 }
 
 

tests/lib/lib.status

@@ -426,6 +426,5 @@ mirrors/load_library_test: Crash # Deferred loading
 
 [ $hot_reload ]
 mirrors/generic_bounded_test/02: Fail # Type equality - Issue 26869
-mirrors/typedef_reflected_type_test/01: Fail # Type equality - Issue 26869
 mirrors/generic_bounded_by_type_parameter_test/02: Fail # Type equality - Issue 26869
 async/timer_regress22626_test: Pass, RuntimeError # Timing dependent.