-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
gh-135228: When @dataclass(slots=True) replaces a dataclass, make the original class collectible (take 2) #137047
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
Conversation
…ke the original class collectible (python#136893) An interesting hack, but more localized in scope than python#135230. This may be a breaking change if people intentionally keep the original class around when using `@dataclass(slots=True)`, and then use `__dict__` or `__weakref__` on the original class. Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
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 check that this is a Python class? Should we clear the whole class dict or set it to NULL for the case if there are more descriptors?
Maybe we can check that it's a heap type?
I'm not aware of other descriptors that could cause problems. It's better to minimize the amount of changes we make to the class, because there are edge cases where the class is still accessible after the dataclass transformation is applied. |
Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@encukou @serhiy-storchaka could you review again and approve if you're satisfied, so that we can get this fixed in 3.14? |
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.
I can't find any issues in the PR; this is pretty much is the local optimum.
But, I can't tell if there will be unintended consequences. This PR feels quite risky for a RC fix.
Docs-wise: the notes/comments shouldn't imply that the original class is guaranteed to be garbage collected -- something else could still keep it alive.
Misc/NEWS.d/next/Library/2025-07-20-16-56-55.gh-issue-135228.n_XIao.rst
Outdated
Show resolved
Hide resolved
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.
I also agree that #136966 is a better long-term solution; we can merge it into 3.15 after this goes in, and we can also consider using it in 3.14.1. |
Let's go then. |
Thanks @JelleZijlstra for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
…ke the original class collectible (take 2) (pythonGH-137047) Remove the `__dict__` and `__weakref__` descriptors from the original class when creating a dataclass from it. An interesting hack, but more localized in scope than pythongh-135230. This may be a breaking change if people intentionally keep the original class around when using `@dataclass(slots=True)`, and then use `__dict__` or `__weakref__` on the original class. (cherry picked from commit 6859b95) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com> Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com> Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
GH-137666 is a backport of this pull request to the 3.14 branch. |
|
* main: pythongh-137288: Update 3.14 magic numbers (pythonGH-137665) pythongh-135228: When @DataClass(slots=True) replaces a dataclass, make the original class collectible (take 2) (pythonGH-137047) pythongh-126008: Improve docstrings for Tkinter cget and configure methods (pythonGH-133303) pythongh-131885: Use positional-only markers for ``max()`` and ``min()`` (python#131868) pythonGH-137426: Remove code deprecation of `importlib.abc.ResourceLoader` (pythonGH-137567) pythongh-125897: Mark range function parameters as positional only (python#125945) pythongh-137400: Fix a crash when disabling profiling across all threads (pythongh-137471) pythongh-115766: Fix IPv4Interface.is_unspecified (pythonGH-137326) pythongh-128813: cleanup C-API docs for PyComplexObject (pythonGH-137579) pythongh-135953: Profile a module or script with sampling profiler (python#136777) Fix documentation of hash in PyHash_FuncDef (python#137595)
…-136966) This partially reverts #137047, keeping the tests for GC collectability of the original class that dataclass adds `__slots__` to. The reference leaks solved there are instead solved by having the `__dict__` & `__weakref__` descriptors not tied to (and referencing) their class. Instead, they're shared between all classes that need them (within an interpreter). The `__objclass__` ol the descriptors is set to `object`, since these descriptors work with *any* object. (The appropriate checks were already made in the get/set code, so the `__objclass__` check was redundant.) The repr of these descriptors (and any others whose `__objclass__` is `object`) now doesn't mention the objclass. This change required adjustment of introspection code that checks `__objclass__` to determine an object's “own” (i.e. not inherited) `__dict__`. Third-party code that does similar introspection of the internals will also need adjusting. Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
…ke the original class collectible (take 2) (pythonGH-137047) Remove the `__dict__` and `__weakref__` descriptors from the original class when creating a dataclass from it. An interesting hack, but more localized in scope than pythongh-135230. This may be a breaking change if people intentionally keep the original class around when using `@dataclass(slots=True)`, and then use `__dict__` or `__weakref__` on the original class. Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com> Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…ct (pythonGH-136966) This partially reverts python#137047, keeping the tests for GC collectability of the original class that dataclass adds `__slots__` to. The reference leaks solved there are instead solved by having the `__dict__` & `__weakref__` descriptors not tied to (and referencing) their class. Instead, they're shared between all classes that need them (within an interpreter). The `__objclass__` ol the descriptors is set to `object`, since these descriptors work with *any* object. (The appropriate checks were already made in the get/set code, so the `__objclass__` check was redundant.) The repr of these descriptors (and any others whose `__objclass__` is `object`) now doesn't mention the objclass. This change required adjustment of introspection code that checks `__objclass__` to determine an object's “own” (i.e. not inherited) `__dict__`. Third-party code that does similar introspection of the internals will also need adjusting. Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
This is a redo of #136893 without relying on a hack to get to the type dictionary.