Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
"""
Should emit:
B017 - on lines 23 and 41
B017 - on lines 24, 28, 46, 49, 52, and 58
"""
import asyncio
import unittest
import pytest
import pytest, contextlib

CONSTANT = True

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"""
Should emit:
B017 - on lines 20, 21, 25, and 26
"""
import unittest
import pytest


def something_else() -> None:
for i in (1, 2, 3):
print(i)


class Foo:
pass


class Foobar(unittest.TestCase):
def call_form_raises(self) -> None:
self.assertRaises(Exception, something_else)
self.assertRaises(BaseException, something_else)


def test_pytest_call_form() -> None:
pytest.raises(Exception, something_else)
pytest.raises(BaseException, something_else)

pytest.raises(Exception, something_else, match="hello")
9 changes: 8 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ use ruff_python_semantic::analyze::typing;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::preview::is_optional_as_none_in_union_enabled;
use crate::preview::{
is_assert_raises_exception_call_enabled, is_optional_as_none_in_union_enabled,
};
use crate::registry::Rule;
use crate::rules::{
airflow, flake8_2020, flake8_async, flake8_bandit, flake8_boolean_trap, flake8_bugbear,
Expand Down Expand Up @@ -1230,6 +1232,11 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
if checker.is_rule_enabled(Rule::NonOctalPermissions) {
ruff::rules::non_octal_permissions(checker, call);
}
if checker.is_rule_enabled(Rule::AssertRaisesException)
&& is_assert_raises_exception_call_enabled(checker.settings())
{
flake8_bugbear::rules::assert_raises_exception_call(checker, call);
}
}
Expr::Dict(dict) => {
if checker.any_rule_enabled(&[
Expand Down
5 changes: 5 additions & 0 deletions crates/ruff_linter/src/preview.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,8 @@ pub(crate) const fn is_safe_super_call_with_parameters_fix_enabled(
) -> bool {
settings.preview.is_enabled()
}

// https://github.com/astral-sh/ruff/pull/19063
pub(crate) const fn is_assert_raises_exception_call_enabled(settings: &LinterSettings) -> bool {
settings.preview.is_enabled()
}
24 changes: 23 additions & 1 deletion crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@ mod tests {
use crate::settings::LinterSettings;
use crate::test::test_path;

use crate::settings::types::PreviewMode;

use ruff_python_ast::PythonVersion;

#[test_case(Rule::AbstractBaseClassWithoutAbstractMethod, Path::new("B024.py"))]
#[test_case(Rule::AssertFalse, Path::new("B011.py"))]
#[test_case(Rule::AssertRaisesException, Path::new("B017.py"))]
#[test_case(Rule::AssertRaisesException, Path::new("B017_0.py"))]
#[test_case(Rule::AssertRaisesException, Path::new("B017_1.py"))]
#[test_case(Rule::AssignmentToOsEnviron, Path::new("B003.py"))]
#[test_case(Rule::CachedInstanceMethod, Path::new("B019.py"))]
#[test_case(Rule::ClassAsDataStructure, Path::new("class_as_data_structure.py"))]
Expand Down Expand Up @@ -174,4 +177,23 @@ mod tests {
assert_diagnostics!(snapshot, diagnostics);
Ok(())
}

#[test_case(Rule::AssertRaisesException, Path::new("B017_0.py"))]
#[test_case(Rule::AssertRaisesException, Path::new("B017_1.py"))]
fn rules_preview(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
rule_code.noqa_code(),
path.to_string_lossy()
);
let diagnostics = test_path(
Path::new("flake8_bugbear").join(path).as_path(),
&LinterSettings {
preview: PreviewMode::Enabled,
..LinterSettings::for_rule(rule_code)
},
)?;
assert_diagnostics!(snapshot, diagnostics);
Ok(())
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::fmt;

use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::{self as ast, Expr, WithItem};
use ruff_python_ast::{self as ast, Arguments, Expr, WithItem};
use ruff_text_size::Ranged;

use crate::Violation;
Expand Down Expand Up @@ -56,6 +56,39 @@ impl fmt::Display for ExceptionKind {
}
}

fn detect_blind_exception(
semantic: &ruff_python_semantic::SemanticModel<'_>,
func: &Expr,
arguments: &Arguments,
) -> Option<ExceptionKind> {
let is_assert_raises = matches!(
func,
&Expr::Attribute(ast::ExprAttribute { ref attr, .. }) if attr.as_str() == "assertRaises"
);

let is_pytest_raises = semantic
.resolve_qualified_name(func)
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["pytest", "raises"]));

if !(is_assert_raises || is_pytest_raises) {
return None;
}

if is_pytest_raises && arguments.find_keyword("match").is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is supposed to be is_none like before right?

Suggested change
if is_pytest_raises && arguments.find_keyword("match").is_some() {
if is_pytest_raises && arguments.find_keyword("match").is_none() {

It's a little hard to tell because the diff shifted around, but I'm pretty sure one of the snapshots is showing a different result.

I think this is_some is actually correct and the existing is_none is a bug unless I'm misunderstanding something, but I'd rather handle that separately, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, I believe is_some() is the right test here. pytest.raises(..., match="pattern") is a form that already narrows the check to a specific exception message (equivalent to the assertRaisesRegex case for unittest), so when that match= keyword is present we want to skip B017, because it’s no longer a “blind” exception assertion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe I got confused by the logic here. The snapshot on main looks correct. Let's regroup the test cases and make sure none of the stable results change, but I think you may be right.

is_some definitely makes sense intuitively, I guess the condition was negated in the original code.

return None;
}

let first_arg = arguments.args.first()?;

let builtin_symbol = semantic.resolve_builtin_symbol(first_arg)?;

match builtin_symbol {
"Exception" => Some(ExceptionKind::Exception),
"BaseException" => Some(ExceptionKind::BaseException),
_ => None,
}
}

/// B017
pub(crate) fn assert_raises_exception(checker: &Checker, items: &[WithItem]) {
for item in items {
Expand All @@ -73,33 +106,31 @@ pub(crate) fn assert_raises_exception(checker: &Checker, items: &[WithItem]) {
continue;
}

let [arg] = &*arguments.args else {
continue;
};

let semantic = checker.semantic();

let Some(builtin_symbol) = semantic.resolve_builtin_symbol(arg) else {
continue;
};

let exception = match builtin_symbol {
"Exception" => ExceptionKind::Exception,
"BaseException" => ExceptionKind::BaseException,
_ => continue,
};

if !(matches!(func.as_ref(), Expr::Attribute(ast::ExprAttribute { attr, .. }) if attr == "assertRaises")
|| semantic
.resolve_qualified_name(func)
.is_some_and(|qualified_name| {
matches!(qualified_name.segments(), ["pytest", "raises"])
})
&& arguments.find_keyword("match").is_none())
if let Some(exception) =
detect_blind_exception(checker.semantic(), func.as_ref(), arguments)
{
continue;
checker.report_diagnostic(AssertRaisesException { exception }, item.range());
}
}
}

/// B017 (call form)
pub(crate) fn assert_raises_exception_call(
checker: &Checker,
ast::ExprCall {
func,
arguments,
range,
node_index: _,
}: &ast::ExprCall,
) {
let semantic = checker.semantic();

if arguments.args.len() < 2 && arguments.find_argument("func", 1).is_none() {
return;
}

checker.report_diagnostic(AssertRaisesException { exception }, item.range());
if let Some(exception) = detect_blind_exception(semantic, func.as_ref(), arguments) {
checker.report_diagnostic(AssertRaisesException { exception }, *range);
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
---
B017.py:23:14: B017 Do not assert blind exception: `Exception`
B017_0.py:23:14: B017 Do not assert blind exception: `Exception`
|
21 | class Foobar(unittest.TestCase):
22 | def evil_raises(self) -> None:
Expand All @@ -10,23 +10,23 @@ B017.py:23:14: B017 Do not assert blind exception: `Exception`
24 | raise Exception("Evil I say!")
|

B017.py:27:14: B017 Do not assert blind exception: `BaseException`
B017_0.py:27:14: B017 Do not assert blind exception: `BaseException`
|
26 | def also_evil_raises(self) -> None:
27 | with self.assertRaises(BaseException):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017
28 | raise Exception("Evil I say!")
|

B017.py:45:10: B017 Do not assert blind exception: `Exception`
B017_0.py:45:10: B017 Do not assert blind exception: `Exception`
|
44 | def test_pytest_raises():
45 | with pytest.raises(Exception):
| ^^^^^^^^^^^^^^^^^^^^^^^^ B017
46 | raise ValueError("Hello")
|

B017.py:48:10: B017 Do not assert blind exception: `Exception`
B017_0.py:48:10: B017 Do not assert blind exception: `Exception`
|
46 | raise ValueError("Hello")
47 |
Expand All @@ -35,7 +35,16 @@ B017.py:48:10: B017 Do not assert blind exception: `Exception`
49 | raise ValueError("Hello")
|

B017.py:57:36: B017 Do not assert blind exception: `Exception`
B017_0.py:51:10: B017 Do not assert blind exception: `Exception`
|
49 | raise ValueError("Hello")
50 |
51 | with pytest.raises(Exception, "hello"):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017
52 | raise ValueError("This is fine")
|

B017_0.py:57:36: B017 Do not assert blind exception: `Exception`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a couple of commits to minimize the diff a bit more. I think something is slightly off with the new logic because it looks like this is getting flagged now when it should be allowed, but I haven't pinned down where it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the issue—we weren't considering the second positional string/bytes literal argument. I added in that additional check so now it's not registering as a false positive anymore.

|
55 | raise ValueError("This is also fine")
56 |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
---

Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
---
source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
---
B017_0.py:23:14: B017 Do not assert blind exception: `Exception`
|
21 | class Foobar(unittest.TestCase):
22 | def evil_raises(self) -> None:
23 | with self.assertRaises(Exception):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017
24 | raise Exception("Evil I say!")
|

B017_0.py:27:14: B017 Do not assert blind exception: `BaseException`
|
26 | def also_evil_raises(self) -> None:
27 | with self.assertRaises(BaseException):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017
28 | raise Exception("Evil I say!")
|

B017_0.py:45:10: B017 Do not assert blind exception: `Exception`
|
44 | def test_pytest_raises():
45 | with pytest.raises(Exception):
| ^^^^^^^^^^^^^^^^^^^^^^^^ B017
46 | raise ValueError("Hello")
|

B017_0.py:48:10: B017 Do not assert blind exception: `Exception`
|
46 | raise ValueError("Hello")
47 |
48 | with pytest.raises(Exception), pytest.raises(ValueError):
| ^^^^^^^^^^^^^^^^^^^^^^^^ B017
49 | raise ValueError("Hello")
|

B017_0.py:51:10: B017 Do not assert blind exception: `Exception`
|
49 | raise ValueError("Hello")
50 |
51 | with pytest.raises(Exception, "hello"):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017
52 | raise ValueError("This is fine")
|

B017_0.py:51:10: B017 Do not assert blind exception: `Exception`
|
49 | raise ValueError("Hello")
50 |
51 | with pytest.raises(Exception, "hello"):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017
52 | raise ValueError("This is fine")
|

B017_0.py:57:36: B017 Do not assert blind exception: `Exception`
|
55 | raise ValueError("This is also fine")
56 |
57 | with contextlib.nullcontext(), pytest.raises(Exception):
| ^^^^^^^^^^^^^^^^^^^^^^^^ B017
58 | raise ValueError("Multiple context managers")
|
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
---
source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs
---
B017_1.py:20:9: B017 Do not assert blind exception: `Exception`
|
18 | class Foobar(unittest.TestCase):
19 | def call_form_raises(self) -> None:
20 | self.assertRaises(Exception, something_else)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017
21 | self.assertRaises(BaseException, something_else)
|

B017_1.py:21:9: B017 Do not assert blind exception: `BaseException`
|
19 | def call_form_raises(self) -> None:
20 | self.assertRaises(Exception, something_else)
21 | self.assertRaises(BaseException, something_else)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017
|

B017_1.py:25:5: B017 Do not assert blind exception: `Exception`
|
24 | def test_pytest_call_form() -> None:
25 | pytest.raises(Exception, something_else)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017
26 | pytest.raises(BaseException, something_else)
|

B017_1.py:26:5: B017 Do not assert blind exception: `BaseException`
|
24 | def test_pytest_call_form() -> None:
25 | pytest.raises(Exception, something_else)
26 | pytest.raises(BaseException, something_else)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B017
27 |
28 | pytest.raises(Exception, something_else, match="hello")
|
Loading