Repository: python-attrs/attrs
Pull request #1401 (2025-01-31 23:19:58)

Perform attr order checks after field transformer.
Fixes: https://github.com/python-attrs/attrs/issues/1147

# Summary

Previously, attrs performed order checks for mandatory vs non-mandatory attributes before applying the field transformer, giving errors even when the field transformer intended to fix such errors such as by supplying defaults, factories, making args keyword-only, removing attrs, or reordering attrs.

This PR moves the checks after the field transformer.


# Pull Request Check List

<!--
This is just a friendly reminder about the most common mistakes.
Please make sure that you tick all boxes.
But please read our [contribution guide](https://github.com/python-attrs/attrs/blob/main/.github/CONTRIBUTING.md) at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, **check it anyway** to make it apparent that there's nothing left to do.
If your pull request is a documentation fix or a trivial typo, feel free to delete the whole thing.
-->

- [x] Do **not** open pull requests from your `main` branch – **use a separate branch**!
  - There's a ton of footguns waiting if you don't heed this warning. You can still go back to your project, create a branch from your main branch, push it, and open the pull request from the new branch.
  - This is not a pre-requisite for your pull request to be accepted, but **you have been warned**.
- [x] Added **tests** for changed code.
  Our CI fails if coverage is not 100%.
- [x] New features have been added to our [Hypothesis testing strategy](https://github.com/python-attrs/attrs/blob/main/tests/strategies.py).
- [x] Changes or additions to public APIs are reflected in our type stubs (files ending in ``.pyi``).
    - [x] ...and used in the stub test file `tests/typing_example.py`.
    - [x] If they've been added to `attr/__init__.pyi`, they've *also* been re-imported in `attrs/__init__.pyi`.
- [x] Updated **documentation** for changed code.
    - [x] New functions/classes have to be added to `docs/api.rst` by hand.
    - [x] Changes to the signatures of `@attr.s()` and `@attrs.define()` have to be added by hand too.
    - [x] Changed/added classes/methods/functions have appropriate `versionadded`, `versionchanged`, or `deprecated` [directives](http://www.sphinx-doc.org/en/stable/markup/para.html#directive-versionadded).
          The next version is the second number in the current release + 1.
          The first number represents the current year.
          So if the current version on PyPI is 22.2.0, the next version is gonna be 22.3.0.
          If the next version is the first in the new year, it'll be 23.1.0.
      - [x] If something changed that affects both `attrs.define()` and `attr.s()`, you have to add version directives to both.
- [x] Documentation in `.rst` and `.md` files is written using [semantic newlines](https://rhodesmill.org/brandon/2012/one-sentence-per-line/).
- [x] Changes (and possible deprecations) have news fragments in [`changelog.d`](https://github.com/python-attrs/attrs/blob/main/changelog.d).
- [x] Consider granting [push permissions to the PR branch](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork), so maintainers can fix minor issues themselves without pestering you.

<!--
If you have *any* questions to *any* of the points above, just **submit and ask**!
This checklist is here to *help* you, not to deter you from contributing!
-->



Patch:
diff --git a/changelog.d/1147.change.md b/changelog.d/1147.change.md
new file mode 100644
index 0000000..17c6263
--- /dev/null
+++ b/changelog.d/1147.change.md
@@ -0,0 +1,1 @@
+Checking mandatory vs non-mandatory attribute order is now performed after the field transformer, since the field transformer may change attributes and/or their order.
diff --git a/src/attr/_make.py b/src/attr/_make.py
index a7a4075..b1550eb 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -449,6 +449,10 @@ def _transform_attrs(
 
     attrs = base_attrs + own_attrs
 
+    if field_transformer is not None:
+        attrs = field_transformer(cls, attrs)
+
+    # Check attr order after executing the field_transformer.
     # Mandatory vs non-mandatory attr order only matters when they are part of
     # the __init__ signature and when they aren't kw_only (which are moved to
     # the end and can be mandatory or non-mandatory in any order, as they will
@@ -462,9 +466,6 @@ def _transform_attrs(
         if had_default is False and a.default is not NOTHING:
             had_default = True
 
-    if field_transformer is not None:
-        attrs = field_transformer(cls, attrs)
-
     # Resolve default field alias after executing field_transformer.
     # This allows field_transformer to differentiate between explicit vs
     # default aliases and supply their own defaults.


Test patch:
diff --git a/tests/test_hooks.py b/tests/test_hooks.py
index 9c37a98..6b34206 100644
--- a/tests/test_hooks.py
+++ b/tests/test_hooks.py
@@ -4,6 +4,8 @@ from __future__ import annotations
 
 from datetime import datetime
 
+import pytest
+
 import attr
 
 
@@ -30,7 +32,7 @@ class TestTransformHook:
             y = attr.ib(type=int)
             z: float = attr.ib()
 
-        assert results == [("x", None), ("y", int), ("z", float)]
+        assert [("x", None), ("y", int), ("z", float)] == results
 
     def test_hook_applied_auto_attrib(self):
         """
@@ -49,7 +51,7 @@ class TestTransformHook:
             x: int
             y: str = attr.ib()
 
-        assert results == [("x", int), ("y", str)]
+        assert [("x", int), ("y", str)] == results
 
     def test_hook_applied_modify_attrib(self):
         """
@@ -66,7 +68,8 @@ class TestTransformHook:
             y: float
 
         c = C(x="3", y="3.14")
-        assert c == C(x=3, y=3.14)
+
+        assert C(x=3, y=3.14) == c
 
     def test_hook_remove_field(self):
         """
@@ -82,7 +85,7 @@ class TestTransformHook:
             x: int
             y: float
 
-        assert attr.asdict(C(2.7)) == {"y": 2.7}
+        assert {"y": 2.7} == attr.asdict(C(2.7))
 
     def test_hook_add_field(self):
         """
@@ -98,7 +101,7 @@ class TestTransformHook:
         class C:
             x: int
 
-        assert attr.asdict(C(1, 2)) == {"x": 1, "new": 2}
+        assert {"x": 1, "new": 2} == attr.asdict(C(1, 2))
 
     def test_hook_override_alias(self):
         """
@@ -118,13 +121,73 @@ class TestTransformHook:
             1, 2, 3
         )
 
+    def test_hook_reorder_fields(self):
+        """
+        It is possible to reorder fields via the hook.
+        """
+
+        def hook(cls, attribs):
+            return sorted(attribs, key=lambda x: x.metadata["field_order"])
+
+        @attr.s(field_transformer=hook)
+        class C:
+            x: int = attr.ib(metadata={"field_order": 1})
+            y: int = attr.ib(metadata={"field_order": 0})
+
+        assert {"x": 0, "y": 1} == attr.asdict(C(1, 0))
+
+    def test_hook_reorder_fields_before_order_check(self):
+        """
+        It is possible to reorder fields via the hook before order-based errors are raised.
+
+        Regression test for #1147.
+        """
+
+        def hook(cls, attribs):
+            return sorted(attribs, key=lambda x: x.metadata["field_order"])
+
+        @attr.s(field_transformer=hook)
+        class C:
+            x: int = attr.ib(metadata={"field_order": 1}, default=0)
+            y: int = attr.ib(metadata={"field_order": 0})
+
+        assert {"x": 0, "y": 1} == attr.asdict(C(1))
+
+    def test_hook_conflicting_defaults_after_reorder(self):
+        """
+        Raises `ValueError` if attributes with defaults are followed by
+        mandatory attributes after the hook reorders fields.
+
+        Regression test for #1147.
+        """
+
+        def hook(cls, attribs):
+            return sorted(attribs, key=lambda x: x.metadata["field_order"])
+
+        with pytest.raises(ValueError) as e:
+
+            @attr.s(field_transformer=hook)
+            class C:
+                x: int = attr.ib(metadata={"field_order": 1})
+                y: int = attr.ib(metadata={"field_order": 0}, default=0)
+
+        assert (
+            "No mandatory attributes allowed after an attribute with a "
+            "default value or factory.  Attribute in question: Attribute"
+            "(name='x', default=NOTHING, validator=None, repr=True, "
+            "eq=True, eq_key=None, order=True, order_key=None, "
+            "hash=None, init=True, "
+            "metadata=mappingproxy({'field_order': 1}), type='int', converter=None, "
+            "kw_only=False, inherited=False, on_setattr=None, alias=None)",
+        ) == e.value.args
+
     def test_hook_with_inheritance(self):
         """
         The hook receives all fields from base classes.
         """
 
         def hook(cls, attribs):
-            assert [a.name for a in attribs] == ["x", "y"]
+            assert ["x", "y"] == [a.name for a in attribs]
             # Remove Base' "x"
             return attribs[1:]
 
@@ -136,7 +199,7 @@ class TestTransformHook:
         class Sub(Base):
             y: int
 
-        assert attr.asdict(Sub(2)) == {"y": 2}
+        assert {"y": 2} == attr.asdict(Sub(2))
 
     def test_attrs_attrclass(self):
         """
@@ -151,7 +214,7 @@ class TestTransformHook:
             x: int
 
         fields_type = type(attr.fields(C))
-        assert fields_type.__name__ == "CAttributes"
+        assert "CAttributes" == fields_type.__name__
         assert issubclass(fields_type, tuple)
 
 
@@ -187,12 +250,12 @@ class TestAsDictHook:
         )
 
         result = attr.asdict(inst, value_serializer=hook)
-        assert result == {
+        assert {
             "a": {"x": 1, "y": ["2020-07-01T00:00:00"]},
             "b": [{"x": 2, "y": ["2020-07-02T00:00:00"]}],
             "c": {"spam": {"x": 3, "y": ["2020-07-03T00:00:00"]}},
             "d": {"eggs": "2020-07-04T00:00:00"},
-        }
+        } == result
 
     def test_asdict_calls(self):
         """
@@ -217,7 +280,7 @@ class TestAsDictHook:
         inst = Parent(a=Child(1), b=[Child(2)], c={"spam": Child(3)})
 
         attr.asdict(inst, value_serializer=hook)
-        assert calls == [
+        assert [
             (inst, "a", inst.a),
             (inst.a, "x", inst.a.x),
             (inst, "b", inst.b),
@@ -225,4 +288,4 @@ class TestAsDictHook:
             (inst, "c", inst.c),
             (None, None, "spam"),
             (inst.c["spam"], "x", inst.c["spam"].x),
-        ]
+        ] == calls