Add an else clause to with statements to detect early termination

That’s a fair point. I edited in to the last comment a real world example. There are other similar examples in that code base, and I’ve seen similar examples in my code.

How would you correct it so that:

  • it guarantees that the variables are bound, and
  • passes reasonable type checking?

The current status quo is that type checkers implicitly annotate variables to the RHS type of their first assignment, which is generally beneficial. And they consider a type error to assign to such implicitly annotated variables other types. I recognize that this rule is inconvenient for this example, but there are plenty of other examples where it is convenient.

I’m happy to debate the merits of this if you want, but I think it’s tangent to this issue.

Thanks for the real-world example. My feeling is that the code needs a bigger redesign - just catching the “name is undefined” error is patching over a deeper problem.

Why does pe.trace_to_jaxpr_dynamic fail, and why does core.extend_axis_env suppress that exception? In this context, at least, suppressing the exception is obviously wrong[1], so maybe the fault is there?

I don’t think ExitStack is the issue here - you’d have the same problem with a normal context manager that suppressed an exception (such as contextlib.suppress). Nor do I think an else clause is the answer - suppressing an exception is a significant thing to do, and you should not do it casually. Having a way to “let the caller work out what to do” is simply a way of making an incompletely thought through design the user’s problem.

The comment in the documentation for contextlib.suppress is important here:

As with any other mechanism that completely suppresses exceptions, this context manager should be used only to cover very specific errors where silently continuing with program execution is known to be the right thing to do.

(my emphasis)

So yes, I think the type checker picked up on a genuine potential problem. Your job is now to work out how to fix that problem. Whether you patch over it with some slightly clumsy, but effective (in a practical sense) code, or take a step back and look at the overall design, is your choice as the developer.


  1. because the subsequent code isn’t prepared for an unboud variable ↩︎

1 Like

I have no idea whether it can fail or whether the other context manager can suppress the exception. And neither do type checkers. And neither do ordinary readers of the code. That’s why I’m motivated to easily check for that possibility.

Yes, I completely agree. I came to the same conclusion and changed the title of this issue.

I don’t understand your point here. Can you elaborate?

(Just so it’s clear else isn’t suppressing an exception; else is catching the case when an exception was suppressed.)

I’m not the person who wrote this code. I was just curious how easy it would be to patch over this error. I still think the else clause would be an ideal solution (just two lines: else: raise RuntimeError), but I’m interested in what you think would be the best solution.

I don’t understand your point here. Can you elaborate?

I’m not sure what’s to elaborate (see the contextlib.suppress quote for a bit of elaboration). If you, as the author of a context manager, catch and suppress an expection, you need to understand that what you are doing is arranging for a with statement to exit early. Therefore, you should only suppress exceptions that you are sure cannot occur in unexpected situations, and you should document the limitations carefully. Better still, you should never include in your public API a context manager that suppresses an exception that can be triggered using your public API (i.e., only use exception suppression in carefully controlled internal code).

The else clause isn’t the answer because “fix the context manager”[1] is the actual answer.

Patch over the error. The library you’re using has a usability bug - by all means report it to them, but if it’s not your code, you have to deal with the problem as best you can until the issue is fixed at the source.

In effect, what you’re doing here is trying to persuade the Python developers to change the language because you’re not able to persuade the authors of this library to change their API design. Unless there’s an argument for this proposal that doesn’t boil down to “to let people work around bad library design rather than get the library fixed”, I don’t think there’s a good case for the feature.


  1. or get the context manager’s author to fix it, or don’t use that context manager… ↩︎

1 Like

Yes.

Even if you do this, the caller doesn’t know that you’ve done this. And the caller has no easy way to verify that the with statement hasn’t exited early.

That’s a fair idea, but there’s no way to promise this. You can promise that you suppress nothing (by annotating __exit__ as returning Literal[False], but that doesn’t help users of ExitStack, which can’t make that annotation.

Therefore, neither the authors nor the type checkers of calling code can easily check these promises. That’s the problem. As an author, how do you check? You have carefully read documentation or follow code, which is very time consuming. And the type checker has no way to know that this invariant you’re proposing is satisfied.

ExitStack cannot satisfy your proposed criteria of never suppressing exceptions. How should it be fixed?

I don’t see where the “bad library design” is here to be honest. The context managers probably do what you say. It’s just that there’s no easy way for authors and type checkers to check that they do what you say. My proposal is about making it easy for authors and type checkers to check.

I didn’t mean to be that absolute. You “fix” the issue by documenting your interface clearly. You can never protect against everything (KeyboardInterrupt or MemoryError can happen pretty much anywhere).

What exceptions get suppressed in your example? If you can’t find that out easily from the documentation, that’s the sort of issue I’m talking about. When do those exceptions occur? Can you make sure the conditions that could cause them don’t happen?

Yes, it could be time consuming. But more so than trying to get a language change implemented, and then desupporting all versions of Python prior to the one the new feature is available in?

To be worth a new language feature, this problem should be common enough that there should be many examples we can all relate to, not just a single bit of problem code. Or, the feature should be something that people can immediately relate to, and understand the usefulness of, in another way.

Also, let’s be clear here, when I say “bad design” I’m not saying that can’t happen. Pretty much every software project is “badly designed” at some level. Looking for perfection is futile.

I’m not seeing what would make it easy to check. The else clause seems designed to help with not checking, but actually just reacting. Whoops, something got suppressed, but we don’t know exactly what and we don’t know how much of the code in the with statement got executed.

What if core.extend_axis_env suppressed ZeroDivisionError, and mlir_module_to_xla_computation divided by zero for some reason? How would you write your else clause so that it handled that case properly, as well as the unbound name case? And if your answer is “by reporting the problem and stopping” they why suppress exceptions at all?

And if you want to avoid suppressing exceptions, use a helper:

>>> class MustNotSuppress:
...     def __init__(self, inner):
...         self.inner = inner
...     def __enter__(self, *args, **kw):
...         return self.inner.__enter__(*args, **kw)
...     def __exit__(self, *args, **kw):
...         ret = self.inner.__exit__(*args, **kw)
...         if ret: raise RuntimeError("Context manager suppressed an exception!")
...         return ret
...
>>> with MustNotSuppress(suppress(ZeroDivisionError)):
...   1/0
...
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
ZeroDivisionError: division by zero

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 8, in __exit__
RuntimeError: Context manager suppressed an exception!
2 Likes

I just don’t think it’s realistic to expect that all code is going to conform to this. And it doesn’t help when you’re editing code to blame the code as being poorly documented. You still have to fix the error, and the idea of reading docs that don’t exist that you’re proposing very time consuming.

The idea is to make things easier for future Python programmers.

It’s not just a single bit of code, it’s probably practically every use of ExitStack, and every other context manager that may suppress exceptions (ones that support nesting for example).

Right, and we don’t care what happened because it’s a runtime error. This is exactly the same as when you do:

x = f()
assert isinstance(x, int)

This is also " Whoops, something got returned, but we don’t know exactly what". It’s an error, and we just raise unconditionally. Same idea with the else clause. This allows type checkers to proceed with the invariant.

Just

else:
    raise RuntimeError

Same as before.

The decision to suppress the exception is made by a context manager. The caller is the one who just can’t deal with that aspect of the context manager. So the caller wants to impose an additional constraint.

Yes, great idea. This is the best solution so far in my opinion. Still, I think an else clause is simpler and more efficient than a NoSuppress context manager. The NoSuppress would be a great utility to have right now.

So you seem to be suggesting that suppressing exceptions is either always bad, or always needs the caller to take care to not include code in the with statement that would cause problems if omitted.

I guess we’ll just have to agree to either differ, or not quite understand each other’s point. Either way, I’m glad the NoSuppress idea was of use.

1 Like

Why does pyright only do this for ExitStack?

I just tried this:

from contextlib import ExitStack, contextmanager

@contextmanager
def c():
    try:
        yield
    except ZeroDivisionError:
        pass

def f():
    return 1/0

with c():
    y = f()

print(y)  # y is possibly unbound!

I get:

$ pyright y.py
0 errors, 0 warnings, 0 informations 
$ python y.py
Traceback (most recent call last):
  File "/home/oscar/current/active/sympy/y.py", line 16, in <module>
    print(y)  # y is possibly unbound!
          ^
NameError: name 'y' is not defined

But if I use ExtStack then pyright complains:

from contextlib import ExitStack, contextmanager

@contextmanager
def c():
    try:
        yield
    except ZeroDivisionError:
        pass

def f():
    return 1/0

with ExitStack() as stack:
    stack.enter_context(c())
    y = f()

print(y)  # y is possibly unbound!

That gives:

$ pyright y.py
/home/oscar/current/active/sympy/y.py
  /home/oscar/current/active/sympy/y.py:17:7 - error: "y" is possibly unbound (reportUnboundVariable)
1 error, 0 warnings, 0 informations

Mostly type checkers seem to ignore exceptions so I am not sure why pyright worries about them in this case (if that is what is happening rather than just a bug).

The reason ExitStack is different than, say, contextlib.closing is because ExitStack.__exit__ return bool whereas closing returns None (or Literal[False] would be fine).

The exceptions are ignored, but the possibility that an exception can lead to unbound variables is caught by PyRight.

So is this related to the fact that type checkers don’t really handle the idea of “truthy” values?

IMO the whole discussion about what type checkers do is probably a distraction here. Trying to suppress type checker warnings, or debating whether a type checker warning is “right” or not is probably pointless if we can’t even expect type checkers to consistently handle closing and ExitStack.

I suggest we stick to the non-typing point here, about whether there’s a need for language support for determining whether a context manager suppressed an exception. Bringing type checkers into the discussion is likely too controversial to be helpful.

1 Like

No. ExitStack is annotated bool because it’s uncertain whether its enclosed context managers will suppress exceptions. It merely forwards their behavior.

The type checkers are doing the right thing though, so I don’t understand this concern.

The reason that I think type checkers are relevant is that I’m looking for a solution that is clear to both humans and automated tools. The else clause is one such solution. The NoSuppress wrapper is another. “Read the documentation” is not great for humans and doesn’t work for automated tools.

I just demonstrated an example above where pyright checks the code fine even though it gives NameError at runtime.

Why does pyright only detect this with ExitStack and not other context managers created with @contextmanager?

1 Like

Ah, sorry I missed that! It appears that the contextmanager decorator doesn’t produce the right annotation for __exit__. That seems to me to be a bug in PyRight or perhaps this was a conscious decision on their part to prevent false positives at the cost of the false negative you’re seeing? Might be worth asking @erictraut?

It’s a fact that the following code block

with ExitStack() as stack:
    stack.enter_context(c())
    y = f()
print(y)

can leave variable y undefined at runtime. So, pyright’s warning about this is correct (in a way). It’s also a fact that not all typecheckers handle this the same way (mypy doesn’t complain about this).

The most readable way to fix this, imo, is to explicitly define y upfront – similar to the sentinel idea, (though in practice I expect you usually don’t need to define a special class).

My main reasoning is that that code block seems to be kind of non-idiomatic code: You are iterating over a stack of context managers and, after doing so, but within the outer context manager, introducing other variables and making other calls. That’s all fine, but in that case, shouldn’t the burden be on this code to make sure that y is always well-defined and that any exceptions are handled?

If I consider this, then I’m very much against the “else” solution. Having the y variable (and extra calls) makes it harder to follow what’s going on, but using an “else” clause seems to make this only worse. I don’t see how this could possibly work in non-trivial cases (with multiple new variables and calls, each of which could themselves either raise an exception or not). It would be much clearer, imo, to explicitly take care of any of those variables upfront.

4 Likes

What did you think of Paul’s idea?

with NoSuppress(ExitStack()) as stack:
     ...

I don’t know – Can that directly be applied to ExitStack? ExitStack iterates over context-managers, making it more complicated than a simple one, so I don’t quite understand how this could work or solve the original issue (with y being possibly unbound when first defined inside the block and used afterwards). I think you’d need a special derived or alternative class for ExitStack to apply that idea.

(Another issue is that I wonder if that wouldn’t negate the whole purpose of ExitStack? ExitStack itself doesn’t suppress exceptions, so it seems this construct is used to second-guess the exception handling in the stack of context managers…)

My personal preference is simply to define something like y upfront. This may be unelegant, but the lack of elegance in this case is caused by (what I see as) non-idiomatic use of ExitStack.

Yeah, it just works as is. It will raise an exception if any context manager in the ExitStack swallows an exception. I think it’s the best solution we have right now.

Any context manager within the exit stack can suppress exceptions. Wrapping it like this doesn’t stop it from working as it normally does.

That’s just the problem, I think. The wrapped ExitStack doesn’t work as a normal ExitStack, since now you’d raise a RuntimeError on any earlier suppressed exception. You would not be able to do anything with that RuntimeError, except pass it on – basically just let it crash since suppressing it now makes no sense – and it’s not clear which context manager caused it or what the actual error was. The code snippet as-is (the NoSuppress code) makes it impossible to know this, since __exit__ doesn’t know about the original exception (for ExitStack there is none at that level). So, that’s why I was wondering if you don’t need a special implementation.

Btw I think it could be made to work with a special implementation - taking care of any of those issues - but I would still avoid ever using any construct like that. If I would not be satisfied about exception handling in those other context managers (the ones being iterated over), I would try to address this at that level - by using other context managers or modifying the originial ones.

On the other hand, if you’re ok with getting an indiscriminate RuntimeError - of course, that’s your choice.
But wouldn’t it be better to get a precise error?

I think you’re misunderstanding how it works? It runs the exact same code and stops at the exact same time. It just now raises after all the code that it would run has run. Maybe you should try to play with it to get a feel for how it works?

The idea is to let it crash, and it doesn’t matter which context manager caused it or what the error was. It’s just like isinstance(x, int). No one cares what x is, or what happens next. The purpose of this kind of assertion is to act as an assertion.

The point is that the context managers aren’t doing anything wrong. They may not even suppress any exceptions. The purpose of this construct is to give the calling code a mechanism for asserting that they don’t suppress exceptions.

It’s not “indiscriminate” —any more than an instance assertion is indiscriminate.