From 211d8e170d2e0a212bca398e86e80937e75d14ed Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 2 Jun 2023 00:59:45 -0400 Subject: [PATCH] Ignore error calls with `exc_info` in TRY400 (#4797) --- .../test/fixtures/tryceratops/TRY400.py | 15 ++++ .../rules/logging_call.rs | 63 ++++++---------- crates/ruff/src/rules/tryceratops/helpers.rs | 14 ++-- .../rules/error_instead_of_exception.rs | 15 ++-- .../tryceratops/rules/verbose_log_message.rs | 32 +++------ ..._error-instead-of-exception_TRY400.py.snap | 72 +++++++++---------- .../src/analyze/logging.rs | 31 +++++++- 7 files changed, 129 insertions(+), 113 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/tryceratops/TRY400.py b/crates/ruff/resources/test/fixtures/tryceratops/TRY400.py index f8deb6ad9b..1c31197c26 100644 --- a/crates/ruff/resources/test/fixtures/tryceratops/TRY400.py +++ b/crates/ruff/resources/test/fixtures/tryceratops/TRY400.py @@ -4,6 +4,7 @@ Use '.exception' over '.error' inside except blocks """ 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()) diff --git a/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs index 410eb43b84..dceee291b9 100644 --- a/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/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(), + )); + } + } + _ => {} } } } diff --git a/crates/ruff/src/rules/tryceratops/helpers.rs b/crates/ruff/src/rules/tryceratops/helpers.rs index b0a1cdb992..3517f33e2b 100644 --- a/crates/ruff/src/rules/tryceratops/helpers.rs +++ b/crates/ruff/src/rules/tryceratops/helpers.rs @@ -7,14 +7,14 @@ 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(), } } @@ -22,9 +22,9 @@ impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> { 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); 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 ed27a6f10d..09af41b601 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 @@ -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())); + } } } } 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 46d4d7465e..dd1dd7d34c 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::{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())); diff --git a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap b/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap index b02e9153f6..3824e75fdd 100644 --- a/crates/ruff/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__error-instead-of-exception_TRY400.py.snap +++ b/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 | diff --git a/crates/ruff_python_semantic/src/analyze/logging.rs b/crates/ruff_python_semantic/src/analyze/logging.rs index d16ed0425d..9dd4079ed6 100644 --- a/crates/ruff_python_semantic/src/analyze/logging.rs +++ b/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 +}