-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mypyc] Calling tp_finalize from tp_dealloc. #18519
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -779,6 +787,9 @@ def generate_dealloc_for_class( | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid the runtime check if the caller would pass a flag indicating whether there is a __del__
method defined for the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
emitter.emit_line(f"{finalize_func_name}(PyObject *self)") | ||
emitter.emit_line("{") | ||
emitter.emit_line( | ||
"{}{}{}(self);".format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if del_str == NULL
? If it's NULL, we can assign NULL to del_method
, since PyErr_WriteUnraisable
accepts NULL args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Fixes mypyc/mypyc#1035 * Populating `.tp_finalize` if the user has defined `__del__`. * Calling `.tp_finalize` from `.tp_dealloc`.
Fixes mypyc/mypyc#1035
.tp_finalize
if the user has defined__del__
..tp_finalize
from.tp_dealloc
.