Make TRY301 trigger if a `raise` throws the same exception that the surrounding `try` catches

This commit is contained in:
Evan Rittenhouse 2023-06-30 18:23:14 -05:00
parent f0ec9ecd67
commit 3831594e42
2 changed files with 59 additions and 6 deletions

View File

@ -57,3 +57,11 @@ def fine():
a = process() # This throws the exception now
finally:
print("finally")
def fine():
try:
raise ValueError("a doesn't exist")
except TypeError: # A different exception is caught
print("A different exception is caught")

View File

@ -1,13 +1,18 @@
use rustpython_parser::ast::{ExceptHandler, Ranged, Stmt};
use rustc_hash::FxHashSet;
use rustpython_parser::ast::{self, ExceptHandler, Expr, Ranged, Stmt};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::statement_visitor::{walk_stmt, StatementVisitor};
use ruff_python_ast::{
helpers,
statement_visitor::{walk_stmt, StatementVisitor},
};
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for `raise` statements within `try` blocks.
/// Checks for `raise` statements within `try` blocks. The only `raise`s
/// caught are those that throw exceptions caught by the `try` statement itself.
///
/// ## Why is this bad?
/// Raising and catching exceptions within the same `try` block is redundant,
@ -77,6 +82,20 @@ pub(crate) fn raise_within_try(checker: &mut Checker, body: &[Stmt], handlers: &
return;
}
// The names of exceptions handled by the `try` Stmt. We can't compare `Expr`'s since they have
// different ranges, but by virtue of this function's call path we know that the `raise`
// statements will always be within the surrounding `try`.
let handler_ids: FxHashSet<&str> = helpers::extract_handled_exceptions(handlers)
.into_iter()
.filter_map(|handler| {
if let Expr::Name(ast::ExprName { id, .. }) = handler {
Some(id.as_str())
} else {
None
}
})
.collect();
let raises = {
let mut visitor = RaiseStatementVisitor::default();
visitor.visit_body(body);
@ -84,8 +103,34 @@ pub(crate) fn raise_within_try(checker: &mut Checker, body: &[Stmt], handlers: &
};
for stmt in raises {
checker
.diagnostics
.push(Diagnostic::new(RaiseWithinTry, stmt.range()));
let Stmt::Raise(ast::StmtRaise { exc, .. }) = stmt else {
continue;
};
let Some(exception) = exc else {
continue;
};
let exc_name = get_function_name(exception.as_ref()).unwrap_or_default();
// We can't check exception sub-classes without a type-checker implementation, so let's
// just catch the blanket `Exception` for now.
if handler_ids.contains(exc_name) || handler_ids.contains("Exception") {
checker
.diagnostics
.push(Diagnostic::new(RaiseWithinTry, stmt.range()));
}
}
}
/// Get the name of an [`Expr::Call`], if applicable. If the passed [`Expr`] isn't a [`Expr::Call`], return an
/// empty [`Option`].
fn get_function_name(expr: &Expr) -> Option<&str> {
let Expr::Call(ast::ExprCall { func, .. }) = expr else {
return None;
};
match func.as_ref() {
Expr::Name(ast::ExprName { id, .. }) => Some(id.as_str()),
_ => None,
}
}