From a415d518a277feb5c63641352289557628997066 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Tue, 1 Sep 2020 05:50:27 +0200 Subject: [PATCH 01/11] Don't reset custom __setattr__ in slotted classes Fixes #680 Signed-off-by: Hynek Schlawack --- changelog.d/681.change.rst | 1 + src/attr/_make.py | 35 +++++++++++++++++++--------------- tests/test_setattr.py | 39 ++++++++++++++++++++++++++++++++++++++ tox.ini | 1 + 4 files changed, 61 insertions(+), 15 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..216f3d0a6 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[.*] From 79048886ab039ad651c27af09221daafe2dbd05b Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Wed, 2 Sep 2020 07:59:27 +0200 Subject: [PATCH 02/11] Simplify Signed-off-by: Hynek Schlawack --- src/attr/_make.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 216f3d0a6..4533cc052 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -675,12 +675,11 @@ def _create_slots_class(self): # 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 + if not self._has_own_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 From e0ffd23bd6b5e9690c5525a1a5ac7fbbb3981a21 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Wed, 2 Sep 2020 08:05:22 +0200 Subject: [PATCH 03/11] Be defensive about __bases__ Signed-off-by: Hynek Schlawack --- src/attr/_make.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 4533cc052..9e670ff65 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -676,7 +676,9 @@ def _create_slots_class(self): # XXX: class. See `test_slotted_confused for details. For now that's # XXX: OK with us. if not self._has_own_setattr: - for base_cls in self._cls.__bases__: + # There's metaclass magic that may result in a baseclass without + # __bases__ which results in _us_ not having one. cf. #681 + for base_cls in getattr(self._cls, "__bases__", ()): if base_cls.__dict__.get("__attrs_own_setattr__", False): cd["__setattr__"] = object.__setattr__ break From c2b6a0b274104c451cc939600a81c8f6970956a0 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Wed, 2 Sep 2020 08:09:15 +0200 Subject: [PATCH 04/11] _Classes_ always have a __dict__ --- src/attr/_make.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 9e670ff65..1d0ee375f 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -686,10 +686,7 @@ def _create_slots_class(self): # Traverse the MRO to check for an existing __weakref__. weakref_inherited = False for base_cls in self._cls.__mro__[1:-1]: - if ( - getattr(base_cls, "__dict__", {}).get("__weakref__", None) - is not None - ): + if base_cls.__dict__.get("__weakref__", None) is not None: weakref_inherited = True break From 3adf48a1451274b1d5e00433f201165ed20f6bf1 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Wed, 2 Sep 2020 08:10:43 +0200 Subject: [PATCH 05/11] Tighten xfail --- tests/test_setattr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_setattr.py b/tests/test_setattr.py index 2fc781bf0..9db72c952 100644 --- a/tests/test_setattr.py +++ b/tests/test_setattr.py @@ -290,7 +290,7 @@ def __setattr__(self, key, value): with pytest.raises(SystemError): A().x = 1 - @pytest.mark.xfail + @pytest.mark.xfail(raises=attr.exceptions.FrozenAttributeError) def test_slotted_confused(self): """ If we have a in-between non-attrs class, setattr reset detection From 5644d211f13c163259a62ec0ee7f2022838354d6 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Wed, 2 Sep 2020 08:20:55 +0200 Subject: [PATCH 06/11] Clarify what need to be reset and when Signed-off-by: Hynek Schlawack --- src/attr/_make.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 1d0ee375f..f77b5bc8f 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -669,8 +669,9 @@ def _create_slots_class(self): if k not in tuple(self._attr_names) + ("__dict__", "__weakref__") } - # Check the bases if one of them has an attrs-made __setattr__ that - # needs to be reset. + # If our class doesn't have an own __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. # 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 From f144cfa9a880ff6be576e2361c34ae8cb8e9614a Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Thu, 3 Sep 2020 08:19:48 +0200 Subject: [PATCH 07/11] Reset __attrs_own_setattr__ along with __setattr__ Signed-off-by: Hynek Schlawack --- src/attr/_make.py | 9 ++++++--- tests/test_setattr.py | 3 +++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index f77b5bc8f..edb1b5d33 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -655,6 +655,7 @@ def _patch_original_class(self): cls, "__attrs_own_setattr__", False ): cls.__setattr__ = object.__setattr__ + cls.__attrs_own_setattr__ = False return cls @@ -669,9 +670,10 @@ def _create_slots_class(self): if k not in tuple(self._attr_names) + ("__dict__", "__weakref__") } - # If our class doesn't have an own __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. + # 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 @@ -682,6 +684,7 @@ def _create_slots_class(self): for base_cls in getattr(self._cls, "__bases__", ()): if base_cls.__dict__.get("__attrs_own_setattr__", False): cd["__setattr__"] = object.__setattr__ + cd["__attrs_own_setattr__"] = False break # Traverse the MRO to check for an existing __weakref__. diff --git a/tests/test_setattr.py b/tests/test_setattr.py index 9db72c952..02867135a 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): From aea0a7ba8be301973a7f32ceb4452a20aefe3125 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Fri, 4 Sep 2020 05:45:54 +0200 Subject: [PATCH 08/11] Update src/attr/_make.py Co-authored-by: Paul Ganssle --- src/attr/_make.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index edb1b5d33..5edcc0d54 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -676,7 +676,7 @@ def _create_slots_class(self): # 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: class. See `test_slotted_confused` for details. For now that's # XXX: OK with us. if not self._has_own_setattr: # There's metaclass magic that may result in a baseclass without From c5b690ff93904bfdc716e656d84bd7007faf32ee Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Fri, 4 Sep 2020 08:27:10 +0200 Subject: [PATCH 09/11] Differentiate between own (= attrs) and custom (= user) __setattrs__ Signed-off-by: Hynek Schlawack --- src/attr/_make.py | 35 ++++++++++++++++---------------- tests/test_setattr.py | 46 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 17 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 5edcc0d54..7e8ace39f 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,9 +656,11 @@ 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 def _create_slots_class(self): @@ -679,13 +683,16 @@ def _create_slots_class(self): # XXX: class. See `test_slotted_confused` for details. For now that's # XXX: OK with us. if not self._has_own_setattr: - # There's metaclass magic that may result in a baseclass without - # __bases__ which results in _us_ not having one. cf. #681 - for base_cls in getattr(self._cls, "__bases__", ()): - if base_cls.__dict__.get("__attrs_own_setattr__", False): - cd["__setattr__"] = object.__setattr__ - cd["__attrs_own_setattr__"] = False - break + cd["__attrs_own_setattr__"] = False + + if not self._has_custom_setattr: + # There's metaclass magic that may result in a baseclassa + # without __bases__ which results in _us_ not having one. cf. + # #681 + for base_cls in getattr(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 @@ -864,20 +871,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 02867135a..34d9ad723 100644 --- a/tests/test_setattr.py +++ b/tests/test_setattr.py @@ -340,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]) @@ -387,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__ + ) From 32597f24bbcef46711d3c7469e0f47d3039d9d05 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Fri, 4 Sep 2020 08:29:24 +0200 Subject: [PATCH 10/11] We've always used __bases__ in our call to type() So no reason for being defensive. --- src/attr/_make.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/attr/_make.py b/src/attr/_make.py index 7e8ace39f..0fbbd7c16 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -686,10 +686,7 @@ def _create_slots_class(self): cd["__attrs_own_setattr__"] = False if not self._has_custom_setattr: - # There's metaclass magic that may result in a baseclassa - # without __bases__ which results in _us_ not having one. cf. - # #681 - for base_cls in getattr(self._cls, "__bases__", ()): + for base_cls in self._cls.__bases__: if base_cls.__dict__.get("__attrs_own_setattr__", False): cd["__setattr__"] = object.__setattr__ break From 0c0c8c71a7be64772b2a5ebb403548a0ec363c26 Mon Sep 17 00:00:00 2001 From: Hynek Schlawack Date: Sat, 5 Sep 2020 09:40:27 +0200 Subject: [PATCH 11/11] Update tests/test_setattr.py Co-authored-by: Paul Ganssle --- tests/test_setattr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_setattr.py b/tests/test_setattr.py index 34d9ad723..8e55da2d1 100644 --- a/tests/test_setattr.py +++ b/tests/test_setattr.py @@ -400,7 +400,7 @@ def test_setattr_inherited_do_not_reset_intermediate( 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__. + attrs can know that there is a user-provided __setattr__. """ @attr.s(slots=a_slots)