KEMBAR78
[mypyc] Calling tp_finalize from tp_dealloc. by advait-dixit · Pull Request #18519 · python/mypy · GitHub
Skip to content

Conversation

advait-dixit
Copy link
Contributor

Fixes mypyc/mypyc#1035

  • Populating .tp_finalize if the user has defined __del__.
  • Calling .tp_finalize from .tp_dealloc.

Copy link
Collaborator

@JukkaL JukkaL left a 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.

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) {")
Copy link
Collaborator

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?

Copy link
Contributor Author

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(
Copy link
Collaborator

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

Copy link
Contributor Author

@advait-dixit advait-dixit Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

advait-dixit and others added 2 commits January 29, 2025 15:30
* Calling PyObject_GC_IsFinalized(...) to ensure tp_finalize is called exactly once.
@advait-dixit
Copy link
Contributor Author

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.

I did further testing including reference-loops. To avoid multiple calls to .tp_finalize, I had to add an if-statement to check return value of PyObject_GC_IsFinalized. Please see second commit in this PR.
I don't understand and cannot test many corner-cases for calling tp_finalize either. Things get even more complicated with inheritance and exceptions. But, I think this change handles most common cases now.

@advait-dixit
Copy link
Contributor Author

I am on vacation for the next 6 days. Will come back and fix tests.

@JukkaL JukkaL added the topic-mypyc mypyc bugs label Jan 31, 2025
@advait-dixit advait-dixit requested a review from JukkaL February 9, 2025 06:41
Copy link
Collaborator

@JukkaL JukkaL left a 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.

)
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);")
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@JukkaL JukkaL merged commit 8104d01 into python:master Feb 14, 2025
12 checks passed
@advait-dixit advait-dixit deleted the call_del branch February 14, 2025 17:56
x612skm pushed a commit to x612skm/mypy-dev that referenced this pull request Feb 24, 2025
Fixes mypyc/mypyc#1035

* Populating `.tp_finalize` if the user has defined `__del__`.
* Calling `.tp_finalize` from `.tp_dealloc`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic-mypyc mypyc bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mypyc ignores custom __del__ without warning

2 participants