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

Variable set in with block reported as possibly unbound in certain cases #7009

Closed
FeldrinH opened this issue Jan 16, 2024 · 7 comments
Closed
Labels
as designed Not a bug, working as intended bug Something isn't working

Comments

@FeldrinH
Copy link

Describe the bug

When creating variables inside a with block where matplotlib.pyplot.ion() or matplotlib.pyplot.ioff() is the context manager, these variables are reported as possibly unbound after the with block.

To the best of my knowledge this is just plain wrong. The with block body always runs and the variables are always bound. I would expect Pylance/Pyright to treat them as such.

This does not seem to happen with other context managers.

Code or Screenshots

import matplotlib.pyplot

with matplotlib.pyplot.ion():
    a = 1

with matplotlib.pyplot.ioff():
    b = 1

print(a) # <-- Pylance error: "a" is possibly unbound
print(b) # <-- Pylance error: "b" is possibly unbound

with open('test.txt'):
    c = 2

print(c) # <-- No error

Environment data

  • Pyright version: Pyright 1.1.338, commit c93d9eb (executed from Pylance 2023.12.103 in VSCode)
  • OS and version: Windows 10
  • Python version: Python 3.10.11
  • matplotlib version: 3.8.2
@FeldrinH FeldrinH added the bug Something isn't working label Jan 16, 2024
@erictraut
Copy link
Collaborator

This behavior is intended, so I don't consider this a bug — at least not in pyright.

Context managers can be written such that they "swallow" all exceptions that occur within their scope. Other context managers do not do this and allow any exceptions to bubble up. By convention, type checkers determine the difference by looking at the defined return type of the __exit__ method for a context manager. If the __exit__ method is None or Literal[False], then it is assumed to allow exceptions to bubble up. In this case, the return type of the __exit__ method is unannotated in the matplotlib stubs bundled with pylance, so pyright assumes in this case that all exceptions are swallowed. That means any variable assigned within the context manager block may be skipped.

Your options:

  1. File a bug in the pylance-release project and report the bug in the matplotlib stubs.
  2. Change your code to initialize a and b with a value before the context manager block.
  3. Use a # type: ignore or # pyright: ignore comment.
  4. Disable the reportUnboundVariable check in your pyright configuration.

@erictraut erictraut added the as designed Not a bug, working as intended label Jan 16, 2024
@erictraut erictraut closed this as not planned Won't fix, can't repro, duplicate, stale Jan 16, 2024
@FeldrinH
Copy link
Author

FeldrinH commented Jan 16, 2024

This seems kind of counterintuitive. At least with Pylance basic type checking, if a type is unknown Pyright makes the related type checking logic more lax and allows things that might be incorrect. However, in this case Pytight seems to make the type checking more strict when the type is unknown. It seems to me more appropriate and consistent (or at least more practical, given that many libraries lack proper typing) to assume that exceptions are not swallowed, if the return type of __exit__ is unknown.

@erictraut
Copy link
Collaborator

erictraut commented Jan 16, 2024

A bad assumption in either direction will produce false positives. One isn't "more lax" than the other.

The __exit__ return type mechanism was never formalized as part of the typing spec, so it's underspecified currently. It was initially introduced by mypy, and I've tried to match mypy's behavior, but without a spec, there are likely differences. This issue is on a long list of issues that I plan to formally introduce in the typing spec over the next several months so the behavior is standardized and documented. Until we have an agreed-upon specification, I don't plan to modify pyright's behavior in this regard because it would cause churn for existing pyright users. If a changes is needed (once the spec is pinned down), I will make the change once.

@FeldrinH
Copy link
Author

FeldrinH commented Jan 16, 2024

Though in this specific case it seems that Pyright is technically correct because of a combination of a few things:

  1. I believe that with matplotlib 3.8.2 Pylance actually uses type hints bundled with matplotlib (see Revert "Remove matplotlib stubs and tests" python-type-stubs#299, correct me if I'm wrong)
  2. ion and ioff return a general purpose contextlib.ExitStack
  3. contextlib.ExitStack.__exit__ returns a bool and can actually swallow exceptions if configured to do so (in practice matplotlib configures it such that it can't swallow exceptions, but there is no way for the type checker to know that)

I think I'm going to try opening an issue with matplotlib in the hopes that they can make the return type more specific to avoid this problem.

@FeldrinH
Copy link
Author

FeldrinH commented Jan 18, 2024

By convention, type checkers determine the difference by looking at the defined return type of the __exit__ method for a context manager. If the __exit__ method is None or Literal[False], then it is assumed to allow exceptions to bubble up.

@erictraut Currently an __exit__ method with a return type of bool | None also makes Pyright assume that exceptions are never supressed. I just wanted to clarify if this is intended behavior? If for example matplotlib changes its type hints to return AbstractContextManager instead of ExitStack then that currently gets rid of the error, because AbstractContextManager.__exit__ returns bool | None, but is this intended or is it simply a limitation of how Pyright currently checks the return type of __exit__?

@erictraut
Copy link
Collaborator

is this intended?

When I was writing this logic in pyright, I matched mypy's behavior. However, I don't know if mypy's behavior in this case was intended or accidental. It strikes me as inconsistent. For example, mypy treats a return type of bool, int, and int | None as "may swallow exceptions" but a return type of bool | None as "never swallows exceptions". That seems strange to me, and I suspect it wasn't an intentional decision.

This is clearly an area that should be specified in the typing spec, and I'm hopeful that we'll get to it in the near future. It's on a long list of items that the recently-formed "typing council" (of which I'm a member) needs to prioritize.

@FeldrinH
Copy link
Author

Intended or not, it is convenient that this makes type checking assume that a general AbstractContextManager will not swallow errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants