swiftlang · jrose-apple · May 20, 2019 · May 20, 2019
diff --git a/include/swift/Serialization/ModuleFormat.h b/include/swift/Serialization/ModuleFormat.h
@@ -52,7 +52,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
 /// describe what change you made. The content of this comment isn't important;
 /// it just ensures a conflict if two people change the module format.
 /// Don't worry about adhering to the 80-column limit for this line.
-const uint16_t SWIFTMODULE_VERSION_MINOR = 489; // mangled class names as vtable keys
+const uint16_t SWIFTMODULE_VERSION_MINOR = 490; // dependency types for structs
 
 using DeclIDField = BCFixed<31>;
 
@@ -948,7 +948,8 @@ namespace decls_block {
     GenericEnvironmentIDField, // generic environment
     AccessLevelField,       // access level
     BCVBR<4>,               // number of conformances
-    BCArray<TypeIDField>    // inherited types
+    BCVBR<4>,               // number of inherited types
+    BCArray<TypeIDField>    // inherited types, followed by dependency types
     // Trailed by the generic parameters (if any), the members record, and
     // finally conformance info (if any).
   >;
@@ -981,7 +982,8 @@ namespace decls_block {
     TypeIDField,            // superclass
     AccessLevelField,       // access level
     BCVBR<4>,               // number of conformances
-    BCArray<TypeIDField>    // inherited types
+    BCVBR<4>,               // number of inherited types
+    BCArray<TypeIDField>    // inherited types, followed by dependency types
     // Trailed by the generic parameters (if any), the members record, and
     // finally conformance info (if any).
   >;

diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp
@@ -2497,14 +2497,25 @@ class swift::DeclDeserializer {
     bool isObjC;
     GenericEnvironmentID genericEnvID;
     uint8_t rawAccessLevel;
-    unsigned numConformances;
-    ArrayRef<uint64_t> rawInheritedIDs;
+    unsigned numConformances, numInheritedTypes;
+    ArrayRef<uint64_t> rawInheritedAndDependencyIDs;
 
     decls_block::StructLayout::readRecord(scratch, nameID, contextID,
                                           isImplicit, isObjC, genericEnvID,
                                           rawAccessLevel,
-                                          numConformances,
-                                          rawInheritedIDs);
+                                          numConformances, numInheritedTypes,
+                                          rawInheritedAndDependencyIDs);
+
+    Identifier name = MF.getIdentifier(nameID);
+
+    for (TypeID dependencyID :
+           rawInheritedAndDependencyIDs.slice(numInheritedTypes)) {
+      auto dependency = MF.getTypeChecked(dependencyID);
+      if (!dependency) {
+        return llvm::make_error<TypeError>(
+            name, takeErrorInfo(dependency.takeError()));
+      }
+    }
 
     auto DC = MF.getDeclContext(contextID);
     if (declOrOffset.isComplete())
@@ -2514,10 +2525,8 @@ class swift::DeclDeserializer {
     if (declOrOffset.isComplete())
       return declOrOffset;
 
-    auto theStruct = MF.createDecl<StructDecl>(SourceLoc(),
-                                               MF.getIdentifier(nameID),
-                                               SourceLoc(), None, genericParams,
-                                               DC);
+    auto theStruct = MF.createDecl<StructDecl>(SourceLoc(), name, SourceLoc(),
+                                               None, genericParams, DC);
     declOrOffset = theStruct;
 
     // Read the generic environment.
@@ -2537,7 +2546,8 @@ class swift::DeclDeserializer {
 
     theStruct->computeType();
 
-    handleInherited(theStruct, rawInheritedIDs);
+    handleInherited(theStruct,
+                    rawInheritedAndDependencyIDs.slice(0, numInheritedTypes));
 
     theStruct->setMemberLoader(&MF, MF.DeclTypeCursor.GetCurrentBitNo());
     skipRecord(MF.DeclTypeCursor, decls_block::MEMBERS);
@@ -3424,15 +3434,27 @@ class swift::DeclDeserializer {
     GenericEnvironmentID genericEnvID;
     TypeID superclassID;
     uint8_t rawAccessLevel;
-    unsigned numConformances;
-    ArrayRef<uint64_t> rawInheritedIDs;
+    unsigned numConformances, numInheritedTypes;
+    ArrayRef<uint64_t> rawInheritedAndDependencyIDs;
     decls_block::ClassLayout::readRecord(scratch, nameID, contextID,
                                          isImplicit, isObjC,
                                          requiresStoredPropertyInits,
                                          inheritsSuperclassInitializers,
                                          genericEnvID, superclassID,
                                          rawAccessLevel, numConformances,
-                                         rawInheritedIDs);
+                                         numInheritedTypes,
+                                         rawInheritedAndDependencyIDs);
+
+    Identifier name = MF.getIdentifier(nameID);
+
+    for (TypeID dependencyID :
+           rawInheritedAndDependencyIDs.slice(numInheritedTypes)) {
+      auto dependency = MF.getTypeChecked(dependencyID);
+      if (!dependency) {
+        return llvm::make_error<TypeError>(
+            name, takeErrorInfo(dependency.takeError()));
+      }
+    }
 
     auto DC = MF.getDeclContext(contextID);
     if (declOrOffset.isComplete())
@@ -3442,10 +3464,8 @@ class swift::DeclDeserializer {
     if (declOrOffset.isComplete())
       return declOrOffset;
 
-    auto theClass = MF.createDecl<ClassDecl>(SourceLoc(),
-                                             MF.getIdentifier(nameID),
-                                             SourceLoc(), None, genericParams,
-                                             DC);
+    auto theClass = MF.createDecl<ClassDecl>(SourceLoc(), name, SourceLoc(),
+                                             None, genericParams, DC);
     declOrOffset = theClass;
 
     MF.configureGenericEnvironment(theClass, genericEnvID);
@@ -3469,7 +3489,8 @@ class swift::DeclDeserializer {
 
     theClass->computeType();
 
-    handleInherited(theClass, rawInheritedIDs);
+    handleInherited(theClass,
+                    rawInheritedAndDependencyIDs.slice(0, numInheritedTypes));
 
     theClass->setMemberLoader(&MF, MF.DeclTypeCursor.GetCurrentBitNo());
     theClass->setHasDestructor();

diff --git a/lib/Serialization/Serialization.cpp b/lib/Serialization/Serialization.cpp
@@ -2697,22 +2697,39 @@ void Serializer::writeForeignErrorConvention(const ForeignErrorConvention &fec){
 /// - \p decl is declared in an extension of a type that depends on
 ///   \p problemContext
 static bool contextDependsOn(const NominalTypeDecl *decl,
-                             const ModuleDecl *problemModule) {
-  return decl->getParentModule() == problemModule;
+                             const DeclContext *problemContext) {
+  SmallPtrSet<const ExtensionDecl *, 8> seenExtensionDCs;
+
+  const DeclContext *dc = decl;
+  do {
+    if (dc == problemContext)
+      return true;
+
+    if (auto *extension = dyn_cast<ExtensionDecl>(dc)) {
+      if (extension->isChildContextOf(problemContext))
+        return true;
+
+      // Avoid cycles when Left.Nested depends on Right.Nested somehow.
+      bool isNewlySeen = seenExtensionDCs.insert(extension).second;
+      if (!isNewlySeen)
+        break;
+      dc = extension->getSelfNominalTypeDecl();
+
+    } else {
+      dc = dc->getParent();
+    }
+  } while (dc);
+
+  return false;
 }
 
 static void collectDependenciesFromType(llvm::SmallSetVector<Type, 4> &seen,
                                         Type ty,
-                                        const ModuleDecl *excluding) {
+                                        const DeclContext *excluding) {
   ty.visit([&](Type next) {
     auto *nominal = next->getAnyNominal();
     if (!nominal)
       return;
-    // FIXME: Types in the same module are still important for enums. It's
-    // possible an enum element has a payload that references a type declaration
-    // from the same module that can't be imported (for whatever reason).
-    // However, we need a more robust handling of deserialization dependencies
-    // that can handle circularities. rdar://problem/32359173
     if (contextDependsOn(nominal, excluding))
       return;
     seen.insert(nominal->getDeclaredInterfaceType());
@@ -2722,7 +2739,7 @@ static void collectDependenciesFromType(llvm::SmallSetVector<Type, 4> &seen,
 static void
 collectDependenciesFromRequirement(llvm::SmallSetVector<Type, 4> &seen,
                                    const Requirement &req,
-                                   const ModuleDecl *excluding) {
+                                   const DeclContext *excluding) {
   collectDependenciesFromType(seen, req.getFirstType(), excluding);
   if (req.getKind() != RequirementKind::Layout)
     collectDependenciesFromType(seen, req.getSecondType(), excluding);
@@ -3123,12 +3140,20 @@ void Serializer::writeDecl(const Decl *D) {
     auto conformances = theStruct->getLocalConformances(
                           ConformanceLookupKind::All, nullptr);
 
-    SmallVector<TypeID, 4> inheritedTypes;
+    SmallVector<TypeID, 4> inheritedAndDependencyTypes;
     for (auto inherited : theStruct->getInherited()) {
       assert(!inherited.getType()->hasArchetype());
-      inheritedTypes.push_back(addTypeRef(inherited.getType()));
+      inheritedAndDependencyTypes.push_back(addTypeRef(inherited.getType()));
     }
 
+    llvm::SmallSetVector<Type, 4> dependencyTypes;
+    for (Requirement req : theStruct->getGenericRequirements()) {
+      collectDependenciesFromRequirement(dependencyTypes, req,
+                                         /*excluding*/nullptr);
+    }
+    for (Type ty : dependencyTypes)
+      inheritedAndDependencyTypes.push_back(addTypeRef(ty));
+
     uint8_t rawAccessLevel =
       getRawStableAccessLevel(theStruct->getFormalAccess());
 
@@ -3142,7 +3167,8 @@ void Serializer::writeDecl(const Decl *D) {
                                             theStruct->getGenericEnvironment()),
                              rawAccessLevel,
                              conformances.size(),
-                             inheritedTypes);
+                             theStruct->getInherited().size(),
+                             inheritedAndDependencyTypes);
 
 
     writeGenericParams(theStruct->getGenericParams());
@@ -3170,6 +3196,11 @@ void Serializer::writeDecl(const Decl *D) {
     for (const EnumElementDecl *nextElt : theEnum->getAllElements()) {
       if (!nextElt->hasAssociatedValues())
         continue;
+      // FIXME: Types in the same module are still important for enums. It's
+      // possible an enum element has a payload that references a type
+      // declaration from the same module that can't be imported (for whatever
+      // reason). However, we need a more robust handling of deserialization
+      // dependencies that can handle circularities. rdar://problem/32359173
       collectDependenciesFromType(dependencyTypes,
                                   nextElt->getArgumentInterfaceType(),
                                   /*excluding*/theEnum->getParentModule());
@@ -3214,11 +3245,28 @@ void Serializer::writeDecl(const Decl *D) {
     auto conformances = theClass->getLocalConformances(
                           ConformanceLookupKind::NonInherited, nullptr);
 
-    SmallVector<TypeID, 4> inheritedTypes;
+    SmallVector<TypeID, 4> inheritedAndDependencyTypes;
     for (auto inherited : theClass->getInherited()) {
       assert(!inherited.getType()->hasArchetype());
-      inheritedTypes.push_back(addTypeRef(inherited.getType()));
+      inheritedAndDependencyTypes.push_back(addTypeRef(inherited.getType()));
+    }
+
+    llvm::SmallSetVector<Type, 4> dependencyTypes;
+    if (theClass->hasSuperclass()) {
+      // FIXME: Nested types can still be a problem here: it's possible that (for
+      // whatever reason) they won't be able to be deserialized, in which case
+      // we'll be in trouble forming the actual superclass type. However, we
+      // need a more robust handling of deserialization dependencies that can
+      // handle circularities. rdar://problem/50835214
+      collectDependenciesFromType(dependencyTypes, theClass->getSuperclass(),
+                                  /*excluding*/theClass);
+    }
+    for (Requirement req : theClass->getGenericRequirements()) {
+      collectDependenciesFromRequirement(dependencyTypes, req,
+                                         /*excluding*/nullptr);
     }
+    for (Type ty : dependencyTypes)
+      inheritedAndDependencyTypes.push_back(addTypeRef(ty));
 
     uint8_t rawAccessLevel =
       getRawStableAccessLevel(theClass->getFormalAccess());
@@ -3240,7 +3288,8 @@ void Serializer::writeDecl(const Decl *D) {
                             addTypeRef(theClass->getSuperclass()),
                             rawAccessLevel,
                             conformances.size(),
-                            inheritedTypes);
+                            theClass->getInherited().size(),
+                            inheritedAndDependencyTypes);
 
     writeGenericParams(theClass->getGenericParams());
     writeMembers(id, theClass->getMembers(), true);

diff --git a/test/Serialization/Recovery/Inputs/custom-modules/SuperclassObjC.h b/test/Serialization/Recovery/Inputs/custom-modules/SuperclassObjC.h
@@ -0,0 +1,9 @@
+@interface Root
+- (instancetype)init;
+@end
+
+#if !BAD
+@interface DisappearingSuperclass : Root
+@end
+#else
+#endif
diff --git a/test/Serialization/Recovery/Inputs/custom-modules/module.modulemap b/test/Serialization/Recovery/Inputs/custom-modules/module.modulemap
@@ -9,6 +9,7 @@ module IndirectlyImported {
 module Overrides { header "Overrides.h" }
 module ProtocolInheritance { header "ProtocolInheritance.h" }
 module RenameAcrossVersions { header "RenameAcrossVersions.h" }
+module SuperclassObjC { header "SuperclassObjC.h" }
 module Typedefs { header "Typedefs.h" }
 module TypeRemovalObjC { header "TypeRemovalObjC.h" }
 module Types { header "Types.h" }
diff --git a/test/Serialization/Recovery/superclass.swift b/test/Serialization/Recovery/superclass.swift
@@ -0,0 +1,56 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-sil -enable-objc-interop -o - -emit-module-path %t/Lib.swiftmodule -module-name Lib -I %S/Inputs/custom-modules %s > /dev/null
+
+// RUN: %target-swift-ide-test -enable-objc-interop -source-filename=x -print-module -module-to-print Lib -I %t -I %S/Inputs/custom-modules | %FileCheck -check-prefix CHECK -check-prefix CHECK-ALWAYS %s
+
+// RUN: %target-swift-ide-test -enable-objc-interop -source-filename=x -print-module -module-to-print Lib -I %t -I %S/Inputs/custom-modules -Xcc -DBAD | %FileCheck -check-prefix CHECK-RECOVERY -check-prefix CHECK-ALWAYS %s
+
+// RUN: %target-swift-frontend -typecheck -enable-objc-interop -I %t -I %S/Inputs/custom-modules -Xcc -DBAD -DTEST %s -verify
+
+#if TEST
+
+import Lib
+
+func use(_: C2_CRTPish) {}
+func use(_: C3_NestingCRTPish) {}
+func use(_: C4_ExtensionNestingCRTPish) {}
+
+// FIXME: Better to import the class and make it unavailable.
+func use(_: A1_Sub) {} // expected-error {{use of undeclared type 'A1_Sub'}}
+func use(_: A2_Grandchild) {} // expected-error {{use of undeclared type 'A2_Grandchild'}}
+
+#else // TEST
+
+import SuperclassObjC
+
+// CHECK-LABEL: class A1_Sub : DisappearingSuperclass {
+// CHECK-RECOVERY-NOT: class A1_Sub
+public class A1_Sub: DisappearingSuperclass {}
+// CHECK-LABEL: class A2_Grandchild : A1_Sub {
+// CHECK-RECOVERY-NOT: class A2_Grandchild
+public class A2_Grandchild: A1_Sub {}
+
+// CHECK-LABEL: class B1_ConstrainedGeneric<T> where T : DisappearingSuperclass {
+// CHECK-RECOVERY-NOT: class B1_ConstrainedGeneric
+public class B1_ConstrainedGeneric<T> where T: DisappearingSuperclass {}
+
+// CHECK-LABEL: struct B2_ConstrainedGeneric<T> where T : DisappearingSuperclass {
+// CHECK-RECOVERY-NOT: struct B2_ConstrainedGeneric
+public struct B2_ConstrainedGeneric<T> where T: DisappearingSuperclass {}
+
+// CHECK-ALWAYS-LABEL: class C1_GenericBase<T> {
+public class C1_GenericBase<T> {}
+
+// CHECK-ALWAYS-LABEL: class C2_CRTPish : C1_GenericBase<C2_CRTPish> {
+public class C2_CRTPish: C1_GenericBase<C2_CRTPish> {}
+// CHECK-ALWAYS-LABEL: class C3_NestingCRTPish : C1_GenericBase<C3_NestingCRTPish.Nested> {
+public class C3_NestingCRTPish: C1_GenericBase<C3_NestingCRTPish.Nested> {
+  public struct Nested {}
+}
+// CHECK-ALWAYS-LABEL: class C4_ExtensionNestingCRTPish : C1_GenericBase<C4_ExtensionNestingCRTPish.Nested> {
+public class C4_ExtensionNestingCRTPish: C1_GenericBase<C4_ExtensionNestingCRTPish.Nested> {}
+extension C4_ExtensionNestingCRTPish {
+  public struct Nested {}
+}
+
+#endif // TEST
diff --git a/test/Serialization/Recovery/typedefs-in-enums.swift b/test/Serialization/Recovery/typedefs-in-enums.swift
@@ -110,4 +110,31 @@ public func producesOkayEnum() -> OkayEnum { return .noPayload }
 // CHECK-LABEL: func producesOkayEnum() -> OkayEnum
 // CHECK-RECOVERY-LABEL: func producesOkayEnum() -> OkayEnum
 
+
+extension Int /* or any imported type, really */ {
+  public enum OkayEnumWithSelfRefs {
+    public struct Nested {}
+    indirect case selfRef(OkayEnumWithSelfRefs)
+    case nested(Nested)
+  }
+}
+// CHECK-LABEL: extension Int {
+//  CHECK-NEXT:   enum OkayEnumWithSelfRefs {
+//  CHECK-NEXT:     struct Nested {
+//  CHECK-NEXT:       init()
+//  CHECK-NEXT:     }
+//  CHECK-NEXT:     indirect case selfRef(Int.OkayEnumWithSelfRefs)
+//  CHECK-NEXT:     case nested(Int.OkayEnumWithSelfRefs.Nested)
+//  CHECK-NEXT:   }
+//  CHECK-NEXT: }
+// CHECK-RECOVERY-LABEL: extension Int {
+//  CHECK-RECOVERY-NEXT:   enum OkayEnumWithSelfRefs {
+//  CHECK-RECOVERY-NEXT:     struct Nested {
+//  CHECK-RECOVERY-NEXT:       init()
+//  CHECK-RECOVERY-NEXT:     }
+//  CHECK-RECOVERY-NEXT:     indirect case selfRef(Int.OkayEnumWithSelfRefs)
+//  CHECK-RECOVERY-NEXT:     case nested(Int.OkayEnumWithSelfRefs.Nested)
+//  CHECK-RECOVERY-NEXT:   }
+//  CHECK-RECOVERY-NEXT: }
+
 #endif // TEST
diff --git a/validation-test/Serialization/Inputs/custom-modules/SuperclassObjC.h b/validation-test/Serialization/Inputs/custom-modules/SuperclassObjC.h
@@ -0,0 +1,9 @@
+@interface Root
+- (instancetype)init;
+@end
+
+#if !BAD
+@interface DisappearingSuperclass : Root
+@end
+#else
+#endif
diff --git a/validation-test/Serialization/Inputs/custom-modules/module.modulemap b/validation-test/Serialization/Inputs/custom-modules/module.modulemap
@@ -1 +1,2 @@
 module rdar40899824Helper { header "rdar40899824Helper.h" }
+module SuperclassObjC { header "SuperclassObjC.h" }
Original file line number	Diff line number	Diff line change
		@@ -1 +1,2 @@
		module rdar40899824Helper { header "rdar40899824Helper.h" }
		module SuperclassObjC { header "SuperclassObjC.h" }