Skip to content

Commit

Permalink
Include BaseException in B017 rule
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Jul 2, 2023
1 parent f0ec9ec commit 1238c86
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 68 deletions.
7 changes: 7 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_bugbear/B017.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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")

Expand Down
2 changes: 1 addition & 1 deletion crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
122 changes: 74 additions & 48 deletions crates/ruff/src/rules/flake8_bugbear/rules/assert_raises_exception.rs
Original file line number Diff line number Diff line change
@@ -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=<REGEX>)` respectively.
Expand All @@ -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
Expand All @@ -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(),
));
}
}
Original file line number Diff line number Diff line change
@@ -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")
|


Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 1238c86

Please sign in to comment.