revert actual tp_finalize() usages, needs more thought

CHANGES.rst

@@ -8,10 +8,6 @@ Cython Changelog
 Features added
 --------------
 
-* Starting with CPython 3.4, the user provided finalisation code in the
-  ``__dealloc__()`` special method is called by ``tp_finalize()`` instead
-  of ``tp_dealloc()`` to provide a safer execution environment.
-
 * During cyclic garbage collection, attributes of extension types that
   cannot create reference cycles due to their type (e.g. strings) are
   no longer considered for traversal or clearing.

Cython/Compiler/ModuleNode.py

@@ -1006,7 +1006,6 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
                 if scope: # could be None if there was an error
                     self.generate_exttype_vtable(scope, code)
                     self.generate_new_function(scope, code, entry)
-                    self.generate_finalize_function(scope, code)
                     self.generate_dealloc_function(scope, code)
                     if scope.needs_gc():
                         self.generate_traverse_function(scope, code, entry)
@@ -1173,40 +1172,33 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
         slot_func = scope.mangle_internal("tp_dealloc")
         base_type = scope.parent_type.base_type
         if tp_slot.slot_code(scope) != slot_func:
-            return  # never used
-
-        cpp_class_attrs = [entry for entry in scope.var_entries
-                           if entry.type.is_cpp_class]
-        needs_finalisation = scope.needs_finalisation()
-        needs_gc = scope.needs_gc()
+            return # never used
 
         slot_func_cname = scope.mangle_internal("tp_dealloc")
         code.putln("")
         code.putln(
             "static void %s(PyObject *o) {" % slot_func_cname)
-        if cpp_class_attrs:
-            self.generate_self_cast(scope, code)
 
-        if needs_finalisation:
-            # before Py3.4, tp_finalize() isn't available, so this is
-            # the earliest possible time where we can call it ourselves
-            code.putln("#if PY_VERSION_HEX < 0x030400a1")
-            if needs_gc:
-                # We must mark ths object as (gc) untracked while tearing
-                # it down, lest the garbage collection is invoked while
-                # running this destructor.
-                code.putln("PyObject_GC_UnTrack(o);")
-            slot_func_cname = scope.mangle_internal("tp_finalize")
-            code.putln("%s(o);" % slot_func_cname)
-            if needs_gc and base_type:
-                # The base class deallocator probably expects this to be
-                # tracked, so undo the untracking above.
-                code.putln("PyObject_GC_Track(o);")
-            code.putln("#endif")
+        weakref_slot = scope.lookup_here("__weakref__")
+        _, (py_attrs, _, memoryview_slices) = scope.get_refcounted_entries()
+        cpp_class_attrs = [entry for entry in scope.var_entries if entry.type.is_cpp_class]
 
-        if needs_gc and not base_type:
+        if (py_attrs
+            or cpp_class_attrs
+            or memoryview_slices
+            or weakref_slot in scope.var_entries):
+            self.generate_self_cast(scope, code)
+        
+        # We must mark ths object as (gc) untracked while tearing it down, lest
+        # the garbage collection is invoked while running this destructor.
+        if scope.needs_gc():
             code.putln("PyObject_GC_UnTrack(o);")
 
+        # call the user's __dealloc__
+        self.generate_usr_dealloc_call(scope, code)
+        if weakref_slot in scope.var_entries:
+            code.putln("if (p->__weakref__) PyObject_ClearWeakRefs(o);")
+
         for entry in cpp_class_attrs:
             split_cname = entry.type.cname.split('::')
             destructor_name = split_cname.pop()
@@ -1214,10 +1206,23 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
             while destructor_name.count('<') != destructor_name.count('>'):
                 destructor_name = split_cname.pop() + '::' + destructor_name
             destructor_name = destructor_name.split('<',1)[0]
-            code.putln("p->%s.%s::~%s();" % (
-                entry.cname, entry.type.declaration_code(""), destructor_name))
+            code.putln("p->%s.%s::~%s();" %
+                (entry.cname, entry.type.declaration_code(""), destructor_name))
+
+        for entry in py_attrs:
+            code.put_xdecref_clear("p->%s" % entry.cname, entry.type, nanny=False,
+                                   clear_before_decref=True)
+
+        for entry in memoryview_slices:
+            code.put_xdecref_memoryviewslice("p->%s" % entry.cname,
+                                             have_gil=True)
 
         if base_type:
+            # The base class deallocator probably expects this to be tracked, so
+            # undo the untracking above.
+            if scope.needs_gc():
+                code.putln("PyObject_GC_Track(o);")
+
             tp_dealloc = TypeSlots.get_base_slot_function(scope, tp_slot)
             if tp_dealloc is not None:
                 code.putln("%s(o);" % tp_dealloc)
@@ -1229,8 +1234,7 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
                 # the module cleanup, which may already have cleared it.
                 # In that case, fall back to traversing the type hierarchy.
                 base_cname = base_type.typeptr_cname
-                code.putln("if (likely(%s)) %s->tp_dealloc(o); "
-                           "else __Pyx_call_next_tp_dealloc(o, %s);" % (
+                code.putln("if (likely(%s)) %s->tp_dealloc(o); else __Pyx_call_next_tp_dealloc(o, %s);" % (
                     base_cname, base_cname, slot_func_cname))
                 code.globalstate.use_utility_code(
                     UtilityCode.load_cached("CallNextTpDealloc", "ExtensionTypes.c"))
@@ -1252,81 +1256,28 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
         code.putln(
             "}")
 
-    def generate_finalize_function(self, scope, code):
-        if not scope.needs_finalisation():
-            return
-
-        weakref_slot = scope.lookup_here("__weakref__")
-        if weakref_slot not in scope.var_entries:
-            weakref_slot = None
-        _, (py_attrs, _, memoryview_slices) = scope.get_refcounted_entries()
-
-        slot_func_cname = scope.mangle_internal("tp_finalize")
-        code.putln("")
-        code.putln("static void %s(PyObject *o) {" % slot_func_cname)
-
-        if py_attrs or memoryview_slices or weakref_slot:
-            self.generate_self_cast(scope, code)
-
+    def generate_usr_dealloc_call(self, scope, code):
         entry = scope.lookup_here("__dealloc__")
         if entry:
-            code.putln("PyObject *etype, *eval, *etb;")
-            code.putln("PyErr_Fetch(&etype, &eval, &etb);")
-
-            code.putln("#if PY_VERSION_HEX < 0x030400a1")
-            code.putln("++Py_REFCNT(o);")
-            code.putln("#endif")
-
-            code.putln("%s(o);" % entry.func_cname)
-            code.putln("if (PyErr_Occurred()) PyErr_WriteUnraisable(o);")
-
-            code.putln("#if PY_VERSION_HEX < 0x030400a1")
-            code.putln("--Py_REFCNT(o);")
-            code.putln("#endif")
-
-            code.putln("PyErr_Restore(etype, eval, etb);")
-
-        if weakref_slot:
-            # Not using a preprocessor test here to avoid warning about
-            # "unused variable p".  Py3.4+ already cleared the weak refs
-            # when we get here.
-            # FIXME: so did Py<3.4 I think? Isn't this redundant?
-            code.putln("if ((PY_VERSION_HEX < 0x030400a1) && p->__weakref__) "
-                       "PyObject_ClearWeakRefs(o);")
-
-        for entry in py_attrs:
-            code.put_xdecref_clear("p->%s" % entry.cname, entry.type, nanny=False,
-                                   clear_before_decref=True)
-
-        for entry in memoryview_slices:
-            code.put_xdecref_memoryviewslice("p->%s" % entry.cname,
-                                             have_gil=True)
-
-        base_type = scope.parent_type.base_type
-        if base_type:
-            code.putln("#if PY_VERSION_HEX >= 0x030400a1")
-            # need to call base_type method
-            tp_finalize = TypeSlots.get_base_slot_function(
-                scope, TypeSlots.FinaliserSlot())
-            if tp_finalize is not None:
-                code.putln("%s(o);" % tp_finalize)
-            elif base_type.is_builtin_type:
-                code.putln("if (%s->tp_finalize) %s->tp_finalize(o);" % (
-                    base_type.typeptr_cname, base_type.typeptr_cname))
-            else:
-                # This is an externally defined type.  Calling through the
-                # cimported base type pointer directly interacts badly with
-                # the module cleanup, which may already have cleared it.
-                # In that case, fall back to traversing the type hierarchy.
-                base_cname = base_type.typeptr_cname
-                code.putln("if (likely(%s && %s->tp_finalize)) %s->tp_finalize(o); "
-                           "else __Pyx_call_next_tp_finalize(o, %s);" % (
-                    base_cname, base_cname, base_cname, slot_func_cname))
-                code.globalstate.use_utility_code(
-                    UtilityCode.load_cached("CallNextTpFinalize", "ExtensionTypes.c"))
-            code.putln("#endif")
-
-        code.putln("}")
+            code.putln(
+                "{")
+            code.putln(
+                    "PyObject *etype, *eval, *etb;")
+            code.putln(
+                    "PyErr_Fetch(&etype, &eval, &etb);")
+            code.putln(
+                    "++Py_REFCNT(o);")
+            code.putln(
+                    "%s(o);" %
+                        entry.func_cname)
+            code.putln(
+                    "if (PyErr_Occurred()) PyErr_WriteUnraisable(o);")
+            code.putln(
+                    "--Py_REFCNT(o);")
+            code.putln(
+                    "PyErr_Restore(etype, eval, etb);")
+            code.putln(
+                "}")
 
     def generate_traverse_function(self, scope, code, cclass_entry):
         tp_slot = TypeSlots.GCDependentSlot("tp_traverse")

Cython/Compiler/Symtab.py

@@ -832,6 +832,25 @@ class Scope(object):
     def add_include_file(self, filename):
         self.outer_scope.add_include_file(filename)
 
+    def get_refcounted_entries(self, include_weakref=False,
+                               include_gc_simple=True):
+        py_attrs = []
+        py_buffers = []
+        memoryview_slices = []
+
+        for entry in self.var_entries:
+            if entry.type.is_pyobject:
+                if include_weakref or entry.name != "__weakref__":
+                    if include_gc_simple or not entry.type.is_gc_simple:
+                        py_attrs.append(entry)
+            elif entry.type == PyrexTypes.c_py_buffer_type:
+                py_buffers.append(entry)
+            elif entry.type.is_memoryviewslice:
+                memoryview_slices.append(entry)
+
+        have_entries = py_attrs or py_buffers or memoryview_slices
+        return have_entries, (py_attrs, py_buffers, memoryview_slices)
+
 
 class PreImportScope(Scope):
 
@@ -1785,33 +1804,6 @@ class CClassScope(ClassScope):
             return not self.parent_type.is_gc_simple
         return False
 
-    def get_refcounted_entries(self, include_weakref=False,
-                               include_gc_simple=True):
-        py_attrs = []
-        py_buffers = []
-        memoryview_slices = []
-
-        for entry in self.var_entries:
-            if entry.type.is_pyobject:
-                if include_weakref or entry.name != "__weakref__":
-                    if include_gc_simple or not entry.type.is_gc_simple:
-                        py_attrs.append(entry)
-            elif entry.type == PyrexTypes.c_py_buffer_type:
-                py_buffers.append(entry)
-            elif entry.type.is_memoryviewslice:
-                memoryview_slices.append(entry)
-
-        have_entries = py_attrs or py_buffers or memoryview_slices
-        return have_entries, (py_attrs, py_buffers, memoryview_slices)
-
-    def needs_finalisation(self):
-        if self.lookup_here("__dealloc__"):
-            return True
-        has_gc_entries, _ = self.get_refcounted_entries(include_weakref=True)
-        if has_gc_entries:
-            return True
-        return False
-
     def declare_var(self, name, type, pos,
                     cname = None, visibility = 'private',
                     api = 0, in_pxd = 0, is_cdef = 0):
@@ -1873,6 +1865,7 @@ class CClassScope(ClassScope):
             self.namespace_cname = "(PyObject *)%s" % self.parent_type.typeptr_cname
             return entry
 
+
     def declare_pyfunction(self, name, pos, allow_redefine=False):
         # Add an entry for a method.
         if name in ('__eq__', '__ne__', '__lt__', '__gt__', '__le__', '__ge__'):
@@ -2040,7 +2033,6 @@ class CClassScope(ClassScope):
             if base_entry.utility_code:
                 entry.utility_code = base_entry.utility_code
 
-
 class CppClassScope(Scope):
     #  Namespace of a C++ class.
 

Cython/Compiler/TypeSlots.py

@@ -331,21 +331,6 @@ class GCDependentSlot(InternalMethodSlot):
         return InternalMethodSlot.slot_code(self, scope)
 
 
-class FinaliserSlot(InternalMethodSlot):
-    """
-    Descriptor for tp_finalize().
-    """
-    def __init__(self):
-        InternalMethodSlot.__init__(
-            self, 'tp_finalize',
-            ifdef="PY_VERSION_HEX >= 0x030400a1")
-
-    def slot_code(self, scope):
-        if not scope.needs_finalisation():
-            return '0'
-        return InternalMethodSlot.slot_code(self, scope)
-
-
 class ConstructorSlot(InternalMethodSlot):
     #  Descriptor for tp_new and tp_dealloc.
 
@@ -405,8 +390,6 @@ class TypeFlagsSlot(SlotDescriptor):
             value += "|Py_TPFLAGS_BASETYPE"
         if scope.needs_gc():
             value += "|Py_TPFLAGS_HAVE_GC"
-        if scope.needs_finalisation():
-            value += "|Py_TPFLAGS_HAVE_FINALIZE"
         return value
 
 
@@ -805,7 +788,8 @@ slot_table = (
     EmptySlot("tp_weaklist"),
     EmptySlot("tp_del"),
     EmptySlot("tp_version_tag", ifdef="PY_VERSION_HEX >= 0x02060000"),
-    FinaliserSlot(),  # 'tp_finalize'
+    # TODO: change __dealloc__ to be called by tp_finalize (PEP 442)
+    EmptySlot("tp_finalize", ifdef="PY_VERSION_HEX >= 0x03040a00"),
 )
 
 #------------------------------------------------------------------------------------------

Cython/Utility/CythonFunction.c

@@ -634,7 +634,7 @@ static PyTypeObject __pyx_CyFunctionType_type = {
 #if PY_VERSION_HEX >= 0x02060000
     0,                                  /*tp_version_tag*/
 #endif
-#if PY_VERSION_HEX >= 0x030400a1
+#if PY_VERSION_HEX >= 0x03040a00
     0,                                  /*tp_finalize*/
 #endif
 };
@@ -1082,7 +1082,7 @@ static PyTypeObject __pyx_FusedFunctionType_type = {
 #if PY_VERSION_HEX >= 0x02060000
     0,                                  /*tp_version_tag*/
 #endif
-#if PY_VERSION_HEX >= 0x030400a1
+#if PY_VERSION_HEX >= 0x03040a00
     0,                                  /*tp_finalize*/
 #endif
 };

Cython/Utility/ExtensionTypes.c

@@ -16,27 +16,6 @@ static void __Pyx_call_next_tp_dealloc(PyObject* obj, destructor current_tp_deal
         type->tp_dealloc(obj);
 }
 
-/////////////// CallNextTpFinalize.proto ///////////////
-
-#if PY_VERSION_HEX >= 0x030400a1
-static void __Pyx_call_next_tp_finalize(PyObject* obj, destructor current_tp_finalize);
-#endif
-
-/////////////// CallNextTpFinalize ///////////////
-
-#if PY_VERSION_HEX >= 0x030400a1
-static void __Pyx_call_next_tp_finalize(PyObject* obj, destructor current_tp_finalize) {
-    PyTypeObject* type = Py_TYPE(obj);
-    /* try to find the first parent type that has a different tp_finalize() function */
-    while (type && type->tp_finalize != current_tp_finalize)
-        type = type->tp_base;
-    while (type && (!type->tp_finalize || type->tp_finalize == current_tp_finalize))
-        type = type->tp_base;
-    if (type)
-        type->tp_finalize(obj);
-}
-#endif
-
 /////////////// CallNextTpTraverse.proto ///////////////
 
 static int __Pyx_call_next_tp_traverse(PyObject* obj, visitproc v, void *a, traverseproc current_tp_traverse);

Cython/Utility/Generator.c

@@ -460,20 +460,16 @@ static void __Pyx_Generator_dealloc(PyObject *self) {
     PyObject_GC_UnTrack(gen);
     if (gen->gi_weakreflist != NULL)
         PyObject_ClearWeakRefs(self);
+    PyObject_GC_Track(self);
 
     if (gen->resume_label > 0) {
         /* Generator is paused, so we need to close */
-        PyObject_GC_Track(self);
-#if PY_VERSION_HEX >= 0x030400a1
-        if (PyObject_CallFinalizerFromDealloc(self))
-#else
         Py_TYPE(gen)->tp_del(self);
         if (self->ob_refcnt > 0)
-#endif
             return;                     /* resurrected.  :( */
-        PyObject_GC_UnTrack(self);
     }
 
+    PyObject_GC_UnTrack(self);
     __Pyx_Generator_clear(self);
     PyObject_GC_Del(gen);
 }
@@ -486,11 +482,9 @@ static void __Pyx_Generator_del(PyObject *self) {
     if (gen->resume_label <= 0)
         return ;
 
-#if PY_VERSION_HEX < 0x030400a1
     /* Temporarily resurrect the object. */
     assert(self->ob_refcnt == 0);
     self->ob_refcnt = 1;
-#endif
 
     /* Save the current exception, if any. */
     __Pyx_ErrFetch(&error_type, &error_value, &error_traceback);
@@ -505,7 +499,6 @@ static void __Pyx_Generator_del(PyObject *self) {
     /* Restore the saved exception. */
     __Pyx_ErrRestore(error_type, error_value, error_traceback);
 
-#if PY_VERSION_HEX < 0x030400a1
     /* Undo the temporary resurrection; can't use DECREF here, it would
      * cause a recursive call.
      */
@@ -539,7 +532,6 @@ static void __Pyx_Generator_del(PyObject *self) {
     --Py_TYPE(self)->tp_frees;
     --Py_TYPE(self)->tp_allocs;
 #endif
-#endif
 }
 
 static PyMemberDef __pyx_Generator_memberlist[] = {
@@ -586,7 +578,7 @@ static PyTypeObject __pyx_GeneratorType_type = {
     0,                                  /*tp_getattro*/
     0,                                  /*tp_setattro*/
     0,                                  /*tp_as_buffer*/
-    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC | Py_TPFLAGS_HAVE_FINALIZE, /* tp_flags*/
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC, /* tp_flags*/
     0,                                  /*tp_doc*/
     (traverseproc) __Pyx_Generator_traverse,   /*tp_traverse*/
     0,                                  /*tp_clear*/
@@ -612,16 +604,12 @@ static PyTypeObject __pyx_GeneratorType_type = {
     0,                                  /*tp_cache*/
     0,                                  /*tp_subclasses*/
     0,                                  /*tp_weaklist*/
-#if PY_VERSION_HEX >= 0x030400a1
-    0,
-#else
     __Pyx_Generator_del,                /*tp_del*/
-#endif
 #if PY_VERSION_HEX >= 0x02060000
     0,                                  /*tp_version_tag*/
 #endif
-#if PY_VERSION_HEX >= 0x030400a1
-    __Pyx_Generator_del,                /*tp_finalize*/
+#if PY_VERSION_HEX >= 0x03040a00
+    0,                                  /*tp_finalize*/
 #endif
 };
 

Cython/Utility/ModuleSetupCode.c

@@ -128,10 +128,6 @@
   #define Py_TPFLAGS_HAVE_VERSION_TAG 0
 #endif
 
-#if PY_VERSION_HEX < 0x030400a1 && !defined(Py_TPFLAGS_HAVE_FINALIZE)
-  #define Py_TPFLAGS_HAVE_FINALIZE 0
-#endif
-
 /* new Py3.3 unicode type (PEP 393) */
 #if PY_VERSION_HEX > 0x03030000 && defined(PyUnicode_KIND)
   #define CYTHON_PEP393_ENABLED 1