Detect unreachable except branches

[jcrist]

Since except blocks are checked in order, and the first matching block is executed, it's possible to accidentally write a try-except that will never execute some branches. For example:

def bad_reader(path: str) -> bytes:
    if path == "bad":
        raise PermissionError

    with open(path, "rb") as f:
        return f.read()


def load_data(path: str) -> bytes:
    try:
        return bad_reader(path)
    except OSError:
        print("OSError caught")
    except PermissionError:
        # This branch will never be hit, since `OSError` will always catch
        # permission error
        print("PermissionError caught")

    return b""


load_data("bad")

Since PermissionError is a subclass of OSError, the except PermissionError case will never execute. I'd expect/hope that these kind of mistakes would be detectable with type information. I think adding checks for this would make sense under the --warn-unreachable config flag, so a new CLI option may not be needed.

[sobolevn]

Great idea!

[jcrist]

If possible, I'd be interested in hacking on this as an interesting side project. I took a scan through the code and couldn't figure out where such an analysis pass might fit in to mypy. If this seems like a reasonable project for someone who's new-to-mypy-but-not-new-to-python, I'd welcome some advice on where/how this feature should be added. "No, this is a tricky feature that would be best handled by the core mypy team" is also a fine response. Happy to help if possible.

[sobolevn]

@jcrist yes, this looks rather simple. I can guide you, if you wish 🙂

So, here's how I see this:

1. We need to add this hook to visit_try_stmt in checker.py:

   mypy/mypy/checker.py

   Line 3563 in fdcda96

   def visit_try_stmt(self, s: TryStmt) -> None:

2. There under if self.options.warn_unreachable we check TryStmt.types prop:

   mypy/mypy/nodes.py

   Line 1314 in fdcda96

   types: List[Optional[Expression]] # Except type expressions

   You can convert Expression to Type with self.expr_checker.accept(expression). It is a good idea to disable errors while doing that. Because we properly check expression types in a different place. Or maybe you can add this check to a place where we do check except types 🤔
3. Then, we detect which cases are already handled via is_subtype function:

   mypy/mypy/subtypes.py

   Line 49 in fdcda96

   def is_subtype(left: Type, right: Type,

4. Lastly, we report errors via self.fail
5. Tests can probably live in https://github.com/python/mypy/blob/fdcda966023840b854567df0643091653b179210/test-data/unit/check-unreachable-code.test Make sure you use the correct [fixture ...] with proper exception hierarchy!

I hope that this helps. Feel free to ping me if you have any questions 🙂
Btw, I am not a maintainer, so this can still be rejected during the review from real mypy experts.
But, you never know untill you try! 👍

[kreathon]

@jcrist are you still working on this? If not I would give it a try :)

[jcrist]

No, I never gave it a go - please feel free to pick this up.