[mypyc] Calling tp_finalize from tp_dealloc.#18519
Conversation
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the PR! Finalization is a tricky topic so I want to study how it works in more detail before merging this. However, I left a few initial suggestions.
mypyc/codegen/emitclass.py
Outdated
| emitter.emit_line("static void") | ||
| emitter.emit_line(f"{dealloc_func_name}({cl.struct_name(emitter.names)} *self)") | ||
| emitter.emit_line("{") | ||
| emitter.emit_line("if (Py_TYPE(self)->tp_finalize) {") |
There was a problem hiding this comment.
Can we avoid the runtime check if the caller would pass a flag indicating whether there is a __del__ method defined for the class?
| emitter.emit_line(f"{finalize_func_name}(PyObject *self)") | ||
| emitter.emit_line("{") | ||
| emitter.emit_line( | ||
| "{}{}{}(self);".format( |
There was a problem hiding this comment.
Should we preserve the current exception status, as suggested in the docs: https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_finalize
* Calling PyObject_GC_IsFinalized(...) to ensure tp_finalize is called exactly once.
for more information, see https://pre-commit.ci
I did further testing including reference-loops. To avoid multiple calls to |
|
I am on vacation for the next 6 days. Will come back and fix tests. |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the updates! Looks good now, just one remaining minor thing -- this is almost ready to merge.
mypyc/codegen/emitclass.py
Outdated
| ) | ||
| emitter.emit_line("if (PyErr_Occurred() != NULL) {") | ||
| emitter.emit_line('PyObject *del_str = PyUnicode_FromString("__del__");') | ||
| emitter.emit_line("PyObject *del_method = _PyType_Lookup(Py_TYPE(self), del_str);") |
There was a problem hiding this comment.
Check if del_str == NULL? If it's NULL, we can assign NULL to del_method, since PyErr_WriteUnraisable accepts NULL args.
Fixes mypyc/mypyc#1035 * Populating `.tp_finalize` if the user has defined `__del__`. * Calling `.tp_finalize` from `.tp_dealloc`.
2 participants
Fixes mypyc/mypyc#1035
.tp_finalizeif the user has defined__del__..tp_finalizefrom.tp_dealloc.