[red-knot] typing.dataclass_transform
#17445
Merged
Conversation
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
Contributor
|
sharkdp
force-pushed
the
david/dataclass_transform
branch
2 times, most recently
from
0ff175b to
a2c00b0
Compare
sharkdp
force-pushed
the
david/dataclass_transform
branch
2 times, most recently
from
3acb56b to
5aca67b
Compare
Contributor
Author
|
Ecosystem analysis: There are a few new false positives. We do not understand unannotated dataclass fields: @attr.define
class Foo:
foo = attr.field()
Foo(foo=1) # Argument `foo` does not match any known parameterThe false positives in |
sharkdp
force-pushed
the
david/dataclass_transform
branch
from
5aca67b to
36ad484
Compare
sharkdp
commented
Apr 18, 2025
sharkdp
commented
Apr 18, 2025
sharkdp
force-pushed
the
david/dataclass_transform
branch
from
36ad484 to
63995af
Compare
sharkdp
commented
Apr 18, 2025
Comment on lines
+234
to
+236
| # Salsa generates functions with parameters for each field of a `salsa::interned` struct. | ||
| # If we don't allow this, we get warnings for structs with too many fields. | ||
| too_many_arguments = "allow" |
Contributor
Author
There was a problem hiding this comment.
I didn't find a way to only set this for red_knot_python_semantic, as it inherits this configuration, and it looks like I can't overwrite workspace settings for lints?
Contributor
Author
There was a problem hiding this comment.
I'll merge this for now, but I will open a salsa ticket to improve this, and add a reminder for myself to remove this again.
Contributor
|
sharkdp
force-pushed
the
david/dataclass_transform
branch
4 times, most recently
from
598bfb3 to
4abc9c6
Compare
sharkdp
commented
Apr 18, 2025
Comment on lines
+284
to
+290
| # TODO: these should not be errors | ||
| D1("a") # error: [too-many-positional-arguments] | ||
| D2("a") # error: [too-many-positional-arguments] | ||
|
|
||
| # TODO: these should be invalid-argument-type errors | ||
| D1(1.2) # error: [too-many-positional-arguments] | ||
| D2(1.2) # error: [too-many-positional-arguments] |
Contributor
Author
There was a problem hiding this comment.
This is similar to the (working) test above, but now it's not the implementation that is decorated with @dataclass_transform(), but one of the overloads. This is explicitly allowed in the spec.
I haven't looked into this deeply, but I am assume this isn't working since we never see the decorator on the actual function definition. I am not sure how to handle this, as I'm not yet familiar with how overloads work. And how we would treat decorators on overloads. @dhruvmanila This is not urgent, but if you see an easy solution to this, I'd be glad to hear it.
Contributor
There was a problem hiding this comment.
The logic for collecting overloads is in FunctionType::signature method. I think you'd either need to have that method also return "dataclass transform params, if found" (which is a little weird), or extract the overload-walking logic into a separate method that iterates overloads as FunctionLiteral types, and use that both for your purposes and in ::signature.
Member
There was a problem hiding this comment.
extract the overload-walking logic into a separate method that iterates overloads as
FunctionLiteraltypes, and use that both for your purposes and in::signature.
Yeah, I think this is what I'd do as well.
Initially, I thought we could have a method that returns an iterator over the overloaded FunctionType but that wouldn't work reliably because we need to follow through all of the symbols with the same name to find the boundary where the overload ends. This means the method would need to collect all of them, so maybe something like the following method:
struct OverloadedFunction<'db> {
/// The overloads of this function.
overloads: Vec<FunctionType<'db>>,
/// The implementation of this overloaded function, if any.
implementation: Option<FunctionType<'db>>,
}
#[salsa::tracked]
impl<'db> FunctionType<'db> {
/// Returns `self` as [`OverloadedFunction`] if it is overloaded, [`None`] otherwise.
fn to_overloaded(self, db: &'db dyn Db) -> Option<OverloadedFunction<'db>> {
todo!()
}
}This could be used to get the DataclassTransformerParams that's added to a FunctionType which is decorated with @dataclass_transform. Another way would be to add a method to return this and to keep the above method internal-only:
#[salsa::tracked]
impl<'db> FunctionType<'db> {
/// Returns `self` as [`OverloadedFunction`] if it is overloaded, [`None`] otherwise.
fn to_overloaded(self, db: &'db dyn Db) -> Option<OverloadedFunction<'db>> {
todo!()
}
// TODO: What should be the name of this method? Or, should we change the field name?
pub(crate) fn dataclass_transformer_params(self, ...) -> Option<DataclassTransformerParams> {
if let Some(overloaded) = self.to_overloaded(db) {
// get the `DataclassTransformerParams` from `OverloadedFunction`
} else {
self.dataclass_transformer_params(db)
}
}
}We might then need to check whether "the dataclass_transform decorator is applied to any one, but not more than one, of the overloads" and raise diagnostic (although Pyright and mypy doesn't do that) within this function and then it might just be better to actually create the DataclassTransformerParams in this method instead of during inference.
I also thought about this for a bit and I would also need to somehow iterate over all the overloaded function definitions during the inference (at the end of infer_region_scope) to validate the overload usages. The information that I require would all exists on the Signature (would need to copy the FunctionDecorators) which can be fetched by the signature method so that might be ok.
Contributor
Author
There was a problem hiding this comment.
Thank you Carl and Dhruv. I left this open for now, but opened a new (post-alpha?) ticket.
sharkdp
commented
Apr 18, 2025
sharkdp
force-pushed
the
david/dataclass_transform
branch
from
4abc9c6 to
4983da5
Compare
carljm
approved these changes
Apr 21, 2025
Comment on lines
+284
to
+290
| # TODO: these should not be errors | ||
| D1("a") # error: [too-many-positional-arguments] | ||
| D2("a") # error: [too-many-positional-arguments] | ||
|
|
||
| # TODO: these should be invalid-argument-type errors | ||
| D1(1.2) # error: [too-many-positional-arguments] | ||
| D2(1.2) # error: [too-many-positional-arguments] |
Contributor
There was a problem hiding this comment.
The logic for collecting overloads is in FunctionType::signature method. I think you'd either need to have that method also return "dataclass transform params, if found" (which is a little weird), or extract the overload-walking logic into a separate method that iterates overloads as FunctionLiteral types, and use that both for your purposes and in ::signature.
dhruvmanila
approved these changes
Apr 21, 2025
Comment on lines
+284
to
+290
| # TODO: these should not be errors | ||
| D1("a") # error: [too-many-positional-arguments] | ||
| D2("a") # error: [too-many-positional-arguments] | ||
|
|
||
| # TODO: these should be invalid-argument-type errors | ||
| D1(1.2) # error: [too-many-positional-arguments] | ||
| D2(1.2) # error: [too-many-positional-arguments] |
Member
There was a problem hiding this comment.
extract the overload-walking logic into a separate method that iterates overloads as
FunctionLiteraltypes, and use that both for your purposes and in::signature.
Yeah, I think this is what I'd do as well.
Initially, I thought we could have a method that returns an iterator over the overloaded FunctionType but that wouldn't work reliably because we need to follow through all of the symbols with the same name to find the boundary where the overload ends. This means the method would need to collect all of them, so maybe something like the following method:
struct OverloadedFunction<'db> {
/// The overloads of this function.
overloads: Vec<FunctionType<'db>>,
/// The implementation of this overloaded function, if any.
implementation: Option<FunctionType<'db>>,
}
#[salsa::tracked]
impl<'db> FunctionType<'db> {
/// Returns `self` as [`OverloadedFunction`] if it is overloaded, [`None`] otherwise.
fn to_overloaded(self, db: &'db dyn Db) -> Option<OverloadedFunction<'db>> {
todo!()
}
}This could be used to get the DataclassTransformerParams that's added to a FunctionType which is decorated with @dataclass_transform. Another way would be to add a method to return this and to keep the above method internal-only:
#[salsa::tracked]
impl<'db> FunctionType<'db> {
/// Returns `self` as [`OverloadedFunction`] if it is overloaded, [`None`] otherwise.
fn to_overloaded(self, db: &'db dyn Db) -> Option<OverloadedFunction<'db>> {
todo!()
}
// TODO: What should be the name of this method? Or, should we change the field name?
pub(crate) fn dataclass_transformer_params(self, ...) -> Option<DataclassTransformerParams> {
if let Some(overloaded) = self.to_overloaded(db) {
// get the `DataclassTransformerParams` from `OverloadedFunction`
} else {
self.dataclass_transformer_params(db)
}
}
}We might then need to check whether "the dataclass_transform decorator is applied to any one, but not more than one, of the overloads" and raise diagnostic (although Pyright and mypy doesn't do that) within this function and then it might just be better to actually create the DataclassTransformerParams in this method instead of during inference.
I also thought about this for a bit and I would also need to somehow iterate over all the overloaded function definitions during the inference (at the end of infer_region_scope) to validate the overload usages. The information that I require would all exists on the Signature (would need to copy the FunctionDecorators) which can be fetched by the signature method so that might be ok.
sharkdp
force-pushed
the
david/dataclass_transform
branch
from
4983da5 to
8db0e6e
Compare
dcreager
added a commit
that referenced
this pull request
Apr 22, 2025
* main: (37 commits) [red-knot] Add list of failing/slow ecosystem projects (#17474) [red-knot] mypy_primer: extend ecosystem checks (#17544) [red-knot] Move `InstanceType` to its own submodule (#17525) [red-knot] mypy_primer: capture backtraces (#17543) [red-knot] mypy_primer: Use upstream repo (#17500) [red-knot] `typing.dataclass_transform` (#17445) Update dependency react-resizable-panels to v2.1.8 (#17513) Update dependency smol-toml to v1.3.3 (#17505) Update dependency uuid to v11.1.0 (#17517) Update actions/setup-node action to v4.4.0 (#17514) [red-knot] Fix variable name (#17532) [red-knot] Add basic subtyping between class literal and callable (#17469) [`pyupgrade`] Add fix safety section to docs (`UP030`) (#17443) [`perflint`] Allow list function calls to be replaced with a comprehension (`PERF401`) (#17519) Update pre-commit dependencies (#17506) [red-knot] Simplify visibility constraint handling for `*`-import definitions (#17486) [red-knot] Detect (some) invalid protocols (#17488) [red-knot] Correctly identify protocol classes (#17487) Update dependency ruff to v0.11.6 (#17516) Update Rust crate shellexpand to v3.1.1 (#17512) ...
Comment on lines
+257
to
+278
| @overload | ||
| @dataclass_transform() | ||
| def versioned_class( | ||
| cls: T, | ||
| *, | ||
| version: int = 1, | ||
| ) -> T: ... | ||
| @overload | ||
| def versioned_class( | ||
| *, | ||
| version: int = 1, | ||
| ) -> Callable[[T], T]: ... | ||
| def versioned_class( | ||
| cls: T | None = None, | ||
| *, | ||
| version: int = 1, | ||
| ) -> T | Callable[[T], T]: | ||
| raise NotImplementedError | ||
|
|
||
| @versioned_class | ||
| class D1: | ||
| x: str |
Contributor
There was a problem hiding this comment.
In this example, versioned_class takes in a parameter (version).
In that case, versioned_class, when called, should return a decorator which will accept a class, right?
If the default version value has to be picked up, even then, the invocation should be like this, isn't it?
@versioned_class() # notice ()
class D1:
x: str@sharkdp I did not understand how @versioned_class (without the call) will work.
Contributor
Author
There was a problem hiding this comment.
In this example,
versioned_classtakes in a parameter (version).
Well... it takes a keyword-only argument version. But it can also take a positional cls argument (first overload).
In that case,
versioned_class, when called, should return a decorator which will accept a class, right?
If it's called with versioned_class(version=2), it should return a decorator, yes.
But if it's called with versioned_class(some_class), where some_class is of type T, it should return a T (first overload). This lets versioned_class itself also act as a decorator.
If the default
versionvalue has to be picked up, even then, the invocation should be like this, isn't it?
@versioned_class() # notice ()
Using @versioned_class() should also work (it calls the second overload), but the purpose of this test was to check that it also works without the parens (in which case it is called with one argument, the D1 class, and picks the first overload).
Contributor
There was a problem hiding this comment.
Thanks for the explanation.
If it's called with versioned_class(version=2), it should return a decorator, yes.
For cases where versioned_class itself is the decorator (first overload), there is no way to pass a custom version value right? If so, what is the point of having the version kwarg in the first overload's signature? (ie, why not just versioned_class(cls: T) -> T?)
Contributor
Author
There was a problem hiding this comment.
I guess you could still use it with non-decorator syntax
class C: …
C = versioned_class(C, version=2)But yeah, I agree, it's probably not that useful. I added this test because a lot of libraries like attrs, pydantic, or strawberry use this exact pattern. They provide dataclass-like decorators which you can use with or without arguments. And using these two overloads is exactly how they are implemented.
Contributor
There was a problem hiding this comment.
Got it, thanks for the context!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
typing.dataclass_transform@dataclass_transform(…)(used byattrs,strawberry)@dataclass_transform(…)(used bypydantic, but doesn't work yet, because we don't seem to model__new__calls correctly?)@dataclass_transform(…). I haven't figured out how this even supposed to work. And haven't seen it being used.strawberryas an ecosystem project, as it makes heavy use of@dataclass_transformTest Plan
New Markdown tests