Add an else clause to with statements to detect early termination

Ideas
1

PyRight rightly complains when ExitStack is used:

with ExitStack() as stack:
  for x in l:
    stack.enter_context(x)
  y = f()
print(y)  # y is possibly unbound!

This is because there’s no way to know whether one of the context managers didn’t suppress an exception raised by another context manager.

How should this code be made safe?
You might try:

y = some_sentinel
with ExitStack() as stack:
  for x in l:
    stack.enter_context(x)
  y = f()
assert y is not some_sentinel
print(y)  # no error

But this gets annoying when annotating y, and when there are many variables. You might try adding a flag:

flag = False
with ExitStack() as stack:
  for x in l:
    stack.enter_context(x)
  y = f()
  flag = True
assert flag
print(y)  # y is possibly unbound!

but this doesn’t fix the type error.

Would it make sense to add either a flag or a parameter to ExitStack that says that it cannot exit early? Either ExitStack(no_early_exit=True) or ExitStack(on_early_exit=RuntimeError("blah"))? Although I’m not sure how the annotation for ExitStack would work to let type checkers know that __exit__ always return False when that flag is given. I guess a final option would be to add something like:

In [15]: class SafeExitStack(ExitStack):
    ...:   def __exit__(
    ...:       self,
    ...:       __exc_type: type[BaseException] | None,
    ...:       __exc_value: BaseException | None,
    ...:       __traceback: TracebackType | None
    ...:       ) -> Literal[False]:
    ...:     retval = super().__exit__(__exc_type, __exc_value, __traceback)
    ...:     if retval:
    ...:       raise RuntimeError("Internal error: suppressed excpeption")
    ...:     return False

This would give a simple transformation of the above code to silence PyRight.

3 Likes
2

Use whatever the magic comment is to suppress the error (linters use # noqa, black uses # fmt: off, I don’t know what type checkers use).

Everyone is happy with such an “escape clause” for formatters and linters, why isn’t that enough for type checkers?

2 Likes
3

But the type checker isn’t complaining spuriously. Context managers might suppress exceptions and leave variables unbound. I think it would be better to raise an exception in this case if you expect variables to be bound.

4

So what do you want here? Your example is artificial so I can’t tell what you’re trying to do.

If you know (from the documentation) that your usage is safe, then suppress, like I said. If you don’t know, then use a flag like you show, and suppress (because you know that’s now safe to do).

1 Like
5

I tried

from contextlib import ExitStack

filenames = ("test", "test2")
with ExitStack() as stack:
    for x in filenames:
        stack.enter_context(open(x))
    y = 2

print(y)

and got no complaints, so the behaviour between type checkers is not consistent in this case.

1 Like
6

You’re right: not all type checkers check for unbound variables.

My suggestion isn’t motivated by a problem with type checkers though. The problem is that there is no easy way to both:

  • guarantee that a context manager will run all of the code within it, and
  • make it obvious to type checkers that it has done so.

I just thought of another solution, which would fix the problem for all context managers. What if we added an else clause to with statements that triggers when there was a suppressed exception:

from contextlib import contextmanager

@contextmanager
def f():
    try:
        yield
    except:
        pass


with f():
    raise KeyError
    x = 1
else:
    raise RuntimeError("x is unbound although no exception would otherwise be"
                       "raised")

This pattern would satisfy the two goals, and would allow you to convert any general context manager into one that guarantees that the code within it has completed as far as I can tell.

If you don’t know, then use a flag like you show

Yes, using a flag works, but it’s very inelegant since you then need to suppress errors in multiple places. I think the else clause would be more elegant.

3 Likes
7

I think I agree with Paul here, use # type: ignore and move on. ISTM that if one of the context managers raises an error there is a bug in your code, and if I were to write a type checker (which I admittedly couldn’t, because I don’t know enough about typing in general) I would assume that the context managers work as intended and consider this failure case a runtime bug.

Also, can’t you solve it by giving y a value before the context manager in this case?

2 Likes
8

I’m not sure what you mean? The context managers are working as intended even when they swallow exceptions and terminate early.

I think people are really underestimating the “error at a distance” that can happen when one type checker swallows another’s exception and the with statement code doesn’t complete. Then there are downstream errors caused by this. I think the reason it’s easy to underestimate this is because exceptions are pretty rare, and context managers that swallow exceptions are extremely rare. So it’s unlikely to see a bug like this. Yet it’s easy for type checkers to detect this possibility, which induces some defensive strategy. This proposal is about making that defensive strategy as painless as possible.

You still want to ensure that the context manager has completed all of the code within it. But yes giving y a value (which may be difficult) would silence the type checker.

So, the current best solution appears to be:

  • adding a flag,
  • setting it at the end of the with statement,
  • checking it and raising after the with statement, and
  • ignoring any unbound variable errors.

I guess I’m alone then in thinking the else clause is simpler.

4 Likes
9

Or

  • Setting the variable before the with statement (to a sentinel value like None if there’s no clear fallback)

(and nothing else, unless you want to check for a sentinel before using the variable later).

I think you’re making this whole thing a lot harder than it needs to be, out of a desire to code around every type checker message.

1 Like
10

I’m either misunderstanding you, or I don’t see what the problem is (possibly both!), so I’ll tap out until some of the typing gurus chime in.

However, have you considered asking this question in a more open way in the typing category? I.e., without suggesting changes to the language (which will always be met with some resistance) just describe you problem, that mypy and pyright behave differently and ask if there are aome nice ways to solve it currently.

I think that would be more productive than jumping straight into changing the language.

2 Likes
11

Fair enough, so the best solution then is to

  • Set all of the variables that will be used to a sentinel, and
  • assert that each variable is not equal to that sentinel.

So for example,

from contextlib import contextmanager

@contextmanager
def f():
    try:
        yield
    except:
        pass

class Sentinel: pass
x: int | Sentinel = Sentinel()
with f():
    raise KeyError
    x = 1
if isinstance(x, Sentinel):
    raise RuntimeError("x is unbound although no exception would otherwise be"
                       "raised")

Not beautiful, but that works. If you know x is not none, then you can replace Sentinel with None.

It’s annoying in general code to annotate x explicitly (here int) where it would otherwise be implicitly annotated by code. For example by a line like x = g(...).

It’s also annoying since if x is reused within the with statement block, you’ll have to assert that it’s not the sentinel. So more assertions.

Good point. Maybe it would help to look at concrete code? Consider this code. After the with statement, there’s no way to know that out_avals and built are bound.

Now imagine trying to change the code to make that promise. If I could just add an else clause to the with statement, I could be done in 1 minute. Otherwise, I have to set out_avals and built to sentinels (it’s not obvious if they can be none or not), and then I have check them against those sentinels and assert if they match. I also have to give them explicit annotations, but it’s really hard to do that since it’s not obvious what type they are. And people reading the code may not understand why the sentinels even exist since it appears that they are unconditionally set.

Again, I don’t think this is a problem with type checkers. They’re detecting a problem that I think should be solved. The question is what is the easiest way to solve the problem. So far, the sentinel idea seems to be best, but trying to do that on the above code seems over-complicated to me.

5 Likes
12

You can construct an artificial example to make any suggestion look arbitrarily clumsy. In real world code I imagine a much simpler version would almost certainly be sufficient. And in fact, given that I’ve never even seen a case like this come up in real world code, I think that a bit of clumsiness in the odd complex case is perfectly fine.

Again, so just don’t. If type checkers can’t work out what you’re doing, that’s a limitation of the type checkers. Your code is fine at this point. Or if you want to use explicit types, then you’re free to do so, but in that case, you accepted the need for the extra verbosity.

Are you saying that type checkers can’t infer that x is an int, given:

  • You’ve explicitly declared x to be int | Sentinel (or, you’ve assigned x in 2 places, once as the sentinel and once as an int).
  • You’ve raised an exception if x is the sentinel.

Surely inferring that x is an int after the type check is straightforward for a type checker? And if it’s not, then isn’t that simply a consequence of the fact that type checkers are limited because Python is at heart a dynamic language?

If it’s that important to guide the type checkers, why not ask for a hint-style construct that lets you say to the type checker “after this point, x is always an int”?

1 Like
13

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.

14

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
15

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.

16

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
17

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.

18

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
19

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.

20

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