diff --git a/crates/ruff/resources/test/fixtures/tryceratops/TRY401.py b/crates/ruff/resources/test/fixtures/tryceratops/TRY401.py index 84f9d922353b8..b96661a68ba5c 100644 --- a/crates/ruff/resources/test/fixtures/tryceratops/TRY401.py +++ b/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,15 @@ 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 + + if True: + logger.exception(f"Found an error: {bad}") # TRY401 import logging @@ -33,21 +36,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 +59,6 @@ def main_function(): process() handle() finish() - except Exception as ex: # noqa: F841 + except Exception as ex: logger.exception(f"Found an error: {er}") + logger.exception(f"Found an error: {ex.field}") diff --git a/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs b/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs index 2cee57583639a..30741da9ce0e6 100644 --- a/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs +++ b/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,13 +39,13 @@ 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> @@ -51,22 +53,13 @@ where 'b: 'a, { fn visit_expr(&mut self, expr: &'b Expr) { - if let ExprKind::Name { .. } = &expr.node { - self.names.push(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), - )); - } + match &expr.node { + ExprKind::Name { + id, + ctx: ExprContext::Load, + } => self.names.push((id, expr)), + ExprKind::Attribute { .. } => {} + _ => visitor::walk_expr(self, expr), } } } @@ -75,23 +68,39 @@ fn check_names(checker: &mut Checker, exprs: &[&Expr], target: &str) { 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), + )); } } } diff --git a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-log-message_TRY401.py.snap b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-log-message_TRY401.py.snap index 8525fd78f5f58..6d5266790ffc9 100644 --- a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-log-message_TRY401.py.snap +++ b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-log-message_TRY401.py.snap @@ -26,70 +26,80 @@ 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 + row: 27 + column: 48 end_location: - row: 36 - column: 42 + row: 27 + column: 51 fix: ~ parent: ~ - kind: VerboseLogMessage: ~ location: - row: 43 - column: 47 + row: 39 + column: 46 + end_location: + row: 39 + column: 48 + fix: ~ + parent: ~ +- kind: + VerboseLogMessage: ~ + location: + row: 46 + column: 52 end_location: - row: 43 - column: 49 + row: 46 + column: 54 fix: ~ parent: ~ - kind: VerboseLogMessage: ~ location: - row: 50 - column: 40 + row: 53 + column: 46 end_location: - row: 50 - column: 42 + row: 53 + column: 48 fix: ~ parent: ~