Merge pull request #77522 from fahadnayyar/cxx-frt-inheritance-diagnostics

[cxx-interop] Infer SWIFT_SHARED_REFERENCE for types inheriting from a C++ foreign reference type

Author: fahadnayyar

include/swift/AST/DiagnosticsClangImporter.def

@@ -295,7 +295,9 @@ ERROR(returns_retained_or_returns_unretained_for_non_cxx_frt_values, none,
       "a SWIFT_SHARED_REFERENCE type",
       (const clang::NamedDecl *))
 
-// TODO: make this case an error in next cxx-interop versions rdar://138806722
+// TODO: In the next C++ interop version, convert this warning into an error and
+// stop importing unannotated C++ APIs that return SWIFT_SHARED_REFERENCE.
+// rdar://138806722
 WARNING(no_returns_retained_returns_unretained, none,
         "%0 should be annotated with either SWIFT_RETURNS_RETAINED or "
         "SWIFT_RETURNS_UNRETAINED as it is returning a SWIFT_SHARED_REFERENCE",
@@ -308,6 +310,15 @@ WARNING(returns_retained_returns_unretained_on_overloaded_operator, none,
         "SWIFT_SHARED_REFERENCE types as owned ",
         (const clang::NamedDecl *))
 
+// TODO: In the next C++ interop version, convert this warning into an error and
+// stop importing C++ types that inherit from SWIFT_SHARED_REFERENCE if the
+// Swift compiler cannot find unique retain/release functions.
+// rdar://145194375
+WARNING(cant_infer_frt_in_cxx_inheritance, none,
+        "unable to infer SWIFT_SHARED_REFERENCE for %0, although one of its "
+        "transitive base types is marked as SWIFT_SHARED_REFERENCE",
+        (const clang::NamedDecl *))
+
 NOTE(unsupported_builtin_type, none, "built-in type '%0' not supported", (StringRef))
 NOTE(record_field_not_imported, none, "field %0 unavailable (cannot import)", (const clang::NamedDecl*))
 NOTE(invoked_func_not_imported, none, "function %0 unavailable (cannot import)", (const clang::NamedDecl*))

include/swift/ClangImporter/ClangImporterRequests.h

@@ -343,15 +343,17 @@ enum class CxxRecordSemanticsKind {
 struct CxxRecordSemanticsDescriptor final {
   const clang::RecordDecl *decl;
   ASTContext &ctx;
+  ClangImporter::Implementation *importerImpl;
 
   /// Whether to emit warnings for missing destructor or copy constructor
   /// whenever the classification of the type assumes that they exist (e.g. for
   /// a value type).
   bool shouldDiagnoseLifetimeOperations;
 
   CxxRecordSemanticsDescriptor(const clang::RecordDecl *decl, ASTContext &ctx,
+                               ClangImporter::Implementation *importerImpl,
                                bool shouldDiagnoseLifetimeOperations = true)
-      : decl(decl), ctx(ctx),
+      : decl(decl), ctx(ctx), importerImpl(importerImpl),
         shouldDiagnoseLifetimeOperations(shouldDiagnoseLifetimeOperations) {}
 
   friend llvm::hash_code hash_value(const CxxRecordSemanticsDescriptor &desc) {

lib/AST/Decl.cpp

@@ -6608,9 +6608,11 @@ bool ClassDecl::isForeignReferenceType() const {
   if (!clangRecordDecl)
     return false;
 
+  // `importerImpl` is set to nullptr here to avoid diagnostics during this
+  // CxxRecordSemantics evaluation.
   CxxRecordSemanticsKind kind = evaluateOrDefault(
       getASTContext().evaluator,
-      CxxRecordSemantics({clangRecordDecl, getASTContext()}), {});
+      CxxRecordSemantics({clangRecordDecl, getASTContext(), nullptr}), {});
   return kind == CxxRecordSemanticsKind::Reference;
 }
 

lib/ClangImporter/ClangClassTemplateNamePrinter.cpp

@@ -12,6 +12,7 @@
 
 #include "ClangClassTemplateNamePrinter.h"
 #include "ImporterImpl.h"
+#include "swift/ClangImporter/ClangImporter.h"
 #include "clang/AST/TemplateArgumentVisitor.h"
 #include "clang/AST/TypeVisitor.h"
 
@@ -24,10 +25,14 @@ struct TemplateInstantiationNamePrinter
   NameImporter *nameImporter;
   ImportNameVersion version;
 
+  ClangImporter::Implementation *importerImpl;
+
   TemplateInstantiationNamePrinter(ASTContext &swiftCtx,
                                    NameImporter *nameImporter,
-                                   ImportNameVersion version)
-      : swiftCtx(swiftCtx), nameImporter(nameImporter), version(version) {}
+                                   ImportNameVersion version,
+                                   ClangImporter::Implementation *importerImpl)
+      : swiftCtx(swiftCtx), nameImporter(nameImporter), version(version),
+        importerImpl(importerImpl) {}
 
   std::string VisitType(const clang::Type *type) {
     // Print "_" as a fallback if we couldn't emit a more meaningful type name.
@@ -100,9 +105,7 @@ struct TemplateInstantiationNamePrinter
     bool isReferenceType = false;
     if (auto tagDecl = type->getPointeeType()->getAsTagDecl()) {
       if (auto *rd = dyn_cast<clang::RecordDecl>(tagDecl))
-        isReferenceType =
-            ClangImporter::Implementation::recordHasReferenceSemantics(
-                rd, swiftCtx);
+        isReferenceType = recordHasReferenceSemantics(rd, importerImpl);
     }
 
     TagTypeDecorator decorator;
@@ -167,8 +170,9 @@ struct TemplateArgumentPrinter
   TemplateInstantiationNamePrinter typePrinter;
 
   TemplateArgumentPrinter(ASTContext &swiftCtx, NameImporter *nameImporter,
-                          ImportNameVersion version)
-      : typePrinter(swiftCtx, nameImporter, version) {}
+                          ImportNameVersion version,
+                          ClangImporter::Implementation *importerImpl)
+      : typePrinter(swiftCtx, nameImporter, version, importerImpl) {}
 
   void VisitTemplateArgument(const clang::TemplateArgument &arg,
                              llvm::raw_svector_ostream &buffer) {
@@ -219,7 +223,8 @@ struct TemplateArgumentPrinter
 std::string swift::importer::printClassTemplateSpecializationName(
     const clang::ClassTemplateSpecializationDecl *decl, ASTContext &swiftCtx,
     NameImporter *nameImporter, ImportNameVersion version) {
-  TemplateArgumentPrinter templateArgPrinter(swiftCtx, nameImporter, version);
+  TemplateArgumentPrinter templateArgPrinter(swiftCtx, nameImporter, version,
+                                             nameImporter->getImporterImpl());
 
   llvm::SmallString<128> storage;
   llvm::raw_svector_ostream buffer(storage);

lib/ClangImporter/ClangImporter.cpp

@@ -1406,10 +1406,10 @@ ClangImporter::create(ASTContext &ctx,
   // Install a Clang module file extension to build Swift name lookup tables.
   importer->Impl.Invocation->getFrontendOpts().ModuleFileExtensions.push_back(
       std::make_shared<SwiftNameLookupExtension>(
-          importer->Impl.BridgingHeaderLookupTable,
-          importer->Impl.LookupTables, importer->Impl.SwiftContext,
+          importer->Impl.BridgingHeaderLookupTable, importer->Impl.LookupTables,
+          importer->Impl.SwiftContext,
           importer->Impl.getBufferImporterForDiagnostics(),
-          importer->Impl.platformAvailability));
+          importer->Impl.platformAvailability, &importer->Impl));
 
   // Create a compiler instance.
   {
@@ -1557,7 +1557,7 @@ ClangImporter::create(ASTContext &ctx,
 
   importer->Impl.nameImporter.reset(new NameImporter(
       importer->Impl.SwiftContext, importer->Impl.platformAvailability,
-      importer->Impl.getClangSema()));
+      importer->Impl.getClangSema(), &importer->Impl));
 
   // FIXME: These decls are not being parsed correctly since (a) some of the
   // callbacks are still being added, and (b) the logic to parse them has
@@ -7680,7 +7680,146 @@ bool importer::isForeignReferenceTypeWithoutImmortalAttrs(const clang::QualType
          !hasImmortalAtts(pointeeType->getDecl());
 }
 
+static bool hasDiamondInheritanceRefType(const clang::CXXRecordDecl *decl) {
+  if (!decl->hasDefinition() || decl->isDependentType())
+    return false;
+
+  llvm::DenseSet<const clang::CXXRecordDecl *> seenBases;
+  bool hasRefDiamond = false;
+
+  decl->forallBases([&](const clang::CXXRecordDecl *Base) {
+    if (hasImportAsRefAttr(Base) && !seenBases.insert(Base).second &&
+        !decl->isVirtuallyDerivedFrom(Base))
+      hasRefDiamond = true;
+    return true;
+  });
+
+  return hasRefDiamond;
+}
+
+// Returns the given declaration along with all its parent declarations that are
+// reference types.
+static llvm::SmallVector<const clang::RecordDecl *, 4>
+getRefParentDecls(const clang::RecordDecl *decl, ASTContext &ctx,
+                  ClangImporter::Implementation *importerImpl) {
+  assert(decl && "decl is null inside getRefParentDecls");
+
+  llvm::SmallVector<const clang::RecordDecl *, 4> matchingDecls;
+
+  if (hasImportAsRefAttr(decl))
+    matchingDecls.push_back(decl);
+
+  if (const auto *cxxRecordDecl = llvm::dyn_cast<clang::CXXRecordDecl>(decl)) {
+    if (!cxxRecordDecl->hasDefinition())
+      return matchingDecls;
+    if (hasDiamondInheritanceRefType(cxxRecordDecl)) {
+      if (importerImpl) {
+        if (!importerImpl->DiagnosedCxxRefDecls.count(decl)) {
+          HeaderLoc loc(decl->getLocation());
+          importerImpl->diagnose(loc, diag::cant_infer_frt_in_cxx_inheritance,
+                                 decl);
+          importerImpl->DiagnosedCxxRefDecls.insert(decl);
+        }
+      } else {
+        ctx.Diags.diagnose({}, diag::cant_infer_frt_in_cxx_inheritance, decl);
+        assert(false && "nullpointer passeed for importerImpl when calling "
+                        "getRefParentOrDiag");
+      }
+      return matchingDecls;
+    }
+    cxxRecordDecl->forallBases([&](const clang::CXXRecordDecl *baseDecl) {
+      if (hasImportAsRefAttr(baseDecl))
+        matchingDecls.push_back(baseDecl);
+      return true;
+    });
+  }
+
+  return matchingDecls;
+}
+
+static llvm::SmallVector<ValueDecl *, 1>
+getValueDeclsForName(const clang::Decl *decl, ASTContext &ctx, StringRef name) {
+  llvm::SmallVector<ValueDecl *, 1> results;
+  auto *clangMod = decl->getOwningModule();
+  if (clangMod && clangMod->isSubModule())
+    clangMod = clangMod->getTopLevelModule();
+  if (clangMod) {
+    auto parentModule =
+        ctx.getClangModuleLoader()->getWrapperForModule(clangMod);
+    ctx.lookupInModule(parentModule, name, results);
+  } else {
+    // There is no Clang module for this declaration, so perform lookup from
+    // the main module. This will find declarations from the bridging header.
+    namelookup::lookupInModule(
+        ctx.MainModule, ctx.getIdentifier(name), results,
+        NLKind::UnqualifiedLookup, namelookup::ResolutionKind::Overloadable,
+        ctx.MainModule, SourceLoc(), NL_UnqualifiedDefault);
+
+    // Filter out any declarations that didn't come from Clang.
+    auto newEnd =
+        std::remove_if(results.begin(), results.end(),
+                       [&](ValueDecl *decl) { return !decl->getClangDecl(); });
+    results.erase(newEnd, results.end());
+  }
+  return results;
+}
+
+static const clang::RecordDecl *
+getRefParentOrDiag(const clang::RecordDecl *decl, ASTContext &ctx,
+                   ClangImporter::Implementation *importerImpl) {
+  auto refParentDecls = getRefParentDecls(decl, ctx, importerImpl);
+  if (refParentDecls.empty())
+    return nullptr;
+
+  std::unordered_set<ValueDecl *> uniqueRetainDecls{}, uniqueReleaseDecls{};
+  constexpr StringRef retainPrefix = "retain:";
+  constexpr StringRef releasePrefix = "release:";
+
+  for (const auto *refParentDecl : refParentDecls) {
+    assert(refParentDecl && "refParentDecl is null inside getRefParentOrDiag");
+    for (const auto *attr : refParentDecl->getAttrs()) {
+      if (const auto swiftAttr = llvm::dyn_cast<clang::SwiftAttrAttr>(attr)) {
+        const auto &attribute = swiftAttr->getAttribute();
+        llvm::SmallVector<ValueDecl *, 1> valueDecls;
+        if (attribute.starts_with(retainPrefix)) {
+          auto name = attribute.drop_front(retainPrefix.size()).str();
+          valueDecls = getValueDeclsForName(decl, ctx, name);
+          uniqueRetainDecls.insert(valueDecls.begin(), valueDecls.end());
+        } else if (attribute.starts_with(releasePrefix)) {
+          auto name = attribute.drop_front(releasePrefix.size()).str();
+          valueDecls = getValueDeclsForName(decl, ctx, name);
+          uniqueReleaseDecls.insert(valueDecls.begin(), valueDecls.end());
+        }
+      }
+    }
+  }
+
+  // Ensure that exactly one unique retain function and one unique release
+  // function are found.
+  if (uniqueRetainDecls.size() != 1 || uniqueReleaseDecls.size() != 1) {
+    if (importerImpl) {
+      if (!importerImpl->DiagnosedCxxRefDecls.count(decl)) {
+        HeaderLoc loc(decl->getLocation());
+        importerImpl->diagnose(loc, diag::cant_infer_frt_in_cxx_inheritance,
+                               decl);
+        importerImpl->DiagnosedCxxRefDecls.insert(decl);
+      }
+    } else {
+      ctx.Diags.diagnose({}, diag::cant_infer_frt_in_cxx_inheritance, decl);
+      assert(false && "nullpointer passed for importerImpl when calling "
+                      "getRefParentOrDiag");
+    }
+    return nullptr;
+  }
+
+  return refParentDecls.front();
+}
+
 // Is this a pointer to a foreign reference type.
+// TODO: We need to review functions like this to ensure that
+// CxxRecordSemantics::evaluate is consistently invoked wherever we need to
+// determine whether a C++ type qualifies as a foreign reference type
+// rdar://145184659
 static bool isForeignReferenceType(const clang::QualType type) {
   if (!type->isPointerType())
     return false;
@@ -7929,10 +8068,10 @@ CxxRecordSemanticsKind
 CxxRecordSemantics::evaluate(Evaluator &evaluator,
                              CxxRecordSemanticsDescriptor desc) const {
   const auto *decl = desc.decl;
-
-  if (hasImportAsRefAttr(decl)) {
+  ClangImporter::Implementation *importerImpl = desc.importerImpl;
+  if (hasImportAsRefAttr(decl) ||
+      getRefParentOrDiag(decl, desc.ctx, importerImpl))
     return CxxRecordSemanticsKind::Reference;
-  }
 
   auto cxxDecl = dyn_cast<clang::CXXRecordDecl>(decl);
   if (!cxxDecl) {
@@ -7945,15 +8084,16 @@ CxxRecordSemantics::evaluate(Evaluator &evaluator,
   if (!hasDestroyTypeOperations(cxxDecl) ||
       (!hasCopyTypeOperations(cxxDecl) && !hasMoveTypeOperations(cxxDecl))) {
     if (desc.shouldDiagnoseLifetimeOperations) {
+      HeaderLoc loc(decl->getLocation());
       if (hasUnsafeAPIAttr(cxxDecl))
-        desc.ctx.Diags.diagnose({}, diag::api_pattern_attr_ignored,
-                                "import_unsafe", decl->getNameAsString());
+        importerImpl->diagnose(loc, diag::api_pattern_attr_ignored,
+                               "import_unsafe", decl->getNameAsString());
       if (hasOwnedValueAttr(cxxDecl))
-        desc.ctx.Diags.diagnose({}, diag::api_pattern_attr_ignored,
-                                "import_owned", decl->getNameAsString());
+        importerImpl->diagnose(loc, diag::api_pattern_attr_ignored,
+                               "import_owned", decl->getNameAsString());
       if (hasIteratorAPIAttr(cxxDecl))
-        desc.ctx.Diags.diagnose({}, diag::api_pattern_attr_ignored,
-                                "import_iterator", decl->getNameAsString());
+        importerImpl->diagnose(loc, diag::api_pattern_attr_ignored,
+                               "import_iterator", decl->getNameAsString());
     }
 
     return CxxRecordSemanticsKind::MissingLifetimeOperation;
@@ -8160,6 +8300,12 @@ CustomRefCountingOperationResult CustomRefCountingOperation::evaluate(
                                  : "release:";
 
   auto decl = cast<clang::RecordDecl>(swiftDecl->getClangDecl());
+
+  if (!hasImportAsRefAttr(decl)) {
+    if (auto parentRefDecl = getRefParentOrDiag(decl, ctx, nullptr))
+      decl = parentRefDecl;
+  }
+
   if (!decl->hasAttrs())
     return {CustomRefCountingOperationResult::noAttribute, nullptr, ""};
 
@@ -8186,27 +8332,8 @@ CustomRefCountingOperationResult CustomRefCountingOperation::evaluate(
   if (name == "immortal")
     return {CustomRefCountingOperationResult::immortal, nullptr, name};
 
-  llvm::SmallVector<ValueDecl *, 1> results;
-  auto *clangMod = swiftDecl->getClangDecl()->getOwningModule();
-  if (clangMod && clangMod->isSubModule())
-    clangMod = clangMod->getTopLevelModule();
-  if (clangMod) {
-    auto parentModule = ctx.getClangModuleLoader()->getWrapperForModule(clangMod);
-    ctx.lookupInModule(parentModule, name, results);
-  } else {
-    // There is no Clang module for this declaration, so perform lookup from
-    // the main module. This will find declarations from the bridging header.
-    namelookup::lookupInModule(
-        ctx.MainModule, ctx.getIdentifier(name), results,
-        NLKind::UnqualifiedLookup, namelookup::ResolutionKind::Overloadable,
-        ctx.MainModule, SourceLoc(), NL_UnqualifiedDefault);
-
-    // Filter out any declarations that didn't come from Clang.
-    auto newEnd = std::remove_if(results.begin(), results.end(), [&](ValueDecl *decl) {
-      return !decl->getClangDecl();
-    });
-    results.erase(newEnd, results.end());
-  }
+  llvm::SmallVector<ValueDecl *, 1> results =
+      getValueDeclsForName(swiftDecl->getClangDecl(), ctx, name);
   if (results.size() == 1)
     return {CustomRefCountingOperationResult::foundOperation, results.front(),
             name};