Clear __setattr__ if it was inherited && written by us (#663)

Signed-off-by: Hynek Schlawack <[email protected]>

Author: hynek

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 @@ def __init__(
         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,20 +587,24 @@ def __init__(
         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))
         self._delete_attribs = not bool(these)
         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 @@ def _patch_original_class(self):
         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 @@ 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
         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 @@ def add_setattr(self):
         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 @@ def __setattr__(self, name, val):
 
             _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 wrap(cls):
         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 wrap(cls):
             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 wrap(cls):
                     " 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:

tests/test_functional.py

@@ -16,11 +16,9 @@
 
 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 @@ def test_pickle_object(self, cls, protocol):
             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 @@ def test_subclassing_frozen_gives_frozen(self):
         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 C(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]