Skip to content

Commit

Permalink
Respect logged and re-raised expressions in nested statements (#11301)
Browse files Browse the repository at this point in the history
## 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 #11289.
  • Loading branch information
charliermarsh authored May 6, 2024
1 parent b7fe2b5 commit 1bb61ba
Show file tree
Hide file tree
Showing 3 changed files with 155 additions and 95 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -129,3 +129,12 @@
...
except Exception as e:
raise ValueError from e


try:
pass
except Exception:
if True:
exception("An error occurred")
else:
exception("An error occurred")
195 changes: 123 additions & 72 deletions crates/ruff_linter/src/rules/flake8_blind_except/rules/blind_except.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -87,94 +89,143 @@ 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)
let mut visitor = ReraiseVisitor::new(name);
visitor.visit_body(body);
if visitor.seen() {
return;
}

// If the exception is logged, don't flag an error.
let mut visitor = LogExceptionVisitor::new(semantic, &checker.settings.logger_objects);
visitor.visit_body(body);
if visitor.seen() {
return;
}

checker.diagnostics.push(Diagnostic::new(
BlindExcept {
name: builtin_exception_type.to_string(),
},
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 {
false
}
} else {
if let Some(exc) = exc {
if let Expr::Name(ast::ExprName { id, .. }) = exc.as_ref() {
name.is_some_and(|name| id == name)
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 {
false
self.seen = true;
}
} else {
true
}
}
} else {
false
Stmt::Try(_) | Stmt::FunctionDef(_) | Stmt::ClassDef(_) => {}
_ => walk_stmt(self, stmt),
}
}) {
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;
}
}
/// 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 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;
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;
}
}
}
_ => {
return false;
_ => {}
}
}
}
Stmt::Try(_) | Stmt::FunctionDef(_) | Stmt::ClassDef(_) => {}
_ => walk_stmt(self, stmt),
}
false
}) {
return;
}

checker.diagnostics.push(Diagnostic::new(
BlindExcept {
name: builtin_exception_type.to_string(),
},
type_.range(),
));
}
46 changes: 23 additions & 23 deletions crates/ruff_linter/src/rules/tryceratops/rules/verbose_raise.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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),
}
}
}

0 comments on commit 1bb61ba

Please sign in to comment.