Repository: python/cpython
Pull request: GH-93521: Filter out `__weakref__` slot if present in bases
This PR fixes #93521 by moving the `('__weakref__',) if weakref_slot else ()` ternary *into* the iterator that filters out already present slots.

Problem statement:
Cannot specify `weakref_slot=True` twice in dataclass MRO
## Summary

If a child dataclass of a parent dataclass who specified `weakref_slot`, also decides to set `weakref_slot` to True, Python will raise a TypeError as seen below:
```
TypeError: __weakref__ slot disallowed: either we already got one, or __itemsize__ != 0
```

## Description

I am porting dataclasses' slots and weakref slots kwargs into a decorator I can use. While doing so, I decided to change the default of the `weakref_slot` because I would like all of my classes to be weakref:able. This is when I ran into the issue which can be reproduced by the code below:
```python
from dataclasses import dataclass


@dataclass(slots=True, weakref_slot=True)
class A:
    field: str


@dataclass(slots=True, weakref_slot=True)
class B(A):
    ...
```
```
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "C:\Users\%username%\Projects\testing\copyclasses.py", line 1207, in wrap
    return _process_class(cls, init, repr, eq, order, unsafe_hash,
  File "C:\Users\%username%\Projects\testing\copyclasses.py", line 1108, in _process_class
    cls = _add_slots(cls, frozen, weakref_slot)
  File "C:\Users\%username%\Projects\testing\copyclasses.py", line 1176, in _add_slots    
    cls = type(cls)(cls.__name__, cls.__bases__, cls_dict)
TypeError: __weakref__ slot disallowed: either we already got one, or __itemsize__ != 0
```

### Environment

This was tested by creating a file named `copyclasses.py` with the current contents of `Lib/dataclasses.py` ran using a 3.10 CPython interpreter. Note that I've restored the naming of the module in the codeblock above assuming those who will be running it will have a built 3.11 CPython interpreter.

---

Footnotes: I will mention @ericvsmith as the code owner of the dataclasses file.

Hints from issue discussion:
Here's an example of the same thing without dataclasses:
```python
class A:
    __slots__='__weakref__'

class B(A):
    __slots__='__weakref__'
```
```
Traceback (most recent call last):
  File "gh-93521.py", line 4, in <module>
    class B(A):
TypeError: __weakref__ slot disallowed: either we already got one, or __itemsize__ != 0
```
I'm not sure this is a bug, just the way things work.
It would be nice if dataclasses took care of this issue automatically though.

Example scenario: I have a library in which I expose a dataclass. A user subclasses my class in another dataclass, and wants to use weakrefs, so they use `weakref_slots`. Then I change my library to allow weakrefs on the base class, but now the user's child class is broken.
Locally I fixed this already, and I am planning on opening a PR with an added test. Should that PR point to `main` or the `3.11` branch (seeing as both branches should get the patch)?
main

Commit message: 📜🤖 Added by blurb_it.

Patch to review:
diff --git a/Lib/dataclasses.py b/Lib/dataclasses.py
index 4645ebfa71e71c..18ab69053fd520 100644
--- a/Lib/dataclasses.py
+++ b/Lib/dataclasses.py
@@ -1156,11 +1156,16 @@ def _add_slots(cls, is_frozen, weakref_slot):
         itertools.chain.from_iterable(map(_get_slots, cls.__mro__[1:-1]))
     )
     # The slots for our class.  Remove slots from our base classes.  Add
-    # '__weakref__' if weakref_slot was given.
+    # '__weakref__' if weakref_slot was given, unless it is already present.
     cls_dict["__slots__"] = tuple(
-        itertools.chain(
-            itertools.filterfalse(inherited_slots.__contains__, field_names),
-            ("__weakref__",) if weakref_slot else ())
+        itertools.filterfalse(
+            inherited_slots.__contains__,
+            itertools.chain(
+                # gh-93521: '__weakref__' also needs to be filtered out if
+                # already present in inherited_slots
+                field_names, ('__weakref__',) if weakref_slot else ()
+            )
+        ),
     )
 
     for field_name in field_names:
diff --git a/Lib/test/test_dataclasses.py b/Lib/test/test_dataclasses.py
index cf29cd07516f0d..1f607478324330 100644
--- a/Lib/test/test_dataclasses.py
+++ b/Lib/test/test_dataclasses.py
@@ -3109,6 +3109,22 @@ def test_weakref_slot_make_dataclass(self):
                                     "weakref_slot is True but slots is False"):
             B = make_dataclass('B', [('a', int),], weakref_slot=True)
 
+    def test_weakref_slot_subclass_also_weakref_slot(self):
+        @dataclass(slots=True, weakref_slot=True)
+        class Base:
+            field: int
+
+        # A *can* also specify weakref_slot=True if it wants to (gh-93521)
+        @dataclass(slots=True, weakref_slot=True)
+        class A(Base):
+            ...
+
+        # __weakref__ is in the base class, not A.  But an instance of A
+        # is still weakref-able.
+        self.assertIn("__weakref__", Base.__slots__)
+        self.assertNotIn("__weakref__", A.__slots__)
+        a = A(1)
+        weakref.ref(a)
 
 class TestDescriptors(unittest.TestCase):
     def test_set_name(self):
diff --git a/Misc/NEWS.d/next/Library/2022-06-06-13-19-43.gh-issue-93521._vE8m9.rst b/Misc/NEWS.d/next/Library/2022-06-06-13-19-43.gh-issue-93521._vE8m9.rst
new file mode 100644
index 00000000000000..0e19b428f0f0b5
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2022-06-06-13-19-43.gh-issue-93521._vE8m9.rst
@@ -0,0 +1 @@
+Allow specifying ``weakref_slot`` several times in dataclass MRO


Code review comments:
diff_hunk: @@ -0,0 +1 @@
+Allow specifying ``weakref_slot`` several times in dataclass MRO
path: Misc/NEWS.d/next/Library/2022-06-06-13-19-43.gh-issue-93521._vE8m9.rst
text: @user1:
"MRO" is jargony, how about "base classes" instead? Also, this isn't just about `weakref_slot`, it's about `__weakref__` in any base class's `__slots__`, whether or not that class is a dataclass. I'm not sure the best way to word all of this, maybe drop any reference to dataclass here and just mention `__weakref__` in base classes. Or maybe start with the generic base class statement, and add another sentence saying this includes base dataclasses that have `weakref_slot=True`.

And by convention, please end the blurb with a period. They should contain one or more sentences. (I know this is a trivial point.)

@author:
Thank you. I wrote it more like a commit message, than a point on a page of patch notes for a game.