Skip to content

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

Open
tyralla wants to merge 2 commits into
python:masterfrom
tyralla:feature/deprecated/override
Open

PEP 702 (@deprecated): overriding deprecated methods#18085
tyralla wants to merge 2 commits into
python:masterfrom
tyralla:feature/deprecated/override

Conversation

@tyralla

@tyralla tyralla commented Nov 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

tyralla commented Nov 1, 2024

Copy link
Copy Markdown
Collaborator Author

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

@github-actions

github-actions Bot commented Nov 1, 2024

Copy link
Copy Markdown
Contributor

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
Closed
8 tasks
@tyralla tyralla mentioned this pull request Jan 17, 2025
Merged
JelleZijlstra
JelleZijlstra reviewed Jun 3, 2026
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I dropped the ball on this. This now has a merge conflict.

My thoughts on your questions:

  • I don't think we should emit an error in the case where B incompatibly overrides A, and C compatibly overrides B. That feels like noise. However, that's an independent change from this one.
  • Regarding overloads, I think it's fine to not worry about overrides that override one deprecated overload from an overload set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

Assignees

No one assigned

Labels

topic-pep-702 PEP 702, @deprecated

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tyralla @JelleZijlstra