Skip to content

Report unreachable except blocks #12086

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Report unreachable except blocks #12086

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1867387
Report unreachable except blocks
kreathon Jan 24, 2022
14ee9a1
Move errors to message_registry
kreathon Jan 28, 2022
374e680
Split tests and make them more expressive
kreathon Jan 28, 2022
5b4d73d
Add safety check
kreathon Jan 28, 2022
0576eff
Add test for no errors without --warn-unreachable flag
kreathon Jan 28, 2022
0c3d194
Add type to 'UNREACHABLE' (to satisfy mypyc)
kreathon Jan 28, 2022
143ae23
Merge branch 'master' into unreachable-except-blocks-errors
kreathon May 1, 2022
329bd36
Replace disable_errors call with new filter_errors call
kreathon May 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions mypy/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2336,6 +2336,28 @@ def check_assignment(self, lvalue: Lvalue, rvalue: Expression, infer_lvalue_type
self.infer_variable_type(inferred, lvalue, rvalue_type, rvalue)
self.check_assignment_to_slots(lvalue)

def check_except_reachability(self, s: TryStmt) -> None:
"""Perform except block reachability checks.

1. Check for duplicate exception clauses.
2. Check if super class has already been caught.
"""
seen: List[Type] = []
for expr in s.types:
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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a Union with Any type as well 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

continue
if typ in seen:
self.msg.already_caught(typ, expr)
continue
seen_superclass = next((s for s in seen if is_subtype(typ, s)), None)
if seen_superclass is not None:
self.msg.superclass_already_caught(typ, seen_superclass, expr)
continue
seen.append(typ)

# (type, operator) tuples for augmented assignments supported with partial types
partial_type_augmented_ops: Final = {
('builtins.list', '+'),
Expand Down Expand Up @@ -3608,6 +3630,9 @@ def visit_try_stmt(self, s: TryStmt) -> None:
if not self.binder.is_unreachable():
self.accept(s.finally_body)

if self.should_report_unreachable_issues():
self.check_except_reachability(s)

def visit_try_without_finally(self, s: TryStmt, try_frame: bool) -> None:
"""Type check a try statement, ignoring the finally block.

Expand Down
9 changes: 9 additions & 0 deletions mypy/messages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add new simple errors to message_registry

context, code=codes.UNREACHABLE)

def superclass_already_caught(self, typ: Type, super_typ: Type, context: Context) -> None:
self.fail("Except is unreachable, superclass {} of {} has already been caught"
.format(format_type(super_typ), format_type(typ)), context,
code=codes.UNREACHABLE)

def redundant_left_operand(self, op_name: str, context: Context) -> None:
"""Indicates that the left operand of a boolean expression is redundant:
it does not change the truth value of the entire condition as a whole.
Expand Down
32 changes: 32 additions & 0 deletions test-data/unit/check-unreachable-code.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,38 @@ def baz(x: int) -> int:
return x
[builtins fixtures/exception.pyi]

[case testUnreachableFlagExcepts]
# flags: --warn-unreachable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

from typing import Type, NoReturn, Tuple

def error() -> NoReturn: ...

ignore: Type[Exception]
exclude: Tuple[Type[Exception], ...]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also test Union type here.

class ContinueOnError: pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class ContinueOnError: pass
class ContinueOnError(Exception): pass

Is this what you meant? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


try:
error()
except ignore:
pass
except exclude:
pass
except ContinueOnError: # E: Exception type must be derived from BaseException
pass
except Exception:
pass
except Exception as err: # E: Except is unreachable, "Exception" has already been caught
pass
except RuntimeError: # E: Except is unreachable, superclass "Exception" of "RuntimeError" has already been caught
pass
except (NotImplementedError, ):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kreathon kreathon Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pass
except NotImplementedError: # E: Except is unreachable, superclass "Exception" of "NotImplementedError" has already been caught
pass
except NotImplementedError: # E: Except is unreachable, superclass "Exception" of "NotImplementedError" has already been caught
pass
[builtins fixtures/exception.pyi]

[case testUnreachableFlagIgnoresSemanticAnalysisUnreachable]
# flags: --warn-unreachable --python-version 3.7 --platform win32 --always-false FOOBAR
import sys
Expand Down