Skip to content

Commit

Permalink
Make TRY301 trigger only if a raise throws a caught exception (#5455)
Browse files Browse the repository at this point in the history
## Summary

Fixes #5246. We generate a hash set of all exception IDs caught by the
`try` statement, then check that the inner `raise` actually raises a
caught exception.

## Test Plan

Added a new test, `cargo t`.
  • Loading branch information
evanrittenhouse committed Jul 10, 2023
1 parent cab3a50 commit ae4a7ef
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 6 deletions.
7 changes: 7 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,10 @@ 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")
44 changes: 38 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 rustpython_parser::ast::{self, ExceptHandler, 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::{
comparable::ComparableExpr,
helpers::{self, map_callable},
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 @@ -83,9 +88,36 @@ pub(crate) fn raise_within_try(checker: &mut Checker, body: &[Stmt], handlers: &
visitor.raises
};

if raises.is_empty() {
return;
}

let handled_exceptions = helpers::extract_handled_exceptions(handlers);
let comparables: Vec<ComparableExpr> = handled_exceptions
.iter()
.map(|handler| ComparableExpr::from(*handler))
.collect();

for stmt in raises {
checker
.diagnostics
.push(Diagnostic::new(RaiseWithinTry, stmt.range()));
let Stmt::Raise(ast::StmtRaise { exc: Some(exception), .. }) = stmt else {
continue;
};

// We can't check exception sub-classes without a type-checker implementation, so let's
// just catch the blanket `Exception` for now.
if comparables.contains(&ComparableExpr::from(map_callable(exception)))
|| handled_exceptions.iter().any(|expr| {
checker
.semantic()
.resolve_call_path(expr)
.map_or(false, |call_path| {
matches!(call_path.as_slice(), ["", "Exception" | "BaseException"])
})
})
{
checker
.diagnostics
.push(Diagnostic::new(RaiseWithinTry, stmt.range()));
}
}
}

0 comments on commit ae4a7ef

Please sign in to comment.