From 2f90157ce2582cf9e547f2c4c83c0cad84e40c29 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 31 Mar 2023 23:50:44 -0400 Subject: [PATCH] Move logging resolver into `logging.rs` (#3843) --- .../src/rules/flake8_blind_except/rules.rs | 4 +-- .../src/rules/flake8_logging_format/rules.rs | 16 +++++----- crates/ruff/src/rules/pylint/rules/logging.rs | 8 ++--- crates/ruff/src/rules/tryceratops/helpers.rs | 5 ++-- crates/ruff_python_ast/src/helpers.rs | 26 ---------------- crates/ruff_python_ast/src/logging.rs | 30 +++++++++++++++++++ 6 files changed, 46 insertions(+), 43 deletions(-) diff --git a/crates/ruff/src/rules/flake8_blind_except/rules.rs b/crates/ruff/src/rules/flake8_blind_except/rules.rs index 68a1de97ca..15b5fa62ba 100644 --- a/crates/ruff/src/rules/flake8_blind_except/rules.rs +++ b/crates/ruff/src/rules/flake8_blind_except/rules.rs @@ -2,8 +2,8 @@ use rustpython_parser::ast::{Expr, ExprKind, Stmt, StmtKind}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers; use ruff_python_ast::helpers::{find_keyword, is_const_true}; +use ruff_python_ast::logging; use ruff_python_ast::types::Range; use crate::checkers::ast::Checker; @@ -59,7 +59,7 @@ pub fn blind_except( if body.iter().any(|stmt| { if let StmtKind::Expr { value } = &stmt.node { if let ExprKind::Call { func, keywords, .. } = &value.node { - if helpers::is_logger_candidate(&checker.ctx, func) { + if logging::is_logger_candidate(&checker.ctx, func) { if let ExprKind::Attribute { attr, .. } = &func.node { if attr == "exception" { return true; diff --git a/crates/ruff/src/rules/flake8_logging_format/rules.rs b/crates/ruff/src/rules/flake8_logging_format/rules.rs index b2b9557351..eafdf5d25b 100644 --- a/crates/ruff/src/rules/flake8_logging_format/rules.rs +++ b/crates/ruff/src/rules/flake8_logging_format/rules.rs @@ -1,8 +1,8 @@ use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword, Location, Operator}; use ruff_diagnostics::{Diagnostic, Edit}; -use ruff_python_ast::helpers::{find_keyword, is_logger_candidate, SimpleCallArgs}; -use ruff_python_ast::logging::LoggingLevel; +use ruff_python_ast::helpers::{find_keyword, SimpleCallArgs}; +use ruff_python_ast::logging; use ruff_python_ast::types::Range; use crate::checkers::ast::Checker; @@ -128,7 +128,7 @@ fn check_log_record_attr_clash(checker: &mut Checker, extra: &Keyword) { #[derive(Copy, Clone)] enum LoggingCallType { /// Logging call with a level method, e.g., `logging.info`. - LevelCall(LoggingLevel), + LevelCall(logging::LoggingLevel), /// Logging call with an integer level as an argument, e.g., `logger.log(level, ...)`. LogCall, } @@ -138,14 +138,14 @@ impl LoggingCallType { if attr == "log" { Some(LoggingCallType::LogCall) } else { - LoggingLevel::from_attribute(attr).map(LoggingCallType::LevelCall) + logging::LoggingLevel::from_attribute(attr).map(LoggingCallType::LevelCall) } } } /// Check logging calls for violations. pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: &[Keyword]) { - if !is_logger_candidate(&checker.ctx, func) { + if !logging::is_logger_candidate(&checker.ctx, func) { return; } @@ -173,7 +173,7 @@ pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: if checker.settings.rules.enabled(Rule::LoggingWarn) && matches!( logging_call_type, - LoggingCallType::LevelCall(LoggingLevel::Warn) + LoggingCallType::LevelCall(logging::LoggingLevel::Warn) ) { let mut diagnostic = Diagnostic::new(LoggingWarn, level_call_range); @@ -228,14 +228,14 @@ pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: if let LoggingCallType::LevelCall(logging_level) = logging_call_type { match logging_level { - LoggingLevel::Error => { + logging::LoggingLevel::Error => { if checker.settings.rules.enabled(Rule::LoggingExcInfo) { checker .diagnostics .push(Diagnostic::new(LoggingExcInfo, level_call_range)); } } - LoggingLevel::Exception => { + logging::LoggingLevel::Exception => { if checker .settings .rules diff --git a/crates/ruff/src/rules/pylint/rules/logging.rs b/crates/ruff/src/rules/pylint/rules/logging.rs index 838d2dce9b..f3634e58f1 100644 --- a/crates/ruff/src/rules/pylint/rules/logging.rs +++ b/crates/ruff/src/rules/pylint/rules/logging.rs @@ -2,8 +2,8 @@ use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::{is_logger_candidate, SimpleCallArgs}; -use ruff_python_ast::logging::LoggingLevel; +use ruff_python_ast::helpers::SimpleCallArgs; +use ruff_python_ast::logging; use ruff_python_ast::types::Range; use crate::checkers::ast::Checker; @@ -100,12 +100,12 @@ pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: return; } - if !is_logger_candidate(&checker.ctx, func) { + if !logging::is_logger_candidate(&checker.ctx, func) { return; } if let ExprKind::Attribute { attr, .. } = &func.node { - if LoggingLevel::from_attribute(attr.as_str()).is_some() { + if logging::LoggingLevel::from_attribute(attr.as_str()).is_some() { let call_args = SimpleCallArgs::new(args, keywords); if let Some(msg) = call_args.argument("msg", 0) { if let ExprKind::Constant { diff --git a/crates/ruff/src/rules/tryceratops/helpers.rs b/crates/ruff/src/rules/tryceratops/helpers.rs index a1d95af17a..16a4d4aab1 100644 --- a/crates/ruff/src/rules/tryceratops/helpers.rs +++ b/crates/ruff/src/rules/tryceratops/helpers.rs @@ -1,9 +1,8 @@ use rustpython_parser::ast::{Expr, ExprKind}; use ruff_python_ast::context::Context; -use ruff_python_ast::helpers::is_logger_candidate; -use ruff_python_ast::visitor; use ruff_python_ast::visitor::Visitor; +use ruff_python_ast::{logging, visitor}; /// Collect `logging`-like calls from an AST. pub struct LoggerCandidateVisitor<'a> { @@ -26,7 +25,7 @@ where { fn visit_expr(&mut self, expr: &'b Expr) { if let ExprKind::Call { func, .. } = &expr.node { - if is_logger_candidate(self.context, func) { + if logging::is_logger_candidate(self.context, func) { self.calls.push((expr, func)); } } diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 0e9f9e1409..3da832a020 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1237,32 +1237,6 @@ impl<'a> SimpleCallArgs<'a> { } } -/// Return `true` if the given `Expr` is a potential logging call. Matches -/// `logging.error`, `logger.error`, `self.logger.error`, etc., but not -/// arbitrary `foo.error` calls. -/// -/// It even matches direct `logging.error` calls even if the `logging` module -/// is aliased. Example: -/// ```python -/// import logging as bar -/// -/// # This is detected to be a logger candidate -/// bar.error() -/// ``` -pub fn is_logger_candidate(context: &Context, func: &Expr) -> bool { - if let ExprKind::Attribute { value, .. } = &func.node { - let call_path = context - .resolve_call_path(value) - .unwrap_or_else(|| collect_call_path(value)); - if let Some(tail) = call_path.last() { - if tail.starts_with("log") || tail.ends_with("logger") || tail.ends_with("logging") { - return true; - } - } - } - false -} - /// Check if a node is parent of a conditional branch. pub fn on_conditional_branch<'a>(parents: &mut impl Iterator) -> bool { parents.any(|parent| { diff --git a/crates/ruff_python_ast/src/logging.rs b/crates/ruff_python_ast/src/logging.rs index 262132967d..7d3d57383b 100644 --- a/crates/ruff_python_ast/src/logging.rs +++ b/crates/ruff_python_ast/src/logging.rs @@ -1,3 +1,7 @@ +use crate::context::Context; +use crate::helpers::collect_call_path; +use rustpython_parser::ast::{Expr, ExprKind}; + #[derive(Copy, Clone)] pub enum LoggingLevel { Debug, @@ -23,3 +27,29 @@ impl LoggingLevel { } } } + +/// Return `true` if the given `Expr` is a potential logging call. Matches +/// `logging.error`, `logger.error`, `self.logger.error`, etc., but not +/// arbitrary `foo.error` calls. +/// +/// It even matches direct `logging.error` calls even if the `logging` module +/// is aliased. Example: +/// ```python +/// import logging as bar +/// +/// # This is detected to be a logger candidate +/// bar.error() +/// ``` +pub fn is_logger_candidate(context: &Context, func: &Expr) -> bool { + if let ExprKind::Attribute { value, .. } = &func.node { + let call_path = context + .resolve_call_path(value) + .unwrap_or_else(|| collect_call_path(value)); + if let Some(tail) = call_path.last() { + if tail.starts_with("log") || tail.ends_with("logger") || tail.ends_with("logging") { + return true; + } + } + } + false +}