Add an else clause to with statements to detect early termination

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?

You achieve this by using a more strict Protocol on the input parameter of enter_context:

class ExitStack(Generic[_ExitT_co], metaclass=abc.ABCMeta):
    def enter_context(self, cm: _AbstractContextManager[_T, _ExitT_co]) -> _T: ...
   ...
1 Like

Got it, so we have to make AbstractContextManager generic on one more parameter. Yeah, that works.

So the upside of the generic context manager solution would also be efficiency since it has no runtime cost, and all the checking is done statically.

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.

1 Like

No worries! Balanced priorities are important.

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. :slight_smile:

Yes, you’re right. I think we’re on the same page.

But consider an ordinary user-defined context manager:

@contextlib.contextmanager
def f() -> Generator[None, None, None]:
  yield

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:

@contextlib.contextmanager
def f() -> Generator[None, None, None]:
  try:
    yield
  except:
    pass

Hm, you’re right. I’m guessing that could be solved by making contextmanager generic too, or perhaps by having a variant of it (it’s late here).

1 Like

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.