From c8360a13336cd87fee8f1a8c7845e00ea90d6cca Mon Sep 17 00:00:00 2001 From: qdegraaf <34540841+qdegraaf@users.noreply.github.com> Date: Wed, 27 Sep 2023 17:38:52 +0200 Subject: [PATCH] Expand `DeprecatedLogWarn` to check for `Expr::Atrribute` calls (#7677) ## Summary `PGH002`, which checks for use of deprecated `logging.warn` calls, did not check for calls made on the attribute `warn` yet. Since https://github.com/astral-sh/ruff/pull/7521 we check both cases for similar rules wherever possible. To be consistent this PR expands PGH002 to do the same. ## Test Plan Expanded existing fixtures with `logger.warn()` calls ## Issue links Fixes final inconsistency mentioned in https://github.com/astral-sh/ruff/issues/7502 --- .../test/fixtures/pygrep_hooks/PGH002_0.py | 3 ++ .../test/fixtures/pygrep_hooks/PGH002_1.py | 3 ++ .../src/checkers/ast/analyze/expression.rs | 2 +- .../pygrep_hooks/rules/deprecated_log_warn.rs | 44 ++++++++++++++----- ...grep_hooks__tests__PGH002_PGH002_1.py.snap | 9 ++++ 5 files changed, 50 insertions(+), 11 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH002_0.py b/crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH002_0.py index 523b65bf41c56..d1bc313fc821d 100644 --- a/crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH002_0.py +++ b/crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH002_0.py @@ -5,3 +5,6 @@ warnings.warn("this is ok") warn("by itself is also ok") logging.warning("this is fine") + +logger = logging.getLogger(__name__) +logger.warning("this is fine") diff --git a/crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH002_1.py b/crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH002_1.py index c2866d0969fd9..8ff160407c6fa 100644 --- a/crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH002_1.py +++ b/crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH002_1.py @@ -3,3 +3,6 @@ logging.warn("this is not ok") warn("not ok") + +logger = logging.getLogger(__name__) +logger.warn("this is not ok") diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 5106274653a2b..77185c3c44231 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -746,7 +746,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { pygrep_hooks::rules::no_eval(checker, func); } if checker.enabled(Rule::DeprecatedLogWarn) { - pygrep_hooks::rules::deprecated_log_warn(checker, func); + pygrep_hooks::rules::deprecated_log_warn(checker, call); } if checker.enabled(Rule::UnnecessaryDirectLambdaCall) { pylint::rules::unnecessary_direct_lambda_call(checker, expr, func); diff --git a/crates/ruff_linter/src/rules/pygrep_hooks/rules/deprecated_log_warn.rs b/crates/ruff_linter/src/rules/pygrep_hooks/rules/deprecated_log_warn.rs index c91566f365532..547f1aa9f79c5 100644 --- a/crates/ruff_linter/src/rules/pygrep_hooks/rules/deprecated_log_warn.rs +++ b/crates/ruff_linter/src/rules/pygrep_hooks/rules/deprecated_log_warn.rs @@ -1,7 +1,9 @@ -use ruff_python_ast::Expr; +use ruff_python_ast::{self as ast, Expr, ExprCall}; +use ruff_python_semantic::analyze::logging; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_stdlib::logging::LoggingLevel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -43,14 +45,36 @@ impl Violation for DeprecatedLogWarn { } /// PGH002 -pub(crate) fn deprecated_log_warn(checker: &mut Checker, func: &Expr) { - if checker - .semantic() - .resolve_call_path(func) - .is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "warn"])) - { - checker - .diagnostics - .push(Diagnostic::new(DeprecatedLogWarn, func.range())); +pub(crate) fn deprecated_log_warn(checker: &mut Checker, call: &ExprCall) { + match call.func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => { + if !logging::is_logger_candidate( + &call.func, + checker.semantic(), + &checker.settings.logger_objects, + ) { + return; + } + if !matches!( + LoggingLevel::from_attribute(attr.as_str()), + Some(LoggingLevel::Warn) + ) { + return; + } + } + Expr::Name(_) => { + if !checker + .semantic() + .resolve_call_path(call.func.as_ref()) + .is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "warn"])) + { + return; + } + } + _ => return, } + + checker + .diagnostics + .push(Diagnostic::new(DeprecatedLogWarn, call.func.range())); } diff --git a/crates/ruff_linter/src/rules/pygrep_hooks/snapshots/ruff_linter__rules__pygrep_hooks__tests__PGH002_PGH002_1.py.snap b/crates/ruff_linter/src/rules/pygrep_hooks/snapshots/ruff_linter__rules__pygrep_hooks__tests__PGH002_PGH002_1.py.snap index 7c8d4bc82744a..df73d2858d097 100644 --- a/crates/ruff_linter/src/rules/pygrep_hooks/snapshots/ruff_linter__rules__pygrep_hooks__tests__PGH002_PGH002_1.py.snap +++ b/crates/ruff_linter/src/rules/pygrep_hooks/snapshots/ruff_linter__rules__pygrep_hooks__tests__PGH002_PGH002_1.py.snap @@ -15,6 +15,15 @@ PGH002_1.py:5:1: PGH002 `warn` is deprecated in favor of `warning` 4 | logging.warn("this is not ok") 5 | warn("not ok") | ^^^^ PGH002 +6 | +7 | logger = logging.getLogger(__name__) + | + +PGH002_1.py:8:1: PGH002 `warn` is deprecated in favor of `warning` + | +7 | logger = logging.getLogger(__name__) +8 | logger.warn("this is not ok") + | ^^^^^^^^^^^ PGH002 |