mirror of
https://github.com/astral-sh/ruff.git
synced 2025-12-04 17:51:57 +00:00
Make TRY301 trigger only if a raise throws a caught exception (#5455)
## 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`.
This commit is contained in:
parent
cab3a507bc
commit
ae4a7ef0ed
2 changed files with 45 additions and 6 deletions
|
|
@ -57,3 +57,10 @@ def fine():
|
||||||
a = process() # This throws the exception now
|
a = process() # This throws the exception now
|
||||||
finally:
|
finally:
|
||||||
print("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")
|
||||||
|
|
|
||||||
|
|
@ -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_diagnostics::{Diagnostic, Violation};
|
||||||
use ruff_macros::{derive_message_formats, 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;
|
use crate::checkers::ast::Checker;
|
||||||
|
|
||||||
/// ## What it does
|
/// ## 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?
|
/// ## Why is this bad?
|
||||||
/// Raising and catching exceptions within the same `try` block is redundant,
|
/// Raising and catching exceptions within the same `try` block is redundant,
|
||||||
|
|
@ -83,9 +88,36 @@ pub(crate) fn raise_within_try(checker: &mut Checker, body: &[Stmt], handlers: &
|
||||||
visitor.raises
|
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 {
|
for stmt in raises {
|
||||||
checker
|
let Stmt::Raise(ast::StmtRaise { exc: Some(exception), .. }) = stmt else {
|
||||||
.diagnostics
|
continue;
|
||||||
.push(Diagnostic::new(RaiseWithinTry, stmt.range()));
|
};
|
||||||
|
|
||||||
|
// 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()));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue