Fix a few bugs in the new "cpp_locals" directive implementation (GH-4259)

1) Entry utility_code wasn't being imported unless the entry was visibly used. The utility code is needed for the definition even if unused. I think this is what utility_code_definition is for, but I'm not completely sure. I also had to add some calls to ensure it's used.

2) CppOptionalTempCoercions weren't being moved correctly. Tested by printing from the destructor.

3) cdef-class attributes weren't being created or destroyed correctly. Tested by printing from the destructor

Cython/Compiler/Code.py

@@ -2083,6 +2083,7 @@ class CCodeWriter(object):
         elif entry.type.is_pyobject:
             self.put(" = NULL")
         self.putln(";")
+        self.funcstate.scope.use_entry_utility_code(entry)
 
     def put_temp_declarations(self, func_context):
         for name, type, manage_ref, static in func_context.temps_allocated:

Cython/Compiler/ExprNodes.py

@@ -841,6 +841,9 @@ class ExprNode(Node):
         self.not_implemented("generate_result_code")
 
     def generate_disposal_code(self, code):
+        if self.has_temp_moved:
+            code.globalstate.use_utility_code(
+                    UtilityCode.load_cached("MoveIfSupported", "CppSupport.cpp"))
         if self.is_temp:
             if self.type.is_string or self.type.is_pyunicode_ptr:
                 # postponed from self.generate_evaluation_code()
@@ -849,9 +852,6 @@ class ExprNode(Node):
             if self.result():
                 code.put_decref_clear(self.result(), self.ctype(),
                                         have_gil=not self.in_nogil_context)
-            if self.has_temp_moved:
-                code.globalstate.use_utility_code(
-                    UtilityCode.load_cached("MoveIfSupported", "CppSupport.cpp"))
         else:
             # Already done if self.is_temp
             self.generate_subexpr_disposal_code(code)
@@ -13814,6 +13814,11 @@ class CppOptionalTempCoercion(CoercionNode):
     def generate_result_code(self, code):
         pass
 
+    def _make_move_result_rhs(self, result, optional=False):
+        # this wouldn't normally get moved (because it isn't a temp), but force it to be because it
+        # is a thin wrapper around a temp
+        return super(CppOptionalTempCoercion, self)._make_move_result_rhs(result, optional=False)
+
 
 class CMethodSelfCloneNode(CloneNode):
     # Special CloneNode for the self argument of builtin C methods

Cython/Compiler/ModuleNode.py

@@ -1281,6 +1281,7 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
                 decl = attr_type.cpp_optional_declaration_code(attr.cname)
             else:
                 decl = attr_type.declaration_code(attr.cname)
+            type.scope.use_entry_utility_code(attr)
             code.putln("%s;" % decl)
         code.putln(footer)
         if type.objtypedef_cname is not None:
@@ -1371,6 +1372,7 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
             code.putln(";")
             if entry.cname != cname:
                 code.putln("#define %s (*%s)" % (entry.cname, cname))
+            env.use_entry_utility_code(entry)
 
     def generate_cfunction_declarations(self, env, code, definition):
         for entry in env.cfunc_entries:
@@ -1477,8 +1479,7 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
         if scope.is_internal:
             # internal classes (should) never need None inits, normal zeroing will do
             py_attrs = []
-        cpp_constructable_attrs = [entry for entry in scope.var_entries
-                                   if (entry.type.needs_cpp_construction and not entry.is_cpp_optional)]
+        cpp_constructable_attrs = [entry for entry in scope.var_entries if entry.type.needs_cpp_construction]
 
         cinit_func_entry = scope.lookup_here("__cinit__")
         if cinit_func_entry and not cinit_func_entry.is_special:
@@ -1583,8 +1584,12 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
                 struct_type_cast, type.vtabptr_cname))
 
         for entry in cpp_constructable_attrs:
+            if entry.is_cpp_optional:
+                decl_code = entry.type.cpp_optional_declaration_code("")
+            else:
+                decl_code = entry.type.empty_declaration_code()
             code.putln("new((void*)&(p->%s)) %s();" % (
-                entry.cname, entry.type.empty_declaration_code()))
+                entry.cname, decl_code))
 
         for entry in py_attrs:
             if entry.name == "__dict__":

Cython/Compiler/Symtab.py

@@ -274,7 +274,7 @@ class Entry(object):
         assert self.type.is_cpp_class
         self.is_cpp_optional = True
         assert not self.utility_code  # we're not overwriting anything?
-        self.utility_code = Code.UtilityCode.load_cached("OptionalLocals", "CppSupport.cpp")
+        self.utility_code_definition = Code.UtilityCode.load_cached("OptionalLocals", "CppSupport.cpp")
 
 
 class InnerEntry(Entry):

tests/run/cpp_optional_temps.pyx

@@ -5,17 +5,36 @@
 
 cimport cython
 
+from libcpp cimport bool as cppbool
+
 cdef extern from *:
-    """
+    r"""
+    static void print_C_destructor();
+
     class C {
         public:
             C() = delete; // look! No default constructor
-            C(int x) : x(x) {}
+            C(int x, bool print_destructor=true) : x(x), print_destructor(print_destructor) {}
+            C(C&& rhs) : x(rhs.x), print_destructor(rhs.print_destructor) {
+                rhs.print_destructor = false; // moved-from instances are deleted silently
+            }
+            C& operator=(C&& rhs) {
+                x=rhs.x;
+                print_destructor=rhs.print_destructor;
+                rhs.print_destructor = false; // moved-from instances are deleted silently
+                return *this;
+            }
+            C(const C& rhs) = default;
+            C& operator=(const C& rhs) = default;
+            ~C() {
+                if (print_destructor) print_C_destructor();
+            }
 
             int getX() const { return x; }
 
         private:
             int x;
+            bool print_destructor;
     };
 
     C make_C(int x) {
@@ -24,26 +43,35 @@ cdef extern from *:
     """
     cdef cppclass C:
         C(int)
+        C(int, cppbool)
         int getX() const
     C make_C(int) except +  # needs a temp to receive
 
-def maybe_assign_infer(assign, value):
+# this function just makes sure the output from the destructor can be captured by doctest
+cdef void print_C_destructor "print_C_destructor" () nogil:
+    print("~C()")
+
+def maybe_assign_infer(assign, value, do_print):
     """
-    >>> maybe_assign_infer(True, 5)
+    >>> maybe_assign_infer(True, 5, True)
     5
-    >>> maybe_assign_infer(False, 0)
+    ~C()
+    >>> maybe_assign_infer(False, 0, True)
     Traceback (most recent call last):
         ...
     UnboundLocalError: local variable 'x' referenced before assignment
+    >>> maybe_assign_infer(False, 0, False)  # no destructor call here
     """
     if assign:
         x = C(value)
-    print(x.getX())
+    if do_print:
+        print(x.getX())
 
 def maybe_assign_cdef(assign, value):
     """
     >>> maybe_assign_cdef(True, 5)
     5
+    ~C()
     >>> maybe_assign_cdef(False, 0)
     Traceback (most recent call last):
         ...
@@ -58,6 +86,7 @@ def maybe_assign_annotation(assign, value):
     """
     >>> maybe_assign_annotation(True, 5)
     5
+    ~C()
     >>> maybe_assign_annotation(False, 0)
     Traceback (most recent call last):
         ...
@@ -72,6 +101,7 @@ def maybe_assign_directive1(assign, value):
     """
     >>> maybe_assign_directive1(True, 5)
     5
+    ~C()
     >>> maybe_assign_directive1(False, 0)
     Traceback (most recent call last):
         ...
@@ -87,6 +117,7 @@ def maybe_assign_directive2(assign, value):
     """
     >>> maybe_assign_directive2(True, 5)
     5
+    ~C()
     >>> maybe_assign_directive2(False, 0)
     Traceback (most recent call last):
         ...
@@ -100,6 +131,7 @@ def maybe_assign_nocheck(assign, value):
     """
     >>> maybe_assign_nocheck(True, 5)
     5
+    ~C()
 
     # unfortunately it's quite difficult to test not assigning because there's a decent chance it'll crash
     """
@@ -113,6 +145,7 @@ def uses_temp(value):
     needs a temp to handle the result of make_C - still doesn't use the default constructor
     >>> uses_temp(10)
     10
+    ~C()
     """
 
     x = make_C(value)
@@ -127,32 +160,47 @@ def call_has_argument():
     >>> call_has_argument()
     50
     """
-    has_argument(C(50))
+    has_argument(C(50, False))
 
 cdef class HoldsC:
     """
-    >>> inst = HoldsC(True)
+    >>> inst = HoldsC(True, False)
     >>> inst.getCX()
     10
-    >>> inst = HoldsC(False)
+    >>> inst = HoldsC(False, False)
     >>> inst.getCX()
     Traceback (most recent call last):
         ...
     AttributeError: C++ attribute 'value' is not initialized
     """
     cdef C value
-    def __cinit__(self, initialize):
+    def __cinit__(self, initialize, print_destructor):
         if initialize:
-            self.value = C(10)
+            self.value = C(10, print_destructor)
 
     def getCX(self):
         return self.value.getX()
 
+def dont_test_on_pypy(f):
+    import sys
+    if not hasattr(sys, "pypy_version_info"):
+        return f
+
+@dont_test_on_pypy  # non-deterministic destruction
+def testHoldsCDestruction(initialize):
+    """
+    >>> testHoldsCDestruction(True)
+    ~C()
+    >>> testHoldsCDestruction(False)  # no destructor
+    """
+    x = HoldsC(initialize, True)
+    del x
+
 cdef C global_var
 
 def initialize_global_var():
     global global_var
-    global_var = C(-1)
+    global_var = C(-1, False)
 
 def read_global_var():
     """

tests/run/cpp_optional_temps_unused.pyx

+# mode: run
+# tag: cpp, cpp17
+
+# cython: cpp_locals=True
+
+cdef cppclass C:
+    C()
+
+cdef class PyC:
+    """
+    >>> PyC() and None  # doesn't really do anything, but should run
+    """
+    cdef C  # this limited usage wasn't triggering the creation of utility code