From 9579faffa8dbb31bb799d7016abb03c6c38a4f08 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 7 Dec 2022 10:37:08 -0500 Subject: [PATCH] Avoid flagging bare exception issues when exception is re-raised (#1124) --- README.md | 2 +- .../test/fixtures/flake8_blind_except/BLE.py | 8 ++ src/check_ast.rs | 26 ++++-- src/checks.rs | 8 +- src/flake8_blind_except/plugins.rs | 45 ++++++--- ...e8_blind_except__tests__BLE001_BLE.py.snap | 73 ++++++--------- src/pycodestyle/checks.rs | 93 +++++++++++-------- 7 files changed, 145 insertions(+), 110 deletions(-) diff --git a/README.md b/README.md index 51bc193367..bdcb73fedc 100644 --- a/README.md +++ b/README.md @@ -631,7 +631,7 @@ For more, see [flake8-blind-except](https://pypi.org/project/flake8-blind-except | Code | Name | Message | Fix | | ---- | ---- | ------- | --- | -| BLE001 | BlindExcept | Blind except Exception: statement | | +| BLE001 | BlindExcept | Do not catch blind exception: `Exception` | | ### flake8-boolean-trap (FBT) diff --git a/resources/test/fixtures/flake8_blind_except/BLE.py b/resources/test/fixtures/flake8_blind_except/BLE.py index a37a3511d1..9c3db8562b 100644 --- a/resources/test/fixtures/flake8_blind_except/BLE.py +++ b/resources/test/fixtures/flake8_blind_except/BLE.py @@ -53,3 +53,11 @@ try: raise e except Exception: pass + + +try: + pass +except Exception as e: + raise bad +except BaseException: + pass diff --git a/src/check_ast.rs b/src/check_ast.rs index 6c7d28b42e..a582a2f8f0 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -1077,9 +1077,6 @@ where if self.settings.enabled.contains(&CheckCode::B013) { flake8_bugbear::plugins::redundant_tuple_in_exception_handler(self, handlers); } - if self.settings.enabled.contains(&CheckCode::BLE001) { - flake8_blind_except::plugins::blind_except(self, handlers); - } } StmtKind::Assign { targets, value, .. } => { if self.settings.enabled.contains(&CheckCode::E731) { @@ -2347,16 +2344,25 @@ where ExcepthandlerKind::ExceptHandler { type_, name, body, .. } => { - if self.settings.enabled.contains(&CheckCode::E722) && type_.is_none() { - self.add_check(Check::new( - CheckKind::DoNotUseBareExcept, + if self.settings.enabled.contains(&CheckCode::E722) { + if let Some(check) = pycodestyle::checks::do_not_use_bare_except( + type_.as_deref(), + body, Range::from_located(excepthandler), - )); + ) { + self.add_check(check); + } } if self.settings.enabled.contains(&CheckCode::B904) { - { - flake8_bugbear::plugins::raise_without_from_inside_except(self, body); - } + flake8_bugbear::plugins::raise_without_from_inside_except(self, body); + } + if self.settings.enabled.contains(&CheckCode::BLE001) { + flake8_blind_except::plugins::blind_except( + self, + type_.as_deref(), + name.as_ref().map(String::as_str), + body, + ); } match name { Some(name) => { diff --git a/src/checks.rs b/src/checks.rs index ea75aff676..a5ab1282e7 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -631,7 +631,7 @@ pub enum CheckKind { BuiltinArgumentShadowing(String), BuiltinAttributeShadowing(String), // flake8-blind-except - BlindExcept, + BlindExcept(String), // flake8-bugbear AbstractBaseClassWithoutAbstractMethod(String), AssignmentToOsEnviron, @@ -1045,7 +1045,7 @@ impl CheckCode { CheckCode::YTT302 => CheckKind::SysVersionCmpStr10, CheckCode::YTT303 => CheckKind::SysVersionSlice1Referenced, // flake8-blind-except - CheckCode::BLE001 => CheckKind::BlindExcept, + CheckCode::BLE001 => CheckKind::BlindExcept("Exception".to_string()), // pyupgrade CheckCode::UP001 => CheckKind::UselessMetaclassType, CheckCode::UP003 => CheckKind::TypeOfPrimitive(Primitive::Str), @@ -1529,7 +1529,7 @@ impl CheckKind { CheckKind::UselessContextlibSuppress => &CheckCode::B022, CheckKind::UselessExpression => &CheckCode::B018, // flake8-blind-except - CheckKind::BlindExcept => &CheckCode::BLE001, + CheckKind::BlindExcept(_) => &CheckCode::BLE001, // flake8-comprehensions CheckKind::UnnecessaryGeneratorList => &CheckCode::C400, CheckKind::UnnecessaryGeneratorSet => &CheckCode::C401, @@ -2460,7 +2460,7 @@ impl CheckKind { format!("Possible hardcoded password: `\"{string}\"`") } // flake8-blind-except - CheckKind::BlindExcept => "Blind except Exception: statement".to_string(), + CheckKind::BlindExcept(name) => format!("Do not catch blind exception: `{name}`"), // mccabe CheckKind::FunctionIsTooComplex(name, complexity) => { format!("`{name}` is too complex ({complexity})") diff --git a/src/flake8_blind_except/plugins.rs b/src/flake8_blind_except/plugins.rs index 4a9638baf2..072f4ce399 100644 --- a/src/flake8_blind_except/plugins.rs +++ b/src/flake8_blind_except/plugins.rs @@ -1,21 +1,42 @@ -use rustpython_ast::{Excepthandler, ExcepthandlerKind, ExprKind}; +use rustpython_ast::{Expr, ExprKind, Stmt, StmtKind}; use crate::ast::types::Range; use crate::check_ast::Checker; use crate::checks::{Check, CheckKind}; -pub fn blind_except(checker: &mut Checker, handlers: &[Excepthandler]) { - for handler in handlers { - let ExcepthandlerKind::ExceptHandler { type_: Some(type_), .. } = &handler.node else { - continue; - }; - let ExprKind::Name { id, .. } = &type_.node else { - continue; - }; - for exception in ["BaseException", "Exception"] { - if id == exception { +/// BLE001 +pub fn blind_except( + checker: &mut Checker, + type_: Option<&Expr>, + name: Option<&str>, + body: &[Stmt], +) { + let Some(type_) = type_ else { + return; + }; + let ExprKind::Name { id, .. } = &type_.node else { + return; + }; + for exception in ["BaseException", "Exception"] { + if id == exception && checker.is_builtin(exception) { + // If the exception is re-raised, don't flag an error. + if !body.iter().any(|stmt| { + if let StmtKind::Raise { exc, .. } = &stmt.node { + if let Some(exc) = exc { + if let ExprKind::Name { id, .. } = &exc.node { + name.map_or(false, |name| name == id) + } else { + false + } + } else { + true + } + } else { + false + } + }) { checker.add_check(Check::new( - CheckKind::BlindExcept, + CheckKind::BlindExcept(id.to_string()), Range::from_located(type_), )); } diff --git a/src/flake8_blind_except/snapshots/ruff__flake8_blind_except__tests__BLE001_BLE.py.snap b/src/flake8_blind_except/snapshots/ruff__flake8_blind_except__tests__BLE001_BLE.py.snap index 2b9c70cfaa..d765b84188 100644 --- a/src/flake8_blind_except/snapshots/ruff__flake8_blind_except__tests__BLE001_BLE.py.snap +++ b/src/flake8_blind_except/snapshots/ruff__flake8_blind_except__tests__BLE001_BLE.py.snap @@ -2,31 +2,8 @@ source: src/flake8_blind_except/mod.rs expression: checks --- -- kind: BlindExcept - location: - row: 5 - column: 7 - end_location: - row: 5 - column: 16 - fix: ~ -- kind: BlindExcept - location: - row: 13 - column: 7 - end_location: - row: 13 - column: 20 - fix: ~ -- kind: BlindExcept - location: - row: 23 - column: 7 - end_location: - row: 23 - column: 16 - fix: ~ -- kind: BlindExcept +- kind: + BlindExcept: BaseException location: row: 25 column: 7 @@ -34,7 +11,8 @@ expression: checks row: 25 column: 20 fix: ~ -- kind: BlindExcept +- kind: + BlindExcept: Exception location: row: 31 column: 7 @@ -42,15 +20,8 @@ expression: checks row: 31 column: 16 fix: ~ -- kind: BlindExcept - location: - row: 36 - column: 11 - end_location: - row: 36 - column: 24 - fix: ~ -- kind: BlindExcept +- kind: + BlindExcept: Exception location: row: 42 column: 7 @@ -58,7 +29,8 @@ expression: checks row: 42 column: 16 fix: ~ -- kind: BlindExcept +- kind: + BlindExcept: BaseException location: row: 45 column: 11 @@ -66,15 +38,8 @@ expression: checks row: 45 column: 24 fix: ~ -- kind: BlindExcept - location: - row: 52 - column: 11 - end_location: - row: 52 - column: 24 - fix: ~ -- kind: BlindExcept +- kind: + BlindExcept: Exception location: row: 54 column: 7 @@ -82,4 +47,22 @@ expression: checks row: 54 column: 16 fix: ~ +- kind: + BlindExcept: Exception + location: + row: 60 + column: 7 + end_location: + row: 60 + column: 16 + fix: ~ +- kind: + BlindExcept: BaseException + location: + row: 62 + column: 7 + end_location: + row: 62 + column: 20 + fix: ~ diff --git a/src/pycodestyle/checks.rs b/src/pycodestyle/checks.rs index 48c0d61a05..81a1467f36 100644 --- a/src/pycodestyle/checks.rs +++ b/src/pycodestyle/checks.rs @@ -1,11 +1,65 @@ use itertools::izip; -use rustpython_ast::Location; +use rustpython_ast::{Location, Stmt, StmtKind}; use rustpython_parser::ast::{Cmpop, Expr, ExprKind}; use crate::ast::types::Range; use crate::checks::{Check, CheckKind}; use crate::source_code_locator::SourceCodeLocator; +/// E721 +pub fn type_comparison(ops: &[Cmpop], comparators: &[Expr], location: Range) -> Vec { + let mut checks: Vec = vec![]; + + for (op, right) in izip!(ops, comparators) { + if !matches!(op, Cmpop::Is | Cmpop::IsNot | Cmpop::Eq | Cmpop::NotEq) { + continue; + } + match &right.node { + ExprKind::Call { func, args, .. } => { + if let ExprKind::Name { id, .. } = &func.node { + // Ex) type(False) + if id == "type" { + if let Some(arg) = args.first() { + // Allow comparison for types which are not obvious. + if !matches!(arg.node, ExprKind::Name { .. }) { + checks.push(Check::new(CheckKind::TypeComparison, location)); + } + } + } + } + } + ExprKind::Attribute { value, .. } => { + if let ExprKind::Name { id, .. } = &value.node { + // Ex) types.IntType + if id == "types" { + checks.push(Check::new(CheckKind::TypeComparison, location)); + } + } + } + _ => {} + } + } + + checks +} + +/// E722 +pub fn do_not_use_bare_except( + type_: Option<&Expr>, + body: &[Stmt], + location: Range, +) -> Option { + if type_.is_none() + && !body + .iter() + .any(|stmt| matches!(stmt.node, StmtKind::Raise { exc: None, .. })) + { + Some(Check::new(CheckKind::DoNotUseBareExcept, location)) + } else { + None + } +} + fn is_ambiguous_name(name: &str) -> bool { name == "l" || name == "I" || name == "O" } @@ -46,43 +100,6 @@ pub fn ambiguous_function_name(name: &str, location: Range) -> Option { } } -/// E721 -pub fn type_comparison(ops: &[Cmpop], comparators: &[Expr], location: Range) -> Vec { - let mut checks: Vec = vec![]; - - for (op, right) in izip!(ops, comparators) { - if !matches!(op, Cmpop::Is | Cmpop::IsNot | Cmpop::Eq | Cmpop::NotEq) { - continue; - } - match &right.node { - ExprKind::Call { func, args, .. } => { - if let ExprKind::Name { id, .. } = &func.node { - // Ex) type(False) - if id == "type" { - if let Some(arg) = args.first() { - // Allow comparison for types which are not obvious. - if !matches!(arg.node, ExprKind::Name { .. }) { - checks.push(Check::new(CheckKind::TypeComparison, location)); - } - } - } - } - } - ExprKind::Attribute { value, .. } => { - if let ExprKind::Name { id, .. } = &value.node { - // Ex) types.IntType - if id == "types" { - checks.push(Check::new(CheckKind::TypeComparison, location)); - } - } - } - _ => {} - } - } - - checks -} - // See: https://docs.python.org/3/reference/lexical_analysis.html#string-and-bytes-literals const VALID_ESCAPE_SEQUENCES: &[char; 23] = &[ '\n', '\\', '\'', '"', 'a', 'b', 'f', 'n', 'r', 't', 'v', '0', '1', '2', '3', '4', '5', '6',