Merge pull request #42372 from skrtks/clang-nonmutating-attribute

Clang nonmutating attribute

Author: egorzhdan

include/swift/AST/DiagnosticsClangImporter.def

@@ -106,6 +106,15 @@ 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'", ())
+
+WARNING(nonmutating_without_const,none,
+        "attribute 'nonmutating' has no effect on non-const method", ())
+
+WARNING(nonmutating_without_mutable_fields,none,
+        "attribute 'nonmutating' has no effect without any mutable fields", ())
+
 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", ())

include/swift/ClangImporter/ClangImporter.h

@@ -516,6 +516,8 @@ class ClangImporter final : public ClangModuleLoader {
 
   bool isCXXMethodMutating(const clang::CXXMethodDecl *method) override;
 
+  bool isAnnotatedWith(const clang::CXXMethodDecl *method, StringRef attr);
+
   /// Find the lookup table that corresponds to the given Clang module.
   ///
   /// \param clangModule The module, or null to indicate that we're talking

lib/ClangImporter/ClangImporter.cpp

@@ -5656,18 +5656,20 @@ ClangImporter::getCXXFunctionTemplateSpecialization(SubstitutionMap subst,
 
 bool ClangImporter::isCXXMethodMutating(const clang::CXXMethodDecl *method) {
   return isa<clang::CXXConstructorDecl>(method) || !method->isConst() ||
-         // method->getParent()->hasMutableFields() ||
-         // FIXME(rdar://91961524): figure out a way to handle mutable fields
-         // without breaking classes from the C++ standard library (e.g.
-         // `std::string` which has a mutable member in old libstdc++ version
-         // used on CentOS 7)
-         (method->hasAttrs() &&
-          llvm::any_of(method->getAttrs(), [](clang::Attr *a) {
-            if (auto swiftAttr = dyn_cast<clang::SwiftAttrAttr>(a)) {
-              return swiftAttr->getAttribute() == "mutating";
-            }
-            return false;
-          }));
+         (method->getParent()->hasMutableFields() &&
+          !isAnnotatedWith(method, "nonmutating")) ||
+         isAnnotatedWith(method, "mutating");
+}
+
+bool ClangImporter::isAnnotatedWith(const clang::CXXMethodDecl *method,
+                                    StringRef attr) {
+  return method->hasAttrs() &&
+         llvm::any_of(method->getAttrs(), [attr](clang::Attr *a) {
+           if (auto swiftAttr = dyn_cast<clang::SwiftAttrAttr>(a)) {
+             return swiftAttr->getAttribute() == attr;
+           }
+           return false;
+         });
 }
 
 SwiftLookupTable *

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,63 @@ 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)) {
+
+        // Check if 'nonmutating' attr is applicable
+        if (swiftAttr->getAttribute() == "nonmutating") {
+          if (auto *method = dyn_cast<clang::CXXMethodDecl>(ClangDecl)) {
+            if (!method->isConst()) {
+              diagnose(HeaderLoc(swiftAttr->getLocation()),
+                       diag::nonmutating_without_const);
+            }
+            if (!method->getParent()->hasMutableFields()) {
+              diagnose(HeaderLoc(swiftAttr->getLocation()),
+                       diag::nonmutating_without_mutable_fields);
+            }
+          }
+        }
+
+        // Check for contradicting mutability attr
+        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

@@ -15,4 +15,37 @@ struct HasConstMethodAnnotatedAsMutating {
   }
 };
 
+struct HasMutableProperty {
+  mutable int a;
+  int b;
+
+  int annotatedNonMutating() const __attribute__((__swift_attr__("nonmutating"))) {
+    return b;
+  }
+
+  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;
+  }
+};
+
+struct NoMutableProperty {
+  int a;
+
+  // expected-warning@+1 {{attribute 'nonmutating' has no effect without any mutable fields}}
+  int isConst() const __attribute__((__swift_attr__("nonmutating"))) {
+    return a;
+  }
+
+  // expected-warning@+2 {{attribute 'nonmutating' has no effect without any mutable fields}}
+  // expected-warning@+1 {{attribute 'nonmutating' has no effect on non-const method}}
+  int nonConst() __attribute__((__swift_attr__("nonmutating"))) { return a; }
+};
+
 #endif // TEST_INTEROP_CXX_CLASS_INPUTS_MUTABILITY_ANNOTATIONS_H

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

@@ -5,3 +5,18 @@
 // CHECK:   mutating func annotatedMutatingWithOtherAttrs() -> Int32
 // CHECK:   var a: Int32
 // CHECK: }
+
+// 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: }
+
+// CHECK: struct NoMutableProperty {
+// CHECK:   func isConst() -> Int32
+// CHECK:   mutating func nonConst() -> Int32
+// CHECK:   var a: Int32
+// CHECK: }

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

@@ -1,6 +1,18 @@
-// RUN: %target-typecheck-verify-swift -I %S/Inputs -enable-experimental-cxx-interop
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -I %S/Inputs %s -enable-experimental-cxx-interop -verify -verify-additional-file %S/Inputs/mutability-annotations.h
 
 import MutabilityAnnotations
 
 let obj = HasConstMethodAnnotatedAsMutating(a: 42) // expected-note {{change 'let' to 'var' to make it mutable}}
 let i = obj.annotatedMutating() // expected-error {{cannot use mutating member on immutable value: 'obj' is a 'let' constant}}
+
+let objWMutableProperty = HasMutableProperty(a: 42, b: 21) // expected-note {{change 'let' to 'var' to make it mutable}}
+// expected-note@-1 {{change 'let' to 'var' to make it mutable}}
+
+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()
+
+let objWithoutMutableProperty = NoMutableProperty(a: 42) // expected-note {{change 'let' to 'var' to make it mutable}}
+let _ = objWithoutMutableProperty.isConst()
+let _ = objWithoutMutableProperty.nonConst() // expected-error {{cannot use mutating member on immutable value: 'objWithoutMutableProperty' is a 'let' constant}}