diff --git a/crates/ruff/resources/test/fixtures/tryceratops/TRY301.py b/crates/ruff/resources/test/fixtures/tryceratops/TRY301.py index da7c82e4871127..4feb050e2b6f93 100644 --- a/crates/ruff/resources/test/fixtures/tryceratops/TRY301.py +++ b/crates/ruff/resources/test/fixtures/tryceratops/TRY301.py @@ -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") diff --git a/crates/ruff/src/rules/tryceratops/rules/raise_within_try.rs b/crates/ruff/src/rules/tryceratops/rules/raise_within_try.rs index f980a21deef65b..8850a7d4ceb4e2 100644 --- a/crates/ruff/src/rules/tryceratops/rules/raise_within_try.rs +++ b/crates/ruff/src/rules/tryceratops/rules/raise_within_try.rs @@ -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 { + return Some(id.as_str()); + }; + + 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, } }