Don't reset custom __setattr__ in slotted classes (#681)

* Don't reset custom __setattr__ in slotted classes

Fixes #680

Signed-off-by: Hynek Schlawack <hs@ox.cx>

* Simplify

Signed-off-by: Hynek Schlawack <hs@ox.cx>

* Be defensive about __bases__

Signed-off-by: Hynek Schlawack <hs@ox.cx>

* _Classes_ always have a __dict__

* Tighten xfail

* Clarify what need to be reset and when

Signed-off-by: Hynek Schlawack <hs@ox.cx>

* Reset __attrs_own_setattr__ along with __setattr__

Signed-off-by: Hynek Schlawack <hs@ox.cx>

* Update src/attr/_make.py

Co-authored-by: Paul Ganssle <paul@ganssle.io>

* Differentiate between own (= attrs) and custom (= user) __setattrs__

Signed-off-by: Hynek Schlawack <hs@ox.cx>

* We've always used __bases__ in our call to type()

So no reason for being defensive.

* Update tests/test_setattr.py

Co-authored-by: Paul Ganssle <paul@ganssle.io>

Co-authored-by: Paul Ganssle <paul@ganssle.io>

changelog.d/681.change.rst

@@ -0,0 +1 @@
+It's now possible to define custom ``__setattr__`` methods on slotted classes again.

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 @@ def __init__(
         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 @@ def _patch_original_class(self):
         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 @@ def _create_slots_class(self):
             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 @@ def _create_slots_class(self):
             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 @@ def add_setattr(self):
         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:

tests/test_setattr.py

@@ -227,6 +227,9 @@ class NoHook(WithOnSetAttrHook):
             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 @@ def test_pickling_retains_attrs_own(self, slots):
 
         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 @@ def __setattr__(self, name, val):
 
         i = RemoveNeedForOurSetAttr(1)
 
+        assert not RemoveNeedForOurSetAttr.__attrs_own_setattr__
         assert 2 == i.x
 
     @pytest.mark.parametrize("slots", [True, False])
@@ -345,3 +388,48 @@ class HookAndCustomSetAttr(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__
+        )

tox.ini

@@ -1,6 +1,7 @@
 [pytest]
 addopts = -ra
 testpaths = tests
+xfail_strict = true
 filterwarnings =
     once::Warning
     ignore:::pympler[.*]