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

Add support for PEP 698 - override decorator #14609

Merged
merged 5 commits into from
May 12, 2023

Conversation

tmke8
Copy link
Contributor

@tmke8 tmke8 commented Feb 4, 2023

closes #14072

This implements support for PEP 698, which has recently been accepted for Python 3.12. However, this doesn't yet add the "strict mode" that is recommended in the PEP.

Comment on lines +2875 to +2918
class C:
@override
def __init__(self, a: int) -> None: pass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure whether to allow it on __init__.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem very useful, but I also don't see a reason to explicitly disallow it. @stroxler any thoughts?

@JelleZijlstra
Copy link
Member

Thanks! I'll take a look soon. FYI to @stroxler.

@JelleZijlstra JelleZijlstra self-requested a review February 4, 2023 18:44
@github-actions

This comment has been minimized.

Comment on lines +2875 to +2918
class C:
@override
def __init__(self, a: int) -> None: pass
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem very useful, but I also don't see a reason to explicitly disallow it. @stroxler any thoughts?

@stroxler
Copy link
Contributor

stroxler commented Feb 5, 2023

I believe the Pyre implementation would allow it, and certainly the rules for the PEP do.

Agreed that it probably isn't very useful - if object has an __init__ then it's not possible for the check to fail.

@tmke8
Copy link
Contributor Author

tmke8 commented Feb 8, 2023

Another thing that could be done here is making the compatibility check stricter when @override is used. As issue #1013 notes, mypy is quite lenient when it comes to function parameter names and allows this:

class A:
    def f(self, x: int) -> str: ...
class B(A):
    def f(self, xx: int) -> str: ...

even though B().f(x=2) will not work at runtime.

@github-actions

This comment has been minimized.

@tmke8
Copy link
Contributor Author

tmke8 commented Feb 9, 2023

@stroxler what about @overloads? The PEP doesn't mention them, but it seems pyright at least supports the following:

from abc import abstractmethod
from typing import overload
from typing_extensions import override

class A:
    @abstractmethod
    def f(self, x: int) -> str: ...

class B(A):
    @overload
    def f2(self, x: int) -> str: ...
    @overload
    def f2(self, x: str) -> str: ...
    @override
    def f2(self, x: int | str) -> str:  #  error: Method "f2" is marked as override, but no base method of same name is present (reportGeneralTypeIssues)
        return str(x)

class C(A):
    @overload
    def f(self, y: int) -> str: ...
    @overload
    def f(self, y: str) -> str: ...
    @override
    def f(self, y: int | str) -> str:  # error: Method "f" overrides class "A" in an incompatible manner
                                       # No overload signature in override is compatible with base method (reportIncompatibleMethodOverride)
        return str(y)

but I don't see an error in the Pyre playground.

@github-actions

This comment has been minimized.

@override
def f2(self, x: int) -> str: pass # E: Method "f2" is marked as an override, but no base method was found with this name
[typing fixtures/typing-full.pyi]
[builtins fixtures/tuple.pyi]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't see a test case that uses overloaded methods (using the @overload decorator). Can you test overriding when it involves overloaded methods as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the test and also added support for overloads decorated with @override. The rule is now that if any overload item has the decorator, the whole overload is treated as an explicit override.

Comment on lines +2992 to +2999
class C(A):
@overload
def f(self, y: int) -> str: pass
@override
@overload
def f(self, y: str) -> str: pass
@override
def f(self, y: int | str) -> str: pass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation allows that multiple overload items are decorated with @override. I can also disallow this if desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could also restrict it to only the first and the last, or something like that.

@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

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

graphql-core (https://github.com/graphql-python/graphql-core) got 1.05x slower (283.0s -> 298.1s)

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you for implementing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support PEP 698 – Override Decorator
5 participants