Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code after with statement which always returns is considered unreachable when the dynamic type of the context manager is unknown. #1945

Closed
rik-degraaff opened this issue Oct 15, 2021 · 9 comments
Labels
bug Something isn't working fixed in next version (main) A fix has been implemented and will appear in an upcoming version

Comments

@rik-degraaff
Copy link

rik-degraaff commented Oct 15, 2021

Environment data

  • Language Server version: v2021.10.0
  • OS and version: Ubuntu 20.04.3 LTS
  • Python version (& distribution if applicable, e.g. Anaconda): 3.8.5

Expected behaviour

Since Foo suppresses all exceptions when used as a context manager and an exception is raised before the return statement is reached, the print statement after the with statement actually gets executed. It should not be marked as dead code.

Actual behaviour

The print statement gets marked as unreachable.

Code Snippet / Additional information

class Foo:
    def __enter__(self):
        pass
    
    def __exit__(self, exc_type, exc_value, stack trace) -> bool:
        return True

def bar():
    with Foo():
        print(1 / 0)
        return
    print('This actually gets printed')

bar()

In general, code after a with block should only be marked unreachable because of a statement within the with block if it is known that no exception can be raised before that statement is raised which the context manager might suppress.

Pragmatically, the code after the with block should probably always be considered reachable unless it can be proven no exception can be raised or the dynamic type of the context manager is known and it is known that that type's __exit__ function never returns True. Otherwise, only the remaining code within the with block should be marked unreachable.

@erictraut
Copy link
Contributor

Your sample contains a syntax error in the signature for __exit__. The third parameter is missing an underscore between stack and trace. If I add the underscore, it works as expected — the print statement is seen as reachable.

@rik-degraaff
Copy link
Author

Your sample contains a syntax error in the signature for __exit__. The third parameter is missing an underscore between stack and trace. If I add the underscore, it works as expected — the print statement is seen as reachable.

Sorry about the typo, I did test this without the typo before posting it here and saw this issue. I've updated to v2021.10.1 and I'm still seeing the same issue.

unreachable code
intellisense

@erictraut
Copy link
Contributor

In your screen shot above, you have not included a return type annotation for the __exit__ method. By default, type checkers assume that context managers do not catch exceptions. You can override that default assumption by including a -> bool return type annotation for __exit__, like you did in your original code sample at the top of this thread.

@rik-degraaff
Copy link
Author

@erictraut Sorry about the back and forth. I believe I've now isolated what the real issue was in my actual code. Here is a minimal example.

class Foo:
    def __enter__(self) -> None:
        pass

    def __exit__(self, exc_type, exc_value, traceback) -> bool:
        return True


def bar(foo: Foo):
    with foo:
        print(1 / 0)
        return
    print('This actually gets printed.')

bar(Foo())

Screenshot from 2021-10-15 23-35-35

It seems like pylance assumes the context manager won't suppress any exceptions if it can't figure out the dynamic type of the context manager. That seems wrong to me, especially if the static type has a return type of bool for the __exit__ method.

@jakebailey jakebailey reopened this Oct 15, 2021
@rik-degraaff rik-degraaff changed the title Code after with statement which always returns is always considered unreachable. Code after with statement which always returns is considered unreachable when the dynamic type of the context manager is unknown. Oct 15, 2021
@erictraut
Copy link
Contributor

This is a known limitation. Your example above is not instantiating the context manager within the with statement. If you change it to with Foo(), it will work as expected. It's relatively easy to accommodate the case in your sample, so I've made that code change. This will be addressed in the next release, so your example will also work.

If you're interested in the reasons why this limitation exists...

Pyright (the type checker that underlies pylance) needs to determine whether a context manager catches exceptions or not. This affects the shape of the code flow graph. However, the code flow graph is required to evaluate the types of the expressions used within the function. This creates a chicken-and-egg problem within the type checker because it can't evaluate the type of the expression used within the with statement prior to constructing the code flow graph, but the shape of the code flow graph depends on the type of the expression in the with statement.

Previously, pyright assumed in all cases that context managers did not swallow exceptions. About six months ago, I added limited support for context managers that swallow exceptions. This logic cannot generally evaluate the type of the expression used within the with statement for the reasons described above, but it is able to handle common cases. In particular, it can handle the case where the expression uses a known pattern and the type of that expression depends only on type declarations (type annotations) rather than type inference. The current code specifically looks for a call expression in the with statement that instantiates the context manager. This is the most typical usage pattern for context managers. You've deviated from this in your sample above because you're accepting an already-instantiated context manager as an input to your function. I've added support for the expression pattern you've used.

@erictraut erictraut added bug Something isn't working fixed in next version (main) A fix has been implemented and will appear in an upcoming version and removed triage labels Oct 15, 2021
@rik-degraaff
Copy link
Author

That makes a lot of sense, thanks for the explanation. Will your fix also cover the following patterns as well?

def foo() -> Foo:
    return Foo()

with foo():
    pass
foo: Foo = Foo()
with foo():
    pass

What I'm actually doing in my project is something like

from __future__ import annotations

class Foo:
    def __enter__(self) -> None:
        pass

    def __exit__(self, exc_type, exc_value, traceback) -> bool:
        return True


    def bar(self) -> Foo:
        return self


def baz():
    with Foo().bar():
        print(1 / 0)
        return
    print('This actually gets printed.')


baz()

I assume that would be covered by the patterns above, or are these usage patterns more rigid than I'm imagining them?

@erictraut
Copy link
Contributor

No, this change won't work with the first usage pattern above. (The second one is invalid because it attempts to call an object). The logic is very rigid because it can't use the general type evaluator; it needs to work directly on the parse tree.

Now that you understand the details, you can change your code to accommodate the known limitations.

@rik-degraaff
Copy link
Author

rik-degraaff commented Oct 18, 2021

@erictraut I do understand the details. Thanks for all of your help. Unfortunately, clients of my code will be writing with statements, so I don't want to require them to jump through too many hoops to make the type checker happy. I had a look at your commit addressing this and used that as a basis to make a pull reques of my ownt which addresses my use case as well as a few others in what I think is a reasonable and rather general way:
microsoft/pyright#2448

I hope it reaches the desired quality for the pyright project and that you'll be able to merge it soon.

@bschnurr
Copy link
Member

This issue has been fixed in version 2021.10.2, which we've just released. You can find the changelog here: https://github.com/microsoft/pylance-release/blob/main/CHANGELOG.md#2021102-20-october-2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed in next version (main) A fix has been implemented and will appear in an upcoming version
Projects
None yet
Development

No branches or pull requests

4 participants