From 450fb9b99a5d899094fcb338c748d361e90b9feb Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 14 Sep 2023 23:07:10 -0400 Subject: [PATCH] [`flake8-logging`] Implement `LOG001`: `direct-logger-instantiation` (#7397) ## Summary See: https://github.com/astral-sh/ruff/issues/7248. --- .../test/fixtures/flake8_logging/LOG001.py | 5 ++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff/src/codes.rs | 1 + .../rules/mutable_argument_default.rs | 1 + crates/ruff/src/rules/flake8_logging/mod.rs | 1 + .../rules/direct_logger_instantiation.rs | 77 +++++++++++++++++++ .../src/rules/flake8_logging/rules/mod.rs | 2 + .../flake8_logging/rules/undocumented_warn.rs | 2 +- ...ake8_logging__tests__LOG001_LOG001.py.snap | 40 ++++++++++ crates/ruff_workspace/src/configuration.rs | 4 +- ruff.schema.json | 1 + 11 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/flake8_logging/LOG001.py create mode 100644 crates/ruff/src/rules/flake8_logging/rules/direct_logger_instantiation.rs create mode 100644 crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG001_LOG001.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_logging/LOG001.py b/crates/ruff/resources/test/fixtures/flake8_logging/LOG001.py new file mode 100644 index 0000000000000..a48653f5d64e0 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_logging/LOG001.py @@ -0,0 +1,5 @@ +import logging + +logging.Logger(__name__) +logging.Logger() +logging.getLogger(__name__) diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index 321cd4fcdb5df..f7f0856c79831 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -892,6 +892,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::QuadraticListSummation) { ruff::rules::quadratic_list_summation(checker, call); } + if checker.enabled(Rule::DirectLoggerInstantiation) { + flake8_logging::rules::direct_logger_instantiation(checker, call); + } } Expr::Dict(ast::ExprDict { keys, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 1cf96802f1a87..40129d51267da 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -921,6 +921,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy), // flake8-logging + (Flake8Logging, "001") => (RuleGroup::Preview, rules::flake8_logging::rules::DirectLoggerInstantiation), (Flake8Logging, "009") => (RuleGroup::Preview, rules::flake8_logging::rules::UndocumentedWarn), _ => return None, diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs b/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs index c1a34be60c5d3..f604d18d0c887 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/mutable_argument_default.rs @@ -70,6 +70,7 @@ impl Violation for MutableArgumentDefault { fn message(&self) -> String { format!("Do not use mutable data structures for argument defaults") } + fn autofix_title(&self) -> Option { Some(format!("Replace with `None`; initialize within function")) } diff --git a/crates/ruff/src/rules/flake8_logging/mod.rs b/crates/ruff/src/rules/flake8_logging/mod.rs index 05700213f993f..b71cc64a8c46d 100644 --- a/crates/ruff/src/rules/flake8_logging/mod.rs +++ b/crates/ruff/src/rules/flake8_logging/mod.rs @@ -13,6 +13,7 @@ mod tests { use crate::settings::Settings; use crate::test::test_path; + #[test_case(Rule::DirectLoggerInstantiation, Path::new("LOG001.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/direct_logger_instantiation.rs b/crates/ruff/src/rules/flake8_logging/rules/direct_logger_instantiation.rs new file mode 100644 index 0000000000000..c0c2893e736da --- /dev/null +++ b/crates/ruff/src/rules/flake8_logging/rules/direct_logger_instantiation.rs @@ -0,0 +1,77 @@ +use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast as ast; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; +use crate::importer::ImportRequest; +use crate::registry::AsRule; + +/// ## What it does +/// Checks for direct instantiation of `logging.Logger`, as opposed to using +/// `logging.getLogger()`. +/// +/// ## Why is this bad? +/// The [Logger Objects] documentation states that: +/// +/// > Note that Loggers should NEVER be instantiated directly, but always +/// > through the module-level function `logging.getLogger(name)`. +/// +/// If a logger is directly instantiated, it won't be added to the logger +/// tree, and will bypass all configuration. Messages logged to it will +/// only be sent to the "handler of last resort", skipping any filtering +/// or formatting. +/// +/// ## Example +/// ```python +/// import logging +/// +/// logger = logging.Logger(__name__) +/// ``` +/// +/// Use instead: +/// ```python +/// import logging +/// +/// logger = logging.getLogger(__name__) +/// ``` +/// +/// [Logger Objects]: https://docs.python.org/3/library/logging.html#logger-objects +#[violation] +pub struct DirectLoggerInstantiation; + +impl Violation for DirectLoggerInstantiation { + const AUTOFIX: AutofixKind = AutofixKind::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Use `logging.getLogger()` to instantiate loggers") + } + + fn autofix_title(&self) -> Option { + Some(format!("Replace with `logging.getLogger()`")) + } +} + +/// LOG001 +pub(crate) fn direct_logger_instantiation(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", "Logger"])) + { + let mut diagnostic = Diagnostic::new(DirectLoggerInstantiation, call.func.range()); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.try_set_fix(|| { + let (import_edit, binding) = checker.importer().get_or_import_symbol( + &ImportRequest::import("logging", "getLogger"), + call.func.start(), + checker.semantic(), + )?; + let reference_edit = Edit::range_replacement(binding, call.func.range()); + Ok(Fix::suggested_edits(import_edit, [reference_edit])) + }); + } + 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 07b712a4f4239..e103e801257cd 100644 --- a/crates/ruff/src/rules/flake8_logging/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_logging/rules/mod.rs @@ -1,3 +1,5 @@ +pub(crate) use direct_logger_instantiation::*; pub(crate) use undocumented_warn::*; +mod direct_logger_instantiation; mod undocumented_warn; diff --git a/crates/ruff/src/rules/flake8_logging/rules/undocumented_warn.rs b/crates/ruff/src/rules/flake8_logging/rules/undocumented_warn.rs index abbdf81c38ab1..d09b32da891d9 100644 --- a/crates/ruff/src/rules/flake8_logging/rules/undocumented_warn.rs +++ b/crates/ruff/src/rules/flake8_logging/rules/undocumented_warn.rs @@ -59,7 +59,7 @@ pub(crate) fn undocumented_warn(checker: &mut Checker, expr: &Expr) { diagnostic.try_set_fix(|| { let (import_edit, binding) = checker.importer().get_or_import_symbol( &ImportRequest::import("logging", "WARNING"), - expr.range().start(), + expr.start(), checker.semantic(), )?; let reference_edit = Edit::range_replacement(binding, expr.range()); diff --git a/crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG001_LOG001.py.snap b/crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG001_LOG001.py.snap new file mode 100644 index 0000000000000..c94fed6a12fc4 --- /dev/null +++ b/crates/ruff/src/rules/flake8_logging/snapshots/ruff__rules__flake8_logging__tests__LOG001_LOG001.py.snap @@ -0,0 +1,40 @@ +--- +source: crates/ruff/src/rules/flake8_logging/mod.rs +--- +LOG001.py:3:1: LOG001 [*] Use `logging.getLogger()` to instantiate loggers + | +1 | import logging +2 | +3 | logging.Logger(__name__) + | ^^^^^^^^^^^^^^ LOG001 +4 | logging.Logger() +5 | logging.getLogger(__name__) + | + = help: Replace with `logging.getLogger()` + +ℹ Suggested fix +1 1 | import logging +2 2 | +3 |-logging.Logger(__name__) + 3 |+logging.getLogger(__name__) +4 4 | logging.Logger() +5 5 | logging.getLogger(__name__) + +LOG001.py:4:1: LOG001 [*] Use `logging.getLogger()` to instantiate loggers + | +3 | logging.Logger(__name__) +4 | logging.Logger() + | ^^^^^^^^^^^^^^ LOG001 +5 | logging.getLogger(__name__) + | + = help: Replace with `logging.getLogger()` + +ℹ Suggested fix +1 1 | import logging +2 2 | +3 3 | logging.Logger(__name__) +4 |-logging.Logger() + 4 |+logging.getLogger() +5 5 | logging.getLogger(__name__) + + diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 701af08c380a7..ee6823c2d113f 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -853,9 +853,11 @@ mod tests { ]; const PREVIEW_RULES: &[Rule] = &[ + Rule::DirectLoggerInstantiation, Rule::ManualDictComprehension, - Rule::TooManyPublicMethods, Rule::SliceCopy, + Rule::TooManyPublicMethods, + Rule::TooManyPublicMethods, Rule::UndocumentedWarn, ]; diff --git a/ruff.schema.json b/ruff.schema.json index 9b99604d3f554..9984598eaa4a7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2136,6 +2136,7 @@ "LOG", "LOG0", "LOG00", + "LOG001", "LOG009", "N", "N8",