From ae4a7ef0edb287b52adef004f08b57e5a2786aa4 Mon Sep 17 00:00:00 2001
From: Evan Rittenhouse <evanrittenhouse@gmail.com>
Date: Mon, 10 Jul 2023 09:00:43 -0500
Subject: [PATCH] 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`.
---
 .../test/fixtures/tryceratops/TRY301.py       |  7 +++
 .../tryceratops/rules/raise_within_try.rs     | 44 ++++++++++++++++---
 2 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/crates/ruff/resources/test/fixtures/tryceratops/TRY301.py b/crates/ruff/resources/test/fixtures/tryceratops/TRY301.py
index da7c82e487..00544aa856 100644
--- a/crates/ruff/resources/test/fixtures/tryceratops/TRY301.py
+++ b/crates/ruff/resources/test/fixtures/tryceratops/TRY301.py
@@ -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")
diff --git a/crates/ruff/src/rules/tryceratops/rules/raise_within_try.rs b/crates/ruff/src/rules/tryceratops/rules/raise_within_try.rs
index f980a21dee..37711de6f5 100644
--- a/crates/ruff/src/rules/tryceratops/rules/raise_within_try.rs
+++ b/crates/ruff/src/rules/tryceratops/rules/raise_within_try.rs
@@ -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,
@@ -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()));
+        }
     }
 }