[ty] Support frozen dataclasses #17974

thejchap · 2025-05-09T02:11:29Z

Summary

This PR adds support for frozen dataclasses. It will emit a diagnostic with a similar message to mypy

Note: This does not include emitting a diagnostic if __setattr__ or __delattr__ are defined on the object as per the spec

Test Plan

mdtest

thejchap · 2025-05-09T02:13:46Z

@sharkdp ready for some feedback - let me know if this is headed in the right direction

github-actions · 2025-05-09T02:16:20Z

`mypy_primer` results

Changes were detected when running on open source projects

attrs (https://github.com/python-attrs/attrs)
+ error[invalid-assignment] tests/dataclass_transform_example.py:32:1: Property `a` defined in `Frozen` is read-only
- error[invalid-assignment] tests/test_next_gen.py:219:13: Object of type `Literal[2]` is not assignable to attribute `x` of type `str`
+ error[invalid-assignment] tests/test_next_gen.py:219:13: Property `x` defined in `F` is read-only
+ error[invalid-assignment] tests/test_next_gen.py:258:13: Property `a` defined in `A` is read-only
+ error[invalid-assignment] tests/test_next_gen.py:261:13: Property `a` defined in `B` is read-only
+ error[invalid-assignment] tests/test_next_gen.py:264:13: Property `b` defined in `B` is read-only
- Found 617 diagnostics
+ Found 621 diagnostics

sympy (https://github.com/sympy/sympy)
- error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1683:13: Object of type `None` is not assignable to attribute `tendon_force_length` of type `CharacteristicCurveFunction`
+ error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1683:13: Property `tendon_force_length` defined in `CharacteristicCurveCollection` is read-only
- error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1685:13: Object of type `None` is not assignable to attribute `tendon_force_length_inverse` of type `CharacteristicCurveFunction`
- error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1687:13: Object of type `None` is not assignable to attribute `fiber_force_length_passive` of type `CharacteristicCurveFunction`
- error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1689:13: Object of type `None` is not assignable to attribute `fiber_force_length_passive_inverse` of type `CharacteristicCurveFunction`
- error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1691:13: Object of type `None` is not assignable to attribute `fiber_force_length_active` of type `CharacteristicCurveFunction`
+ error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1685:13: Property `tendon_force_length_inverse` defined in `CharacteristicCurveCollection` is read-only
+ error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1687:13: Property `fiber_force_length_passive` defined in `CharacteristicCurveCollection` is read-only
+ error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1689:13: Property `fiber_force_length_passive_inverse` defined in `CharacteristicCurveCollection` is read-only
+ error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1691:13: Property `fiber_force_length_active` defined in `CharacteristicCurveCollection` is read-only
- error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1693:13: Object of type `None` is not assignable to attribute `fiber_force_velocity` of type `CharacteristicCurveFunction`
+ error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1693:13: Property `fiber_force_velocity` defined in `CharacteristicCurveCollection` is read-only
- error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1695:13: Object of type `None` is not assignable to attribute `fiber_force_velocity_inverse` of type `CharacteristicCurveFunction`
+ error[invalid-assignment] sympy/physics/biomechanics/tests/test_curve.py:1695:13: Property `fiber_force_velocity_inverse` defined in `CharacteristicCurveCollection` is read-only

carljm

Code and ecosystem results look pretty good to me. A couple comments. Thanks for the PR!

crates/ty_python_semantic/src/types/infer.rs

crates/ty_python_semantic/resources/mdtest/dataclasses.md

carljm

Thanks for the updates! On looking at the new version more closely, I think we still need to adjust the priority of this error relative to other ones. Sorry I didn't comment on this more thoroughly the first time around! More detailed comment inline.

crates/ty_python_semantic/src/types/infer.rs

carljm · 2025-05-09T23:28:21Z

crates/ty_python_semantic/src/types/infer.rs

                        }
                    }
+                };
+                if (assignable_if_rw && ro) && emit_diagnostics {


The change to prioritize "unresolved-attribute" over the read-only error looks great -- but I think I have the reverse issue now. If the assigned type is not assignable to the attribute type, and the attribute is read-only, which of those do you think should take priority? I think it would actually be fine to emit both, but if we emit only one of those two errors in that scenario, I think it should be the read-only error. It doesn't make sense to send the user on a mission to fix the assigned type, when we'll only then tell them the attribute is read-only; we should tell them it's read-only right upfront. WDYT?

The read-only error should also take precedence over the descriptor __set__ handling, because that's what happens at runtime -- even if the attribute of a frozen dataclass is a descriptor, its __set__ method is not called, you just get the read-only error.

I think this will require integrating the read-only error into the block of cases above (I think it should be the first thing we check in the block starting on 2957 for "there is an attribute of this name") rather than trying to handle it externally. I think we can also move looking up whether this is a frozen dataclass into that arm, because I don't think we need it otherwise -- both the ClassVar error and the Unbound handling don't need to check for a frozen dataclass, I don't think?

It doesn't make sense to send the user on a mission

yes this definitely makes sense from a ux perspective, good call

if the attribute of a frozen dataclass is a descriptor, its set method is not called

also makes sense, thank you!

both the ClassVar error and the Unbound handling don't need to check for a frozen dataclass

makes sense ClassVar doesn't, hadn't thought that through enough - although i think the unbound handling might also need to check for a frozen dataclass. if it doesn't, the a.x = 2 assignment below won't emit a diagnostic because the x: int type declaration/meta attr is unbound for A:

from dataclasses import dataclass @dataclass(frozen=True) class A: x: int a = A(1) a.x = 2 # no diagnostic @dataclass(frozen=True) class B: x: int = 1 b = B() b.x = 2 # emits read-only diagnostic

this wasn't super intuitive to me - i would have thought from this comment that x: int would be considered bound in both cases, but i'm probably not fully grokking whats going on here

Sorry I didn't comment on this more thoroughly

no problem! i appreciate the guidance (and patience :)

Is that speculation or something you are seeing in your results? Because I also think (because we kind of conflate declaredness and boundness in a way we maybe shouldn't) that both A.x and B.x will actually report back Bound in that example. If that's not what you're seeing, I'd need to dig in further to understand it. It looks to me like the Unbound handling here is only for the cases where there is no such instance attribute at all.

that's what i'm seeing in results - added a failing test case to illustrate

in this test, if the 1 is moved out of the constructor and instead set as a default value for x, it hits the other branch (now starting line 2958) and emits the diagnostic as expected

i can do a little more digging next week as well

Sorry, this was my mistake; I'd failed to realize that the outer Unbound check was for looking up a class member. An x: int annotation at class level with no binding describes an instance-only attribute (in our model), so it comes back Unbound on the class. Which is why we then do an instance_member lookup. So I think the code you added (and then commented out) is necessary and correct. (I did make one small change to wrap up the dataclass work in a closure, so we only do that work if we need it.)

17675

Markdown formatting Co-authored-by: Alex Waygood <[email protected]>

carljm · 2025-05-22T04:20:45Z

Thank you for the PR! Merged.

crates/ty_python_semantic/src/types/infer.rs

#18347) ## Summary Related: - astral-sh/ty#111 - #17974 (comment) Previously, when validating an attribute assignment, a `__setattr__` call check was only done if the attribute wasn't found as either a class member or instance member This PR changes the `__setattr__` call check to be attempted first, prior to the "[normal mechanism](https://docs.python.org/3/reference/datamodel.html#object.__setattr__)", as a defined `__setattr__` should take precedence over setting an attribute on the instance dictionary directly. if the return type of `__setattr__` is `Never`, an `invalid-assignment` diagnostic is emitted Once this is merged, a subsequent PR will synthesize a `__setattr__` method with a `Never` return type for frozen dataclasses. ## Test Plan Existing tests + mypy_primer --------- Co-authored-by: David Peter <[email protected]>

## 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) ## Test Plan New Markdown tests --------- Co-authored-by: David Peter <[email protected]>

thejchap requested review from AlexWaygood, carljm, dcreager and sharkdp as code owners May 9, 2025 02:11

thejchap force-pushed the feature/dataclass-frozen branch from 0fe7255 to 6b64036 Compare May 9, 2025 02:12

thejchap force-pushed the feature/dataclass-frozen branch 4 times, most recently from b3947b3 to 3b1c145 Compare May 9, 2025 02:46

carljm reviewed May 9, 2025

View reviewed changes

crates/ty_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

crates/ty_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

AlexWaygood reviewed May 9, 2025

View reviewed changes

crates/ty_python_semantic/resources/mdtest/dataclasses.md Outdated Show resolved Hide resolved

crates/ty_python_semantic/resources/mdtest/dataclasses.md Outdated Show resolved Hide resolved

MichaReiser added the ty Multi-file analysis & type inference label May 9, 2025

thejchap force-pushed the feature/dataclass-frozen branch from 20a58fc to 09a0af5 Compare May 9, 2025 22:53

thejchap requested a review from carljm May 9, 2025 23:10

carljm reviewed May 9, 2025

View reviewed changes

thejchap force-pushed the feature/dataclass-frozen branch 3 times, most recently from 4892268 to 04355b9 Compare May 17, 2025 00:44

thejchap and others added 4 commits May 21, 2025 23:58

[ty] Support frozen dataclasses

dcf210d

17675

Apply suggestions from code review

c9d1aaf

Markdown formatting Co-authored-by: Alex Waygood <[email protected]>

[ty] Refactor frozen dataclass diagnostic

c38ec21

a few adjustments

c9ca199

carljm force-pushed the feature/dataclass-frozen branch from 04355b9 to c9ca199 Compare May 22, 2025 03:58

carljm merged commit 01eeb2f into astral-sh:main May 22, 2025
35 checks passed

sharkdp reviewed May 22, 2025

View reviewed changes

crates/ty_python_semantic/src/types/infer.rs Show resolved Hide resolved

carljm mentioned this pull request May 22, 2025

Advanced dataclass support astral-sh/ty#111

Open

19 tasks

thejchap mentioned this pull request May 28, 2025

[ty] Fix __setattr__ call check precedence during attribute assignment #18347

Merged

thejchap mentioned this pull request Jun 1, 2025

[ty] Synthesize __setattr__ and __delattr__ for frozen dataclasses thejchap/ruff#2

Closed

thejchap mentioned this pull request Jul 13, 2025

[ty] synthesize __setattr__ for frozen dataclasses #19307

Merged

[ty] Support frozen dataclasses #17974

[ty] Support frozen dataclasses #17974

Uh oh!

Conversation

thejchap commented May 9, 2025

Summary

Test Plan

Uh oh!

thejchap commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carljm May 9, 2025

Choose a reason for hiding this comment

Uh oh!

thejchap May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carljm May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thejchap May 10, 2025

Choose a reason for hiding this comment

Uh oh!

carljm May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carljm commented May 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thejchap commented May 9, 2025 •

edited

Loading

github-actions bot commented May 9, 2025 •

edited

Loading

`mypy_primer` results

thejchap May 10, 2025 •

edited

Loading

carljm May 10, 2025 •

edited

Loading