Ignore error calls with exc_info in TRY400

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

@@ -4,6 +4,7 @@
 """
 
 import logging
+import sys
 
 logger = logging.getLogger(__name__)
 
@@ -60,3 +61,17 @@ def good():
         a = 1
     except Exception:
         foo.exception("Context message here")
+
+
+def fine():
+    try:
+        a = 1
+    except Exception:
+        logger.error("Context message here", exc_info=True)
+
+
+def fine():
+    try:
+        a = 1
+    except Exception:
+        logger.error("Context message here", exc_info=sys.exc_info())

crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs

@@ -4,6 +4,7 @@ use rustpython_parser::ast::{self, Constant, Expr, Keyword, Operator, Ranged};
 use ruff_diagnostics::{Diagnostic, Edit, Fix};
 use ruff_python_ast::helpers::{find_keyword, SimpleCallArgs};
 use ruff_python_semantic::analyze::logging;
+use ruff_python_semantic::analyze::logging::exc_info;
 use ruff_python_stdlib::logging::LoggingLevel;
 
 use crate::checkers::ast::Checker;
@@ -192,53 +193,31 @@ pub(crate) fn logging_call(
             }
 
             // G201, G202
-            if checker.enabled(Rule::LoggingExcInfo)
-                || checker.enabled(Rule::LoggingRedundantExcInfo)
-            {
+            if checker.any_enabled(&[Rule::LoggingExcInfo, Rule::LoggingRedundantExcInfo]) {
                 if !checker.semantic_model().in_exception_handler() {
                     return;
                 }
-                if let Some(exc_info) = find_keyword(keywords, "exc_info") {
-                    // If `exc_info` is `True` or `sys.exc_info()`, it's redundant; but otherwise,
-                    // return.
-                    if !(matches!(
-                        exc_info.value,
-                        Expr::Constant(ast::ExprConstant {
-                            value: Constant::Bool(true),
-                            ..
-                        })
-                    ) || if let Expr::Call(ast::ExprCall { func, .. }) = &exc_info.value {
-                        checker
-                            .semantic_model()
-                            .resolve_call_path(func)
-                            .map_or(false, |call_path| {
-                                call_path.as_slice() == ["sys", "exc_info"]
-                            })
-                    } else {
-                        false
-                    }) {
-                        return;
-                    }
-
-                    if let LoggingCallType::LevelCall(logging_level) = logging_call_type {
-                        match logging_level {
-                            LoggingLevel::Error => {
-                                if checker.enabled(Rule::LoggingExcInfo) {
-                                    checker
-                                        .diagnostics
-                                        .push(Diagnostic::new(LoggingExcInfo, level_call_range));
-                                }
+                let Some(exc_info) = exc_info(keywords, checker.semantic_model()) else {
+                    return;
+                };
+                if let LoggingCallType::LevelCall(logging_level) = logging_call_type {
+                    match logging_level {
+                        LoggingLevel::Error => {
+                            if checker.enabled(Rule::LoggingExcInfo) {
+                                checker
+                                    .diagnostics
+                                    .push(Diagnostic::new(LoggingExcInfo, level_call_range));
                             }
-                            LoggingLevel::Exception => {
-                                if checker.enabled(Rule::LoggingRedundantExcInfo) {
-                                    checker.diagnostics.push(Diagnostic::new(
-                                        LoggingRedundantExcInfo,
-                                        exc_info.range(),
-                                    ));
-                                }
+                        }
+                        LoggingLevel::Exception => {
+                            if checker.enabled(Rule::LoggingRedundantExcInfo) {
+                                checker.diagnostics.push(Diagnostic::new(
+                                    LoggingRedundantExcInfo,
+                                    exc_info.range(),
+                                ));
                             }
-                            _ => {}
                         }
+                        _ => {}
                     }
                 }
             }

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

@@ -7,24 +7,24 @@ use ruff_python_semantic::model::SemanticModel;
 
 /// Collect `logging`-like calls from an AST.
 pub(crate) struct LoggerCandidateVisitor<'a, 'b> {
-    context: &'a SemanticModel<'b>,
-    pub(crate) calls: Vec<(&'b Expr, &'b Expr)>,
+    semantic_model: &'a SemanticModel<'b>,
+    pub(crate) calls: Vec<&'b ast::ExprCall>,
 }
 
 impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> {
-    pub(crate) fn new(context: &'a SemanticModel<'b>) -> Self {
+    pub(crate) fn new(semantic_model: &'a SemanticModel<'b>) -> Self {
         LoggerCandidateVisitor {
-            context,
+            semantic_model,
             calls: Vec::new(),
         }
     }
 }
 
 impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a, 'b> {
     fn visit_expr(&mut self, expr: &'b Expr) {
-        if let Expr::Call(ast::ExprCall { func, .. }) = expr {
-            if logging::is_logger_candidate(func, self.context) {
-                self.calls.push((expr, func));
+        if let Expr::Call(call) = expr {
+            if logging::is_logger_candidate(&call.func, self.semantic_model) {
+                self.calls.push(call);
             }
         }
         visitor::walk_expr(self, expr);

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

@@ -3,6 +3,7 @@ use rustpython_parser::ast::{self, Excepthandler, Expr, Ranged};
 use ruff_diagnostics::{Diagnostic, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::visitor::Visitor;
+use ruff_python_semantic::analyze::logging::exc_info;
 
 use crate::checkers::ast::Checker;
 use crate::rules::tryceratops::helpers::LoggerCandidateVisitor;
@@ -41,7 +42,7 @@ use crate::rules::tryceratops::helpers::LoggerCandidateVisitor;
 /// ```
 ///
 /// ## References
-/// - [Python documentation](https://docs.python.org/3/library/logging.html#logging.exception)
+/// - [Python documentation: `logging.exception`](https://docs.python.org/3/library/logging.html#logging.exception)
 #[violation]
 pub struct ErrorInsteadOfException;
 
@@ -61,12 +62,14 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce
             visitor.visit_body(body);
             visitor.calls
         };
-        for (expr, func) in calls {
-            if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func {
+        for expr in calls {
+            if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = expr.func.as_ref() {
                 if attr == "error" {
-                    checker
-                        .diagnostics
-                        .push(Diagnostic::new(ErrorInsteadOfException, expr.range()));
+                    if exc_info(&expr.keywords, checker.semantic_model()).is_none() {
+                        checker
+                            .diagnostics
+                            .push(Diagnostic::new(ErrorInsteadOfException, expr.range()));
+                    }
                 }
             }
         }

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

@@ -1,4 +1,4 @@
-use rustpython_parser::ast::{self, Excepthandler, Expr, ExprContext, Ranged};
+use rustpython_parser::ast::{self, Excepthandler, Expr, Ranged};
 
 use ruff_diagnostics::{Diagnostic, Violation};
 use ruff_macros::{derive_message_formats, violation};
@@ -43,20 +43,13 @@ impl Violation for VerboseLogMessage {
 
 #[derive(Default)]
 struct NameVisitor<'a> {
-    names: Vec<(&'a str, &'a Expr)>,
+    names: Vec<&'a ast::ExprName>,
 }
 
-impl<'a, 'b> Visitor<'b> for NameVisitor<'a>
-where
-    'b: 'a,
-{
-    fn visit_expr(&mut self, expr: &'b Expr) {
+impl<'a> Visitor<'a> for NameVisitor<'a> {
+    fn visit_expr(&mut self, expr: &'a Expr) {
         match expr {
-            Expr::Name(ast::ExprName {
-                id,
-                ctx: ExprContext::Load,
-                range: _,
-            }) => self.names.push((id, expr)),
+            Expr::Name(name) if name.ctx.is_load() => self.names.push(name),
             Expr::Attribute(_) => {}
             _ => visitor::walk_expr(self, expr),
         }
@@ -79,24 +72,21 @@ pub(crate) fn verbose_log_message(checker: &mut Checker, handlers: &[Excepthandl
             visitor.calls
         };
 
-        for (expr, func) in calls {
-            let Expr::Call(ast::ExprCall { args, .. }) = expr else {
-                continue;
-            };
-            if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func {
+        for expr in calls {
+            if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = expr.func.as_ref() {
                 if attr == "exception" {
                     // Collect all referenced names in the `logging.exception` call.
-                    let names: Vec<(&str, &Expr)> = {
+                    let names: Vec<&ast::ExprName> = {
                         let mut names = Vec::new();
-                        for arg in args {
+                        for arg in &expr.args {
                             let mut visitor = NameVisitor::default();
                             visitor.visit_expr(arg);
                             names.extend(visitor.names);
                         }
                         names
                     };
-                    for (id, expr) in names {
-                        if target == id {
+                    for expr in names {
+                        if expr.id == *target {
                             checker
                                 .diagnostics
                                 .push(Diagnostic::new(VerboseLogMessage, expr.range()));

crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap

@@ -1,71 +1,71 @@
 ---
 source: crates/ruff/src/rules/tryceratops/mod.rs
 ---
-TRY400.py:15:9: TRY400 Use `logging.exception` instead of `logging.error`
+TRY400.py:16:9: TRY400 Use `logging.exception` instead of `logging.error`
    |
-15 |         a = 1
-16 |     except Exception:
-17 |         logging.error("Context message here")
+16 |         a = 1
+17 |     except Exception:
+18 |         logging.error("Context message here")
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
-18 | 
-19 |         if True:
+19 | 
+20 |         if True:
    |
 
-TRY400.py:18:13: TRY400 Use `logging.exception` instead of `logging.error`
+TRY400.py:19:13: TRY400 Use `logging.exception` instead of `logging.error`
    |
-18 |         if True:
-19 |             logging.error("Context message here")
+19 |         if True:
+20 |             logging.error("Context message here")
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
    |
 
-TRY400.py:25:9: TRY400 Use `logging.exception` instead of `logging.error`
+TRY400.py:26:9: TRY400 Use `logging.exception` instead of `logging.error`
    |
-25 |         a = 1
-26 |     except Exception:
-27 |         logger.error("Context message here")
+26 |         a = 1
+27 |     except Exception:
+28 |         logger.error("Context message here")
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
-28 | 
-29 |         if True:
+29 | 
+30 |         if True:
    |
 
-TRY400.py:28:13: TRY400 Use `logging.exception` instead of `logging.error`
+TRY400.py:29:13: TRY400 Use `logging.exception` instead of `logging.error`
    |
-28 |         if True:
-29 |             logger.error("Context message here")
+29 |         if True:
+30 |             logger.error("Context message here")
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
    |
 
-TRY400.py:35:9: TRY400 Use `logging.exception` instead of `logging.error`
+TRY400.py:36:9: TRY400 Use `logging.exception` instead of `logging.error`
    |
-35 |         a = 1
-36 |     except Exception:
-37 |         log.error("Context message here")
+36 |         a = 1
+37 |     except Exception:
+38 |         log.error("Context message here")
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
-38 | 
-39 |         if True:
+39 | 
+40 |         if True:
    |
 
-TRY400.py:38:13: TRY400 Use `logging.exception` instead of `logging.error`
+TRY400.py:39:13: TRY400 Use `logging.exception` instead of `logging.error`
    |
-38 |         if True:
-39 |             log.error("Context message here")
+39 |         if True:
+40 |             log.error("Context message here")
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
    |
 
-TRY400.py:45:9: TRY400 Use `logging.exception` instead of `logging.error`
+TRY400.py:46:9: TRY400 Use `logging.exception` instead of `logging.error`
    |
-45 |         a = 1
-46 |     except Exception:
-47 |         self.logger.error("Context message here")
+46 |         a = 1
+47 |     except Exception:
+48 |         self.logger.error("Context message here")
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
-48 | 
-49 |         if True:
+49 | 
+50 |         if True:
    |
 
-TRY400.py:48:13: TRY400 Use `logging.exception` instead of `logging.error`
+TRY400.py:49:13: TRY400 Use `logging.exception` instead of `logging.error`
    |
-48 |         if True:
-49 |             self.logger.error("Context message here")
+49 |         if True:
+50 |             self.logger.error("Context message here")
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TRY400
    |
 

crates/ruff_python_semantic/src/analyze/logging.rs

@@ -1,6 +1,7 @@
-use rustpython_parser::ast::{self, Expr};
+use rustpython_parser::ast::{self, Constant, Expr, Keyword};
 
 use ruff_python_ast::call_path::collect_call_path;
+use ruff_python_ast::helpers::find_keyword;
 
 use crate::model::SemanticModel;
 
@@ -37,3 +38,31 @@ pub fn is_logger_candidate(func: &Expr, model: &SemanticModel) -> bool {
     }
     false
 }
+
+/// If the keywords to a  logging call contain `exc_info=True` or `exc_info=sys.exc_info()`,
+/// return the `Keyword` for `exc_info`.
+pub fn exc_info<'a>(keywords: &'a [Keyword], model: &SemanticModel) -> Option<&'a Keyword> {
+    let exc_info = find_keyword(keywords, "exc_info")?;
+
+    // Ex) `logging.error("...", exc_info=True)`
+    if matches!(
+        exc_info.value,
+        Expr::Constant(ast::ExprConstant {
+            value: Constant::Bool(true),
+            ..
+        })
+    ) {
+        return Some(exc_info);
+    }
+
+    // Ex) `logging.error("...", exc_info=sys.exc_info())`
+    if let Expr::Call(ast::ExprCall { func, .. }) = &exc_info.value {
+        if model.resolve_call_path(func).map_or(false, |call_path| {
+            call_path.as_slice() == ["sys", "exc_info"]
+        }) {
+            return Some(exc_info);
+        }
+    }
+
+    None
+}