Skip to content

Commit

Permalink
Make TRY301 trigger if a raise throws the same exception that the s…
Browse files Browse the repository at this point in the history
…urrounding `try` catches
  • Loading branch information
evanrittenhouse committed Jun 30, 2023
1 parent f0ec9ec commit 8556421
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 6 deletions.
8 changes: 8 additions & 0 deletions crates/ruff/resources/test/fixtures/tryceratops/TRY301.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
57 changes: 51 additions & 6 deletions crates/ruff/src/rules/tryceratops/rules/raise_within_try.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -77,15 +82,55 @@ 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()
.flat_map(|handler| {
if let Expr::Name(ast::ExprName { id, .. }) = handler {
return Some(id.as_str());
} else {
return None;
}
})
.collect();

let raises = {
let mut visitor = RaiseStatementVisitor::default();
visitor.visit_body(body);
visitor.raises
};

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,
}
}

0 comments on commit 8556421

Please sign in to comment.