mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-27 04:19:43 +00:00
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.
This commit is contained in:
parent
b7fe2b57de
commit
1bb61bab67
3 changed files with 159 additions and 99 deletions
|
@ -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")
|
||||
|
|
|
@ -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),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue