brentyi / tyro

CLI interfaces & config objects, from types
https://brentyi.github.io/tyro
MIT License
467 stars 23 forks source link

Incorrect warning message when using field with default #127

Closed idc9 closed 6 months ago

idc9 commented 6 months ago

Tyro gives a warning for setting mutable dataclass defaults when you use field with default (incorrect behavior). It correctly does not give a warning when you use field with default_factory.

Here is a simple script

from dataclasses import dataclass, field
import tyro

@dataclass
class A:
    x: int = 1

@dataclass
class B:
    a_default: A = field(default=A())
    a_factory: A = field(default_factory=A)

if __name__ == '__main__':
    tyro.cli(B)

And it outputs the following warning (notice the warning is for a_default and there is no warning for a_factory.

/Users/iain/anaconda3/envs/cpath/lib/python3.8/site-packages/tyro/_fields.py:1031: UserWarning: Mutable type <class '__main__.A'> is used as a default value for `a_default`. This is dangerous! Consider using `dataclasses.field(default_factory=...)` or marking <class '__main__.A'> as frozen.
  warnings.warn(

I'm using tyro version 0.7.2 and python version 3.8.18

brentyi commented 6 months ago

Hi, appreciate the succinct example script!

To me this looks like it's working as designed: the default value of a_default, A(), is mutable. This is unsafe.

There's a reasonable question about whether safety here is any of tyro's business, but either way I'd recommend either marking your dataclass as frozen or using default_factory. Note that your example will throw an error starting in Python 3.11:

Traceback (most recent call last):
  File "/Users/brentyi/Documents/code/idc9.py", line 8, in <module>
    @dataclass
     ^^^^^^^^^
  File "/Users/brentyi/miniconda3/envs/py311/lib/python3.11/dataclasses.py", line 1223, in dataclass
    return wrap(cls)
           ^^^^^^^^^
  File "/Users/brentyi/miniconda3/envs/py311/lib/python3.11/dataclasses.py", line 1213, in wrap
    return _process_class(cls, init, repr, eq, order, unsafe_hash,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/brentyi/miniconda3/envs/py311/lib/python3.11/dataclasses.py", line 958, in _process_class
    cls_fields.append(_get_field(cls, name, type, kw_only))
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/brentyi/miniconda3/envs/py311/lib/python3.11/dataclasses.py", line 815, in _get_field
    raise ValueError(f'mutable default {type(f.default)} for field '
ValueError: mutable default <class '__main__.A'> for field a_default is not allowed: use default_factory
idc9 commented 6 months ago

Ah ok I misunderstood how the default argument works with field... I agree with how tyro works in this case. Tyro 1, Iain 0.