from collections.abc import Generator
from contextlib import contextmanager
@contextmanager
def oopsie() -> Generator[None]:
yield
yield
with oopsie():
...
This will raise a RuntimeError: generator didn't stop.
Do you think we could detect that with type checkers? I think it shouldn’t be hard to cover 99% of cases, just counting yields (including from yield froms) up to 1 and erroring out on anything more or less than 1 in one code path (like, in the same if block branch).
I don’t think this is something that type checkers should handle. The contextmanager decorator expects a generator function and it’s getting exactly that. For this to be a type checker’s responsibility, the type system would have to differentiate between generators that yield once and ones that yield some other number of times.
However, this seems like a great linting rule. Unfortunately, I can’t find something that would cover this in ruff or pylint currently.
Special semantics for generator context managers could help make sure that the analyzed bodies are correct generator context managers and therefore reason about them
from collections.abc import Generator
from contextlib import contextmanager
x: int = int(input())
def P() -> int:
# Some function of x
...
@contextmanager
def H() -> Generator[None]:
yield
if P():
yield
It doesn’t matter we can’t predict the outcome of P–the type checker sees the if P(): yield as reachable and should therefore error out on that, because in no case is it valid to yield after one yield that’s guaranteed to happen.
A more concerning pattern would be
@contextmanager
def H() -> Generator[None]:
if P():
yield
if P():
yield
but still, the practical problem we’re trying to solve is analyzing whether a yield is in suppressing context or not. To do that, we have to validate that the analyzed generator body is correct for a generator context manager. So I’d opt for erroring out on the example above as well. In no case is it correct to yield twice. It is not correct to not yield at all, and in this program it is what we see possible. Type checkers often don’t follow the runtime magic and therefore can’t understand that something does work when types don’t exclude a bad scenario. That’s what makes type checkers useful and can in fact lead to more straightforward and well-modeled logic.
Why do you want specifically type checkers to error on this code? I totally agree that you probably shouldn’t write a contextmanager like that and some part of your toolchain should tell you that you most likely made a mistake. But I think that this kind of thing is better handled in a linter than in a type checker.
The idea behind a type checker is that it looks through your code and for each variable determines what values are allowed to be stored in it and tells you if you’re potentially putting something in a place you shouldn’t. This isn’t really related to this code pattern. Of course, you can define a new type of generators that yield exactly once and then have the contextmanager decorator take that type as an argument. But I don’t think that that’s really in the spirit of Python typing. We don’t have a type for generators that yield exactly 17 times either or one for functions that access global variables or one for iterators that never stop. That kind of program behaviour just isn’t something that the type system is trying to model.
It would also be a lot of work to make this work in the type system. You’d have to define precise semantics for which functions count as yielding exactly once. And since this is undecidable in general, you’re gonna need pretty complex rules to cover most useful cases. Type checkers would also have to be changed pretty fundamentally. While they do perform some level of code flow analysis as part of their type inference algorithms, I don’t think those tools are general enough to cover this kind of request.
On the other hand, this kind of thing is exactly what linters are for. You’ve identified an easy to make mistake in a fairly common pattern. Most of the time, the “incorrect” code also is less readable than a version that certainly yields exactly once. That’s the perfect starting point for a new linting rule!
To clarify, the reason why I believed this should be fixed by type checkers is because:
What led me to creating this thread is incorrect reachability inference caused by something that leads to a runtime type error not caught by type checkers. I’ve described it in much more detail here: Support suppressing generator context managers. There are also other people arguing that this is a linter’s responsibility rather than type checker’s, which makes perfect sense.
Looking at the symptoms of the problem I naturally assumed that this is type checker’s responsibility, and I haven’t been alone in this: just now, I’ve flicked through @Jelle’s repo that collects examples of unsoundness in the Python type system (i.e., code that produces a runtime error that the type system should catch, but that is accepted by type checkers), and found that exactly that same case has been reported and accepted some time ago: Add context manager examples by decorator-factory · Pull Request #18 · JelleZijlstra/unsoundness · GitHub
Fixing that problem with a type checker would help detect that in existing codebases without having to migrate to any other solution such as specifying redundant keyword arguments just to exercise the right overload with no runtime benefit of using them.
These two things were why I concluded that special semantics for generator context managers would be a good idea in general, since type checkers don’t cover other aspects of control flow in generator context managers as well. I’ll admit that I’m not extremely experienced with type checker internal building blocks. I know that they would do some control flow analysis, and I think I found the right spot in mypy to start the patch to address the original problem. I understand though that I might be naive in thinking that this is the right place to address it.
I’d prefer a solution that would report an error on the generator suppressing context managers problem just as soon as someone affected updates their toolchain—not having to migrate the entire codebase. If linters can do that, that would be awesome.
Anyways, the core problem is rooted in an incorrect type. My patch to typeshed is the first step to solving this, question is whether a better way exists to make the type checker see the proper _ExitT_co and _ExcReturnT of _GeneratorContextManager (and its async counterpart) without having to maintain a redundant keyword argument may_suppress= that will not detect the problem in existing codebases without (possibly automated) migration. I assumed type checkers could figure out these type vars on their own. Linters, obviously, won’t do that. Linters can provide an autofixer that updates the keyword parameters. And people supporting versions lower than 3.15 won’t migrate in years to come, because only contextlib from 3.15 could take these extra arguments.
I think we can agree that the keyword parameters approach is flawed in numerous ways, and that it is an ugly workaround. Question is whether it is possible to avoid these costs (new redundant params to @contextmanager, new things to remember, new redundancy, new migration, new ways of having a desync) or are they low enough for not complicating current type checkers. At the same time, the problem in itself is probably very, very rare in practice—I may be giving this too much meaning. If that’s the case, I’ll take the keyword parameters path.