[cxx-interop] Prevent usage in Swift of C++ move constructor with default args

Author: susmonteiro

include/swift/AST/DiagnosticsClangImporter.def

@@ -174,6 +174,10 @@ NOTE(record_not_automatically_importable, none, "record '%0' is not "
                                                 "does this type have reference "
                                                 "semantics?",
      (StringRef, StringRef))
+NOTE(record_unsupported_default_args, none,
+     "copy constructors and move constructors with more than one parameter are "
+     "unavailable in Swift",
+     ())
 
 NOTE(projection_value_not_imported, none, "C++ method '%0' that returns a value "
                                           "of type '%1' is unavailable",

include/swift/ClangImporter/ClangImporter.h

@@ -688,6 +688,8 @@ class ClangImporter final : public ClangModuleLoader {
       llvm::cas::ObjectStore &CAS, llvm::cas::ObjectRef ChainedPCHIncludeTree);
 
   SourceLoc importSourceLocation(clang::SourceLocation loc) override;
+
+  static bool hasNonFirstDefaultArg(const clang::CXXConstructorDecl *ctor);
 };
 
 ImportDecl *createImportDecl(ASTContext &Ctx, DeclContext *DC, ClangNode ClangN,

include/swift/ClangImporter/ClangImporterRequests.h

@@ -347,8 +347,10 @@ enum class CxxRecordSemanticsKind {
   MoveOnly,
   Reference,
   Iterator,
-  // A record that is either not copyable or not destructible.
+  // A record that is either not copyable/movable or not destructible.
   MissingLifetimeOperation,
+  // A record that has no copy and no move operations
+  UnavailableConstructors,
   // A C++ record that represents a Swift class type exposed to C++ from Swift.
   SwiftClassType
 };

lib/ClangImporter/ClangImporter.cpp

@@ -8010,12 +8010,9 @@ static bool isSufficientlyTrivial(const clang::CXXRecordDecl *decl) {
   return true;
 }
 
-static bool hasNonFirstDefaultArg(const clang::CXXConstructorDecl *ctor) {
-  if (ctor->getNumParams() < 2)
-    return false;
-
-  auto lastParam = ctor->parameters().back();
-  return lastParam->hasDefaultArg();
+bool ClangImporter::hasNonFirstDefaultArg(
+    const clang::CXXConstructorDecl *ctor) {
+  return ctor->getNumParams() > 1;
 }
 
 /// Checks if a record provides the required value type lifetime operations
@@ -8034,7 +8031,7 @@ static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
   // struct.
   return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
     return ctor->isCopyConstructor() && !ctor->isDeleted() &&
-           !hasNonFirstDefaultArg(ctor) &&
+           !ClangImporter::hasNonFirstDefaultArg(ctor) &&
            ctor->getAccess() == clang::AccessSpecifier::AS_public;
   });
 }
@@ -8050,7 +8047,8 @@ static bool hasMoveTypeOperations(const clang::CXXRecordDecl *decl) {
     return false;
 
   return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
-    return ctor->isMoveConstructor();
+    return ctor->isMoveConstructor() &&
+           !ClangImporter::hasNonFirstDefaultArg(ctor);
   });
 }
 
@@ -8069,6 +8067,14 @@ static bool hasCustomCopyOrMoveConstructor(const clang::CXXRecordDecl *decl) {
          decl->hasUserDeclaredMoveConstructor();
 }
 
+static bool
+hasConstructorWithUnsupportedDefaultArgs(const clang::CXXRecordDecl *decl) {
+  return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
+    return (ctor->isCopyConstructor() || ctor->isMoveConstructor()) &&
+           ClangImporter::hasNonFirstDefaultArg(ctor);
+  });
+}
+
 static bool isSwiftClassType(const clang::CXXRecordDecl *decl) {
   // Swift type must be annotated with external_source_symbol attribute.
   auto essAttr = decl->getAttr<clang::ExternalSourceSymbolAttr>();
@@ -8124,6 +8130,9 @@ CxxRecordSemantics::evaluate(Evaluator &evaluator,
                                "import_iterator", decl->getNameAsString());
     }
 
+    if (hasConstructorWithUnsupportedDefaultArgs(cxxDecl))
+      return CxxRecordSemanticsKind::UnavailableConstructors;
+
     return CxxRecordSemanticsKind::MissingLifetimeOperation;
   }
 

lib/ClangImporter/ImportDecl.cpp

@@ -2966,15 +2966,23 @@ namespace {
       auto semanticsKind = evaluateOrDefault(
           Impl.SwiftContext.evaluator,
           CxxRecordSemantics({decl, Impl.SwiftContext, &Impl}), {});
-      if (semanticsKind == CxxRecordSemanticsKind::MissingLifetimeOperation &&
+      if ((semanticsKind == CxxRecordSemanticsKind::MissingLifetimeOperation ||
+           semanticsKind == CxxRecordSemanticsKind::UnavailableConstructors) &&
           // Let un-specialized class templates through. We'll sort out their
-          // members once they're instranciated.
+          // members once they're instantiated.
           !Impl.importSymbolicCXXDecls) {
+
+        if (semanticsKind == CxxRecordSemanticsKind::UnavailableConstructors) {
+          Impl.addImportDiagnostic(
+              decl, Diagnostic(diag::record_unsupported_default_args),
+              decl->getLocation());
+        }
+
         Impl.addImportDiagnostic(
             decl,
             Diagnostic(diag::record_not_automatically_importable,
                        Impl.SwiftContext.AllocateCopy(decl->getNameAsString()),
-                       "does not have a copy constructor or destructor"),
+                       "it must have a copy/move constructor and a destructor"),
             decl->getLocation());
         return nullptr;
       }

lib/IRGen/GenStruct.cpp

@@ -25,6 +25,7 @@
 #include "swift/AST/SubstitutionMap.h"
 #include "swift/AST/Types.h"
 #include "swift/Basic/Assertions.h"
+#include "swift/ClangImporter/ClangImporter.h"
 #include "swift/IRGen/Linking.h"
 #include "swift/SIL/SILFunctionBuilder.h"
 #include "swift/SIL/SILModule.h"
@@ -541,20 +542,13 @@ namespace {
                                   FixedTypeInfo, ClangFieldInfo> {
     const clang::RecordDecl *ClangDecl;
 
-    bool hasNonFirstDefaultArg(const clang::CXXConstructorDecl *ctor) const {
-      if (ctor->getNumParams() < 2)
-        return false;
-
-      auto lastParam = ctor->parameters().back();
-      return lastParam->hasDefaultArg();
-    }
-
     const clang::CXXConstructorDecl *findCopyConstructor() const {
       const auto *cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(ClangDecl);
       if (!cxxRecordDecl)
         return nullptr;
       for (auto ctor : cxxRecordDecl->ctors()) {
-        if (ctor->isCopyConstructor() && !hasNonFirstDefaultArg(ctor) &&
+        if (ctor->isCopyConstructor() &&
+            !ClangImporter::hasNonFirstDefaultArg(ctor) &&
             ctor->getAccess() == clang::AS_public && !ctor->isDeleted())
           return ctor;
       }
@@ -567,6 +561,7 @@ namespace {
         return nullptr;
       for (auto ctor : cxxRecordDecl->ctors()) {
         if (ctor->isMoveConstructor() &&
+            !ClangImporter::hasNonFirstDefaultArg(ctor) &&
             ctor->getAccess() == clang::AS_public && !ctor->isDeleted())
           return ctor;
       }

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -681,7 +681,7 @@ static void addHighLevelFunctionPipeline(SILPassPipelinePlan &P) {
 
   // Skip EagerSpecializer on embedded Swift, which already specializes
   // everything. Otherwise this would create metatype references for functions
-  // with @_specialize attribute and those are incompatible with Emebdded Swift.
+  // with @_specialize attribute and those are incompatible with Embedded Swift.
   if (!P.getOptions().EmbeddedSwift) {
     P.addEagerSpecializer();
   }

test/Interop/Cxx/class/Inputs/constructors.h

@@ -108,4 +108,18 @@ struct HasPtrAuthMember {
 };
 #endif
 
+struct MoveConstructorWithOneParameterWithDefaultArg {
+  int value;
+
+  MoveConstructorWithOneParameterWithDefaultArg(int value) : value(value) {}
+
+  MoveConstructorWithOneParameterWithDefaultArg(
+      const MoveConstructorWithOneParameterWithDefaultArg &) = delete;
+
+  MoveConstructorWithOneParameterWithDefaultArg(
+      MoveConstructorWithOneParameterWithDefaultArg &&other =
+          MoveConstructorWithOneParameterWithDefaultArg{1})
+      : value(other.value + 1) {}
+};
+
 #endif // TEST_INTEROP_CXX_CLASS_INPUTS_CONSTRUCTORS_H

test/Interop/Cxx/class/Inputs/type-classification.h

@@ -274,4 +274,26 @@ struct HasCopyConstructorWithDefaultArgs {
       default;
 };
 
+struct HasMoveConstructorWithDefaultArgs {
+  int value;
+  HasMoveConstructorWithDefaultArgs(int value) : value(value) {}
+
+  HasMoveConstructorWithDefaultArgs(HasMoveConstructorWithDefaultArgs &&other,
+                                    int value = 1)
+      : value(other.value + value) {}
+};
+
+struct HasCopyAndMoveConstructorWithDefaultArgs {
+  int value;
+  HasCopyAndMoveConstructorWithDefaultArgs(int value) : value(value) {}
+
+  HasCopyAndMoveConstructorWithDefaultArgs(
+      const HasCopyAndMoveConstructorWithDefaultArgs &other, int value = 1)
+      : value(other.value + value) {}
+
+  HasCopyAndMoveConstructorWithDefaultArgs(
+      HasCopyAndMoveConstructorWithDefaultArgs &&other, int value = 1)
+      : value(other.value + value) {}
+};
+
 #endif // TEST_INTEROP_CXX_CLASS_INPUTS_TYPE_CLASSIFICATION_H

test/Interop/Cxx/class/constructors-executable.swift

@@ -63,4 +63,11 @@ CxxConstructorTestSuite.test("implicit default ctor") {
   expectNil(instance3.ptr)
 }
 
+CxxConstructorTestSuite.test("MoveConstructorWithOneParamWithDefaultArg") {
+  let instance1 = MoveConstructorWithOneParameterWithDefaultArg(5)
+  let instance2 = instance1
+  let instance3 = MoveConstructorWithOneParameterWithDefaultArg(5)
+  expectEqual(instance2.value, instance3.value + 1)
+}
+
 runAllTests()

test/Interop/Cxx/class/copy-move-assignment-executable.swift

@@ -25,7 +25,7 @@ CxxCopyMoveAssignTestSuite.test("NonTrivialCopyAssign") {
         expectEqual(0, instance.copyAssignCounter)
         takeValue(instance2)
     }
-    // The number of construcors and destructors called for `NonTrivialCopyAssign` must be balanced.
+    // The number of constructors and destructors called for `NonTrivialCopyAssign` must be balanced.
     expectEqual(0, InstanceBalanceCounter.getCounterValue())
 }
 
@@ -37,7 +37,7 @@ CxxCopyMoveAssignTestSuite.test("NonTrivialMoveAssign") {
         // `operator=` isn't called.
         expectEqual(0, instance.moveAssignCounter)
     }
-    // The number of construcors and destructors called for `NonTrivialCopyAssign` must be balanced.
+    // The number of constructors and destructors called for `NonTrivialCopyAssign` must be balanced.
     expectEqual(0, InstanceBalanceCounter.getCounterValue())
 }
 
@@ -51,7 +51,7 @@ CxxCopyMoveAssignTestSuite.test("NonTrivialCopyAndCopyMoveAssign") {
         expectEqual(0, instance.assignCounter)
         takeValue(instance2)
     }
-    // The number of construcors and destructors called for `NonTrivialCopyAndCopyMoveAssign` must be balanced.
+    // The number of constructors and destructors called for `NonTrivialCopyAndCopyMoveAssign` must be balanced.
     expectEqual(0, InstanceBalanceCounter.getCounterValue())
 }
 

test/Interop/Cxx/class/invalid-class-errors.swift

@@ -27,17 +27,17 @@ struct Nested {
 
 import Test
 
-// CHECK: note: record 'A' is not automatically available: does not have a copy constructor or destructor; does this type have reference semantics?
+// CHECK: note: record 'A' is not automatically available: it must have a copy/move constructor and a destructor; does this type have reference semantics?
 // CHECK: struct A {
 // CHECK: ^
 // CHECK: SWIFT_SHARED_REFERENCE(<#retain#>, <#release#>)
 public func test(x: A) { }
-// CHECK: note: record 'B' is not automatically available: does not have a copy constructor or destructor; does this type have reference semantics?
+// CHECK: note: record 'B' is not automatically available: it must have a copy/move constructor and a destructor; does this type have reference semantics?
 // CHECK: struct {{.*}}B {
 // CHECK: ^
 // CHECK: SWIFT_SHARED_REFERENCE(<#retain#>, <#release#>)
 public func test(x: B) { }
-// CHECK: note: record 'Nested' is not automatically available: does not have a copy constructor or destructor; does this type have reference semantics?
+// CHECK: note: record 'Nested' is not automatically available: it must have a copy/move constructor and a destructor; does this type have reference semantics?
 // CHECK: struct Nested {
 // CHECK: ^
 // CHECK: SWIFT_SHARED_REFERENCE(<#retain#>, <#release#>)

test/Interop/Cxx/class/type-classification-typechecker.swift

@@ -22,13 +22,35 @@ func testAnnotated() {
   _ = v
 }
 
-func testNonCopyable() {
+func testHasCopyTypeOperations() {
   let x = HasCopyConstructorWithDefaultArgs(5)
   let v = copy x // expected-error {{'copy' cannot be applied to noncopyable types}}
   _ = v
 }
 
+func testFuncCallNoncopyable() {
+  func aux(input: HasCopyConstructorWithDefaultArgs) {}
+  let x = HasCopyConstructorWithDefaultArgs(5)
+  aux(input: x) 
+  // expected-error@-3 {{parameter of noncopyable type 'HasCopyConstructorWithDefaultArgs' must specify ownership}}
+  // expected-note@-4 {{add 'borrowing' for an immutable reference}}
+  // expected-note@-5 {{add 'consuming' to take the value from the caller}}
+  // expected-note@-6 {{add 'inout' for a mutable reference}}
+}
+
+func testHasMoveTypeOperations() {
+  let x = HasMoveConstructorWithDefaultArgs(5) // expected-error {{cannot find 'HasMoveConstructorWithDefaultArgs' in scope}}
+}
+
+func testHasCopyOrMoveTypeOperations() {
+  let x = HasCopyAndMoveConstructorWithDefaultArgs(5) 
+  // expected-error@-1 {{cannot find 'HasCopyAndMoveConstructorWithDefaultArgs' in scope}}
+}
+
 test()
 testField()
 testAnnotated()
-testNonCopyable()
+testHasCopyTypeOperations()
+testFuncCallNoncopyable()
+testHasMoveTypeOperations()
+testHasCopyOrMoveTypeOperations()