[c++-interop] For failed imports in ClangImporter, cache them regardless. (#41173)

As per SR-14137 this caches entries in ImportedDecls even when the
import failed.

Also have to mention I did this based on Thomas's PR #36747.

This should help us better handle complex templates and dependant types.

Author: plotfi

include/swift/ClangImporter/ClangImporter.h

@@ -441,9 +441,12 @@ class ClangImporter final : public ClangModuleLoader {
 
   std::string getClangModuleHash() const;
 
-  /// If we already imported a given decl, return the corresponding Swift decl.
-  /// Otherwise, return nullptr.
-  Decl *importDeclCached(const clang::NamedDecl *ClangDecl);
+  /// If we already imported a given decl successfully, return the corresponding
+  /// Swift decl as an Optional<Decl *>, but if we previously tried and failed
+  /// to import said decl then return nullptr.
+  /// Otherwise, if we have never encountered this decl previously then return
+  /// None.
+  Optional<Decl *> importDeclCached(const clang::NamedDecl *ClangDecl);
 
   // Returns true if it is expected that the macro is ignored.
   bool shouldIgnoreMacro(StringRef Name, const clang::MacroInfo *Macro);

lib/ClangImporter/ClangImporter.cpp

@@ -3631,7 +3631,8 @@ std::string ClangImporter::getClangModuleHash() const {
   return Impl.Invocation->getModuleHash(Impl.Instance->getDiagnostics());
 }
 
-Decl *ClangImporter::importDeclCached(const clang::NamedDecl *ClangDecl) {
+Optional<Decl *>
+ClangImporter::importDeclCached(const clang::NamedDecl *ClangDecl) {
   return Impl.importDeclCached(ClangDecl, Impl.CurrentVersion);
 }
 

lib/ClangImporter/ImportDecl.cpp

@@ -4841,8 +4841,9 @@ namespace {
 
       // While importing the DeclContext, we might have imported the decl
       // itself.
-      if (auto Known = Impl.importDeclCached(decl, getVersion()))
-        return Known;
+      auto Known = Impl.importDeclCached(decl, getVersion());
+      if (Known.hasValue())
+        return Known.getValue();
 
       ImportedName importedName;
       std::tie(importedName, std::ignore) = importFullName(decl);
@@ -4926,8 +4927,11 @@ namespace {
     }
 
   private:
-    static bool isAcceptableResult(Decl *fn,
-                                   Optional<AccessorInfo> accessorInfo) {
+    static bool isAcceptableResultOrNull(Decl *fn,
+                                         Optional<AccessorInfo> accessorInfo) {
+      if (nullptr == fn)
+        return true;
+
       // We can't safely re-use the same declaration if it disagrees
       // in accessor-ness.
       auto accessor = dyn_cast<AccessorDecl>(fn);
@@ -5038,7 +5042,7 @@ namespace {
                                               getVersion()});
         if (known != Impl.ImportedDecls.end()) {
           auto decl = known->second;
-          if (isAcceptableResult(decl, accessorInfo))
+          if (isAcceptableResultOrNull(decl, accessorInfo))
             return decl;
         }
       }
@@ -5056,7 +5060,7 @@ namespace {
       if (isActiveSwiftVersion()) {
         if (isMethodAlreadyImported(selector, importedName, isInstance, dc,
                                     [&](AbstractFunctionDecl *fn) {
-              return isAcceptableResult(fn, accessorInfo);
+              return isAcceptableResultOrNull(fn, accessorInfo);
             })) {
           return nullptr;
         }
@@ -5190,7 +5194,7 @@ namespace {
                                               getVersion()});
         if (known != Impl.ImportedDecls.end()) {
           auto decl = known->second;
-          if (isAcceptableResult(decl, accessorInfo))
+          if (isAcceptableResultOrNull(decl, accessorInfo))
             return decl;
         }
       }
@@ -5885,8 +5889,9 @@ namespace {
 
       // While importing the DeclContext, we might have imported the decl
       // itself.
-      if (auto Known = Impl.importDeclCached(decl, getVersion()))
-        return Known;
+      auto Known = Impl.importDeclCached(decl, getVersion());
+      if (Known.hasValue())
+        return Known.getValue();
 
       return importObjCPropertyDecl(decl, dc);
     }
@@ -8589,16 +8594,16 @@ void SwiftDeclConverter::importInheritedConstructors(
   }
 }
 
-Decl *ClangImporter::Implementation::importDeclCached(
+Optional<Decl *> ClangImporter::Implementation::importDeclCached(
     const clang::NamedDecl *ClangDecl,
     ImportNameVersion version,
     bool UseCanonical) {
   auto Known = ImportedDecls.find(
     { UseCanonical? ClangDecl->getCanonicalDecl(): ClangDecl, version });
-  if (Known != ImportedDecls.end())
-    return Known->second;
+  if (Known == ImportedDecls.end())
+    return None;
 
-  return nullptr;
+  return Known->second;
 }
 
 /// Checks if we don't need to import the typedef itself.  If the typedef
@@ -9451,11 +9456,12 @@ Decl *ClangImporter::Implementation::importDeclAndCacheImpl(
 
   auto Canon = cast<clang::NamedDecl>(UseCanonicalDecl? ClangDecl->getCanonicalDecl(): ClangDecl);
 
-  if (auto Known = importDeclCached(Canon, version, UseCanonicalDecl)) {
+  auto Known = importDeclCached(Canon, version, UseCanonicalDecl);
+  if (Known.hasValue()) {
     if (!SuperfluousTypedefsAreTransparent &&
         SuperfluousTypedefs.count(Canon))
       return nullptr;
-    return Known;
+    return Known.getValue();
   }
 
   bool TypedefIsSuperfluous = false;
@@ -9464,8 +9470,10 @@ Decl *ClangImporter::Implementation::importDeclAndCacheImpl(
   startedImportingEntity();
   Decl *Result = importDeclImpl(ClangDecl, version, TypedefIsSuperfluous,
                                 HadForwardDeclaration);
-  if (!Result)
+  if (!Result) {
+    ImportedDecls[{Canon, version}] = nullptr;
     return nullptr;
+  }
 
   if (TypedefIsSuperfluous) {
     SuperfluousTypedefs.insert(Canon);
@@ -9633,8 +9641,9 @@ ClangImporter::Implementation::importDeclForDeclContext(
 
   // There's a cycle. Is the declaration imported enough to break the cycle
   // gracefully? If so, we'll have it in the decl cache.
-  if (auto cached = importDeclCached(contextDecl, version, useCanonicalDecl))
-    return cached;
+  auto cached = importDeclCached(contextDecl, version, useCanonicalDecl);
+  if (cached.hasValue())
+    return cached.getValue();
 
   // Can't break it? Warn and return nullptr, which is at least better than
   // stack overflow by recursion.

lib/ClangImporter/ImporterImpl.h

@@ -958,8 +958,9 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
 
   /// If we already imported a given decl, return the corresponding Swift decl.
   /// Otherwise, return nullptr.
-  Decl *importDeclCached(const clang::NamedDecl *ClangDecl, Version version,
-                         bool UseCanonicalDecl = true);
+  Optional<Decl *> importDeclCached(const clang::NamedDecl *ClangDecl,
+                                    Version version,
+                                    bool UseCanonicalDecl = true);
 
   Decl *importDeclImpl(const clang::NamedDecl *ClangDecl, Version version,
                        bool &TypedefIsSuperfluous, bool &HadForwardDeclaration);

lib/ClangImporter/Serializability.cpp

@@ -66,7 +66,10 @@ class SerializationPathFinder {
 
   StableSerializationPath findImportedPath(const clang::NamedDecl *decl) {
     // We've almost certainly imported this declaration, look for it.
-    if (auto swiftDecl = Impl.importDeclCached(decl, Impl.CurrentVersion)) {
+    Optional<Decl *> swiftDeclOpt =
+      Impl.importDeclCached(decl, Impl.CurrentVersion);
+    if (swiftDeclOpt.hasValue() && swiftDeclOpt.getValue()) {
+      auto swiftDecl = swiftDeclOpt.getValue();
       // The serialization code doesn't allow us to cross-reference
       // typealias declarations directly.  We could fix that, but it's
       // easier to just avoid doing so and fall into the external-path code.

test/ClangImporter/attr-swift_name.swift

@@ -45,14 +45,6 @@ func test(_ i: Int) {
   // CHECK: SWIFT_NAME(MutuallyCircularNameA.Inner) @interface MutuallyCircularNameB : NSObject @end
   // CHECK-NOT: {{warning|note}}:
   // CHECK: note: please report this issue to the owners of 'ObjCIRExtras'
-  // CHECK-NOT: warning:
-
-  // CHECK: warning: cycle detected while resolving 'MutuallyCircularNameA' in swift_name attribute for 'MutuallyCircularNameB'
-  // CHECK: SWIFT_NAME(MutuallyCircularNameA.Inner) @interface MutuallyCircularNameB : NSObject @end
   // CHECK-NOT: {{warning|note}}:
-  // CHECK: note: while resolving 'MutuallyCircularNameB' in swift_name attribute for 'MutuallyCircularNameA'
   // CHECK: SWIFT_NAME(MutuallyCircularNameB.Inner) @interface MutuallyCircularNameA : NSObject @end
-  // CHECK-NOT: {{warning|note}}:
-  // CHECK: note: please report this issue to the owners of 'ObjCIRExtras'
-  // CHECK-NOT: warning:
 }