-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Add copy.replace
for 3.13
#12262
Add copy.replace
for 3.13
#12262
Conversation
@@ -11,6 +12,10 @@ PyStringMap: Any | |||
def deepcopy(x: _T, memo: dict[int, Any] | None = None, _nil: Any = []) -> _T: ... | |||
def copy(x: _T) -> _T: ... | |||
|
|||
if sys.version_info >= (3, 13): | |||
def replace(obj: _T, /, **changes: Any) -> _T: ... | |||
__all__ += ["replace"] |
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.
See us mostly using +=
in most places with __all__
, guessing this is for better static analysis support, or something like 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.
Yes, this is fine, it's so we don't have to repeat the whole list for every version.
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.
Oh, I meant versus using append
.
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.
Except it actually isn't in __all__
: https://github.com/python/cpython/blob/89bf33732fd53fbb954aa088cfb34f13264746ad/Lib/copy.py#L59 . So we shouldn't add it in the stub either.
This may be an oversight, so consider opening an issue over on CPython for adding replace to copy.__all__
. But as long as it's not there at runtime, we shouldn't add it in the stub either.
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.
My bad, let me fix 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.
This comment has been minimized.
This comment has been minimized.
stdlib/copy.pyi
Outdated
@@ -11,6 +16,10 @@ PyStringMap: Any | |||
def deepcopy(x: _T, memo: dict[int, Any] | None = None, _nil: Any = []) -> _T: ... | |||
def copy(x: _T) -> _T: ... | |||
|
|||
if sys.version_info >= (3, 13): | |||
def replace(obj: _SR, /, **kwargs: Any) -> _SR: ... |
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.
Higher-kinded types could be cool here, but obviously no way to get great kwargs type-checking. Even in that case you can't use P.kwargs
without P.args
.
Some classes are missing |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Interestingly, Pyright is fine with this example, and MpPy is not (causing the test failure): from __future__ import annotations
from typing import Self, Protocol, ClassVar
import copy
class ReplaceableClass:
def __init__(self, val: int) -> None:
self.val = val
def __replace__(self, val: int) -> Self:
cpy = copy.copy(self)
cpy.val = val
return cpy
from collections.abc import Callable
class _SupportsReplace(Protocol):
__replace__: Callable[..., Self]
def foo(sr: _SupportsReplace) -> None:
...
foo(ReplaceableClass(23)) Would you consider this a MyPy bug @JelleZijlstra? |
Using paramspec is a workaround that works for both: 58bb691 |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
I think there's a problem with the way you defined the I recommend changing it to: class _SupportsReplace(Protocol):
def __replace__(self, *args: Any, **kwargs: Any) -> Self:
... Mypy works fine if you define it this way. |
Thanks, I think I thought originally this wouldn't work since it doesn't work if you leave out Fixed: 860f674 |
This comment has been minimized.
This comment has been minimized.
Added |
CI will fail for now since it's not in |
This comment has been minimized.
This comment has been minimized.
_SR = TypeVar("_SR", bound=_SupportsReplace) | ||
|
||
class _SupportsReplace(Protocol): | ||
# In reality doesn't support args, but there's no other great way to express this. |
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.
Technically speaking, it does support positionals, but those will be passed as keywords. What it does not support however is positional-only arguments.
On the other hand, you can just force people to use keyword-only arguments so that it matches https://docs.python.org/3.13/library/copy.html#object.__replace__. Or at least, make self
a positional-only argument.
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.
On the other hand, you can just force people to use keyword-only arguments so that it matches
The only issue is that won't work that way you might think with either MyPy or Pyright. Take this example: https://pyright-play.net/?pythonVersion=3.13&strict=true&code=GYJw9gtgBALgngBwJYDsDmUkQWEMoAK4MYAxmADYA0UAginAFDOkUCGAzh1APoDKAVwQ48HAEoBTBO1ISAFETAlyFAJQAuRlG1QAJhOC8eIKTIk8ecjhIrAaAKnsBrAO5sQaDuroNVUALQAfFAAcmAoEt4AdDHMjKyc3ACS2BSaOnoGRibSbLIWVjZ2UO5oPACM3qgwfkGh4ZFQMVHM%2BoYwEhwwcjkybABGFI38QiIw4qZ5ErXBYRHRscxY0lAAvFAp0nKqjB1dcstqQA
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.
Yes, callables with *args: Any, **kwargs: Any
are treated specially in the spec.
We just added |
My guess is we'll get around this once we start running stubtest on 3.14. |
_SR = TypeVar("_SR", bound=_SupportsReplace) | ||
|
||
class _SupportsReplace(Protocol): | ||
# In reality doesn't support args, but there's no other great way to express this. |
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.
Yes, callables with *args: Any, **kwargs: Any
are treated specially in the spec.
This comment has been minimized.
This comment has been minimized.
99de1e7
to
b404c60
Compare
@JelleZijlstra It looks like we're running against beta 4 now, do we want to merge this to resolve the CI failure? |
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Sure. Seems like there's some other unrelated new stubtest failures too. |
Add
copy.replace
which is new in 3.13: https://docs.python.org/3.13/library/copy.html#copy.replace