From c384fa513b2215b37f72edab00d117360df88cd3 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 10 Oct 2022 12:18:31 -0400 Subject: [PATCH] Implement B025 from flake8-bugbear (#391) --- README.md | 5 +- resources/test/fixtures/B011.py | 6 ++ resources/test/fixtures/B025.py | 38 +++++++++++ src/ast/checks.rs | 60 +++++++++++++--- src/ast/helpers.rs | 68 +++++++++++++------ src/check_ast.rs | 10 ++- src/checks.rs | 10 ++- src/linter.rs | 12 ++++ src/plugins/duplicate_exceptions.rs | 61 +++++++++++++++++ src/plugins/super_call_with_parameters.rs | 4 +- .../ruff__linter__tests__b011.snap.new | 38 +++++++++++ .../ruff__linter__tests__b025.snap.new | 42 ++++++++++++ 12 files changed, 317 insertions(+), 37 deletions(-) create mode 100644 resources/test/fixtures/B025.py create mode 100644 src/plugins/duplicate_exceptions.rs create mode 100644 src/snapshots/ruff__linter__tests__b011.snap.new create mode 100644 src/snapshots/ruff__linter__tests__b025.snap.new diff --git a/README.md b/README.md index 4cc1cec2d1..dff6c54e7e 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/) (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 | | | diff --git a/resources/test/fixtures/B011.py b/resources/test/fixtures/B011.py index 5dd14bd2c4..3fa20eba6c 100644 --- a/resources/test/fixtures/B011.py +++ b/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" diff --git a/resources/test/fixtures/B025.py b/resources/test/fixtures/B025.py new file mode 100644 index 0000000000..085f82f343 --- /dev/null +++ b/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 diff --git a/src/ast/checks.rs b/src/ast/checks.rs index 628b5b0401..7c2857381f 100644 --- a/src/ast/checks.rs +++ b/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,14 +748,55 @@ pub fn check_builtin_shadowing( } } -/// Returns `true` if a call is an argumented `super` invocation. -pub fn is_super_call_with_arguments(func: &Expr, args: &Vec) -> bool { - // Check: is this a `super` call? - if let ExprKind::Name { id, .. } = &func.node { - id == "super" && !args.is_empty() - } else { - false +// 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 @@ -1076,7 +1118,7 @@ pub fn check_super_args( func: &Expr, args: &Vec, ) -> Option { - if !is_super_call_with_arguments(func, args) { + if !helpers::is_super_call_with_arguments(func, args) { return None; } diff --git a/src/ast/helpers.rs b/src/ast/helpers.rs index 1389f8101e..1ea7761722 100644 --- a/src/ast/helpers.rs +++ b/src/ast/helpers.rs @@ -4,7 +4,39 @@ use rustpython_ast::{Excepthandler, ExcepthandlerKind, Expr, ExprKind, StmtKind} use crate::python::typing; -static DUNDER_REGEX: Lazy = 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 { + 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 { } } -fn node_name(expr: &Expr) -> Option<&str> { - 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, - } -} +static DUNDER_REGEX: Lazy = Lazy::new(|| Regex::new(r"__[^\s]+__").unwrap()); 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 -/// `module.CustomException`. (But will catch all builtin exceptions.) -pub fn extract_handler_names(handlers: &[Excepthandler]) -> Vec<&str> { +pub fn extract_handler_names(handlers: &[Excepthandler]) -> Vec { 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) -> bool { + // Check: is this a `super` call? + if let ExprKind::Name { id, .. } = &func.node { + id == "super" && !args.is_empty() + } else { + false + } +} diff --git a/src/check_ast.rs b/src/check_ast.rs index 6528315020..8d466769c0 100644 --- a/src/check_ast.rs +++ b/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>, + except_handlers: Vec>, } 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; } } diff --git a/src/checks.rs b/src/checks.rs index 35cd1aa042..b9dc6401a8 100644 --- a/src/checks.rs +++ b/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 => { diff --git a/src/linter.rs b/src/linter.rs index d679e17688..86dc693d13 100644 --- a/src/linter.rs +++ b/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( diff --git a/src/plugins/duplicate_exceptions.rs b/src/plugins/duplicate_exceptions.rs new file mode 100644 index 0000000000..c4393f36d9 --- /dev/null +++ b/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>) -> 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>) { + 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); + } +} diff --git a/src/plugins/super_call_with_parameters.rs b/src/plugins/super_call_with_parameters.rs index 2477641c08..f352668f36 100644 --- a/src/plugins/super_call_with_parameters.rs +++ b/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 diff --git a/src/snapshots/ruff__linter__tests__b011.snap.new b/src/snapshots/ruff__linter__tests__b011.snap.new new file mode 100644 index 0000000000..198ce831a2 --- /dev/null +++ b/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 + diff --git a/src/snapshots/ruff__linter__tests__b025.snap.new b/src/snapshots/ruff__linter__tests__b025.snap.new new file mode 100644 index 0000000000..dac686255d --- /dev/null +++ b/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: ~ +