From 17191e4387db07a3d022a6c850f1ca73fbd22747 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Tue, 4 Aug 2020 10:58:44 +0200 Subject: [PATCH] Clear __setattr__ if it was inherited && written by us Signed-off-by: Hynek Schlawack --- src/attr/_make.py | 64 +++++++- tests/test_functional.py | 192 +--------------------- tests/test_make.py | 3 + tests/test_setattr.py | 347 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 414 insertions(+), 192 deletions(-) create mode 100644 tests/test_setattr.py diff --git a/src/attr/_make.py b/src/attr/_make.py index 68cdf43ab..9b65590d7 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 @@ 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,7 +587,7 @@ 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)) @@ -593,12 +595,16 @@ def __init__( 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: diff --git a/tests/test_functional.py b/tests/test_functional.py index 6203adc5b..f6a196971 100644 --- a/tests/test_functional.py +++ b/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] diff --git a/tests/test_make.py b/tests/test_make.py index 769a76122..a8e28b32c 100644 --- a/tests/test_make.py +++ b/tests/test_make.py @@ -1466,6 +1466,7 @@ class C(object): False, True, None, + False, ) assert "<_ClassBuilder(cls=C)>" == repr(b) @@ -1491,6 +1492,7 @@ class C(object): False, True, None, + False, ) cls = ( @@ -1568,6 +1570,7 @@ class C(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 000000000..aebd3890b --- /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