PyROOT calls into deleted copy-constructor in valid C++ scenarios

[vepadulano]

Check duplicate issues.

• [X] Checked for duplicates

Description

Passing arguments to functions that expect types with a deleted copy constructor leads to PyROOT calling said constructor and thus triggering a compilation error:

input_line_35:11:68: error: call to implicitly-deleted copy constructor of 'std::unique_ptr<A, std::default_delete<A> >'
      ((void (&)(std::unique_ptr<A, std::default_delete<A> >))foo)(*(std::unique_ptr<A, std::default_delete<A> >*)args[0]);
                                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__memory/unique_ptr.h:210:59: note: copy constructor is implicitly deleted because 'unique_ptr<A, std::default_delete<A> >' has a user-declared move constructor
  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr(unique_ptr&& __u) _NOEXCEPT
                                                          ^
Traceback (most recent call last):
  File "/Users/vpadulan/Projects/rootcode/rntuple-fix-pyroot-model-create/pass_by_value_unique_ptr.py", line 18, in <module>
    a = ROOT.std.make_unique[ROOT.A](33)
TypeError: Could not instantiate make_unique<A>:
  Failed to instantiate "make_unique<A>(int)"

Reproducer

import ROOT

ROOT.gInterpreter.Declare(r'''

struct A{
    int mA{42};
    void print() { std::cout << "A: " << mA << "\n"; }
    A() {}
    A(int val): mA(val) {}
};

void foo(std::unique_ptr<A> a = std::make_unique<A>()) { a->print(); }

''')

ROOT.foo()

a = ROOT.std.make_unique[ROOT.A](33)
ROOT.foo(a)

ROOT version

master, but any will do

Installation method

built from source

Operating system

Any

Additional context

No response

[vepadulano]

This is dealt with in cppyy with a patch to TCling at https://github.com/wlav/cppyy-backend/blob/25caf988cef1f2f76705c07b7262f076e8ed0e01/cling/src/core/metacling/src/TClingCallFunc.cxx#L468-L485 .

Applying this patch as-is is not possible as it does not work on all platforms and is not generic enough (doesn't take into account the case of a templated move constructor). More in general, it is not yet clear to me that we want to force the implicit move in these cases. Python does not have the equivalent concepts of smart pointers, move semantics etc., so the line is quite blurry here.

[guitargeek]

As you say, the ownership model is a bit too ad-hoc here, and the unique_ptr is not even std::move()d here, which is not good.

If you move it as one should, you can actually use an overload with an rvalue reference signature, which has no drawbacks in this case:

import ROOT

ROOT.gInterpreter.Declare(r'''

struct A{
    int mA{42};
    void print() { std::cout << "A: " << mA << "\n"; }
    A() {}
    A(int val): mA(val) {}
};

void foo(std::unique_ptr<A> && a = std::make_unique<A>()) { a->print(); }

''')

ROOT.foo()

a = ROOT.std.make_unique[ROOT.A](33)
ROOT.foo(ROOT.std.move(a))

Given that it's easy to work around the problem, that this issue is not a regression, and that it was not reported by users yet, I suggest to give this low priority. To be reconsidered if this problem would also get reported by users, or it becomes a bigger nuisance for us developers than it is now.

[vepadulano]

To be reconsidered if this problem would also get reported by users, or it becomes a bigger nuisance for us developers than it is now.

The difference between my original snippet and yours is the function signature, specifically the fact that your function expects an r-value reference while mine expects the argument to be passed by-value. The following is valid C++ code

#include <iostream>
#include <memory>

struct C {
    int m_i{42};
    C() = default;
    C(const C &) = delete;
    C &operator=(const C &) = delete;
    C(C &&) = default;
    C &operator=(C &&) = default;
};

void foo(C c) { std::cout << "C: " << c.m_i << "\n"; }

int main() { foo(C()); }

And the following is an equivalent PyROOT example

import ROOT

ROOT.gInterpreter.Declare(r"""

#include <iostream>
#include <memory>

struct C {
    int m_i{42};
    C() = default;
    C(const C &) = delete;
    C &operator=(const C &) = delete;
    C(C &&) = default;
    C &operator=(C &&) = default;
};

void foo(C c) { std::cout << "C: " << c.m_i << "\n"; }

""")


ROOT.foo(ROOT.C())

# Or the slightly more verbose version
c = ROOT.C()
ROOT.foo(ROOT.std.move(c))

Which fails (in both function call cases) with:

$: python t.py                                                                                                      1 ↵
input_line_38:6:23: error: call to deleted constructor of 'C'
   ((void (&)(C))foo)(*(C*)args[0]);
                      ^~~~~~~~~~~~
input_line_35:9:5: note: 'C' has been explicitly marked deleted here
    C(const C &) = delete;
    ^

Bottom line, the problem at its core is that we cannot make use of function signatures where the input parameter type is non-copyable and the parameter is taken by value. Neither in our APIs, neither in downstream user code which might want to write similar function signatures.

[github-actions[bot]]

Hi @hahnjo, @vepadulano,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely, :robot: