From f7a56dd337acda060aa643c80f20b2e780ad2fa2 Mon Sep 17 00:00:00 2001 From: ScDor <18174994+ScDor@users.noreply.github.com> Date: Fri, 12 May 2023 10:50:40 +0300 Subject: [PATCH] feat: useless try-except --- src/tests/analyzers_exception_block_test.py | 19 ++ .../samples/violations/try_useless_except.py | 165 ++++++++++++++++++ src/tryceratops/analyzers/__init__.py | 1 + src/tryceratops/analyzers/exception_block.py | 28 +++ src/tryceratops/violations/codes.py | 2 + 5 files changed, 215 insertions(+) create mode 100644 src/tests/samples/violations/try_useless_except.py diff --git a/src/tests/analyzers_exception_block_test.py b/src/tests/analyzers_exception_block_test.py index eac48ba..a76cc4a 100644 --- a/src/tests/analyzers_exception_block_test.py +++ b/src/tests/analyzers_exception_block_test.py @@ -56,6 +56,25 @@ def test_broad_pass(): assert_broad(35, 12, third) +def test_useless_except(): + tree = read_sample("try_useless_except") + + analyzer = analyzers.exception_block.UselessTryExceptAnalyzer() + code, msg = codes.USELESS_TRY_EXCEPT + + violations = analyzer.check(tree, "filename") + assert_useless_except = partial(assert_violation, code, msg) + + assert len(violations) == 7 + assert_useless_except(22, 4, violations[0]) + assert_useless_except(54, 4, violations[1]) + assert_useless_except(62, 4, violations[2]) + assert_useless_except(70, 4, violations[3]) + assert_useless_except(78, 4, violations[4]) + assert_useless_except(88, 4, violations[5]) + assert_useless_except(153, 4, violations[6]) + + def test_log_error(): tree = read_sample("log_error") analyzer = analyzers.exception_block.LogErrorAnalyzer() diff --git a/src/tests/samples/violations/try_useless_except.py b/src/tests/samples/violations/try_useless_except.py new file mode 100644 index 0000000..89a7e86 --- /dev/null +++ b/src/tests/samples/violations/try_useless_except.py @@ -0,0 +1,165 @@ +""" +Violation: + +Capturing an exception just to raise it again has no effect in runtime, +and is considered a bad practice. + +It's recommended to either log the error, change the behavior on capture, +or just remove the try-except block. + +""" +import logging + +logger = logging.getLogger(__name__) + + +class MyException(Exception): + pass + + +def func_blanket(): + # ERROR + try: + ... + except: + raise + + +def func_blanket_custom(): + # OK - raises other + try: + ... + except: + raise MyException + + +def func_blanket_custom_e(): + # OK - raises other + try: + ... + except Exception as e: + raise MyException + + +def func_blanket_custom_from_e(): + # OK - raises other + try: + ... + except Exception as e: + raise MyException from e + + +def func_blanket_as_e_reraise(): + # ERROR + try: + ... + except Exception as e: + raise e + + +def func_blanket_exception(): + # ERROR + try: + ... + except Exception: + raise + + +def func_specific_exception(): + # ERROR + try: + ... + except MyException: + raise + + +def func_custom_then_blanket(): + # ERROR - all are useless + try: + ... + except MyException: + raise + except: + raise + + +def func_three_useless(): + # ERROR - all are useless + try: + ... + except MyException: + raise + except Exception: + raise + except: + raise + + +def func_blanket_then_code(): + # OK - more than just a `raise` handler + try: + ... + except: + ... + raise + + +def func_only_second_useless(): + # OK - more than just a `raise` (1st handler) + try: + ... + except MyException: + ... + except: + raise + + +def func_only_first_useless(): + # OK - more than just a `raise` (2nd handler) + try: + ... + except MyException: + raise + except: + ... + raise + + +def func_multiple_second_and_third_useless(): + # OK - more than just a `raise` (1st handler) + try: + ... + except MyException: + ... + raise + except Exception: + raise + except: + raise + + +def func_except_finally_vaild(): + # OK - not useless + try: + ... + except: + ... + finally: + ... + + +def func_except_finally_useless(): + # ERROR - useless + try: + ... + except: + raise + finally: + ... + + +def func_single_finally(): + try: + ... + finally: + ... diff --git a/src/tryceratops/analyzers/__init__.py b/src/tryceratops/analyzers/__init__.py index f57e367..604c38e 100644 --- a/src/tryceratops/analyzers/__init__.py +++ b/src/tryceratops/analyzers/__init__.py @@ -20,6 +20,7 @@ exception_block.ExceptBroadPassAnalyzer, exception_block.LogErrorAnalyzer, exception_block.LogObjectAnalyzer, + exception_block.UselessTryExceptAnalyzer, try_block.TryConsiderElseAnalyzer, try_block.TryShouldntRaiseAnalyzer, } diff --git a/src/tryceratops/analyzers/exception_block.py b/src/tryceratops/analyzers/exception_block.py index 84e4e07..6b2c1fd 100644 --- a/src/tryceratops/analyzers/exception_block.py +++ b/src/tryceratops/analyzers/exception_block.py @@ -148,3 +148,31 @@ def visit_ExceptHandler(self, node: ast.ExceptHandler) -> None: self._find_violations(node) self.generic_visit(node) + + +class UselessTryExceptAnalyzer(BaseAnalyzer): + violation_code = codes.USELESS_TRY_EXCEPT + + @visit_error_handler + def visit_Try(self, node: ast.Try) -> None: + def is_handler_useless(handler: ast.ExceptHandler) -> bool: + # a handler whose body is just `raise` is considered useless + + if len(handler.body) == 1 and isinstance(handler.body[0], ast.Raise): + raise_argument = handler.body[0].exc + return ( + raise_argument + is None + # `except ... as e: raise`, no argument + ) or ( + isinstance(raise_argument, ast.Name) + and raise_argument.id == handler.name + # `except ... as e: raise e` + ) + return False + + if node.handlers and all(is_handler_useless(handler) for handler in node.handlers): + # the `if node.handlers` is for allowing try-finally cases, without except + self._mark_violation(node) + + self.generic_visit(node) diff --git a/src/tryceratops/violations/codes.py b/src/tryceratops/violations/codes.py index 6aab035..f86c147 100644 --- a/src/tryceratops/violations/codes.py +++ b/src/tryceratops/violations/codes.py @@ -20,6 +20,7 @@ "Simply use 'raise' without specifying exception object again", ) IGNORING_EXCEPTION = ("TRY202", "You're ignoring a broad exception without even logging") +USELESS_TRY_EXCEPT = ("TRY203", "Useless try-except, remove it or handle the exception") # TRY3xx - Try CONSIDER_ELSE = ("TRY300", "Consider moving this statement to an 'else' block") @@ -39,6 +40,7 @@ "TRY202", "TRY300", "TRY301", + "TRY302", "TRY400", "TRY401", }