[tryceratops]: Verbose Log Messages (#3036)

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

@@ -0,0 +1,64 @@
+# Errors
+def main_function():
+    try:
+        process()
+        handle()
+        finish()
+    except Exception as ex:
+        logger.exception(f"Found an error: {ex}")  # TRY401
+
+
+def main_function():
+    try:
+        process()
+        handle()
+        finish()
+    except ValueError as bad:
+        if True is False:
+            for i in range(10):
+                logger.exception(f"Found an error: {bad} {good}")  # TRY401
+    except IndexError as bad:
+        logger.exception(f"Found an error: {bad} {bad}")  # TRY401
+    except Exception as bad:
+        logger.exception(f"Found an error: {bad}")  # TRY401
+        logger.exception(f"Found an error: {bad}")  # TRY401
+
+        if True:
+            logger.exception(f"Found an error: {bad}")  # TRY401
+
+
+import logging
+
+logger = logging.getLogger(__name__)
+
+
+def func_fstr():
+    try:
+        ...
+    except Exception as ex:
+        logger.exception(f"Logging an error: {ex}")  # TRY401
+
+
+def func_concat():
+    try:
+        ...
+    except Exception as ex:
+        logger.exception("Logging an error: " + str(ex))  # TRY401
+
+
+def func_comma():
+    try:
+        ...
+    except Exception as ex:
+        logger.exception("Logging an error:", ex)  # TRY401
+
+
+# OK
+def main_function():
+    try:
+        process()
+        handle()
+        finish()
+    except Exception as ex:
+        logger.exception(f"Found an error: {er}")
+        logger.exception(f"Found an error: {ex.field}")

crates/ruff/src/checkers/ast.rs

@@ -1752,6 +1752,9 @@ where
                 if self.settings.rules.enabled(&Rule::VerboseRaise) {
                     tryceratops::rules::verbose_raise(self, handlers);
                 }
+                if self.settings.rules.enabled(&Rule::VerboseLogMessage) {
+                    tryceratops::rules::verbose_log_message(self, handlers);
+                }
                 if self.settings.rules.enabled(&Rule::RaiseWithinTry) {
                     tryceratops::rules::raise_within_try(self, body);
                 }

crates/ruff/src/codes.rs

@@ -545,6 +545,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
         (Tryceratops, "300") => Rule::TryConsiderElse,
         (Tryceratops, "301") => Rule::RaiseWithinTry,
         (Tryceratops, "400") => Rule::ErrorInsteadOfException,
+        (Tryceratops, "401") => Rule::VerboseLogMessage,
 
         // flake8-use-pathlib
         (Flake8UsePathlib, "100") => Rule::PathlibAbspath,

crates/ruff/src/registry.rs

@@ -512,6 +512,7 @@ ruff_macros::register_rules!(
     rules::tryceratops::rules::TryConsiderElse,
     rules::tryceratops::rules::RaiseWithinTry,
     rules::tryceratops::rules::ErrorInsteadOfException,
+    rules::tryceratops::rules::VerboseLogMessage,
     // flake8-use-pathlib
     rules::flake8_use_pathlib::violations::PathlibAbspath,
     rules::flake8_use_pathlib::violations::PathlibChmod,

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

@@ -0,0 +1,24 @@
+use crate::ast::helpers::is_logger_candidate;
+use crate::ast::visitor;
+use crate::ast::visitor::Visitor;
+use rustpython_parser::ast::{Expr, ExprKind};
+
+#[derive(Default)]
+/// Collect `logging`-like calls from an AST.
+pub struct LoggerCandidateVisitor<'a> {
+    pub calls: Vec<(&'a Expr, &'a Expr)>,
+}
+
+impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a>
+where
+    'b: 'a,
+{
+    fn visit_expr(&mut self, expr: &'b Expr) {
+        if let ExprKind::Call { func, .. } = &expr.node {
+            if is_logger_candidate(func) {
+                self.calls.push((expr, func));
+            }
+        }
+        visitor::walk_expr(self, expr);
+    }
+}

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

@@ -1,4 +1,5 @@
 //! Rules from [tryceratops](https://pypi.org/project/tryceratops/1.1.0/).
+pub(crate) mod helpers;
 pub(crate) mod rules;
 
 #[cfg(test)]
@@ -21,6 +22,7 @@ mod tests {
     #[test_case(Rule::TryConsiderElse, Path::new("TRY300.py"); "TRY300")]
     #[test_case(Rule::RaiseWithinTry , Path::new("TRY301.py"); "TRY301")]
     #[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<()> {
         let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy());
         let diagnostics = test_path(

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

@@ -1,12 +1,11 @@
 use ruff_macros::{define_violation, derive_message_formats};
-use rustpython_parser::ast::{Excepthandler, ExcepthandlerKind, Expr, ExprKind};
+use rustpython_parser::ast::{Excepthandler, ExcepthandlerKind, ExprKind};
 
-use crate::ast::helpers::is_logger_candidate;
 use crate::ast::types::Range;
-use crate::ast::visitor;
 use crate::ast::visitor::Visitor;
 use crate::checkers::ast::Checker;
 use crate::registry::Diagnostic;
+use crate::rules::tryceratops::helpers::LoggerCandidateVisitor;
 use crate::violation::Violation;
 
 define_violation!(
@@ -19,26 +18,6 @@ impl Violation for ErrorInsteadOfException {
     }
 }
 
-#[derive(Default)]
-/// Collect `logging`-like calls from an AST.
-struct LoggerCandidateVisitor<'a> {
-    calls: Vec<(&'a Expr, &'a Expr)>,
-}
-
-impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a>
-where
-    'b: 'a,
-{
-    fn visit_expr(&mut self, expr: &'b Expr) {
-        if let ExprKind::Call { func, .. } = &expr.node {
-            if is_logger_candidate(func) {
-                self.calls.push((expr, func));
-            }
-        }
-        visitor::walk_expr(self, expr);
-    }
-}
-
 /// TRY400
 pub fn error_instead_of_exception(checker: &mut Checker, handlers: &[Excepthandler]) {
     for handler in handlers {

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

@@ -5,6 +5,7 @@ pub use raise_vanilla_class::{raise_vanilla_class, RaiseVanillaClass};
 pub use raise_within_try::{raise_within_try, RaiseWithinTry};
 pub use reraise_no_cause::{reraise_no_cause, ReraiseNoCause};
 pub use try_consider_else::{try_consider_else, TryConsiderElse};
+pub use verbose_log_message::{verbose_log_message, VerboseLogMessage};
 pub use verbose_raise::{verbose_raise, VerboseRaise};
 
 mod error_instead_of_exception;
@@ -14,4 +15,5 @@ mod raise_vanilla_class;
 mod raise_within_try;
 mod reraise_no_cause;
 mod try_consider_else;
+mod verbose_log_message;
 mod verbose_raise;

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

@@ -0,0 +1,110 @@
+use rustpython_parser::ast::{Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind};
+
+use ruff_macros::{define_violation, derive_message_formats};
+
+use crate::ast::types::Range;
+use crate::ast::visitor;
+use crate::ast::visitor::Visitor;
+use crate::checkers::ast::Checker;
+use crate::registry::Diagnostic;
+use crate::rules::tryceratops::helpers::LoggerCandidateVisitor;
+use crate::violation::Violation;
+
+define_violation!(
+    /// ### What it does
+    /// Checks for excessive logging of exception objects.
+    ///
+    /// ### Why is this bad?
+    /// 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
+    /// try:
+    ///     ...
+    /// except ValueError as e:
+    ///     logger.exception(f"Found an error: {e}")
+    /// ```
+    ///
+    /// Use instead:
+    /// ```python
+    /// try:
+    ///     ...
+    /// except ValueError as e:
+    ///     logger.exception(f"Found an error")
+    /// ```
+    pub struct VerboseLogMessage;
+);
+impl Violation for VerboseLogMessage {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Redundant exception object included in `logging.exception` call")
+    }
+}
+
+#[derive(Default)]
+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) {
+        match &expr.node {
+            ExprKind::Name {
+                id,
+                ctx: ExprContext::Load,
+            } => self.names.push((id, expr)),
+            ExprKind::Attribute { .. } => {}
+            _ => visitor::walk_expr(self, expr),
+        }
+    }
+}
+
+/// TRY401
+pub fn verbose_log_message(checker: &mut Checker, handlers: &[Excepthandler]) {
+    for handler in handlers {
+        let ExcepthandlerKind::ExceptHandler { name, body, .. } = &handler.node;
+        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;
+            };
+            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),
+                            ));
+                        }
+                    }
+                }
+            }
+        }
+    }
+}