[cxx-interop] Avoid unchecked recursion when importing C++ classes with circular inheritance

It is possible for a C++ class template to inherit from a specialization
of itself. Normally, these are imported to Swift as separate (unrelated)
types, but when symbolic import is enabled, unspecialized templates are
imported in place of their specializations, leading to circularly
inheriting classes to seemingly inherit from themselves.

This patch adds a check to guard against the most common case of
circular inheritance, when a class template directly inherits from
itself. This pattern appears in a recent version of libc++,
necessitating this patch. However, the solution here is imperfect as it
does not handle more complex/contrived circular inheritance patterns.

This patch also adds a test case exercising this pattern. The
-index-store-path flag causes swift-frontend to index the C++ module
with symbolic import enabled, without the fix in this patch, that test
triggers an assertion failure due to the circular reference (and can
infinitely recurse in the StorageVisitor when assertions are disabled).

rdar://148026461

Author: j-hui

include/swift/ClangImporter/ClangImporter.h

@@ -786,6 +786,22 @@ bool declIsCxxOnly(const Decl *decl);
 
 /// Is this DeclContext an `enum` that represents a C++ namespace?
 bool isClangNamespace(const DeclContext *dc);
+
+/// For some \a templatedClass that inherits from \a base, whether they are
+/// derived from the same class template.
+///
+/// This kind of circular inheritance can happen when a templated class inherits
+/// from a specialization of itself, e.g.:
+///
+///     template <typename T> class C;
+///     template <> class C<void> { /* ... */ };
+///     template <typename T> class C<T> : C<void> { /* ... */ };
+///
+/// Checking for this kind of scenario is necessary for avoiding infinite
+/// recursion during symbolic imports (importSymbolicCXXDecls), where
+/// specialized class templates are instead imported as unspecialized.
+bool isSymbolicCircularBase(const clang::CXXRecordDecl *templatedClass,
+                            const clang::RecordDecl *base);
 } // namespace importer
 
 struct ClangInvocationFileMapping {

lib/ClangImporter/ClangImporter.cpp

@@ -6364,6 +6364,11 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
         continue;
 
       auto *baseRecord = baseType->getAs<clang::RecordType>()->getDecl();
+
+      if (isSymbolicCircularBase(cxxRecord, baseRecord))
+        // Skip circular bases to avoid unbounded recursion
+        continue;
+
       if (auto import = clangModuleLoader->importDeclDirectly(baseRecord)) {
         // If we are looking up the base class, go no further. We will have
         // already found it during the other lookup.
@@ -8774,3 +8779,18 @@ bool importer::isClangNamespace(const DeclContext *dc) {
 
   return false;
 }
+
+bool importer::isSymbolicCircularBase(const clang::CXXRecordDecl *symbolicClass,
+                                      const clang::RecordDecl *base) {
+  auto *classTemplate = symbolicClass->getDescribedClassTemplate();
+  if (!classTemplate)
+    return false;
+
+  auto *specializedBase =
+      dyn_cast<clang::ClassTemplateSpecializationDecl>(base);
+  if (!specializedBase)
+    return false;
+
+  return classTemplate->getCanonicalDecl() ==
+         specializedBase->getSpecializedTemplate()->getCanonicalDecl();
+}

lib/Sema/TypeCheckInvertible.cpp

@@ -17,11 +17,12 @@
 //===----------------------------------------------------------------------===//
 
 #include "TypeCheckInvertible.h"
+#include "TypeChecker.h"
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/ClangModuleLoader.h"
 #include "swift/AST/GenericEnvironment.h"
 #include "swift/Basic/Assertions.h"
-#include "TypeChecker.h"
+#include "swift/ClangImporter/ClangImporter.h"
 
 using namespace swift;
 
@@ -316,6 +317,9 @@ bool StorageVisitor::visit(NominalTypeDecl *nominal, DeclContext *dc) {
             dyn_cast_or_null<clang::CXXRecordDecl>(nominal->getClangDecl())) {
       for (auto cxxBase : cxxRecordDecl->bases()) {
         if (auto cxxBaseDecl = cxxBase.getType()->getAsCXXRecordDecl()) {
+          if (importer::isSymbolicCircularBase(cxxRecordDecl, cxxBaseDecl))
+            // Skip circular bases to avoid unbounded recursion
+            continue;
           auto clangModuleLoader = dc->getASTContext().getClangModuleLoader();
           auto importedDecl =
               clangModuleLoader->importDeclDirectly(cxxBaseDecl);

test/Interop/Cxx/class/inheritance/Inputs/circular-inheritance.h

@@ -0,0 +1,32 @@
+#ifndef CIRCULAR_INHERITANCE_H
+#define CIRCULAR_INHERITANCE_H
+
+struct DinoEgg {
+  void dinoEgg(void) const {}
+};
+
+template <typename Chicken>
+struct Egg;
+
+template <>
+struct Egg<void> : DinoEgg {
+  Egg() {}
+  void voidEgg(void) const {}
+};
+
+template <typename Chicken>
+struct Egg : Egg<void> {
+  Egg(Chicken _chicken) {}
+  Chicken chickenEgg(Chicken c) const { return c; }
+};
+
+typedef Egg<void> VoidEgg;
+typedef Egg<bool> BoolEgg;
+typedef Egg<Egg<void>> EggEgg;
+
+struct NewEgg : Egg<int> {
+  NewEgg() : Egg<int>(555) {}
+  void newEgg() const {}
+};
+
+#endif // !CIRCULAR_INHERITANCE_H

test/Interop/Cxx/class/inheritance/Inputs/module.modulemap

@@ -48,3 +48,8 @@ module InheritedLookup {
   header "inherited-lookup.h"
   requires cplusplus
 }
+
+module CircularInheritance {
+  header "circular-inheritance.h"
+  requires cplusplus
+}

test/Interop/Cxx/class/inheritance/circular-inheritance-typechecker.swift

@@ -0,0 +1,27 @@
+// RUN: %empty-directory(%t/index)
+// RUN: %target-typecheck-verify-swift -verify-ignore-unknown -I %S/Inputs -cxx-interoperability-mode=default -index-store-path %t/index
+//
+// Note that we specify an -index-store-path to ensure we also test importing symbolic C++ decls,
+// to exercise code that handles importing unspecialized class templates.
+
+import CircularInheritance
+
+let voidEgg = VoidEgg()
+voidEgg.dinoEgg()
+voidEgg.voidEgg()
+
+let boolEgg = BoolEgg(false)
+boolEgg.dinoEgg()
+boolEgg.voidEgg()
+boolEgg.chickenEgg(true)
+
+let eggEgg = EggEgg(VoidEgg())
+eggEgg.dinoEgg()
+eggEgg.voidEgg()
+eggEgg.chickenEgg(VoidEgg())
+
+let newEgg = NewEgg()
+newEgg.dinoEgg()
+newEgg.voidEgg()
+newEgg.chickenEgg(555)
+newEgg.newEgg()