Repository: python-attrs/attrs (python)

Issue:
Warning in docs about frozen classes no longer applicable
If you check the docs on frozen classes (https://www.attrs.org/en/stable/how-does-it-work.html#immutability), you'll note it says "You should avoid instantiating lots of frozen slotted classes [...] in performance-critical code."

However... with `attr.define`/`attr.frozen` both take the same amount of time.

```py
In [1]: import attr

In [2]: @attr.frozen
   ...: class SlottedFrozen:
   ...:     foo: int
   ...:     bar: int
   ...:     baz: str
   ...:     bing: float = 5.0
   ...: 

In [3]: @attr.define
   ...: class SlottedUnfrozen:
   ...:     foo: int
   ...:     bar: int
   ...:     baz: str
   ...:     bing: float = 5.0
   ...: 

In [4]: @attr.s(slots=True, frozen=True, auto_attribs=True)
   ...: class OldSlottedFrozen:
   ...:     foo: int
   ...:     bar: int
   ...:     baz: str
   ...:     bing: float = 5.0
   ...: 

In [5]: @attr.s(slots=True, auto_attribs=True)
   ...: class OldSlottedUnfrozen:
   ...:     foo: int
   ...:     bar: int
   ...:     baz: str
   ...:     bing: float = 5.0
   ...: 

In [6]: %timeit SlottedFrozen(7, 42, "hi!")
553 ns ± 10.6 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [7]: %timeit SlottedUnfrozen(7, 42, "hi!")
566 ns ± 17.2 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [8]: %timeit OldSlottedFrozen(7, 42, "hi!")
553 ns ± 8.64 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [9]: %timeit OldSlottedUnfrozen(7, 42, "hi!")
296 ns ± 3.09 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
```

What's up? Well, turns out the new API is setting `on_setattr=attr.setters.validate`. This turns the `__init__` from this: (old)
```py
In [14]: print(inspect.getsource(OldSlottedUnfrozen.__init__))
def __init__(self, foo, bar, baz, bing=attr_dict['bing'].default):
    self.foo = foo
    self.bar = bar
    self.baz = baz
    self.bing = bing
```

To this: (new)
```py
In [15]: print(inspect.getsource(SlottedUnfrozen.__init__))
def __init__(self, foo, bar, baz, bing=attr_dict['bing'].default):
    _setattr = _cached_setattr.__get__(self, self.__class__)
    _setattr('foo', foo)
    _setattr('bar', bar)
    _setattr('baz', baz)
    _setattr('bing', bing)
```

Possible solutions:
 - Mark that the note no longer applies
 - Make a fast path -- only set `on_setattr=attr.setters.validate` in new API if there is a validator.

Pull request:
Optimize the case of on_setattr=validate & no validators
This is important because define/mutable have on_setattr=setters.validate on
default.

Fixes #816

Patch:
diff --git a/changelog.d/817.change.rst b/changelog.d/817.change.rst
new file mode 100644
index 0000000..3a53efc
--- /dev/null
+++ b/changelog.d/817.change.rst
@@ -0,0 +1,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 that it's not set.
diff --git a/src/attr/_make.py b/src/attr/_make.py
index ceb08f9..f14369a 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 @@ class _ClassBuilder(object):
         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 @@ class _ClassBuilder(object):
             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 @@ class _ClassBuilder(object):
 
         # 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 @@ class _ClassBuilder(object):
         # 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 @@ class _ClassBuilder(object):
                 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 @@ class _ClassBuilder(object):
                 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 @@ class _ClassBuilder(object):
 
         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 _attrs_to_init_script(
 
         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("_")
 


Test patch:
diff --git a/tests/test_dunders.py b/tests/test_dunders.py
index 70f26d0..ba8f3ce 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,
+        cls_on_setattr=None,
         attrs_init=False,
     )
     return cls
diff --git a/tests/test_functional.py b/tests/test_functional.py
index a17023f..e9aa3c2 100644
--- a/tests/test_functional.py
+++ b/tests/test_functional.py
@@ -4,6 +4,7 @@ End-to-end tests.
 
 from __future__ import absolute_import, division, print_function
 
+import inspect
 import pickle
 
 from copy import deepcopy
@@ -687,3 +688,48 @@ class TestFunctional(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__