From 1238c868ebf175b2ad9f3f8b063c9412933190fc Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 2 Jul 2023 19:28:43 -0400 Subject: [PATCH] Include BaseException in B017 rule --- .../test/fixtures/flake8_bugbear/B017.py | 7 + crates/ruff/src/checkers/ast/mod.rs | 2 +- .../rules/assert_raises_exception.rs | 122 +++++++++++------- ...__flake8_bugbear__tests__B017_B017.py.snap | 47 ++++--- .../pyflakes/rules/yield_outside_function.rs | 2 +- 5 files changed, 112 insertions(+), 68 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_bugbear/B017.py b/crates/ruff/resources/test/fixtures/flake8_bugbear/B017.py index 43f1b986e9362..917a848ba1884 100644 --- a/crates/ruff/resources/test/fixtures/flake8_bugbear/B017.py +++ b/crates/ruff/resources/test/fixtures/flake8_bugbear/B017.py @@ -23,6 +23,10 @@ def evil_raises(self) -> None: with self.assertRaises(Exception): raise Exception("Evil I say!") + def also_evil_raises(self) -> None: + with self.assertRaises(BaseException): + raise Exception("Evil I say!") + def context_manager_raises(self) -> None: with self.assertRaises(Exception) as ex: raise Exception("Context manager is good") @@ -41,6 +45,9 @@ def test_pytest_raises(): with pytest.raises(Exception): raise ValueError("Hello") + with pytest.raises(Exception), pytest.raises(ValueError): + raise ValueError("Hello") + with pytest.raises(Exception, "hello"): raise ValueError("This is fine") diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 012d999ea3c8a..bd408fb22a6a7 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1409,7 +1409,7 @@ where Stmt::With(ast::StmtWith { items, body, .. }) | Stmt::AsyncWith(ast::StmtAsyncWith { items, body, .. }) => { if self.enabled(Rule::AssertRaisesException) { - flake8_bugbear::rules::assert_raises_exception(self, stmt, items); + flake8_bugbear::rules::assert_raises_exception(self, items); } if self.enabled(Rule::PytestRaisesWithMultipleStatements) { flake8_pytest_style::rules::complex_raises(self, stmt, items, body); diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/assert_raises_exception.rs b/crates/ruff/src/rules/flake8_bugbear/rules/assert_raises_exception.rs index 7dfb6c6d912e2..d7a3994e76621 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/assert_raises_exception.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/assert_raises_exception.rs @@ -1,22 +1,20 @@ -use rustpython_parser::ast::{self, Expr, Ranged, Stmt, WithItem}; +use std::fmt; + +use rustpython_parser::ast::{self, Expr, Ranged, WithItem}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use crate::checkers::ast::Checker; -#[derive(Debug, PartialEq, Eq)] -pub(crate) enum AssertionKind { - AssertRaises, - PytestRaises, -} - /// ## What it does -/// Checks for `self.assertRaises(Exception)` or `pytest.raises(Exception)`. +/// Checks for `assertRaises` and `pytest.raises` context managers that catch +/// `Exception` or `BaseException`. /// /// ## Why is this bad? /// These forms catch every `Exception`, which can lead to tests passing even -/// if, e.g., the code being tested is never executed due to a typo. +/// if, e.g., the code under consideration raises a `SyntaxError` or +/// `IndentationError`. /// /// Either assert for a more specific exception (builtin or custom), or use /// `assertRaisesRegex` or `pytest.raises(..., match=)` respectively. @@ -32,51 +30,76 @@ pub(crate) enum AssertionKind { /// ``` #[violation] pub struct AssertRaisesException { - kind: AssertionKind, + assertion: AssertionKind, + exception: ExceptionKind, } impl Violation for AssertRaisesException { #[derive_message_formats] fn message(&self) -> String { - match self.kind { - AssertionKind::AssertRaises => { - format!("`assertRaises(Exception)` should be considered evil") - } - AssertionKind::PytestRaises => { - format!("`pytest.raises(Exception)` should be considered evil") - } - } + let AssertRaisesException { + assertion, + exception, + } = self; + format!("`{assertion}({exception})` should be considered evil") } } -/// B017 -pub(crate) fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: &[WithItem]) { - let Some(item) = items.first() else { - return; - }; - let item_context = &item.context_expr; - let Expr::Call(ast::ExprCall { func, args, keywords, range: _ }) = &item_context else { - return; - }; - if args.len() != 1 { - return; - } - if item.optional_vars.is_some() { - return; +#[derive(Debug, PartialEq, Eq)] +enum AssertionKind { + AssertRaises, + PytestRaises, +} + +impl fmt::Display for AssertionKind { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + match self { + AssertionKind::AssertRaises => fmt.write_str("assertRaises"), + AssertionKind::PytestRaises => fmt.write_str("pytest.raises"), + } } +} - if !checker - .semantic() - .resolve_call_path(args.first().unwrap()) - .map_or(false, |call_path| { - matches!(call_path.as_slice(), ["", "Exception"]) - }) - { - return; +#[derive(Debug, PartialEq, Eq)] +enum ExceptionKind { + BaseException, + Exception, +} + +impl fmt::Display for ExceptionKind { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + match self { + ExceptionKind::BaseException => fmt.write_str("BaseException"), + ExceptionKind::Exception => fmt.write_str("Exception"), + } } +} + +/// B017 +pub(crate) fn assert_raises_exception(checker: &mut Checker, items: &[WithItem]) { + for item in items { + let Expr::Call(ast::ExprCall { func, args, keywords, range: _ }) = &item.context_expr else { + return; + }; + if args.len() != 1 { + return; + } + if item.optional_vars.is_some() { + return; + } - let kind = { - if matches!(func.as_ref(), Expr::Attribute(ast::ExprAttribute { attr, .. }) if attr == "assertRaises") + let Some(exception) = checker + .semantic() + .resolve_call_path(args.first().unwrap()) + .and_then(|call_path| { + match call_path.as_slice() { + ["", "Exception"] => Some(ExceptionKind::Exception), + ["", "BaseException"] => Some(ExceptionKind::BaseException), + _ => None, + } + }) else { return; }; + + let assertion = if matches!(func.as_ref(), Expr::Attribute(ast::ExprAttribute { attr, .. }) if attr == "assertRaises") { AssertionKind::AssertRaises } else if checker @@ -92,11 +115,14 @@ pub(crate) fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: AssertionKind::PytestRaises } else { return; - } - }; + }; - checker.diagnostics.push(Diagnostic::new( - AssertRaisesException { kind }, - stmt.range(), - )); + checker.diagnostics.push(Diagnostic::new( + AssertRaisesException { + assertion, + exception, + }, + item.range(), + )); + } } diff --git a/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B017_B017.py.snap b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B017_B017.py.snap index f46fd7d74e088..d6a332dfc237b 100644 --- a/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B017_B017.py.snap +++ b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B017_B017.py.snap @@ -1,27 +1,38 @@ --- source: crates/ruff/src/rules/flake8_bugbear/mod.rs --- -B017.py:23:9: B017 `assertRaises(Exception)` should be considered evil +B017.py:23:14: B017 `assertRaises(Exception)` should be considered evil | -21 | class Foobar(unittest.TestCase): -22 | def evil_raises(self) -> None: -23 | with self.assertRaises(Exception): - | _________^ -24 | | raise Exception("Evil I say!") - | |__________________________________________^ B017 -25 | -26 | def context_manager_raises(self) -> None: +21 | class Foobar(unittest.TestCase): +22 | def evil_raises(self) -> None: +23 | with self.assertRaises(Exception): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017 +24 | raise Exception("Evil I say!") | -B017.py:41:5: B017 `pytest.raises(Exception)` should be considered evil - | -40 | def test_pytest_raises(): -41 | with pytest.raises(Exception): - | _____^ -42 | | raise ValueError("Hello") - | |_________________________________^ B017 -43 | -44 | with pytest.raises(Exception, "hello"): +B017.py:27:14: B017 `assertRaises(BaseException)` should be considered evil + | +26 | def also_evil_raises(self) -> None: +27 | with self.assertRaises(BaseException): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017 +28 | raise Exception("Evil I say!") + | + +B017.py:45:10: B017 `pytest.raises(Exception)` should be considered evil + | +44 | def test_pytest_raises(): +45 | with pytest.raises(Exception): + | ^^^^^^^^^^^^^^^^^^^^^^^^ B017 +46 | raise ValueError("Hello") + | + +B017.py:48:10: B017 `pytest.raises(Exception)` should be considered evil + | +46 | raise ValueError("Hello") +47 | +48 | with pytest.raises(Exception), pytest.raises(ValueError): + | ^^^^^^^^^^^^^^^^^^^^^^^^ B017 +49 | raise ValueError("Hello") | diff --git a/crates/ruff/src/rules/pyflakes/rules/yield_outside_function.rs b/crates/ruff/src/rules/pyflakes/rules/yield_outside_function.rs index 54fab4be1418f..f57f092ce54e0 100644 --- a/crates/ruff/src/rules/pyflakes/rules/yield_outside_function.rs +++ b/crates/ruff/src/rules/pyflakes/rules/yield_outside_function.rs @@ -9,7 +9,7 @@ use ruff_python_semantic::ScopeKind; use crate::checkers::ast::Checker; #[derive(Debug, PartialEq, Eq)] -pub(crate) enum DeferralKeyword { +enum DeferralKeyword { Yield, YieldFrom, Await,