From 1bb61bab67a22dc52c8ce3ae237e2d6df2369470 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 5 May 2024 21:52:09 -0400 Subject: [PATCH] Respect logged and re-raised expressions in nested statements (#11301) ## Summary Historically, we only ignored `flake8-blind-except` if you re-raised or logged the exception as a _direct_ child statement; but it could be nested somewhere. This was just a known limitation at the time of adding the previous logic. Closes https://github.com/astral-sh/ruff/issues/11289. --- .../test/fixtures/flake8_blind_except/BLE.py | 9 + .../flake8_blind_except/rules/blind_except.rs | 203 +++++++++++------- .../rules/tryceratops/rules/verbose_raise.rs | 46 ++-- 3 files changed, 159 insertions(+), 99 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_blind_except/BLE.py b/crates/ruff_linter/resources/test/fixtures/flake8_blind_except/BLE.py index 10cca835cc..8282bd1059 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_blind_except/BLE.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_blind_except/BLE.py @@ -129,3 +129,12 @@ try: ... except Exception as e: raise ValueError from e + + +try: + pass +except Exception: + if True: + exception("An error occurred") + else: + exception("An error occurred") diff --git a/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs b/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs index be2262c130..35945d9c1d 100644 --- a/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs +++ b/crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs @@ -1,8 +1,10 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::is_const_true; +use ruff_python_ast::statement_visitor::{walk_stmt, StatementVisitor}; use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_python_semantic::analyze::logging; +use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -87,87 +89,18 @@ pub(crate) fn blind_except( if !matches!(builtin_exception_type, "BaseException" | "Exception") { return; } + // If the exception is re-raised, don't flag an error. - if body.iter().any(|stmt| { - if let Stmt::Raise(ast::StmtRaise { exc, cause, .. }) = stmt { - if let Some(cause) = cause { - if let Expr::Name(ast::ExprName { id, .. }) = cause.as_ref() { - name.is_some_and(|name| id == name) - } else { - false - } - } else { - if let Some(exc) = exc { - if let Expr::Name(ast::ExprName { id, .. }) = exc.as_ref() { - name.is_some_and(|name| id == name) - } else { - false - } - } else { - true - } - } - } else { - false - } - }) { + let mut visitor = ReraiseVisitor::new(name); + visitor.visit_body(body); + if visitor.seen() { return; } // If the exception is logged, don't flag an error. - if body.iter().any(|stmt| { - if let Stmt::Expr(ast::StmtExpr { value, range: _ }) = stmt { - if let Expr::Call(ast::ExprCall { - func, arguments, .. - }) = value.as_ref() - { - match func.as_ref() { - Expr::Attribute(ast::ExprAttribute { attr, .. }) => { - if logging::is_logger_candidate( - func, - semantic, - &checker.settings.logger_objects, - ) { - match attr.as_str() { - "exception" => return true, - "error" => { - if let Some(keyword) = arguments.find_keyword("exc_info") { - if is_const_true(&keyword.value) { - return true; - } - } - } - _ => {} - } - } - } - Expr::Name(ast::ExprName { .. }) => { - if semantic - .resolve_qualified_name(func) - .is_some_and(|qualified_name| match qualified_name.segments() { - ["logging", "exception"] => true, - ["logging", "error"] => { - if let Some(keyword) = arguments.find_keyword("exc_info") { - if is_const_true(&keyword.value) { - return true; - } - } - false - } - _ => false, - }) - { - return true; - } - } - _ => { - return false; - } - } - } - } - false - }) { + let mut visitor = LogExceptionVisitor::new(semantic, &checker.settings.logger_objects); + visitor.visit_body(body); + if visitor.seen() { return; } @@ -178,3 +111,121 @@ pub(crate) fn blind_except( type_.range(), )); } + +/// A visitor to detect whether the exception with the given name was re-raised. +struct ReraiseVisitor<'a> { + name: Option<&'a str>, + seen: bool, +} + +impl<'a> ReraiseVisitor<'a> { + /// Create a new [`ReraiseVisitor`] with the given exception name. + fn new(name: Option<&'a str>) -> Self { + Self { name, seen: false } + } + + /// Returns `true` if the exception was re-raised. + fn seen(&self) -> bool { + self.seen + } +} + +impl<'a> StatementVisitor<'a> for ReraiseVisitor<'a> { + fn visit_stmt(&mut self, stmt: &'a Stmt) { + match stmt { + Stmt::Raise(ast::StmtRaise { exc, cause, .. }) => { + if let Some(cause) = cause { + if let Expr::Name(ast::ExprName { id, .. }) = cause.as_ref() { + if self.name.is_some_and(|name| id == name) { + self.seen = true; + } + } + } else { + if let Some(exc) = exc { + if let Expr::Name(ast::ExprName { id, .. }) = exc.as_ref() { + if self.name.is_some_and(|name| id == name) { + self.seen = true; + } + } + } else { + self.seen = true; + } + } + } + Stmt::Try(_) | Stmt::FunctionDef(_) | Stmt::ClassDef(_) => {} + _ => walk_stmt(self, stmt), + } + } +} + +/// A visitor to detect whether the exception was logged. +struct LogExceptionVisitor<'a> { + semantic: &'a SemanticModel<'a>, + logger_objects: &'a [String], + seen: bool, +} + +impl<'a> LogExceptionVisitor<'a> { + /// Create a new [`LogExceptionVisitor`] with the given exception name. + fn new(semantic: &'a SemanticModel<'a>, logger_objects: &'a [String]) -> Self { + Self { + semantic, + logger_objects, + seen: false, + } + } + + /// Returns `true` if the exception was logged. + fn seen(&self) -> bool { + self.seen + } +} + +impl<'a> StatementVisitor<'a> for LogExceptionVisitor<'a> { + fn visit_stmt(&mut self, stmt: &'a Stmt) { + match stmt { + Stmt::Expr(ast::StmtExpr { value, .. }) => { + if let Expr::Call(ast::ExprCall { + func, arguments, .. + }) = value.as_ref() + { + match func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => { + if logging::is_logger_candidate( + func, + self.semantic, + self.logger_objects, + ) { + if match attr.as_str() { + "exception" => true, + "error" => arguments + .find_keyword("exc_info") + .is_some_and(|keyword| is_const_true(&keyword.value)), + _ => false, + } { + self.seen = true; + } + } + } + Expr::Name(ast::ExprName { .. }) => { + if self.semantic.resolve_qualified_name(func).is_some_and( + |qualified_name| match qualified_name.segments() { + ["logging", "exception"] => true, + ["logging", "error"] => arguments + .find_keyword("exc_info") + .is_some_and(|keyword| is_const_true(&keyword.value)), + _ => false, + }, + ) { + self.seen = true; + } + } + _ => {} + } + } + } + Stmt::Try(_) | Stmt::FunctionDef(_) | Stmt::ClassDef(_) => {} + _ => walk_stmt(self, stmt), + } + } +} diff --git a/crates/ruff_linter/src/rules/tryceratops/rules/verbose_raise.rs b/crates/ruff_linter/src/rules/tryceratops/rules/verbose_raise.rs index b1747f63e3..4d975889d1 100644 --- a/crates/ruff_linter/src/rules/tryceratops/rules/verbose_raise.rs +++ b/crates/ruff_linter/src/rules/tryceratops/rules/verbose_raise.rs @@ -49,29 +49,6 @@ impl AlwaysFixableViolation for VerboseRaise { } } -#[derive(Default)] -struct RaiseStatementVisitor<'a> { - raises: Vec<&'a ast::StmtRaise>, -} - -impl<'a> StatementVisitor<'a> for RaiseStatementVisitor<'a> { - fn visit_stmt(&mut self, stmt: &'a Stmt) { - match stmt { - Stmt::Raise(raise @ ast::StmtRaise { .. }) => { - self.raises.push(raise); - } - Stmt::Try(ast::StmtTry { - body, finalbody, .. - }) => { - for stmt in body.iter().chain(finalbody.iter()) { - walk_stmt(self, stmt); - } - } - _ => walk_stmt(self, stmt), - } - } -} - /// TRY201 pub(crate) fn verbose_raise(checker: &mut Checker, handlers: &[ExceptHandler]) { for handler in handlers { @@ -108,3 +85,26 @@ pub(crate) fn verbose_raise(checker: &mut Checker, handlers: &[ExceptHandler]) { } } } + +#[derive(Default)] +struct RaiseStatementVisitor<'a> { + raises: Vec<&'a ast::StmtRaise>, +} + +impl<'a> StatementVisitor<'a> for RaiseStatementVisitor<'a> { + fn visit_stmt(&mut self, stmt: &'a Stmt) { + match stmt { + Stmt::Raise(raise @ ast::StmtRaise { .. }) => { + self.raises.push(raise); + } + Stmt::Try(ast::StmtTry { + body, finalbody, .. + }) => { + for stmt in body.iter().chain(finalbody.iter()) { + walk_stmt(self, stmt); + } + } + _ => walk_stmt(self, stmt), + } + } +}