From 67237098e0fd7b96fd5d646011598ec6c8f5b648 Mon Sep 17 00:00:00 2001
From: Hynek Schlawack <hs@ox.cx>
Date: Tue, 1 Sep 2020 05:50:27 +0200
Subject: [PATCH] Don't reset custom __setattr__ in slotted classes

Fixes #680

Signed-off-by: Hynek Schlawack <hs@ox.cx>
---
 .pre-commit-config.yaml |  4 ++--
 src/attr/_compat.py     |  3 ++-
 src/attr/_make.py       | 35 ++++++++++++++++++++---------------
 tests/test_setattr.py   | 39 +++++++++++++++++++++++++++++++++++++++
 tox.ini                 |  1 +
 5 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index c2f0dcd64..433908707 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -15,7 +15,7 @@ repos:
       - id: seed-isort-config
 
   - repo: https://github.com/pre-commit/mirrors-isort
-    rev: v4.3.21
+    rev: v5.4.2
     hooks:
       - id: isort
         additional_dependencies: [toml]
@@ -34,7 +34,7 @@ repos:
         language_version: python3.8
 
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v3.1.0
+    rev: v3.2.0
     hooks:
       - id: trailing-whitespace
       - id: end-of-file-fixer
diff --git a/src/attr/_compat.py b/src/attr/_compat.py
index a915db8eb..bed5b1357 100644
--- a/src/attr/_compat.py
+++ b/src/attr/_compat.py
@@ -19,9 +19,10 @@
 
 
 if PY2:
-    from UserDict import IterableUserDict
     from collections import Mapping, Sequence
 
+    from UserDict import IterableUserDict
+
     # We 'bundle' isclass instead of using inspect as importing inspect is
     # fairly expensive (order of 10-15 ms for a modern machine in 2016)
     def isclass(klass):
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 1e6b4bdc8..ec9b947d2 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -669,24 +669,29 @@ 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
+        # Check the bases if one of them has an attrs-made __setattr__ that
+        # needs to be reset.
+        # 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.
+        for base_cls in self._cls.__bases__:
+            if not self._has_own_setattr and getattr(
+                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 (
+                getattr(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 +702,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)
diff --git a/tests/test_setattr.py b/tests/test_setattr.py
index aebd3890b..2fc781bf0 100644
--- a/tests/test_setattr.py
+++ b/tests/test_setattr.py
@@ -274,6 +274,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
+    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):
diff --git a/tox.ini b/tox.ini
index 1f04ec79a..0508fdaa8 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,6 +1,7 @@
 [pytest]
 addopts = -ra
 testpaths = tests
+xfail_strict = true
 filterwarnings =
     once::Warning
     ignore:::pympler[.*]