From 33196f1859d8a7d0a3b5a603f800865dde87849f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 24 Jul 2023 01:14:48 -0400 Subject: [PATCH] Fix logging rules with whitespace around dot (#6022) ## Summary Attempting to fix, e.g., `logging . warn("Hello World!")` was causing a syntax error. --- .../fixtures/flake8_logging_format/G010.py | 2 + .../rules/logging_call.rs | 115 +++++++++--------- ...flake8_logging_format__tests__G010.py.snap | 35 ++++-- crates/ruff_python_stdlib/src/logging.rs | 2 +- 4 files changed, 88 insertions(+), 66 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_logging_format/G010.py b/crates/ruff/resources/test/fixtures/flake8_logging_format/G010.py index cd22173b5acac..fc0029c40987a 100644 --- a/crates/ruff/resources/test/fixtures/flake8_logging_format/G010.py +++ b/crates/ruff/resources/test/fixtures/flake8_logging_format/G010.py @@ -6,3 +6,5 @@ logging.warn("Hello World!") log.warn("Hello world!") # This shouldn't be considered as a logger candidate logger.warn("Hello world!") + +logging . warn("Hello World!") diff --git a/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs index 85cc170b89036..5b75402168235 100644 --- a/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs @@ -1,4 +1,3 @@ -use ruff_text_size::{TextRange, TextSize}; use rustpython_parser::ast::{self, Constant, Expr, Keyword, Operator, Ranged}; use ruff_diagnostics::{Diagnostic, Edit, Fix}; @@ -133,7 +132,7 @@ fn check_log_record_attr_clash(checker: &mut Checker, extra: &Keyword) { } } -#[derive(Copy, Clone)] +#[derive(Debug, Copy, Clone)] enum LoggingCallType { /// Logging call with a level method, e.g., `logging.info`. LevelCall(LoggingLevel), @@ -158,73 +157,73 @@ pub(crate) fn logging_call( args: &[Expr], keywords: &[Keyword], ) { + let Expr::Attribute(ast::ExprAttribute { value: _, attr, .. }) = func else { + return; + }; + + let Some(logging_call_type) = LoggingCallType::from_attribute(attr.as_str()) else { + return; + }; + if !logging::is_logger_candidate(func, checker.semantic(), &checker.settings.logger_objects) { return; } - if let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func { - if let Some(logging_call_type) = LoggingCallType::from_attribute(attr.as_str()) { - let call_args = CallArguments::new(args, keywords); - let level_call_range = TextRange::new(value.end() + TextSize::from(1), func.end()); - - // G001 - G004 - let msg_pos = usize::from(matches!(logging_call_type, LoggingCallType::LogCall)); - if let Some(format_arg) = call_args.argument("msg", msg_pos) { - check_msg(checker, format_arg); - } + // G001 - G004 + let msg_pos = usize::from(matches!(logging_call_type, LoggingCallType::LogCall)); + if let Some(format_arg) = CallArguments::new(args, keywords).argument("msg", msg_pos) { + check_msg(checker, format_arg); + } - // G010 - if checker.enabled(Rule::LoggingWarn) - && matches!( - logging_call_type, - LoggingCallType::LevelCall(LoggingLevel::Warn) - ) - { - let mut diagnostic = Diagnostic::new(LoggingWarn, level_call_range); - if checker.patch(diagnostic.kind.rule()) { - diagnostic.set_fix(Fix::automatic(Edit::range_replacement( - "warning".to_string(), - level_call_range, - ))); - } - checker.diagnostics.push(diagnostic); + // G010 + if checker.enabled(Rule::LoggingWarn) { + if matches!( + logging_call_type, + LoggingCallType::LevelCall(LoggingLevel::Warn) + ) { + let mut diagnostic = Diagnostic::new(LoggingWarn, attr.range()); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.set_fix(Fix::automatic(Edit::range_replacement( + "warning".to_string(), + attr.range(), + ))); } + checker.diagnostics.push(diagnostic); + } + } - // G101 - if checker.enabled(Rule::LoggingExtraAttrClash) { - if let Some(extra) = find_keyword(keywords, "extra") { - check_log_record_attr_clash(checker, extra); - } - } + // G101 + if checker.enabled(Rule::LoggingExtraAttrClash) { + if let Some(extra) = find_keyword(keywords, "extra") { + check_log_record_attr_clash(checker, extra); + } + } - // G201, G202 - if checker.any_enabled(&[Rule::LoggingExcInfo, Rule::LoggingRedundantExcInfo]) { - if !checker.semantic().in_exception_handler() { - return; + // G201, G202 + if checker.any_enabled(&[Rule::LoggingExcInfo, Rule::LoggingRedundantExcInfo]) { + if !checker.semantic().in_exception_handler() { + return; + } + let Some(exc_info) = logging::exc_info(keywords, checker.semantic()) else { + return; + }; + if let LoggingCallType::LevelCall(logging_level) = logging_call_type { + match logging_level { + LoggingLevel::Error => { + if checker.enabled(Rule::LoggingExcInfo) { + checker + .diagnostics + .push(Diagnostic::new(LoggingExcInfo, attr.range())); + } } - let Some(exc_info) = logging::exc_info(keywords, checker.semantic()) else { - return; - }; - if let LoggingCallType::LevelCall(logging_level) = logging_call_type { - match logging_level { - LoggingLevel::Error => { - if checker.enabled(Rule::LoggingExcInfo) { - checker - .diagnostics - .push(Diagnostic::new(LoggingExcInfo, level_call_range)); - } - } - LoggingLevel::Exception => { - if checker.enabled(Rule::LoggingRedundantExcInfo) { - checker.diagnostics.push(Diagnostic::new( - LoggingRedundantExcInfo, - exc_info.range(), - )); - } - } - _ => {} + LoggingLevel::Exception => { + if checker.enabled(Rule::LoggingRedundantExcInfo) { + checker + .diagnostics + .push(Diagnostic::new(LoggingRedundantExcInfo, exc_info.range())); } } + _ => {} } } } diff --git a/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G010.py.snap b/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G010.py.snap index e426e51f942bc..faa3918fa1a9b 100644 --- a/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G010.py.snap +++ b/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G010.py.snap @@ -20,15 +20,18 @@ G010.py:6:9: G010 [*] Logging statement uses `warn` instead of `warning` 6 |+logging.warning("Hello World!") 7 7 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate 8 8 | logger.warn("Hello world!") +9 9 | G010.py:8:8: G010 [*] Logging statement uses `warn` instead of `warning` - | -6 | logging.warn("Hello World!") -7 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate -8 | logger.warn("Hello world!") - | ^^^^ G010 - | - = help: Convert to `warn` + | + 6 | logging.warn("Hello World!") + 7 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate + 8 | logger.warn("Hello world!") + | ^^^^ G010 + 9 | +10 | logging . warn("Hello World!") + | + = help: Convert to `warn` ℹ Fix 5 5 | @@ -36,5 +39,23 @@ G010.py:8:8: G010 [*] Logging statement uses `warn` instead of `warning` 7 7 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate 8 |-logger.warn("Hello world!") 8 |+logger.warning("Hello world!") +9 9 | +10 10 | logging . warn("Hello World!") + +G010.py:10:11: G010 [*] Logging statement uses `warn` instead of `warning` + | + 8 | logger.warn("Hello world!") + 9 | +10 | logging . warn("Hello World!") + | ^^^^ G010 + | + = help: Convert to `warn` + +ℹ Fix +7 7 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate +8 8 | logger.warn("Hello world!") +9 9 | +10 |-logging . warn("Hello World!") + 10 |+logging . warning("Hello World!") diff --git a/crates/ruff_python_stdlib/src/logging.rs b/crates/ruff_python_stdlib/src/logging.rs index 262132967dbc4..c1d164539ff30 100644 --- a/crates/ruff_python_stdlib/src/logging.rs +++ b/crates/ruff_python_stdlib/src/logging.rs @@ -1,4 +1,4 @@ -#[derive(Copy, Clone)] +#[derive(Debug, Copy, Clone)] pub enum LoggingLevel { Debug, Critical,