Implement TRY302 - raise after except (#4461)

crates/ruff/resources/test/fixtures/tryceratops/TRY301.py

@@ -1,3 +1,8 @@
+"""
+Violation:
+
+Checks for `raise` statements within `try` blocks.
+"""
 class MyException(Exception):
     pass
 

crates/ruff/resources/test/fixtures/tryceratops/TRY302.py

@@ -0,0 +1,111 @@
+"""
+Violation:
+
+Checks for uses of `raise` directly after a `rescue`.
+"""
+class MyException(Exception):
+    pass
+
+def bad():
+    try:
+        process()
+    except Exception:
+        raise
+
+def bad():
+    try:
+        process()
+    except Exception:
+        raise
+        print("this code is pointless!")
+
+def bad():
+    try:
+        process()
+    except:
+        # I am a comment, not a statement!
+        raise
+
+def bad():
+    try:
+        process()
+    except Exception:
+        raise
+
+def bad():
+    try:
+        process()
+    except Exception as e:
+        raise
+
+def bad():
+    try:
+        process()
+    except Exception as e:
+        raise e
+
+def bad():
+    try:
+        process()
+    except MyException:
+        raise
+    except Exception:
+        raise
+
+def bad():
+    try:
+        process()
+    except MyException as e:
+        raise e
+    except Exception as e:
+        raise e
+
+def bad():
+    try:
+        process()
+    except MyException as ex:
+        raise ex
+    except Exception as e:
+        raise e
+
+def fine():
+    try:
+        process()
+    except Exception as e:
+        raise ex
+
+def fine():
+    try:
+        process()
+    except MyException:
+        raise
+    except Exception:
+        print("bar")
+
+def fine():
+    try:
+        process()
+    except Exception:
+        print("initiating rapid unscheduled disassembly of program")
+
+def fine():
+    try:
+        process()
+    except MyException:
+        print("oh no!")
+        raise
+
+def fine():
+    try:
+        process()
+    except Exception:
+        if True:
+            raise
+
+def fine():
+    try:
+        process()
+    finally:
+        # I am a comment, not a statement!
+        print("but i am a statement")
+        raise

crates/ruff/src/checkers/ast/mod.rs

@@ -1699,6 +1699,9 @@ where
                 if self.settings.rules.enabled(Rule::RaiseWithinTry) {
                     tryceratops::rules::raise_within_try(self, body, handlers);
                 }
+                if self.settings.rules.enabled(Rule::UselessTryExcept) {
+                    tryceratops::rules::useless_try_except(self, handlers);
+                }
                 if self.settings.rules.enabled(Rule::ErrorInsteadOfException) {
                     tryceratops::rules::error_instead_of_exception(self, handlers);
                 }

crates/ruff/src/codes.rs

@@ -656,6 +656,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Tryceratops, "201") => (RuleGroup::Unspecified, Rule::VerboseRaise),
         (Tryceratops, "300") => (RuleGroup::Unspecified, Rule::TryConsiderElse),
         (Tryceratops, "301") => (RuleGroup::Unspecified, Rule::RaiseWithinTry),
+        (Tryceratops, "302") => (RuleGroup::Unspecified, Rule::UselessTryExcept),
         (Tryceratops, "400") => (RuleGroup::Unspecified, Rule::ErrorInsteadOfException),
         (Tryceratops, "401") => (RuleGroup::Unspecified, Rule::VerboseLogMessage),
 

crates/ruff/src/registry.rs

@@ -581,6 +581,7 @@ ruff_macros::register_rules!(
     rules::tryceratops::rules::ReraiseNoCause,
     rules::tryceratops::rules::VerboseRaise,
     rules::tryceratops::rules::TryConsiderElse,
+    rules::tryceratops::rules::UselessTryExcept,
     rules::tryceratops::rules::RaiseWithinTry,
     rules::tryceratops::rules::ErrorInsteadOfException,
     rules::tryceratops::rules::VerboseLogMessage,

crates/ruff/src/rules/tryceratops/mod.rs

@@ -22,6 +22,7 @@ mod tests {
     #[test_case(Rule::VerboseRaise, Path::new("TRY201.py"); "TRY201")]
     #[test_case(Rule::TryConsiderElse, Path::new("TRY300.py"); "TRY300")]
     #[test_case(Rule::RaiseWithinTry , Path::new("TRY301.py"); "TRY301")]
+    #[test_case(Rule::UselessTryExcept , Path::new("TRY302.py"); "TRY302")]
     #[test_case(Rule::ErrorInsteadOfException, Path::new("TRY400.py"); "TRY400")]
     #[test_case(Rule::VerboseLogMessage, Path::new("TRY401.py"); "TRY401")]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {

crates/ruff/src/rules/tryceratops/rules/mod.rs

@@ -7,6 +7,7 @@ pub(crate) use try_consider_else::{try_consider_else, TryConsiderElse};
 pub(crate) use type_check_without_type_error::{
     type_check_without_type_error, TypeCheckWithoutTypeError,
 };
+pub(crate) use useless_try_except::{useless_try_except, UselessTryExcept};
 pub(crate) use verbose_log_message::{verbose_log_message, VerboseLogMessage};
 pub(crate) use verbose_raise::{verbose_raise, VerboseRaise};
 
@@ -17,5 +18,6 @@ mod raise_within_try;
 mod reraise_no_cause;
 mod try_consider_else;
 mod type_check_without_type_error;
+mod useless_try_except;
 mod verbose_log_message;
 mod verbose_raise;

crates/ruff/src/rules/tryceratops/rules/useless_try_except.rs

@@ -0,0 +1,68 @@
+use rustpython_parser::ast::Excepthandler::ExceptHandler;
+use rustpython_parser::ast::{self, Excepthandler, ExcepthandlerExceptHandler, Expr, Ranged, Stmt};
+
+use ruff_diagnostics::{Diagnostic, Violation};
+use ruff_macros::{derive_message_formats, violation};
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for immediate uses of `raise` within exception handlers.
+///
+/// ## Why is this bad?
+/// Capturing an exception, only to immediately reraise it, has no effect.
+/// Instead, remove the error-handling code and let the exception propagate
+/// upwards without the unnecessary `try`-`except` block.
+///
+/// ## Example
+/// ```python
+/// def foo():
+///     try:
+///         bar()
+///     except NotImplementedError:
+///         raise
+/// ```
+///
+/// Use instead:
+/// ```python
+/// def foo():
+///     bar()
+/// ```
+#[violation]
+pub struct UselessTryExcept;
+
+impl Violation for UselessTryExcept {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Remove exception handler; error is immediately re-raised")
+    }
+}
+
+/// TRY302
+pub(crate) fn useless_try_except(checker: &mut Checker, handlers: &[Excepthandler]) {
+    if let Some(diagnostics) = handlers
+        .iter()
+        .map(|handler| {
+            let ExceptHandler(ExcepthandlerExceptHandler { name, body, .. }) = handler;
+            let Some(Stmt::Raise(ast::StmtRaise {  exc, .. })) = &body.first() else {
+                return None;
+            };
+            if let Some(expr) = exc {
+                // E.g., `except ... as e: raise e`
+                if let Expr::Name(ast::ExprName { id, .. }) = expr.as_ref() {
+                    if Some(id) == name.as_ref() {
+                        return Some(Diagnostic::new(UselessTryExcept, handler.range()));
+                    }
+                }
+                None
+            } else {
+                // E.g., `except ...: raise`
+                Some(Diagnostic::new(UselessTryExcept, handler.range()))
+            }
+        })
+        .collect::<Option<Vec<_>>>()
+    {
+        // Require that all handlers are useless, but create one diagnostic per handler.
+        checker.diagnostics.extend(diagnostics);
+    }
+}

crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__raise-within-try_TRY301.py.snap

@@ -1,64 +1,64 @@
 ---
 source: crates/ruff/src/rules/tryceratops/mod.rs
 ---
-TRY301.py:9:13: TRY301 Abstract `raise` to an inner function
+TRY301.py:14:13: TRY301 Abstract `raise` to an inner function
    |
- 9 |         a = process()
-10 |         if not a:
-11 |             raise MyException(a)
+14 |         a = process()
+15 |         if not a:
+16 |             raise MyException(a)
    |             ^^^^^^^^^^^^^^^^^^^^ TRY301
-12 | 
-13 |         raise MyException(a)
+17 | 
+18 |         raise MyException(a)
    |
 
-TRY301.py:11:9: TRY301 Abstract `raise` to an inner function
+TRY301.py:16:9: TRY301 Abstract `raise` to an inner function
    |
-11 |             raise MyException(a)
-12 | 
-13 |         raise MyException(a)
+16 |             raise MyException(a)
+17 | 
+18 |         raise MyException(a)
    |         ^^^^^^^^^^^^^^^^^^^^ TRY301
-14 | 
-15 |         try:
+19 | 
+20 |         try:
    |
 
-TRY301.py:16:17: TRY301 Abstract `raise` to an inner function
+TRY301.py:21:17: TRY301 Abstract `raise` to an inner function
    |
-16 |             b = process()
-17 |             if not b:
-18 |                 raise MyException(b)
+21 |             b = process()
+22 |             if not b:
+23 |                 raise MyException(b)
    |                 ^^^^^^^^^^^^^^^^^^^^ TRY301
-19 |         except Exception:
-20 |             logger.exception("something failed")
+24 |         except Exception:
+25 |             logger.exception("something failed")
    |
 
-TRY301.py:27:13: TRY301 Abstract `raise` to an inner function
+TRY301.py:32:13: TRY301 Abstract `raise` to an inner function
    |
-27 |         a = process()
-28 |         if not a:
-29 |             raise MyException(a)
+32 |         a = process()
+33 |         if not a:
+34 |             raise MyException(a)
    |             ^^^^^^^^^^^^^^^^^^^^ TRY301
-30 | 
-31 |         raise MyException(a)
+35 | 
+36 |         raise MyException(a)
    |
 
-TRY301.py:29:9: TRY301 Abstract `raise` to an inner function
+TRY301.py:34:9: TRY301 Abstract `raise` to an inner function
    |
-29 |             raise MyException(a)
-30 | 
-31 |         raise MyException(a)
+34 |             raise MyException(a)
+35 | 
+36 |         raise MyException(a)
    |         ^^^^^^^^^^^^^^^^^^^^ TRY301
-32 | 
-33 |         try:
+37 | 
+38 |         try:
    |
 
-TRY301.py:34:17: TRY301 Abstract `raise` to an inner function
+TRY301.py:39:17: TRY301 Abstract `raise` to an inner function
    |
-34 |             b = process()
-35 |             if not b:
-36 |                 raise MyException(b)
+39 |             b = process()
+40 |             if not b:
+41 |                 raise MyException(b)
    |                 ^^^^^^^^^^^^^^^^^^^^ TRY301
-37 |         except* Exception:
-38 |             logger.exception("something failed")
+42 |         except* Exception:
+43 |             logger.exception("something failed")
    |