[cxx-interop] Add diagnostic for contradicting mutability annotations

Author: skrtks

include/swift/AST/DiagnosticsClangImporter.def

@@ -106,6 +106,9 @@ WARNING(import_multiple_mainactor_attr,none,
       "this attribute for global actor '%0' is invalid; the declaration already has attribute for global actor '%1'",
       (StringRef, StringRef))
 
+WARNING(contradicting_mutation_attrs,none,
+        "attribute 'nonmutating' is ignored when combined with attribute 'mutating'", ())
+
 ERROR(module_map_not_found, none, "module map file '%0' not found", (StringRef))
 
 NOTE(macro_not_imported_unsupported_operator, none, "operator not supported in macro arithmetic", ())

lib/ClangImporter/ImportDecl.cpp

@@ -8768,12 +8768,13 @@ SourceFile &ClangImporter::Implementation::getClangSwiftAttrSourceFile(
 }
 
 bool swift::importer::isMainActorAttr(const clang::SwiftAttrAttr *swiftAttr) {
-  if (swiftAttr->getAttribute() == "@MainActor" ||
-      swiftAttr->getAttribute() == "@UIActor") {
-    return true;
-  }
+  return swiftAttr->getAttribute() == "@MainActor" ||
+         swiftAttr->getAttribute() == "@UIActor";
+}
 
-  return false;
+bool swift::importer::isMutabilityAttr(const clang::SwiftAttrAttr *swiftAttr) {
+  return swiftAttr->getAttribute() == "mutating" ||
+         swiftAttr->getAttribute() == "nonmutating";
 }
 
 void
@@ -8792,7 +8793,8 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) {
     if (maybeDefinition.getValue())
       ClangDecl = cast<clang::NamedDecl>(maybeDefinition.getValue());
 
-  Optional<const clang::SwiftAttrAttr *> SeenMainActorAttr;
+  Optional<const clang::SwiftAttrAttr *> seenMainActorAttr;
+  Optional<const clang::SwiftAttrAttr *> seenMutabilityAttr;
   PatternBindingInitializer *initContext = nullptr;
 
   auto importAttrsFromDecl = [&](const clang::NamedDecl *ClangDecl) {
@@ -8803,26 +8805,47 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) {
       // FIXME: Hard-code @MainActor and @UIActor, because we don't have a
       // point at which to do name lookup for imported entities.
       if (isMainActorAttr(swiftAttr)) {
-        if (SeenMainActorAttr) {
+        if (seenMainActorAttr) {
           // Cannot add main actor annotation twice. We'll keep the first
           // one and raise a warning about the duplicate.
           HeaderLoc attrLoc(swiftAttr->getLocation());
           diagnose(attrLoc, diag::import_multiple_mainactor_attr,
                    swiftAttr->getAttribute(),
-                   SeenMainActorAttr.getValue()->getAttribute());
+                   seenMainActorAttr.getValue()->getAttribute());
           continue;
         }
 
         if (Type mainActorType = SwiftContext.getMainActorType()) {
           auto typeExpr = TypeExpr::createImplicit(mainActorType, SwiftContext);
           auto attr = CustomAttr::create(SwiftContext, SourceLoc(), typeExpr);
           MappedDecl->getAttrs().add(attr);
-          SeenMainActorAttr = swiftAttr;
+          seenMainActorAttr = swiftAttr;
         }
 
         continue;
       }
 
+      if (isMutabilityAttr(swiftAttr)) {
+        if (seenMutabilityAttr) {
+          StringRef seenAttribute =
+              seenMutabilityAttr.getValue()->getAttribute();
+          if ((seenAttribute == "nonmutating" &&
+               swiftAttr->getAttribute() == "mutating") ||
+              (seenAttribute == "mutating" &&
+               swiftAttr->getAttribute() == "nonmutating")) {
+            const clang::SwiftAttrAttr *nonmutatingAttr =
+                seenAttribute == "nonmutating" ? seenMutabilityAttr.getValue()
+                                               : swiftAttr;
+
+            diagnose(HeaderLoc(nonmutatingAttr->getLocation()),
+                     diag::contradicting_mutation_attrs);
+            continue;
+          }
+        }
+
+        seenMutabilityAttr = swiftAttr;
+      }
+
       // Hard-code @actorIndependent, until Objective-C clients start
       // using nonisolated.
       if (swiftAttr->getAttribute() == "@actorIndependent") {

lib/ClangImporter/ImporterImpl.h

@@ -1849,6 +1849,9 @@ class SwiftNameLookupExtension : public clang::ModuleFileExtension {
 /// actor.
 bool isMainActorAttr(const clang::SwiftAttrAttr *swiftAttr);
 
+/// Determines whether the given swift_attr controls mutability
+bool isMutabilityAttr(const clang::SwiftAttrAttr *swiftAttr);
+
 /// Apply an attribute to a function type.
 static inline Type applyToFunctionType(
     Type type, llvm::function_ref<ASTExtInfo(ASTExtInfo)> transform) {

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

@@ -24,6 +24,15 @@ struct HasMutableProperty {
   }
 
   int noAnnotation() const { return b; }
+
+  // expected-warning@+1 {{attribute 'nonmutating' is ignored when combined with attribute 'mutating'}}
+  int contradictingAnnotations() const __attribute__((__swift_attr__("nonmutating"))) __attribute__((__swift_attr__("mutating"))) {
+    return b;
+  }
+
+  int duplicateAnnotations() const __attribute__((__swift_attr__("nonmutating"))) __attribute__((__swift_attr__("nonmutating"))) {
+    return b;
+  }
 };
 
 #endif // TEST_INTEROP_CXX_CLASS_INPUTS_MUTABILITY_ANNOTATIONS_H

test/Interop/Cxx/class/mutability-annotations-module-interface.swift

@@ -9,6 +9,8 @@
 // CHECK: struct HasMutableProperty {
 // CHECK:   func annotatedNonMutating() -> Int32
 // CHECK:   mutating func noAnnotation() -> Int32
+// CHECK:   mutating func contradictingAnnotations() -> Int32
+// CHECK:   func duplicateAnnotations() -> Int32
 // CHECK:   var a: Int32
 // CHECK:   var b: Int32
 // CHECK: }

test/Interop/Cxx/class/mutability-annotations-typechecker.swift

@@ -11,3 +11,5 @@ let objWMutableProperty = HasMutableProperty(a: 42, b: 21) // expected-note {{ch
 
 let _ = objWMutableProperty.annotatedNonMutating()
 let _ = objWMutableProperty.noAnnotation() // expected-error {{cannot use mutating member on immutable value: 'objWMutableProperty' is a 'let' constant}}
+let _ = objWMutableProperty.contradictingAnnotations() // expected-error {{cannot use mutating member on immutable value: 'objWMutableProperty' is a 'let' constant}}
+let _ = objWMutableProperty.duplicateAnnotations()