Implement B025 from flake8-bugbear (#391)

README.md

@@ -216,7 +216,7 @@ ruff also implements some of the most popular Flake8 plugins natively, including
 - [`flake8-super`](https://pypi.org/project/flake8-super/)
 - [`flake8-print`](https://pypi.org/project/flake8-print/)
 - [`flake8-comprehensions`](https://pypi.org/project/flake8-comprehensions/) (11/16)
-- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (1/32)
+- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (2/32)
 - [`pyupgrade`](https://pypi.org/project/pyupgrade/) (partial)
 
 Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis Flake8:
@@ -279,7 +279,8 @@ The 🛠 emoji indicates that a rule is automatically fixable by the `--fix` com
 | A001 | BuiltinVariableShadowing | Variable `...` is shadowing a python builtin |  |  |
 | A002 | BuiltinArgumentShadowing | Argument `...` is shadowing a python builtin |  |  |
 | A003 | BuiltinAttributeShadowing | Class attribute `...` is shadowing a python builtin |  |  |
-| B011 | DoNotAssertFalse | Do not `assert False` (`python -O` removes these calls). Instead, raise `AssertionError()`. |  | 🛠 |
+| B011 | DoNotAssertFalse | Do not `assert False` (`python -O` removes these calls), raise `AssertionError()` |  | 🛠 |
+| B025 | DuplicateExceptions | try-except block with duplicate exception `Exception` |  |  |
 | C400 | UnnecessaryGeneratorList | Unnecessary generator - rewrite as a list comprehension |  |  |
 | C401 | UnnecessaryGeneratorSet | Unnecessary generator - rewrite as a set comprehension |  |  |
 | C402 | UnnecessaryGeneratorDict | Unnecessary generator - rewrite as a dict comprehension |  |  |

resources/test/fixtures/B011.py

@@ -1,3 +1,9 @@
+"""
+Should emit:
+B011 - on line 8
+B011 - on line 10
+"""
+
 assert 1 != 2
 assert False
 assert 1 != 2, "message"

resources/test/fixtures/B025.py

@@ -0,0 +1,38 @@
+"""
+Should emit:
+B025 - on lines 15, 22, 31
+"""
+
+import pickle
+
+try:
+    a = 1
+except ValueError:
+    a = 2
+finally:
+    a = 3
+
+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

src/ast/checks.rs

@@ -1,6 +1,7 @@
 use std::collections::BTreeSet;
 
-use itertools::izip;
+use crate::ast::helpers;
+use itertools::{izip, Itertools};
 use num_bigint::BigInt;
 use regex::Regex;
 use rustpython_parser::ast::{
@@ -747,15 +748,56 @@ pub fn check_builtin_shadowing(
     }
 }
 
-/// Returns `true` if a call is an argumented `super` invocation.
+// flake8-bugbear
-pub fn is_super_call_with_arguments(func: &Expr, args: &Vec<Expr>) -> bool {
+/// Check DuplicateExceptions compliance.
-    // Check: is this a `super` call?
+pub fn duplicate_exceptions(handlers: &[Excepthandler], location: Range) -> Vec<Check> {
-    if let ExprKind::Name { id, .. } = &func.node {
+    let mut checks: Vec<Check> = vec![];
-        id == "super" && !args.is_empty()
+
+    let mut seen: BTreeSet<String> = Default::default();
+    let mut duplicates: BTreeSet<String> = Default::default();
+    for handler in handlers {
+        match &handler.node {
+            ExcepthandlerKind::ExceptHandler { type_, .. } => {
+                if let Some(type_) = type_ {
+                    match &type_.node {
+                        ExprKind::Attribute { .. } | ExprKind::Name { .. } => {
+                            if let Some(name) = helpers::compose_call_path(type_) {
+                                if seen.contains(&name) {
+                                    duplicates.insert(name);
                                 } else {
-        false
+                                    seen.insert(name);
                                 }
                             }
+                        }
+                        ExprKind::Tuple { elts, .. } => {
+                            // TODO(charlie): If we have duplicates within a single handler, that
+                            // should be handled by B014 (as yet unimplemented).
+                            for type_ in elts {
+                                if let Some(name) = helpers::compose_call_path(type_) {
+                                    if seen.contains(&name) {
+                                        duplicates.insert(name);
+                                    } else {
+                                        seen.insert(name);
+                                    }
+                                }
+                            }
+                        }
+                        _ => {}
+                    }
+                }
+            }
+        }
+    }
+
+    for duplicate in duplicates.into_iter().sorted() {
+        checks.push(Check::new(
+            CheckKind::DuplicateExceptions(duplicate),
+            location,
+        ));
+    }
+
+    checks
+}
 
 // flake8-comprehensions
 /// Check `list(generator)` compliance.
@@ -1076,7 +1118,7 @@ pub fn check_super_args(
     func: &Expr,
     args: &Vec<Expr>,
 ) -> Option<Check> {
-    if !is_super_call_with_arguments(func, args) {
+    if !helpers::is_super_call_with_arguments(func, args) {
         return None;
     }
 

src/ast/helpers.rs

@@ -4,7 +4,39 @@ use rustpython_ast::{Excepthandler, ExcepthandlerKind, Expr, ExprKind, StmtKind}
 
 use crate::python::typing;
 
-static DUNDER_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"__[^\s]+__").unwrap());
+fn compose_call_path_inner<'a>(expr: &'a Expr, parts: &mut Vec<&'a str>) {
+    match &expr.node {
+        ExprKind::Call { func, .. } => {
+            compose_call_path_inner(func, parts);
+        }
+        ExprKind::Attribute { value, attr, .. } => {
+            compose_call_path_inner(value, parts);
+            parts.push(attr);
+        }
+        ExprKind::Name { id, .. } => {
+            parts.push(id);
+        }
+        _ => {}
+    }
+}
+
+pub fn compose_call_path(expr: &Expr) -> Option<String> {
+    let mut segments = vec![];
+    compose_call_path_inner(expr, &mut segments);
+    if segments.is_empty() {
+        None
+    } else {
+        Some(segments.join("."))
+    }
+}
+
+pub fn match_name_or_attr(expr: &Expr, target: &str) -> bool {
+    match &expr.node {
+        ExprKind::Attribute { attr, .. } => target == attr,
+        ExprKind::Name { id, .. } => target == id,
+        _ => false,
+    }
+}
 
 pub enum SubscriptKind {
     AnnotatedSubscript,
@@ -35,21 +67,7 @@ pub fn match_annotated_subscript(expr: &Expr) -> Option<SubscriptKind> {
     }
 }
 
-fn node_name(expr: &Expr) -> Option<&str> {
+static DUNDER_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"__[^\s]+__").unwrap());
-    if let ExprKind::Name { id, .. } = &expr.node {
-        Some(id)
-    } else {
-        None
-    }
-}
-
-pub fn match_name_or_attr(expr: &Expr, target: &str) -> bool {
-    match &expr.node {
-        ExprKind::Attribute { attr, .. } => target == attr,
-        ExprKind::Name { id, .. } => target == id,
-        _ => false,
-    }
-}
 
 pub fn is_assignment_to_a_dunder(node: &StmtKind) -> bool {
     // Check whether it's an assignment to a dunder, with or without a type annotation.
@@ -82,9 +100,7 @@ pub fn is_assignment_to_a_dunder(node: &StmtKind) -> bool {
 }
 
 /// Extract the names of all handled exceptions.
-/// Note that, for now, this only matches on ExprKind::Name, and so won't catch exceptions like
+pub fn extract_handler_names(handlers: &[Excepthandler]) -> Vec<String> {
-/// `module.CustomException`. (But will catch all builtin exceptions.)
-pub fn extract_handler_names(handlers: &[Excepthandler]) -> Vec<&str> {
     let mut handler_names = vec![];
     for handler in handlers {
         match &handler.node {
@@ -92,11 +108,11 @@ pub fn extract_handler_names(handlers: &[Excepthandler]) -> Vec<&str> {
                 if let Some(type_) = type_ {
                     if let ExprKind::Tuple { elts, .. } = &type_.node {
                         for type_ in elts {
-                            if let Some(name) = node_name(type_) {
+                            if let Some(name) = compose_call_path(type_) {
                                 handler_names.push(name);
                             }
                         }
-                    } else if let Some(name) = node_name(type_) {
+                    } else if let Some(name) = compose_call_path(type_) {
                         handler_names.push(name);
                     }
                 }
@@ -105,3 +121,13 @@ pub fn extract_handler_names(handlers: &[Excepthandler]) -> Vec<&str> {
     }
     handler_names
 }
+
+/// Returns `true` if a call is an argumented `super` invocation.
+pub fn is_super_call_with_arguments(func: &Expr, args: &Vec<Expr>) -> bool {
+    // Check: is this a `super` call?
+    if let ExprKind::Name { id, .. } = &func.node {
+        id == "super" && !args.is_empty()
+    } else {
+        false
+    }
+}

src/check_ast.rs

@@ -60,7 +60,7 @@ pub struct Checker<'a> {
     seen_docstring: bool,
     futures_allowed: bool,
     annotations_future_enabled: bool,
-    except_handlers: Vec<Vec<&'a str>>,
+    except_handlers: Vec<Vec<String>>,
 }
 
 impl<'a> Checker<'a> {
@@ -562,6 +562,12 @@ where
                         self.checks.push(check);
                     }
                 }
+                if self.settings.enabled.contains(&CheckCode::B025) {
+                    self.checks.extend(checks::duplicate_exceptions(
+                        handlers,
+                        self.locate_check(Range::from_located(stmt)),
+                    ));
+                }
             }
             StmtKind::Assign { targets, value, .. } => {
                 if self.settings.enabled.contains(&CheckCode::E731) {
@@ -1515,7 +1521,7 @@ impl<'a> Checker<'a> {
 
                 // Avoid flagging if NameError is handled.
                 if let Some(handler_names) = self.except_handlers.last() {
-                    if handler_names.contains(&"NameError") {
+                    if handler_names.contains(&"NameError".to_string()) {
                         return;
                     }
                 }

src/checks.rs

@@ -123,6 +123,7 @@ pub enum CheckCode {
     A003,
     // flake8-bugbear
     B011,
+    B025,
     // flake8-comprehensions
     C400,
     C401,
@@ -217,6 +218,7 @@ pub enum CheckKind {
     BuiltinAttributeShadowing(String),
     // flake8-bugbear
     DoNotAssertFalse,
+    DuplicateExceptions(String),
     // flake8-comprehensions
     UnnecessaryGeneratorList,
     UnnecessaryGeneratorSet,
@@ -312,6 +314,7 @@ impl CheckCode {
             CheckCode::A003 => CheckKind::BuiltinAttributeShadowing("...".to_string()),
             // flake8-bugbear
             CheckCode::B011 => CheckKind::DoNotAssertFalse,
+            CheckCode::B025 => CheckKind::DuplicateExceptions("Exception".to_string()),
             // flake8-comprehensions
             CheckCode::C400 => CheckKind::UnnecessaryGeneratorList,
             CheckCode::C401 => CheckKind::UnnecessaryGeneratorSet,
@@ -408,6 +411,7 @@ impl CheckKind {
             CheckKind::BuiltinAttributeShadowing(_) => &CheckCode::A003,
             // flake8-bugbear
             CheckKind::DoNotAssertFalse => &CheckCode::B011,
+            CheckKind::DuplicateExceptions(_) => &CheckCode::B025,
             // flake8-comprehensions
             CheckKind::UnnecessaryGeneratorList => &CheckCode::C400,
             CheckKind::UnnecessaryGeneratorSet => &CheckCode::C401,
@@ -582,7 +586,11 @@ impl CheckKind {
             }
             // flake8-bugbear
             CheckKind::DoNotAssertFalse => {
-                "Do not `assert False` (`python -O` removes these calls). Instead, raise `AssertionError()`.".to_string()
+                "Do not `assert False` (`python -O` removes these calls), raise `AssertionError()`"
+                    .to_string()
+            }
+            CheckKind::DuplicateExceptions(name) => {
+                format!("try-except block with duplicate exception `{name}`")
             }
             // flake8-comprehensions
             CheckKind::UnnecessaryGeneratorList => {

src/linter.rs

@@ -822,6 +822,18 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn b025() -> Result<()> {
+        let mut checks = check_path(
+            Path::new("./resources/test/fixtures/B025.py"),
+            &settings::Settings::for_rule(CheckCode::B025),
+            &fixer::Mode::Generate,
+        )?;
+        checks.sort_by_key(|check| check.location);
+        insta::assert_yaml_snapshot!(checks);
+        Ok(())
+    }
+
     #[test]
     fn c400() -> Result<()> {
         let mut checks = check_path(

src/plugins/duplicate_exceptions.rs

@@ -0,0 +1,61 @@
+use rustpython_ast::{Constant, Expr, ExprContext, ExprKind, Stmt, StmtKind};
+
+use crate::ast::types::Range;
+use crate::autofix::fixer;
+use crate::check_ast::Checker;
+use crate::checks::{Check, CheckKind, Fix};
+use crate::code_gen::SourceGenerator;
+
+fn assertion_error(msg: &Option<Box<Expr>>) -> Stmt {
+    Stmt::new(
+        Default::default(),
+        Default::default(),
+        StmtKind::Raise {
+            exc: Some(Box::new(Expr::new(
+                Default::default(),
+                Default::default(),
+                ExprKind::Call {
+                    func: Box::new(Expr::new(
+                        Default::default(),
+                        Default::default(),
+                        ExprKind::Name {
+                            id: "AssertionError".to_string(),
+                            ctx: ExprContext::Load,
+                        },
+                    )),
+                    args: if let Some(msg) = msg {
+                        vec![*msg.clone()]
+                    } else {
+                        vec![]
+                    },
+                    keywords: vec![],
+                },
+            ))),
+            cause: None,
+        },
+    )
+}
+
+pub fn assert_false(checker: &mut Checker, stmt: &Stmt, test: &Expr, msg: &Option<Box<Expr>>) {
+    if let ExprKind::Constant {
+        value: Constant::Bool(false),
+        ..
+    } = &test.node
+    {
+        let mut check = Check::new(CheckKind::DoNotAssertFalse, Range::from_located(test));
+        if matches!(checker.autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
+            let mut generator = SourceGenerator::new();
+            if let Ok(()) = generator.unparse_stmt(&assertion_error(msg)) {
+                if let Ok(content) = generator.generate() {
+                    check.amend(Fix {
+                        content,
+                        location: stmt.location,
+                        end_location: stmt.end_location,
+                        applied: false,
+                    })
+                }
+            }
+        }
+        checker.add_check(check);
+    }
+}

src/plugins/super_call_with_parameters.rs

@@ -1,6 +1,6 @@
 use rustpython_ast::{Expr, Stmt};
 
-use crate::ast::checks;
+use crate::ast::{checks, helpers};
 use crate::autofix::{fixer, fixes};
 use crate::check_ast::Checker;
 
@@ -12,7 +12,7 @@ pub fn super_call_with_parameters(
 ) {
     // Only bother going through the super check at all if we're in a `super` call.
     // (We check this in `check_super_args` too, so this is just an optimization.)
-    if checks::is_super_call_with_arguments(func, args) {
+    if helpers::is_super_call_with_arguments(func, args) {
         let scope = checker.current_scope();
         let parents: Vec<&Stmt> = checker
             .parent_stack

src/snapshots/ruff__linter__tests__b011.snap.new

@@ -0,0 +1,38 @@
+---
+source: src/linter.rs
+assertion_line: 821
+expression: checks
+---
+- kind: DoNotAssertFalse
+  location:
+    row: 8
+    column: 8
+  end_location:
+    row: 8
+    column: 13
+  fix:
+    content: raise AssertionError()
+    location:
+      row: 8
+      column: 1
+    end_location:
+      row: 8
+      column: 13
+    applied: false
+- kind: DoNotAssertFalse
+  location:
+    row: 10
+    column: 8
+  end_location:
+    row: 10
+    column: 13
+  fix:
+    content: "raise AssertionError(\"message\")"
+    location:
+      row: 10
+      column: 1
+    end_location:
+      row: 10
+      column: 24
+    applied: false
+

src/snapshots/ruff__linter__tests__b025.snap.new

@@ -0,0 +1,42 @@
+---
+source: src/linter.rs
+assertion_line: 833
+expression: checks
+---
+- kind:
+    DuplicateExceptions: ValueError
+  location:
+    row: 15
+    column: 1
+  end_location:
+    row: 22
+    column: 1
+  fix: ~
+- kind:
+    DuplicateExceptions: pickle.PickleError
+  location:
+    row: 22
+    column: 1
+  end_location:
+    row: 31
+    column: 1
+  fix: ~
+- kind:
+    DuplicateExceptions: TypeError
+  location:
+    row: 31
+    column: 1
+  end_location:
+    row: 39
+    column: 1
+  fix: ~
+- kind:
+    DuplicateExceptions: ValueError
+  location:
+    row: 31
+    column: 1
+  end_location:
+    row: 39
+    column: 1
+  fix: ~
+