Skip to content
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

PEP 702 (@deprecated): overriding deprecated methods #18085

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tyralla
Copy link
Collaborator

@tyralla tyralla commented Nov 1, 2024

I think some points need discussion before implementing everything neatly, especially regarding overloads. So, my first (untidy) commit focuses on "normal" methods (testDeprecatedOverriddenMethod).


Is it okay to override a deprecated method with another signature when not using @override?

My tendency is yes; see testDeprecatedOverriddenMethod.


Is it desirable that Mypy emits an error for C.f in the following case (as it currently does)?

class A:
    def f(self) -> int: ...
class B(A):
    def f(self) -> str: ..  # error !!!
class C(B):
    def f(self) -> str: ..  # error ???

My tendency is that no error should be emitted for C.f. I did not change this, but it appears to be simple.

I am asking because:

class A:
    @deprecated
    def f(self) -> int: ...
class B(A):
    def f(self) -> str: ..  # maybe okay, see above
class C(B):
    @override
    def f(self) -> str: ..  # should then be also okay

PEP 698 does not mention @overload. In Mypy, a single @override for any overload item or the implementation affects the whole overloaded method. This was decided here without any discussions I know of and will likely result in some inconsistencies with the overload item-specific implementation of @deprecated:

class A:
    @overload
    @deprecated("replaced by g")
    def f(self, x: int) -> None: ...
    @overload
    def f(self, x: str) -> None: ...
    @overload
    def f(self, x: None) -> None: ...
    def f(self, x: Union[int, str, None]) -> None: ...

class B:
    @overload
    @override
    def f(self, x: str) -> None: ...
    @overload
    @override
    def f(self, x: None) -> None: ...
    def f(self, x: Union[str, None]) -> None: ...

I think it would be favourable to neither emit deprecation nor signature-incompatible notes in the given example. (I did not investigate how hard it would be to implement this. override and deprecated are properties of symbols. But maybe we could just remove some items before doing the checks.)

@tyralla
Copy link
Collaborator Author

tyralla commented Nov 1, 2024

@JelleZijlstra: I am not allowed to add the correct label, am I?

Copy link
Contributor

github-actions bot commented Nov 1, 2024

Diff from mypy_primer, showing the effect of this PR on open source code:

Tanjun (https://github.com/FasterSpeeding/Tanjun)
+ tanjun/context/slash.py:1045: note:      Superclass:
+ tanjun/context/slash.py:1045: note:          Optional[ExecutableCommand[SlashContext]]
+ tanjun/context/slash.py:1045: note:      Subclass:
+ tanjun/context/slash.py:1045: note:          Optional[BaseSlashCommand]
+ tanjun/context/message.py:130: note:      Superclass:
+ tanjun/context/message.py:130: note:          Optional[ExecutableCommand[MessageContext]]
+ tanjun/context/message.py:130: note:      Subclass:
+ tanjun/context/message.py:130: note:          Optional[MessageCommand[Any]]
+ tanjun/context/menu.py:101: note:      Superclass:
+ tanjun/context/menu.py:101: note:          Optional[ExecutableCommand[MenuContext]]
+ tanjun/context/menu.py:101: note:      Subclass:
+ tanjun/context/menu.py:101: note:          Optional[MenuCommand[Any, Any]]

steam.py (https://github.com/Gobot1234/steam.py)
+ steam/ext/csgo/models.py:178: note:      Superclass:
+ steam/ext/csgo/models.py:178: note:          def inventory(self, App[str | None], /, *, context_id: int | None = ..., language: Language | None = ...) -> Coroutine[Any, Any, Inventory[Item[ClientUser], ClientUser]]
+ steam/ext/csgo/models.py:178: note:      Subclass:
+ steam/ext/csgo/models.py:178: note:          @overload
+ steam/ext/csgo/models.py:178: note:          def inventory(self, app: Any, *, language: object = ...) -> Coroutine[Any, Any, Backpack]
+ steam/ext/csgo/models.py:178: note:          @overload
+ steam/ext/csgo/models.py:178: note:          def inventory(self, app: App[str | None], *, language: Language | None = ...) -> Coroutine[Any, Any, Inventory[Item[ClientUser], ClientUser]]

@JelleZijlstra JelleZijlstra added the topic-pep-702 PEP 702, @deprecated label Nov 1, 2024
@tyralla tyralla mentioned this pull request Nov 19, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-pep-702 PEP 702, @deprecated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants