diff --git a/changelog.d/817.change.rst b/changelog.d/817.change.rst new file mode 100644 index 000000000..48340b586 --- /dev/null +++ b/changelog.d/817.change.rst @@ -0,0 +1 @@ +If the class-level *on_setattr* is set to ``attr.setters.validate`` (default in ``@attr.define`` and ``@attr.mutable``) but no field defines a validator, pretend like it's not set. diff --git a/src/attr/_make.py b/src/attr/_make.py index ceb08f910..f14369a99 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -654,7 +654,7 @@ class _ClassBuilder(object): "_on_setattr", "_slots", "_weakref_slot", - "_has_own_setattr", + "_wrote_own_setattr", "_has_custom_setattr", ) @@ -701,7 +701,7 @@ def __init__( self._on_setattr = on_setattr self._has_custom_setattr = has_custom_setattr - self._has_own_setattr = False + self._wrote_own_setattr = False self._cls_dict["__attrs_attrs__"] = self._attrs @@ -709,7 +709,15 @@ def __init__( self._cls_dict["__setattr__"] = _frozen_setattrs self._cls_dict["__delattr__"] = _frozen_delattrs - self._has_own_setattr = True + self._wrote_own_setattr = True + elif on_setattr == setters.validate: + for a in attrs: + if a.validator is not None: + break + else: + # If class-level on_setattr is set to validating, but there's + # no field to validate, pretend like there's no on_setattr. + self._on_setattr = None if getstate_setstate: ( @@ -759,7 +767,7 @@ def _patch_original_class(self): # 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( + if not self._wrote_own_setattr and getattr( cls, "__attrs_own_setattr__", False ): cls.__attrs_own_setattr__ = False @@ -787,7 +795,7 @@ 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. - if not self._has_own_setattr: + if not self._wrote_own_setattr: cd["__attrs_own_setattr__"] = False if not self._has_custom_setattr: @@ -958,8 +966,7 @@ def add_init(self): self._cache_hash, self._base_attr_map, self._is_exc, - self._on_setattr is not None - and self._on_setattr is not setters.NO_OP, + self._on_setattr, attrs_init=False, ) ) @@ -978,8 +985,7 @@ def add_attrs_init(self): self._cache_hash, self._base_attr_map, self._is_exc, - self._on_setattr is not None - and self._on_setattr is not setters.NO_OP, + self._on_setattr, attrs_init=True, ) ) @@ -1038,7 +1044,7 @@ def __setattr__(self, name, val): self._cls_dict["__attrs_own_setattr__"] = True self._cls_dict["__setattr__"] = self._add_method_dunders(__setattr__) - self._has_own_setattr = True + self._wrote_own_setattr = True return self @@ -2008,10 +2014,14 @@ def _make_init( cache_hash, base_attr_map, is_exc, - has_global_on_setattr, + cls_on_setattr, attrs_init, ): - if frozen and has_global_on_setattr: + has_cls_on_setattr = ( + cls_on_setattr is not None and cls_on_setattr is not setters.NO_OP + ) + + if frozen and has_cls_on_setattr: raise ValueError("Frozen classes can't use on_setattr.") needs_cached_setattr = cache_hash or frozen @@ -2030,7 +2040,7 @@ def _make_init( needs_cached_setattr = True elif ( - has_global_on_setattr and a.on_setattr is not setters.NO_OP + has_cls_on_setattr and a.on_setattr is not setters.NO_OP ) or _is_slot_attr(a.name, base_attr_map): needs_cached_setattr = True @@ -2046,7 +2056,7 @@ def _make_init( base_attr_map, is_exc, needs_cached_setattr, - has_global_on_setattr, + has_cls_on_setattr, attrs_init, ) if cls.__module__ in sys.modules: @@ -2183,7 +2193,7 @@ def _attrs_to_init_script( base_attr_map, is_exc, needs_cached_setattr, - has_global_on_setattr, + has_cls_on_setattr, attrs_init, ): """ @@ -2257,7 +2267,7 @@ def fmt_setter_with_converter( attr_name = a.name has_on_setattr = a.on_setattr is not None or ( - a.on_setattr is not setters.NO_OP and has_global_on_setattr + a.on_setattr is not setters.NO_OP and has_cls_on_setattr ) arg_name = a.name.lstrip("_") diff --git a/tests/test_dunders.py b/tests/test_dunders.py index 70f26d044..b12ea0597 100644 --- a/tests/test_dunders.py +++ b/tests/test_dunders.py @@ -94,7 +94,7 @@ def _add_init(cls, frozen): cache_hash=False, base_attr_map={}, is_exc=False, - has_global_on_setattr=False, + global_on_setattr=None, attrs_init=False, ) return cls diff --git a/tests/test_functional.py b/tests/test_functional.py index a17023ffc..e9aa3c267 100644 --- a/tests/test_functional.py +++ b/tests/test_functional.py @@ -4,6 +4,7 @@ from __future__ import absolute_import, division, print_function +import inspect import pickle from copy import deepcopy @@ -687,3 +688,48 @@ class C(object): "2021-06-01. Please use `eq` and `order` instead." == w.message.args[0] ) + + @pytest.mark.parametrize("slots", [True, False]) + def test_no_setattr_if_validate_without_validators(self, slots): + """ + If a class has on_setattr=attr.setters.validate (default in NG APIs) + but sets no validators, don't use the (slower) setattr in __init__. + + Regression test for #816. + """ + + @attr.s(on_setattr=attr.setters.validate) + class C(object): + x = attr.ib() + + @attr.s(on_setattr=attr.setters.validate) + class D(C): + y = attr.ib() + + src = inspect.getsource(D.__init__) + + assert "setattr" not in src + assert "self.x = x" in src + assert "self.y = y" in src + assert object.__setattr__ == D.__setattr__ + + def test_on_setattr_detect_inherited_validators(self): + """ + _make_init detects the presence of a validator even if the field is + inherited. + """ + + @attr.s(on_setattr=attr.setters.validate) + class C(object): + x = attr.ib(validator=42) + + @attr.s(on_setattr=attr.setters.validate) + class D(C): + y = attr.ib() + + src = inspect.getsource(D.__init__) + + assert "_setattr = _cached_setattr" in src + assert "_setattr('x', x)" in src + assert "_setattr('y', y)" in src + assert object.__setattr__ != D.__setattr__