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 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.
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.)
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.
Yes, although I felt that would be too invasive of a change so I created a new Protocol instead until we can default _ExitT_co to bool | None using PEP696.
That’s not the right question to ask though, is it? The specific problem with ExitStack is not that it suppresses exceptions. Its problem is that it is declared as always (potentially) suppressing exceptions, even in situations where the user actually can prove that it never will (i.e., when only non-suppressing context managers are actually pushed upon its stack).
I think you meant MustNotSuppress? The generic version I propose ensures the same condition at type-check time: its push() method only accepts context managers whose declared __exit__() type is None. So I see little advantage to MustNotSuppress unless you want the whole thing to happen at runtime instead of compile time – clearly, if that’s your requirement, generics aren’t going to help.
This also produces a context manager with an exit defined to return bool | None, and therefore it is also declared as always (potentially) suppressing exceptions? So doesn’t this problem affect all such context managers too?
After all, the type checker doesn’t know that you didn’t do this:
Making the decorator generic should actually be the easier/less invasive change. We could probably do that right now without fearing too many additional mypy primer hits. Although rather than defining a completely new Protocol for this one, it should probably be one that inherits from AbstractContextManager to clue the type checker into the fact, that the returned context manager derives from that class.
Edit: Wait, actually the decorator version is a different case entirely… I don’t think we can do this without special-casing in the type checker. In this case it isn’t even that the generator returns a special value to prevent bubbling the exception. It would require binding the exit type of the returned contextmanager, based on what the function does internally, there’s no way to model this with static typing right now and there likely never will be, so it will require special casing.
But a 90/10 solution in a type checker would likely be fairly little effort, you could just scan the generator for a try node with at least one except block, if it contains one you keep the return type as bool | None otherwise you can specialize it to None or Literal[False] (what it actually would return).
This would allow some false negatives in eccentric cases, but should still cover most context managers created this way.
Couldn’t type checkers use their reachability evaluator to figure out if the end of every except clause within a function is unreachable? If so, then that function is almost surely not suppressing any exceptions?
With such an evaluation, could the type checker mark context managers and generators as suppressing or non-suppressing, and then use those marks to say that a with clause applied to a non-suppressing context manager must reach the end of the block?
Sure, you can make the implementation arbitrarily complex to cover more cases. It’s just a question of whether it’s worth doing. If this more complex solution can just re-use existing reachability code, then maybe so. That will be up to the maintainers of the various type checkers to decide.
I prefer simple solutions that cover the majority of cases over complex solutions that cover most or all cases until there is a proven need/desire for the latter. I personally have written extremely few functions using the contextmanager decorator that use a try with anything other than a finally.
In complex cases I usually prefer to write the class over the generator.