[core] Move arguments with deleted copy ctor in TClingCallFunc

core/metacling/src/TClingCallFunc.cxx

@@ -229,6 +229,30 @@
    GetTypeAsString(QT, type_name, C, Policy);
 }
 
+static bool IsCopyConstructorDeleted(QualType QT)
+{
+   CXXRecordDecl *RD = QT->getAsCXXRecordDecl();
+   if (!RD) {
+      // For types that are not C++ records (such as PODs), we assume that they are copyable, ie their copy constructor
+      // is not deleted.
+      return false;
+   }
+
+   RD = RD->getDefinition();
+   assert(RD && "expecting a definition");
+
+   if (RD->hasSimpleCopyConstructor())
+      return false;
+
+   for (auto *Ctor : RD->ctors()) {
+      if (Ctor->isCopyConstructor()) {
+         return Ctor->isDeleted();
+      }
+   }
+
+   assert(0 && "did not find a copy constructor?");
+}
+
 void TClingCallFunc::make_narg_ctor(const unsigned N, ostringstream &typedefbuf,
                                     ostringstream &callbuf, const string &class_name,
                                     int indent_level)
@@ -240,6 +264,10 @@
    const FunctionDecl *FD = GetDecl();
 
    callbuf << "new " << class_name << "(";
+
+   // IsCopyConstructorDeleted could trigger deserialization of decls.
+   cling::Interpreter::PushTransactionRAII RAII(fInterp);
+
    for (unsigned i = 0U; i < N; ++i) {
       const ParmVarDecl *PVD = FD->getParamDecl(i);
       QualType Ty = PVD->getType();
@@ -268,7 +296,17 @@
          callbuf << "*(" << type_name.c_str() << "**)args["
                  << i << "]";
       } else {
+         // By-value construction: Figure out if the type can be copy-constructed. This is tricky and cannot be done in
+         // a fully reliable way, also because std::vector<T> always defines a copy constructor, even if the type T is
+         // only moveable. As a heuristic, we only check if the copy constructor is deleted, or would be if implicit.
+         bool Move = IsCopyConstructorDeleted(QT);
+         if (Move) {
+            callbuf << "static_cast<" << type_name << "&&>(";
+         }
          callbuf << "*(" << type_name.c_str() << "*)args[" << i << "]";
+         if (Move) {
+            callbuf << ")";
+         }
       }
    }
    callbuf << ")";
@@ -354,6 +392,10 @@
    if (ShouldCastFunction) callbuf << ")";
 
    callbuf << "(";
+
+   // IsCopyConstructorDeleted could trigger deserialization of decls.
+   cling::Interpreter::PushTransactionRAII RAII(fInterp);
+
    for (unsigned i = 0U; i < N; ++i) {
       const ParmVarDecl *PVD = FD->getParamDecl(i);
       QualType Ty = PVD->getType();
@@ -383,9 +425,17 @@
          callbuf << "*(" << type_name.c_str() << "**)args["
                  << i << "]";
       } else {
-         // pointer falls back to non-pointer case; the argument preserves
-         // the "pointerness" (i.e. doesn't reference the value).
+         // By-value construction: Figure out if the type can be copy-constructed. This is tricky and cannot be done in
+         // a fully reliable way, also because std::vector<T> always defines a copy constructor, even if the type T is
+         // only moveable. As a heuristic, we only check if the copy constructor is deleted, or would be if implicit.
+         bool Move = IsCopyConstructorDeleted(QT);
+         if (Move) {
+            callbuf << "static_cast<" << type_name << "&&>(";
+         }
          callbuf << "*(" << type_name.c_str() << "*)args[" << i << "]";
+         if (Move) {
+            callbuf << ")";
+         }
       }
    }
    callbuf << ")";

core/metacling/test/CMakeLists.txt

@@ -31,3 +31,7 @@ ROOT_ADD_GTEST(
 add_dependencies(TClingTest Cling RIO)
 
 ROOT_ADD_GTEST(TClingLoadUnloadFile TClingLoadUnloadFileTests.cxx LIBRARIES Cling RIO)
+
+if(pyroot)
+  ROOT_ADD_PYUNITTEST(TClingCallFuncTests TClingCallFuncTests.py)
+endif()

core/metacling/test/TClingCallFuncTests.cxx

@@ -324,3 +324,182 @@ TEST(TClingCallFunc, GH_14405) {
   result = gInterpreter->CallFunc_ExecDouble(cf, /*address=*/0);
   EXPECT_NEAR(result, 7.28, /*abs_error=*/1e-6);
 }
+
+TEST(TClingCallFunc, GH_14425)
+{
+   gInterpreter->Declare(R"cpp(
+                           struct GH_14425 {
+                              int fMember;
+                              GH_14425(int m = 1) : fMember(m) {}
+                              GH_14425(const GH_14425&) = delete;
+                              GH_14425(GH_14425&&) = default;
+                           };
+                           int GH_14425_f(GH_14425 p = GH_14425()) { return p.fMember; }
+                           int GH_14425_g(GH_14425 p) { return p.fMember; }
+                           struct GH_14425_Copyable {
+                              int fMember;
+                              GH_14425_Copyable(int m = 1) : fMember(m) {}
+                              GH_14425_Copyable(const GH_14425_Copyable &o) : fMember(o.fMember) {}
+                              GH_14425_Copyable(GH_14425_Copyable &&o) : fMember(o.fMember) { o.fMember = 0; }
+                           };
+                           int GH_14425_h(GH_14425_Copyable p) { return p.fMember; }
+                           struct GH_14425_Default {
+                              int fMember;
+                              GH_14425_Default(GH_14425 p = GH_14425()) : fMember(p.fMember) {}
+                           };
+                           struct GH_14425_Required {
+                              int fMember;
+                              GH_14425_Required(GH_14425 p) : fMember(p.fMember) {}
+                           };
+                           )cpp");
+   CallFuncRAII CfDefaultRAII("", "GH_14425_f", "");
+   int valDefault = gInterpreter->CallFunc_ExecInt(CfDefaultRAII.GetCF(), /*address*/ 0);
+   EXPECT_EQ(valDefault, 1);
+
+   CallFuncRAII CfArgumentRAII("", "GH_14425_f", "GH_14425");
+   CallFunc_t *CfArgument = CfArgumentRAII.GetCF();
+   // Cheat a bit: GH_14425 has only one int fMember in memory...
+   int objArgument = 2;
+   gInterpreter->CallFunc_SetArg(CfArgument, &objArgument);
+   int valArgument = gInterpreter->CallFunc_ExecInt(CfArgument, /*address*/ 0);
+   EXPECT_EQ(valArgument, 2);
+
+   CallFuncRAII CfRequiredRAII("", "GH_14425_g", "GH_14425");
+   CallFunc_t *CfRequired = CfRequiredRAII.GetCF();
+   // Cheat a bit: GH_14425 has only one int fMember in memory...
+   int objRequired = 3;
+   gInterpreter->CallFunc_SetArg(CfRequired, &objRequired);
+   int valRequired = gInterpreter->CallFunc_ExecInt(CfRequired, /*address*/ 0);
+   EXPECT_EQ(valRequired, 3);
+
+   CallFuncRAII CfCopyableRAII("", "GH_14425_h", "GH_14425_Copyable");
+   CallFunc_t *CfCopyable = CfCopyableRAII.GetCF();
+   // Cheat a bit: GH_14425_Copyable has only one int fMember in memory...
+   int objCopyable = 4;
+   gInterpreter->CallFunc_SetArg(CfCopyable, &objCopyable);
+   int valCopyable = gInterpreter->CallFunc_ExecInt(CfCopyable, /*address*/ 0);
+   EXPECT_EQ(valCopyable, 4);
+   // The original value should not have changed; if it did, TClingCallFunc called the move constructor.
+   EXPECT_EQ(objCopyable, 4);
+
+   CallFuncRAII CfConstructorDefaultRAII("GH_14425_Default", "GH_14425_Default", "");
+   int *valConstructorDefault;
+   gInterpreter->CallFunc_ExecWithReturn(CfConstructorDefaultRAII.GetCF(), /*address*/ 0, &valConstructorDefault);
+   EXPECT_EQ(*valConstructorDefault, 1);
+
+   CallFuncRAII CfConstructorArgumentRAII("GH_14425_Default", "GH_14425_Default", "GH_14425");
+   CallFunc_t *CfConstructorArgument = CfConstructorArgumentRAII.GetCF();
+   // Cheat a bit: GH_14425 has only one int fMember in memory...
+   int objConstructorArgument = 2;
+   gInterpreter->CallFunc_SetArg(CfConstructorArgument, &objConstructorArgument);
+   int *valConstructorArgument;
+   gInterpreter->CallFunc_ExecWithReturn(CfConstructorArgument, /*address*/ 0, &valConstructorArgument);
+   EXPECT_EQ(*valConstructorArgument, 2);
+
+   CallFuncRAII CfConstructorRequiredRAII("GH_14425_Required", "GH_14425_Required", "GH_14425");
+   CallFunc_t *CfConstructorRequired = CfConstructorRequiredRAII.GetCF();
+   // Cheat a bit: GH_14425 has only one int fMember in memory...
+   int objConstructorRequired = 3;
+   gInterpreter->CallFunc_SetArg(CfConstructorRequired, &objConstructorRequired);
+   int *valConstructorRequired;
+   gInterpreter->CallFunc_ExecWithReturn(CfConstructorRequired, /*address*/ 0, &valConstructorRequired);
+   EXPECT_EQ(*valConstructorRequired, 3);
+}
+
+TEST(TClingCallFunc, GH_14425_Virtual)
+{
+   // Virtual classes are a bit more complicated, we need to declare them both compiled and in the interpreter.
+   struct GH_14425_Virtual {
+      int fMember;
+      GH_14425_Virtual(int m = 1) : fMember(m) {}
+      GH_14425_Virtual(const GH_14425_Virtual &) = default;
+      GH_14425_Virtual(GH_14425_Virtual &&o) : fMember(o.fMember) { o.fMember = 0; }
+      void f() {}
+   };
+   gInterpreter->Declare(R"cpp(
+                           struct GH_14425_Virtual {
+                              int fMember;
+                              GH_14425_Virtual(int m = 1) : fMember(m) {}
+                              GH_14425_Virtual(const GH_14425_Virtual &) = default;
+                              GH_14425_Virtual(GH_14425_Virtual &&o) : fMember(o.fMember) { o.fMember = 0; }
+                              void f() {}
+                           };
+                           int GH_14425_v(GH_14425_Virtual p) { return p.fMember; }
+                           )cpp");
+   CallFuncRAII CfVirtualRAII("", "GH_14425_v", "GH_14425_Virtual");
+   CallFunc_t *CfVirtual = CfVirtualRAII.GetCF();
+   GH_14425_Virtual objVirtual(2);
+   gInterpreter->CallFunc_SetArg(CfVirtual, &objVirtual);
+   int valVirtual = gInterpreter->CallFunc_ExecInt(CfVirtual, /*address*/ 0);
+   EXPECT_EQ(valVirtual, 2);
+   // The original value should not have changed; if it did, TClingCallFunc called the move constructor.
+   EXPECT_EQ(objVirtual.fMember, 2);
+}
+
+TEST(TClingCallFunc, GH_14425_Templates)
+{
+   // While according to the C++ standard, GH_14425_Moveable has no move-constructor (which must not be a template),
+   // it has a template constructor that can be instantiated to the move-constructor signature. MSVC uses this for their
+   // implementation of std::unique_ptr.
+   gInterpreter->Declare(R"cpp(
+                           struct GH_14425_Moveable {
+                              int fMember = 0;
+                              GH_14425_Moveable(int m = 1) : fMember(m) {};
+                              GH_14425_Moveable(const GH_14425_Moveable&) = delete;
+                              template <typename T>
+                              GH_14425_Moveable(T &&t) : fMember(t.fMember) {}
+                           };
+                           int GH_14425_Moveable_f(GH_14425_Moveable p) { return p.fMember; }
+                           struct GH_14425_Moveable_Required {
+                              int fMember;
+                              GH_14425_Moveable_Required(GH_14425_Moveable p) : fMember(p.fMember) {}
+                           };
+                           template <typename T> struct GH_14425_T {
+                              T fMember = 0;
+                              GH_14425_T(T m = 1) : fMember(m) {}
+                              GH_14425_T(const GH_14425_T&) = delete;
+                              GH_14425_T(GH_14425_T &&t) = default;
+                           };
+                           template <typename T>
+                           T GH_14425_t(GH_14425_T<T> p) { return p.fMember; }
+                           template <typename T> struct GH_14425_I {
+                              std::unique_ptr<T> fMember;
+                              GH_14425_I(std::unique_ptr<T> m) : fMember(std::move(m)) {}
+                           };
+                           template <typename T>
+                           T GH_14425_i(GH_14425_I<T> p) { return *p.fMember; }
+                           )cpp");
+   CallFuncRAII CfMoveableRAII("", "GH_14425_Moveable_f", "GH_14425_Moveable");
+   CallFunc_t *CfMoveable = CfMoveableRAII.GetCF();
+   // Cheat a bit: GH_14425_Moveable has only one int fMember in memory...
+   int objMoveable = 4;
+   gInterpreter->CallFunc_SetArg(CfMoveable, &objMoveable);
+   int valMoveable = gInterpreter->CallFunc_ExecInt(CfMoveable, /*address*/ 0);
+   EXPECT_EQ(valMoveable, 4);
+
+   CallFuncRAII CfConstructorRequiredRAII("GH_14425_Moveable_Required", "GH_14425_Moveable_Required",
+                                          "GH_14425_Moveable");
+   CallFunc_t *CfConstructorRequired = CfConstructorRequiredRAII.GetCF();
+   // Cheat a bit: GH_14425_Moveable has only one int fMember in memory...
+   int objConstructorRequired = 5;
+   gInterpreter->CallFunc_SetArg(CfConstructorRequired, &objConstructorRequired);
+   int *valConstructor;
+   gInterpreter->CallFunc_ExecWithReturn(CfConstructorRequired, /*address*/ 0, &valConstructor);
+   EXPECT_EQ(*valConstructor, 5);
+
+   CallFuncRAII CfTRAII("", "GH_14425_t<int>", "GH_14425_T<int>");
+   CallFunc_t *CfT = CfTRAII.GetCF();
+   // Cheat a bit: GH_14425_T<int> has only one int fMember in memory...
+   int objT = 6;
+   gInterpreter->CallFunc_SetArg(CfT, &objT);
+   int valT = gInterpreter->CallFunc_ExecInt(CfT, /*address*/ 0);
+   EXPECT_EQ(valT, 6);
+
+   CallFuncRAII CfIRAII("", "GH_14425_i<int>", "GH_14425_I<int>");
+   CallFunc_t *CfI = CfIRAII.GetCF();
+   // Cheat a bit: GH_14425_I<int> has only one std::unique_ptr<int> fMember in memory...
+   auto objI = std::make_unique<int>(7);
+   gInterpreter->CallFunc_SetArg(CfI, &objI);
+   int valI = gInterpreter->CallFunc_ExecInt(CfI, /*address*/ 0);
+   EXPECT_EQ(valI, 7);
+}

core/metacling/test/TClingCallFuncTests.py

@@ -0,0 +1,34 @@
+import unittest
+
+import ROOT
+
+class TClingCallFuncTest(unittest.TestCase):
+    """Tests related to TClingCallFunc usage from Python"""
+
+    def test_GH_14425(self):
+        """Can call a function with non-copyable argument."""
+
+        ROOT.gInterpreter.Declare(r"""
+struct GH_14425 {
+   int fMember;
+   GH_14425(int m = 1) : fMember(m) {}
+   GH_14425(const GH_14425&) = delete;
+   GH_14425(GH_14425&&) = default;
+};
+int GH_14425_f(GH_14425 p = GH_14425()) { return p.fMember; }
+int GH_14425_g(GH_14425 p) { return p.fMember; }
+struct GH_14425_Default {
+   int fMember;
+   GH_14425_Default(GH_14425 p = GH_14425()) : fMember(p.fMember) {}
+};
+struct GH_14425_Required {
+   int fMember;
+   GH_14425_Required(GH_14425 p) : fMember(p.fMember) {}
+};
+""")
+        self.assertEqual(ROOT.GH_14425_f(), 1)
+        self.assertEqual(ROOT.GH_14425_f(ROOT.GH_14425(2)), 2)
+        self.assertEqual(ROOT.GH_14425_g(ROOT.GH_14425(3)), 3)
+        self.assertEqual(ROOT.GH_14425_Default().fMember, 1)
+        self.assertEqual(ROOT.GH_14425_Default(ROOT.GH_14425(2)).fMember, 2)
+        self.assertEqual(ROOT.GH_14425_Required(ROOT.GH_14425(3)).fMember, 3)

tree/ntuple/v7/test/CMakeLists.txt

@@ -136,5 +136,6 @@ endif()
 
 # RNTuple Python interface tests
 if(pyroot)
+  ROOT_ADD_PYUNITTEST(ntuple_py_basics ntuple_basics.py)
   ROOT_ADD_PYUNITTEST(ntuple_py_model ntuple_model.py)
 endif()

tree/ntuple/v7/test/ntuple_basics.py

@@ -0,0 +1,22 @@
+import unittest
+
+import ROOT
+
+RNTupleModel = ROOT.Experimental.RNTupleModel
+RNTupleReader = ROOT.Experimental.RNTupleReader
+RNTupleWriter = ROOT.Experimental.RNTupleWriter
+
+class RNTupleBasics(unittest.TestCase):
+    """Basic tests of using RNTuple from Python"""
+
+    def test_write_read(self):
+        """Can write and read a basic RNTuple."""
+
+        model = RNTupleModel.Create()
+        model.MakeField["int"]("f")
+        writer = RNTupleWriter.Recreate(model, "ntpl", "test_ntuple_py_write_read.root")
+        writer.Fill()
+        del writer
+
+        reader = RNTupleReader.Open("ntpl", "test_ntuple_py_write_read.root")
+        self.assertEqual(reader.GetNEntries(), 1)