Merge pull request #85793 from susmonteiro/susmonteiro/diagnose-swift-attrs

[cxx-interop] Diagnose invalid swift attributes

Author: susmonteiro

include/swift/AST/DiagnosticsClangImporter.def

@@ -120,10 +120,6 @@ GROUPED_WARNING(import_multiple_mainactor_attr, ClangDeclarationImport, none,
       "this attribute for global actor '%0' is invalid; the declaration already has attribute for global actor '%1'",
       (StringRef, StringRef))
 
-GROUPED_WARNING(contradicting_mutation_attrs, ClangDeclarationImport, none,
-        "attribute '%0' is ignored when combined with attribute '%1'",
-        (StringRef, StringRef))
-
 GROUPED_WARNING(nonmutating_without_const, ClangDeclarationImport, none,
         "attribute 'nonmutating' has no effect on non-const method", ())
 
@@ -199,10 +195,10 @@ NOTE(projection_reference_not_imported, none, "C++ method '%0' that returns a re
 NOTE(projection_may_return_interior_ptr, none, "C++ method '%0' may return an "
                                     "interior pointer",
      (StringRef))
-NOTE(mark_self_contained, none, "annotate type '%0' with 'SWIFT_SELF_CONTAINED' in C++ to "
+NOTE(mark_self_contained, none, "annotate type '%0' with SWIFT_SELF_CONTAINED in C++ to "
                                  "make methods that return it available in Swift",
      (StringRef))
-NOTE(mark_safe_to_import, none, "annotate method '%0' with 'SWIFT_RETURNS_INDEPENDENT_VALUE' in C++ to "
+NOTE(mark_safe_to_import, none, "annotate method '%0' with SWIFT_RETURNS_INDEPENDENT_VALUE in C++ to "
                                 "make it available in Swift",
      (StringRef))
 
@@ -231,8 +227,6 @@ ERROR(private_fileid_attr_repeated, none,
  ERROR(private_fileid_attr_on_incomplete_type, none,
      "SWIFT_PRIVATE_FILEID cannot be applied to incomplete type, '%0'",
      (StringRef))
- NOTE(private_fileid_attr_here, none,
-     "SWIFT_PRIVATE_FILEID annotation found here", ())
 
  GROUPED_WARNING(private_fileid_attr_format_invalid, ClangDeclarationImport, none,
      "SWIFT_PRIVATE_FILEID annotation on '%0' does not have a valid file ID",
@@ -414,6 +408,18 @@ NOTE(
     "SWIFT_REFCOUNTED_PTR on %1 has incorrect signature; expected a function "
     "that takes no arguments and returns a pointer to a foreign reference type",
     (const Decl *, const clang::NamedDecl *))
+ERROR(invalid_conditional_attr, none,
+      "%0 is invalid because it is only supported in "
+      "class templates",
+      (StringRef))
+
+ERROR(conflicting_swift_attrs, none,
+      "multiple conflicting annotations found on %0", (const clang::NamedDecl *))
+
+ERROR(repeating_swift_attrs, none, "multiple %0 annotations found on %1",
+      (StringRef, const clang::NamedDecl *))
+
+NOTE(annotation_here, none, "%0 annotation found here", (StringRef))
 
 #define UNDEFINE_DIAGNOSTIC_MACROS
 #include "DefineDiagnosticMacros.h"

include/swift/AST/DiagnosticsIRGen.def

@@ -96,24 +96,24 @@ NOTE(use_requires_expression, none,
      ())
 
 NOTE(annotate_copyable_if, none,
-     "annotate a type with 'SWIFT_COPYABLE_IF(<T>)' in C++ to specify "
+     "annotate a type with SWIFT_COPYABLE_IF(<T>) in C++ to specify "
      "that the type is Copyable if <T> is Copyable",
      ())
 
 NOTE(annotate_non_copyable, none,
-     "annotate a type with 'SWIFT_NONCOPYABLE' in C++ to import it as "
+     "annotate a type with SWIFT_NONCOPYABLE in C++ to import it as "
      "~Copyable",
      ())
 
 NOTE(maybe_missing_annotation, none,
      "one of the types that %0 depends on may need a 'requires' clause (since "
-     "C++20) in the copy constructor, a 'SWIFT_COPYABLE_IF' annotation or a "
-     "'SWIFT_NONCOPYABLE' annotation'",
+     "C++20) in the copy constructor, a SWIFT_COPYABLE_IF annotation or a "
+     "SWIFT_NONCOPYABLE annotation'",
      (const clang::NamedDecl *))
 
 NOTE(maybe_missing_parameter, none,
      "the %select{'requires' clause on the copy constructor "
-     "of|'SWIFT_COPYABLE_IF' annotation on}0 %1 may be missing a "
+     "of|SWIFT_COPYABLE_IF annotation on}0 %1 may be missing a "
      "%select{constraint|parameter}0",
      (bool, const clang::NamedDecl *))
 

lib/ClangImporter/ClangImporter.cpp

@@ -5455,28 +5455,34 @@ static bool checkConditionalParams(
 }
 
 static std::set<StringRef>
-getConditionalAttrParams(const clang::RecordDecl *decl, StringRef attrName) {
+getConditionalAttrParams(clang::SwiftAttrAttr *swiftAttr, StringRef attrName) {
   std::set<StringRef> result;
-  if (!decl->hasAttrs())
-    return result;
-  for (auto attr : decl->getAttrs()) {
-    if (auto swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr)) {
-      StringRef params = swiftAttr->getAttribute();
-      if (params.consume_front(attrName)) {
-        auto commaPos = params.find(',');
-        StringRef nextParam = params.take_front(commaPos);
-        while (!nextParam.empty() && commaPos != StringRef::npos) {
-          result.insert(nextParam.trim());
-          params = params.drop_front(nextParam.size() + 1);
-          commaPos = params.find(',');
-          nextParam = params.take_front(commaPos);
-        }
-      }
+  StringRef params = swiftAttr->getAttribute();
+  if (params.consume_front(attrName)) {
+    auto commaPos = params.find(',');
+    StringRef nextParam = params.take_front(commaPos);
+    while (!nextParam.empty() && commaPos != StringRef::npos) {
+      result.insert(nextParam.trim());
+      params = params.drop_front(nextParam.size() + 1);
+      commaPos = params.find(',');
+      nextParam = params.take_front(commaPos);
     }
   }
   return result;
 }
 
+static std::set<StringRef>
+getConditionalAttrParams(const clang::NamedDecl *decl, StringRef attrName) {
+  if (decl->hasAttrs()) {
+    for (auto attr : decl->getAttrs()) {
+      if (auto swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr))
+        return getConditionalAttrParams(swiftAttr, attrName);
+    }
+  }
+
+  return {};
+}
+
 static std::set<StringRef>
 getConditionalEscapableAttrParams(const clang::RecordDecl *decl) {
   return getConditionalAttrParams(decl, "escapable_if:");
@@ -5509,15 +5515,6 @@ ClangTypeEscapability::evaluate(Evaluator &evaluator,
   // escapability annotations, malformed escapability annotations)
 
   bool hasUnknown = false;
-  auto desugared = desc.type->getUnqualifiedDesugaredType();
-  if (const auto *recordType = desugared->getAs<clang::RecordType>()) {
-    auto recordDecl = recordType->getDecl();
-    // If the root type has a SWIFT_ESCAPABLE annotation, we import the type as
-    // Escapable without entering the loop
-    if (hasEscapableAttr(recordDecl))
-      return CxxEscapability::Escapable;
-  }
-
   llvm::SmallVector<const clang::Type *, 4> stack;
   // Keep track of Types we've seen to avoid cycles
   llvm::SmallDenseSet<const clang::Type *, 4> seen;
@@ -5551,13 +5548,6 @@ ClangTypeEscapability::evaluate(Evaluator &evaluator,
         hasUnknown &= checkConditionalParams<CxxEscapability>(
             recordDecl, desc.impl, STLParams, conditionalParams,
             maybePushToStack);
-
-        if (desc.impl) {
-          HeaderLoc loc{recordDecl->getLocation()};
-          for (auto name : conditionalParams)
-            desc.impl->diagnose(loc, diag::unknown_template_parameter, name);
-        }
-
         continue;
       }
       // Only try to infer escapability if the record doesn't have any
@@ -8593,16 +8583,12 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
       auto conditionalParams = getConditionalCopyableAttrParams(recordDecl);
 
       if (!STLParams.empty() || !conditionalParams.empty()) {
-        hasUnknown &= checkConditionalParams<CxxValueSemanticsKind>(
-            recordDecl, importerImpl, STLParams, conditionalParams,
-            maybePushToStack);
-
-        if (importerImpl) {
-          HeaderLoc loc{recordDecl->getLocation()};
-          for (auto name : conditionalParams)
-            importerImpl->diagnose(loc, diag::unknown_template_parameter, name);
+        if (isa<clang::ClassTemplateSpecializationDecl>(recordDecl) &&
+            importerImpl) {
+          hasUnknown &= checkConditionalParams<CxxValueSemanticsKind>(
+              recordDecl, importerImpl, STLParams, conditionalParams,
+              maybePushToStack);
         }
-
         continue;
       }
     }
@@ -8636,7 +8622,8 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
     if (!hasDestroyTypeOperations(cxxRecordDecl) ||
         (!isCopyable && !isMovable)) {
 
-      if (hasConstructorWithUnsupportedDefaultArgs(cxxRecordDecl))
+      if (hasConstructorWithUnsupportedDefaultArgs(cxxRecordDecl) &&
+          importerImpl)
         importerImpl->addImportDiagnostic(
             cxxRecordDecl, Diagnostic(diag::record_unsupported_default_args),
             cxxRecordDecl->getLocation());
@@ -9263,6 +9250,85 @@ swift::importer::getCxxRefConventionWithAttrs(const clang::Decl *decl) {
   return std::nullopt;
 }
 
+static const std::vector<std::vector<std::pair<StringRef, StringRef>>>
+    ConflictingSwiftAttrs = {
+        {{"Escapable", "SWIFT_ESCAPABLE"},
+         {"~Escapable", "SWIFT_NONESCAPABLE"},
+         {"escapable_if:", "SWIFT_ESCAPABLE_IF"}},
+        {{"~Copyable", "SWIFT_NONCOPYABLE"},
+         {"copyable_if:", "SWIFT_COPYABLE_IF"}},
+        {{"mutating", "SWIFT_MUTATING"}, {"nonmutating", "'nonmutating'"}}};
+
+static bool isConditionalAttr(StringRef attrName) {
+  return attrName == "escapable_if:" || attrName == "copyable_if:";
+}
+
+void ClangImporter::Implementation::validateSwiftAttributes(
+    const clang::NamedDecl *decl) {
+  if (!decl->hasAttrs())
+    return;
+
+  for (const auto &groupOfAttrs : ConflictingSwiftAttrs) {
+    // stores this decl's attributes, grouped by annotation kind
+    llvm::StringMap<std::vector<clang::SwiftAttrAttr *>> found;
+    unsigned count = 0;
+    for (auto [attrName, annotationName] : groupOfAttrs) {
+      for (auto attr : decl->getAttrs()) {
+        auto swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr);
+        if (swiftAttr && swiftAttr->getAttribute().starts_with(attrName)) {
+          found[annotationName].push_back(swiftAttr);
+          count += 1;
+
+          // in the common case, we validate at most one conditional attribute
+          if (isConditionalAttr(attrName)) {
+            auto specDecl =
+                dyn_cast<clang::ClassTemplateSpecializationDecl>(decl);
+            if (!specDecl) {
+              diagnose(HeaderLoc{swiftAttr->getLocation()},
+                       diag::invalid_conditional_attr, annotationName);
+              continue;
+            }
+            auto conditionalParams =
+                getConditionalAttrParams(swiftAttr, attrName);
+            while (specDecl && !conditionalParams.empty()) {
+              auto templateDecl = specDecl->getSpecializedTemplate();
+              for (auto [idx, param] :
+                   llvm::enumerate(*templateDecl->getTemplateParameters()))
+                conditionalParams.erase(param->getName());
+
+              const clang::DeclContext *dc = specDecl;
+              specDecl = nullptr;
+              while ((dc = dc->getParent())) {
+                specDecl = dyn_cast<clang::ClassTemplateSpecializationDecl>(dc);
+                if (specDecl)
+                  break;
+              }
+            }
+            for (auto name : conditionalParams)
+              diagnose(HeaderLoc{decl->getLocation()},
+                       diag::unknown_template_parameter, name);
+          }
+        }
+      }
+    }
+
+    if (count > 1) {
+      if (found.size() == 1)
+        diagnose(HeaderLoc{decl->getLocation()}, diag::repeating_swift_attrs,
+                 found.begin()->getKey(), decl);
+      else
+        diagnose(HeaderLoc{decl->getLocation()}, diag::conflicting_swift_attrs,
+                 decl);
+
+      for (auto &[annotationName, attrs] : found) {
+        for (auto attr : attrs)
+          diagnose(HeaderLoc{attr->getLocation()}, diag::annotation_here,
+                   annotationName);
+      }
+    }
+  }
+}
+
 NominalType *swift::stripInlineNamespaces(NominalType *outer,
                                           NominalType *inner) {
   if (!outer || !inner || inner == outer)

lib/ClangImporter/ImportDecl.cpp

@@ -2355,6 +2355,7 @@ namespace {
       if (alreadyImportedResult != Impl.ImportedDecls.end())
         return alreadyImportedResult->second;
 
+      Impl.validateSwiftAttributes(decl);
       auto loc = Impl.importSourceLoc(decl->getLocation());
       if (recordHasReferenceSemantics(decl))
         result = Impl.createDeclWithClangNode<ClassDecl>(
@@ -2837,7 +2838,8 @@ namespace {
         Impl.diagnose(HeaderLoc(decl->getLocation()),
                       diag::private_fileid_attr_repeated, decl->getName());
         for (auto ann : anns)
-          Impl.diagnose(HeaderLoc(ann.second), diag::private_fileid_attr_here);
+          Impl.diagnose(HeaderLoc(ann.second), diag::annotation_here,
+                        "SWIFT_PRIVATE_FILEID");
       } else if (anns.size() == 1) {
         auto ann = anns[0];
         if (!SourceFile::FileIDStr::parse(ann.first)) {
@@ -3123,8 +3125,8 @@ namespace {
                         diag::private_fileid_attr_on_incomplete_type,
                         decl->getName());
           for (auto attr : attrs)
-            Impl.diagnose(HeaderLoc(attr.second),
-                          diag::private_fileid_attr_here);
+            Impl.diagnose(HeaderLoc(attr.second), diag::annotation_here,
+                          "SWIFT_PRIVATE_FILEID");
         }
 
         forwardDeclaration = true;
@@ -3651,6 +3653,7 @@ namespace {
       }
 
       checkBridgingAttrs(decl);
+      Impl.validateSwiftAttributes(decl);
 
       return importFunctionDecl(decl, importedName, correctSwiftName,
                                 std::nullopt);
@@ -9003,7 +9006,6 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) {
       ClangDecl = cast<clang::NamedDecl>(maybeDefinition.value());
 
   std::optional<const clang::SwiftAttrAttr *> seenMainActorAttr;
-  const clang::SwiftAttrAttr *seenMutabilityAttr = nullptr;
   llvm::SmallSet<ProtocolDecl *, 4> conformancesSeen;
   const clang::SwiftAttrAttr *seenSendableSuppressionAttr = nullptr;
 
@@ -9054,19 +9056,6 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) {
             }
           }
         }
-
-        // Check for contradicting mutability attr
-        if (seenMutabilityAttr) {
-          StringRef previous = seenMutabilityAttr->getAttribute();
-
-          if (previous != attr) {
-            diagnose(HeaderLoc(swiftAttr->getLocation()),
-                     diag::contradicting_mutation_attrs, attr, previous);
-            continue;
-          }
-        }
-
-        seenMutabilityAttr = swiftAttr;
       }
 
       // Hard-code @actorIndependent, until Objective-C clients start

lib/ClangImporter/ImporterImpl.h

@@ -1128,6 +1128,12 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   Type applyImportTypeAttrs(ImportTypeAttrs attrs, Type type,
                  llvm::function_ref<void(Diagnostic &&)> addImportDiagnosticFn);
 
+  /// Determines whether the given Clang declaration has conflicting
+  /// Swift attributes and emits diagnostics for any violations found.
+  ///
+  /// \param decl The Clang record or function declaration to validate.
+  void validateSwiftAttributes(const clang::NamedDecl *decl);
+
   /// If we already imported a given decl, return the corresponding Swift decl.
   /// Otherwise, return nullptr.
   std::optional<Decl *> importDeclCached(const clang::NamedDecl *ClangDecl,

test/Interop/Cxx/class/Inputs/mutability-annotations.h

@@ -25,11 +25,16 @@ struct HasMutableProperty {
 
   int noAnnotation() const { return b; }
 
-  // expected-warning@+1 {{attribute 'mutating' is ignored when combined with attribute 'nonmutating'}}
+  // expected-error@+3 {{multiple conflicting annotations found on 'contradictingAnnotations'}}
+  // expected-note@+2 {{'nonmutating' annotation found here}}
+  // expected-note@+1 {{SWIFT_MUTATING annotation found here}}
   int contradictingAnnotations() const __attribute__((__swift_attr__("nonmutating"))) __attribute__((__swift_attr__("mutating"))) {
     return b;
   }
 
+  // expected-error@+3 {{multiple 'nonmutating' annotations found on 'duplicateAnnotations'}}
+  // expected-note@+2 {{'nonmutating' annotation found here}}
+  // expected-note@+1 {{'nonmutating' annotation found here}}
   int duplicateAnnotations() const __attribute__((__swift_attr__("nonmutating"))) __attribute__((__swift_attr__("nonmutating"))) {
     return b;
   }

test/Interop/Cxx/class/fixit-add-safe-to-import-self-contained.swift

@@ -23,19 +23,19 @@ struct X {
 import Test
 
 public func test(x: X) {
-  // CHECK: note: annotate method 'test' with 'SWIFT_RETURNS_INDEPENDENT_VALUE' in C++ to make it available in Swift
+  // CHECK: note: annotate method 'test' with SWIFT_RETURNS_INDEPENDENT_VALUE in C++ to make it available in Swift
   // CHECK: int *test() { }
   // CHECK: ^
   // CHECK: SWIFT_RETURNS_INDEPENDENT_VALUE
 
   x.test()
 
-  // CHECK: note: annotate method 'other' with 'SWIFT_RETURNS_INDEPENDENT_VALUE' in C++ to make it available in Swift
+  // CHECK: note: annotate method 'other' with SWIFT_RETURNS_INDEPENDENT_VALUE in C++ to make it available in Swift
   // CHECK: Ptr other() { }
   // CHECK: ^
   // CHECK: SWIFT_RETURNS_INDEPENDENT_VALUE
 
-  // CHECK: note: annotate type 'Ptr' with 'SWIFT_SELF_CONTAINED' in C++ to make methods that return it available in Swift
+  // CHECK: note: annotate type 'Ptr' with SWIFT_SELF_CONTAINED in C++ to make methods that return it available in Swift
   // CHECK: struct Ptr {
   // CHECK: ^
   // CHECK: SWIFT_SELF_CONTAINED