Merge pull request #84517 from susmonteiro/susmonteiro/noncopyable-attr

[cxx-interop] SWIFT_NONCOPYABLE overlooked in some cases

Author: susmonteiro

lib/ClangImporter/ClangImporter.cpp

@@ -8232,17 +8232,15 @@ static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
 }
 
 static bool hasMoveTypeOperations(const clang::CXXRecordDecl *decl) {
-  if (llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
-        return ctor->isMoveConstructor() &&
-               (ctor->isDeleted() || ctor->isIneligibleOrNotSelected() ||
-                ctor->getAccess() != clang::AS_public);
-      }))
-    return false;
+  if (decl->hasSimpleMoveConstructor())
+    return true;
 
   return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
-    return ctor->isMoveConstructor() &&
+    return ctor->isMoveConstructor() && !ctor->isDeleted() &&
+           !ctor->isIneligibleOrNotSelected() &&
            // FIXME: Support default arguments (rdar://142414553)
-           ctor->getNumParams() == 1;
+           ctor->getNumParams() == 1 &&
+           ctor->getAccess() == clang::AS_public;
   });
 }
 
@@ -8374,46 +8372,48 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
       return CxxValueSemanticsKind::Copyable;
   }
 
-  auto injectedStlAnnotation =
-      recordDecl->isInStdNamespace()
-          ? STLConditionalParams.find(recordDecl->getName())
-          : STLConditionalParams.end();
-  auto STLParams = injectedStlAnnotation != STLConditionalParams.end()
-                       ? injectedStlAnnotation->second
-                       : std::vector<int>();
-  auto conditionalParams = getConditionalCopyableAttrParams(recordDecl);
-
-  if (!STLParams.empty() || !conditionalParams.empty()) {
-    HeaderLoc loc{recordDecl->getLocation()};
-    std::function checkArgValueSemantics =
-        [&](clang::TemplateArgument &arg,
-            StringRef argToCheck) -> std::optional<CxxValueSemanticsKind> {
-      if (arg.getKind() != clang::TemplateArgument::Type && importerImpl) {
-        importerImpl->diagnose(loc, diag::type_template_parameter_expected,
-                               argToCheck);
-        return CxxValueSemanticsKind::Unknown;
-      }
+  if (!hasNonCopyableAttr(recordDecl)) {
+    auto injectedStlAnnotation =
+        recordDecl->isInStdNamespace()
+            ? STLConditionalParams.find(recordDecl->getName())
+            : STLConditionalParams.end();
+    auto STLParams = injectedStlAnnotation != STLConditionalParams.end()
+                         ? injectedStlAnnotation->second
+                         : std::vector<int>();
+    auto conditionalParams = getConditionalCopyableAttrParams(recordDecl);
 
-      auto argValueSemantics = evaluateOrDefault(
-          evaluator,
-          CxxValueSemantics(
-              {arg.getAsType()->getUnqualifiedDesugaredType(), importerImpl}),
-          {});
-      if (argValueSemantics != CxxValueSemanticsKind::Copyable)
-        return argValueSemantics;
-      return std::nullopt;
-    };
+    if (!STLParams.empty() || !conditionalParams.empty()) {
+      HeaderLoc loc{recordDecl->getLocation()};
+      std::function checkArgValueSemantics =
+          [&](clang::TemplateArgument &arg,
+              StringRef argToCheck) -> std::optional<CxxValueSemanticsKind> {
+        if (arg.getKind() != clang::TemplateArgument::Type && importerImpl) {
+          importerImpl->diagnose(loc, diag::type_template_parameter_expected,
+                                 argToCheck);
+          return CxxValueSemanticsKind::Unknown;
+        }
 
-    auto result = checkConditionalParams<CxxValueSemanticsKind>(
-        recordDecl, STLParams, conditionalParams, checkArgValueSemantics);
-    if (result.has_value())
-      return result.value();
+        auto argValueSemantics = evaluateOrDefault(
+            evaluator,
+            CxxValueSemantics(
+                {arg.getAsType()->getUnqualifiedDesugaredType(), importerImpl}),
+            {});
+        if (argValueSemantics != CxxValueSemanticsKind::Copyable)
+          return argValueSemantics;
+        return std::nullopt;
+      };
 
-    if (importerImpl)
-      for (auto name : conditionalParams)
-        importerImpl->diagnose(loc, diag::unknown_template_parameter, name);
+      auto result = checkConditionalParams<CxxValueSemanticsKind>(
+          recordDecl, STLParams, conditionalParams, checkArgValueSemantics);
+      if (result.has_value())
+        return result.value();
 
-    return CxxValueSemanticsKind::Copyable;
+      if (importerImpl)
+        for (auto name : conditionalParams)
+          importerImpl->diagnose(loc, diag::unknown_template_parameter, name);
+
+      return CxxValueSemanticsKind::Copyable;
+    }
   }
 
   const auto cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(recordDecl);
@@ -8423,7 +8423,8 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
     return CxxValueSemanticsKind::Copyable;
   }
 
-  bool isCopyable = hasCopyTypeOperations(cxxRecordDecl);
+  bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) && 
+                    hasCopyTypeOperations(cxxRecordDecl);
   bool isMovable = hasMoveTypeOperations(cxxRecordDecl);
 
   if (!hasDestroyTypeOperations(cxxRecordDecl) || (!isCopyable && !isMovable)) {

lib/ClangImporter/SwiftDeclSynthesizer.cpp

@@ -3011,10 +3011,8 @@ FuncDecl *SwiftDeclSynthesizer::findExplicitDestroy(
       ctx.evaluator, 
       CxxValueSemantics({clangType->getTypeForDecl(), &ImporterImpl}), {});
 
-  if (valueSemanticsKind == CxxValueSemanticsKind::MoveOnly)
-    return destroyFunc;
-
-  if (valueSemanticsKind != CxxValueSemanticsKind::Copyable)
+  if (valueSemanticsKind != CxxValueSemanticsKind::Copyable &&
+      valueSemanticsKind != CxxValueSemanticsKind::MoveOnly)
     return nullptr;
 
   auto cxxRecordSemanticsKind = evaluateOrDefault(
@@ -3033,6 +3031,9 @@ FuncDecl *SwiftDeclSynthesizer::findExplicitDestroy(
       }
     }
 
+    if (valueSemanticsKind == CxxValueSemanticsKind::MoveOnly)
+      return destroyFunc;
+
     markDeprecated(
         nominal,
         "destroy operation '" +

test/Interop/C/struct/Inputs/noncopyable-struct.h

@@ -47,7 +47,7 @@ void badDestroy2(BadDestroyNonCopyableType2 *ptr);
 
 struct __attribute__((swift_attr("~Copyable"))) __attribute__((swift_attr("destroy:extraDestroy"))) ExtraDestroy {
   void *storage;
-
+  ExtraDestroy(ExtraDestroy&&) = default;
   ~ExtraDestroy() { }
 };
 

test/Interop/C/struct/noncopyable_structs_nontrivial.swift

@@ -3,7 +3,6 @@
 // RUN: %target-swift-frontend -emit-sil -I %S/Inputs/ -I %swift_src_root/lib/ClangImporter/SwiftBridging %s -verify -DERRORS -verify-additional-prefix conly-
 // RUN: %target-swift-frontend -emit-sil -I %S/Inputs/ -I %swift_src_root/lib/ClangImporter/SwiftBridging %s -verify -DERRORS -DCPLUSPLUS -verify-additional-prefix cplusplus- -cxx-interoperability-mode=default
 
-// XFAIL: OS=windows-msvc
 import NoncopyableStructs
 
 #if CPLUSPLUS

test/Interop/Cxx/class/noncopyable-typechecker.swift

@@ -1,7 +1,7 @@
 // RUN: %empty-directory(%t)
 // RUN: split-file %s %t
-// RUN: %target-swift-frontend -cxx-interoperability-mode=default -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t/Inputs %t/test.swift
-// RUN: %target-swift-frontend -cxx-interoperability-mode=default -Xcc -std=c++20 -verify-additional-prefix cpp20- -D CPP20 -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t/Inputs %t/test.swift
+// RUN: %target-swift-frontend -cxx-interoperability-mode=default -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t%{fs-sep}Inputs %t%{fs-sep}test.swift -verify-additional-file %t%{fs-sep}Inputs%{fs-sep}noncopyable.h
+// RUN: %target-swift-frontend -cxx-interoperability-mode=default -Xcc -std=c++20 -verify-additional-prefix cpp20- -D CPP20 -typecheck -verify -I %swift_src_root/lib/ClangImporter/SwiftBridging -I %t%{fs-sep}Inputs %t%{fs-sep}test.swift -verify-additional-file %t%{fs-sep}Inputs%{fs-sep}noncopyable.h
 
 //--- Inputs/module.modulemap
 module Test {
@@ -68,6 +68,18 @@ MyPair<int, NonCopyableRequires> p7();
 
 #endif
 
+template<typename T>
+struct SWIFT_COPYABLE_IF(T) SWIFT_NONCOPYABLE DoubleAnnotation {};
+
+using DoubleAnnotationInt = DoubleAnnotation<int>;
+
+struct SWIFT_NONCOPYABLE NonCopyableNonMovable { // expected-note {{record 'NonCopyableNonMovable' is not automatically available: it must have a copy/move constructor and a destructor; does this type have reference semantics?}}
+    NonCopyableNonMovable() {}
+    NonCopyableNonMovable(const NonCopyableNonMovable& other) {}
+    NonCopyableNonMovable(NonCopyableNonMovable&& other) = delete;
+};
+
+
 //--- test.swift
 import Test
 import CxxStdlib
@@ -107,3 +119,13 @@ func useOfRequires() {
     takeCopyable(p7()) // expected-cpp20-error {{global function 'takeCopyable' requires that 'MyPair<CInt, RequiresCopyableT<NonCopyable>>' conform to 'Copyable'}}
 }
 #endif
+
+func doubleAnnotation() {
+    let s = DoubleAnnotationInt()
+    takeCopyable(s) // expected-error {{global function 'takeCopyable' requires that 'DoubleAnnotationInt' (aka 'DoubleAnnotation<CInt>') conform to 'Copyable'}}
+}
+
+func missingLifetimeOperation() {
+    let s = NonCopyableNonMovable() // expected-error {{cannot find 'NonCopyableNonMovable' in scope}}
+    takeCopyable(s)
+}