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 Y036: check for badly-defined __(a)exit__ methods #199

Merged
merged 8 commits into from
Apr 1, 2022

Conversation

AlexWaygood
Copy link
Collaborator

Fixes #188

@AlexWaygood
Copy link
Collaborator Author

AlexWaygood commented Apr 1, 2022

There is one remaining typeshed hit, which I'm not quite sure what to do with:

https://github.com/python/typeshed/blob/ec27c00ca220c501a55e51617117f5b2d60fc06c/stdlib/imp.pyi#L48

The check I've written argues that it should be rewritten:

def __exit__(self, *args: Any) -> Any: ...  # OLD
def __exit__(self, *args: object) -> Any: ...  # NEW

The only thing is, this method is part of a protocol definition. I'm not sure whether it's important to keep it as it is or not. I think it would probably be fine? But also protocols generally need more permissive definitions for argument types. So, I'm not sure.

If we decide not to change that method in typeshed, we have two options:

  1. # noqa it in typeshed.
  2. Add further complexity to this check by special-casing __(a)exit__ methods inside protocol definitions.

@AlexWaygood AlexWaygood requested a review from Akuli April 1, 2022 15:10
pyi.py Outdated Show resolved Hide resolved
pyi.py Outdated Show resolved Hide resolved
pyi.py Show resolved Hide resolved
pyi.py Outdated Show resolved Hide resolved
pyi.py Show resolved Hide resolved
@@ -972,6 +1052,103 @@ def visit_ClassDef(self, node: ast.ClassDef) -> None:
):
self.error(statement, Y013)

def _check_exit_method( # noqa: C901
Copy link
Collaborator

Choose a reason for hiding this comment

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

On the one hand, I agree with flake8 that this is quite complicated. On the other hand, it's good because all exit method checking is in the same place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I tried splitting it up, and, in my opinion, it made the code much less readable.

Co-authored-by: Akuli <akuviljanen17@gmail.com>
@Akuli
Copy link
Collaborator

Akuli commented Apr 1, 2022

The typeshed hit seems buggy. Isn't it saying that for an object to be compatible with that protocol, it must have an exit method that accepts any number of arguments?

To test this, I made this toy example:

from types import TracebackType
from typing import Any
from typing_extensions import Protocol

class AbstractFoo(Protocol):
    def __exit__(self, *args: Any) -> None: ...

class ConcreteFoo(AbstractFoo):
    def __exit__(self, typ: type[BaseException] | None, exc: BaseException | None, tb: TracebackType | None) -> None:
        print("Hi")

f: AbstractFoo = ConcreteFoo()

Mypying it fails with:

asd.py:9: error: Signature of "__exit__" incompatible with supertype "AbstractFoo"
asd.py:9: note:      Superclass:
asd.py:9: note:          def __exit__(self, *Any) -> None
asd.py:9: note:      Subclass:
asd.py:9: note:          def __exit__(self, Optional[Type[BaseException]], Optional[BaseException], Optional[TracebackType]) -> None
Found 1 error in 1 file (checked 1 source file)

@Akuli
Copy link
Collaborator

Akuli commented Apr 1, 2022

I get that and it's fine, but *args in a Protocol seems to be broken and a bad idea.

@AlexWaygood
Copy link
Collaborator Author

The typeshed hit seems buggy. Isn't it saying that for an object to be compatible with that protocol, it must have an exit method that accepts any number of arguments?

Hah, good point. So the stub is broken anyway.

@AlexWaygood
Copy link
Collaborator Author

I get that and it's fine, but *args in a Protocol seems to be broken and a bad idea.

Yeah sorry, I misread your message :)

@AlexWaygood AlexWaygood requested a review from Akuli April 1, 2022 16:34
Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

One more nit.

pyi.py Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra removed their request for review April 1, 2022 16:48
Co-authored-by: Akuli <akuviljanen17@gmail.com>
AlexWaygood added a commit to python/typeshed that referenced this pull request Apr 1, 2022
The signature of this method currently doesn't work quite as intended: see PyCQA/flake8-pyi#199 (comment)

Classes will [still be accepted](https://mypy-play.net/?mypy=latest&python=3.10&gist=77efe095d01edeb1a4614166f0c9cf68) as conforming to this protocol if they have more permissive signatures such as `def __exit__(self, *args: object) -> None: ...`
AlexWaygood added a commit to python/typeshed that referenced this pull request Apr 1, 2022
The signature of this method currently doesn't work quite as intended: see PyCQA/flake8-pyi#199 (comment)

Classes will [still be accepted](https://mypy-play.net/?mypy=latest&python=3.10&gist=77efe095d01edeb1a4614166f0c9cf68) as conforming to this protocol if they have more permissive signatures such as `def __exit__(self, *args: object) -> None: ...`
@AlexWaygood AlexWaygood closed this Apr 1, 2022
@AlexWaygood AlexWaygood reopened this Apr 1, 2022
@AlexWaygood AlexWaygood requested a review from Akuli April 1, 2022 17:04
@Akuli Akuli merged commit b7f7e9a into PyCQA:master Apr 1, 2022
@AlexWaygood AlexWaygood deleted the exit-methods branch April 1, 2022 18:35
@AlexWaygood
Copy link
Collaborator Author

Thanks for the reviews!

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

Successfully merging this pull request may close these issues.

Catch badly defined __exit__ methods
2 participants