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!
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.
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.
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?
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.
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.
Wasn’t this exact thing discussed here a month or two ago? I though the conclusion was that the desire for this feature had to do with a desire to handle exceptions at basically the wrong scope. In other words, if you’re wanting a context manager to tell you when it suppresses a certain exception then you’re not using that context manager right (or the context manager needs to be redesigned if that is in your control).
Sorry for double post, but more pointedly: context managers should not be SUPPRESSING exceptions. Nothing should ever suppress exceptions. Rather exceptions should be HANDLED. In my mind handling an exception means that downstream code in the program should be able to run without ever knowing that an exception was raised and handled (this doesn’t exclude logging the information about an exception being raised and using that information to change downstream behavior). If this is not the case then this feels like a design flaw.
I remember that too. To be honest, I didn’t understand his proposal, but reading the thread again, you are right that we are asking for the same thing. I think my arguments were very different though.
Yes, we agree that it’s a perfectly fine use of context managers to have them suppress exceptions. But, the reason to find out if they suppressed an exception isn’t to do anything special for that exception. I agree that any special behavior should happen in the handler.
The reason to find out about suppression is to know whether the code in the with statement was entirely run. The reason we want to know that is so that we know if we can procede with code that relies on it.
I agree, but unfortunately, there’s no way to know that the context managers haven’t handled an exception and thereby exited from a with block early. So the calling code does need to know whether the exception has been handled. At least as an assertion in the case where handling is supposed to be impossible.
That’s like saying that assert isinstance(x, int) indicates a design flaw since why is someone not passing x as an int. The assertion is for humans and type checkers to catch errors as early as possible. Same thing here: we just want to assert that the with statement has been run entirely.
Calling it a design flaw if it hasn’t been run, doesn’t help us write correct code. Please look at the code I linked and ask yourself how you would edit it to guarantee that the variables are bound and have correct type annotations.
Without either checked exceptions or the use of result types that are themselves sum types, sentinel values for possibly unbound and handling those sentinels is the best available option.
Simply adding type-syntax on top of this without corresponding enforcement of it doesn’t actually change anything over just a type: ignore because neither would guarantee that the context manager passed in adheres to the expectations. This is because it’s generally not possible to statically determine all possible exceptions have been handled in python without creating other problems such as suppressing something which usually shouldn’t be. Various signal handlers (most visibly, the default handling of KeyboardInterrupt) show this.
Ehm, that’s precisely what I am saying there… The internal stack runs the same code, yes, because that’s not touched, but the wrapped stack behaves differently.
I think there is an important difference. In the case of a simple assertion, even if there is no clear error message, it’s clear what the error is when you look at the code. (Plus some followup code could actually try to handle this.) In this case (wrapped ExitStack) there is no way to find out what caused the error, or what the original error is, unless you actually tweak the ExitStack code. The simple wrapper around that code (around the ExitStack object) hides any errors, it will just tell you that something somewhere errored. If that’s all you want - ok, I cannot argue with that. It’s just a construct I would never use myself (and definitely never in any library code), since I do want to know where runtime errors come from.
You might argue against that and say that without using a construct like that the errors would be even more hidden (since they would be suppressed). So, from my side it might be better to say I’d never use ExitStack on a sequence of context managers if it’s possible that those cm’s can suppress important exceptions… So, in that sense, I would never run into your problem
I really think we’re talking past each other here. The wrapped stack does not behave differently except when there is suppression, and then the only difference is that it raises an exception that otherwise wouldn’t be raised. So in the usual case (no suppression) nothing has happened, and in the unusual case, you get an exception, which you can just treat as an assertion to stop the program if you like.
I don’t see the difference: it’s clear that x isn’t an int and it should be; Similarly, it’s clear that there was suppression when there shouldn’t have been.
Sure, just like you won’t know why x is not an int unless you adjust your code (adding prints, adding assertions to callers, etc.)
It doesn’t hide anything that was previously visible. I don’t know where you’re getting this idea?
Right, well, maybe you should look at the code I linked and think about how you would edit it to ensure that the variables are bound. Of course, any idea doesn’t make sense without motivating examples.
In that case, you actually should be using NoSuppress(ExitStack) so that you can guarantee that they can’t suppress exceptions.
And FYI this has nothing to do with the “importance” of exceptions. The nature of exceptions are irrelevant. This is all about suppressed exceptions resulting in code not being run that would otherwise be run.
What you’re proposing is to use ExitStack when you think that no exceptions will be suppressed. But you don’t want to actually assert that. You want to just check when you write the code and hope that nothing changes.
Why not add an assertion to verify that no exceptions are suppressed? Why not assert on your assumptions?
I understand. But you are making an assumption since you’re assuming that there’s no suppression. This proposal is about adding an assertion (that works both at runtime and statically) to check that assumption.
Even if they don’t when you check the source code, things can change. Or you check and overlook something. Adding assertions is just prudent.
Yes, fair enough. But this proposal is about asserting this one thing. If you look at the linked source code, I think it will be clear why I want to assert this thing.
