Add an else clause to with statements to detect early termination

I agree with this description of tradeoffs, but I don’t agree that this is a “false warning”. You don’t know that there are no suppressed exceptions. Therefore, it would be better in my opinion to assert that. Doing that assertion will both shut the type checker up as well as verify your assumption.

Just because you’re used to making an assumption and you think that assumption is almost always reasonable (I agree that it is), it doesn’t mean that code isn’t improved by checking that assumption.

But I can understand the feeling that such a check “pollutes” code if that check is extremely unlikely to catch anything—and I admit that it is.

So this is a tradeoff: more checks or fewer checks.

1 Like

I’m hesitant to add to such a heated thread, but I feel nerd-sniped. I’m probably just going to summarize the problem for posterity and for myself.

The core behavior here that is causing some problems is that with gives __exit__ control over whether the with statement as a whole “succeeds” even though an exception was raised in its block. In particular, if __exit__ is handed an exception, normally the with statement re-raises the exception after __exit__ returns. However, if __exit__ returns a truthy value, the exception is not re-raised. (See The with statement in the language reference, point 7, second paragraph.) Thus, we have this problem:

with cm():
    x = f()
print(x)

This will raise NameError if f() raises an exception and cm’s __exit__ returns True.

Type checkers that are interested in reporting whether print(x) can be reached while x is undefined will pay attention to the type of the __exit__ method of the context manager: If it is declared using -> None they assume x is always defined, since if f() raises, the exception will be re-raised after __exit__ returns. However, if __exit__ is declared using -> bool (or -> bool|None), they see this as a signal that __exit__ may return True and suppress the exception when the with statement is complete, so print(x) may potentially try to print an undefined variable.

This is why ExitStack is being picked on: its __exit__ is declared as -> bool (in typeshed) because, indeed, under certain circumstances it may pass along the True returned by one of the context managers that were added to its stack. (The code is complicated so I didn’t try to understand what happens if there are multiple context managers on its stack that may or may not return True; but this doesn’t matter for the problem caused by ExitStack.)

The unfortunate thing is that some users may want to use ExitStack but only pass it context managers whose __exit__ always returns None/False. In this case the user knows that this particular ExitStack instance will never return True from its __exit__, thus never suppressing exceptions, thus never causing print(x) to be reached without x being defined. But the type checker doesn’t know all this about ExitStack – it only knows that its __exit__ is declared using -> bool so it will warn that print(x) may find x undefined.

One possible solution (which I haven’t seen yet, but maybe I missed it) would be to have a variant of ExitStack, e.g. NonSuppressingExitStack, whose __exit__ never returns True. Its __exit__ is declared using -> None, and if you pass it a context manager whose __exit__ is declared using -> bool that’s a type error. More over, if at runtime it encounters a context manager whose __exit__ returns True, it does something to tell the user about the problem – maybe it logs a warning, or maybe it raises a different exception (I haven’t thought about this enough to have a solid opionion).

Independently, maybe we could have an else clause on with statements. But what would it mean? IIUC @NeilGirdhar’s proposal is to have it execute only in the situation where __exit__ is passed an exception and returns True in order to suppress it. IMO this would be pretty confusing, since the other compound statement that is most similar to with, i.e. try, already has an else clause that pretty much means the opposite (it is executed only if there are no exceptions). So I am not in favor of that. (And that means maybe I’m barking up the wrong thread?)

15 Likes

What’s really needed here is for ExitStack to somehow say “my return type is the union of all my underlying return types”. But I have absolutely zero idea of how that could be implemented :smiley:

2 Likes

Just for the curious, I think that would probably require this: https://en.wikipedia.org/wiki/Dependent_type

…that’s about where my type theory knowledge ends, but I do find it interesting to learn about. :slightly_smiling_face:

Yeah, with every call to enter_context() updating the effective type of the context manager. But I suspect that this is getting beyond the realm of “type checking” and edging towards more complex static analysis systems (see eg Coverity, which traces different code paths in detail and can determine if there’s one specific path that can lead to problems). For example, should this trip a type error?

with ExitStack() as ctx:
    if random.randrange(2):
        x = 1
        ctx.enter_context(suppress(ZeroDivisionError))
    1/0
print(x)

Okay, this is a faintly ridiculous example, but it highlights the inherent problem. Personally, I would be fine with the “type” of this being “yes, this is capable of suppressing exceptions” - anything else is asking too much.

1 Like

I think the problem with that is that the enter_context() calls are dynamic. If the underlying context managers were present in the ExitStack constructor, you could probably do this with typevars. But here we’re calling cm.enter_context(subcm), and we’d want those calls to affect the type of cm. That makes me think of how (in mypy, at least) we have a special case for

xs = []
for ...:
    x = ...
    xs.append(x)

where the inferred type for x will affect the type inferred for xs – this is hard, and except for the very simplest cases we require you to provide the answer by starting with

xs: list[SomeType] = []

so that the type of x is checked against SomeType instead of changing it.

If the answer was dependent types, since we don’t have those, we’d end up needing my proposed solution anyway.

Also, the enter_context() calls might occur inside some other thing that is called, e.g.

with ExitStack() as cm:
    foo(cm)

where foo() is something like this:

def foo(cm: ExitStack):
    cm.enter_context(...)

Well, maybe this could be solved by making ExitStack generic:

class ExitStack[T: bool|None]:
    ...
    def __exit__(self, ...) -> T: ...

If this could work we’d be able to say

type NonSuppressingExitStack = ExitStack[None]
2 Likes

Does anyone have examples of a context manager suppressing exceptions? When would that be a good idea?

2 Likes

I think it’s one of the more questionable ideas from PEP 343. Maybe the PEP has a good use case though?

See e.g. pytest’s raises (or stdlib contextlib.supress):
https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest-raises

2 Likes

Lots of popular third party libraries seem to be using contextlib.suppress which is doing exactly this, right?

https://github.com/search?q=contextlib.suppress+language%3APython&type=code&l=Python

Looks like it’s also very handy for debugging, too.

The difference there is that the programmer is declaring they don’t care about any exceptions – sort of like a try: ... / except: pass.

1 Like

Wondering what @NeilGirdhar thinks of my proposal.

This one, right? I like it. If you go that route, you may want to make it so that ExitStack[None] only accepts calling enter_context on context managers that return None/Literal[False]?

Also, it’s similar to Paul’s suggestion and applying it: MustNotSuppress(ExitStack). With Paul’s approach, the exit stack can suppress if it wants, but it will raise an error, so you get a runtime check.

I like both ideas.

2 Likes

Okay, can you turn it into an actual proposal? Does it just affect typeshed, or would we have to change contextlib.py?

1 Like

Still waiting for @NeilGirdhar to turn my idea into a proposal. It’s not going to write itself!

I worry about the impact of changing ExitStack to be generic on third party libraries, but I don’t think this would require any changes to contextlib.py, since ExitStack inherits from AbstractContextManager which is subscriptable at runtime.

The diff in typeshed should be fairly small (although it would require defining a new Protocol for the context managers that is generic in the return type of __exit__, I don’t think it’s worth changing AbstractContextManager to be generic in two type vars, at least not until PEP696 is accepted and we can provide a default of bool | None for the second type var), but whether or not this would introduce a lot of new type errors would only really show once someone opens a pull request on typeshed and gets the mypy primer report back.

The impact could be minimized once PEP696 has been accepted, then you could default ExitStack() to ExitStack[bool | None] to ensure backwards compatibility, so if the impact is too large we could defer changing this.

Edit: I ended up drafting a PR to look at the impact: Make `ExitStack` generic in return type of `__exit__` by Daverball · Pull Request #11048 · python/typeshed · GitHub

As expected the mypy primer output is fairly large, I would personally defer this until PEP696.

I’m really sorry about the delay. I’ve been falling behind on my work, and trying to balance priorities.

I did spend some time thinking about the write up, and wanted to follow up about some questions about this proposal (I’m open to hearing anyone’s thoughts).

I understand that the generic exit stack solution solves to the problem for ExitStack. My question is: Is this the only significant context manager that suppresses exceptions? (It’s okay to just declare “yes”; I just don’t know.)

By contrast, the NoSuppress wrapper prevents suppression for all context managers.

Additonally, the NoSuppress wrapper actually verifies that no suppression took place (and raises otherwise) whereas the generic exit stack just makes a promise without checking it? Or am I mistaken about this?

The generic version would prevent you from calling enter_context on context managers that have the wrong __exit__ return type, so it should be fairly safe. Although the NoSuppress wrapper has its own merit of enforcing this at runtime. It certainly seems like the more immediately viable solution right now, considering the mypy primer output from my Draft PR on typeshed.

Oh, does it have a separate name? I thought we were still calling it ExitStack? What I mean is, how does it do that?