Repository: cython/cython
Pull request: Support cpp_locals and std::move on prange temps
Fixes #4354

Problem statement:
[BUG] cpp_locals generates broken code for thread local temp variables
**Describe the bug**
When using cpp_locals(True) in combination with  exception handling inside of a prange, an incorrect assignment is generated. The following code:
```cython
cimport cython
from cython.parallel import prange

cdef extern from *:
    """
    class Test {
    };
    """
    cdef cppclass Test:
        Test() nogil except +

@cython.cpp_locals(True)
cdef test():
    cdef int i
    for i in prange(10, nogil=True):
        var = Test()
```
uses std::optional<Test> to store the variable, while it uses Test to store the thread local variable. This results in an invalid assignment:
```
CYTHON_UNUSED __Pyx_Optional_Type<Test> __pyx_v_var;
__Pyx_Optional_Type<Test> __pyx_t_3;
...
Test __pyx_parallel_temp1;
...
__pyx_t_3 = Test();
__pyx_v_var = std::move((*__pyx_t_3));
...
__pyx_parallel_temp1 = __pyx_v_var;
```
here the last assignment to `__pyx_parallel_temp1` is invalid.

**Expected behavior**
The temporary variable `__pyx_parallel_temp1` should use the type `__Pyx_Optional_Type<Test>` as well.

Hints from issue discussion:
I'd obviously missed that pranges have their own type of temp. I also wonder if the assignment to parallel_temp should use `std::move` too (although that would need a close look at the context it's used in)
> I also wonder if the assignment to parallel_temp should use std::move too (although that would need a close look at the context it's used in)

I wondered this too, since it would help me with my problem from https://github.com/cython/cython/issues/4352.

Commit message: Support cpp_locals and std::move on prange temps

Fixes #4354

Patch to review:
diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py
index 4b0da59cf46..47d9864ab3f 100644
--- a/Cython/Compiler/Nodes.py
+++ b/Cython/Compiler/Nodes.py
@@ -9237,7 +9237,10 @@ def save_parallel_vars(self, code):
             if not lastprivate or entry.type.is_pyobject:
                 continue
 
-            type_decl = entry.type.empty_declaration_code()
+            if entry.type.is_cpp_class and not entry.type.is_fake_reference and code.globalstate.directives['cpp_locals']:
+                type_decl = entry.type.cpp_optional_declaration_code("")
+            else:
+                type_decl = entry.type.empty_declaration_code()
             temp_cname = "__pyx_parallel_temp%d" % temp_count
             private_cname = entry.cname
 
@@ -9251,10 +9254,17 @@ def save_parallel_vars(self, code):
             # Declare the parallel private in the outer block
             c.putln("%s %s%s;" % (type_decl, temp_cname, init))
 
+            self.parallel_private_temps.append((temp_cname, private_cname, entry.type))
+
+            if entry.type.is_cpp_class:
+                # moving is fine because we're quitting the loop and so won't be directly accessing the variable again
+                code.globalstate.use_utility_code(
+                    UtilityCode.load_cached("MoveIfSupported", "CppSupport.cpp"))
+                private_cname = "__PYX_STD_MOVE_IF_SUPPORTED(%s)" % private_cname
             # Initialize before escaping
             code.putln("%s = %s;" % (temp_cname, private_cname))
 
-            self.parallel_private_temps.append((temp_cname, private_cname))
+
 
         code.end_block()  # end critical section
 
@@ -9375,7 +9385,10 @@ def end_parallel_control_flow_block(
             code.putln(
                 "if (%s) {" % Naming.parallel_why)
 
-            for temp_cname, private_cname in self.parallel_private_temps:
+            for temp_cname, private_cname, temp_type in self.parallel_private_temps:
+                if temp_type.is_cpp_class:
+                    # utility code was loaded earlier
+                    temp_cname = "__PYX_STD_MOVE_IF_SUPPORTED(%s)" % temp_cname
                 code.putln("%s = %s;" % (private_cname, temp_cname))
 
             code.putln("switch (%s) {" % Naming.parallel_why)
diff --git a/tests/compile/cpp_temp_assignment.pyx b/tests/compile/cpp_temp_assignment.pyx
index 58ae39a70a9..8ef7f45448c 100644
--- a/tests/compile/cpp_temp_assignment.pyx
+++ b/tests/compile/cpp_temp_assignment.pyx
@@ -1,6 +1,6 @@
 # tag: cpp,cpp11
-# mode: compile 
-# tag: no-cpp-locals
+# mode: compile
+# tag: no-cpp-locals, openmp
 # TODO cpp_locals works fine with the standard library that comes with gcc11
 # but not with gcc8. Therefore disable the test for now
 
@@ -49,6 +49,9 @@ cdef extern from *:
     NoAssign get_NoAssign_Cpp() {
         return NoAssign();
     }
+    NoAssign get_NoAssign() {
+        return NoAssign();
+    }
 
     """
     cdef cppclass NoAssignIterator:
@@ -65,6 +68,8 @@ cdef extern from *:
     NoAssign get_NoAssign_Py() except *
     # might raise C++ exception (thus needs a temp)
     NoAssign get_NoAssign_Cpp() except +
+    # just a plan factory function
+    NoAssign get_NoAssign() nogil
 
 cdef internal_cpp_func(NoAssign arg):
     pass
@@ -96,3 +101,10 @@ cdef class AssignToClassAttr:
 def test_generator_cpp_iterator_as_temp():
     for i in get_NoAssign_Py():
         yield i
+
+from cython.parallel cimport prange
+
+def test_in_prange():
+    cdef int i
+    for i in prange(10, nogil=True):
+        var = get_NoAssign()


Code review comments:
diff_hunk: @@ -96,3 +101,10 @@ cdef class AssignToClassAttr:
 def test_generator_cpp_iterator_as_temp():
     for i in get_NoAssign_Py():
         yield i
+
+from cython.parallel cimport prange
+
+def test_in_prange():
+    cdef int i
+    for i in prange(10, nogil=True):
+        var = get_NoAssign()
path: tests/compile/cpp_temp_assignment.pyx
text: @author:
So it looks like GCC takes `lastprivate` to require a copy-constructor (not sure about MSVC). I it's probably (just) worth generating the move constructors but I don't think it's easy to test.