Skip to content

Commit

Permalink
[flake8-bugbear] Allow tuples of exceptions (B030) (#10437)
Browse files Browse the repository at this point in the history
Fixes #10426 

## Summary

Fix rule B030 giving a false positive with Tuple operations like `+`.

[Playground](https://play.ruff.rs/17b086bc-cc43-40a7-b5bf-76d7d5fce78a)
```python
try:
    ...
except (ValueError,TypeError) + (EOFError,ArithmeticError):
    ...
```

## Reviewer notes

This is a little more convoluted than I was expecting -- because we can
have valid nested Tuples with operations done on them, the flattening
logic has become a bit more complex.

Shall I guard this behind --preview?

## Test Plan

Unit tested.
  • Loading branch information
ottaviohartman committed Mar 18, 2024
1 parent 526abeb commit 6123a5b
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 43 deletions.
78 changes: 66 additions & 12 deletions crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,62 +9,69 @@

try:
pass
except 1: # error
except 1: # Error
pass

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

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

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


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


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


try:
pass
except (ValueError, *(RuntimeError, TypeError)): # ok
except (ValueError, *(RuntimeError, TypeError)): # OK
pass

try:
pass
except (ValueError, *[RuntimeError, *(TypeError,)]): # ok
except (ValueError, *[RuntimeError, *(TypeError,)]): # OK
pass


try:
pass
except (*a, *b): # ok
except (*a, *b): # OK
pass


try:
pass
except (*a, *(RuntimeError, TypeError)): # ok
except (*a, *(RuntimeError, TypeError)): # OK
pass


try:
pass
except (*a, *(b, c)): # ok
except (*a, *(b, c)): # OK
pass


try:
pass
except (*a, *(*b, *c)): # ok
except (*a, *(*b, *c)): # OK
pass


Expand All @@ -74,5 +81,52 @@ def what_to_catch():

try:
pass
except what_to_catch(): # ok
except what_to_catch(): # OK
pass


try:
pass
except (a, b) + (c, d): # OK
pass


try:
pass
except* (a, b) + (c, d): # OK
pass


try:
pass
except* (a, (b) + (c)): # OK
pass


try:
pass
except (a, b) + (c, d) + (e, f): # OK
pass


try:
pass
except a + (b, c): # OK
pass


try:
pass
except (ValueError, *(RuntimeError, TypeError), *((ArithmeticError,) + (EOFError,))):
pass


try:
pass
except ((a, b) + (c, d)) + ((e, f) + (g)): # OK
pass

try:
pass
except (a, b) * (c, d): # B030
pass
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::collections::VecDeque;

use ruff_python_ast::{self as ast, ExceptHandler, Expr};
use ruff_python_ast::{self as ast, ExceptHandler, Expr, Operator};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
Expand Down Expand Up @@ -44,30 +44,6 @@ impl Violation for ExceptWithNonExceptionClasses {
}
}

/// Given an [`Expr`], flatten any [`Expr::Starred`] expressions.
/// This should leave any unstarred iterables alone (subsequently raising a
/// warning for B029).
fn flatten_starred_iterables(expr: &Expr) -> Vec<&Expr> {
let Expr::Tuple(ast::ExprTuple { elts, .. }) = expr else {
return vec![expr];
};
let mut flattened_exprs: Vec<&Expr> = Vec::with_capacity(elts.len());
let mut exprs_to_process: VecDeque<&Expr> = elts.iter().collect();
while let Some(expr) = exprs_to_process.pop_front() {
match expr {
Expr::Starred(ast::ExprStarred { value, .. }) => match value.as_ref() {
Expr::Tuple(ast::ExprTuple { elts, .. })
| Expr::List(ast::ExprList { elts, .. }) => {
exprs_to_process.append(&mut elts.iter().collect());
}
_ => flattened_exprs.push(value),
},
_ => flattened_exprs.push(expr),
}
}
flattened_exprs
}

/// B030
pub(crate) fn except_with_non_exception_classes(
checker: &mut Checker,
Expand All @@ -78,7 +54,7 @@ pub(crate) fn except_with_non_exception_classes(
let Some(type_) = type_ else {
return;
};
for expr in flatten_starred_iterables(type_) {
for expr in flatten_iterables(type_) {
if !matches!(
expr,
Expr::Subscript(_) | Expr::Attribute(_) | Expr::Name(_) | Expr::Call(_),
Expand All @@ -89,3 +65,61 @@ pub(crate) fn except_with_non_exception_classes(
}
}
}

/// Given an [`Expr`], flatten any [`Expr::Starred`] expressions and any
/// [`Expr::BinOp`] expressions into a flat list of expressions.
///
/// This should leave any unstarred iterables alone (subsequently raising a
/// warning for B029).
fn flatten_iterables(expr: &Expr) -> Vec<&Expr> {
// Unpack the top-level Tuple into queue, otherwise add as-is.
let mut exprs_to_process: VecDeque<&Expr> = match expr {
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().collect(),
_ => vec![expr].into(),
};
let mut flattened_exprs: Vec<&Expr> = Vec::with_capacity(exprs_to_process.len());

while let Some(expr) = exprs_to_process.pop_front() {
match expr {
Expr::Starred(ast::ExprStarred { value, .. }) => match value.as_ref() {
Expr::Tuple(ast::ExprTuple { elts, .. })
| Expr::List(ast::ExprList { elts, .. }) => {
exprs_to_process.append(&mut elts.iter().collect());
}
Expr::BinOp(ast::ExprBinOp {
op: Operator::Add, ..
}) => {
exprs_to_process.push_back(value);
}
_ => flattened_exprs.push(value),
},
Expr::BinOp(ast::ExprBinOp {
left,
right,
op: Operator::Add,
..
}) => {
for expr in [left, right] {
// If left or right are tuples, starred, or binary operators, flatten them.
match expr.as_ref() {
Expr::Tuple(ast::ExprTuple { elts, .. }) => {
exprs_to_process.append(&mut elts.iter().collect());
}
Expr::Starred(ast::ExprStarred { value, .. }) => {
exprs_to_process.push_back(value);
}
Expr::BinOp(ast::ExprBinOp {
op: Operator::Add, ..
}) => {
exprs_to_process.push_back(expr);
}
_ => flattened_exprs.push(expr),
}
}
}
_ => flattened_exprs.push(expr),
}
}

flattened_exprs
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ B030.py:12:8: B030 `except` handlers should only be exception classes or tuples
|
10 | try:
11 | pass
12 | except 1: # error
12 | except 1: # Error
| ^ B030
13 | pass
|
Expand All @@ -14,7 +14,7 @@ B030.py:17:9: B030 `except` handlers should only be exception classes or tuples
|
15 | try:
16 | pass
17 | except (1, ValueError): # error
17 | except (1, ValueError): # Error
| ^ B030
18 | pass
|
Expand All @@ -23,7 +23,7 @@ B030.py:22:21: B030 `except` handlers should only be exception classes or tuples
|
20 | try:
21 | pass
22 | except (ValueError, (RuntimeError, (KeyError, TypeError))): # error
22 | except (ValueError, (RuntimeError, (KeyError, TypeError))): # Error
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B030
23 | pass
|
Expand All @@ -32,7 +32,7 @@ B030.py:27:37: B030 `except` handlers should only be exception classes or tuples
|
25 | try:
26 | pass
27 | except (ValueError, *(RuntimeError, (KeyError, TypeError))): # error
27 | except (ValueError, *(RuntimeError, (KeyError, TypeError))): # Error
| ^^^^^^^^^^^^^^^^^^^^^ B030
28 | pass
|
Expand All @@ -41,9 +41,25 @@ B030.py:33:29: B030 `except` handlers should only be exception classes or tuples
|
31 | try:
32 | pass
33 | except (*a, *(RuntimeError, (KeyError, TypeError))): # error
33 | except (*a, *(RuntimeError, (KeyError, TypeError))): # Error
| ^^^^^^^^^^^^^^^^^^^^^ B030
34 | pass
|

B030.py:39:28: B030 `except` handlers should only be exception classes or tuples of exception classes
|
37 | try:
38 | pass
39 | except* a + (RuntimeError, (KeyError, TypeError)): # Error
| ^^^^^^^^^^^^^^^^^^^^^ B030
40 | pass
|

B030.py:131:8: B030 `except` handlers should only be exception classes or tuples of exception classes
|
129 | try:
130 | pass
131 | except (a, b) * (c, d): # B030
| ^^^^^^^^^^^^^^^ B030
132 | pass
|

0 comments on commit 6123a5b

Please sign in to comment.