From e3f71c8a969607d526386f099a2b6fc774684bce Mon Sep 17 00:00:00 2001 From: Ottavio Hartman Date: Sat, 16 Mar 2024 23:06:25 -0400 Subject: [PATCH 01/10] working but not with star packed tuples --- .../test/fixtures/flake8_bugbear/B030.py | 18 +++++++++++++++ .../except_with_non_exception_classes.rs | 23 ++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py index 7152536a7cbbe..0a9c78ea99041 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py @@ -76,3 +76,21 @@ def what_to_catch(): pass except what_to_catch(): # ok pass + + +try: + pass +except (ValueError,TypeError) + (EOFError,ArithmeticError): # ok + pass + + +try: + pass +except *(ValueError,TypeError) + (EOFError,ArithmeticError): # ok + pass + + +try: + pass +except *(ValueError,(TypeError,) + (EOFError)): # ok + pass \ No newline at end of file diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs index 78557f0bc7181..70bd2257b51af 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs @@ -68,6 +68,23 @@ fn flatten_starred_iterables(expr: &Expr) -> Vec<&Expr> { flattened_exprs } +/// Given an [`Expr`], flatten any binary operations. +fn flatten_bin_op(expr: &Expr) -> Vec<&Expr> { + let mut stack = vec![expr]; + let mut flattened_exprs = Vec::new(); + + while let Some(expr) = stack.pop() { + if let Expr::BinOp(ast::ExprBinOp { left, right, .. }) = expr { + stack.push(left); + stack.push(right); + } else { + flattened_exprs.push(expr); + } + } + + flattened_exprs +} + /// B030 pub(crate) fn except_with_non_exception_classes( checker: &mut Checker, @@ -78,7 +95,11 @@ pub(crate) fn except_with_non_exception_classes( let Some(type_) = type_ else { return; }; - for expr in flatten_starred_iterables(type_) { + let flattened_exprs = flatten_bin_op(type_); + let flattened_exprs = flattened_exprs + .iter() + .flat_map(|expr| flatten_starred_iterables(expr)); + for expr in flattened_exprs { if !matches!( expr, Expr::Subscript(_) | Expr::Attribute(_) | Expr::Name(_) | Expr::Call(_), From 1c76c3a84206d067020660d5791598b7d475dee9 Mon Sep 17 00:00:00 2001 From: Ottavio Hartman Date: Sun, 17 Mar 2024 10:42:53 -0400 Subject: [PATCH 02/10] test passing --- .../test/fixtures/flake8_bugbear/B030.py | 9 ++-- .../except_with_non_exception_classes.rs | 50 +++++++++++++++---- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py index 0a9c78ea99041..1f3fe6dc96536 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py @@ -33,6 +33,7 @@ except (*a, *(RuntimeError, (KeyError, TypeError))): # error pass + try: pass except (ValueError, *(RuntimeError, TypeError)): # ok @@ -80,17 +81,17 @@ def what_to_catch(): try: pass -except (ValueError,TypeError) + (EOFError,ArithmeticError): # ok +except (a, b) + (c, d): # ok pass try: pass -except *(ValueError,TypeError) + (EOFError,ArithmeticError): # ok +except *(a, b) + (c, d): # ok pass try: pass -except *(ValueError,(TypeError,) + (EOFError)): # ok - pass \ No newline at end of file +except *(a, (b) + (c)): # ok + pass diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs index 70bd2257b51af..d824c0ef0377f 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs @@ -48,11 +48,19 @@ impl Violation for ExceptWithNonExceptionClasses { /// 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 Expr::Tuple(ast::ExprTuple { elts, .. }) = expr else { + // return vec![expr]; + // }; + // let mut exprs_to_process: VecDeque<&Expr> = elts.iter().collect(); + let mut exprs_to_process: VecDeque<&Expr> = match expr { + Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().collect(), + Expr::BinOp(ast::ExprBinOp { .. }) => { + // vec![left.as_ref(), right.as_ref()].into() + vec![expr].into() + } + _ => return vec![expr], }; - let mut flattened_exprs: Vec<&Expr> = Vec::with_capacity(elts.len()); - let mut exprs_to_process: VecDeque<&Expr> = elts.iter().collect(); + let mut flattened_exprs: Vec<&Expr> = Vec::new(); while let Some(expr) = exprs_to_process.pop_front() { match expr { Expr::Starred(ast::ExprStarred { value, .. }) => match value.as_ref() { @@ -62,6 +70,27 @@ fn flatten_starred_iterables(expr: &Expr) -> Vec<&Expr> { } _ => flattened_exprs.push(value), }, + Expr::BinOp(ast::ExprBinOp { left, right, .. }) => { + // if left or right are tuples, we should flatten them + match left.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); + } + _ => flattened_exprs.push(left), + } + match right.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); + } + _ => flattened_exprs.push(right), + } + } _ => flattened_exprs.push(expr), } } @@ -77,7 +106,10 @@ fn flatten_bin_op(expr: &Expr) -> Vec<&Expr> { if let Expr::BinOp(ast::ExprBinOp { left, right, .. }) = expr { stack.push(left); stack.push(right); + // } else if let Expr::Tuple(ast::ExprTuple { elts, .. }) = expr { + // stack.extend(elts.iter()); } else { + println!("Pushing expression: {:#?}", expr); flattened_exprs.push(expr); } } @@ -95,15 +127,15 @@ pub(crate) fn except_with_non_exception_classes( let Some(type_) = type_ else { return; }; - let flattened_exprs = flatten_bin_op(type_); - let flattened_exprs = flattened_exprs - .iter() - .flat_map(|expr| flatten_starred_iterables(expr)); - for expr in flattened_exprs { + // let flattened_exprs = flatten_starred_iterables(type_); + // println!("{:#?}", flattened_exprs); + // let flattened_exprs = flattened_exprs.iter().flat_map(|expr| flatten_bin_op(expr)); + for expr in flatten_starred_iterables(type_) { if !matches!( expr, Expr::Subscript(_) | Expr::Attribute(_) | Expr::Name(_) | Expr::Call(_), ) { + println!("Adding diagnostic for expression: {:#?}", expr); checker .diagnostics .push(Diagnostic::new(ExceptWithNonExceptionClasses, expr.range())); From e49bbd20ffafbcf3a70bbba05fa51dc1c3a456f8 Mon Sep 17 00:00:00 2001 From: Ottavio Hartman Date: Sun, 17 Mar 2024 10:51:44 -0400 Subject: [PATCH 03/10] cleanup --- .../except_with_non_exception_classes.rs | 67 +++++-------------- 1 file changed, 16 insertions(+), 51 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs index d824c0ef0377f..84e1346daa43c 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs @@ -44,22 +44,18 @@ impl Violation for ExceptWithNonExceptionClasses { } } -/// Given an [`Expr`], flatten any [`Expr::Starred`] expressions. +/// 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_starred_iterables(expr: &Expr) -> Vec<&Expr> { - // let Expr::Tuple(ast::ExprTuple { elts, .. }) = expr else { - // return vec![expr]; - // }; - // let mut exprs_to_process: VecDeque<&Expr> = elts.iter().collect(); +fn flatten_starred_iterables_and_binops(expr: &Expr) -> Vec<&Expr> { + // The top-level expression should be a tuple or a binop, else return as-is. let mut exprs_to_process: VecDeque<&Expr> = match expr { Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().collect(), - Expr::BinOp(ast::ExprBinOp { .. }) => { - // vec![left.as_ref(), right.as_ref()].into() - vec![expr].into() - } + Expr::BinOp(ast::ExprBinOp { .. }) => vec![expr].into(), _ => return vec![expr], }; + let mut flattened_exprs: Vec<&Expr> = Vec::new(); while let Some(expr) = exprs_to_process.pop_front() { match expr { @@ -72,23 +68,16 @@ fn flatten_starred_iterables(expr: &Expr) -> Vec<&Expr> { }, Expr::BinOp(ast::ExprBinOp { left, right, .. }) => { // if left or right are tuples, we should flatten them - match left.as_ref() { - Expr::Tuple(ast::ExprTuple { elts, .. }) => { - exprs_to_process.append(&mut elts.iter().collect()); + for expr in [left, right] { + 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); + } + _ => flattened_exprs.push(expr), } - Expr::Starred(ast::ExprStarred { value, .. }) => { - exprs_to_process.push_back(value); - } - _ => flattened_exprs.push(left), - } - match right.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); - } - _ => flattened_exprs.push(right), } } _ => flattened_exprs.push(expr), @@ -97,26 +86,6 @@ fn flatten_starred_iterables(expr: &Expr) -> Vec<&Expr> { flattened_exprs } -/// Given an [`Expr`], flatten any binary operations. -fn flatten_bin_op(expr: &Expr) -> Vec<&Expr> { - let mut stack = vec![expr]; - let mut flattened_exprs = Vec::new(); - - while let Some(expr) = stack.pop() { - if let Expr::BinOp(ast::ExprBinOp { left, right, .. }) = expr { - stack.push(left); - stack.push(right); - // } else if let Expr::Tuple(ast::ExprTuple { elts, .. }) = expr { - // stack.extend(elts.iter()); - } else { - println!("Pushing expression: {:#?}", expr); - flattened_exprs.push(expr); - } - } - - flattened_exprs -} - /// B030 pub(crate) fn except_with_non_exception_classes( checker: &mut Checker, @@ -127,15 +96,11 @@ pub(crate) fn except_with_non_exception_classes( let Some(type_) = type_ else { return; }; - // let flattened_exprs = flatten_starred_iterables(type_); - // println!("{:#?}", flattened_exprs); - // let flattened_exprs = flattened_exprs.iter().flat_map(|expr| flatten_bin_op(expr)); - for expr in flatten_starred_iterables(type_) { + for expr in flatten_starred_iterables_and_binops(type_) { if !matches!( expr, Expr::Subscript(_) | Expr::Attribute(_) | Expr::Name(_) | Expr::Call(_), ) { - println!("Adding diagnostic for expression: {:#?}", expr); checker .diagnostics .push(Diagnostic::new(ExceptWithNonExceptionClasses, expr.range())); From e4e795b0b5ce05e3b2ca5488272f82f30961b44f Mon Sep 17 00:00:00 2001 From: Ottavio Hartman Date: Sun, 17 Mar 2024 10:57:37 -0400 Subject: [PATCH 04/10] insta --- .../resources/test/fixtures/flake8_bugbear/B030.py | 13 +++++++++++++ .../rules/except_with_non_exception_classes.rs | 6 +++++- ..._rules__flake8_bugbear__tests__B030_B030.py.snap | 9 ++++++++- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py index 1f3fe6dc96536..f8758789385db 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py @@ -34,6 +34,12 @@ pass +try: + pass +except *(a + (RuntimeError, (KeyError, TypeError))): # error + pass + + try: pass except (ValueError, *(RuntimeError, TypeError)): # ok @@ -95,3 +101,10 @@ def what_to_catch(): pass except *(a, (b) + (c)): # ok pass + + +try: + pass +except (a, b) + (c, d) + (e, f): # ok + pass + diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs index 84e1346daa43c..a3bbe15af1280 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs @@ -67,15 +67,19 @@ fn flatten_starred_iterables_and_binops(expr: &Expr) -> Vec<&Expr> { _ => flattened_exprs.push(value), }, Expr::BinOp(ast::ExprBinOp { left, right, .. }) => { - // if left or right are tuples, we should flatten them for expr in [left, right] { match expr.as_ref() { + // If left or right are tuples or starred we should flatten them. Expr::Tuple(ast::ExprTuple { elts, .. }) => { exprs_to_process.append(&mut elts.iter().collect()); } Expr::Starred(ast::ExprStarred { value, .. }) => { exprs_to_process.push_back(value); } + // If their binops we should flatten them. + Expr::BinOp(ast::ExprBinOp { .. }) => { + exprs_to_process.push_back(expr); + } _ => flattened_exprs.push(expr), } } diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap index e93dfe9db3880..ab2cb8de3a4c9 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap @@ -46,4 +46,11 @@ B030.py:33:29: B030 `except` handlers should only be exception classes or tuples 34 | pass | - +B030.py:39:29: 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 + | From 95f0fdc7079d0e93f96a7ab5b349c96c39824ca0 Mon Sep 17 00:00:00 2001 From: Ottavio Hartman Date: Sun, 17 Mar 2024 10:58:31 -0400 Subject: [PATCH 05/10] doc --- .../flake8_bugbear/rules/except_with_non_exception_classes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs index a3bbe15af1280..b8b77cfe7e0da 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs @@ -76,7 +76,7 @@ fn flatten_starred_iterables_and_binops(expr: &Expr) -> Vec<&Expr> { Expr::Starred(ast::ExprStarred { value, .. }) => { exprs_to_process.push_back(value); } - // If their binops we should flatten them. + // If they are binops we should flatten them. Expr::BinOp(ast::ExprBinOp { .. }) => { exprs_to_process.push_back(expr); } From a2d1e760f84cbd6fd8f9e095c772c23d2af5c2fb Mon Sep 17 00:00:00 2001 From: Ottavio Hartman Date: Sun, 17 Mar 2024 11:00:25 -0400 Subject: [PATCH 06/10] add one more test --- .../resources/test/fixtures/flake8_bugbear/B030.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py index f8758789385db..960514de2a159 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py @@ -108,3 +108,8 @@ def what_to_catch(): except (a, b) + (c, d) + (e, f): # ok pass + +try: + pass +except a + (b, c): # ok + pass From 2d70e29a5e8e851b277659e4cdf7f241be30a4df Mon Sep 17 00:00:00 2001 From: Ottavio Hartman Date: Sun, 17 Mar 2024 15:09:29 -0400 Subject: [PATCH 07/10] fix recursion --- .../resources/test/fixtures/flake8_bugbear/B030.py | 6 ++++++ .../rules/except_with_non_exception_classes.rs | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py index 960514de2a159..cb3313c3cb8dc 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py @@ -113,3 +113,9 @@ def what_to_catch(): pass except a + (b, c): # ok pass + + +try: + pass +except (ValueError, *(RuntimeError, TypeError), *((ArithmeticError,) + (EOFError,))): + pass diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs index b8b77cfe7e0da..30c301c3853ee 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs @@ -64,6 +64,9 @@ fn flatten_starred_iterables_and_binops(expr: &Expr) -> Vec<&Expr> { | Expr::List(ast::ExprList { elts, .. }) => { exprs_to_process.append(&mut elts.iter().collect()); } + Expr::BinOp(ast::ExprBinOp { .. }) => { + exprs_to_process.push_back(value); + } _ => flattened_exprs.push(value), }, Expr::BinOp(ast::ExprBinOp { left, right, .. }) => { @@ -105,6 +108,7 @@ pub(crate) fn except_with_non_exception_classes( expr, Expr::Subscript(_) | Expr::Attribute(_) | Expr::Name(_) | Expr::Call(_), ) { + println!("expr: {:#?}", expr); checker .diagnostics .push(Diagnostic::new(ExceptWithNonExceptionClasses, expr.range())); From b72071798d91606899a528a0cc7933e35421ef50 Mon Sep 17 00:00:00 2001 From: Ottavio Hartman Date: Sun, 17 Mar 2024 15:17:36 -0400 Subject: [PATCH 08/10] add more tests --- .../resources/test/fixtures/flake8_bugbear/B030.py | 12 +++++++++--- .../rules/except_with_non_exception_classes.rs | 5 ++--- ...__rules__flake8_bugbear__tests__B030_B030.py.snap | 6 +++--- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py index cb3313c3cb8dc..ef63a9ed0733d 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py @@ -36,7 +36,7 @@ try: pass -except *(a + (RuntimeError, (KeyError, TypeError))): # error +except* a + (RuntimeError, (KeyError, TypeError)): # error pass @@ -93,13 +93,13 @@ def what_to_catch(): try: pass -except *(a, b) + (c, d): # ok +except* (a, b) + (c, d): # ok pass try: pass -except *(a, (b) + (c)): # ok +except* (a, (b) + (c)): # ok pass @@ -119,3 +119,9 @@ def what_to_catch(): pass except (ValueError, *(RuntimeError, TypeError), *((ArithmeticError,) + (EOFError,))): pass + + +try: + pass +except ((a, b) + (c, d)) + ((e, f) + (g)): # ok + pass diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs index 30c301c3853ee..d50a6fa8af450 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs @@ -49,11 +49,10 @@ impl Violation for ExceptWithNonExceptionClasses { /// This should leave any unstarred iterables alone (subsequently raising a /// warning for B029). fn flatten_starred_iterables_and_binops(expr: &Expr) -> Vec<&Expr> { - // The top-level expression should be a tuple or a binop, else return as-is. + // 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(), - Expr::BinOp(ast::ExprBinOp { .. }) => vec![expr].into(), - _ => return vec![expr], + _ => vec![expr].into(), }; let mut flattened_exprs: Vec<&Expr> = Vec::new(); diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap index ab2cb8de3a4c9..64723b3de1e90 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap @@ -46,11 +46,11 @@ B030.py:33:29: B030 `except` handlers should only be exception classes or tuples 34 | pass | -B030.py:39:29: B030 `except` handlers should only be exception classes or tuples of exception classes +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 +39 | except* a + (RuntimeError, (KeyError, TypeError)): # error + | ^^^^^^^^^^^^^^^^^^^^^ B030 40 | pass | From b3aa93a6776c0a49fc94ca03f01c26f19c044c51 Mon Sep 17 00:00:00 2001 From: Ottavio Hartman Date: Sun, 17 Mar 2024 15:18:23 -0400 Subject: [PATCH 09/10] remove print --- .../flake8_bugbear/rules/except_with_non_exception_classes.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs index d50a6fa8af450..3d5e7e1e6768d 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs @@ -107,7 +107,6 @@ pub(crate) fn except_with_non_exception_classes( expr, Expr::Subscript(_) | Expr::Attribute(_) | Expr::Name(_) | Expr::Call(_), ) { - println!("expr: {:#?}", expr); checker .diagnostics .push(Diagnostic::new(ExceptWithNonExceptionClasses, expr.range())); From bd64fc0726730d731ddba57903b9e0f2d4d507f0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 17 Mar 2024 20:24:12 -0400 Subject: [PATCH 10/10] Add a * test case --- .../test/fixtures/flake8_bugbear/B030.py | 43 +++++++----- .../except_with_non_exception_classes.rs | 70 +++++++++++-------- ...__flake8_bugbear__tests__B030_B030.py.snap | 21 ++++-- 3 files changed, 79 insertions(+), 55 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py index ef63a9ed0733d..098f724249ac2 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B030.py @@ -9,69 +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 +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 @@ -81,37 +81,37 @@ 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 +except (a, b) + (c, d): # OK pass try: pass -except* (a, b) + (c, d): # ok +except* (a, b) + (c, d): # OK pass try: pass -except* (a, (b) + (c)): # ok +except* (a, (b) + (c)): # OK pass try: pass -except (a, b) + (c, d) + (e, f): # ok +except (a, b) + (c, d) + (e, f): # OK pass try: pass -except a + (b, c): # ok +except a + (b, c): # OK pass @@ -123,5 +123,10 @@ def what_to_catch(): try: pass -except ((a, b) + (c, d)) + ((e, f) + (g)): # ok +except ((a, b) + (c, d)) + ((e, f) + (g)): # OK + pass + +try: + pass +except (a, b) * (c, d): # B030 pass diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs index 3d5e7e1e6768d..43e0348e5dcc8 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/except_with_non_exception_classes.rs @@ -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}; @@ -44,18 +44,41 @@ impl Violation for ExceptWithNonExceptionClasses { } } +/// B030 +pub(crate) fn except_with_non_exception_classes( + checker: &mut Checker, + except_handler: &ExceptHandler, +) { + let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { type_, .. }) = + except_handler; + let Some(type_) = type_ else { + return; + }; + for expr in flatten_iterables(type_) { + if !matches!( + expr, + Expr::Subscript(_) | Expr::Attribute(_) | Expr::Name(_) | Expr::Call(_), + ) { + checker + .diagnostics + .push(Diagnostic::new(ExceptWithNonExceptionClasses, expr.range())); + } + } +} + /// 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_starred_iterables_and_binops(expr: &Expr) -> Vec<&Expr> { +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()); - let mut flattened_exprs: Vec<&Expr> = Vec::new(); while let Some(expr) = exprs_to_process.pop_front() { match expr { Expr::Starred(ast::ExprStarred { value, .. }) => match value.as_ref() { @@ -63,23 +86,31 @@ fn flatten_starred_iterables_and_binops(expr: &Expr) -> Vec<&Expr> { | Expr::List(ast::ExprList { elts, .. }) => { exprs_to_process.append(&mut elts.iter().collect()); } - Expr::BinOp(ast::ExprBinOp { .. }) => { + Expr::BinOp(ast::ExprBinOp { + op: Operator::Add, .. + }) => { exprs_to_process.push_back(value); } _ => flattened_exprs.push(value), }, - Expr::BinOp(ast::ExprBinOp { left, right, .. }) => { + 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() { - // If left or right are tuples or starred we should flatten them. Expr::Tuple(ast::ExprTuple { elts, .. }) => { exprs_to_process.append(&mut elts.iter().collect()); } Expr::Starred(ast::ExprStarred { value, .. }) => { exprs_to_process.push_back(value); } - // If they are binops we should flatten them. - Expr::BinOp(ast::ExprBinOp { .. }) => { + Expr::BinOp(ast::ExprBinOp { + op: Operator::Add, .. + }) => { exprs_to_process.push_back(expr); } _ => flattened_exprs.push(expr), @@ -89,27 +120,6 @@ fn flatten_starred_iterables_and_binops(expr: &Expr) -> Vec<&Expr> { _ => flattened_exprs.push(expr), } } - flattened_exprs -} -/// B030 -pub(crate) fn except_with_non_exception_classes( - checker: &mut Checker, - except_handler: &ExceptHandler, -) { - let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { type_, .. }) = - except_handler; - let Some(type_) = type_ else { - return; - }; - for expr in flatten_starred_iterables_and_binops(type_) { - if !matches!( - expr, - Expr::Subscript(_) | Expr::Attribute(_) | Expr::Name(_) | Expr::Call(_), - ) { - checker - .diagnostics - .push(Diagnostic::new(ExceptWithNonExceptionClasses, expr.range())); - } - } + flattened_exprs } diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap index 64723b3de1e90..6606e05ad0427 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B030_B030.py.snap @@ -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 | @@ -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 | @@ -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 | @@ -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 | @@ -41,7 +41,7 @@ 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 | @@ -50,7 +50,16 @@ B030.py:39:28: B030 `except` handlers should only be exception classes or tuples | 37 | try: 38 | pass -39 | except* a + (RuntimeError, (KeyError, TypeError)): # error +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 + |