-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add signature for dataclasses.replace #14849
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
Add signature for dataclasses.replace #14849
Conversation
This comment has been minimized.
This comment has been minimized.
mypy/plugins/dataclasses.py
Outdated
arg_names = [None] | ||
arg_kinds = [ARG_POS] | ||
arg_types = [obj_type] | ||
for attr in dataclass["attributes"]: |
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 wish I could've deserialized metadata, or even accessed a list of DataclassAttribute
s that's already been deserialized...
Or maybe the right way to go is to replace dataclasses.replace
with an OverloadedFuncDef
that the transform
(which has full context) will keep adding to.
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.
The OverloadedFuncDef
appears to be a non-starter since by the time I can do anything to replace dataclasses.replace
with a synthesized OverloadedFuncDef
, the call expression has been resolved with the pointer to the generic FunctionDef
.
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.
OK, solved it by creating a "secret" symbol with a signature at semantic analysis time, then using this signature in the function sig callback.
f6c3a8f
to
1f08816
Compare
This comment has been minimized.
This comment has been minimized.
@ilevkivskyi care to take a look? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
I'm just a triager over at mypy, so I'm afraid I don't have merge privileges! Here's a quick comment though:
@@ -32,3 +32,5 @@ def field(*, | |||
|
|||
|
|||
class Field(Generic[_T]): pass | |||
|
|||
def replace(obj: _T, **changes: Any) -> _T: ... |
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 pretty different to typeshed's stub for replace
. Would it be worth adding a pythoneval
test as well, to check that this machinery all works with the full typeshed stubs for the stdlib, outside the context of mypy's test suite?
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.
The stub has:
def replace(__obj: _DataclassT, **changes: Any) -> _DataclassT: ...
I can rename obj
to __obj
in this fixture. We can also change the stub to better reflect reality:
if sys.version_info >= (3, 9):
def replace(obj: _DataclassT, /, **changes: Any) -> _DataclassT: ...
else:
def replace(obj: _DataclassT, **changes: Any) -> _DataclassT: ...
Can we just use the Python 3.9 syntax even for Python 3.8 targets? Passing obj
as kw is deprecated in Python 3.8 so being a checker/linter we might as well just disallow it, no?
Do you think it matters much? For the plugin to function, replace
has to:
- exist
- have a first argument
The **changes
are optional, the retval is overwritten, and FWIW it doesn't look for a keyword arg obj
since it's deprecated since Python 3.8 and was never a good idea to begin with, so we might as well just not support it, right?
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.
We can also change the stub to better reflect reality:
No need: prepending the parameter name with __
is the py37-compatible way of declaring that a parameter is positional-only, as is described in PEP 484: https://peps.python.org/pep-0484/#positional-only-arguments. So the obj
parameter is already marked as positional-only on all Python versions in typeshed's stubs.
I don't think you need to change anything in your existing fixtures/tests for this PR, I'm just suggesting adding a single additional test in the pythoneval.test
file. Unlike mypy's other tests, the tests in that file don't use fixtures — they run with typeshed's full stdlib stubs. So it's a good sanity check to make sure that the machinery you've added works with typeshed's stubs for this function as well as with mypy's fixtures.
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.
…konst/mypy into 2023-03-06-dataclasses-replace
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Alex Waygood <[email protected]>
This comment has been minimized.
This comment has been minimized.
Diff from mypy_primer, showing the effect of this PR on open source code: vision (https://github.com/pytorch/vision) got 1.42x slower (29.2s -> 41.3s)
|
@ilevkivskyi can take another look? |
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
@ilevkivskyi can take another look? |
@ikonst Please note there is now a merge conflict. You can also address couple remaining comments before I merge this. |
Diff from mypy_primer, showing the effect of this PR on open source code: graphql-core (https://github.com/graphql-python/graphql-core): typechecking got 1.06x slower (281.6s -> 297.4s)
(Performance measurements are based on a single noisy sample)
|
…5503) Now we use a similar approach to #14849 First, we generate a private name to store in a metadata (with `-`, so - no conflicts, ever). Next, we check override to be compatible: we take the currect signature and compare it to the ideal one we have. Simple and it works :) Closes #15498 Closes #9254 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ivan Levkivskyi <[email protected]>
Validate
dataclassses.replace
actual arguments to match the fields:__init__
, the arguments are always named.InitVar
s without a default value.The tricks:
__mypy-replace
(obviously not part of PEP-557 but contains a hyphen so should not conflict with any future valid identifier). Stashing the signature into the symbol table allows it to be passed across phases and cached across invocations. The stashed signature lacks the first argument, which we prepend at function signature hook time, since it depends on the type thatreplace
is called on.Based on #14526 but actually simpler.
Partially addresses #5152.
Remaining tasks