[ty] synthesize __setattr__ for frozen dataclasses

[thejchap]

Summary

Synthesize a __setattr__ method with a return type of Never for frozen dataclasses.

https://docs.python.org/3/library/dataclasses.html#frozen-instances
https://docs.python.org/3/library/dataclasses.html#dataclasses.FrozenInstanceError

Related

astral-sh/ty#111
#17974 (comment)
#18347 (comment)

WIP

Test Plan

New Markdown tests

[github-actions]

mypy_primer results

Changes were detected when running on open source projects

attrs (https://github.com/python-attrs/attrs)
+ error[invalid-assignment] tests/test_next_gen.py:267:13: Property `c` defined in `C` is read-only
- Found 636 diagnostics
+ Found 637 diagnostics

No memory usage changes detected ✅

[thejchap]

@sharkdp i could use some help understanding your comment here

It returns an AttributeError if the attribute doesn't exist, and it returns a FrozenInstanceError if it's an existing attribute

did you mean to say raises instead returns? and if so, can you maybe expand a bit on your idea for differentiating based on the function body (which doesn't exist in the ast since it's synthesized), since as far as i understand it from this code a raise is considered to have a return type of Never, which would look the same regardless of the function body? apologies if im missing something obvious...

also as an aside, it looks like at runtime a FrozenInstanceError gets thrown regardless of if the attribute exists or not

from dataclasses import dataclass

@dataclass(frozen=True)
class Test:
    b: int

test = Test(1)
test.a = 2

Traceback (most recent call last):
  File "/Users/justinchapman/src/ruff/../test.py", line 8, in <module>
    test.a = 2  # This will raise an error because the dataclass is frozen
    ^^^^^^
  File "<string>", line 4, in __setattr__
dataclasses.FrozenInstanceError: cannot assign to field 'a'

[sharkdp]

It returns an AttributeError if the attribute doesn't exist, and it returns a FrozenInstanceError if it's an existing attribute

did you mean to say raises instead returns?

Yes

can you maybe expand a bit on your idea for differentiating based on the function body (which doesn't exist in the ast since it's synthesized), since as far as i understand it from this code a raise is considered to have a return type of Never, which would look the same regardless of the function body? apologies if im missing something obvious...

My idea was that we could distinguish between calling a user-defined __setattr__ method and the synthesized __setattr__ method that you implemented here. This maybe more difficult than I imagined, since we only (easily) have access to the return type here. One way that would work (but I don't think I like that idea) would be to "tag" the Never return type. Type::Never could internally track some metadata (like a bit field) that would allow us to track the "origin" of that type. User-defined methods would just return a "normal" Type::Never(NeverMetadata::default()), but the synthesized dataclass __setattr__ could return Type::Never(NeverMetadata::FROZEN_DATACLASS_SETATTR) or similar. The metadata wouldn't change the behavior of the Never type in any way. But here, when checking the return type of __setattr__, we could use that flag to distinguish the different origins of Never.

Another way to do this would be to look up __setattr__ as an attribute (in addition to calling try_call_dunder). Instead of getting some Type::FunctionLiteral, we should see the function-like Type::Callable that would be evidence for it being a synthesized __setattr__ method.

All of this would only be a way to provide a nicer error message. Something that mentions "frozen dataclass" instead of "custom __setattr__ with a return type of Never", which might be really confusing for users.

also as an aside, it looks like at runtime a FrozenInstanceError gets thrown regardless of if the attribute exists or not

Ah, interesting. I think I probably just tried accessing an unknown member, not setting it. I think it would also be fine to emit the invalid-assignment diagnostic here instead of unresolved-attribute. What's more important is the thing above.

[sharkdp]

Thank you very much for this. Look, this found a new true positive!

attrs (https://github.com/python-attrs/attrs)
+ error[invalid-assignment] tests/test_next_gen.py:267:13: Property `c` defined in `C` is read-only

I made a few minor updates to bring this into a mergeable state. Most importantly, we can not skip the whole check for the frozen flag, and can rely on __setattr__ alone. This leads to a big code improvement.

[thejchap]

@sharkdp ah, awesome - thanks! i had planned to work on it a bit more, but if its good with you its good with me :) i appreciate you getting it merged

is there anything else in astral-sh/ty#111 that you think would be good to work on next?

one thing this is still missing i believe is emitting the same error for __delattr__ (mypy doesn't do this, but at runtime you get the same dataclasses.FrozenInstanceError: cannot delete field 'a')

[sharkdp]

i had planned to work on it a bit more

I noticed that it was still in draft, but it seemed like a good and consistent contribution. You're certainly welcome to add something on top of this!

one thing this is still missing i believe is emitting the same error for __delattr__ (mypy doesn't do this, but at runtime you get the same dataclasses.FrozenInstanceError: cannot delete field 'a')

Adding that would be great. I noticed that you already added a test for it.

is there anything else in astral-sh/ty#111 that you think would be good to work on next?

Adding support for more of the synthesized methods/arguments would certainly be a good next step. I haven't looked closely at this, and maybe some of these might not require any special casing at all. The next thing I'd do would probably be to write a few more tests for these, even if they are initially still failing. That should give us an idea what to focus on next.

Support for dataclass fields is another interesting area, but we're already working on this.