Don’t crash from circular swift_name attributes

include/swift/AST/DiagnosticsClangImporter.def

@@ -67,6 +67,13 @@ WARNING(inconsistent_swift_name,none,
         (bool, StringRef, StringRef, DeclName, StringRef, DeclName,
          StringRef))
 
+WARNING(swift_name_circular_context_import,none,
+        "cycle detected while resolving '%0' in swift_name attribute for '%1'",
+        (StringRef, StringRef))
+NOTE(swift_name_circular_context_import_other,none,
+     "while resolving '%0' in swift_name attribute for '%1'",
+     (StringRef, StringRef))
+
 WARNING(unresolvable_clang_decl,none,
         "imported declaration '%0' could not be mapped to '%1'",
         (StringRef, StringRef))

lib/ClangImporter/ImportDecl.cpp

@@ -9204,7 +9204,7 @@ ClangImporter::Implementation::importMirroredDecl(const clang::NamedDecl *decl,
 }
 
 DeclContext *ClangImporter::Implementation::importDeclContextImpl(
-    const clang::DeclContext *dc) {
+    const clang::Decl *ImportingDecl, const clang::DeclContext *dc) {
   // Every declaration should come from a module, so we should not see the
   // TranslationUnit DeclContext here.
   assert(!dc->isTranslationUnit());
@@ -9217,7 +9217,8 @@ DeclContext *ClangImporter::Implementation::importDeclContextImpl(
   // leads to the first category of the given name. We'd like to keep these
   // categories separated.
   auto useCanonical = !isa<clang::ObjCCategoryDecl>(decl);
-  auto swiftDecl = importDecl(decl, CurrentVersion, useCanonical);
+  auto swiftDecl = importDeclForDeclContext(ImportingDecl, decl->getName(),
+                                            decl, CurrentVersion, useCanonical);
   if (!swiftDecl)
     return nullptr;
 
@@ -9270,6 +9271,71 @@ GenericSignature ClangImporter::Implementation::buildGenericSignature(
       GenericSignature());
 }
 
+Decl *
+ClangImporter::Implementation::importDeclForDeclContext(
+    const clang::Decl *importingDecl,
+    StringRef writtenName,
+    const clang::NamedDecl *contextDecl,
+    Version version,
+    bool useCanonicalDecl)
+{
+  auto key = std::make_tuple(importingDecl, writtenName, contextDecl, version,
+                             useCanonicalDecl);
+  auto iter = find(llvm::reverse(contextDeclsBeingImported), key);
+
+  // No cycle? Remember that we're importing this, then import normally.
+  if (iter == contextDeclsBeingImported.rend()) {
+    contextDeclsBeingImported.push_back(key);
+    auto imported = importDecl(contextDecl, version, useCanonicalDecl);
+    contextDeclsBeingImported.pop_back();
+    return imported;
+  }
+
+  // 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;
+
+  // Can't break it? Warn and return nullptr, which is at least better than
+  // stack overflow by recursion.
+
+  // Avoid emitting warnings repeatedly.
+  if (!contextDeclsWarnedAbout.insert(contextDecl).second)
+    return nullptr;
+
+  auto convertLoc = [&](clang::SourceLocation clangLoc) {
+    return getBufferImporterForDiagnostics()
+      .resolveSourceLocation(getClangASTContext().getSourceManager(),
+                             clangLoc);
+  };
+
+  auto getDeclName = [](const clang::Decl *D) -> StringRef {
+    if (auto ND = dyn_cast<clang::NamedDecl>(D))
+      return ND->getName();
+    return "<anonymous>";
+  };
+
+  SourceLoc loc = convertLoc(importingDecl->getLocation());
+  diagnose(loc, diag::swift_name_circular_context_import,
+           writtenName, getDeclName(importingDecl));
+
+  // Diagnose other decls involved in the cycle.
+  for (auto entry : make_range(contextDeclsBeingImported.rbegin(), iter)) {
+    auto otherDecl = std::get<0>(entry);
+    auto otherWrittenName = std::get<1>(entry);
+    diagnose(convertLoc(otherDecl->getLocation()),
+             diag::swift_name_circular_context_import_other,
+             otherWrittenName, getDeclName(otherDecl));
+  }
+
+  if (auto *parentModule = contextDecl->getOwningModule()) {
+    diagnose(loc, diag::unresolvable_clang_decl_is_a_framework_bug,
+             parentModule->getFullModuleName());
+  }
+
+  return nullptr;
+}
+
 DeclContext *
 ClangImporter::Implementation::importDeclContextOf(
   const clang::Decl *decl,
@@ -9287,13 +9353,15 @@ ClangImporter::Implementation::importDeclContextOf(
     }
 
     // Import the DeclContext.
-    importedDC = importDeclContextImpl(dc);
+    importedDC = importDeclContextImpl(decl, dc);
     break;
   }
 
   case EffectiveClangContext::TypedefContext: {
     // Import the typedef-name as a declaration.
-    auto importedDecl = importDecl(context.getTypedefName(), CurrentVersion);
+    auto importedDecl = importDeclForDeclContext(
+        decl, context.getTypedefName()->getName(), context.getTypedefName(),
+        CurrentVersion);
     if (!importedDecl) return nullptr;
 
     // Dig out the imported DeclContext.
@@ -9311,17 +9379,19 @@ ClangImporter::Implementation::importDeclContextOf(
       if (auto clangDecl
             = lookupTable->resolveContext(context.getUnresolvedName())) {
         // Import the Clang declaration.
-        auto decl = importDecl(clangDecl, CurrentVersion);
-        if (!decl) return nullptr;
+        auto swiftDecl = importDeclForDeclContext(decl,
+                                                  context.getUnresolvedName(),
+                                                  clangDecl, CurrentVersion);
+        if (!swiftDecl) return nullptr;
 
         // Look through typealiases.
-        if (auto typealias = dyn_cast<TypeAliasDecl>(decl))
+        if (auto typealias = dyn_cast<TypeAliasDecl>(swiftDecl))
           importedDC = typealias->getDeclaredInterfaceType()->getAnyNominal();
         else // Map to a nominal type declaration.
-          importedDC = dyn_cast<NominalTypeDecl>(decl);
-        break;
+          importedDC = dyn_cast<NominalTypeDecl>(swiftDecl);
       }
     }
+    break;
   }
   }
 

lib/ClangImporter/ImporterImpl.h

@@ -913,8 +913,34 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   ///
   /// \returns The imported declaration context, or null if it could not
   /// be converted.
-  DeclContext *importDeclContextImpl(const clang::DeclContext *dc);
+  DeclContext *importDeclContextImpl(const clang::Decl *ImportingDecl,
+                                     const clang::DeclContext *dc);
 
+private:
+  /// Declarations currently being imported by \c importDeclForDeclContext().
+  /// Used to break cycles when a swift_name attribute is circular in a way that
+  /// can't be resolved, or there is some other cycle through
+  /// \c importDeclContextOf().
+  llvm::SmallVector<std::tuple<const clang::Decl *, StringRef,
+                               const clang::NamedDecl *, Version, bool>, 8>
+      contextDeclsBeingImported;
+
+  /// Records which contexts \c importDeclForDeclContext() has already warned
+  /// were unimportable.
+  llvm::SmallPtrSet<const clang::NamedDecl *, 4> contextDeclsWarnedAbout;
+
+  /// Exactly equivalent to \c importDecl(), except with additional
+  /// cycle-breaking code.
+  ///
+  /// \param writtenName The name that should be used for the declaration
+  ///        in cycle diagnostics.
+  Decl *importDeclForDeclContext(const clang::Decl *ImportingDecl,
+                                 StringRef writtenName,
+                                 const clang::NamedDecl *ClangDecl,
+                                 Version version,
+                                 bool UseCanonicalDecl = true);
+
+public:
   /// Import the declaration context of a given Clang declaration into
   /// Swift.
   ///

test/ClangImporter/Inputs/custom-modules/ObjCIRExtras.h

@@ -72,4 +72,11 @@ int global_int SWIFT_NAME(GlobalInt);
 @compatibility_alias SwiftGenericNameAlias SwiftGenericNameTest;
 @compatibility_alias SwiftConstrGenericNameAlias SwiftConstrGenericNameTest;
 
+SWIFT_NAME(CircularName.Inner) @interface CircularName : NSObject @end
+
+SWIFT_NAME(MutuallyCircularNameB.Inner) @interface MutuallyCircularNameA : NSObject @end
+SWIFT_NAME(MutuallyCircularNameA.Inner) @interface MutuallyCircularNameB : NSObject @end
+
+void circularFriends(CircularName*, MutuallyCircularNameA*);
+
 #pragma clang assume_nonnull end

test/ClangImporter/attr-swift_name.swift

@@ -1,5 +1,5 @@
 // RUN: %empty-directory(%t.mcp)
-// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -I %S/Inputs/custom-modules -Xcc -w -typecheck %s -module-cache-path %t.mcp -disable-named-lazy-member-loading 2>&1 | %FileCheck %s
+// RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk) -I %S/Inputs/custom-modules -Xcc -w -typecheck %s -module-cache-path %t.mcp -disable-named-lazy-member-loading 2>&1 | %FileCheck %s
 
 // REQUIRES: objc_interop
 
@@ -15,7 +15,9 @@ func test(_ i: Int) {
   _ = t.renamedSomeProp
   _ = type(of: t).renamedClassProp
 
-  // We only see these two warnings because Clang can catch the other invalid
+  _ = circularFriends(_:_:)
+
+  // We only see these five warnings because Clang can catch the other invalid
   // cases, and marks the attribute as invalid ahead of time.
 
   // CHECK: warning: too few parameters in swift_name attribute (expected 2; got 1)
@@ -29,4 +31,28 @@ func test(_ i: Int) {
   // CHECK-NOT: warning:
   // CHECK: note: please report this issue to the owners of 'ObjCIRExtras'
   // CHECK-NOT: warning:
+
+  // CHECK: warning: cycle detected while resolving 'CircularName' in swift_name attribute for 'CircularName'
+  // CHECK: SWIFT_NAME(CircularName.Inner) @interface CircularName : 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 'MutuallyCircularNameB' in swift_name attribute for 'MutuallyCircularNameA'
+  // CHECK: SWIFT_NAME(MutuallyCircularNameB.Inner) @interface MutuallyCircularNameA : NSObject @end
+  // CHECK-NOT: {{warning|note}}:
+  // CHECK: note: while resolving 'MutuallyCircularNameA' in swift_name attribute for 'MutuallyCircularNameB'
+  // 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:
 }