[BUG] make_unique not annotated with except +

[wence-]

Describe the bug

When make_unique was provided in memory.pxd in #487, it didn't come with an except + annotation. At the time, this was due to the Cython compiler not emitting the correct std::move(...) in the subsequent assignment in the transpiled source code.

AFAICT this can be worked around in a reasonable manner by explicitly putting a move call in the rvalue of the assignment, and although that is less ergonomic, perhaps it is better because exceptions will actually be caught.
In the intervening years, it looks like this restriction has gone, and so except + should be reintroduced. The issue here is that a (potentially) throwing constructor wrapped by make_unique does not

Code to reproduce the behaviour:

from libcpp.utility cimport move
from libcpp.memory cimport unique_ptr
from libcpp.vector cimport vector

def some_function(int n):
    cdef unique_ptr[vector[double]] vec

    with nogil:
        vec = move(make_unique[vector[double]](n))

    return vec.get().size()

Expected behaviour

I expect this to be transpiled to:

        /* "cython_except.pyx":9
 * 
 *     with nogil:
 *         vec = move(make_unique[vector[double]](n))             # <<<<<<<<<<<<<<
 * 
 *     return vec.get().size()
 */
        try {
          __pyx_t_1 = std::make_unique<std::vector<double> >(__pyx_v_n);
        } catch(...) {
          #ifdef WITH_THREAD
          PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
          #endif
          __Pyx_CppExn2PyErr();
          #ifdef WITH_THREAD
          __Pyx_PyGILState_Release(__pyx_gilstate_save);
          #endif
          __PYX_ERR(0, 9, __pyx_L4_error)
        }
        __pyx_v_vec = cython_std::move<std::unique_ptr<std::vector<double> > >(__PYX_STD_MOVE_IF_SUPPORTED(__pyx_t_1));
      }

Whereas in fact it is transpiled to:

      /*try:*/ {

        /* "cython_except.pyx":9
 * 
 *     with nogil:
 *         vec = move(make_unique[vector[double]](n))             # <<<<<<<<<<<<<<
 * 
 *     return vec.get().size()
 */
        __pyx_v_vec = cython_std::move<std::unique_ptr<std::vector<double> > >(std::make_unique<std::vector<double> >(__pyx_v_n));
      }

OS

Linux

Python version

3.10.6

Cython version

3.0.0

Additional context

No response

[da-woods]

Maybe I'm missing some detail of C++, but I believe the explicit call to std::move should be unnecessary and the generated code can be simpler:

// exception stuff as above
__pyx_v_vec = __PYX_STD_MOVE_IF_SUPPORTED(__pyx_t_1);

If that's the case there's basically no reason not to add except+ to our definition since all existing code would continue to work. The only question is whether the code detecting move support is robust enough.

[vyasr]

I think that is correct. From some quick local testing that does appear to work. I'm going to try rebuilding a few of my libraries with this change to verify that there are no surprises.

[wence-]

Maybe I'm missing some detail of C++, but I believe the explicit call to std::move should be unnecessary and the generated code can be simpler:

// exception stuff as above
__pyx_v_vec = __PYX_STD_MOVE_IF_SUPPORTED(__pyx_t_1);

If that's the case there's basically no reason not to add except+ to our definition since all existing code would continue to work. The only question is whether the code detecting move support is robust enough.

I was (also) testing with Cython < 3 which transpiles this example:

from libcpp.utility cimport move
from libcpp.memory cimport unique_ptr
from libcpp.vector cimport vector

cdef extern from "<memory>" namespace "std" nogil:
    unique_ptr[T] make_unique[T](...) except +

def some_function(int n):
    cdef unique_ptr[vector[double]] vec

    with nogil:
        vec = make_unique[vector[double]](n)

    return vec.get().size()

to

      /*try:*/ {

        /* "cython_except.pyx":12
 * 
 *     with nogil:
 *         vec = make_unique[vector[double]](n)             # <<<<<<<<<<<<<<
 * 
 *     return vec.get().size()
 */
        try {
          __pyx_t_1 = std::make_unique<std::vector<double> >(__pyx_v_n);
        } catch(...) {
          #ifdef WITH_THREAD
          PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
          #endif
          __Pyx_CppExn2PyErr();
          #ifdef WITH_THREAD
          __Pyx_PyGILState_Release(__pyx_gilstate_save);
          #endif
          __PYX_ERR(0, 12, __pyx_L4_error)
        }
        __pyx_v_vec = __pyx_t_1;
      }

However, with cython 3 we get

      /*try:*/ {

        /* "cython_except.pyx":12
 * 
 *     with nogil:
 *         vec = make_unique[vector[double]](n)             # <<<<<<<<<<<<<<
 * 
 *     return vec.get().size()
 */
        try {
          __pyx_t_1 = std::make_unique<std::vector<double> >(__pyx_v_n);
        } catch(...) {
          #ifdef WITH_THREAD
          PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
          #endif
          __Pyx_CppExn2PyErr();
          #ifdef WITH_THREAD
          __Pyx_PyGILState_Release(__pyx_gilstate_save);
          #endif
          __PYX_ERR(0, 12, __pyx_L4_error)
        }
        __pyx_v_vec = __PYX_STD_MOVE_IF_SUPPORTED(__pyx_t_1);
      }

So I agree it looks like the explicit move should not be necessary. Though perhaps that also means one shouldn't backport to cython < 3 since existing code might break.

[da-woods]

I don't think this should be backported since it needs updated features to work. I've proposed a change. I've also changed how we detect MOVE_IF_SUPPORTED to let the user force it to on. My one worry with this signature change is that it breaks compatibility for anyone who we aren't detecting move support for. And having a way to force it on mitigates that.