From 099d5414f29040f527257fa4db01c51a26cffe56 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 12 Feb 2023 11:48:13 -0500 Subject: [PATCH] Allow non-verbose raise when cause is present (#2816) The motivating issue here is of the following form: ```py try: raise Exception("We want to hide this error message") except Exception: try: raise Exception("We want to show this") except Exception as exc: raise exc from None ``` However, I think we should avoid this if _any_ cause is present, since causes require a named exception. Closes #2814. --- .../test/fixtures/tryceratops/TRY201.py | 10 ++++++++ .../rules/tryceratops/rules/verbose_raise.rs | 23 +++++++++++-------- ...atops__tests__verbose-raise_TRY201.py.snap | 10 ++++---- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/tryceratops/TRY201.py b/crates/ruff/resources/test/fixtures/tryceratops/TRY201.py index 9ed57caf82..1ed7aa9e0b 100644 --- a/crates/ruff/resources/test/fixtures/tryceratops/TRY201.py +++ b/crates/ruff/resources/test/fixtures/tryceratops/TRY201.py @@ -27,6 +27,7 @@ def good(): logger.exception("process failed") raise + def still_good(): try: process() @@ -35,6 +36,14 @@ def still_good(): raise +def still_good_too(): + try: + process() + except MyException as e: + print(e) + raise e from None + + def still_actually_good(): try: process() @@ -60,5 +69,6 @@ def bad_that_needs_recursion_2(): except MyException as e: logger.exception("process failed") if True: + def foo(): raise e diff --git a/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs b/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs index 2119236a2e..4fa92d9618 100644 --- a/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs +++ b/crates/ruff/src/rules/tryceratops/rules/verbose_raise.rs @@ -20,7 +20,7 @@ impl Violation for VerboseRaise { #[derive(Default)] struct RaiseStatementVisitor<'a> { - raises: Vec>, + raises: Vec<(Option<&'a Expr>, Option<&'a Expr>)>, } impl<'a, 'b> Visitor<'b> for RaiseStatementVisitor<'a> @@ -29,7 +29,7 @@ where { fn visit_stmt(&mut self, stmt: &'b Stmt) { match &stmt.node { - StmtKind::Raise { exc, .. } => self.raises.push(exc.as_ref().map(|expr| &**expr)), + StmtKind::Raise { exc, cause } => self.raises.push((exc.as_deref(), cause.as_deref())), StmtKind::Try { body, finalbody, .. } => { @@ -59,13 +59,18 @@ pub fn verbose_raise(checker: &mut Checker, handlers: &[Excepthandler]) { } visitor.raises }; - for expr in raises.into_iter().flatten() { - // ...and the raised object is bound to the same name... - if let ExprKind::Name { id, .. } = &expr.node { - if id == exception_name { - checker - .diagnostics - .push(Diagnostic::new(VerboseRaise, Range::from_located(expr))); + for (exc, cause) in raises { + if cause.is_some() { + continue; + } + if let Some(exc) = exc { + // ...and the raised object is bound to the same name... + if let ExprKind::Name { id, .. } = &exc.node { + if id == exception_name { + checker + .diagnostics + .push(Diagnostic::new(VerboseRaise, Range::from_located(exc))); + } } } } diff --git a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap index e6c03c5f6c..f230c296b6 100644 --- a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap +++ b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-raise_TRY201.py.snap @@ -1,5 +1,5 @@ --- -source: src/rules/tryceratops/mod.rs +source: crates/ruff/src/rules/tryceratops/mod.rs expression: diagnostics --- - kind: @@ -15,20 +15,20 @@ expression: diagnostics - kind: VerboseRaise: ~ location: - row: 54 + row: 63 column: 18 end_location: - row: 54 + row: 63 column: 19 fix: ~ parent: ~ - kind: VerboseRaise: ~ location: - row: 64 + row: 74 column: 22 end_location: - row: 64 + row: 74 column: 23 fix: ~ parent: ~