Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include BaseException in B017 rule #5466

Merged
merged 1 commit into from
Jul 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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