diff --git a/README.md b/README.md index dff6c54e7e..bc9f40c6e6 100644 --- a/README.md +++ b/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 | | | diff --git a/resources/test/fixtures/B014.py b/resources/test/fixtures/B014.py new file mode 100644 index 0000000000..38473457b3 --- /dev/null +++ b/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 diff --git a/src/ast/checks.rs b/src/ast/checks.rs index 7c2857381f..6bfc2997c5 100644 --- a/src/ast/checks.rs +++ b/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 { - let mut checks: Vec = vec![]; - - let mut seen: BTreeSet = Default::default(); - let mut duplicates: BTreeSet = 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) -> Option { diff --git a/src/check_ast.rs b/src/check_ast.rs index 8d466769c0..a5f1f2a8bd 100644 --- a/src/check_ast.rs +++ b/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, .. } => { diff --git a/src/checks.rs b/src/checks.rs index b9dc6401a8..f4f51313bb 100644 --- a/src/checks.rs +++ b/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 diff --git a/src/linter.rs b/src/linter.rs index 86dc693d13..9cb9989bc1 100644 --- a/src/linter.rs +++ b/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( diff --git a/src/plugins.rs b/src/plugins.rs index da6aaaee90..9fcbff5121 100644 --- a/src/plugins.rs +++ b/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; diff --git a/src/plugins/duplicate_exceptions.rs b/src/plugins/duplicate_exceptions.rs index c4393f36d9..c4f90815a9 100644 --- a/src/plugins/duplicate_exceptions.rs +++ b/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>) -> 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, +) -> BTreeSet { + let mut seen: BTreeSet = Default::default(); + let mut duplicates: BTreeSet = 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>) { - 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 = Default::default(); + let mut duplicates: BTreeSet = 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)), + )); + } } } diff --git a/src/snapshots/ruff__linter__tests__b011.snap b/src/snapshots/ruff__linter__tests__b011.snap index bce0cf16c9..c2868e1432 100644 --- a/src/snapshots/ruff__linter__tests__b011.snap +++ b/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 diff --git a/src/snapshots/ruff__linter__tests__b011.snap.new b/src/snapshots/ruff__linter__tests__b011.snap.new deleted file mode 100644 index 198ce831a2..0000000000 --- a/src/snapshots/ruff__linter__tests__b011.snap.new +++ /dev/null @@ -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 - diff --git a/src/snapshots/ruff__linter__tests__b014.snap b/src/snapshots/ruff__linter__tests__b014.snap new file mode 100644 index 0000000000..88fe034dac --- /dev/null +++ b/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: ~ + diff --git a/src/snapshots/ruff__linter__tests__b025.snap.new b/src/snapshots/ruff__linter__tests__b025.snap similarity index 70% rename from src/snapshots/ruff__linter__tests__b025.snap.new rename to src/snapshots/ruff__linter__tests__b025.snap index dac686255d..033b5dbf69 100644 --- a/src/snapshots/ruff__linter__tests__b025.snap.new +++ b/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