[nfc][cxx-interop] Add diagnostics when something cannot be imported.

Author: zoecarver

include/swift/AST/DiagnosticsClangImporter.def

@@ -128,6 +128,10 @@ WARNING(too_many_class_template_instantiations, none,
         "template instantiation for '%0' not imported: too many instantiations",
         (StringRef))
 
+WARNING(api_pattern_attr_ignored, none,
+        "'%0' swift attribute ignored on type '%1': type is not copyable or destructible",
+        (StringRef, StringRef))
+
 NOTE(macro_not_imported_unsupported_operator, none, "operator not supported in macro arithmetic", ())
 NOTE(macro_not_imported_unsupported_named_operator, none, "operator '%0' not supported in macro arithmetic", (StringRef))
 NOTE(macro_not_imported_invalid_string_literal, none, "invalid string literal", ())
@@ -140,18 +144,40 @@ NOTE(macro_not_imported_unsupported_structure, none, "macro '%0' not imported: s
 NOTE(macro_not_imported, none, "macro '%0' not imported", (StringRef))
 
 NOTE(return_type_not_imported, none, "return type not imported", ())
-NOTE(parameter_type_not_imported, none, "parameter %0 not imported", (const clang::NamedDecl*))
-NOTE(incomplete_interface, none, "interface %0 is incomplete", (const clang::NamedDecl*))
-NOTE(incomplete_protocol, none, "protocol %0 is incomplete", (const clang::NamedDecl*))
-NOTE(unsupported_builtin_type, none, "built-in type '%0' not supported", (StringRef))
+NOTE(parameter_type_not_imported, none, "parameter '%0' not imported", (const clang::NamedDecl*))
+NOTE(incomplete_interface, none, "interface '%0' is incomplete", (const clang::NamedDecl*))
+NOTE(incomplete_protocol, none, "protocol '%0' is incomplete", (const clang::NamedDecl*))
+NOTE(incomplete_record, none, "record '%0' is not defined (incomplete)", (StringRef))
+NOTE(record_over_aligned, none, "record '%0' is over aligned", (StringRef))
+NOTE(record_non_trivial_copy_destroy, none, "record '%0' is not trivial to copy/destroy", (StringRef))
+NOTE(record_is_dependent, none, "record '%0' is dependent", (StringRef))
+NOTE(record_parent_unimportable, none, "record %0's parent is not importable", (StringRef))
+NOTE(reference_passed_by_value, none, "function uses foreign reference type "
+                                      "'%0' as a value in %1 types which breaks "
+                                      "'import_reference' contract (outlined in "
+                                      "C++ Interop User Manual).",
+     (StringRef, StringRef))
+NOTE(record_not_automatically_importable, none, "record '%0' is not "
+                                                "automatically importable: %1. "
+                                                "Refer to the C++ Interop User "
+                                                "Manual to classify this type.",
+     (StringRef, StringRef))
+NOTE(projection_not_imported, none, "C++ method '%0' that returns unsafe "
+                                    "projection of type '%1' not imported",
+     (StringRef, StringRef))
+NOTE(dont_use_iterator_api, none, "C++ method '%0' that returns an unsafe "
+                                  "iterator not imported: use Swift Sequence "
+                                  "APIs instead",
+     (StringRef))
 
-NOTE(record_field_not_imported, none, "field %0 not imported", (const clang::NamedDecl*))
-NOTE(invoked_func_not_imported, none, "function %0 not imported", (const clang::NamedDecl*))
-NOTE(record_method_not_imported, none, "method %0 not imported", (const clang::NamedDecl*))
-NOTE(objc_property_not_imported, none, "property %0 not imported", (const clang::NamedDecl*))
+NOTE(unsupported_builtin_type, none, "built-in type '%0' not supported", (StringRef))
+NOTE(record_field_not_imported, none, "field '%0' not imported", (const clang::NamedDecl*))
+NOTE(invoked_func_not_imported, none, "function '%0' not imported", (const clang::NamedDecl*))
+NOTE(record_method_not_imported, none, "method '%0' not imported", (const clang::NamedDecl*))
+NOTE(objc_property_not_imported, none, "property '%0' not imported", (const clang::NamedDecl*))
 
-NOTE(forward_declared_interface_label, none, "interface %0 forward declared here", (const clang::NamedDecl*))
-NOTE(forward_declared_protocol_label, none, "protocol %0 forward declared here", (const clang::NamedDecl*))
+NOTE(forward_declared_interface_label, none, "interface '%0' forward declared here", (const clang::NamedDecl*))
+NOTE(forward_declared_protocol_label, none, "protocol '%0' forward declared here", (const clang::NamedDecl*))
 
 #define UNDEFINE_DIAGNOSTIC_MACROS
 #include "DefineDiagnosticMacros.h"

include/swift/ClangImporter/ClangImporterRequests.h

@@ -186,6 +186,8 @@ enum class CxxRecordSemanticsKind {
   // An API that has be annotated as explicitly unsafe, but still importable.
   // TODO: we should rename these APIs.
   ExplicitlyUnsafe,
+  // A record that is either not copyable or not destructible.
+  MissingLifetimeOperation,
   // A record that has custom lifetime operations which we cannot prove are
   // safe.
   UnsafeLifetimeOperation,
@@ -195,8 +197,11 @@ enum class CxxRecordSemanticsKind {
 
 struct CxxRecordSemanticsDescriptor final {
   const clang::CXXRecordDecl *decl;
+  ASTContext &ctx;
 
-  CxxRecordSemanticsDescriptor(const clang::CXXRecordDecl *decl) : decl(decl) {}
+  CxxRecordSemanticsDescriptor(const clang::CXXRecordDecl *decl,
+                               ASTContext &ctx)
+      : decl(decl), ctx(ctx) {}
 
   friend llvm::hash_code hash_value(const CxxRecordSemanticsDescriptor &desc) {
     return llvm::hash_combine(desc.decl);
@@ -241,8 +246,11 @@ class CxxRecordSemantics
 
 struct SafeUseOfCxxDeclDescriptor final {
   const clang::Decl *decl;
+  ASTContext &ctx;
 
-  SafeUseOfCxxDeclDescriptor(const clang::Decl *decl) : decl(decl) {}
+  SafeUseOfCxxDeclDescriptor(const clang::Decl *decl,
+                             ASTContext &ctx)
+      : decl(decl), ctx(ctx) {}
 
   friend llvm::hash_code hash_value(const SafeUseOfCxxDeclDescriptor &desc) {
     return llvm::hash_combine(desc.decl);

lib/ClangImporter/ClangImporter.cpp

@@ -5939,9 +5939,21 @@ CxxRecordSemantics::evaluate(Evaluator &evaluator,
     return CxxRecordSemanticsKind::Reference;
   }
 
-  // TODO: diagnose if the decl also has any attrs.
   if (!hasRequiredValueTypeOperations(decl)) {
-    return CxxRecordSemanticsKind::UnsafeLifetimeOperation;
+    if (hasUnsafeAPIAttr(decl))
+      desc.ctx.Diags.diagnose({}, diag::api_pattern_attr_ignored,
+                                            "import_unsafe",
+                                            decl->getNameAsString());
+    if (hasOwnedValueAttr(decl))
+      desc.ctx.Diags.diagnose({}, diag::api_pattern_attr_ignored,
+                                            "import_owned",
+                                            decl->getNameAsString());
+    if (hasIteratorAPIAttr(decl))
+      desc.ctx.Diags.diagnose({}, diag::api_pattern_attr_ignored,
+                                            "import_iterator",
+                                            decl->getNameAsString());
+
+    return CxxRecordSemanticsKind::MissingLifetimeOperation;
   }
 
   if (hasUnsafeAPIAttr(decl)) {
@@ -5992,7 +6004,8 @@ bool IsSafeUseOfCxxDecl::evaluate(Evaluator &evaluator,
       if (auto cxxRecordReturnType =
               dyn_cast<clang::CXXRecordDecl>(returnType->getDecl())) {
         auto semanticsKind = evaluateOrDefault(
-            evaluator, CxxRecordSemantics({cxxRecordReturnType}), {});
+            evaluator,
+            CxxRecordSemantics({cxxRecordReturnType, desc.ctx}), {});
 
         if (semanticsKind == CxxRecordSemanticsKind::UnsafePointerMember ||
             // Pretend all methods that return iterators are unsafe so protocol
@@ -6011,10 +6024,12 @@ bool IsSafeUseOfCxxDecl::evaluate(Evaluator &evaluator,
   }
 
   auto semanticsKind =
-      evaluateOrDefault(evaluator, CxxRecordSemantics({recordDecl}), {});
+      evaluateOrDefault(evaluator,
+                        CxxRecordSemantics({recordDecl, desc.ctx}), {});
 
   // Always unsafe.
-  if (semanticsKind == CxxRecordSemanticsKind::UnsafeLifetimeOperation)
+  if (semanticsKind == CxxRecordSemanticsKind::MissingLifetimeOperation ||
+      semanticsKind == CxxRecordSemanticsKind::UnsafeLifetimeOperation)
     return false;
 
   // Always OK.

lib/ClangImporter/ImportDecl.cpp

@@ -1950,7 +1950,8 @@ namespace {
     bool recordHasReferenceSemantics(const clang::RecordDecl *decl) {
       if (auto cxxRecord = dyn_cast<clang::CXXRecordDecl>(decl)) {
         auto semanticsKind = evaluateOrDefault(
-            Impl.SwiftContext.evaluator, CxxRecordSemantics({cxxRecord}), {});
+            Impl.SwiftContext.evaluator,
+            CxxRecordSemantics({cxxRecord, Impl.SwiftContext}), {});
         return semanticsKind == CxxRecordSemanticsKind::Reference;
       }
 
@@ -1982,6 +1983,13 @@ namespace {
       if (decl->isInterface())
         return nullptr;
 
+      if (!decl->getDefinition()) {
+        Impl.addImportDiagnostic(
+            decl,
+            Diagnostic(diag::incomplete_record,
+                       Impl.SwiftContext.AllocateCopy(decl->getNameAsString())));
+      }
+
       // FIXME: Figure out how to deal with incomplete types, since that
       // notion doesn't exist in Swift.
       decl = decl->getDefinition();
@@ -1991,12 +1999,22 @@ namespace {
       }
 
       // TODO(SR-13809): fix this once we support dependent types.
-      if (decl->getTypeForDecl()->isDependentType())
+      if (decl->getTypeForDecl()->isDependentType()) {
+        Impl.addImportDiagnostic(
+            decl,
+            Diagnostic(diag::record_is_dependent,
+                       Impl.SwiftContext.AllocateCopy(decl->getNameAsString())));
         return nullptr;
+      }
 
       // Don't import nominal types that are over-aligned.
-      if (Impl.isOverAligned(decl))
+      if (Impl.isOverAligned(decl)) {
+        Impl.addImportDiagnostic(
+            decl,
+            Diagnostic(diag::record_over_aligned,
+                       Impl.SwiftContext.AllocateCopy(decl->getNameAsString())));
         return nullptr;
+      }
 
       // FIXME: We should actually support strong ARC references and similar in
       // C structs. That'll require some SIL and IRGen work, though.
@@ -2012,6 +2030,10 @@ namespace {
         // imported function and the developer would be able to use it without
         // referencing the name, which would sidestep our availability
         // diagnostics.
+        Impl.addImportDiagnostic(
+            decl,
+            Diagnostic(diag::record_non_trivial_copy_destroy,
+                       Impl.SwiftContext.AllocateCopy(decl->getNameAsString())));
         return nullptr;
       }
 
@@ -2030,8 +2052,13 @@ namespace {
 
       auto dc =
           Impl.importDeclContextOf(decl, importedName.getEffectiveContext());
-      if (!dc)
+      if (!dc) {
+        Impl.addImportDiagnostic(
+            decl,
+            Diagnostic(diag::record_parent_unimportable,
+                       Impl.SwiftContext.AllocateCopy(decl->getNameAsString())));
         return nullptr;
+      }
 
       // Create the struct declaration and record it.
       auto name = importedName.getDeclName().getBaseIdentifier();
@@ -2403,6 +2430,11 @@ namespace {
       if (!Impl.SwiftContext.LangOpts.EnableCXXInterop)
         return VisitRecordDecl(decl);
 
+      if (!decl->getDefinition()) {
+        Impl.addImportDiagnostic(decl, Diagnostic(diag::incomplete_record,
+                                                  Impl.SwiftContext.AllocateCopy(decl->getNameAsString())));
+      }
+
       decl = decl->getDefinition();
       if (!decl) {
         forwardDeclaration = true;
@@ -2453,10 +2485,24 @@ namespace {
 
       // It is import that we bail on an unimportable record *before* we import
       // any of its members or cache the decl.
-      auto semanticsKind = evaluateOrDefault(Impl.SwiftContext.evaluator,
-                                             CxxRecordSemantics({decl}), {});
-      if (semanticsKind == CxxRecordSemanticsKind::UnsafeLifetimeOperation)
+      auto semanticsKind = evaluateOrDefault(
+          Impl.SwiftContext.evaluator,
+          CxxRecordSemantics({decl, Impl.SwiftContext}), {});
+      if (semanticsKind == CxxRecordSemanticsKind::MissingLifetimeOperation) {
+        Impl.addImportDiagnostic(
+            decl,
+            Diagnostic(diag::record_not_automatically_importable,
+                       Impl.SwiftContext.AllocateCopy(decl->getNameAsString()),
+                       "does not have a copy constructor or destructor"));
+        return nullptr;
+      } else if (semanticsKind == CxxRecordSemanticsKind::UnsafeLifetimeOperation) {
+        Impl.addImportDiagnostic(
+            decl,
+            Diagnostic(diag::record_not_automatically_importable,
+                       Impl.SwiftContext.AllocateCopy(decl->getNameAsString()),
+                       "has custom, potentially unsafe copy constructor or destructor"));
         return nullptr;
+      }
 
       return VisitRecordDecl(decl);
     }
@@ -2634,8 +2680,10 @@ namespace {
       if (auto parent = dyn_cast<clang::CXXRecordDecl>(
               decl->getAnonField()->getParent())) {
         auto semanticsKind = evaluateOrDefault(
-            Impl.SwiftContext.evaluator, CxxRecordSemantics({parent}), {});
-        if (semanticsKind == CxxRecordSemanticsKind::UnsafeLifetimeOperation)
+            Impl.SwiftContext.evaluator,
+            CxxRecordSemantics({parent, Impl.SwiftContext}), {});
+        if (semanticsKind == CxxRecordSemanticsKind::MissingLifetimeOperation ||
+            semanticsKind == CxxRecordSemanticsKind::UnsafeLifetimeOperation)
           return nullptr;
       }
 
@@ -2712,19 +2760,33 @@ namespace {
 
     bool foreignReferenceTypePassedByRef(const clang::FunctionDecl *decl) {
       bool anyParamPassesByVal =
-          llvm::any_of(decl->parameters(), [this](auto *param) {
+          llvm::any_of(decl->parameters(), [this, decl](auto *param) {
             if (auto recordType = dyn_cast<clang::RecordType>(
-                    param->getType().getCanonicalType()))
-              return recordHasReferenceSemantics(recordType->getDecl());
+                    param->getType().getCanonicalType())) {
+              if (recordHasReferenceSemantics(recordType->getDecl())) {
+                Impl.addImportDiagnostic(
+                    decl,
+                    Diagnostic(diag::reference_passed_by_value,
+                               Impl.SwiftContext.AllocateCopy(recordType->getDecl()->getNameAsString()),
+                               "a parameter"));
+                return true;
+              }
+            }
             return false;
           });
 
       if (anyParamPassesByVal)
         return true;
 
       if (auto recordType = dyn_cast<clang::RecordType>(
-              decl->getReturnType().getCanonicalType()))
+              decl->getReturnType().getCanonicalType())) {
+        Impl.addImportDiagnostic(
+            decl,
+            Diagnostic(diag::reference_passed_by_value,
+                       Impl.SwiftContext.AllocateCopy(recordType->getDecl()->getNameAsString()),
+                       "the return"));
         return recordHasReferenceSemantics(recordType->getDecl());
+      }
 
       return false;
     }

lib/ClangImporter/ImportName.cpp

@@ -1482,7 +1482,8 @@ static StringRef renameUnsafeMethod(ASTContext &ctx,
                                     const clang::NamedDecl *decl,
                                     StringRef name) {
   if (isa<clang::CXXMethodDecl>(decl) &&
-      !evaluateOrDefault(ctx.evaluator, IsSafeUseOfCxxDecl({decl}), {})) {
+      !evaluateOrDefault(ctx.evaluator,
+                         IsSafeUseOfCxxDecl({decl, ctx}), {})) {
     return ctx.getIdentifier(("__" + name + "Unsafe").str()).str();
   }
 

lib/ClangImporter/SwiftLookupTable.cpp

@@ -1869,30 +1869,6 @@ void importer::addEntryToLookupTable(SwiftLookupTable &table,
   if (shouldSuppressDeclImport(named))
     return;
 
-  // Leave incomplete struct/enum/union types out of the table; Swift only
-  // handles pointers to them.
-  // FIXME: At some point we probably want to be importing incomplete types,
-  // so that pointers to different incomplete types themselves have distinct
-  // types. At that time it will be necessary to make the decision of whether
-  // or not to import an incomplete type declaration based on whether it's
-  // actually the struct backing a CF type:
-  //
-  //    typedef struct CGColor *CGColorRef;
-  //
-  // The best way to do this is probably to change CFDatabase.def to include
-  // struct names when relevant, not just pointer names. That way we can check
-  // both CFDatabase.def and the objc_bridge attribute and cover all our bases.
-  if (auto *tagDecl = dyn_cast<clang::TagDecl>(named)) {
-    // We add entries for ClassTemplateSpecializations that don't have
-    // definition. It's possible that the decl will be instantiated by
-    // SwiftDeclConverter later on. We cannot force instantiating
-    // ClassTemplateSPecializations here because we're currently writing the
-    // AST, so we cannot modify it.
-    if (!isa<clang::ClassTemplateSpecializationDecl>(named) &&
-        !tagDecl->getDefinition()) {
-      return;
-    }
-  }
   // If we have a name to import as, add this entry to the table.
   auto currentVersion =
       ImportNameVersion::fromOptions(nameImporter.getLangOpts());