Minor tweaks

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

@@ -5,7 +5,7 @@ def main_function():
         handle()
         finish()
     except Exception as ex:
-        logger.exception(f"Found an error: {ex}")
+        logger.exception(f"Found an error: {ex}")  # TRY401
 
 
 def main_function():
@@ -16,12 +16,12 @@ def main_function():
     except ValueError as bad:
         if True is False:
             for i in range(10):
-                logger.exception(f"Found an error: {bad} {good}")
+                logger.exception(f"Found an error: {bad} {good}")  # TRY401
     except IndexError as bad:
-        logger.exception(f"Foud an error: {bad} {bad}")
+        logger.exception(f"Found an error: {bad} {bad}")  # TRY401
     except Exception as bad:
-        logger.exception(f"Foud an error: {bad}")
-        logger.exception(f"Foud an error: {bad}")
+        logger.exception(f"Found an error: {bad}")  # TRY401
+        logger.exception(f"Found an error: {bad}")  # TRY401
 
 
 import logging
@@ -33,21 +33,21 @@ def func_fstr():
     try:
         ...
     except Exception as ex:
-        logger.exception(f"log message {ex}")
+        logger.exception(f"Logging an error: {ex}")  # TRY401
 
 
 def func_concat():
     try:
         ...
     except Exception as ex:
-        logger.exception("log message: " + str(ex))
+        logger.exception("Logging an error: " + str(ex))  # TRY401
 
 
 def func_comma():
     try:
         ...
     except Exception as ex:
-        logger.exception("log message", ex)
+        logger.exception("Logging an error:", ex)  # TRY401
 
 
 # OK
@@ -56,5 +56,5 @@ def main_function():
         process()
         handle()
         finish()
-    except Exception as ex:  # noqa: F841
+    except Exception as ex:
         logger.exception(f"Found an error: {er}")

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

@@ -1,4 +1,4 @@
-use rustpython_parser::ast::{Excepthandler, ExcepthandlerKind, Expr, ExprKind};
+use rustpython_parser::ast::{Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind};
 
 use ruff_macros::{define_violation, derive_message_formats};
 
@@ -12,10 +12,12 @@ use crate::violation::Violation;
 
 define_violation!(
     /// ### What it does
-    /// Checks for excessive logging of the exception object
+    /// Checks for excessive logging of exception objects.
     ///
     /// ### Why is this bad?
-    /// When using `logger.exception`, the exception object is logged automatically.
+    /// When logging exceptions via `logging.exception`, the exception object
+    /// is logged automatically. Including the exception object in the log
+    /// message is redundant and can lead to excessive logging.
     ///
     /// ### Example
     /// ```python
@@ -37,61 +39,68 @@ define_violation!(
 impl Violation for VerboseLogMessage {
     #[derive_message_formats]
     fn message(&self) -> String {
-        format!("Do not log the exception object")
+        format!("Redundant exception object included in `logging.exception` call")
     }
 }
 
 #[derive(Default)]
-pub struct NameVisitor<'a> {
-    pub names: Vec<&'a Expr>,
+struct NameVisitor<'a> {
+    names: Vec<(&'a str, &'a Expr)>,
 }
 
 impl<'a, 'b> Visitor<'b> for NameVisitor<'a>
 where
     'b: 'a,
 {
     fn visit_expr(&mut self, expr: &'b Expr) {
-        if let ExprKind::Name { .. } = &expr.node {
-            self.names.push(expr);
+        if let ExprKind::Name {
+            id,
+            ctx: ExprContext::Load,
+        } = &expr.node
+        {
+            self.names.push((id, expr));
         }
         visitor::walk_expr(self, expr);
     }
 }
 
-fn check_names(checker: &mut Checker, exprs: &[&Expr], target: &str) {
-    for expr in exprs {
-        if let ExprKind::Name { id, .. } = &expr.node {
-            if id == target {
-                checker.diagnostics.push(Diagnostic::new(
-                    VerboseLogMessage,
-                    Range::from_located(expr),
-                ));
-            }
-        }
-    }
-}
-
 /// TRY401
 pub fn verbose_log_message(checker: &mut Checker, handlers: &[Excepthandler]) {
     for handler in handlers {
         let ExcepthandlerKind::ExceptHandler { name, body, .. } = &handler.node;
-        if let Some(clean_name) = name {
-            let calls = {
-                let mut visitor = LoggerCandidateVisitor::default();
-                visitor.visit_body(body);
-                visitor.calls
+        let Some(target) = name else {
+            continue;
+        };
+
+        // Find all calls to `logging.exception`.
+        let calls = {
+            let mut visitor = LoggerCandidateVisitor::default();
+            visitor.visit_body(body);
+            visitor.calls
+        };
+
+        for (expr, func) in calls {
+            let ExprKind::Call { args, .. } = &expr.node else {
+                continue;
             };
-            for (expr, func) in calls {
-                if let ExprKind::Call { args, .. } = &expr.node {
-                    let mut all_names: Vec<&Expr> = vec![];
-                    for arg in args {
-                        let mut visitor = NameVisitor::default();
-                        visitor.visit_expr(arg);
-                        all_names.extend(visitor.names);
-                    }
-                    if let ExprKind::Attribute { attr, .. } = &func.node {
-                        if attr == "exception" {
-                            check_names(checker, &all_names, clean_name);
+            if let ExprKind::Attribute { attr, .. } = &func.node {
+                if attr == "exception" {
+                    // Collect all referenced names in the `logging.exception` call.
+                    let names: Vec<(&str, &Expr)> = {
+                        let mut names = Vec::new();
+                        for arg in args {
+                            let mut visitor = NameVisitor::default();
+                            visitor.visit_expr(arg);
+                            names.extend(visitor.names);
+                        }
+                        names
+                    };
+                    for (id, expr) in names {
+                        if id == target {
+                            checker.diagnostics.push(Diagnostic::new(
+                                VerboseLogMessage,
+                                Range::from_located(expr),
+                            ));
                         }
                     }
                 }

crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-log-message_TRY401.py.snap

@@ -26,70 +26,70 @@ expression: diagnostics
     VerboseLogMessage: ~
   location:
     row: 21
-    column: 43
+    column: 44
   end_location:
     row: 21
-    column: 46
+    column: 47
   fix: ~
   parent: ~
 - kind:
     VerboseLogMessage: ~
   location:
     row: 21
-    column: 49
+    column: 50
   end_location:
     row: 21
-    column: 52
+    column: 53
   fix: ~
   parent: ~
 - kind:
     VerboseLogMessage: ~
   location:
     row: 23
-    column: 43
+    column: 44
   end_location:
     row: 23
-    column: 46
+    column: 47
   fix: ~
   parent: ~
 - kind:
     VerboseLogMessage: ~
   location:
     row: 24
-    column: 43
+    column: 44
   end_location:
     row: 24
-    column: 46
+    column: 47
   fix: ~
   parent: ~
 - kind:
     VerboseLogMessage: ~
   location:
     row: 36
-    column: 40
+    column: 46
   end_location:
     row: 36
-    column: 42
+    column: 48
   fix: ~
   parent: ~
 - kind:
     VerboseLogMessage: ~
   location:
     row: 43
-    column: 47
+    column: 52
   end_location:
     row: 43
-    column: 49
+    column: 54
   fix: ~
   parent: ~
 - kind:
     VerboseLogMessage: ~
   location:
     row: 50
-    column: 40
+    column: 46
   end_location:
     row: 50
-    column: 42
+    column: 48
   fix: ~
   parent: ~