[ty] Support dataclass_transform as a function call#22378
[ty] Support dataclass_transform as a function call#22378charliermarsh merged 3 commits intomainfrom
dataclass_transform as a function call#22378Conversation
charliermarsh
added
bug
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
charliermarsh
force-pushed
the
charlie/class-x
branch
from
e016ff5 to
7f675dd
Compare
|
charliermarsh
force-pushed
the
charlie/class-x
branch
from
7f675dd to
bace8c4
Compare
|
(The |
|
|
||
| // If the return type is class-like and the first argument is a | ||
| // class, return it with dataclass params applied. Otherwise, | ||
| // return a `DataclassDecorator` for application to a class later. |
There was a problem hiding this comment.
I think this is wrong. It's intended to address cases like:
@dataclass_transform()
def hydrated_dataclass(target: type, *, frozen: bool = False) -> Callable[[type[T]], type[T]]:
def decorator(cls: type[T]) -> type[T]:
return cls
return decorator
@hydrated_dataclass(SomeConfig, frozen=True)
class MyConfig:
passIf we make this change without this declared return type handling, then hydrated_dataclass(SomeConfig, frozen=True) returns type[SomeConfig] with dataclass params, and we apply that as a decorator.
There was a problem hiding this comment.
This is an unfortunate ambiguity baked directly into the dataclass_transform spec. It doesn't clarify how we are supposed to tell whether the @dataclass_transform decorated function is a decorator or a decorator factory (yet it clearly specifies that both should be supported). I think the spec's assumption is that it doesn't matter, because only decorator-syntax usage (with @) is supported, and in that case you can tell how it is used (are there parentheses or not). But we are trying to do something more sophisticated here, and I think it means we have to resort to some kind of heuristic.
But I'm not sure the heuristic should depend on the annotated return type like this; there's no real requirement to annotate your dataclass_transform decorator at all. I think a better heuristic might be based solely on the number, kind, and type of arguments. If only one positional argument is given and it's a class type, assume we should decorate that class. Otherwise, assume we are returning the real decorator.
There was a problem hiding this comment.
(Okay this is very comforting to hear, haha.)
There was a problem hiding this comment.
I think that wouldn't work for this case, since both functions take a single class as its positional arguments:
from typing_extensions import dataclass_transform
@dataclass_transform()
def hydrated_dataclass[T](target: type[T], *, frozen: bool = False):
def decorator[U](cls: type[U]) -> type[U]:
return cls
return decoratorFor now, I combined the two heuristics.
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
charliermarsh
force-pushed
the
charlie/class-x
branch
from
bd84250 to
c5a897a
Compare
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks pretty good! We are definitely being more ambitious than other type checkers here (and more ambitious than the spec requires), by supporting non-decorator-syntax usages of dataclass_transform at all. Our initial implementation of dataclass_transform kind of set us on this path, by implementing the logic generally in function-call-binding and having a "DataclassTransformer" type, rather than doing everything as a special case in decorator application. And this PR seems pretty good, so I don't see much harm in continuing on this path -- it's cool if we can support non-decorator-syntax usage like this.
A few comments inline.
| ### Passing a specialized generic class | ||
|
|
||
| When calling a `@dataclass_transform()` decorated function with a specialized generic class, the | ||
| specialization should be preserved. | ||
|
|
||
| ```py | ||
| from typing_extensions import dataclass_transform | ||
|
|
||
| @dataclass_transform() | ||
| def my_dataclass[T](cls: type[T]) -> type[T]: | ||
| return cls | ||
|
|
||
| class A[T]: | ||
| x: T | ||
|
|
||
| B = my_dataclass(A[int]) | ||
|
|
||
| reveal_type(B) # revealed: <class 'A[int]'> | ||
|
|
||
| B(1) | ||
| ``` |
There was a problem hiding this comment.
I'm impressed that you support this, but I'm curious where it came up? Did you see this in the ecosystem?
This won't work at runtime with stdlib dataclass -- it expects to get a class object, not a typing.GenericAlias. But I suppose there could be third-party dataclass-transforms that are built to handle GenericAlias at runtime?
There was a problem hiding this comment.
I honestly can't remember -- I may have asked Claude to add something to verify that we preserve specialization after seeing handling for it in the code?!
| class Target: | ||
| pass | ||
|
|
||
| decorator = hydrated_dataclass(Target) |
There was a problem hiding this comment.
What about then using this like MyClass = decorator(SomeClass)?
Doesn't seem to work, even on this branch:
class M1:
x: int
M2 = decorator(M1)
reveal_type(M2) # reveals `type[M1] & Any`And I'm not sure where the & Any comes from?
(As noted above, I don't think any other type checker supports this non-decorator use of a dataclass transform at all, so I don't know that we need to support this edge case, just curious why it doesn't work when the base case above does.)
| from typing_extensions import dataclass_transform | ||
|
|
||
| @dataclass_transform() | ||
| def my_dataclass[T](cls: type[T]) -> type[T]: | ||
| return cls | ||
|
|
||
| class A: | ||
| x: int | ||
|
|
||
| B = my_dataclass(A) | ||
|
|
||
| reveal_type(B) # revealed: <class 'A'> | ||
|
|
||
| B(1) |
There was a problem hiding this comment.
Doesn't seem like any other type checkers support this, but it's cool that we can.
| let (implementation, overloads) = self.overloads_and_implementation(db); | ||
| overloads.into_iter().chain(implementation.iter().copied()) | ||
| ) -> impl DoubleEndedIterator<Item = OverloadLiteral<'db>> + 'db { | ||
| let (overloads, implementation) = self.overloads_and_implementation(db); |
There was a problem hiding this comment.
lol, oops
I guess no other caller cared about the ordering here?
Looking at the usage in infer_class_definition, I think we might need a .rev() call there also, to make the behavior match the comment?
|
|
||
| // If the return type is class-like and the first argument is a | ||
| // class, return it with dataclass params applied. Otherwise, | ||
| // return a `DataclassDecorator` for application to a class later. |
There was a problem hiding this comment.
This is an unfortunate ambiguity baked directly into the dataclass_transform spec. It doesn't clarify how we are supposed to tell whether the @dataclass_transform decorated function is a decorator or a decorator factory (yet it clearly specifies that both should be supported). I think the spec's assumption is that it doesn't matter, because only decorator-syntax usage (with @) is supported, and in that case you can tell how it is used (are there parentheses or not). But we are trying to do something more sophisticated here, and I think it means we have to resort to some kind of heuristic.
But I'm not sure the heuristic should depend on the annotated return type like this; there's no real requirement to annotate your dataclass_transform decorator at all. I think a better heuristic might be based solely on the number, kind, and type of arguments. If only one positional argument is given and it's a class type, assume we should decorate that class. Otherwise, assume we are returning the real decorator.
charliermarsh
force-pushed
the
charlie/class-x
branch
from
c5a897a to
fb99ce4
Compare
2 participants
Summary
Instead of just as a decorator.
Closes astral-sh/ty#2319.