Work around extension installation order

Bridging headers are weird.

A general principle in the Clang Importer is that when it comes to the
notion of "overrides" or "redeclarations", it's always better to prefer
a declaration in the same translation unit, module, header, etc. over
a declaration outside of that thing.

This behavior, unfortunately, occurred as an emergent property of the
re-entrant lookup that was being performed when importing properties.
The behavior of the visible categories list is in "deserialization
order" - that is, a category defined in a header that imports a module
can potentially appear before the module has had its declarations fully
deserialized and installed.  We were assuming this order was instead
"source order", in which modules would be recursively expanded and their
declarations installed sequentially. If this assumption were to hold,
then the visible categories list would respect the principle above, and
we would see categories in defining modules and headers before we saw
categories in extant translation units.

This is not the case. Delayed deserialization of modules means
the first category that gets parsed actually wins. We were not noticing
this, however, because the few places where it actually mattered were
performing re-entrant lookup and were accidentally deserializing modules
and installing decls in this fictitious "source order".  Since we've
untangled this behavior, we now have to emulate the order the Importer
expects.

Introduce the notion of a "delayed category" - a category that is
defined outside of a module, usually by us. As the name implies, we'll
try to load the categories defined in modules first, then go after the
delayed categories defined in extant translation units.  This gives us
a stable "source order", preserves the Principle of Closest Definition,
and undoes the behavior change for overriden property declarations in
the previous commit.

Author: CodaFi

lib/ClangImporter/ClangImporter.cpp

@@ -2962,12 +2962,36 @@ void ClangImporter::loadExtensions(NominalTypeDecl *nominal,
   // For an Objective-C class, import all of the visible categories.
   if (auto objcClass = dyn_cast_or_null<clang::ObjCInterfaceDecl>(
                          effectiveClangContext.getAsDeclContext())) {
+    SmallVector<clang::NamedDecl *, 4> DelayedCategories;
+
     // Simply importing the categories adds them to the list of extensions.
     for (auto I = objcClass->visible_categories_begin(),
            E = objcClass->visible_categories_end();
          I != E; ++I) {
+      // Delay installing categories that don't have an owning module.
+      if (!I->getOwningModule()) {
+        DelayedCategories.push_back(*I);
+        continue;
+      }
+
       Impl.importDeclReal(*I, Impl.CurrentVersion);
     }
+
+    // Install all the delayed categories.
+    //
+    // The very notion of a delayed category is a result of an emergent behavior
+    // of the visible categories list and the order we import modules. The list
+    // appears in deserialization order rather than some "source order", so it's
+    // possible for, say, a bridging header to import a module that defines an
+    // interface and some categories, but for the categories in the bridging
+    // header to appear *before* the categories in the module - the imported
+    // module will be deserialized on demand. We take it on faith that if there
+    // is no owning module for a given category, that it was created in such a
+    // way, and thus we install it last to try to emulate what we want
+    // "source order" to mean.
+    for (const auto *DelayedCat : DelayedCategories) {
+      Impl.importDeclReal(DelayedCat, Impl.CurrentVersion);
+    }
   }
 
   // Dig through each of the Swift lookup tables, creating extensions

test/ClangImporter/objc_redeclared_properties_incompatible.swift

@@ -71,40 +71,31 @@ func testGenericWithoutInitializer(obj: PropertiesNoInitGeneric<Base>) {
   // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property
 }
 
-// N.B. We consider the category in the imported header Private.h as the
-// canonical declaration of these properties and drop the one in the framework.
-
 func testCategoryWithInitializer() {
   let obj = PropertiesInitCategory()
 
   let _: Int = obj.nullabilityChange
-  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
-  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'Base?' to specified type 'Int'
+  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
 
   let _: Int = obj.missingGenerics
-  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'GenericClass<Base>' to specified type 'Int'
-  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'GenericClass<AnyObject>' to specified type 'Int'
+  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'GenericClass<Base>' to specified type 'Int'
 
   let _: Int = obj.typeChange
-  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
-  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'PrivateSubclass' to specified type 'Int'
+  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
 
   obj.readwriteChange = Base() // CHECK-PRIVATE-NOT: [[@LINE]]:{{.+}}: error
   // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property
 }
 
 func testCategoryWithoutInitializer(obj: PropertiesNoInitCategory) {
   let _: Int = obj.nullabilityChange
-  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
-  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'Base?' to specified type 'Int'
+  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
 
   let _: Int = obj.missingGenerics
-  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'GenericClass<Base>' to specified type 'Int'
-  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'GenericClass<AnyObject>' to specified type 'Int'
+  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'GenericClass<Base>' to specified type 'Int'
 
   let _: Int = obj.typeChange
-  // CHECK-PUBLIC: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
-  // CHECK-PRIVATE: [[@LINE-2]]:20: error: cannot convert value of type 'PrivateSubclass' to specified type 'Int'
+  // CHECK: [[@LINE-1]]:20: error: cannot convert value of type 'Base' to specified type 'Int'
 
   obj.readwriteChange = Base() // CHECK-PRIVATE-NOT: [[@LINE]]:{{.+}}: error
   // CHECK-PUBLIC: [[@LINE-1]]:7: error: cannot assign to property: 'readwriteChange' is a get-only property