Improve type inference of context manager that swallow exceptions

[jacobbogdanov]

This fixes #764

The typeshed has this to say:

When adding type annotations for context manager classes, annotate
the return type of __exit__ as bool only if the context manager
sometimes suppresses exceptions -- if it sometimes returns True
at runtime. If the context manager never suppresses exceptions,
have the return type be either None or Optional[bool]. If you
are not sure whether exceptions are suppressed or not or if the
context manager is meant to be subclassed, pick Optional[bool].
See python/mypy#7214 for more details.

This PR makes pyright follow the statement above, by adding additional code flow analysis to track with blocks and check the return type of the context managers.

The tests are based on python/mypy#7317 but have been modified to more closely resemble the tests in this repo.

[ghost]

All CLA requirements met.

[erictraut]

Thanks for this contribution! I'm really impressed with this PR. The code flow engine is one of the more complex pieces of code in Pyright, and you managed to figure it out. That said, I see some issues with your PR that I'd like to address before I merge it. I'll try to get to it in the next few days. Thanks again!

[jacobbogdanov]

I would be lying if I said that I actually understand the code flow engine, but I was able to piece together just enough for this one use case haha. I'm happy to address any and all comments, and am not in a rush, so take your time.

[erictraut]

I spent some additional time reviewing your solution. Unfortunately, it's not going to work as written. The problem is that the code you added to the isFlowNodeReachableRecursive method references the type cache, but it doesn't properly handle the case where the cache hasn't been populated. So the implementation will fail in a non-deterministic manner depending on the order in which nodes happen to be visited. To address this, it would need to evaluate the type and populate the type cache, but isFlowNodeReachableRecursive method isn't allowed to evaluate types because it would produce a circular dependency. In other words, fixing this problem in the obvious manner will result in infinite recursion within the analyzer. To avoid this circular dependency, reachability of nodes within the code flow graph must be determined without evaluating types of nodes within this graph.

I'll need to think more about whether there's a creative way around this.

Some additional background, in case you're interested. Mypy is a multi-pass analyzer — i.e. it performs type analysis for a source file as many times as necessary for all type information to "converge". Earlier versions of Pyright also used this technique, but it's very inefficient. Later versions of Pyright employ lazy evaluation, which means that the type of each node in the parse tree is evaluated only once — and only if that type is actually needed. Plus, analysis is performed in whatever order is needed. While lazy evaluation is very efficient, it places some constraints on the analyzer. One such constraint is that the reachability of control flow nodes can't depend on type evaluation.

One other (smaller) issue with your solution is that it won't properly detect or report cases where variables are potentially unbound. Take for example:

with SwallowsExceptions():
   maybe_generate_exception()
   b = 3

return b + 2

In this case, b is possibly unbound because it was assigned a value only after the call to maybe_generate_exception. To address this, the "with" statement would need to be handled in a manner similar to a try/except statement where each sub-statement within it is treated as though it could raise an exception.

I'll continue to think about how to work around the reachability limitation.

[jacobbogdanov]

Thank you for spending the time to review this patch and explaining in such great detail the problem space!

It makes sense that in most cases reachability can be determined statically without evaluation of all types. Looking at the code again I see that isCallNoReturn employs a few tricks to avoid this exact same circular dependency.

Do you think a similar approach would make sense here as well? Attempt to look at explicit type hints on __exit__, or would that not work because to get the __exit__ method we need to call getTypeFromObjectMember?

[erictraut]

Yeah, I think a similar approach might work, but that will involve a bunch of extra code that needs to be written, tested, and maintained. The question in my mind is whether all of that extra special-case code is worth it for this functionality. It's a pretty esoteric feature. You're the first person to report it. If this were an official part of the Python spec, then I wouldn't hesitate to support it, but it's not. So my inclination at this point is to say that it's not worth the extra code. Especially given that this is a non-standard way to handle exceptions when the language already has a perfectly good standard way to handle exceptions — a try/except statement. So my inclination at this point is to not address this issue unless/until the Python spec is updated to indicate that the return type of __exit__ has a special meaning.

[jacobbogdanov]

👍 sounds good to me