From 48755905ee8088da927a6dc583d4962f7ca16b9b Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 5 Sep 2020 09:46:06 +0200 Subject: [PATCH] Don't reset custom __setattr__ in slotted classes (#681) * Don't reset custom __setattr__ in slotted classes Fixes #680 Signed-off-by: Hynek Schlawack * Simplify Signed-off-by: Hynek Schlawack * Be defensive about __bases__ Signed-off-by: Hynek Schlawack * _Classes_ always have a __dict__ * Tighten xfail * Clarify what need to be reset and when Signed-off-by: Hynek Schlawack * Reset __attrs_own_setattr__ along with __setattr__ Signed-off-by: Hynek Schlawack * Update src/attr/_make.py Co-authored-by: Paul Ganssle * Differentiate between own (= attrs) and custom (= user) __setattrs__ Signed-off-by: Hynek Schlawack * 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 Co-authored-by: Paul Ganssle --- changelog.d/681.change.rst | 1 + src/attr/_make.py | 55 +++++++++++++----------- tests/test_setattr.py | 88 ++++++++++++++++++++++++++++++++++++++ tox.ini | 1 + 4 files changed, 120 insertions(+), 25 deletions(-) create mode 100644 changelog.d/681.change.rst diff --git a/changelog.d/681.change.rst b/changelog.d/681.change.rst new file mode 100644 index 000000000..a4aaa3240 --- /dev/null +++ b/changelog.d/681.change.rst @@ -0,0 +1 @@ +It's now possible to define custom ``__setattr__`` methods on slotted classes again. diff --git a/src/attr/_make.py b/src/attr/_make.py index 088f2a5df..0fbbd7c16 100644 --- a/src/attr/_make.py +++ b/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: diff --git a/tests/test_setattr.py b/tests/test_setattr.py index aebd3890b..8e55da2d1 100644 --- a/tests/test_setattr.py +++ b/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__ + ) 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[.*]