From 7c894921df43669462edc8516ccc21926dbd2aeb Mon Sep 17 00:00:00 2001 From: Steve C Date: Mon, 18 Dec 2023 15:00:04 -0500 Subject: [PATCH] [`pylint`] Implement `too-many-locals` (`PLR0914`) (#9163) ## Summary Implements [`PLR0914` - `too-many-locals`](https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/too-many-locals.html) See #970 ## Test Plan `cargo test` --- .../test/fixtures/pylint/too_many_locals.py | 36 +++++++++++ .../checkers/ast/analyze/deferred_scopes.rs | 5 ++ crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 16 +++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + .../src/rules/pylint/rules/too_many_locals.rs | 59 +++++++++++++++++++ .../ruff_linter/src/rules/pylint/settings.rs | 2 + ...rules__pylint__tests__too_many_locals.snap | 12 ++++ crates/ruff_workspace/src/options.rs | 6 ++ ruff.schema.json | 10 ++++ 10 files changed, 149 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/too_many_locals.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/too_many_locals.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_many_locals.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/too_many_locals.py b/crates/ruff_linter/resources/test/fixtures/pylint/too_many_locals.py new file mode 100644 index 0000000000000..ffb30d7607e19 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/too_many_locals.py @@ -0,0 +1,36 @@ +def func() -> None: # OK + # 15 is max default + first = 1 + second = 2 + third = 3 + fourth = 4 + fifth = 5 + sixth = 6 + seventh = 7 + eighth = 8 + ninth = 9 + tenth = 10 + eleventh = 11 + twelveth = 12 + thirteenth = 13 + fourteenth = 14 + fifteenth = 15 + + +def func() -> None: # PLR0914 + first = 1 + second = 2 + third = 3 + fourth = 4 + fifth = 5 + sixth = 6 + seventh = 7 + eighth = 8 + ninth = 9 + tenth = 10 + eleventh = 11 + twelfth = 12 + thirteenth = 13 + fourteenth = 14 + fifteenth = 15 + sixteenth = 16 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index 66d0ad25273e9..2cef614ea94ed 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -15,6 +15,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { Rule::RedefinedArgumentFromLocal, Rule::RedefinedWhileUnused, Rule::RuntimeImportInTypeCheckingBlock, + Rule::TooManyLocals, Rule::TypingOnlyFirstPartyImport, Rule::TypingOnlyStandardLibraryImport, Rule::TypingOnlyThirdPartyImport, @@ -336,6 +337,10 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { if checker.enabled(Rule::NoSelfUse) { pylint::rules::no_self_use(checker, scope_id, scope, &mut diagnostics); } + + if checker.enabled(Rule::TooManyLocals) { + pylint::rules::too_many_locals(checker, scope, &mut diagnostics); + } } } checker.diagnostics.extend(diagnostics); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 30a8af89b18b2..158d60d4a3f63 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -252,6 +252,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R0911") => (RuleGroup::Stable, rules::pylint::rules::TooManyReturnStatements), (Pylint, "R0912") => (RuleGroup::Stable, rules::pylint::rules::TooManyBranches), (Pylint, "R0913") => (RuleGroup::Stable, rules::pylint::rules::TooManyArguments), + (Pylint, "R0914") => (RuleGroup::Preview, rules::pylint::rules::TooManyLocals), (Pylint, "R0915") => (RuleGroup::Stable, rules::pylint::rules::TooManyStatements), (Pylint, "R0916") => (RuleGroup::Preview, rules::pylint::rules::TooManyBooleanExpressions), (Pylint, "R0917") => (RuleGroup::Preview, rules::pylint::rules::TooManyPositional), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 3933e07bf9e02..303395de50406 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -346,6 +346,22 @@ mod tests { Ok(()) } + #[test] + fn too_many_locals() -> Result<()> { + let diagnostics = test_path( + Path::new("pylint/too_many_locals.py"), + &LinterSettings { + pylint: pylint::settings::Settings { + max_locals: 15, + ..pylint::settings::Settings::default() + }, + ..LinterSettings::for_rules(vec![Rule::TooManyLocals]) + }, + )?; + assert_messages!(diagnostics); + Ok(()) + } + #[test] fn unspecified_encoding_python39_or_lower() -> Result<()> { let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index aaedd82e2f790..85699ab30e1ed 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -55,6 +55,7 @@ pub(crate) use sys_exit_alias::*; pub(crate) use too_many_arguments::*; pub(crate) use too_many_boolean_expressions::*; pub(crate) use too_many_branches::*; +pub(crate) use too_many_locals::*; pub(crate) use too_many_positional::*; pub(crate) use too_many_public_methods::*; pub(crate) use too_many_return_statements::*; @@ -132,6 +133,7 @@ mod sys_exit_alias; mod too_many_arguments; mod too_many_boolean_expressions; mod too_many_branches; +mod too_many_locals; mod too_many_positional; mod too_many_public_methods; mod too_many_return_statements; diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_many_locals.rs b/crates/ruff_linter/src/rules/pylint/rules/too_many_locals.rs new file mode 100644 index 0000000000000..e2137b637bf52 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/too_many_locals.rs @@ -0,0 +1,59 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::identifier::Identifier; +use ruff_python_semantic::{Scope, ScopeKind}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for functions that include too many local variables. +/// +/// By default, this rule allows up to fifteen locals, as configured by the +/// [`pylint.max-locals`] option. +/// +/// ## Why is this bad? +/// Functions with many local variables are harder to understand and maintain. +/// +/// Consider refactoring functions with many local variables into smaller +/// functions with fewer assignments. +/// +/// ## Options +/// - `pylint.max-locals` +#[violation] +pub struct TooManyLocals { + current_amount: usize, + max_amount: usize, +} + +impl Violation for TooManyLocals { + #[derive_message_formats] + fn message(&self) -> String { + let TooManyLocals { + current_amount, + max_amount, + } = self; + format!("Too many local variables: ({current_amount}/{max_amount})") + } +} + +/// PLR0914 +pub(crate) fn too_many_locals(checker: &Checker, scope: &Scope, diagnostics: &mut Vec) { + let num_locals = scope + .binding_ids() + .filter(|id| { + let binding = checker.semantic().binding(*id); + binding.kind.is_assignment() + }) + .count(); + if num_locals > checker.settings.pylint.max_locals { + if let ScopeKind::Function(func) = scope.kind { + diagnostics.push(Diagnostic::new( + TooManyLocals { + current_amount: num_locals, + max_amount: checker.settings.pylint.max_locals, + }, + func.identifier(), + )); + }; + } +} diff --git a/crates/ruff_linter/src/rules/pylint/settings.rs b/crates/ruff_linter/src/rules/pylint/settings.rs index 020390704c7f8..8ea19cdfaf41d 100644 --- a/crates/ruff_linter/src/rules/pylint/settings.rs +++ b/crates/ruff_linter/src/rules/pylint/settings.rs @@ -45,6 +45,7 @@ pub struct Settings { pub max_branches: usize, pub max_statements: usize, pub max_public_methods: usize, + pub max_locals: usize, } impl Default for Settings { @@ -59,6 +60,7 @@ impl Default for Settings { max_branches: 12, max_statements: 50, max_public_methods: 20, + max_locals: 15, } } } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_many_locals.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_many_locals.snap new file mode 100644 index 0000000000000..5150eac445d4e --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__too_many_locals.snap @@ -0,0 +1,12 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +too_many_locals.py:20:5: PLR0914 Too many local variables: (16/15) + | +20 | def func() -> None: # PLR0914 + | ^^^^ PLR0914 +21 | first = 1 +22 | second = 2 + | + + diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index b032c2be3acf5..169b8c3da5060 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -2703,6 +2703,11 @@ pub struct PylintOptions { #[option(default = r"3", value_type = "int", example = r"max-pos-args = 3")] pub max_positional_args: Option, + /// Maximum number of local variables allowed for a function or method body (see: + /// `PLR0914`). + #[option(default = r"15", value_type = "int", example = r"max-locals = 15")] + pub max_locals: Option, + /// Maximum number of statements allowed for a function or method body (see: /// `PLR0915`). #[option(default = r"50", value_type = "int", example = r"max-statements = 50")] @@ -2742,6 +2747,7 @@ impl PylintOptions { max_public_methods: self .max_public_methods .unwrap_or(defaults.max_public_methods), + max_locals: self.max_locals.unwrap_or(defaults.max_locals), } } } diff --git a/ruff.schema.json b/ruff.schema.json index 92d360d20122c..3d566f7964ff2 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2410,6 +2410,15 @@ "format": "uint", "minimum": 0.0 }, + "max-locals": { + "description": "Maximum number of local variables allowed for a function or method body (see: `PLR0914`).", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 + }, "max-positional-args": { "description": "Maximum number of positional arguments allowed for a function or method definition (see: `PLR0917`).\n\nIf not specified, defaults to the value of `max-args`.", "type": [ @@ -3169,6 +3178,7 @@ "PLR0911", "PLR0912", "PLR0913", + "PLR0914", "PLR0915", "PLR0916", "PLR0917",