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.

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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 26 additions & 0 deletions mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -2415,6 +2415,29 @@ 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.filter_errors():
typ = get_proper_type(self.check_except_handler_test(expr))
if isinstance(typ, AnyType) or isinstance(typ, UnionType):
Copy link
Member

Choose a reason for hiding this comment

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

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?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sobolevn any updates here? ;)

continue
if typ in seen:
self.fail(message_registry.ALREADY_CAUGHT.format(format_type(typ)), expr)
continue
seen_superclass = next((s for s in seen if is_subtype(typ, s)), None)
if seen_superclass is not None:
self.fail(message_registry.SUPERCLASS_ALREADY_CAUGHT.format(
format_type(seen_superclass), format_type(typ)), 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 @@ -3777,6 +3800,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
2 changes: 1 addition & 1 deletion mypy/errorcodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def __str__(self) -> str:
'Reject returning value with "Any" type if return type is not "Any"',
"General",
)
UNREACHABLE: Final = ErrorCode(
UNREACHABLE: Final[ErrorCode] = ErrorCode(
"unreachable", "Warn about unreachable statements or expressions", "General"
)
REDUNDANT_EXPR: Final = ErrorCode(
Expand Down
10 changes: 10 additions & 0 deletions mypy/message_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,3 +257,13 @@ def format(self, *args: object, **kwargs: object) -> "ErrorMessage":
CLASS_PATTERN_UNKNOWN_KEYWORD: Final = 'Class "{}" has no attribute "{}"'
MULTIPLE_ASSIGNMENTS_IN_PATTERN: Final = 'Multiple assignments to name "{}" in pattern'
CANNOT_MODIFY_MATCH_ARGS: Final = 'Cannot assign to "__match_args__"'

# Unreachable
ALREADY_CAUGHT: Final = ErrorMessage(
"Except is unreachable, {} has already been caught",
code=codes.UNREACHABLE,
)
SUPERCLASS_ALREADY_CAUGHT: Final = ErrorMessage(
"Except is unreachable, superclass {} of {} has already been caught",
code=codes.UNREACHABLE,
)
97 changes: 97 additions & 0 deletions test-data/unit/check-unreachable-code.test
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,103 @@ 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

def error() -> NoReturn: ...

try:
error()
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, ):
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 testUnreachableFlagIgnoreVariablesInExcept]
# flags: --warn-unreachable
from typing import NoReturn, Union, Type, Tuple

def error() -> NoReturn: ...
class MyError(BaseException): ...

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.

omit: Union[Type[RuntimeError], Type[MyError]]

try:
error()
except ignore:
pass
except exclude:
pass
except omit:
pass
except RuntimeError:
pass
[builtins fixtures/exception.pyi]

[case testUnreachableFlagExceptWithUnknownBaseClass]
# flags: --warn-unreachable
from typing import Any, NoReturn

Parent: Any
class MyError(Parent): ...
def error() -> NoReturn: ...

try:
error()
except MyError:
pass
except Exception:
pass
except MyError: # E: Except is unreachable, "MyError" has already been caught
pass
[builtins fixtures/exception.pyi]

[case testUnreachableFlagExceptWithError]
# flags: --warn-unreachable
from typing import NoReturn

# The following class is not derived from 'BaseException'. Thus an error is raised
# resulting in Any as type for the class in the exception expression. Such cases
# must be ignored in the rest of the reachability analysis.
class NotFromBaseException: ...

def error() -> NoReturn: ...

try:
error()
except NotFromBaseException: # E: Exception type must be derived from BaseException
pass
except RuntimeError:
pass
except NotImplementedError: # E: Except is unreachable, superclass "RuntimeError" of "NotImplementedError" has already been caught
pass
[builtins fixtures/exception.pyi]

[case testUnreachableExceptWithoutUnreachableFlag]
from typing import NoReturn

def error() -> NoReturn: ...

try:
error()
except Exception:
pass
except RuntimeError:
pass
[builtins fixtures/exception.pyi]

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