-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dataclasses.replace: fall through to typeshed sig #15962
dataclasses.replace: fall through to typeshed sig #15962
Conversation
This comment has been minimized.
This comment has been minimized.
e91d2da
to
a2f3780
Compare
a2f3780
to
a657d10
Compare
This comment has been minimized.
This comment has been minimized.
6fd543b
to
0b95f3e
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, just one question :)
@@ -853,7 +853,7 @@ class Person: | |||
name: str | |||
|
|||
p = Person('John') | |||
y = replace(p, name='Bob') # E: Argument 1 to "replace" has incompatible type "Person"; expected a dataclass | |||
y = replace(p, name='Bob') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this can potentially introduce some new weak spots, but on other hand it can eventually work :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a false positive but it's due to #15974.
# error before type-guard | ||
y = replace(x) # E: Value of type variable "_DataclassT" of "replace" cannot be "object" | ||
# no error after type-guard | ||
if is_dataclass(x) and not isinstance(x, type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to test just if is_dataclass(x):
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because type
is an object, and is_dataclass
on an object either means it's a dataclass instance or type.
I'll improve the test to clarify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff from mypy_primer, showing the effect of this PR on open source code: vision (https://github.com/pytorch/vision): typechecking got 1.37x slower (36.8s -> 50.5s)
(Performance measurements are based on a single noisy sample)
|
If the dataclasses plugin cannot determine a signature for
dataclasses.replace
, it should not report an error. The underlying typeshed signature will get a shot at verifying the type and reporting an error, and it would enable the following pattern (without typingreplace
's kwargs, though):