Skip to content

False positive unreachable statement after exception raised within context block #8766

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

Open
PeterJCLaw opened this issue May 3, 2020 · 5 comments
Labels
bug mypy got something wrong topic-reachability Detecting unreachable code

Comments

@PeterJCLaw
Copy link
Contributor

import contextlib
from typing import Iterator

@contextlib.contextmanager
def context_func() -> Iterator[None]:
    try:
        yield
    except Exception:
        pass

def foo() -> None:
    with context_func():
        raise Exception

    print("xxx")  # mypy: Statement is unreachable

Since context managers can silence exceptions, I don't think mypy should consider statements after the context to be unreachable due to exceptions which might be raised within the context.

Flags needed: --warn-unreachable.


Python 3.6, mypy 0.770, also master (at 2a3de7b)

@JelleZijlstra
Copy link
Member

See #7214.

@johnpmcconnell
Copy link

johnpmcconnell commented Feb 11, 2021

@JelleZijlstra Seeing as that issue is closed and that this is clearly a bug in mypy (It objectively gives the wrong answer.), I respectfully request that some issue be left open until it can be resolved. I would have reported the same issue again if I hadn't been ridiculously diligent and searched closed issues. Furthermore, having an open issue would be more likely to keep it on the project's radar and would allow people to discuss possible approaches to solving it.

@JelleZijlstra JelleZijlstra reopened this Feb 11, 2021
@johnpmcconnell
Copy link

Thank you.

When the contextmanager decorator is applied, would it be possible to detect if the yield occurs inside a try/except block or itself within a context manager? Would that be sufficient for this use case?

@JelleZijlstra
Copy link
Member

That's not the kind of analysis that mypy normally is able to do. I can think of a few solutions/workarounds that change the decorator though:

  • make contextlib.contextmanager a generic class, so you can write e.g. contextlib.contextmanager[True] to get the swallowing behavior
  • create a new swallowing_contextmanager decorator

@johnpmcconnell
Copy link

johnpmcconnell commented Feb 13, 2021

@JelleZijlstra It really seems to me like something is... off about this. My recollection is shaky, but I thought reachability analysis is theoretically undecidable for an arbitrary program. (I believe you can show that it depends on solving the halting problem, but I may be wrong.) As a result, I believe most compilers that do reachability analysis at all take a "best effort" approach and assume that something is reachable unless the system can prove that it isn't. This is because there is essentially no risk to leaving around dead code, while deleting potentially live code can render a program completely nonfunctional. I believe that because this practice is fairly widespread, this is a norm programmers are accustom to: if some unreachable code can't be detected, it will just have to pass whatever normal checks are required for the program to function.

From what you're describing, it appears that (at least with context managers) mypy's team has decided to take an approach that contradicts this norm and seems dubious to me: not only are you assuming that code is unreachable unless you can specifically determine that it is reachable (the inverse of the norm), you are also declining to perform a thorough analysis of the code's behavior in order to make that determination. I'm a little confused about the motivation for that decision, as well. I can't clearly identify the risks associated with a false reachable report from mypy. It seems to me that a false reachable would merely result in mypy ensuring that some dead code properly conforms to the function specification (returns appropriately typed values or throws errors and uses correct types with variables) and that Python will need to be able to parse it, which seems harmless. The more harmful use case seems to me to be that some potentially live code instead is broken or gets wrongly deleted. Is there something I'm missing?

I believe something needs to change on that front. If mypy's team wants to be thorough in detecting dead code, it should dedicate itself to performing a more thorough analysis of the program's behavior to determine if something is unreachable. Specifically for context managers, it should not make assumptions about whether a context manager does or does not swallow an exception, but should only take actions based on what it can prove. Especially in the absence of a more thorough analysis, I can't see the justification for making any assumption about whether an exception will be handled by a context manager or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-reachability Detecting unreachable code
Projects
None yet
Development

No branches or pull requests

4 participants