field checks done too early causing error

jamesmurphy-mc commented 1 year ago

Attrs does some checks on fields like no defaulted fields coming before non-defaulted fields before running custom field transformers. This is a problem when a field transformer intends to modify the fields but gets an error before being able to apply their modification. For example, this field transformer reorders fields so that in fact the defaulted field x comes after the non-defaulted field y. But attrs errors before the transformer is ever called.

def order_by_metadata(cls, fields):
    fields.sort(key=lambda x: x.metadata["field_order"])
    return fields

@define(field_transformer=order_by_metadata)
class A:
    x: int = field(metadata={"field_order": 1}, default=0)
    y: int = field(metadata={"field_order": 0})

Traceback (most recent call last):
  File "...", line 426, in <module>
    class A:
  File ".../attr/_next_gen.py", line 146, in wrap
    return do_it(cls, True)
  File ".../attr/_next_gen.py", line 90, in do_it
    return attrs(
  File ".../attr/_make.py", line 1603, in attrs
    return wrap(maybe_cls)
  File ".../attr/_make.py", line 1499, in wrap
    builder = _ClassBuilder(
  File ".../attr/_make.py", line 657, in __init__
    attrs, base_attrs, base_map = _transform_attrs(
  File ".../attr/_make.py", line 566, in _transform_attrs
    raise ValueError(
ValueError: No mandatory attributes allowed after an attribute with a default value or factory.  Attribute in question: Attribute(name='y', default=NOTHING, validator=None, repr=True, eq=True, eq_key=None, order=True, order_key=None, hash=None, init=True, metadata=mappingproxy({'field_order': 0}), type='int', converter=None, kw_only=False, inherited=False, on_setattr=None, alias=None)

I think these checks should be done after custom field transformers.

mara004 commented 3 months ago

hynek commented 3 months ago

@sscherfke transformers are your labor of love – is this realistic without adding a whole abstraction layer over this?

mara004 commented 2 months ago

is this realistic without adding a whole abstraction layer over this?

Not sure what you mean -- why isn't it sufficient to just move the order check after the field transformer has been applied?

IIUC, field transformers can already do arbitrary rearranging, as long as the initial field order is valid. But all that matters should be the final order, not the initial one, right? Conversely, a field transformer that results in an invalid order would currently slip past the check.

That said, why do we need the manual check in the first place? Python already gives us a pretty good exception when attempting to evaluate the incorrect code. Picking up the above example:

def order_by_metadata(cls, fields):
    fields.sort(key=lambda x: x.metadata["field_order"])
    return fields

@define(field_transformer=order_by_metadata)
class A:
    x: int = field(metadata={"field_order": 1})
    y: int = field(metadata={"field_order": 0}, default=0)

This yields the following error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/me/.local/lib/python3.11/site-packages/attr/_next_gen.py", line 144, in wrap
    return do_it(cls, True)
           ^^^^^^^^^^^^^^^^
  File "/home/me/.local/lib/python3.11/site-packages/attr/_next_gen.py", line 90, in do_it
    return attrs(
           ^^^^^^
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 1715, in attrs
    return wrap(maybe_cls)
           ^^^^^^^^^^^^^^^
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 1694, in wrap
    builder.add_init()
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 1090, in add_init
    _make_init(
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 2181, in _make_init
    init = _make_method(
           ^^^^^^^^^^^^^
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 345, in _make_method
    _compile_and_eval(script, globs, locs, filename)
  File "/home/me/.local/lib/python3.11/site-packages/attr/_make.py", line 317, in _compile_and_eval
    bytecode = compile(script, filename, "exec")
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<attrs generated init __main__.A>", line 1
    def __init__(self, y=attr_dict['y'].default, x):
                                                 ^
SyntaxError: non-default argument follows default argument

python-attrs / attrs

field checks done too early causing error #1147