From 8fde63b323fb1ff6de542be65e0cefcc43ba543e Mon Sep 17 00:00:00 2001 From: Matthieu Devlin Date: Tue, 21 Feb 2023 19:53:11 -0800 Subject: [PATCH] [`pylint`] Implement E1205 and E106 (#3084) --- .../fixtures/pylint/logging_too_few_args.py | 12 ++ .../fixtures/pylint/logging_too_many_args.py | 12 ++ crates/ruff/src/ast/logging.rs | 24 +++ crates/ruff/src/ast/mod.rs | 1 + crates/ruff/src/checkers/ast.rs | 7 + crates/ruff/src/codes.rs | 2 + crates/ruff/src/registry.rs | 2 + .../src/rules/flake8_logging_format/rules.rs | 26 +--- crates/ruff/src/rules/pylint/mod.rs | 2 + crates/ruff/src/rules/pylint/rules/logging.rs | 139 ++++++++++++++++++ crates/ruff/src/rules/pylint/rules/mod.rs | 2 + ...sts__PLE1205_logging_too_many_args.py.snap | 15 ++ ...ests__PLE1206_logging_too_few_args.py.snap | 15 ++ ruff.schema.json | 4 + 14 files changed, 238 insertions(+), 25 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/pylint/logging_too_few_args.py create mode 100644 crates/ruff/resources/test/fixtures/pylint/logging_too_many_args.py create mode 100644 crates/ruff/src/ast/logging.rs create mode 100644 crates/ruff/src/rules/pylint/rules/logging.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1205_logging_too_many_args.py.snap create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1206_logging_too_few_args.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/logging_too_few_args.py b/crates/ruff/resources/test/fixtures/pylint/logging_too_few_args.py new file mode 100644 index 0000000000..1748aea7fa --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/logging_too_few_args.py @@ -0,0 +1,12 @@ +import logging + +logging.warning("Hello %s %s", "World!") # [logging-too-few-args] + +# do not handle calls with kwargs (like pylint) +logging.warning("Hello %s", "World!", "again", something="else") + +logging.warning("Hello %s", "World!") + +import warning + +warning.warning("Hello %s %s", "World!") diff --git a/crates/ruff/resources/test/fixtures/pylint/logging_too_many_args.py b/crates/ruff/resources/test/fixtures/pylint/logging_too_many_args.py new file mode 100644 index 0000000000..3127e97234 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/logging_too_many_args.py @@ -0,0 +1,12 @@ +import logging + +logging.warning("Hello %s", "World!", "again") # [logging-too-many-args] + +# do not handle calls with kwargs (like pylint) +logging.warning("Hello %s", "World!", "again", something="else") + +logging.warning("Hello %s", "World!") + +import warning + +warning.warning("Hello %s", "World!", "again") diff --git a/crates/ruff/src/ast/logging.rs b/crates/ruff/src/ast/logging.rs new file mode 100644 index 0000000000..1437ef64db --- /dev/null +++ b/crates/ruff/src/ast/logging.rs @@ -0,0 +1,24 @@ +pub enum LoggingLevel { + Debug, + Critical, + Error, + Exception, + Info, + Warn, + Warning, +} + +impl LoggingLevel { + pub fn from_str(level: &str) -> Option { + match level { + "debug" => Some(LoggingLevel::Debug), + "critical" => Some(LoggingLevel::Critical), + "error" => Some(LoggingLevel::Error), + "exception" => Some(LoggingLevel::Exception), + "info" => Some(LoggingLevel::Info), + "warn" => Some(LoggingLevel::Warn), + "warning" => Some(LoggingLevel::Warning), + _ => None, + } + } +} diff --git a/crates/ruff/src/ast/mod.rs b/crates/ruff/src/ast/mod.rs index a62dbbdfb9..af474a54a6 100644 --- a/crates/ruff/src/ast/mod.rs +++ b/crates/ruff/src/ast/mod.rs @@ -4,6 +4,7 @@ pub mod comparable; pub mod function_type; pub mod hashable; pub mod helpers; +pub mod logging; pub mod operations; pub mod relocate; pub mod types; diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 50ef194572..19cc9a551d 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -2918,6 +2918,13 @@ where { flake8_logging_format::rules::logging_call(self, func, args, keywords); } + + // pylint logging checker + if self.settings.rules.enabled(&Rule::LoggingTooFewArgs) + || self.settings.rules.enabled(&Rule::LoggingTooManyArgs) + { + pylint::rules::logging_call(self, func, args, keywords); + } } ExprKind::Dict { keys, values } => { if self diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index e923bbafe7..be7fb612b5 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -126,6 +126,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Pylint, "E0101") => Rule::ReturnInInit, (Pylint, "E0604") => Rule::InvalidAllObject, (Pylint, "E0605") => Rule::InvalidAllFormat, + (Pylint, "E1205") => Rule::LoggingTooManyArgs, + (Pylint, "E1206") => Rule::LoggingTooFewArgs, (Pylint, "E1307") => Rule::BadStringFormatType, (Pylint, "E2502") => Rule::BidirectionalUnicode, (Pylint, "E1310") => Rule::BadStrStripCall, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index b615e4d873..4ddd0953e1 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -150,6 +150,8 @@ ruff_macros::register_rules!( rules::pylint::rules::TooManyBranches, rules::pylint::rules::TooManyStatements, rules::pylint::rules::RedefinedLoopName, + rules::pylint::rules::LoggingTooFewArgs, + rules::pylint::rules::LoggingTooManyArgs, // flake8-builtins rules::flake8_builtins::rules::BuiltinVariableShadowing, rules::flake8_builtins::rules::BuiltinArgumentShadowing, diff --git a/crates/ruff/src/rules/flake8_logging_format/rules.rs b/crates/ruff/src/rules/flake8_logging_format/rules.rs index de8db039a9..427b021728 100644 --- a/crates/ruff/src/rules/flake8_logging_format/rules.rs +++ b/crates/ruff/src/rules/flake8_logging_format/rules.rs @@ -1,6 +1,7 @@ use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword, Location, Operator}; use crate::ast::helpers::{find_keyword, is_logger_candidate, SimpleCallArgs}; +use crate::ast::logging::LoggingLevel; use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::fix::Fix; @@ -10,31 +11,6 @@ use crate::rules::flake8_logging_format::violations::{ LoggingRedundantExcInfo, LoggingStringConcat, LoggingStringFormat, LoggingWarn, }; -enum LoggingLevel { - Debug, - Critical, - Error, - Exception, - Info, - Warn, - Warning, -} - -impl LoggingLevel { - fn from_str(level: &str) -> Option { - match level { - "debug" => Some(LoggingLevel::Debug), - "critical" => Some(LoggingLevel::Critical), - "error" => Some(LoggingLevel::Error), - "exception" => Some(LoggingLevel::Exception), - "info" => Some(LoggingLevel::Info), - "warn" => Some(LoggingLevel::Warn), - "warning" => Some(LoggingLevel::Warning), - _ => None, - } - } -} - const RESERVED_ATTRS: &[&str; 22] = &[ "args", "asctime", diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index eed3b0d67c..6a9b341552 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -17,6 +17,8 @@ mod tests { use crate::settings::Settings; use crate::test::test_path; + #[test_case(Rule::LoggingTooManyArgs, Path::new("logging_too_many_args.py"); "PLE1205")] + #[test_case(Rule::LoggingTooFewArgs, Path::new("logging_too_few_args.py"); "PLE1206")] #[test_case(Rule::ReturnInInit, Path::new("return_in_init.py"); "PLE0101")] #[test_case(Rule::UselessImportAlias, Path::new("import_aliasing.py"); "PLC0414")] #[test_case(Rule::UnnecessaryDirectLambdaCall, Path::new("unnecessary_direct_lambda_call.py"); "PLC3002")] diff --git a/crates/ruff/src/rules/pylint/rules/logging.rs b/crates/ruff/src/rules/pylint/rules/logging.rs new file mode 100644 index 0000000000..666ecf6f8d --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/logging.rs @@ -0,0 +1,139 @@ +use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword}; + +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::ast::helpers::{is_logger_candidate, SimpleCallArgs}; +use crate::ast::logging::LoggingLevel; +use crate::ast::types::Range; +use crate::checkers::ast::Checker; +use crate::registry::{Diagnostic, Rule}; +use crate::rules::pyflakes::cformat::CFormatSummary; +use crate::violation::Violation; + +define_violation!( + /// ## What it does + /// Checks for too few positional arguments for a `logging` format string. + /// + /// ## Why is this bad? + /// A `TypeError` will be raised if the statement is run. + /// + /// ## Example + /// ```python + /// import logging + /// + /// try: + /// function() + /// except Exception as e: + /// logging.error('%s error occurred: %s', e) # [logging-too-few-args] + /// raise + /// ``` + /// + /// Use instead: + /// ```python + /// import logging + /// + /// try: + /// function() + /// except Exception as e: + /// logging.error('%s error occurred: %s', type(e), e) + /// raise + /// ``` + pub struct LoggingTooFewArgs; +); +impl Violation for LoggingTooFewArgs { + #[derive_message_formats] + fn message(&self) -> String { + format!("Not enough arguments for `logging` format string") + } +} + +define_violation!( + /// ## What it does + /// Checks for too many positional arguments for a `logging` format string. + /// + /// ## Why is this bad? + /// A `TypeError` will be raised if the statement is run. + /// + /// ## Example + /// ```python + /// import logging + /// + /// try: + /// function() + /// except Exception as e: + /// logging.error('Error occurred: %s', type(e), e) # [logging-too-many-args] + /// raise + /// ``` + /// + /// Use instead: + /// ```python + /// import logging + /// + /// try: + /// function() + /// except Exception as e: + /// logging.error('%s error occurred: %s', type(e), e) + /// raise + /// ``` + pub struct LoggingTooManyArgs; +); +impl Violation for LoggingTooManyArgs { + #[derive_message_formats] + fn message(&self) -> String { + format!("Too many arguments for `logging` format string") + } +} + +/// Check logging calls for violations. +pub fn logging_call(checker: &mut Checker, func: &Expr, args: &[Expr], keywords: &[Keyword]) { + if !is_logger_candidate(func) { + return; + } + + if let ExprKind::Attribute { attr, .. } = &func.node { + if LoggingLevel::from_str(attr.as_str()).is_some() { + let call_args = SimpleCallArgs::new(args, keywords); + + // E1205 - E1206 + if let Some(msg) = call_args.get_argument("msg", Some(0)) { + if let ExprKind::Constant { + value: Constant::Str(value), + .. + } = &msg.node + { + if let Ok(summary) = CFormatSummary::try_from(value.as_str()) { + if summary.starred { + return; + } + + if !call_args.kwargs.is_empty() { + // Keyword checking on logging strings is complicated by + // special keywords - out of scope. + return; + } + + let message_args = call_args.args.len() - 1; + + if checker.settings.rules.enabled(&Rule::LoggingTooManyArgs) + && summary.num_positional < message_args + { + checker.diagnostics.push(Diagnostic::new( + LoggingTooManyArgs, + Range::from_located(func), + )); + } + + if checker.settings.rules.enabled(&Rule::LoggingTooFewArgs) + && summary.num_positional > message_args + { + checker.diagnostics.push(Diagnostic::new( + LoggingTooFewArgs, + Range::from_located(func), + )); + } + } + } + } + } + } +} diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index d4db6fabeb..3c150358fa 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -7,6 +7,7 @@ pub use consider_using_sys_exit::{consider_using_sys_exit, ConsiderUsingSysExit} pub use global_variable_not_assigned::GlobalVariableNotAssigned; pub use invalid_all_format::{invalid_all_format, InvalidAllFormat}; pub use invalid_all_object::{invalid_all_object, InvalidAllObject}; +pub use logging::{logging_call, LoggingTooFewArgs, LoggingTooManyArgs}; pub use magic_value_comparison::{magic_value_comparison, MagicValueComparison}; pub use merge_isinstance::{merge_isinstance, ConsiderMergingIsinstance}; pub use nonlocal_without_binding::NonlocalWithoutBinding; @@ -37,6 +38,7 @@ mod consider_using_sys_exit; mod global_variable_not_assigned; mod invalid_all_format; mod invalid_all_object; +mod logging; mod magic_value_comparison; mod merge_isinstance; mod nonlocal_without_binding; diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1205_logging_too_many_args.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1205_logging_too_many_args.py.snap new file mode 100644 index 0000000000..7030b8b299 --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1205_logging_too_many_args.py.snap @@ -0,0 +1,15 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + LoggingTooManyArgs: ~ + location: + row: 3 + column: 0 + end_location: + row: 3 + column: 15 + fix: ~ + parent: ~ + diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1206_logging_too_few_args.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1206_logging_too_few_args.py.snap new file mode 100644 index 0000000000..3b4a27779f --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1206_logging_too_few_args.py.snap @@ -0,0 +1,15 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + LoggingTooFewArgs: ~ + location: + row: 3 + column: 0 + end_location: + row: 3 + column: 15 + fix: ~ + parent: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index a4d07f6240..ece4b58d81 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1793,6 +1793,10 @@ "PLE11", "PLE114", "PLE1142", + "PLE12", + "PLE120", + "PLE1205", + "PLE1206", "PLE13", "PLE130", "PLE1307",