-
Notifications
You must be signed in to change notification settings - Fork 29
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
add
strictContextManagerExitTypes setting
- Loading branch information
1 parent
cbdd6d3
commit ccbd0fd
Showing
6 changed files
with
177 additions
and
8 deletions.
There are no files selected for viewing
138 changes: 138 additions & 0 deletions
138
docs/benefits-over-pyright/fixed-context-manager-exit-types.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| # fixed handling for context managers that can suppress exceptions | ||
|
|
||
| ## the problem | ||
|
|
||
| if an exception is raised inside a context manager and its `__exit__` method returns `True`, it will be suppressed: | ||
|
|
||
| ```py | ||
| class SuppressError(AbstractContextManager[None, bool]): | ||
| @override | ||
| def __enter__(self) -> None: | ||
| pass | ||
|
|
||
| @override | ||
| def __exit__( | ||
| self, | ||
| exc_type: type[BaseException] | None, | ||
| exc_value: BaseException | None, | ||
| traceback: TracebackType | None, | ||
| /, | ||
| ) -> bool: | ||
| return True | ||
| ``` | ||
|
|
||
| but if it returns `False` or `None`, the exception will not be suppressed: | ||
|
|
||
| ```py | ||
| class Log(AbstractContextManager[None, Literal[False]]): | ||
| @override | ||
| def __enter__(self) -> None: | ||
| print("entering context manager") | ||
|
|
||
| @override | ||
| def __exit__( | ||
| self, | ||
| exc_type: type[BaseException] | None, | ||
| exc_value: BaseException | None, | ||
| traceback: TracebackType | None, | ||
| /, | ||
| ) -> Literal[False]: | ||
| print("exiting context manager") | ||
| return False | ||
| ``` | ||
|
|
||
| pyright will take this into account when determining reachability: | ||
|
|
||
| ```py | ||
| def raise_exception() -> Never: | ||
| raise Exception | ||
|
|
||
| with SuppressError(): | ||
| foo: int = raise_exception() | ||
|
|
||
| # when the exception is raised, the context manager exits before foo is assigned to: | ||
| print(foo) # error: "foo" is unbound (reportPossiblyUnboundVariable) | ||
| ``` | ||
|
|
||
| ```py | ||
| with Log(): | ||
| foo: int = raise_exception() | ||
|
|
||
| # when the exception is raised, it does not get suppressed so this line can never run: | ||
| print(foo) # error: Code is unreachable (reportUnreachable) | ||
| ``` | ||
|
|
||
| however, due to [a bug in mypy](https://github.com/python/mypy/issues/8766) that [pyright blindly copied and accepted as the "standard"](https://github.com/microsoft/pyright/issues/6034#issuecomment-1738941412), a context manager will incorrectly be treated as if it never suppresses exceptions if its return type is a union of `bool | None`: | ||
|
|
||
| ```py | ||
| class SuppressError(AbstractContextManager[None, bool | None]): | ||
| @override | ||
| def __enter__(self) -> None: | ||
| pass | ||
|
|
||
| @override | ||
| def __exit__( | ||
| self, | ||
| exc_type: type[BaseException] | None, | ||
| exc_value: BaseException | None, | ||
| traceback: TracebackType | None, | ||
| /, | ||
| ) -> bool | None: | ||
| return True | ||
|
|
||
|
|
||
| with SuppressError(): | ||
| foo: int = raise_exception() | ||
|
|
||
| # this error is wrong because this line is actually reached at runtime: | ||
| print(foo) # error: Code is unreachable (reportUnreachable) | ||
| ``` | ||
|
|
||
| ## the solution | ||
|
|
||
| basedpyright introduces a new setting, `strictContextManagerExitTypes` to address this issue. when enabled, context managers where the `__exit__` dunder returns `bool | None` are treated the same way as context managers that return `bool` or `Literal[True]`. put simply, if `True` is assignable to the return type, then it's treated as if it can suppress exceptions. | ||
|
|
||
| ## issues with `@contextmanager` | ||
|
|
||
| the reason the `strictContextManagerExitTypes` setting was created to allow users to disable this fix is because it will cause all context managers decorated with `@contextlib.contextmanager` to be treated as if they can suppress an exception, even if they never do: | ||
|
|
||
| ```py | ||
| @contextmanager | ||
| def log(): | ||
| print("entering context manager") | ||
| try: | ||
| yield | ||
| finally: | ||
| print("exiting context manager") | ||
|
|
||
| with log(): | ||
| foo: int = get_value() | ||
|
|
||
| # basedpyright accounts for the possibility that get_value raised an exception and foo | ||
| # was never assigned to, even though this context manager never suppresses exceptions | ||
| print(foo) # error: "foo" is unbound (reportPossiblyUnboundVariable) | ||
| ``` | ||
|
|
||
| this is because there's no way to tell a type checker whether the function body contains a `try`/`except` statement, which is necessary to suppress exeptions when using the `@contextmanager` decorator: | ||
|
|
||
| ```py | ||
| @contextmanager | ||
| def suppress_error(): | ||
| try: | ||
| yield | ||
| except: | ||
| pass | ||
| ``` | ||
|
|
||
| as a workaround, it's recommended to instead use class context managers [like in the examples above](#the-problem) for the following reasons: | ||
|
|
||
| - it forces you to be explicit about whether or not the context manager is able to suppress an exception | ||
| - it prevents you from accidentally creating a context manager that doesn't run its cleanup if an exception occurs: | ||
| ```py | ||
| @contextmanager | ||
| def suppress_error(): | ||
| print("setup") | ||
| yield | ||
| # this part won't run if an exception is raised because you forgot to use a try/finally | ||
| print("cleanup") | ||
| ``` |
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
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
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
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
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