diff --git a/crates/ruff/resources/test/fixtures/tryceratops/TRY401.py b/crates/ruff/resources/test/fixtures/tryceratops/TRY401.py new file mode 100644 index 0000000000000..b96661a68ba5c --- /dev/null +++ b/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}") diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 8fc7d416aa325..e4841b02f1851 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/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); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index eedaf5cc42e7a..186a9a39b5d92 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -545,6 +545,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Tryceratops, "300") => Rule::TryConsiderElse, (Tryceratops, "301") => Rule::RaiseWithinTry, (Tryceratops, "400") => Rule::ErrorInsteadOfException, + (Tryceratops, "401") => Rule::VerboseLogMessage, // flake8-use-pathlib (Flake8UsePathlib, "100") => Rule::PathlibAbspath, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 99c5ef10f099e..0b64ce4690318 100644 --- a/crates/ruff/src/registry.rs +++ b/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, diff --git a/crates/ruff/src/rules/tryceratops/helpers.rs b/crates/ruff/src/rules/tryceratops/helpers.rs new file mode 100644 index 0000000000000..d35556a15194e --- /dev/null +++ b/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); + } +} diff --git a/crates/ruff/src/rules/tryceratops/mod.rs b/crates/ruff/src/rules/tryceratops/mod.rs index ad996f3de6391..a3b5b2bc6080e 100644 --- a/crates/ruff/src/rules/tryceratops/mod.rs +++ b/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( diff --git a/crates/ruff/src/rules/tryceratops/rules/error_instead_of_exception.rs b/crates/ruff/src/rules/tryceratops/rules/error_instead_of_exception.rs index 62b0c81f9c4a5..882b4dea59715 100644 --- a/crates/ruff/src/rules/tryceratops/rules/error_instead_of_exception.rs +++ b/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 { diff --git a/crates/ruff/src/rules/tryceratops/rules/mod.rs b/crates/ruff/src/rules/tryceratops/rules/mod.rs index 9791648aed160..1438e46c9ea01 100644 --- a/crates/ruff/src/rules/tryceratops/rules/mod.rs +++ b/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; diff --git a/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs b/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs new file mode 100644 index 0000000000000..30741da9ce0e6 --- /dev/null +++ b/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), + )); + } + } + } + } + } + } +} 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 new file mode 100644 index 0000000000000..6d5266790ffc9 --- /dev/null +++ b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__verbose-log-message_TRY401.py.snap @@ -0,0 +1,105 @@ +--- +source: crates/ruff/src/rules/tryceratops/mod.rs +expression: diagnostics +--- +- kind: + VerboseLogMessage: ~ + location: + row: 8 + column: 44 + end_location: + row: 8 + column: 46 + fix: ~ + parent: ~ +- kind: + VerboseLogMessage: ~ + location: + row: 19 + column: 52 + end_location: + row: 19 + column: 55 + fix: ~ + parent: ~ +- kind: + VerboseLogMessage: ~ + location: + row: 21 + column: 44 + end_location: + row: 21 + column: 47 + fix: ~ + parent: ~ +- kind: + VerboseLogMessage: ~ + location: + row: 21 + column: 50 + end_location: + row: 21 + column: 53 + fix: ~ + parent: ~ +- kind: + VerboseLogMessage: ~ + location: + row: 23 + column: 44 + end_location: + row: 23 + column: 47 + fix: ~ + parent: ~ +- kind: + VerboseLogMessage: ~ + location: + row: 24 + column: 44 + end_location: + row: 24 + column: 47 + fix: ~ + parent: ~ +- kind: + VerboseLogMessage: ~ + location: + row: 27 + column: 48 + end_location: + row: 27 + column: 51 + fix: ~ + parent: ~ +- kind: + VerboseLogMessage: ~ + location: + row: 39 + column: 46 + end_location: + row: 39 + column: 48 + fix: ~ + parent: ~ +- kind: + VerboseLogMessage: ~ + location: + row: 46 + column: 52 + end_location: + row: 46 + column: 54 + fix: ~ + parent: ~ +- kind: + VerboseLogMessage: ~ + location: + row: 53 + column: 46 + end_location: + row: 53 + column: 48 + fix: ~ + parent: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index c9acae28af678..c926073cf97c2 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2042,6 +2042,7 @@ "TRY4", "TRY40", "TRY400", + "TRY401", "UP", "UP0", "UP00",