NG: convert on setattr by default (#886)

* NG: convert on setattr by default

Not doing that from the get-go was an oversight.

Fixes #835

* Add optimization for default on_setattr w/ no work to do

Otherwise we'd end up with an explicit setattr every time.

* Fix optimization for NG default & j/ convert

* NG is actually 3.6+

* Add test for convert optimization for good measure

Author: hynek

changelog.d/835.breaking.rst

@@ -0,0 +1,4 @@
+When using ``@attr.define``, converters are now run by default when setting an attribute on an instance -- additionally to validators.
+I.e. the new default is ``on_setattr=[attr.setters.convert, attr.setters.validate]``.
+
+This is unfortunately a breaking change, but it was an oversight, impossible to raise a ``DeprecationWarning`` about, and it's better to fix it now while the APIs are very fresh with few users.

changelog.d/886.breaking.rst

@@ -0,0 +1,4 @@
+When using ``@attr.define``, converters are now run by default when setting an attribute on an instance -- additionally to validators.
+I.e. the new default is ``on_setattr=[attr.setters.convert, attr.setters.validate]``.
+
+This is unfortunately a breaking change, but it was an oversight, impossible to raise a ``DeprecationWarning`` about, and it's better to fix it now while the APIs are very fresh with few users.

docs/api.rst

@@ -707,13 +707,15 @@ The most notable differences are:
 - *auto_exc=True*
 - *auto_detect=True*
 - *eq=True*, but *order=False*
-- Validators run when you set an attribute (*on_setattr=attr.setters.validate*).
+- Converters and validators are run when you set an attribute (*on_setattr=[attr.setters.convert, attr.setters.validate*]).
 - Some options that aren't relevant to Python 3 have been dropped.
 
 Please note that these are *defaults* and you're free to override them, just like before.
 
 Since the Python ecosystem has settled on the term ``field`` for defining attributes, we have also added `attr.field` as a substitute for `attr.ib`.
 
+.. versionchanged:: 21.3.0 Converters are also run ``on_setattr``.
+
 .. note::
 
    `attr.s` and `attr.ib` (and their serious business cousins) aren't going anywhere.

src/attr/_make.py

@@ -59,6 +59,8 @@
 # Unique object for unequivocal getattr() defaults.
 _sentinel = object()
 
+_ng_default_on_setattr = setters.pipe(setters.convert, setters.validate)
+
 
 class _Nothing(object):
     """
@@ -722,13 +724,31 @@ def __init__(
             self._cls_dict["__delattr__"] = _frozen_delattrs
 
             self._wrote_own_setattr = True
-        elif on_setattr == setters.validate:
+        elif on_setattr in (
+            _ng_default_on_setattr,
+            setters.validate,
+            setters.convert,
+        ):
+            has_validator = has_converter = False
             for a in attrs:
                 if a.validator is not None:
+                    has_validator = True
+                if a.converter is not None:
+                    has_converter = True
+
+                if has_validator and has_converter:
                     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.
+            if (
+                (
+                    on_setattr == _ng_default_on_setattr
+                    and not (has_validator or has_converter)
+                )
+                or (on_setattr == setters.validate and not has_validator)
+                or (on_setattr == setters.convert and not has_converter)
+            ):
+                # If class-level on_setattr is set to convert + validate, but
+                # there's no field to convert or validate, pretend like there's
+                # no on_setattr.
                 self._on_setattr = None
 
         if getstate_setstate:
@@ -2123,9 +2143,7 @@ def _make_init(
                 raise ValueError("Frozen classes can't use on_setattr.")
 
             needs_cached_setattr = True
-        elif (
-            has_cls_on_setattr and a.on_setattr is not setters.NO_OP
-        ) or _is_slot_attr(a.name, base_attr_map):
+        elif has_cls_on_setattr and a.on_setattr is not setters.NO_OP:
             needs_cached_setattr = True
 
     unique_filename = _generate_unique_filename(cls, "init")

src/attr/_next_gen.py

@@ -8,7 +8,13 @@
 from attr.exceptions import UnannotatedAttributeError
 
 from . import setters
-from ._make import NOTHING, _frozen_setattrs, attrib, attrs
+from ._make import (
+    NOTHING,
+    _frozen_setattrs,
+    _ng_default_on_setattr,
+    attrib,
+    attrs,
+)
 
 
 def define(
@@ -35,8 +41,10 @@ def define(
     match_args=True,
 ):
     r"""
-    The only behavioral differences are the handling of the *auto_attribs*
-    option:
+    Define an ``attrs`` class.
+
+    The behavioral differences to `attr.s` are the handling of the
+    *auto_attribs* option:
 
     :param Optional[bool] auto_attribs: If set to `True` or `False`, it behaves
        exactly like `attr.s`. If left `None`, `attr.s` will try to guess:
@@ -46,9 +54,11 @@ def define(
        2. Otherwise it assumes *auto_attribs=False* and tries to collect
           `attr.ib`\ s.
 
-    and that mutable classes (``frozen=False``) validate on ``__setattr__``.
+    and that mutable classes (``frozen=False``) convert and validate on
+    ``__setattr__``.
 
     .. versionadded:: 20.1.0
+    .. versionchanged:: 21.3.0 Converters are also run ``on_setattr``.
     """
 
     def do_it(cls, auto_attribs):
@@ -86,9 +96,9 @@ def wrap(cls):
 
         had_on_setattr = on_setattr not in (None, setters.NO_OP)
 
-        # By default, mutable classes validate on setattr.
+        # By default, mutable classes convert & validate on setattr.
         if frozen is False and on_setattr is None:
-            on_setattr = setters.validate
+            on_setattr = _ng_default_on_setattr
 
         # However, if we subclass a frozen class, we inherit the immutability
         # and disable on_setattr.

tests/test_functional.py

@@ -17,7 +17,7 @@
 
 import attr
 
-from attr._compat import PY2, TYPE
+from attr._compat import PY2, PY36, TYPE
 from attr._make import NOTHING, Attribute
 from attr.exceptions import FrozenInstanceError
 
@@ -692,8 +692,9 @@ class C(object):
     @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__.
+        If a class has on_setattr=attr.setters.validate (former default in NG
+        APIs) but sets no validators, don't use the (slower) setattr in
+        __init__.
 
         Regression test for #816.
         """
@@ -713,6 +714,58 @@ class D(C):
         assert "self.y = y" in src
         assert object.__setattr__ == D.__setattr__
 
+    @pytest.mark.parametrize("slots", [True, False])
+    def test_no_setattr_if_convert_without_converters(self, slots):
+        """
+        If a class has on_setattr=attr.setters.convert but sets no validators,
+        don't use the (slower) setattr in __init__.
+        """
+
+        @attr.s(on_setattr=attr.setters.convert)
+        class C(object):
+            x = attr.ib()
+
+        @attr.s(on_setattr=attr.setters.convert)
+        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__
+
+    @pytest.mark.skipif(not PY36, reason="NG APIs are 3.6+")
+    @pytest.mark.parametrize("slots", [True, False])
+    def test_no_setattr_with_ng_defaults(self, slots):
+        """
+        If a class has the NG default on_setattr=[convert, validate] but sets
+        no validators or converters, don't use the (slower) setattr in
+        __init__.
+        """
+
+        @attr.define
+        class C(object):
+            x = attr.ib()
+
+        src = inspect.getsource(C.__init__)
+
+        assert "setattr" not in src
+        assert "self.x = x" in src
+        assert object.__setattr__ == C.__setattr__
+
+        @attr.define
+        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

tests/test_next_gen.py

@@ -308,3 +308,28 @@ class MyException(Exception):
 
         assert "foo" == ei.value.x
         assert ei.value.__cause__ is None
+
+    def test_converts_and_validates_by_default(self):
+        """
+        If no on_setattr is set, assume setters.convert, setters.validate.
+        """
+
+        @attr.define
+        class C:
+            x: int = attr.field(converter=int)
+
+            @x.validator
+            def _v(self, _, value):
+                if value < 10:
+                    raise ValueError("must be >=10")
+
+        inst = C(10)
+
+        # Converts
+        inst.x = "11"
+
+        assert 11 == inst.x
+
+        # Validates
+        with pytest.raises(ValueError, match="must be >=10"):
+            inst.x = "9"