From af80c0248f26a8715d9f8647f9988ceded644061 Mon Sep 17 00:00:00 2001 From: Zac-HD Date: Tue, 7 Sep 2021 16:33:04 +1000 Subject: [PATCH 1/5] Strip trailing whitespace --- README.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/README.rst b/README.rst index e8934f3..322edc8 100644 --- a/README.rst +++ b/README.rst @@ -123,9 +123,9 @@ waste CPU instructions. Either prepend ``assert`` or remove it. **B016**: Cannot raise a literal. Did you intend to return it or raise an Exception? -**B017**: ``self.assertRaises(Exception):`` should be considered evil. It can lead -to your test passing even if the code being tested is never executed due to a typo. -Either assert for a more specific exception (builtin or custom), use +**B017**: ``self.assertRaises(Exception):`` should be considered evil. It can lead +to your test passing even if the code being tested is never executed due to a typo. +Either assert for a more specific exception (builtin or custom), use ``assertRaisesRegex``, or use the context manager form of assertRaises (``with self.assertRaises(Exception) as ex:``) with an assertion against the data available in ``ex``. @@ -220,7 +220,7 @@ Change Log 21.4.3 ~~~~~~ -* Verify the element in item_context.args is of type ast.Name for b017 +* Verify the element in item_context.args is of type ast.Name for b017 21.4.2 ~~~~~~ @@ -231,7 +231,7 @@ Change Log ~~~~~~ * Add B017: check for gotta-catch-em-all assertRaises(Exception) - + 21.3.2 ~~~~~~ From 4fd2e86b5d0d39f029e52865ffce18ba5e3cbe35 Mon Sep 17 00:00:00 2001 From: Zac-HD Date: Tue, 7 Sep 2021 16:33:04 +1000 Subject: [PATCH 2/5] Add B018 - use exc chaining --- README.rst | 10 ++++++++++ bugbear.py | 20 ++++++++++++++++++++ tests/b018.py | 18 ++++++++++++++++++ tests/test_bugbear.py | 12 ++++++++++++ 4 files changed, 60 insertions(+) create mode 100644 tests/b018.py diff --git a/README.rst b/README.rst index 322edc8..de585e6 100644 --- a/README.rst +++ b/README.rst @@ -130,6 +130,11 @@ Either assert for a more specific exception (builtin or custom), use (``with self.assertRaises(Exception) as ex:``) with an assertion against the data available in ``ex``. +**B018**: Within an ``except`` clause, raise exceptions with ``raise ... from err`` +or ``raise ... from None`` to distinguish them from errors in exception handling. +See [the exception chaining tutorial](https://docs.python.org/3/tutorial/errors.html#exception-chaining) +for details. + Opinionated warnings ~~~~~~~~~~~~~~~~~~~~ @@ -217,6 +222,11 @@ MIT Change Log ---------- +Future +~~~~~~ + +* Add B018: check for ``raise`` without ``from`` in an ``except`` clause + 21.4.3 ~~~~~~ diff --git a/bugbear.py b/bugbear.py index 68b09db..0f26af7 100644 --- a/bugbear.py +++ b/bugbear.py @@ -292,6 +292,7 @@ def visit_Compare(self, node): def visit_Raise(self, node): self.check_for_b016(node) + self.check_for_b018(node) self.generic_visit(node) def visit_With(self, node): @@ -426,6 +427,18 @@ def check_for_b017(self, node): ): self.errors.append(B017(node.lineno, node.col_offset)) + def check_for_b018(self, node): + """Checks `raise` without `from` inside an `except` clause. + + In these cases, you should use explicit exception chaining from the + earlier error, or suppress it with `raise ... from None`. See + https://docs.python.org/3/tutorial/errors.html#exception-chaining + """ + if node.cause is None and node.exc is not None and any( + isinstance(n, ast.ExceptHandler) for n in self.node_stack + ): + self.errors.append(B018(node.lineno, node.col_offset)) + def walk_function_body(self, node): def _loop(parent, node): if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)): @@ -746,6 +759,13 @@ def visit(self, node): "context manager form of assertRaises." ) ) +B018 = Error( + message=( + "B018 Within an `except` clause, raise exceptions with `raise ... from err` " + "or `raise ... from None` to distinguish them from errors in exception handling. " + "See https://docs.python.org/3/tutorial/errors.html#exception-chaining for details." + ) +) # Warnings disabled by default. B901 = Error( diff --git a/tests/b018.py b/tests/b018.py new file mode 100644 index 0000000..e0d2098 --- /dev/null +++ b/tests/b018.py @@ -0,0 +1,18 @@ +""" +Should emit: +B018 - on lines 10, 11 and 16 +""" + +try: + raise ValueError +except ValueError: + if "abc": + raise TypeError + raise UserWarning +except AssertionError: + raise # Bare `raise` should not be an error +except Exception as err: + assert err + raise Exception("No cause here...") +finally: + raise Exception("Nothing to chain from, so no warning here") diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index 0807f19..ee8c259 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -28,6 +28,7 @@ B015, B016, B017, + B018, B901, B902, B903, @@ -207,6 +208,17 @@ def test_b017(self): expected = self.errors(B017(22, 8)) self.assertEqual(errors, expected) + def test_b018(self): + filename = Path(__file__).absolute().parent / "b018.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + expected = [ + B018(10, 8), + B018(11, 4), + B018(16, 4), + ] + self.assertEqual(errors, self.errors(*expected)) + def test_b901(self): filename = Path(__file__).absolute().parent / "b901.py" bbc = BugBearChecker(filename=str(filename)) From 235ebae9418f1ee7511829347005acd1ffd288f9 Mon Sep 17 00:00:00 2001 From: Zac-HD Date: Tue, 7 Sep 2021 16:33:05 +1000 Subject: [PATCH 3/5] Format with black --- .flake8 | 2 +- bugbear.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.flake8 b/.flake8 index dae0ba2..066df22 100644 --- a/.flake8 +++ b/.flake8 @@ -2,7 +2,7 @@ # Keep in sync with setup.cfg which is used for source packages. [flake8] -ignore = E203, E302, E501, E999 +ignore = E203, E302, E501, E999, W503 max-line-length = 88 max-complexity = 12 select = B,C,E,F,W,B9 diff --git a/bugbear.py b/bugbear.py index 0f26af7..9abc302 100644 --- a/bugbear.py +++ b/bugbear.py @@ -434,8 +434,10 @@ def check_for_b018(self, node): earlier error, or suppress it with `raise ... from None`. See https://docs.python.org/3/tutorial/errors.html#exception-chaining """ - if node.cause is None and node.exc is not None and any( - isinstance(n, ast.ExceptHandler) for n in self.node_stack + if ( + node.cause is None + and node.exc is not None + and any(isinstance(n, ast.ExceptHandler) for n in self.node_stack) ): self.errors.append(B018(node.lineno, node.col_offset)) From 07a3c3dd91bf8d6ec1d40c75be7c9977716f4f64 Mon Sep 17 00:00:00 2001 From: Zac-HD Date: Tue, 7 Sep 2021 16:33:05 +1000 Subject: [PATCH 4/5] Allow explicit re-raise This is useful when you want to explicitly modify the traceback, as done by Hypothesis and Trio --- bugbear.py | 1 + tests/b018.py | 3 +++ 2 files changed, 4 insertions(+) diff --git a/bugbear.py b/bugbear.py index 9abc302..5b2da3c 100644 --- a/bugbear.py +++ b/bugbear.py @@ -437,6 +437,7 @@ def check_for_b018(self, node): if ( node.cause is None and node.exc is not None + and not (isinstance(node.exc, ast.Name) and node.exc.id.islower()) and any(isinstance(n, ast.ExceptHandler) for n in self.node_stack) ): self.errors.append(B018(node.lineno, node.col_offset)) diff --git a/tests/b018.py b/tests/b018.py index e0d2098..5d61237 100644 --- a/tests/b018.py +++ b/tests/b018.py @@ -14,5 +14,8 @@ except Exception as err: assert err raise Exception("No cause here...") +except BaseException as base_err: + # Might use this instead of bare raise with the `.with_traceback()` method + raise base_err finally: raise Exception("Nothing to chain from, so no warning here") From 20a54139c8c5aef1d0243190fefa372256225afc Mon Sep 17 00:00:00 2001 From: Zac-HD Date: Wed, 8 Sep 2021 18:52:11 +1000 Subject: [PATCH 5/5] Disable raises check by default --- README.rst | 12 ++++++------ bugbear.py | 23 ++++++++++++----------- tests/{b018.py => b904.py} | 2 +- tests/test_bugbear.py | 24 ++++++++++++------------ 4 files changed, 31 insertions(+), 30 deletions(-) rename tests/{b018.py => b904.py} (94%) diff --git a/README.rst b/README.rst index de585e6..7f81cc5 100644 --- a/README.rst +++ b/README.rst @@ -130,11 +130,6 @@ Either assert for a more specific exception (builtin or custom), use (``with self.assertRaises(Exception) as ex:``) with an assertion against the data available in ``ex``. -**B018**: Within an ``except`` clause, raise exceptions with ``raise ... from err`` -or ``raise ... from None`` to distinguish them from errors in exception handling. -See [the exception chaining tutorial](https://docs.python.org/3/tutorial/errors.html#exception-chaining) -for details. - Opinionated warnings ~~~~~~~~~~~~~~~~~~~~ @@ -162,6 +157,11 @@ nothing else. If the attributes should be mutable, define the attributes in ``__slots__`` to save per-instance memory and to prevent accidentally creating additional attributes on instances. +**B904**: Within an ``except`` clause, raise exceptions with ``raise ... from err`` +or ``raise ... from None`` to distinguish them from errors in exception handling. +See [the exception chaining tutorial](https://docs.python.org/3/tutorial/errors.html#exception-chaining) +for details. + **B950**: Line too long. This is a pragmatic equivalent of ``pycodestyle``'s E501: it considers "max-line-length" but only triggers when the value has been exceeded by **more than 10%**. You will no @@ -225,7 +225,7 @@ Change Log Future ~~~~~~ -* Add B018: check for ``raise`` without ``from`` in an ``except`` clause +* Add B904: check for ``raise`` without ``from`` in an ``except`` clause 21.4.3 ~~~~~~ diff --git a/bugbear.py b/bugbear.py index 5b2da3c..44cf9d4 100644 --- a/bugbear.py +++ b/bugbear.py @@ -292,7 +292,7 @@ def visit_Compare(self, node): def visit_Raise(self, node): self.check_for_b016(node) - self.check_for_b018(node) + self.check_for_b904(node) self.generic_visit(node) def visit_With(self, node): @@ -427,7 +427,7 @@ def check_for_b017(self, node): ): self.errors.append(B017(node.lineno, node.col_offset)) - def check_for_b018(self, node): + def check_for_b904(self, node): """Checks `raise` without `from` inside an `except` clause. In these cases, you should use explicit exception chaining from the @@ -440,7 +440,7 @@ def check_for_b018(self, node): and not (isinstance(node.exc, ast.Name) and node.exc.id.islower()) and any(isinstance(n, ast.ExceptHandler) for n in self.node_stack) ): - self.errors.append(B018(node.lineno, node.col_offset)) + self.errors.append(B904(node.lineno, node.col_offset)) def walk_function_body(self, node): def _loop(parent, node): @@ -762,13 +762,6 @@ def visit(self, node): "context manager form of assertRaises." ) ) -B018 = Error( - message=( - "B018 Within an `except` clause, raise exceptions with `raise ... from err` " - "or `raise ... from None` to distinguish them from errors in exception handling. " - "See https://docs.python.org/3/tutorial/errors.html#exception-chaining for details." - ) -) # Warnings disabled by default. B901 = Error( @@ -798,6 +791,14 @@ def visit(self, node): ) ) +B904 = Error( + message=( + "B904 Within an `except` clause, raise exceptions with `raise ... from err` " + "or `raise ... from None` to distinguish them from errors in exception handling. " + "See https://docs.python.org/3/tutorial/errors.html#exception-chaining for details." + ) +) + B950 = Error(message="B950 line too long ({} > {} characters)") -disabled_by_default = ["B901", "B902", "B903", "B950"] +disabled_by_default = ["B901", "B902", "B903", "B904", "B950"] diff --git a/tests/b018.py b/tests/b904.py similarity index 94% rename from tests/b018.py rename to tests/b904.py index 5d61237..104fda8 100644 --- a/tests/b018.py +++ b/tests/b904.py @@ -1,6 +1,6 @@ """ Should emit: -B018 - on lines 10, 11 and 16 +B904 - on lines 10, 11 and 16 """ try: diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index ee8c259..3f91edc 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -28,7 +28,7 @@ B015, B016, B017, - B018, + B904, B901, B902, B903, @@ -208,17 +208,6 @@ def test_b017(self): expected = self.errors(B017(22, 8)) self.assertEqual(errors, expected) - def test_b018(self): - filename = Path(__file__).absolute().parent / "b018.py" - bbc = BugBearChecker(filename=str(filename)) - errors = list(bbc.run()) - expected = [ - B018(10, 8), - B018(11, 4), - B018(16, 4), - ] - self.assertEqual(errors, self.errors(*expected)) - def test_b901(self): filename = Path(__file__).absolute().parent / "b901.py" bbc = BugBearChecker(filename=str(filename)) @@ -272,6 +261,17 @@ def test_b903(self): errors = list(bbc.run()) self.assertEqual(errors, self.errors(B903(32, 0), B903(38, 0))) + def test_b904(self): + filename = Path(__file__).absolute().parent / "b904.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + expected = [ + B904(10, 8), + B904(11, 4), + B904(16, 4), + ] + self.assertEqual(errors, self.errors(*expected)) + def test_b950(self): filename = Path(__file__).absolute().parent / "b950.py" bbc = BugBearChecker(filename=str(filename))