Repository: python-attrs/attrs (python)

Issue:
Restore __setattr__ when subclassing and we didn't write a new __setattr__
Currently if you do this:

```python
@attr.s(frozen=True)
class A:
    a = attr.ib()

@attr.s
class B(A):
    b = attr.ib()
```

B is frozen too, because we don't write a new `__setattr__` to overwrite the old one. Once #660 is merged, this will become an even bigger problem.

We have to restore `__setattr__` to `object.__setattr__` if we don't have a reason to overwrite the existing one ourselves.

`auto_detect=True` should be considered.

Pull request:
Clear __setattr__ if it was inherited && written by us
Fixes #661 

Since the frozen behavior was deliberate, the changes should only affect new `on_setattr` code.

This is the final PR before 20.1.0.

I hope we can ship 20.2.0 with `import attrs`, equality, and maybe better serialization soon after.

Patch:
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 68cdf43..9b65590 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -557,6 +557,7 @@ class _ClassBuilder(object):
         "_on_setattr",
         "_slots",
         "_weakref_slot",
+        "_has_own_setattr",
     )
 
     def __init__(
@@ -573,6 +574,7 @@ class _ClassBuilder(object):
         is_exc,
         collect_by_mro,
         on_setattr,
+        has_custom_setattr,
     ):
         attrs, base_attrs, base_map = _transform_attrs(
             cls, these, auto_attribs, kw_only, collect_by_mro,
@@ -585,7 +587,7 @@ class _ClassBuilder(object):
         self._base_attr_map = base_map
         self._attr_names = tuple(a.name for a in attrs)
         self._slots = slots
-        self._frozen = frozen or _has_frozen_base_class(cls)
+        self._frozen = frozen
         self._weakref_slot = weakref_slot
         self._cache_hash = cache_hash
         self._has_post_init = bool(getattr(cls, "__attrs_post_init__", False))
@@ -593,12 +595,16 @@ class _ClassBuilder(object):
         self._is_exc = is_exc
         self._on_setattr = on_setattr
 
+        self._has_own_setattr = has_custom_setattr
+
         self._cls_dict["__attrs_attrs__"] = self._attrs
 
         if frozen:
             self._cls_dict["__setattr__"] = _frozen_setattrs
             self._cls_dict["__delattr__"] = _frozen_delattrs
 
+            self._has_own_setattr = True
+
         if getstate_setstate:
             (
                 self._cls_dict["__getstate__"],
@@ -645,6 +651,13 @@ class _ClassBuilder(object):
         for name, value in self._cls_dict.items():
             setattr(cls, name, value)
 
+        # If we've inherited an attrs __setattr__ and don't write our own,
+        # reset it to object's.
+        if not self._has_own_setattr and getattr(
+            cls, "__attrs_own_setattr__", False
+        ):
+            cls.__setattr__ = object.__setattr__
+
         return cls
 
     def _create_slots_class(self):
@@ -658,14 +671,24 @@ 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
         weakref_inherited = False
-
-        # Traverse the MRO to check for an existing __weakref__.
         for base_cls in self._cls.__mro__[1:-1]:
-            if "__weakref__" in getattr(base_cls, "__dict__", ()):
-                weakref_inherited = True
+            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:
                 break
 
+        if not self._has_own_setattr and not custom_setattr_inherited:
+            cd["__setattr__"] = object.__setattr__
+
         names = self._attr_names
         if (
             self._weakref_slot
@@ -836,7 +859,20 @@ class _ClassBuilder(object):
         if not sa_attrs:
             return self
 
+        if self._has_own_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
+
         def __setattr__(self, name, val):
+            """
+            Method generated by attrs for class %s.
+            """ % (
+                cls.__name__,
+            )
             try:
                 a, hook = sa_attrs[name]
             except KeyError:
@@ -846,7 +882,9 @@ class _ClassBuilder(object):
 
             _obj_setattr(self, name, nval)
 
+        self._cls_dict["__attrs_own_setattr__"] = True
         self._cls_dict["__setattr__"] = self._add_method_dunders(__setattr__)
+        self._has_own_setattr = True
 
         return self
 
@@ -1076,6 +1114,8 @@ def attrs(
                circumvent that limitation by using
                ``object.__setattr__(self, "attribute_name", value)``.
 
+            5. Subclasses of a frozen class are frozen too.
+
     :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
@@ -1200,13 +1240,20 @@ def attrs(
         if getattr(cls, "__class__", None) is None:
             raise TypeError("attrs only works with new-style classes.")
 
+        is_frozen = frozen or _has_frozen_base_class(cls)
         is_exc = auto_exc is True and issubclass(cls, BaseException)
+        has_own_setattr = auto_detect and _has_own_attribute(
+            cls, "__setattr__"
+        )
+
+        if has_own_setattr and is_frozen:
+            raise ValueError("Can't freeze a class with a custom __setattr__.")
 
         builder = _ClassBuilder(
             cls,
             these,
             slots,
-            frozen,
+            is_frozen,
             weakref_slot,
             _determine_whether_to_implement(
                 cls,
@@ -1221,6 +1268,7 @@ def attrs(
             is_exc,
             collect_by_mro,
             on_setattr,
+            has_own_setattr,
         )
         if _determine_whether_to_implement(
             cls, repr, auto_detect, ("__repr__",)
@@ -1263,7 +1311,9 @@ def attrs(
                     " hashing must be either explicitly or implicitly "
                     "enabled."
                 )
-        elif hash is True or (hash is None and eq is True and frozen is True):
+        elif hash is True or (
+            hash is None and eq is True and is_frozen is True
+        ):
             # Build a __hash__ if told so, or if it's safe.
             builder.add_hash()
         else:


Test patch:
diff --git a/tests/test_functional.py b/tests/test_functional.py
index 6203adc..f6a1969 100644
--- a/tests/test_functional.py
+++ b/tests/test_functional.py
@@ -16,11 +16,9 @@ from hypothesis.strategies import booleans
 
 import attr
 
-from attr import setters
 from attr._compat import PY2, TYPE
 from attr._make import NOTHING, Attribute
-from attr.exceptions import FrozenAttributeError, FrozenInstanceError
-from attr.validators import instance_of, matches_re
+from attr.exceptions import FrozenInstanceError
 
 from .strategies import optional_bool
 
@@ -112,9 +110,9 @@ class WithMetaSlots(object):
 FromMakeClass = attr.make_class("FromMakeClass", ["x"])
 
 
-class TestDarkMagic(object):
+class TestFunctional(object):
     """
-    Integration tests.
+    Functional tests.
     """
 
     @pytest.mark.parametrize("cls", [C2, C2Slots])
@@ -320,6 +318,7 @@ class TestDarkMagic(object):
             obj = cls(123, 456)
         else:
             obj = cls(123)
+
         assert repr(obj) == repr(pickle.loads(pickle.dumps(obj, protocol)))
 
     def test_subclassing_frozen_gives_frozen(self):
@@ -332,6 +331,9 @@ class TestDarkMagic(object):
         assert i.x == "foo"
         assert i.y == "bar"
 
+        with pytest.raises(FrozenInstanceError):
+            i.x = "baz"
+
     @pytest.mark.parametrize("cls", [WithMeta, WithMetaSlots])
     def test_metaclass_preserved(self, cls):
         """
@@ -683,183 +685,3 @@ class TestDarkMagic(object):
             "2021-06-01.  Please use `eq` and `order` instead."
             == w.message.args[0]
         )
-
-
-class TestSetAttr(object):
-    def test_change(self):
-        """
-        The return value of a hook overwrites the value. But they are not run
-        on __init__.
-        """
-
-        def hook(*a, **kw):
-            return "hooked!"
-
-        @attr.s
-        class Hooked(object):
-            x = attr.ib(on_setattr=hook)
-            y = attr.ib()
-
-        h = Hooked("x", "y")
-
-        assert "x" == h.x
-        assert "y" == h.y
-
-        h.x = "xxx"
-        h.y = "yyy"
-
-        assert "yyy" == h.y
-        assert "hooked!" == h.x
-
-    def test_frozen_attribute(self):
-        """
-        Frozen attributes raise FrozenAttributeError, others are not affected.
-        """
-
-        @attr.s
-        class PartiallyFrozen(object):
-            x = attr.ib(on_setattr=setters.frozen)
-            y = attr.ib()
-
-        pf = PartiallyFrozen("x", "y")
-
-        pf.y = "yyy"
-
-        assert "yyy" == pf.y
-
-        with pytest.raises(FrozenAttributeError):
-            pf.x = "xxx"
-
-        assert "x" == pf.x
-
-    @pytest.mark.parametrize(
-        "on_setattr",
-        [setters.validate, [setters.validate], setters.pipe(setters.validate)],
-    )
-    def test_validator(self, on_setattr):
-        """
-        Validators are run and they don't alter the value.
-        """
-
-        @attr.s(on_setattr=on_setattr)
-        class ValidatedAttribute(object):
-            x = attr.ib()
-            y = attr.ib(validator=[instance_of(str), matches_re("foo.*qux")])
-
-        va = ValidatedAttribute(42, "foobarqux")
-
-        with pytest.raises(TypeError) as ei:
-            va.y = 42
-
-        assert "foobarqux" == va.y
-
-        assert ei.value.args[0].startswith("'y' must be <")
-
-        with pytest.raises(ValueError) as ei:
-            va.y = "quxbarfoo"
-
-        assert ei.value.args[0].startswith("'y' must match regex '")
-
-        assert "foobarqux" == va.y
-
-        va.y = "foobazqux"
-
-        assert "foobazqux" == va.y
-
-    def test_pipe(self):
-        """
-        Multiple hooks are possible, in that case the last return value is
-        used. They can be supplied using the pipe functions or by passing a
-        list to on_setattr.
-        """
-
-        s = [setters.convert, lambda _, __, nv: nv + 1]
-
-        @attr.s
-        class Piped(object):
-            x1 = attr.ib(converter=int, on_setattr=setters.pipe(*s))
-            x2 = attr.ib(converter=int, on_setattr=s)
-
-        p = Piped("41", "22")
-
-        assert 41 == p.x1
-        assert 22 == p.x2
-
-        p.x1 = "41"
-        p.x2 = "22"
-
-        assert 42 == p.x1
-        assert 23 == p.x2
-
-    def test_make_class(self):
-        """
-        on_setattr of make_class gets forwarded.
-        """
-        C = attr.make_class("C", {"x": attr.ib()}, on_setattr=setters.frozen)
-
-        c = C(1)
-
-        with pytest.raises(FrozenAttributeError):
-            c.x = 2
-
-    def test_no_validator_no_converter(self):
-        """
-        validate and convert tolerate missing validators and converters.
-        """
-
-        @attr.s(on_setattr=[setters.convert, setters.validate])
-        class C(object):
-            x = attr.ib()
-
-        c = C(1)
-
-        c.x = 2
-
-    def test_validate_respects_run_validators_config(self):
-        """
-        If run validators is off, validate doesn't run them.
-        """
-
-        @attr.s(on_setattr=setters.validate)
-        class C(object):
-            x = attr.ib(validator=attr.validators.instance_of(int))
-
-        c = C(1)
-
-        attr.set_run_validators(False)
-
-        c.x = "1"
-
-        assert "1" == c.x
-
-        attr.set_run_validators(True)
-
-        with pytest.raises(TypeError) as ei:
-            c.x = "1"
-
-        assert ei.value.args[0].startswith("'x' must be <")
-
-    def test_frozen_on_setattr_class_is_caught(self):
-        """
-        @attr.s(on_setattr=X, frozen=True) raises an ValueError.
-        """
-        with pytest.raises(ValueError) as ei:
-
-            @attr.s(frozen=True, on_setattr=setters.validate)
-            class C(object):
-                x = attr.ib()
-
-        assert "Frozen classes can't use on_setattr." == ei.value.args[0]
-
-    def test_frozen_on_setattr_attribute_is_caught(self):
-        """
-        attr.ib(on_setattr=X) on a frozen class raises an ValueError.
-        """
-
-        with pytest.raises(ValueError) as ei:
-
-            @attr.s(frozen=True)
-            class C(object):
-                x = attr.ib(on_setattr=setters.validate)
-
-        assert "Frozen classes can't use on_setattr." == ei.value.args[0]
diff --git a/tests/test_make.py b/tests/test_make.py
index 769a761..a8e28b3 100644
--- a/tests/test_make.py
+++ b/tests/test_make.py
@@ -1466,6 +1466,7 @@ class TestClassBuilder(object):
             False,
             True,
             None,
+            False,
         )
 
         assert "<_ClassBuilder(cls=C)>" == repr(b)
@@ -1491,6 +1492,7 @@ class TestClassBuilder(object):
             False,
             True,
             None,
+            False,
         )
 
         cls = (
@@ -1568,6 +1570,7 @@ class TestClassBuilder(object):
             cache_hash=False,
             collect_by_mro=True,
             on_setattr=None,
+            has_custom_setattr=False,
         )
         b._cls = {}  # no __module__; no __qualname__
 
diff --git a/tests/test_setattr.py b/tests/test_setattr.py
new file mode 100644
index 0000000..aebd389
--- /dev/null
+++ b/tests/test_setattr.py
@@ -0,0 +1,347 @@
+from __future__ import absolute_import, division, print_function
+
+import pickle
+
+import pytest
+
+import attr
+
+from attr import setters
+from attr._compat import PY2
+from attr.exceptions import FrozenAttributeError
+from attr.validators import instance_of, matches_re
+
+
+@attr.s(frozen=True)
+class Frozen(object):
+    x = attr.ib()
+
+
+@attr.s
+class WithOnSetAttrHook(object):
+    x = attr.ib(on_setattr=lambda *args: None)
+
+
+class TestSetAttr(object):
+    def test_change(self):
+        """
+        The return value of a hook overwrites the value. But they are not run
+        on __init__.
+        """
+
+        def hook(*a, **kw):
+            return "hooked!"
+
+        @attr.s
+        class Hooked(object):
+            x = attr.ib(on_setattr=hook)
+            y = attr.ib()
+
+        h = Hooked("x", "y")
+
+        assert "x" == h.x
+        assert "y" == h.y
+
+        h.x = "xxx"
+        h.y = "yyy"
+
+        assert "yyy" == h.y
+        assert "hooked!" == h.x
+
+    def test_frozen_attribute(self):
+        """
+        Frozen attributes raise FrozenAttributeError, others are not affected.
+        """
+
+        @attr.s
+        class PartiallyFrozen(object):
+            x = attr.ib(on_setattr=setters.frozen)
+            y = attr.ib()
+
+        pf = PartiallyFrozen("x", "y")
+
+        pf.y = "yyy"
+
+        assert "yyy" == pf.y
+
+        with pytest.raises(FrozenAttributeError):
+            pf.x = "xxx"
+
+        assert "x" == pf.x
+
+    @pytest.mark.parametrize(
+        "on_setattr",
+        [setters.validate, [setters.validate], setters.pipe(setters.validate)],
+    )
+    def test_validator(self, on_setattr):
+        """
+        Validators are run and they don't alter the value.
+        """
+
+        @attr.s(on_setattr=on_setattr)
+        class ValidatedAttribute(object):
+            x = attr.ib()
+            y = attr.ib(validator=[instance_of(str), matches_re("foo.*qux")])
+
+        va = ValidatedAttribute(42, "foobarqux")
+
+        with pytest.raises(TypeError) as ei:
+            va.y = 42
+
+        assert "foobarqux" == va.y
+
+        assert ei.value.args[0].startswith("'y' must be <")
+
+        with pytest.raises(ValueError) as ei:
+            va.y = "quxbarfoo"
+
+        assert ei.value.args[0].startswith("'y' must match regex '")
+
+        assert "foobarqux" == va.y
+
+        va.y = "foobazqux"
+
+        assert "foobazqux" == va.y
+
+    def test_pipe(self):
+        """
+        Multiple hooks are possible, in that case the last return value is
+        used. They can be supplied using the pipe functions or by passing a
+        list to on_setattr.
+        """
+
+        s = [setters.convert, lambda _, __, nv: nv + 1]
+
+        @attr.s
+        class Piped(object):
+            x1 = attr.ib(converter=int, on_setattr=setters.pipe(*s))
+            x2 = attr.ib(converter=int, on_setattr=s)
+
+        p = Piped("41", "22")
+
+        assert 41 == p.x1
+        assert 22 == p.x2
+
+        p.x1 = "41"
+        p.x2 = "22"
+
+        assert 42 == p.x1
+        assert 23 == p.x2
+
+    def test_make_class(self):
+        """
+        on_setattr of make_class gets forwarded.
+        """
+        C = attr.make_class("C", {"x": attr.ib()}, on_setattr=setters.frozen)
+
+        c = C(1)
+
+        with pytest.raises(FrozenAttributeError):
+            c.x = 2
+
+    def test_no_validator_no_converter(self):
+        """
+        validate and convert tolerate missing validators and converters.
+        """
+
+        @attr.s(on_setattr=[setters.convert, setters.validate])
+        class C(object):
+            x = attr.ib()
+
+        c = C(1)
+
+        c.x = 2
+
+        assert 2 == c.x
+
+    def test_validate_respects_run_validators_config(self):
+        """
+        If run validators is off, validate doesn't run them.
+        """
+
+        @attr.s(on_setattr=setters.validate)
+        class C(object):
+            x = attr.ib(validator=attr.validators.instance_of(int))
+
+        c = C(1)
+
+        attr.set_run_validators(False)
+
+        c.x = "1"
+
+        assert "1" == c.x
+
+        attr.set_run_validators(True)
+
+        with pytest.raises(TypeError) as ei:
+            c.x = "1"
+
+        assert ei.value.args[0].startswith("'x' must be <")
+
+    def test_frozen_on_setattr_class_is_caught(self):
+        """
+        @attr.s(on_setattr=X, frozen=True) raises an ValueError.
+        """
+        with pytest.raises(ValueError) as ei:
+
+            @attr.s(frozen=True, on_setattr=setters.validate)
+            class C(object):
+                x = attr.ib()
+
+        assert "Frozen classes can't use on_setattr." == ei.value.args[0]
+
+    def test_frozen_on_setattr_attribute_is_caught(self):
+        """
+        attr.ib(on_setattr=X) on a frozen class raises an ValueError.
+        """
+
+        with pytest.raises(ValueError) as ei:
+
+            @attr.s(frozen=True)
+            class C(object):
+                x = attr.ib(on_setattr=setters.validate)
+
+        assert "Frozen classes can't use on_setattr." == ei.value.args[0]
+
+    @pytest.mark.parametrize("slots", [True, False])
+    def test_setattr_reset_if_no_custom_setattr(self, slots):
+        """
+        If a class with an active setattr is subclassed and no new setattr
+        is generated, the __setattr__ is set to object.__setattr__.
+
+        We do the double test because of Python 2.
+        """
+
+        def boom(*args):
+            pytest.fail("Must not be called.")
+
+        @attr.s
+        class Hooked(object):
+            x = attr.ib(on_setattr=boom)
+
+        @attr.s(slots=slots)
+        class NoHook(WithOnSetAttrHook):
+            x = attr.ib()
+
+        if not PY2:
+            assert NoHook.__setattr__ == object.__setattr__
+
+        assert 1 == NoHook(1).x
+
+    @pytest.mark.parametrize("slots", [True, False])
+    def test_setattr_inherited_do_not_reset(self, slots):
+        """
+        If we inherit a __setattr__ that has been written by the user, we must
+        not reset it unless necessary.
+        """
+
+        class A(object):
+            """
+            Not an attrs class on purpose to prevent accidental resets that
+            would render the asserts meaningless.
+            """
+
+            def __setattr__(self, *args):
+                pass
+
+        @attr.s(slots=slots)
+        class B(A):
+            pass
+
+        assert B.__setattr__ == A.__setattr__
+
+        @attr.s(slots=slots)
+        class C(B):
+            pass
+
+        assert C.__setattr__ == A.__setattr__
+
+    @pytest.mark.parametrize("slots", [True, False])
+    def test_pickling_retains_attrs_own(self, slots):
+        """
+        Pickling/Unpickling does not lose ownership information about
+        __setattr__.
+        """
+        i = WithOnSetAttrHook(1)
+
+        assert True is i.__attrs_own_setattr__
+
+        i2 = pickle.loads(pickle.dumps(i))
+
+        assert True is i2.__attrs_own_setattr__
+
+        WOSAH = pickle.loads(pickle.dumps(WithOnSetAttrHook))
+
+        assert True is WOSAH.__attrs_own_setattr__
+
+
+@pytest.mark.skipif(PY2, reason="Python 3-only.")
+class TestSetAttrNoPy2(object):
+    """
+    __setattr__ tests for Py3+ to avoid the skip repetition.
+    """
+
+    @pytest.mark.parametrize("slots", [True, False])
+    def test_setattr_auto_detect_if_no_custom_setattr(self, slots):
+        """
+        It's possible to remove the on_setattr hook from an attribute and
+        therefore write a custom __setattr__.
+        """
+        assert 1 == WithOnSetAttrHook(1).x
+
+        @attr.s(auto_detect=True, slots=slots)
+        class RemoveNeedForOurSetAttr(WithOnSetAttrHook):
+            x = attr.ib()
+
+            def __setattr__(self, name, val):
+                object.__setattr__(self, name, val * 2)
+
+        i = RemoveNeedForOurSetAttr(1)
+
+        assert 2 == i.x
+
+    @pytest.mark.parametrize("slots", [True, False])
+    def test_setattr_restore_respects_auto_detect(self, slots):
+        """
+        If __setattr__ should be restored but the user supplied its own and
+        set auto_detect, leave is alone.
+        """
+
+        @attr.s(auto_detect=True, slots=slots)
+        class CustomSetAttr:
+            def __setattr__(self, _, __):
+                pass
+
+        assert CustomSetAttr.__setattr__ != object.__setattr__
+
+    @pytest.mark.parametrize("slots", [True, False])
+    def test_setattr_auto_detect_frozen(self, slots):
+        """
+        frozen=True together with a detected custom __setattr__ are rejected.
+        """
+        with pytest.raises(
+            ValueError, match="Can't freeze a class with a custom __setattr__."
+        ):
+
+            @attr.s(auto_detect=True, slots=slots, frozen=True)
+            class CustomSetAttr(Frozen):
+                def __setattr__(self, _, __):
+                    pass
+
+    @pytest.mark.parametrize("slots", [True, False])
+    def test_setattr_auto_detect_on_setattr(self, slots):
+        """
+        on_setattr attributes together with a detected custom __setattr__ are
+        rejected.
+        """
+        with pytest.raises(
+            ValueError,
+            match="Can't combine custom __setattr__ with on_setattr hooks.",
+        ):
+
+            @attr.s(auto_detect=True, slots=slots)
+            class HookAndCustomSetAttr(object):
+                x = attr.ib(on_setattr=lambda *args: None)
+
+                def __setattr__(self, _, __):
+                    pass