Repository: python-attrs/attrs
Pull request #420 (2018-08-01 16:12:28)

T325 weakref with slots
#325

altendky: Hmm, I should probably add a test to confirm that no extra fields are added.  Kind of a given I think, but seeing as the workaround involved creating one and I don't think we want that, it seems worth a couple lines of test code I show and verify that desire.

I should also probably change my added tests to use hypothesis...  :|
hynek: Hmmm, IIUC, you’re only setting `__weakref__` if it's not set already, right? ISTM like we could set the option to True by default because I simply can’t come up with a realistic scenario that might break here?

It’s even questionable whether it should be an option at all, but if it is, it might even be renamed to `slots_weakref` or something since it has a very narrow use case.

Am I missing something @Tinche @njsmith?
njsmith: I'd be fine with switching on the `weakref=True` behavior unconditionally. I only really [suggested](https://github.com/python-attrs/attrs/issues/174#issuecomment-333335228) making it a switch because @Tinche [was against](https://github.com/python-attrs/attrs/issues/174#issuecomment-290531450), wasting 8 bytes of memory per instance when a slotted type is never actually weakref'ed, so I was hoping making it an option would sneak it past his objections :-).
altendky: @hynek, yes, the intent was to only add `"__weakref__"` if it wasn't already in `cd`...  a thing that was just created that explicitly avoids including `"__weakref__"`.  So that check is pointless.  But I am missing a check for `"__weakref__"` already being in the super class.  Adding now.  Presently `type()` raises an exception.

```python3
TypeError: __weakref__ slot disallowed: either we already got one, or __itemsize__ != 0
```

Same thing happens with an attribute named `__weakref__`.
altendky: If the option is kept and renamed, perhaps `weakref_slot`?  That seems to fit a bit better in my head.  It is a thing (like `init` or `hash` or `slots`) which we either create or don't.

Would we want an exception in case of `slots=False` and `weakref=True`?  Though this would make more sense if it were `weakref_slot`.  If not renamed, it could be argued that `slots=False, weakref=True` is actually valid since you can weakref without slots and that the invalid case is actually `slots=False, weakref=False` since we don't disable weakrefability.  So I think yes, rename to be slot-specific and raise an exception if set to True without slots.
altendky: Once we switch to `weakref_slot=True` as the name and default then an exception becomes less obvious to me.  It seems the case to raise an exception then would be a regular non-slotted class without specifying `weakref_slot=False` because, well, a slot was 'requested' (by default) and we aren't going to be making it.  Since we will be defaulting to `True`, `weakref` or `weakrefable` makes more sense to me and we raise an exception in any case where the requested state is not achieved (though that leads to exceptions in PyPy which wouldn't occur in CPython).  I dunno, I guess I'm not totally sold on the exception (that I suggested).

I'm coding towards `weakref_slot=True` but won't add the exception yet.
altendky: Changing the default broke some tests so I stashed that and went ahead and added parametrization to them for `weakref_slots` `True`/`False` to show the issue first.  But, it's bedtime so I'll have to try to fix that later.  Then back to making the default `True`.
njsmith: It does make sense to me that `weakref=True` would mean "this has to support weakrefs", and `weakref=False` would mean "I don't care if this supports weakrefs". I don't think anyone is writing code that *relies on* getting an exception when attempting to take a weakref, and it would be very annoying if you had to go through and remove all your `weakref=False` kwargs in order to port to PyPy.
altendky: I couldn't reason to a sensible exception with the name and default `weakref_slot=True`.  Presumably we wouldn't want the default to result in an exception in any case.  And for the value `False`, we wouldn't want the default of a not-slotted class to raise an exception, I mean it doesn't have a weakref slot as requested...  So I'm just not sure what to implement for an exception.

Anyways, I'm not waiting on anyone but myself to get back to this and figure out the failures.
altendky: Do we have outstanding actions on this at this point?  Should any of the tests I added use Hypothesis?  Or others that haven't been added but should be?  `weakref_slot` has already been added so that other things using the Hypothesis strategies should get variations against `weakref_slot`.
altendky: @hynek, sorry...  i thought i had done all that a few days ago.

Patch:
diff --git a/changelog.d/420.change.rst b/changelog.d/420.change.rst
new file mode 100644
index 0000000..9881366
--- /dev/null
+++ b/changelog.d/420.change.rst
@@ -0,0 +1,1 @@
+Add weakref_slot parameter to ``attr.s()`` allowing for weak referenceable slotted classes.
\ No newline at end of file
diff --git a/docs/api.rst b/docs/api.rst
index cfda359..b88778f 100644
--- a/docs/api.rst
+++ b/docs/api.rst
@@ -18,7 +18,7 @@ What follows is the API explanation, if you'd like a more hands-on introduction,
 Core
 ----
 
-.. autofunction:: attr.s(these=None, repr_ns=None, repr=True, cmp=True, hash=None, init=True, slots=False, frozen=False, str=False, auto_attribs=False, cache_hash=False)
+.. autofunction:: attr.s(these=None, repr_ns=None, repr=True, cmp=True, hash=None, init=True, slots=False, frozen=False, weakref_slot=True, str=False, auto_attribs=False, cache_hash=False)
 
    .. note::
 
diff --git a/docs/glossary.rst b/docs/glossary.rst
index 2fdf806..76c0684 100644
--- a/docs/glossary.rst
+++ b/docs/glossary.rst
@@ -43,21 +43,12 @@ Glossary
         Those methods are created for frozen slotted classes because they won't pickle otherwise.
         `Think twice <https://www.youtube.com/watch?v=7KnfGDajDQw>`_ before using :mod:`pickle` though.
 
-      - As always with slotted classes, you must specify a ``__weakref__`` slot if you wish for the class to be weak-referenceable.
-        Here's how it looks using ``attrs``:
+      - Slotted classes are weak-referenceable by default.
+        This can be disabled in CPython by passing ``weakref_slot=False`` to ``@attr.s`` [#pypyweakref]_.
 
-        .. doctest::
-
-          >>> import weakref
-          >>> @attr.s(slots=True)
-          ... class C(object):
-          ...     __weakref__ = attr.ib(init=False, hash=False, repr=False, cmp=False)
-          ...     x = attr.ib()
-          >>> c = C(1)
-          >>> weakref.ref(c)
-          <weakref at 0x...; to 'C' at 0x...>
       - Since it's currently impossible to make a class slotted after it's created, ``attrs`` has to replace your class with a new one.
         While it tries to do that as graciously as possible, certain metaclass features like ``__init_subclass__`` do not work with slotted classes.
 
 
 .. [#pypy] On PyPy, there is no memory advantage in using slotted classes.
+.. [#pypyweakref] On PyPy, slotted classes are naturally weak-referenceable so ``weakref_slot=False`` has no effect.
diff --git a/src/attr/__init__.pyi b/src/attr/__init__.pyi
index 0c2b346..3d3a375 100644
--- a/src/attr/__init__.pyi
+++ b/src/attr/__init__.pyi
@@ -164,6 +164,7 @@ def attrs(
     init: bool = ...,
     slots: bool = ...,
     frozen: bool = ...,
+    weakref_slot: bool = ...,
     str: bool = ...,
     auto_attribs: bool = ...,
     kw_only: bool = ...,
@@ -180,6 +181,7 @@ def attrs(
     init: bool = ...,
     slots: bool = ...,
     frozen: bool = ...,
+    weakref_slot: bool = ...,
     str: bool = ...,
     auto_attribs: bool = ...,
     kw_only: bool = ...,
@@ -207,6 +209,7 @@ def make_class(
     init: bool = ...,
     slots: bool = ...,
     frozen: bool = ...,
+    weakref_slot: bool = ...,
     str: bool = ...,
     auto_attribs: bool = ...,
     kw_only: bool = ...,
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 4a0095f..3eef8f9 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -453,6 +453,7 @@ class _ClassBuilder(object):
         "_attr_names",
         "_slots",
         "_frozen",
+        "_weakref_slot",
         "_cache_hash",
         "_has_post_init",
         "_delete_attribs",
@@ -460,7 +461,15 @@ class _ClassBuilder(object):
     )
 
     def __init__(
-        self, cls, these, slots, frozen, auto_attribs, kw_only, cache_hash
+        self,
+        cls,
+        these,
+        slots,
+        frozen,
+        weakref_slot,
+        auto_attribs,
+        kw_only,
+        cache_hash,
     ):
         attrs, super_attrs, super_map = _transform_attrs(
             cls, these, auto_attribs, kw_only
@@ -474,6 +483,7 @@ class _ClassBuilder(object):
         self._attr_names = tuple(a.name for a in attrs)
         self._slots = slots
         self._frozen = frozen or _has_frozen_superclass(cls)
+        self._weakref_slot = weakref_slot
         self._cache_hash = cache_hash
         self._has_post_init = bool(getattr(cls, "__attrs_post_init__", False))
         self._delete_attribs = not bool(these)
@@ -531,11 +541,26 @@ class _ClassBuilder(object):
             if k not in tuple(self._attr_names) + ("__dict__", "__weakref__")
         }
 
+        weakref_inherited = False
+
+        # Traverse the MRO to check for an existing __weakref__.
+        for super_cls in self._cls.__mro__[1:-1]:
+            if "__weakref__" in getattr(super_cls, "__dict__", ()):
+                weakref_inherited = True
+                break
+
+        names = self._attr_names
+        if (
+            self._weakref_slot
+            and "__weakref__" not in getattr(self._cls, "__slots__", ())
+            and "__weakref__" not in names
+            and not weakref_inherited
+        ):
+            names += ("__weakref__",)
+
         # We only add the names of attributes that aren't inherited.
         # Settings __slots__ to inherited attributes wastes memory.
-        slot_names = [
-            name for name in self._attr_names if name not in super_names
-        ]
+        slot_names = [name for name in names if name not in super_names]
         if self._cache_hash:
             slot_names.append(_hash_cache_field)
         cd["__slots__"] = tuple(slot_names)
@@ -678,6 +703,7 @@ def attrs(
     init=True,
     slots=False,
     frozen=False,
+    weakref_slot=True,
     str=False,
     auto_attribs=False,
     kw_only=False,
@@ -762,6 +788,8 @@ def attrs(
                ``object.__setattr__(self, "attribute_name", value)``.
 
         ..  _slots: https://docs.python.org/3/reference/datamodel.html#slots
+    :param bool weakref_slot: Make instances weak-referenceable.  This has no
+        effect unless ``slots`` is also enabled.
     :param bool auto_attribs: If True, collect `PEP 526`_-annotated attributes
         (Python 3.6 and later only) from the class body.
 
@@ -800,6 +828,7 @@ def attrs(
     .. versionchanged:: 18.1.0
        If *these* is passed, no attributes are deleted from the class body.
     .. versionchanged:: 18.1.0 If *these* is ordered, the order is retained.
+    .. versionadded:: 18.2.0 *weakref_slot*
     .. deprecated:: 18.2.0
        ``__lt__``, ``__le__``, ``__gt__``, and ``__ge__`` now raise a
        :class:`DeprecationWarning` if the classes compared are subclasses of
@@ -814,7 +843,14 @@ def attrs(
             raise TypeError("attrs only works with new-style classes.")
 
         builder = _ClassBuilder(
-            cls, these, slots, frozen, auto_attribs, kw_only, cache_hash
+            cls,
+            these,
+            slots,
+            frozen,
+            weakref_slot,
+            auto_attribs,
+            kw_only,
+            cache_hash,
         )
 
         if repr is True:


Test patch:
diff --git a/tests/strategies.py b/tests/strategies.py
index 79895d1..1d98945 100644
--- a/tests/strategies.py
+++ b/tests/strategies.py
@@ -147,13 +147,15 @@ list_of_attrs = st.lists(simple_attrs, max_size=9)
 
 
 @st.composite
-def simple_classes(draw, slots=None, frozen=None, private_attrs=None):
+def simple_classes(
+    draw, slots=None, frozen=None, weakref_slot=None, private_attrs=None
+):
     """
     A strategy that generates classes with default non-attr attributes.
 
     For example, this strategy might generate a class such as:
 
-    @attr.s(slots=True, frozen=True)
+    @attr.s(slots=True, frozen=True, weakref_slot=True)
     class HypClass:
         a = attr.ib(default=1)
         _b = attr.ib(default=None)
@@ -161,10 +163,10 @@ def simple_classes(draw, slots=None, frozen=None, private_attrs=None):
         _d = attr.ib(default=1.0)
         c = attr.ib(default={'t': 1})
 
-    By default, all combinations of slots and frozen classes will be generated.
-    If `slots=True` is passed in, only slots classes will be generated, and
-    if `slots=False` is passed in, no slot classes will be generated. The same
-    applies to `frozen`.
+    By default, all combinations of slots, frozen, and weakref_slot classes
+    will be generated. If `slots=True` is passed in, only slots classes will be
+    generated, and if `slots=False` is passed in, no slot classes will be
+    generated. The same applies to `frozen` and `weakref_slot`.
 
     By default, some attributes will be private (i.e. prefixed with an
     underscore). If `private_attrs=True` is passed in, all attributes will be
@@ -173,6 +175,9 @@ def simple_classes(draw, slots=None, frozen=None, private_attrs=None):
     attrs = draw(list_of_attrs)
     frozen_flag = draw(st.booleans()) if frozen is None else frozen
     slots_flag = draw(st.booleans()) if slots is None else slots
+    weakref_slot_flag = (
+        draw(st.booleans()) if weakref_slot is None else weakref_slot
+    )
 
     if private_attrs is None:
         attr_names = maybe_underscore_prefix(gen_attr_names())
@@ -191,7 +196,11 @@ def simple_classes(draw, slots=None, frozen=None, private_attrs=None):
         cls_dict["__attrs_post_init__"] = post_init
 
     return make_class(
-        "HypClass", cls_dict, slots=slots_flag, frozen=frozen_flag
+        "HypClass",
+        cls_dict,
+        slots=slots_flag,
+        frozen=frozen_flag,
+        weakref_slot=weakref_slot_flag,
     )
 
 
diff --git a/tests/test_dark_magic.py b/tests/test_dark_magic.py
index 4c93b3f..d3f8ea9 100644
--- a/tests/test_dark_magic.py
+++ b/tests/test_dark_magic.py
@@ -339,12 +339,13 @@ class TestDarkMagic(object):
 
     @pytest.mark.parametrize("slots", [True, False])
     @pytest.mark.parametrize("frozen", [True, False])
-    def test_attrib_overwrite(self, slots, frozen):
+    @pytest.mark.parametrize("weakref_slot", [True, False])
+    def test_attrib_overwrite(self, slots, frozen, weakref_slot):
         """
         Subclasses can overwrite attributes of their superclass.
         """
 
-        @attr.s(slots=slots, frozen=frozen)
+        @attr.s(slots=slots, frozen=frozen, weakref_slot=weakref_slot)
         class SubOverwrite(Super):
             x = attr.ib(default=attr.Factory(list))
 
@@ -447,6 +448,8 @@ class TestDarkMagic(object):
     @pytest.mark.parametrize("sub_slots", [True, False])
     @pytest.mark.parametrize("base_frozen", [True, False])
     @pytest.mark.parametrize("sub_frozen", [True, False])
+    @pytest.mark.parametrize("base_weakref_slot", [True, False])
+    @pytest.mark.parametrize("sub_weakref_slot", [True, False])
     @pytest.mark.parametrize("base_converter", [True, False])
     @pytest.mark.parametrize("sub_converter", [True, False])
     def test_frozen_slots_combo(
@@ -455,6 +458,8 @@ class TestDarkMagic(object):
         sub_slots,
         base_frozen,
         sub_frozen,
+        base_weakref_slot,
+        sub_weakref_slot,
         base_converter,
         sub_converter,
     ):
@@ -463,11 +468,17 @@ class TestDarkMagic(object):
         with a single attribute.
         """
 
-        @attr.s(frozen=base_frozen, slots=base_slots)
+        @attr.s(
+            frozen=base_frozen,
+            slots=base_slots,
+            weakref_slot=base_weakref_slot,
+        )
         class Base(object):
             a = attr.ib(converter=int if base_converter else None)
 
-        @attr.s(frozen=sub_frozen, slots=sub_slots)
+        @attr.s(
+            frozen=sub_frozen, slots=sub_slots, weakref_slot=sub_weakref_slot
+        )
         class Sub(Base):
             b = attr.ib(converter=int if sub_converter else None)
 
diff --git a/tests/test_make.py b/tests/test_make.py
index 5e7565b..fe6d233 100644
--- a/tests/test_make.py
+++ b/tests/test_make.py
@@ -1368,7 +1368,7 @@ class TestClassBuilder(object):
         class C(object):
             pass
 
-        b = _ClassBuilder(C, None, True, True, False, False, False)
+        b = _ClassBuilder(C, None, True, True, False, False, False, False)
 
         assert "<_ClassBuilder(cls=C)>" == repr(b)
 
@@ -1380,7 +1380,7 @@ class TestClassBuilder(object):
         class C(object):
             x = attr.ib()
 
-        b = _ClassBuilder(C, None, True, True, False, False, False)
+        b = _ClassBuilder(C, None, True, True, False, False, False, False)
 
         cls = (
             b.add_cmp()
@@ -1441,6 +1441,7 @@ class TestClassBuilder(object):
             these=None,
             slots=False,
             frozen=False,
+            weakref_slot=True,
             auto_attribs=False,
             kw_only=False,
             cache_hash=False,
diff --git a/tests/test_slots.py b/tests/test_slots.py
index 38af43e..f2caca0 100644
--- a/tests/test_slots.py
+++ b/tests/test_slots.py
@@ -2,6 +2,8 @@
 Unit tests for slot-related functionality.
 """
 
+import weakref
+
 import pytest
 
 import attr
@@ -83,7 +85,7 @@ def test_slots_being_used():
     assert "__dict__" in dir(non_slot_instance)
     assert "__slots__" not in dir(non_slot_instance)
 
-    assert set(["x", "y"]) == set(slot_instance.__slots__)
+    assert set(["__weakref__", "x", "y"]) == set(slot_instance.__slots__)
 
     if has_pympler:
         assert asizeof(slot_instance) < asizeof(non_slot_instance)
@@ -206,7 +208,7 @@ def test_nonslots_these():
     assert "clsmethod" == c2.classmethod()
     assert "staticmethod" == c2.staticmethod()
 
-    assert set(["x", "y", "z"]) == set(C2Slots.__slots__)
+    assert set(["__weakref__", "x", "y", "z"]) == set(C2Slots.__slots__)
 
     c3 = C2Slots(x=1, y=3, z="test")
     assert c3 > c2
@@ -430,3 +432,100 @@ class TestClosureCellRewriting(object):
         ) == w.message.args
 
         assert just_warn is func
+
+
+@pytest.mark.skipif(PYPY, reason="__slots__ only block weakref on CPython")
+def test_not_weakrefable():
+    """
+    Instance is not weak-referenceable when `weakref_slot=False` in CPython.
+    """
+
+    @attr.s(slots=True, weakref_slot=False)
+    class C(object):
+        pass
+
+    c = C()
+
+    with pytest.raises(TypeError):
+        weakref.ref(c)
+
+
+@pytest.mark.skipif(
+    not PYPY, reason="slots without weakref_slot should only work on PyPy"
+)
+def test_implicitly_weakrefable():
+    """
+    Instance is weak-referenceable even when `weakref_slot=False` in PyPy.
+    """
+
+    @attr.s(slots=True, weakref_slot=False)
+    class C(object):
+        pass
+
+    c = C()
+    w = weakref.ref(c)
+
+    assert c is w()
+
+
+def test_weakrefable():
+    """
+    Instance is weak-referenceable when `weakref_slot=True`.
+    """
+
+    @attr.s(slots=True, weakref_slot=True)
+    class C(object):
+        pass
+
+    c = C()
+    w = weakref.ref(c)
+
+    assert c is w()
+
+
+def test_weakref_does_not_add_a_field():
+    """
+    `weakref_slot=True` does not add a field to the class.
+    """
+
+    @attr.s(slots=True, weakref_slot=True)
+    class C(object):
+        field = attr.ib()
+
+    assert [f.name for f in attr.fields(C)] == ["field"]
+
+
+def tests_weakref_does_not_add_when_inheriting_with_weakref():
+    """
+    `weakref_slot=True` does not add a new __weakref__ slot when inheriting
+    one.
+    """
+
+    @attr.s(slots=True, weakref_slot=True)
+    class C(object):
+        pass
+
+    @attr.s(slots=True, weakref_slot=True)
+    class D(C):
+        pass
+
+    d = D()
+    w = weakref.ref(d)
+
+    assert d is w()
+
+
+def tests_weakref_does_not_add_with_weakref_attribute():
+    """
+    `weakref_slot=True` does not add a new __weakref__ slot when an attribute
+    of that name exists.
+    """
+
+    @attr.s(slots=True, weakref_slot=True)
+    class C(object):
+        __weakref__ = attr.ib(init=False, hash=False, repr=False, cmp=False)
+
+    c = C()
+    w = weakref.ref(c)
+
+    assert c is w()