-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Make warn-unreachable understand exception-swallowing contextmanagers #7317
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
Make warn-unreachable understand exception-swallowing contextmanagers #7317
Conversation
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.
I am going to close/re-open to trigger builds. |
FYI, the tests for this PR aren't going to pass -- python/typeshed#3179 needs to be landed first. |
Oh, I somehow thought it was already merged. We can make then a dedicated sync when this will be ready. Also there is now a bonus self-check failure (easy to fix however). |
I merged the typeshed PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good, I have several small comments.
self.check_with_item(expr, target, s.unanalyzed_type is None) | ||
self.accept(s.body) | ||
exit_ret_type = self.check_with_item(expr, target, s.unanalyzed_type is None) | ||
if is_literal_type(exit_ret_type, "builtins.bool", False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these rules/conventions documented somewhere? I think they should be, otherwise we will just forget them (either in mypy docs, btw do we have a section about context managers?, or in typeshed). Maybe also add a link here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did add some notes about these conventions to the typeshed contribution page (at the bottom of the conventions section), but this conventions currently aren't otherwise documented.
We do mention context managers once on the Protocols page -- I'll add a note about this there in a follow-up PR.
4fee4b8
to
b75f886
Compare
As an FYI, I ended up making one more change to this PR: basically, I made it so we no longer assume context managers suppress exceptions when LMK if you see a better way of handling this or if handled the no-strict-optional check incorrectly, and I can submit a follow-up PR to fix. |
Thanks! Sounds good to me. |
This pull request fixes #7214: it makes mypy treat any contextmanagers where the
__exit__
returnsbool
orLiteral[True]
as ones that can potentially swallow exceptions.Contextmanagers that return
Any
,Optional[bool]
, None, orLiteral[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:This pull request needs the typeshed changes I made in python/typeshed#3179 before the tests and the mypy self-check will pass. Once that one gets merged, I'll update typeshed and rebase this pull request.