From 63adf9f5e84722503de656b3f10e2d995b12340e Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Sat, 25 Mar 2023 02:49:09 +0530 Subject: [PATCH] Allow aliased `logging` module as a logger candidate (#3718) --- .../test/fixtures/flake8_logging_format/G001.py | 2 ++ .../ruff/src/rules/flake8_blind_except/rules.rs | 2 +- .../src/rules/flake8_logging_format/rules.rs | 2 +- ...__flake8_logging_format__tests__G001.py.snap | 17 +++++++++++++++-- crates/ruff/src/rules/pylint/rules/logging.rs | 2 +- crates/ruff/src/rules/tryceratops/helpers.rs | 14 ++++++++++++-- .../rules/error_instead_of_exception.rs | 2 +- .../tryceratops/rules/verbose_log_message.rs | 2 +- crates/ruff_python_ast/src/helpers.rs | 15 +++++++++++++-- 9 files changed, 47 insertions(+), 11 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_logging_format/G001.py b/crates/ruff/resources/test/fixtures/flake8_logging_format/G001.py index 2c3e0bc7d2..016afa5fcd 100644 --- a/crates/ruff/resources/test/fixtures/flake8_logging_format/G001.py +++ b/crates/ruff/resources/test/fixtures/flake8_logging_format/G001.py @@ -1,3 +1,5 @@ import logging +import logging as foo logging.info("Hello {}".format("World!")) +foo.info("Hello {}".format("World!")) diff --git a/crates/ruff/src/rules/flake8_blind_except/rules.rs b/crates/ruff/src/rules/flake8_blind_except/rules.rs index 98b1aab7e7..68a1de97ca 100644 --- a/crates/ruff/src/rules/flake8_blind_except/rules.rs +++ b/crates/ruff/src/rules/flake8_blind_except/rules.rs @@ -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(func) { + if helpers::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 8c98f439fe..8ca2080d2f 100644 --- a/crates/ruff/src/rules/flake8_logging_format/rules.rs +++ b/crates/ruff/src/rules/flake8_logging_format/rules.rs @@ -127,7 +127,7 @@ fn check_log_record_attr_clash(checker: &mut Checker, extra: &Keyword) { /// Check logging calls for violations. pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: &[Keyword]) { - if !is_logger_candidate(func) { + if !is_logger_candidate(&checker.ctx, func) { return; } diff --git a/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G001.py.snap b/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G001.py.snap index cb945fa72f..6661949b98 100644 --- a/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G001.py.snap +++ b/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G001.py.snap @@ -8,11 +8,24 @@ expression: diagnostics suggestion: ~ fixable: false location: - row: 3 + row: 4 column: 13 end_location: - row: 3 + row: 4 column: 40 fix: ~ parent: ~ +- kind: + name: LoggingStringFormat + body: "Logging statement uses `string.format()`" + suggestion: ~ + fixable: false + location: + row: 5 + column: 9 + end_location: + row: 5 + column: 36 + fix: ~ + parent: ~ diff --git a/crates/ruff/src/rules/pylint/rules/logging.rs b/crates/ruff/src/rules/pylint/rules/logging.rs index 7b18a65de9..838d2dce9b 100644 --- a/crates/ruff/src/rules/pylint/rules/logging.rs +++ b/crates/ruff/src/rules/pylint/rules/logging.rs @@ -100,7 +100,7 @@ pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: return; } - if !is_logger_candidate(func) { + if !is_logger_candidate(&checker.ctx, func) { return; } diff --git a/crates/ruff/src/rules/tryceratops/helpers.rs b/crates/ruff/src/rules/tryceratops/helpers.rs index 2bd35149eb..a1d95af17a 100644 --- a/crates/ruff/src/rules/tryceratops/helpers.rs +++ b/crates/ruff/src/rules/tryceratops/helpers.rs @@ -1,22 +1,32 @@ 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; -#[derive(Default)] /// Collect `logging`-like calls from an AST. pub struct LoggerCandidateVisitor<'a> { + context: &'a Context<'a>, pub calls: Vec<(&'a Expr, &'a Expr)>, } +impl<'a> LoggerCandidateVisitor<'a> { + pub fn new(context: &'a Context<'a>) -> Self { + LoggerCandidateVisitor { + context, + calls: Vec::new(), + } + } +} + 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) { + if is_logger_candidate(self.context, func) { self.calls.push((expr, func)); } } 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 006e6f4bc1..97b3e1537a 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 @@ -23,7 +23,7 @@ pub fn error_instead_of_exception(checker: &mut Checker, handlers: &[Excepthandl for handler in handlers { let ExcepthandlerKind::ExceptHandler { body, .. } = &handler.node; let calls = { - let mut visitor = LoggerCandidateVisitor::default(); + let mut visitor = LoggerCandidateVisitor::new(&checker.ctx); visitor.visit_body(body); visitor.calls }; 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 f042fff03b..0b57cdd1a4 100644 --- a/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs +++ b/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs @@ -73,7 +73,7 @@ pub fn verbose_log_message(checker: &mut Checker, handlers: &[Excepthandler]) { // Find all calls to `logging.exception`. let calls = { - let mut visitor = LoggerCandidateVisitor::default(); + let mut visitor = LoggerCandidateVisitor::new(&checker.ctx); visitor.visit_body(body); visitor.calls }; diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index f14a1412b2..906e0aa6db 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1288,9 +1288,20 @@ 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. -pub fn is_logger_candidate(func: &Expr) -> bool { +/// +/// 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 = collect_call_path(value); + 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;