Skip to content

Fix slotted reference cycles on 3.14 #1446

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jul 28, 2025
Merged

Fix slotted reference cycles on 3.14 #1446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ea6a62d
Fix slotted reference cycles on 3.14
hynek Jul 22, 2025
8e26783
Add news fragment
hynek Jul 22, 2025
c60dfa0
Bump version tag to CMA
hynek Jul 23, 2025
98af24a
Apply review comment
hynek Jul 23, 2025
456253a
EAFP
hynek Jul 23, 2025
ce806dc
Merge branch 'main' into fix-slots-314
hynek Jul 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.d/1446.change.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
On 3.14, the cycles in slotted classes are now manually broken.
An explicit call to `gc.collect()` is still necessary, unfortunately.
26 changes: 26 additions & 0 deletions src/attr/_make.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
PY_3_10_PLUS,
PY_3_11_PLUS,
PY_3_13_PLUS,
PY_3_14_PLUS,
_AnnotationExtractor,
_get_annotations,
get_generic_base,
Expand Down Expand Up @@ -618,6 +619,17 @@ def evolve(*args, **changes):
return cls(**changes)


# Hack to the get the underlying dict out of a mappingproxy
# Use it with: cls.__dict__ | _deproxier
# See: https://github.com/python/cpython/pull/136893
class _Deproxier:
def __ror__(self, other):
return other


_deproxier = _Deproxier()


class _ClassBuilder:
"""
Iteratively build *one* class.
Expand Down Expand Up @@ -845,6 +857,20 @@ def _create_slots_class(self):
if k not in (*tuple(self._attr_names), "__dict__", "__weakref__")
}

if PY_3_14_PLUS:
# Clean up old dict to avoid leaks.
old_cls_dict = self._cls.__dict__ | _deproxier
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend putting all this in a try-except in case we break this trick in a later version of CPython; if so, it feels better for your users to leak the original class than to get a mysterious error about mappingproxies.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JelleZijlstra given context from Discord, we’re kinda weighting “broken introspection” vs “things explode if someone adds another decorator at the wrong position” anyways?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this variant definitely has some questionable edge cases. I think you can be a bit less conservative in CPython in terms of judging whether this hack is worth it.

As I understand from Brandt, a problem with the hack is that it mutates the class dict without incrementing the internal version tag for the type object. A workaround could then be to perform some no-op that increments the version tag anyway. For example, setting cls.__abstractmethods__ to its existing value could work.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you can be a bit less conservative in CPython in terms of judging whether this hack is worth it.

I'm afraid we have to be more conservative, because in attrs slots are on by default. :|

But looking again at the code, which part is actually dangerous here? It's just the wekreaf part, no? We do not modify self._cls.__dict__ – we just use your escape hatch to break cycles.

E.g. I tried this:

def test_no_references_to_original_when_using_cached_property(self):
        """
        When subclassing a slotted class and using cached property, there are
        no stray references to the original class.
        """

        @attr.s(slots=True)
        class C:
            pass

        tmp = None

        def decorator(cls):
            nonlocal tmp
            tmp = cls
            return cls

        @attr.s(slots=True)
        @decorator
        class C2(C):
            @functools.cached_property
            def value(self) -> int:
                return 0

        print(tmp.__dict__)

        # The original C2 is in a reference cycle, so force a collect:
        gc.collect()

        print(tmp.__dict__)

        assert [C2] == C.__subclasses__()

and it obviously fails, because there's a legit reference to the original class within tmp. But both prints work and print what would be expected without segfaulting.

Doesn't this mean that the only downside/edge case (aside from it potentially breaking eventually) is that the original class can't be weakref'ed? I'm not super deep in this topic, so happy to be enlightened. That would def be a downside I'd be OK to accept.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the risky part is removing "__dict__" from the __dict__. The interpreter uses various caches and optimizations that are keyed on the "type version", a number that gets incremented when the type is modified. Because we use this trick to get to the type dict (which usually can't be modified directly from Python code), we're modifying the type dict without bumping the version number. That can lead to crashes if interpreter code assumes the dict is unchanged from before.

There is a separate risk that you break users of the original class who rely on the __dict__/__weakref__ slots. But that is Python-level breakage (things throw exceptions), not C-level breakage (the interpreter crashes).

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit more color on what exactly this breaks on the affected class (the original class that we want to be garbage collected):

  • Removing __weakref__ actually breaks very little that matters. Even after the slot is deleted, weakref.ref() still works on these objects. It's just the Python-visible obj.__weakref__ attribute that is gone, but as far as I can tell basically nothing relies on that.
  • Removing __dict__ means that obj.__dict__ no longer works, but direct attribute access (obj.x = 1) still works.

old_cls_dict.pop("__dict__", None)
if "__weakref__" in self._cls.__dict__:
del self._cls.__weakref__

# Manually bump internal version tag.
try:
self._cls.__abstractmethods__ = self._cls.__abstractmethods__
except AttributeError:
self._cls.__abstractmethods__ = frozenset({"__init__"})
del self._cls.__abstractmethods__

# If our class doesn't have its own implementation of __setattr__
# (either from the user or by us), check the bases, if one of them has
# an attrs-made __setattr__, that needs to be reset. We don't walk the
Expand Down
3 changes: 1 addition & 2 deletions tests/test_make.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import attr

from attr import _config
from attr._compat import PY_3_10_PLUS, PY_3_14_PLUS
from attr._compat import PY_3_10_PLUS
from attr._make import (
Attribute,
Factory,
Expand Down Expand Up @@ -1939,7 +1939,6 @@ class C2(C):

assert [C2] == C.__subclasses__()

@pytest.mark.xfail(PY_3_14_PLUS, reason="Currently broken on nightly.")
def test_no_references_to_original_when_using_cached_property(self):
"""
When subclassing a slotted class and using cached property, there are
Expand Down