From 979fa49da1988f262536ca54cd21946b68413a73 Mon Sep 17 00:00:00 2001 From: Saleem Abdulrasool Date: Fri, 7 Oct 2022 16:31:40 -0700 Subject: [PATCH 1/2] ClangImporter: simplify logic for mutability attribute Take the spellings for the attributes as parameters for the diagnostics to allow us to reference the ignored the attribute properly. Use direct pointer checks instead of an unnecessary `llvm::Optional` around a nullable pointer. Use `llvm::StringRef` to simplify the comparison. This avoids re-spelling the attribute parameters which have already been checked by `isMutabilityAttr` for the contradiction. --- .../swift/AST/DiagnosticsClangImporter.def | 3 ++- lib/ClangImporter/ImportDecl.cpp | 23 ++++++++----------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/include/swift/AST/DiagnosticsClangImporter.def b/include/swift/AST/DiagnosticsClangImporter.def index b8818f82ae561..b305464d38ad8 100644 --- a/include/swift/AST/DiagnosticsClangImporter.def +++ b/include/swift/AST/DiagnosticsClangImporter.def @@ -107,7 +107,8 @@ WARNING(import_multiple_mainactor_attr,none, (StringRef, StringRef)) WARNING(contradicting_mutation_attrs,none, - "attribute 'nonmutating' is ignored when combined with attribute 'mutating'", ()) + "attribute '%0' is ignored when combined with attribute '%1'", + (StringRef, StringRef)) WARNING(nonmutating_without_const,none, "attribute 'nonmutating' has no effect on non-const method", ()) diff --git a/lib/ClangImporter/ImportDecl.cpp b/lib/ClangImporter/ImportDecl.cpp index 2c2b8e23eae6e..b04a72b5d7fd2 100644 --- a/lib/ClangImporter/ImportDecl.cpp +++ b/lib/ClangImporter/ImportDecl.cpp @@ -7242,7 +7242,7 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) { ClangDecl = cast(maybeDefinition.getValue()); Optional seenMainActorAttr; - Optional seenMutabilityAttr; + const clang::SwiftAttrAttr *seenMutabilityAttr = nullptr; PatternBindingInitializer *initContext = nullptr; auto importAttrsFromDecl = [&](const clang::NamedDecl *ClangDecl) { @@ -7274,14 +7274,16 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) { } if (isMutabilityAttr(swiftAttr)) { + StringRef attr = swiftAttr->getAttribute(); // Check if 'nonmutating' attr is applicable - if (swiftAttr->getAttribute() == "nonmutating") { + if (attr == "nonmutating") { if (auto *method = dyn_cast(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); @@ -7291,18 +7293,11 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) { // 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); + StringRef previous = seenMutabilityAttr->getAttribute(); + + if (previous != attr) { + diagnose(HeaderLoc(swiftAttr->getLocation()), + diag::contradicting_mutation_attrs, attr, previous); continue; } } From ad1bd8e6850d95d0e0e8d8e37bc577004ce74eb1 Mon Sep 17 00:00:00 2001 From: Saleem Abdulrasool Date: Sat, 8 Oct 2022 20:33:17 -0700 Subject: [PATCH 2/2] Update mutability-annotations.h --- test/Interop/Cxx/class/Inputs/mutability-annotations.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Interop/Cxx/class/Inputs/mutability-annotations.h b/test/Interop/Cxx/class/Inputs/mutability-annotations.h index a5fa4afa42fc4..2d2b95d9c6c4b 100644 --- a/test/Interop/Cxx/class/Inputs/mutability-annotations.h +++ b/test/Interop/Cxx/class/Inputs/mutability-annotations.h @@ -25,7 +25,7 @@ struct HasMutableProperty { int noAnnotation() const { return b; } - // expected-warning@+1 {{attribute 'nonmutating' is ignored when combined with attribute 'mutating'}} + // expected-warning@+1 {{attribute 'mutating' is ignored when combined with attribute 'nonmutating'}} int contradictingAnnotations() const __attribute__((__swift_attr__("nonmutating"))) __attribute__((__swift_attr__("mutating"))) { return b; }