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

Author: susmonteiro

include/swift/AST/DiagnosticsClangImporter.def

@@ -170,6 +170,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/ClangImporterRequests.h

@@ -333,8 +333,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

@@ -7855,6 +7855,7 @@ static bool hasNonFirstDefaultArg(const clang::CXXConstructorDecl *ctor) {
   if (ctor->getNumParams() < 2)
     return false;
 
+  // TODO i could just assume that if numParams > 1, they have default args
   auto lastParam = ctor->parameters().back();
   return lastParam->hasDefaultArg();
 }
@@ -7891,7 +7892,7 @@ static bool hasMoveTypeOperations(const clang::CXXRecordDecl *decl) {
     return false;
 
   return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
-    return ctor->isMoveConstructor();
+    return ctor->isMoveConstructor() && !hasNonFirstDefaultArg(ctor);
   });
 }
 
@@ -7910,6 +7911,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()) &&
+           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>();
@@ -7964,6 +7973,9 @@ CxxRecordSemantics::evaluate(Evaluator &evaluator,
                                 "import_iterator", decl->getNameAsString());
     }
 
+    if (hasConstructorWithUnsupportedDefaultArgs(cxxDecl))
+      return CxxRecordSemanticsKind::UnavailableConstructors;
+
     return CxxRecordSemanticsKind::MissingLifetimeOperation;
   }
 

lib/ClangImporter/ImportDecl.cpp

@@ -2927,15 +2927,23 @@ namespace {
       auto semanticsKind =
           evaluateOrDefault(Impl.SwiftContext.evaluator,
                             CxxRecordSemantics({decl, Impl.SwiftContext}), {});
-      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

@@ -566,7 +566,7 @@ namespace {
       if (!cxxRecordDecl)
         return nullptr;
       for (auto ctor : cxxRecordDecl->ctors()) {
-        if (ctor->isMoveConstructor() &&
+        if (ctor->isMoveConstructor() && !hasNonFirstDefaultArg(ctor) &&
             ctor->getAccess() == clang::AS_public && !ctor->isDeleted())
           return ctor;
       }

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

@@ -37,6 +37,20 @@ struct CopyAndMoveConstructor {
   int *ptr = nullptr;
 };
 
+struct MoveConstructorWithOneParameterWithDefaultArg {
+  int value;
+
+  MoveConstructorWithOneParameterWithDefaultArg(int value) : value(value) {}
+
+  MoveConstructorWithOneParameterWithDefaultArg(
+      const MoveConstructorWithOneParameterWithDefaultArg &) = delete;
+
+  MoveConstructorWithOneParameterWithDefaultArg(
+      MoveConstructorWithOneParameterWithDefaultArg &&other =
+          MoveConstructorWithOneParameterWithDefaultArg{1})
+      : value(other.value + 1) {}
+};
+
 struct Base {};
 
 struct ArgType {

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

@@ -274,4 +274,13 @@ struct HasCopyConstructorWithDefaultArgs {
       default;
 };
 
+struct HasMoveConstructorWithDefaultArgs {
+  int value;
+  HasMoveConstructorWithDefaultArgs(int value) : value(value) {}
+
+  HasMoveConstructorWithDefaultArgs(HasMoveConstructorWithDefaultArgs &&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/type-classification-typechecker.swift

@@ -22,13 +22,18 @@ 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 testHasMoveTypeOperations() {
+  let x = HasMoveConstructorWithDefaultArgs(5) // expected-error {{cannot find 'HasMoveConstructorWithDefaultArgs' in scope}}
+}
+
 test()
 testField()
 testAnnotated()
-testNonCopyable()
+testHasCopyTypeOperations()
+testHasMoveTypeOperations()