[ty] Make dataclass instances adhere to DataclassInstance#18115
Merged
Conversation
Contributor
|
sharkdp
commented
May 15, 2025
Comment on lines
+645
to
+646
| They can be passed into `fields` as well, because it also accepts `type[DataclassInstance]` | ||
| arguments: |
Contributor
Author
There was a problem hiding this comment.
This just passes "by accident" at the moment (because we still think that class objects themselves are also instances of the DataclassInstance protocol, and fields accepts DataclassInstance | type[DataclassInstance]), but I formulated it in such a way that it will be correct eventually.
| )) | ||
| .with_qualifiers(TypeQualifiers::CLASS_VAR); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
After moving this here, the test static_assert(is_subtype_of(Foo, DataclassInstance)) will (incorrectly) start passing, right?
Should that be added as a TODO, along with the asdict TODO in the tests?
Contributor
Author
There was a problem hiding this comment.
After moving this here, the test
static_assert(is_subtype_of(Foo, DataclassInstance))will (incorrectly) start passing, right?
Yes, I think so. I did not add this test, but a similar one (passing Foo to asdict). Feel free to suggest this as a follow-up (with a TODO), if you think it makes sense to add.
Contributor
There was a problem hiding this comment.
Maybe I'm missing something here, but I think static_assert(is_subtype_of(Foo, DataclassInstance)) is correct and should pass. is_subtype_of takes type expressions, and the type expression Foo does not describe a class literal type, it describes the type of all instances of Foo. That type is a subtype of DataclassInstance.
(In the previous PR on this topic I made a comment about a wrong test, but that comment was wrong :) I thought the test fields(Foo) should fail because the class object Foo does not inhabit DataclassInstance. The latter is right, but fields(Foo) should still succeed because fields also accepts type[DataclassInstance].)
The test which should eventually not pass would be is_subtype_of(type[Foo], DataclassInstance) or is_subtype_of(ty_extensions.TypeOf[Foo], DataclassInstance). (The former tests what we call a SubclassOf type, the latter a ClassLiteral type.)
| # TODO: this should be a invalid-argument-type error, but we don't properly check the | ||
| # types (and more importantly, the `ClassVar` type qualifier) of protocol members yet. | ||
| asdict(Foo) | ||
| ``` |
Contributor
There was a problem hiding this comment.
There's also astuple with a similar signature. Should that also be included in the tests?
Contributor
Author
There was a problem hiding this comment.
Sure, that would make sense.
Glyphack
pushed a commit
to Glyphack/ruff
that referenced
this pull request
May 21, 2025
…18115) ## Summary Make dataclass instances adhere to the `DataclassInstance` protocol. fixes astral-sh/ty#400 ## Test Plan New Markdown tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make dataclass instances adhere to the
DataclassInstanceprotocol.fixes astral-sh/ty#400
Test Plan
New Markdown tests