From 2b0538b4b323a9c6b5663acb49357a4f18328a7a Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Fri, 15 Sep 2023 10:11:54 +0530 Subject: [PATCH 1/2] [`flake8-logging`] Implement `LOG002`: `invalid-get-logger-argument` --- .../test/fixtures/flake8_logging/LOG002.py | 24 ++++++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/flake8_logging/mod.rs | 1 + .../rules/invalid_get_logger_argument.rs | 83 +++++++++++++++++++ .../src/rules/flake8_logging/rules/mod.rs | 2 + ...ake8_logging__tests__LOG002_LOG002.py.snap | 82 ++++++++++++++++++ crates/ruff_workspace/src/configuration.rs | 1 + ruff.schema.json | 1 + 9 files changed, 198 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_logging/LOG002.py create mode 100644 crates/ruff/src/rules/flake8_logging/rules/invalid_get_logger_argument.rs create mode 100644 crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG002_LOG002.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_logging/LOG002.py b/crates/ruff/resources/test/fixtures/flake8_logging/LOG002.py new file mode 100644 index 0000000000000..c567f7fe74010 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_logging/LOG002.py @@ -0,0 +1,24 @@ +import logging +from logging import getLogger + +# Ok +logging.getLogger(__name__) +logging.getLogger(name=__name__) +logging.getLogger("custom") +logging.getLogger(name="custom") + +# LOG002 +getLogger(__file__) +logging.getLogger(name=__file__) + +logging.getLogger(__cached__) +getLogger(name=__cached__) + + +# Override `logging.getLogger` +class logging: + def getLogger(self): + pass + + +logging.getLogger(__file__) diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index f7f0856c79831..f0b2a05fce4b8 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -895,6 +895,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::DirectLoggerInstantiation) { flake8_logging::rules::direct_logger_instantiation(checker, call); } + if checker.enabled(Rule::InvalidGetLoggerArgument) { + flake8_logging::rules::invalid_get_logger_argument(checker, call); + } } Expr::Dict(ast::ExprDict { keys, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 40129d51267da..df4c8a5f0b8f2 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -922,6 +922,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { // flake8-logging (Flake8Logging, "001") => (RuleGroup::Preview, rules::flake8_logging::rules::DirectLoggerInstantiation), + (Flake8Logging, "002") => (RuleGroup::Preview, rules::flake8_logging::rules::InvalidGetLoggerArgument), (Flake8Logging, "009") => (RuleGroup::Preview, rules::flake8_logging::rules::UndocumentedWarn), _ => return None, diff --git a/crates/ruff/src/rules/flake8_logging/mod.rs b/crates/ruff/src/rules/flake8_logging/mod.rs index b71cc64a8c46d..88ff0afccd51d 100644 --- a/crates/ruff/src/rules/flake8_logging/mod.rs +++ b/crates/ruff/src/rules/flake8_logging/mod.rs @@ -14,6 +14,7 @@ mod tests { use crate::test::test_path; #[test_case(Rule::DirectLoggerInstantiation, Path::new("LOG001.py"))] + #[test_case(Rule::InvalidGetLoggerArgument, Path::new("LOG002.py"))] #[test_case(Rule::UndocumentedWarn, Path::new("LOG009.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); diff --git a/crates/ruff/src/rules/flake8_logging/rules/invalid_get_logger_argument.rs b/crates/ruff/src/rules/flake8_logging/rules/invalid_get_logger_argument.rs new file mode 100644 index 0000000000000..10d6c3405fe5d --- /dev/null +++ b/crates/ruff/src/rules/flake8_logging/rules/invalid_get_logger_argument.rs @@ -0,0 +1,83 @@ +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; +use crate::registry::AsRule; + +/// ## What it does +/// Checks for any usage of `__cached__` and `__file__` as an argument to +/// `logging.getLogger()`. +/// +/// ## Why is this bad? +/// The [logging documentation] recommends this pattern: +/// +/// ```python +/// logging.getLogger(__name__) +/// ``` +/// +/// Here, `__name__` is the fully qualified module name, such as `foo.bar`, +/// which is the intended format for logger names. +/// +/// This rule detects probably-mistaken usage of similar module-level dunder constants: +/// +/// * `__cached__` - the pathname of the module's compiled version, such as `foo/__pycache__/bar.cpython-311.pyc`. +/// * `__file__` - the pathname of the module, such as `foo/bar.py`. +/// +/// ## Example +/// ```python +/// import logging +/// +/// logger = logging.getLogger(__file__) +/// ``` +/// +/// Use instead: +/// ```python +/// import logging +/// +/// logger = logging.getLogger(__name__) +/// ``` +/// +/// [logging documentation]: https://docs.python.org/3/library/logging.html#logger-objects +#[violation] +pub struct InvalidGetLoggerArgument; + +impl AlwaysAutofixableViolation for InvalidGetLoggerArgument { + #[derive_message_formats] + fn message(&self) -> String { + format!("Use `__name__` with `logging.getLogger()`") + } + + fn autofix_title(&self) -> String { + format!("Replace with `__name__`") + } +} + +/// LOG002 +pub(crate) fn invalid_get_logger_argument(checker: &mut Checker, call: &ast::ExprCall) { + if checker + .semantic() + .resolve_call_path(call.func.as_ref()) + .is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "getLogger"])) + { + let Some(Expr::Name(expr @ ast::ExprName { id, .. })) = + call.arguments.find_argument("name", 0) + else { + return; + }; + + if !matches!(id.as_ref(), "__file__" | "__cached__") { + return; + } + + let mut diagnostic = Diagnostic::new(InvalidGetLoggerArgument, expr.range()); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.set_fix(Fix::suggested(Edit::range_replacement( + "__name__".to_string(), + expr.range(), + ))); + } + checker.diagnostics.push(diagnostic); + } +} diff --git a/crates/ruff/src/rules/flake8_logging/rules/mod.rs b/crates/ruff/src/rules/flake8_logging/rules/mod.rs index e103e801257cd..85272a5c72d37 100644 --- a/crates/ruff/src/rules/flake8_logging/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_logging/rules/mod.rs @@ -1,5 +1,7 @@ pub(crate) use direct_logger_instantiation::*; +pub(crate) use invalid_get_logger_argument::*; pub(crate) use undocumented_warn::*; mod direct_logger_instantiation; +mod invalid_get_logger_argument; mod undocumented_warn; diff --git a/crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG002_LOG002.py.snap b/crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG002_LOG002.py.snap new file mode 100644 index 0000000000000..d59c381528b36 --- /dev/null +++ b/crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG002_LOG002.py.snap @@ -0,0 +1,82 @@ +--- +source: crates/ruff/src/rules/flake8_logging/mod.rs +--- +LOG002.py:11:11: LOG002 [*] Use `__name__` with `logging.getLogger()` + | +10 | # LOG002 +11 | getLogger(__file__) + | ^^^^^^^^ LOG002 +12 | logging.getLogger(name=__file__) + | + = help: Replace with `name` + +ℹ Suggested fix +8 8 | logging.getLogger(name="custom") +9 9 | +10 10 | # LOG002 +11 |-getLogger(__file__) + 11 |+getLogger(__name__) +12 12 | logging.getLogger(name=__file__) +13 13 | +14 14 | logging.getLogger(__cached__) + +LOG002.py:12:24: LOG002 [*] Use `__name__` with `logging.getLogger()` + | +10 | # LOG002 +11 | getLogger(__file__) +12 | logging.getLogger(name=__file__) + | ^^^^^^^^ LOG002 +13 | +14 | logging.getLogger(__cached__) + | + = help: Replace with `name` + +ℹ Suggested fix +9 9 | +10 10 | # LOG002 +11 11 | getLogger(__file__) +12 |-logging.getLogger(name=__file__) + 12 |+logging.getLogger(name=__name__) +13 13 | +14 14 | logging.getLogger(__cached__) +15 15 | getLogger(name=__cached__) + +LOG002.py:14:19: LOG002 [*] Use `__name__` with `logging.getLogger()` + | +12 | logging.getLogger(name=__file__) +13 | +14 | logging.getLogger(__cached__) + | ^^^^^^^^^^ LOG002 +15 | getLogger(name=__cached__) + | + = help: Replace with `name` + +ℹ Suggested fix +11 11 | getLogger(__file__) +12 12 | logging.getLogger(name=__file__) +13 13 | +14 |-logging.getLogger(__cached__) + 14 |+logging.getLogger(__name__) +15 15 | getLogger(name=__cached__) +16 16 | +17 17 | + +LOG002.py:15:16: LOG002 [*] Use `__name__` with `logging.getLogger()` + | +14 | logging.getLogger(__cached__) +15 | getLogger(name=__cached__) + | ^^^^^^^^^^ LOG002 + | + = help: Replace with `name` + +ℹ Suggested fix +12 12 | logging.getLogger(name=__file__) +13 13 | +14 14 | logging.getLogger(__cached__) +15 |-getLogger(name=__cached__) + 15 |+getLogger(name=__name__) +16 16 | +17 17 | +18 18 | # Override `logging.getLogger` + + diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index ee6823c2d113f..b5778bbf22d17 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -859,6 +859,7 @@ mod tests { Rule::TooManyPublicMethods, Rule::TooManyPublicMethods, Rule::UndocumentedWarn, + Rule::InvalidGetLoggerArgument, ]; #[allow(clippy::needless_pass_by_value)] diff --git a/ruff.schema.json b/ruff.schema.json index 9984598eaa4a7..a084a349ab3a7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2137,6 +2137,7 @@ "LOG0", "LOG00", "LOG001", + "LOG002", "LOG009", "N", "N8", From e2a2a959db7165db2d51c4c637991ca058dc0d78 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Fri, 15 Sep 2023 23:45:56 +0530 Subject: [PATCH 2/2] Code review changes --- .../rules/invalid_get_logger_argument.rs | 43 +++++++++++-------- crates/ruff_python_stdlib/src/builtins.rs | 1 + 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/crates/ruff/src/rules/flake8_logging/rules/invalid_get_logger_argument.rs b/crates/ruff/src/rules/flake8_logging/rules/invalid_get_logger_argument.rs index 10d6c3405fe5d..80f3b2bc62833 100644 --- a/crates/ruff/src/rules/flake8_logging/rules/invalid_get_logger_argument.rs +++ b/crates/ruff/src/rules/flake8_logging/rules/invalid_get_logger_argument.rs @@ -1,4 +1,4 @@ -use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; +use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{self as ast, Expr}; use ruff_text_size::Ranged; @@ -43,41 +43,50 @@ use crate::registry::AsRule; #[violation] pub struct InvalidGetLoggerArgument; -impl AlwaysAutofixableViolation for InvalidGetLoggerArgument { +impl Violation for InvalidGetLoggerArgument { + const AUTOFIX: AutofixKind = AutofixKind::Sometimes; + #[derive_message_formats] fn message(&self) -> String { format!("Use `__name__` with `logging.getLogger()`") } - fn autofix_title(&self) -> String { - format!("Replace with `__name__`") + fn autofix_title(&self) -> Option { + Some(format!("Replace with `__name__`")) } } /// LOG002 pub(crate) fn invalid_get_logger_argument(checker: &mut Checker, call: &ast::ExprCall) { - if checker + let Some(Expr::Name(expr @ ast::ExprName { id, .. })) = call.arguments.find_argument("name", 0) + else { + return; + }; + + if !matches!(id.as_ref(), "__file__" | "__cached__") { + return; + } + + if !checker.semantic().is_builtin(id) { + return; + } + + if !checker .semantic() .resolve_call_path(call.func.as_ref()) .is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "getLogger"])) { - let Some(Expr::Name(expr @ ast::ExprName { id, .. })) = - call.arguments.find_argument("name", 0) - else { - return; - }; - - if !matches!(id.as_ref(), "__file__" | "__cached__") { - return; - } + return; + } - let mut diagnostic = Diagnostic::new(InvalidGetLoggerArgument, expr.range()); - if checker.patch(diagnostic.kind.rule()) { + let mut diagnostic = Diagnostic::new(InvalidGetLoggerArgument, expr.range()); + if checker.patch(diagnostic.kind.rule()) { + if checker.semantic().is_builtin("__name__") { diagnostic.set_fix(Fix::suggested(Edit::range_replacement( "__name__".to_string(), expr.range(), ))); } - checker.diagnostics.push(diagnostic); } + checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff_python_stdlib/src/builtins.rs b/crates/ruff_python_stdlib/src/builtins.rs index c0152dcd3a78e..c39ce21cdfc39 100644 --- a/crates/ruff_python_stdlib/src/builtins.rs +++ b/crates/ruff_python_stdlib/src/builtins.rs @@ -167,6 +167,7 @@ pub const MAGIC_GLOBALS: &[&str] = &[ "WindowsError", "__annotations__", "__builtins__", + "__cached__", "__file__", ];