Make most contextmanager __exit__ signatures return Optional[bool]
#3179
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Michael0x2a
added a commit
to Michael0x2a/mypy
that referenced
this pull request
Aug 10, 2019
This pull request fixes python#7214: it makes mypy treat any contextmanagers where the `__exit__` returns `bool` or `Literal[True]` as ones that can potentially swallow exceptions. Contextmanagers that return `Optional[bool]`, None, or `Literal[False]` continue to be treated as non-exception-swallowing ones. This distinction helps the `--warn-unreachable` flag do the right thing in this example program: ```python from contextlib import suppress def should_warn() -> str: with contextlib.suppress(IndexError): return ["a", "b", "c"][0] def should_not_warn() -> str: with open("foo.txt") as f: return "blah" ``` This pull request needs the typeshed changes I made in python/typeshed#3179. Once that one gets merged, I'll update typeshed and rebase this pull request.
This pull request is a follow-up to python/mypy#7214. In short, within that mypy issue, we found it would be helpful to determine between contextmanagers that can "swallow" exceptions vs ones that can't. This helps prevent some false positive when using flags that analyze control flow such as `--warn-unreachable`. To do this, Jelle proposed assuming that only contextmanagers where the `__exit__` returns `bool` are assumed to swallow exceptions. This unfortunately required the following typeshed changes: 1. The typing.IO, threading.Lock, and concurrent.futures.Executor were all modified so `__exit__` returns `Optional[None]` instead of None -- along with all of their subclasses. I believe these three types are meant to be subclassed, so I felt picking the more general type was correct. 2. There were also a few concrete types (e.g. see socketserver, subprocess, ftplib...) that I modified to return `None` -- I checked the source code, and these all seem to return None (and don't appear to be meant to be subclassable). 3. contextlib.suppress was changed to return bool. I also double-checked the unittest modules and modified a subset of those contextmanagers, leaving ones like `_AssertRaisesContext` alone.
Michael0x2a
force-pushed
the
modify-contextmanager-exit-signatures
branch
from
d774ff7
to
3e60829
Compare
Michael0x2a
added a commit
to Michael0x2a/mypy
that referenced
this pull request
Aug 16, 2019
The only change is python/typeshed#3179
ilevkivskyi
pushed a commit
to python/mypy
that referenced
this pull request
Aug 17, 2019
The only change is python/typeshed#3179
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request is a follow-up to python/mypy#7214.
In short, within that mypy issue, we found it would be helpful to determine between contextmanagers that can "swallow" exceptions vs ones that can't. This helps prevent some false positive when using flags that analyze control flow such as
--warn-unreachable. To do this, Jelle proposed assuming that only contextmanagers where the__exit__returnsboolare assumed to swallow exceptions.This unfortunately required the following typeshed changes:
The typing.IO, threading.Lock, and concurrent.futures.Executor were all modified so
__exit__returnsOptional[None]instead of None -- along with all of their subclasses.I believe these three types are meant to be subclassed, so I felt picking the more general type was correct.
There were also a few concrete types (e.g. see socketserver, subprocess, ftplib...) that I modified to return
None-- I checked the source code, and these all seem to return None (and don't appear to be meant to be subclassable).contextlib.suppress was changed to return bool. I also double-checked the unittest modules and modified a subset of those contextmanagers, leaving ones like
_AssertRaisesContextalone.