Implement B014 from flake8-bugbear (#392)

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/) (2/32)
+- [`flake8-bugbear`](https://pypi.org/project/flake8-bugbear/) (3/32)
 - [`pyupgrade`](https://pypi.org/project/pyupgrade/) (partial)
 
 Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis Flake8:
@@ -280,7 +280,8 @@ The 🛠 emoji indicates that a rule is automatically fixable by the `--fix` com
 | 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), raise `AssertionError()` |  | 🛠 |
-| B025 | DuplicateExceptions | try-except block with duplicate exception `Exception` |  |  |
+| B014 | DuplicateHandlerException | Exception handler with duplicate exception `Exception` |  |  |
+| B025 | DuplicateTryBlockException | 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/B014.py

@@ -0,0 +1,76 @@
+"""
+Should emit:
+B014 - on lines 11, 17, 28, 42, 49, 56, and 74.
+"""
+
+import binascii
+import re
+
+try:
+    pass
+except (Exception, TypeError):
+    # TypeError is a subclass of Exception, so it doesn't add anything
+    pass
+
+try:
+    pass
+except (OSError, OSError) as err:
+    # Duplicate exception types are useless
+    pass
+
+
+class MyError(Exception):
+    pass
+
+
+try:
+    pass
+except (MyError, MyError):
+    # Detect duplicate non-builtin errors
+    pass
+
+
+try:
+    pass
+except (MyError, Exception) as e:
+    # Don't assume that we're all subclasses of Exception
+    pass
+
+
+try:
+    pass
+except (MyError, BaseException) as e:
+    # But we *can* assume that everything is a subclass of BaseException
+    pass
+
+
+try:
+    pass
+except (re.error, re.error):
+    # Duplicate exception types as attributes
+    pass
+
+
+try:
+    pass
+except (IOError, EnvironmentError, OSError):
+    # Detect if a primary exception and any its aliases are present.
+    #
+    # Since Python 3.3, IOError, EnvironmentError, WindowsError, mmap.error,
+    # socket.error and select.error are aliases of OSError. See PEP 3151 for
+    # more info.
+    pass
+
+
+try:
+    pass
+except (MyException, NotImplemented):
+    # NotImplemented is not an exception, let's not crash on it.
+    pass
+
+
+try:
+    pass
+except (ValueError, binascii.Error):
+    # binascii.Error is a subclass of ValueError.
+    pass

src/ast/checks.rs

@@ -1,7 +1,6 @@
 use std::collections::BTreeSet;
 
-use crate::ast::helpers;
-use itertools::{izip, Itertools};
+use itertools::izip;
 use num_bigint::BigInt;
 use regex::Regex;
 use rustpython_parser::ast::{
@@ -10,6 +9,7 @@ use rustpython_parser::ast::{
 };
 use serde::{Deserialize, Serialize};
 
+use crate::ast::helpers;
 use crate::ast::types::{
     Binding, BindingKind, CheckLocator, FunctionScope, Range, Scope, ScopeKind,
 };
@@ -748,57 +748,6 @@ pub fn check_builtin_shadowing(
     }
 }
 
-// flake8-bugbear
-/// Check DuplicateExceptions compliance.
-pub fn duplicate_exceptions(handlers: &[Excepthandler], location: Range) -> Vec<Check> {
-    let mut checks: Vec<Check> = vec![];
-
-    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 {
-                                    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.
 pub fn unnecessary_generator_list(expr: &Expr, func: &Expr, args: &Vec<Expr>) -> Option<Check> {

src/check_ast.rs

@@ -562,11 +562,10 @@ 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)),
-                    ));
+                if self.settings.enabled.contains(&CheckCode::B014)
+                    || self.settings.enabled.contains(&CheckCode::B025)
+                {
+                    plugins::duplicate_exceptions(self, stmt, handlers);
                 }
             }
             StmtKind::Assign { targets, value, .. } => {

src/checks.rs

@@ -123,6 +123,7 @@ pub enum CheckCode {
     A003,
     // flake8-bugbear
     B011,
+    B014,
     B025,
     // flake8-comprehensions
     C400,
@@ -218,7 +219,8 @@ pub enum CheckKind {
     BuiltinAttributeShadowing(String),
     // flake8-bugbear
     DoNotAssertFalse,
-    DuplicateExceptions(String),
+    DuplicateHandlerException(String),
+    DuplicateTryBlockException(String),
     // flake8-comprehensions
     UnnecessaryGeneratorList,
     UnnecessaryGeneratorSet,
@@ -314,7 +316,8 @@ impl CheckCode {
             CheckCode::A003 => CheckKind::BuiltinAttributeShadowing("...".to_string()),
             // flake8-bugbear
             CheckCode::B011 => CheckKind::DoNotAssertFalse,
-            CheckCode::B025 => CheckKind::DuplicateExceptions("Exception".to_string()),
+            CheckCode::B014 => CheckKind::DuplicateHandlerException("Exception".to_string()),
+            CheckCode::B025 => CheckKind::DuplicateTryBlockException("Exception".to_string()),
             // flake8-comprehensions
             CheckCode::C400 => CheckKind::UnnecessaryGeneratorList,
             CheckCode::C401 => CheckKind::UnnecessaryGeneratorSet,
@@ -411,7 +414,8 @@ impl CheckKind {
             CheckKind::BuiltinAttributeShadowing(_) => &CheckCode::A003,
             // flake8-bugbear
             CheckKind::DoNotAssertFalse => &CheckCode::B011,
-            CheckKind::DuplicateExceptions(_) => &CheckCode::B025,
+            CheckKind::DuplicateHandlerException(_) => &CheckCode::B014,
+            CheckKind::DuplicateTryBlockException(_) => &CheckCode::B025,
             // flake8-comprehensions
             CheckKind::UnnecessaryGeneratorList => &CheckCode::C400,
             CheckKind::UnnecessaryGeneratorSet => &CheckCode::C401,
@@ -589,7 +593,10 @@ impl CheckKind {
                 "Do not `assert False` (`python -O` removes these calls), raise `AssertionError()`"
                     .to_string()
             }
-            CheckKind::DuplicateExceptions(name) => {
+            CheckKind::DuplicateHandlerException(name) => {
+                format!("Exception handler with duplicate exception `{name}`")
+            }
+            CheckKind::DuplicateTryBlockException(name) => {
                 format!("try-except block with duplicate exception `{name}`")
             }
             // flake8-comprehensions

src/linter.rs

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

src/plugins.rs

@@ -1,6 +1,7 @@
 pub use assert_false::assert_false;
 pub use assert_tuple::assert_tuple;
 pub use deprecated_unittest_alias::deprecated_unittest_alias;
+pub use duplicate_exceptions::duplicate_exceptions;
 pub use if_tuple::if_tuple;
 pub use invalid_print_syntax::invalid_print_syntax;
 pub use print_call::print_call;
@@ -15,6 +16,7 @@ pub use useless_object_inheritance::useless_object_inheritance;
 mod assert_false;
 mod assert_tuple;
 mod deprecated_unittest_alias;
+mod duplicate_exceptions;
 mod if_tuple;
 mod invalid_print_syntax;
 mod print_call;

src/plugins/duplicate_exceptions.rs

@@ -1,61 +1,82 @@
-use rustpython_ast::{Constant, Expr, ExprContext, ExprKind, Stmt, StmtKind};
+use itertools::Itertools;
+use std::collections::BTreeSet;
 
-use crate::ast::types::Range;
-use crate::autofix::fixer;
+use rustpython_ast::{Excepthandler, ExcepthandlerKind, Expr, ExprKind, Stmt};
+
+use crate::ast::helpers;
+use crate::ast::types::{CheckLocator, Range};
 use crate::check_ast::Checker;
-use crate::checks::{Check, CheckKind, Fix};
-use crate::code_gen::SourceGenerator;
+use crate::checks::{Check, CheckCode, CheckKind};
 
-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 duplicate_handler_exceptions(
+    checker: &mut Checker,
+    stmt: &Stmt,
+    elts: &Vec<Expr>,
+) -> BTreeSet<String> {
+    let mut seen: BTreeSet<String> = Default::default();
+    let mut duplicates: BTreeSet<String> = Default::default();
+    for type_ in elts {
+        if let Some(name) = helpers::compose_call_path(type_) {
+            if seen.contains(&name) {
+                duplicates.insert(name);
+            } else {
+                seen.insert(name);
+            }
+        }
+    }
+
+    if checker.settings.enabled.contains(&CheckCode::B014) {
+        // TODO(charlie): Handle "BaseException" and redundant exception aliases.
+        for duplicate in duplicates.into_iter().sorted() {
+            checker.add_check(Check::new(
+                CheckKind::DuplicateHandlerException(duplicate),
+                checker.locate_check(Range::from_located(stmt)),
+            ));
+        }
+    }
+
+    seen
 }
 
-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,
-                    })
+pub fn duplicate_exceptions(checker: &mut Checker, stmt: &Stmt, handlers: &[Excepthandler]) {
+    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 {
+                                    seen.insert(name);
+                                }
+                            }
+                        }
+                        ExprKind::Tuple { elts, .. } => {
+                            for name in duplicate_handler_exceptions(checker, stmt, elts) {
+                                if seen.contains(&name) {
+                                    duplicates.insert(name);
+                                } else {
+                                    seen.insert(name);
+                                }
+                            }
+                        }
+                        _ => {}
+                    }
                 }
             }
         }
-        checker.add_check(check);
+    }
+
+    if checker.settings.enabled.contains(&CheckCode::B025) {
+        for duplicate in duplicates.into_iter().sorted() {
+            checker.add_check(Check::new(
+                CheckKind::DuplicateTryBlockException(duplicate),
+                checker.locate_check(Range::from_located(stmt)),
+            ));
+        }
     }
 }

src/snapshots/ruff__linter__tests__b011.snap

@@ -4,34 +4,34 @@ expression: checks
 ---
 - kind: DoNotAssertFalse
   location:
-    row: 2
+    row: 8
     column: 8
   end_location:
-    row: 2
+    row: 8
     column: 13
   fix:
     content: raise AssertionError()
     location:
-      row: 2
+      row: 8
       column: 1
     end_location:
-      row: 2
+      row: 8
       column: 13
     applied: false
 - kind: DoNotAssertFalse
   location:
-    row: 4
+    row: 10
     column: 8
   end_location:
-    row: 4
+    row: 10
     column: 13
   fix:
     content: "raise AssertionError(\"message\")"
     location:
-      row: 4
+      row: 10
       column: 1
     end_location:
-      row: 4
+      row: 10
       column: 24
     applied: false
 

src/snapshots/ruff__linter__tests__b011.snap.new

@@ -1,38 +0,0 @@
----
-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__b014.snap

@@ -0,0 +1,32 @@
+---
+source: src/linter.rs
+expression: checks
+---
+- kind:
+    DuplicateHandlerException: OSError
+  location:
+    row: 15
+    column: 1
+  end_location:
+    row: 22
+    column: 1
+  fix: ~
+- kind:
+    DuplicateHandlerException: MyError
+  location:
+    row: 26
+    column: 1
+  end_location:
+    row: 33
+    column: 1
+  fix: ~
+- kind:
+    DuplicateHandlerException: re.error
+  location:
+    row: 47
+    column: 1
+  end_location:
+    row: 54
+    column: 1
+  fix: ~
+

src/snapshots/ruff__linter__tests__b025.snap

@@ -1,10 +1,9 @@
 ---
 source: src/linter.rs
-assertion_line: 833
 expression: checks
 ---
 - kind:
-    DuplicateExceptions: ValueError
+    DuplicateTryBlockException: ValueError
   location:
     row: 15
     column: 1
@@ -13,7 +12,7 @@ expression: checks
     column: 1
   fix: ~
 - kind:
-    DuplicateExceptions: pickle.PickleError
+    DuplicateTryBlockException: pickle.PickleError
   location:
     row: 22
     column: 1
@@ -22,7 +21,7 @@ expression: checks
     column: 1
   fix: ~
 - kind:
-    DuplicateExceptions: TypeError
+    DuplicateTryBlockException: TypeError
   location:
     row: 31
     column: 1
@@ -31,7 +30,7 @@ expression: checks
     column: 1
   fix: ~
 - kind:
-    DuplicateExceptions: ValueError
+    DuplicateTryBlockException: ValueError
   location:
     row: 31
     column: 1