Merge pull request swiftlang#28679 from CodaFi/latent-semantic-mapping

[ClangImporter] Block Re-Entrancy In Lookups

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->hasOwningModule()) {
+        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
@@ -3690,6 +3714,24 @@ void ClangImporter::Implementation::lookupAllObjCMembers(
   }
 }
 
+// Force the named member of the entire inheritance hierarchy to be loaded and
+// deserialized before loading the named member of this class. This allows the
+// decl members table to be warmed up and enables the correct identification of
+// overrides.
+static void loadNamedMemberOfSuperclassesIfNeeded(const ClassDecl *CD,
+                                                  DeclBaseName name) {
+  if (!CD)
+    return;
+
+  while ((CD = CD->getSuperclassDecl())) {
+    if (CD->hasClangNode()) {
+      auto ci = CD->getASTContext().getOrCreateLazyIterableContextData(
+          CD, /*lazyLoader=*/nullptr);
+      ci->loader->loadNamedMembers(CD, name, ci->memberData);
+    }
+  }
+}
+
 Optional<TinyPtrVector<ValueDecl *>>
 ClangImporter::Implementation::loadNamedMembers(
     const IterableDeclContext *IDC, DeclBaseName N, uint64_t contextData) {
@@ -3751,6 +3793,8 @@ ClangImporter::Implementation::loadNamedMembers(
 
   assert(isa<clang::ObjCContainerDecl>(CD) || isa<clang::NamespaceDecl>(CD));
 
+  loadNamedMemberOfSuperclassesIfNeeded(dyn_cast<ClassDecl>(D), N);
+
   TinyPtrVector<ValueDecl *> Members;
   for (auto entry : table->lookup(SerializedSwiftName(N),
                                   effectiveClangContext)) {

lib/ClangImporter/ImportDecl.cpp

@@ -2174,6 +2174,13 @@ namespace {
       return getVersion() == getActiveSwiftVersion();
     }
 
+    template <typename T>
+    T *recordMemberInContext(DeclContext *dc, T *member) {
+      assert(member && "Attempted to record null member!");
+      Impl.MembersForNominal[dc->getSelfNominalTypeDecl()].push_back(member);
+      return member;
+    }
+
     /// Import the name of the given entity.
     ///
     /// This version of importFullName introduces any context-specific
@@ -3734,7 +3741,7 @@ namespace {
       if (correctSwiftName)
         markAsVariant(result, *correctSwiftName);
 
-      return result;
+      return recordMemberInContext(dc, result);
     }
 
     void finishFuncDecl(const clang::FunctionDecl *decl,
@@ -4344,7 +4351,7 @@ namespace {
         }
       }
 
-      return result;
+      return recordMemberInContext(dc, result);
     }
 
   public:
@@ -5002,44 +5009,55 @@ namespace {
         return nullptr;
 
       VarDecl *overridden = nullptr;
-      if (dc->getSelfClassDecl()) {
-        // Check whether there is a function with the same name as this
-        // property. If so, suppress the property; the user will have to use
-        // the methods directly, to avoid ambiguities.
-        NominalTypeDecl *lookupContext = dc->getSelfNominalTypeDecl();
+      // Check whether there is a function with the same name as this
+      // property. If so, suppress the property; the user will have to use
+      // the methods directly, to avoid ambiguities.
+      if (auto *subject = dc->getSelfClassDecl()) {
+        bool foundMethod = false;
 
         if (auto *classDecl = dyn_cast<ClassDecl>(dc)) {
-          // If we're importing into the primary @interface for something, as
-          // opposed to an extension, make sure we don't try to load any
-          // categories...by just looking into the super type.
-          lookupContext = classDecl->getSuperclassDecl();
+          // Start looking into the superclass.
+          subject = classDecl->getSuperclassDecl();
         }
 
-        if (lookupContext) {
-          SmallVector<ValueDecl *, 2> lookup;
-          dc->lookupQualified(lookupContext, DeclNameRef(name),
-                              NL_QualifiedDefault | NL_KnownNoDependency,
-                              lookup);
-          bool foundMethod = false;
-          for (auto result : lookup) {
-            if (isa<FuncDecl>(result) &&
-                result->isInstanceMember() == decl->isInstanceProperty() &&
-                result->getFullName().getArgumentNames().empty())
-              foundMethod = true;
-
-            if (auto var = dyn_cast<VarDecl>(result)) {
+        for (; subject; (subject = subject->getSuperclassDecl())) {
+          llvm::SmallVector<ValueDecl *, 8> lookup;
+          auto found = Impl.MembersForNominal.find(subject);
+          if (found != Impl.MembersForNominal.end()) {
+            lookup.append(found->second.begin(), found->second.end());
+            namelookup::pruneLookupResultSet(dc, NL_QualifiedDefault, lookup);
+          }
+
+          for (auto *&result : lookup) {
+            // Skip declarations that don't match the name we're looking for.
+            if (result->getBaseName() != name)
+              continue;
+
+            if (auto *fd = dyn_cast<FuncDecl>(result)) {
+              if (fd->isInstanceMember() != decl->isInstanceProperty())
+                continue;
+
+              if (fd->getFullName().getArgumentNames().empty()) {
+                foundMethod = true;
+              }
+            } else {
+              auto *var = cast<VarDecl>(result);
+              if (var->isInstanceMember() != decl->isInstanceProperty())
+                continue;
+
               // If the selectors of the getter match in Objective-C, we have an
               // override.
-              if (var->isInstanceMember() == decl->isInstanceProperty() &&
-                  var->getObjCGetterSelector() ==
-                    Impl.importSelector(decl->getGetterName()))
+              if (var->getObjCGetterSelector() ==
+                  Impl.importSelector(decl->getGetterName())) {
                 overridden = var;
+              }
             }
           }
-          if (foundMethod && !overridden)
-            return nullptr;
         }
 
+        if (foundMethod && !overridden)
+          return nullptr;
+
         if (overridden) {
           const DeclContext *overrideContext = overridden->getDeclContext();
           // It's okay to compare interface types directly because Objective-C
@@ -5130,7 +5148,7 @@ namespace {
       if (correctSwiftName)
         markAsVariant(result, *correctSwiftName);
 
-      return result;
+      return recordMemberInContext(dc, result);
     }
 
     Decl *
@@ -8405,6 +8423,19 @@ createUnavailableDecl(Identifier name, DeclContext *dc, Type type,
   return var;
 }
 
+// Force the members of the entire inheritance hierarchy to be loaded and
+// deserialized before loading the members of this class. This allows the
+// decl members table to be warmed up and enables the correct identification of
+// overrides.
+static void loadAllMembersOfSuperclassesIfNeeded(const ClassDecl *CD) {
+  if (!CD)
+    return;
+
+  while ((CD = CD->getSuperclassDecl())) {
+    if (CD->hasClangNode())
+      CD->loadAllMembers();
+  }
+}
 
 void
 ClangImporter::Implementation::loadAllMembers(Decl *D, uint64_t extra) {
@@ -8418,6 +8449,7 @@ ClangImporter::Implementation::loadAllMembers(Decl *D, uint64_t extra) {
 
   // If not, we're importing globals-as-members into an extension.
   if (objcContainer) {
+    loadAllMembersOfSuperclassesIfNeeded(dyn_cast<ClassDecl>(D));
     loadAllMembersOfObjcContainer(D, objcContainer);
     return;
   }

lib/ClangImporter/ImporterImpl.h

@@ -447,6 +447,10 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   // Mapping from imported types to their raw value types.
   llvm::DenseMap<const NominalTypeDecl *, Type> RawTypes;
 
+  /// Keep track of all member declarations that have been imported into a nominal type.
+  llvm::DenseMap<const NominalTypeDecl *, std::vector<ValueDecl *>>
+      MembersForNominal;
+
   clang::CompilerInstance *getClangInstance() {
     return Instance.get();
   }