Skip to content

Commit

Permalink
[pylint] Implement too-many-locals (PLR0914) (#9163)
Browse files Browse the repository at this point in the history
## 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`
  • Loading branch information
diceroll123 authored Dec 18, 2023
1 parent be8f8e6 commit 7c89492
Show file tree
Hide file tree
Showing 10 changed files with 149 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
16 changes: 16 additions & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
59 changes: 59 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/too_many_locals.rs
Original file line number Diff line number Diff line change
@@ -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<Diagnostic>) {
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(),
));
};
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -59,6 +60,7 @@ impl Default for Settings {
max_branches: 12,
max_statements: 50,
max_public_methods: 20,
max_locals: 15,
}
}
}
Original file line number Diff line number Diff line change
@@ -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
|


6 changes: 6 additions & 0 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<usize>,

/// 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<usize>,

/// 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")]
Expand Down Expand Up @@ -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),
}
}
}
Expand Down
10 changes: 10 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 7c89492

Please sign in to comment.