diff --git a/README.md b/README.md index bd0396f6df..8016785100 100644 --- a/README.md +++ b/README.md @@ -382,6 +382,7 @@ The 🛠 emoji indicates that a rule is automatically fixable by the `--fix` com | ---- | ---- | ------- | --- | | B011 | DoNotAssertFalse | Do not `assert False` (`python -O` removes these calls), raise `AssertionError()` | 🛠 | | B014 | DuplicateHandlerException | Exception handler with duplicate exception: `ValueError` | 🛠 | +| B017 | NoAssertRaisesException | `assertRaises(Exception):` should be considered evil. | | | B025 | DuplicateTryBlockException | try-except block with duplicate exception `Exception` | | ### flake8-builtins @@ -471,7 +472,7 @@ including: - [`flake8-super`](https://pypi.org/project/flake8-super/) - [`flake8-print`](https://pypi.org/project/flake8-print/) - [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) -- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (3/32) +- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (4/32) - [`pyupgrade`](https://pypi.org/project/pyupgrade/) (8/34) - [`autoflake`](https://pypi.org/project/autoflake/) (1/7) @@ -491,7 +492,7 @@ Today, Ruff can be used to replace Flake8 when used with any of the following pl - [`flake8-super`](https://pypi.org/project/flake8-super/) - [`flake8-print`](https://pypi.org/project/flake8-print/) - [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) -- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (3/32) +- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (4/32) Ruff also implements the functionality that you get from [`yesqa`](https://github.com/asottile/yesqa), and a subset of the rules implemented in [`pyupgrade`](https://pypi.org/project/pyupgrade/) (8/34). diff --git a/examples/generate_rules_table.rs b/examples/generate_rules_table.rs index 6317883c46..357b2ae7d8 100644 --- a/examples/generate_rules_table.rs +++ b/examples/generate_rules_table.rs @@ -19,7 +19,7 @@ fn main() { "| {} | {} | {} | {} |", check_kind.code().as_ref(), check_kind.as_ref(), - check_kind.body().replace("|", r"\|"), + check_kind.summary().replace("|", r"\|"), fix_token ); } diff --git a/resources/test/fixtures/B017.py b/resources/test/fixtures/B017.py new file mode 100644 index 0000000000..6332fc487e --- /dev/null +++ b/resources/test/fixtures/B017.py @@ -0,0 +1,36 @@ +""" +Should emit: +B017 - on lines 20 +""" +import asyncio +import unittest + +CONSTANT = True + + +def something_else() -> None: + for i in (1, 2, 3): + print(i) + + +class Foo: + pass + + +class Foobar(unittest.TestCase): + def evil_raises(self) -> None: + with self.assertRaises(Exception): + raise Exception("Evil I say!") + + def context_manager_raises(self) -> None: + with self.assertRaises(Exception) as ex: + raise Exception("Context manager is good") + self.assertEqual("Context manager is good", str(ex.exception)) + + def regex_raises(self) -> None: + with self.assertRaisesRegex(Exception, "Regex is good"): + raise Exception("Regex is good") + + def raises_with_absolute_reference(self): + with self.assertRaises(asyncio.CancelledError): + Foo() diff --git a/src/check_ast.rs b/src/check_ast.rs index 2becdc9a4a..c63c39f226 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -667,6 +667,11 @@ where flake8_bugbear::plugins::assert_false(self, stmt, test, msg); } } + StmtKind::With { items, .. } | StmtKind::AsyncWith { items, .. } => { + if self.settings.enabled.contains(&CheckCode::B017) { + flake8_bugbear::plugins::assert_raises_exception(self, stmt, items); + } + } StmtKind::Try { handlers, .. } => { if self.settings.enabled.contains(&CheckCode::F707) { if let Some(check) = pyflakes::checks::default_except_not_last(handlers) { diff --git a/src/checks.rs b/src/checks.rs index 13207b64d8..96b1387ea3 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -77,6 +77,7 @@ pub enum CheckCode { // flake8-bugbear B011, B014, + B017, B025, // flake8-comprehensions C400, @@ -267,6 +268,7 @@ pub enum CheckKind { // flake8-bugbear DoNotAssertFalse, DuplicateHandlerException(Vec), + NoAssertRaisesException, DuplicateTryBlockException(String), // flake8-comprehensions UnnecessaryGeneratorList, @@ -426,6 +428,7 @@ impl CheckCode { // flake8-bugbear CheckCode::B011 => CheckKind::DoNotAssertFalse, CheckCode::B014 => CheckKind::DuplicateHandlerException(vec!["ValueError".to_string()]), + CheckCode::B017 => CheckKind::NoAssertRaisesException, CheckCode::B025 => CheckKind::DuplicateTryBlockException("Exception".to_string()), // flake8-comprehensions CheckCode::C400 => CheckKind::UnnecessaryGeneratorList, @@ -600,6 +603,7 @@ impl CheckCode { CheckCode::A003 => CheckCategory::Flake8Builtins, CheckCode::B011 => CheckCategory::Flake8Bugbear, CheckCode::B014 => CheckCategory::Flake8Bugbear, + CheckCode::B017 => CheckCategory::Flake8Bugbear, CheckCode::B025 => CheckCategory::Flake8Bugbear, CheckCode::C400 => CheckCategory::Flake8Comprehensions, CheckCode::C401 => CheckCategory::Flake8Comprehensions, @@ -743,6 +747,7 @@ impl CheckKind { // flake8-bugbear CheckKind::DoNotAssertFalse => &CheckCode::B011, CheckKind::DuplicateHandlerException(_) => &CheckCode::B014, + CheckKind::NoAssertRaisesException => &CheckCode::B017, CheckKind::DuplicateTryBlockException(_) => &CheckCode::B025, // flake8-comprehensions CheckKind::UnnecessaryGeneratorList => &CheckCode::C400, @@ -992,6 +997,9 @@ impl CheckKind { format!("Exception handler with duplicate exceptions: {names}") } } + CheckKind::NoAssertRaisesException => { + "`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`.".to_string() + } CheckKind::DuplicateTryBlockException(name) => { format!("try-except block with duplicate exception `{name}`") } @@ -1258,6 +1266,16 @@ impl CheckKind { } } + /// The summary text for the check. + pub fn summary(&self) -> String { + match self { + CheckKind::NoAssertRaisesException => { + "`assertRaises(Exception):` should be considered evil.".to_string() + } + _ => self.body(), + } + } + /// Whether the check kind is (potentially) fixable. pub fn fixable(&self) -> bool { matches!( diff --git a/src/flake8_bugbear/plugins/assert_raises_exception.rs b/src/flake8_bugbear/plugins/assert_raises_exception.rs new file mode 100644 index 0000000000..c408f4b7b9 --- /dev/null +++ b/src/flake8_bugbear/plugins/assert_raises_exception.rs @@ -0,0 +1,25 @@ +use rustpython_ast::{ExprKind, Stmt, Withitem}; + +use crate::ast::helpers::match_name_or_attr; +use crate::ast::types::Range; +use crate::check_ast::Checker; +use crate::checks::{Check, CheckKind}; + +/// B017 +pub fn assert_raises_exception(checker: &mut Checker, stmt: &Stmt, items: &[Withitem]) { + if let Some(item) = items.first() { + let item_context = &item.context_expr; + if let ExprKind::Call { func, args, .. } = &item_context.node { + if match_name_or_attr(func, "assertRaises") + && args.len() == 1 + && match_name_or_attr(args.first().unwrap(), "Exception") + && item.optional_vars.is_none() + { + checker.add_check(Check::new( + CheckKind::NoAssertRaisesException, + Range::from_located(stmt), + )); + } + } + } +} diff --git a/src/flake8_bugbear/plugins/mod.rs b/src/flake8_bugbear/plugins/mod.rs index fc09f2ab5f..4ce3535705 100644 --- a/src/flake8_bugbear/plugins/mod.rs +++ b/src/flake8_bugbear/plugins/mod.rs @@ -1,6 +1,8 @@ pub use assert_false::assert_false; +pub use assert_raises_exception::assert_raises_exception; pub use duplicate_exceptions::duplicate_exceptions; pub use duplicate_exceptions::duplicate_handler_exceptions; mod assert_false; +mod assert_raises_exception; mod duplicate_exceptions; diff --git a/src/linter.rs b/src/linter.rs index 7eb8589ae7..74af40eac0 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -247,6 +247,7 @@ mod tests { #[test_case(CheckCode::A003, Path::new("A003.py"); "A003")] #[test_case(CheckCode::B011, Path::new("B011.py"); "B011")] #[test_case(CheckCode::B014, Path::new("B014.py"); "B014")] + #[test_case(CheckCode::B017, Path::new("B017.py"); "B017")] #[test_case(CheckCode::B025, Path::new("B025.py"); "B025")] #[test_case(CheckCode::C400, Path::new("C400.py"); "C400")] #[test_case(CheckCode::C401, Path::new("C401.py"); "C401")] diff --git a/src/snapshots/ruff__linter__tests__B017_B017.py.snap b/src/snapshots/ruff__linter__tests__B017_B017.py.snap new file mode 100644 index 0000000000..87adcc39b6 --- /dev/null +++ b/src/snapshots/ruff__linter__tests__B017_B017.py.snap @@ -0,0 +1,13 @@ +--- +source: src/linter.rs +expression: checks +--- +- kind: NoAssertRaisesException + location: + row: 22 + column: 9 + end_location: + row: 25 + column: 5 + fix: ~ +