Report unreachable except blocks #12086

kreathon · 2022-01-27T18:52:04Z

Motivation

Detect unreachable except branches as proposed here #11514.

try:
    error()
except Exception:
    pass
except RuntimeError:  # This is unreachable.
    pass

These checks are already implemented in PyCharm and were also picked up by pyright (microsoft/pyright#2761).

Implementation

I mainly followed the idea/plan given by @sobolevn in the linked issue. Thank you for that! :)

Rationale

I decided to not implement this directly in visit_try_without_finally where the except expressions are already evaluated, because the feature is only enabled via the warn-unreachable flag and it would make the loop even more complex.

In order to avoid false positives, the check is skipped for variables in the except clause (they are usually typed as Type[Exception]).

Limitations

Even though the checks could be extended to Tuples in the except clause, I decided to not implement it in this first draft.

Test Plan

[case testUnreachableFlagExcepts] was added.

It checks the error messages as well as that no false positives for variables are generated.

Closes #11514

sobolevn

Thanks! Cool feature! I had some bugs with except order in my life 👍

sobolevn · 2022-01-27T19:07:31Z

mypy/checker.py

+            if expr and isinstance(expr, NameExpr) and isinstance(expr.node, TypeInfo):
+                with self.expr_checker.msg.disable_errors():
+                    typ = get_proper_type(self.check_except_handler_test(expr))
+                if isinstance(typ, AnyType):


This can be a Union with Any type as well 🤔

Can you think of a case where this happens?

I would argue that his can only happen for variables and tuples (which are both filtered with the if expr and isinstance(expr, NameExpr) and isinstance(expr.node, TypeInfo):)

For safety reasons: I can add Union to the check if isinstance(typ, AnyType) or isinstance(typ, UnionType):

sobolevn · 2022-01-27T19:08:30Z

mypy/messages.py

@@ -1386,6 +1386,15 @@ def note_call(self,
    def unreachable_statement(self, context: Context) -> None:
        self.fail("Statement is unreachable", context, code=codes.UNREACHABLE)

+    def already_caught(self, typ: Type, context: Context) -> None:
+        self.fail("Except is unreachable, {} has already been caught".format(format_type(typ)),


Let's add new simple errors to message_registry

sobolevn · 2022-01-27T19:09:58Z

test-data/unit/check-unreachable-code.test

@@ -841,6 +841,38 @@ def baz(x: int) -> int:
        return x
 [builtins fixtures/exception.pyi]

+[case testUnreachableFlagExcepts]
+# flags: --warn-unreachable


We need the same test (or simplified) without this flag to make sure it does not produce any errors in normal mode.

Where should this test go? Still in check-unreachable-code.test? I cannot find the equivalent for other features in this file. Into check-statements.test?

In check-statements.test there is already a test covering the use-case.

[case testMultipleExceptHandlers] import typing try: pass except BaseException as e: pass except Err as f: f = BaseException() # Fail f = Err() class Err(BaseException): pass [builtins fixtures/exception.pyi] [out] main:7: error: Incompatible types in assignment (expression has type "BaseException", variable has type "Err")

Yes, check-unreachable-code.test is fine I guess 🙂

sobolevn · 2022-01-27T19:10:19Z

test-data/unit/check-unreachable-code.test

+def error() -> NoReturn: ...
+
+ignore: Type[Exception]
+exclude: Tuple[Type[Exception], ...]


Let's also test Union type here.

sobolevn · 2022-01-27T19:11:13Z

test-data/unit/check-unreachable-code.test

+
+ignore: Type[Exception]
+exclude: Tuple[Type[Exception], ...]
+class ContinueOnError: pass


Suggested change

class ContinueOnError: pass

class ContinueOnError(Exception): pass

Is this what you meant? 🤔

I wanted to provoke self.check_except_handler_test(expr) to produce and error and thus return Any. In this case, we must ignore it, otherwise is_subtype always evaluates to True.

I will add a test to make this more clear.

sobolevn · 2022-01-27T19:11:45Z

test-data/unit/check-unreachable-code.test

+    pass
+except RuntimeError:               # E: Except is unreachable, superclass "Exception" of "RuntimeError" has already been caught
+    pass
+except (NotImplementedError, ):


I think that this should be reported as unreachable as well.

Do you mean I should support tuples in general or only this special case?

My feeling is that this already makes the code more complex, because the simple check expr and isinstance(expr, NameExpr) and isinstance(expr.node, TypeInfo) to support everything that is not supported is no longer sufficient. If its fine, I would like to move it into a separate PR to keep this a bit "cleaner".

We can defer this to the next PR I guess 🙂
But, please don't forget to open an issue for it!

kreathon · 2022-01-28T18:29:21Z

@sobolevn Thank you for your feedback 🙂. I pushed some updates. Can you have a look again?

sobolevn · 2022-02-02T09:18:01Z

mypy/checker.py

+            if expr and isinstance(expr, NameExpr) and isinstance(expr.node, TypeInfo):
+                with self.expr_checker.msg.disable_errors():
+                    typ = get_proper_type(self.check_except_handler_test(expr))
+                if isinstance(typ, AnyType) or isinstance(typ, UnionType):


Sorry, this is not what I meant 🙂

Theoretically we can have a case when some var has type[SomeException] | Any type. For example, from unresolved import or some runtime magic.

Is there anything we can do to handle this case here? Or shall we ignore it?

I think I know what you mean. So, we do not have a variable, but a class whose type might be a Union.

I do not how how to construct such a case for testing, can you help me with that? I was also not able to find something in check-statements.test

The tests already cover:

Parent: Any class MyError(Parent): ... omit: Union[Type[RuntimeError], Type[MyError]]

Nevertheless, it think this scenario would not pass the new or isinstance(typ, UnionType) filter, thus we would just ignore it.

@sobolevn any updates here? ;)

JelleZijlstra · 2022-05-01T12:51:44Z

You'll need to adjust to the error checker changes from #12631 and #12699.

github-actions · 2022-05-01T15:57:15Z

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Report unreachable except blocks

1867387

sobolevn requested changes Jan 27, 2022

View reviewed changes

kreathon added 5 commits January 28, 2022 16:40

Move errors to message_registry

14ee9a1

Split tests and make them more expressive

374e680

Add safety check

5b4d73d

Add test for no errors without --warn-unreachable flag

0576eff

Add type to 'UNREACHABLE' (to satisfy mypyc)

0c3d194

sobolevn reviewed Feb 2, 2022

View reviewed changes

kreathon requested a review from sobolevn March 10, 2022 16:57

Merge branch 'master' into unreachable-except-blocks-errors

143ae23

This comment has been minimized.

Sign in to view

Replace disable_errors call with new filter_errors call

329bd36

kreathon closed this Dec 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report unreachable except blocks #12086

Report unreachable except blocks #12086

kreathon commented Jan 27, 2022

sobolevn left a comment

sobolevn Jan 27, 2022

kreathon Jan 28, 2022

sobolevn Jan 27, 2022

sobolevn Jan 27, 2022

kreathon Jan 28, 2022

sobolevn Jan 28, 2022

sobolevn Jan 27, 2022

sobolevn Jan 27, 2022

kreathon Jan 28, 2022

sobolevn Jan 27, 2022

kreathon Jan 28, 2022 •

edited

Loading

sobolevn Jan 28, 2022

kreathon commented Jan 28, 2022

sobolevn Feb 2, 2022

kreathon Feb 3, 2022

kreathon Feb 17, 2022

This comment has been minimized.

JelleZijlstra commented May 1, 2022

github-actions bot commented May 1, 2022

	class ContinueOnError: pass
	class ContinueOnError(Exception): pass

Report unreachable except blocks #12086

Report unreachable except blocks #12086

Conversation

kreathon commented Jan 27, 2022

Motivation

Implementation

Rationale

Limitations

Test Plan

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kreathon Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kreathon commented Jan 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

JelleZijlstra commented May 1, 2022

github-actions bot commented May 1, 2022

kreathon Jan 28, 2022 •

edited

Loading