Repository: python-attrs/attrs (python)

Issue:
Using slots class overrides custom `__setattr__` in 20.1.0
In 19.3.0, it was possible to have a custom `__setattr__` on your `slots` class, but in 20.1.0 the custom `__setattr__` is replaced with the default one:

```python
import attr
@attr.s(slots=True)
class A:
    def __setattr__(self, key, value):
        print(f"{key}: {value}")
```

In 19.3.0:

```python
>>> A().a = 3
a: 3
```

In 20.1.0:

```python
>>> A().a = 3
AttributeError: 'A' object has no attribute 'a'
>>> A.__setattr__
<slot wrapper '__setattr__' of 'object' objects>
```

When `slots=False`, the same thing does not occur.

Pull request:
Don't reset custom __setattr__ in slotted classes
The detection of whether or not we should be resetting `__setattr__` in slotted classes is currently wrong.

Fixes #680 

Patch:
diff --git a/changelog.d/681.change.rst b/changelog.d/681.change.rst
new file mode 100644
index 0000000..a4aaa32
--- /dev/null
+++ b/changelog.d/681.change.rst
@@ -0,0 +1,1 @@
+It's now possible to define custom ``__setattr__`` methods on slotted classes again.
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 088f2a5..0fbbd7c 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -556,6 +556,7 @@ class _ClassBuilder(object):
         "_slots",
         "_weakref_slot",
         "_has_own_setattr",
+        "_has_custom_setattr",
     )
 
     def __init__(
@@ -593,7 +594,8 @@ class _ClassBuilder(object):
         self._is_exc = is_exc
         self._on_setattr = on_setattr
 
-        self._has_own_setattr = has_custom_setattr
+        self._has_custom_setattr = has_custom_setattr
+        self._has_own_setattr = False
 
         self._cls_dict["__attrs_attrs__"] = self._attrs
 
@@ -654,7 +656,10 @@ class _ClassBuilder(object):
         if not self._has_own_setattr and getattr(
             cls, "__attrs_own_setattr__", False
         ):
-            cls.__setattr__ = object.__setattr__
+            cls.__attrs_own_setattr__ = False
+
+            if not self._has_custom_setattr:
+                cls.__setattr__ = object.__setattr__
 
         return cls
 
@@ -669,24 +674,30 @@ class _ClassBuilder(object):
             if k not in tuple(self._attr_names) + ("__dict__", "__weakref__")
         }
 
-        # Traverse the MRO to check for an existing __weakref__ and
-        # __setattr__.
-        custom_setattr_inherited = False
+        # 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
+        # MRO because we only care about our immediate base classes.
+        # XXX: This can be confused by subclassing a slotted attrs class with
+        # XXX: a non-attrs class and subclass the resulting class with an attrs
+        # XXX: class.  See `test_slotted_confused` for details.  For now that's
+        # XXX: OK with us.
+        if not self._has_own_setattr:
+            cd["__attrs_own_setattr__"] = False
+
+            if not self._has_custom_setattr:
+                for base_cls in self._cls.__bases__:
+                    if base_cls.__dict__.get("__attrs_own_setattr__", False):
+                        cd["__setattr__"] = object.__setattr__
+                        break
+
+        # Traverse the MRO to check for an existing __weakref__.
         weakref_inherited = False
         for base_cls in self._cls.__mro__[1:-1]:
-            d = getattr(base_cls, "__dict__", {})
-
-            weakref_inherited = weakref_inherited or "__weakref__" in d
-            custom_setattr_inherited = custom_setattr_inherited or not (
-                d.get("__attrs_own_setattr__", False)
-            )
-
-            if weakref_inherited and custom_setattr_inherited:
+            if base_cls.__dict__.get("__weakref__", None) is not None:
+                weakref_inherited = True
                 break
 
-        if not self._has_own_setattr and not custom_setattr_inherited:
-            cd["__setattr__"] = object.__setattr__
-
         names = self._attr_names
         if (
             self._weakref_slot
@@ -697,7 +708,7 @@ class _ClassBuilder(object):
             names += ("__weakref__",)
 
         # We only add the names of attributes that aren't inherited.
-        # Settings __slots__ to inherited attributes wastes memory.
+        # Setting __slots__ to inherited attributes wastes memory.
         slot_names = [name for name in names if name not in base_names]
         if self._cache_hash:
             slot_names.append(_hash_cache_field)
@@ -857,20 +868,14 @@ class _ClassBuilder(object):
         if not sa_attrs:
             return self
 
-        if self._has_own_setattr:
+        if self._has_custom_setattr:
             # We need to write a __setattr__ but there already is one!
             raise ValueError(
                 "Can't combine custom __setattr__ with on_setattr hooks."
             )
 
-        cls = self._cls
-
+        # docstring comes from _add_method_dunders
         def __setattr__(self, name, val):
-            """
-            Method generated by attrs for class %s.
-            """ % (
-                cls.__name__,
-            )
             try:
                 a, hook = sa_attrs[name]
             except KeyError:
diff --git a/tox.ini b/tox.ini
index 1f04ec7..0508fda 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,6 +1,7 @@
 [pytest]
 addopts = -ra
 testpaths = tests
+xfail_strict = true
 filterwarnings =
     once::Warning
     ignore:::pympler[.*]


Test patch:
diff --git a/tests/test_setattr.py b/tests/test_setattr.py
index aebd389..8e55da2 100644
--- a/tests/test_setattr.py
+++ b/tests/test_setattr.py
@@ -227,6 +227,9 @@ class TestSetAttr(object):
             assert NoHook.__setattr__ == object.__setattr__
 
         assert 1 == NoHook(1).x
+        assert Hooked.__attrs_own_setattr__
+        assert not NoHook.__attrs_own_setattr__
+        assert WithOnSetAttrHook.__attrs_own_setattr__
 
     @pytest.mark.parametrize("slots", [True, False])
     def test_setattr_inherited_do_not_reset(self, slots):
@@ -274,6 +277,45 @@ class TestSetAttr(object):
 
         assert True is WOSAH.__attrs_own_setattr__
 
+    def test_slotted_class_can_have_custom_setattr(self):
+        """
+        A slotted class can define a custom setattr and it doesn't get
+        overwritten.
+
+        Regression test for #680.
+        """
+
+        @attr.s(slots=True)
+        class A(object):
+            def __setattr__(self, key, value):
+                raise SystemError
+
+        with pytest.raises(SystemError):
+            A().x = 1
+
+    @pytest.mark.xfail(raises=attr.exceptions.FrozenAttributeError)
+    def test_slotted_confused(self):
+        """
+        If we have a in-between non-attrs class, setattr reset detection
+        should still work, but currently doesn't.
+
+        It works with dict classes because we can look the finished class and
+        patch it.  With slotted classes we have to deduce it ourselves.
+        """
+
+        @attr.s(slots=True)
+        class A(object):
+            x = attr.ib(on_setattr=setters.frozen)
+
+        class B(A):
+            pass
+
+        @attr.s(slots=True)
+        class C(B):
+            x = attr.ib()
+
+        C(1).x = 2
+
 
 @pytest.mark.skipif(PY2, reason="Python 3-only.")
 class TestSetAttrNoPy2(object):
@@ -298,6 +340,7 @@ class TestSetAttrNoPy2(object):
 
         i = RemoveNeedForOurSetAttr(1)
 
+        assert not RemoveNeedForOurSetAttr.__attrs_own_setattr__
         assert 2 == i.x
 
     @pytest.mark.parametrize("slots", [True, False])
@@ -345,3 +388,48 @@ class TestSetAttrNoPy2(object):
 
                 def __setattr__(self, _, __):
                     pass
+
+    @pytest.mark.parametrize("a_slots", [True, False])
+    @pytest.mark.parametrize("b_slots", [True, False])
+    @pytest.mark.parametrize("c_slots", [True, False])
+    def test_setattr_inherited_do_not_reset_intermediate(
+        self, a_slots, b_slots, c_slots
+    ):
+        """
+        A user-provided intermediate __setattr__ is not reset to
+        object.__setattr__.
+
+        This only can work on Python 3+ with auto_detect activated, such that
+        attrs can know that there is a user-provided __setattr__.
+        """
+
+        @attr.s(slots=a_slots)
+        class A(object):
+            x = attr.ib(on_setattr=setters.frozen)
+
+        @attr.s(slots=b_slots, auto_detect=True)
+        class B(A):
+            x = attr.ib(on_setattr=setters.NO_OP)
+
+            def __setattr__(self, key, value):
+                raise SystemError
+
+        @attr.s(slots=c_slots)
+        class C(B):
+            pass
+
+        assert getattr(A, "__attrs_own_setattr__", False) is True
+        assert getattr(B, "__attrs_own_setattr__", False) is False
+        assert getattr(C, "__attrs_own_setattr__", False) is False
+
+        with pytest.raises(SystemError):
+            C(1).x = 3
+
+    def test_docstring(self):
+        """
+        Generated __setattr__ has a useful docstring.
+        """
+        assert (
+            "Method generated by attrs for class WithOnSetAttrHook."
+            == WithOnSetAttrHook.__setattr__.__doc__
+        )