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

Improve error messages for except* (B025, B029, B030, B904) #14791 #14815

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
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""
Should emit:
B025 - on lines 15, 22, 31
B025 - on lines 15, 22, 31, 40, 47, 56
"""

import pickle
Expand Down Expand Up @@ -36,3 +36,28 @@
a = 2
except (OSError, TypeError):
a = 2

try:
a = 1
except* ValueError:
a = 2
except* ValueError:
a = 2

try:
a = 1
except* pickle.PickleError:
a = 2
except* ValueError:
a = 2
except* pickle.PickleError:
a = 2

try:
a = 1
except* (ValueError, TypeError):
a = 2
except* ValueError:
a = 2
except* (OSError, TypeError):
a = 2
14 changes: 12 additions & 2 deletions crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B029.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""
Should emit:
B029 - on lines 8 and 13
B029 - on lines 8, 13, 18 and 23
"""

try:
Expand All @@ -11,4 +11,14 @@
try:
pass
except () as e:
pass
pass

try:
pass
except* ():
pass

try:
pass
except* () as e:
pass
20 changes: 20 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,23 @@ def what_to_catch():
pass
except (a, b) * (c, d): # B030
pass

try:
pass
except* 1: # Error
pass

try:
pass
except* (1, ValueError): # Error
pass

try:
pass
except* (ValueError, (RuntimeError, (KeyError, TypeError))): # Error
pass

try:
pass
except* (a, b) * (c, d): # B030
pass
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""
Should emit:
B904 - on lines 10, 11, 16, 62, and 64
B904 - on lines 10, 11, 16, 62, 64, 79, 81, 87, 88, 93 and 97
"""

try:
Expand Down Expand Up @@ -71,3 +71,29 @@ def context_switch():
match 0:
case 0:
raise RuntimeError("boom!")

try:
...
except* Exception as e:
if ...:
raise RuntimeError("boom!")
else:
raise RuntimeError("bang!")

try:
raise ValueError
except* ValueError:
if "abc":
raise TypeError
raise UserWarning
except* AssertionError:
raise # Bare `raise` should not be an error
except* Exception as err:
assert err
raise Exception("No cause here...")
except* BaseException as err:
raise err
except* BaseException as err:
raise some_other_err
finally:
raise Exception("Nothing to chain from, so no warning here")
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,18 @@ use crate::registry::Rule;
#[derive(ViolationMetadata)]
pub(crate) struct DuplicateTryBlockException {
name: String,
is_star: bool,
}

impl Violation for DuplicateTryBlockException {
#[derive_message_formats]
fn message(&self) -> String {
let DuplicateTryBlockException { name } = self;
format!("try-except block with duplicate exception `{name}`")
let DuplicateTryBlockException { name, is_star } = self;
if *is_star {
format!("try-except* block with duplicate exception `{name}`")
} else {
format!("try-except block with duplicate exception `{name}`")
}
}
}

Expand Down Expand Up @@ -207,9 +212,15 @@ pub(crate) fn duplicate_exceptions(checker: &mut Checker, handlers: &[ExceptHand
if checker.enabled(Rule::DuplicateTryBlockException) {
for (name, exprs) in duplicates {
for expr in exprs {
let is_star = checker
.semantic()
.current_statement()
.as_try_stmt()
.is_some_and(|try_stmt| try_stmt.is_star);
Comment on lines +215 to +219
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for learning purposes. @MichaReiser
Why was this moved inside the diagnostic-branch?

I thought calculating it outside the loop once would be better.

checker.diagnostics.push(Diagnostic::new(
DuplicateTryBlockException {
name: name.segments().join("."),
is_star,
},
expr.range(),
));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,18 @@ use crate::checkers::ast::Checker;
/// ## References
/// - [Python documentation: `except` clause](https://docs.python.org/3/reference/compound_stmts.html#except-clause)
#[derive(ViolationMetadata)]
pub(crate) struct ExceptWithEmptyTuple;
pub(crate) struct ExceptWithEmptyTuple {
is_star: bool,
}

impl Violation for ExceptWithEmptyTuple {
#[derive_message_formats]
fn message(&self) -> String {
"Using `except ():` with an empty tuple does not catch anything; add exceptions to handle"
.to_string()
if self.is_star {
"Using `except* ():` with an empty tuple does not catch anything; add exceptions to handle".to_string()
} else {
"Using `except ():` with an empty tuple does not catch anything; add exceptions to handle".to_string()
}
}
}

Expand All @@ -54,9 +59,15 @@ pub(crate) fn except_with_empty_tuple(checker: &mut Checker, except_handler: &Ex
let Expr::Tuple(ast::ExprTuple { elts, .. }) = type_.as_ref() else {
return;
};

if elts.is_empty() {
let is_star = checker
.semantic()
.current_statement()
.as_try_stmt()
.is_some_and(|try_stmt| try_stmt.is_star);
checker.diagnostics.push(Diagnostic::new(
ExceptWithEmptyTuple,
ExceptWithEmptyTuple { is_star },
except_handler.range(),
));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,20 @@ use crate::checkers::ast::Checker;
/// - [Python documentation: `except` clause](https://docs.python.org/3/reference/compound_stmts.html#except-clause)
/// - [Python documentation: Built-in Exceptions](https://docs.python.org/3/library/exceptions.html#built-in-exceptions)
#[derive(ViolationMetadata)]
pub(crate) struct ExceptWithNonExceptionClasses;
pub(crate) struct ExceptWithNonExceptionClasses {
is_star: bool,
}

impl Violation for ExceptWithNonExceptionClasses {
#[derive_message_formats]
fn message(&self) -> String {
"`except` handlers should only be exception classes or tuples of exception classes"
.to_string()
if self.is_star {
"`except*` handlers should only be exception classes or tuples of exception classes"
.to_string()
} else {
"`except` handlers should only be exception classes or tuples of exception classes"
.to_string()
}
}
}

Expand All @@ -60,9 +67,15 @@ pub(crate) fn except_with_non_exception_classes(
expr,
Expr::Subscript(_) | Expr::Attribute(_) | Expr::Name(_) | Expr::Call(_),
) {
checker
.diagnostics
.push(Diagnostic::new(ExceptWithNonExceptionClasses, expr.range()));
let is_star = checker
.semantic()
.current_statement()
.as_try_stmt()
.is_some_and(|try_stmt| try_stmt.is_star);
checker.diagnostics.push(Diagnostic::new(
ExceptWithNonExceptionClasses { is_star },
expr.range(),
));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,22 @@ use crate::checkers::ast::Checker;
/// ## References
/// - [Python documentation: `raise` statement](https://docs.python.org/3/reference/simple_stmts.html#the-raise-statement)
#[derive(ViolationMetadata)]
pub(crate) struct RaiseWithoutFromInsideExcept;
pub(crate) struct RaiseWithoutFromInsideExcept {
is_star: bool,
}

impl Violation for RaiseWithoutFromInsideExcept {
#[derive_message_formats]
fn message(&self) -> String {
"Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... \
from None` to distinguish them from errors in exception handling"
.to_string()
if self.is_star {
"Within an `except*` clause, raise exceptions with `raise ... from err` or `raise ... \
from None` to distinguish them from errors in exception handling"
.to_string()
} else {
"Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... \
from None` to distinguish them from errors in exception handling"
.to_string()
}
}
}

Expand Down Expand Up @@ -92,9 +100,16 @@ pub(crate) fn raise_without_from_inside_except(
}
}

checker
.diagnostics
.push(Diagnostic::new(RaiseWithoutFromInsideExcept, range));
let is_star = checker
.semantic()
.current_statement()
.as_try_stmt()
.is_some_and(|try_stmt| try_stmt.is_star);

checker.diagnostics.push(Diagnostic::new(
RaiseWithoutFromInsideExcept { is_star },
range,
));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,40 @@ B025.py:37:18: B025 try-except block with duplicate exception `TypeError`
| ^^^^^^^^^ B025
38 | a = 2
|

B025.py:44:9: B025 try-except* block with duplicate exception `ValueError`
|
42 | except* ValueError:
43 | a = 2
44 | except* ValueError:
| ^^^^^^^^^^ B025
45 | a = 2
|

B025.py:53:9: B025 try-except* block with duplicate exception `pickle.PickleError`
|
51 | except* ValueError:
52 | a = 2
53 | except* pickle.PickleError:
| ^^^^^^^^^^^^^^^^^^ B025
54 | a = 2
|

B025.py:60:9: B025 try-except* block with duplicate exception `ValueError`
|
58 | except* (ValueError, TypeError):
59 | a = 2
60 | except* ValueError:
| ^^^^^^^^^^ B025
61 | a = 2
62 | except* (OSError, TypeError):
|

B025.py:62:19: B025 try-except* block with duplicate exception `TypeError`
|
60 | except* ValueError:
61 | a = 2
62 | except* (OSError, TypeError):
| ^^^^^^^^^ B025
63 | a = 2
|
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,26 @@ B029.py:13:1: B029 Using `except ():` with an empty tuple does not catch anythin
13 | / except () as e:
14 | | pass
| |________^ B029
15 |
16 | try:
|

B029.py:18:1: B029 Using `except* ():` with an empty tuple does not catch anything; add exceptions to handle
|
16 | try:
17 | pass
18 | / except* ():
19 | | pass
| |________^ B029
20 |
21 | try:
|

B029.py:23:1: B029 Using `except* ():` with an empty tuple does not catch anything; add exceptions to handle
|
21 | try:
22 | pass
23 | / except* () as e:
24 | | pass
| |________^ B029
|
Loading
Loading