Skip to content

Commit

Permalink
[pylint] Implement too-many-positional (PLR0917) (#8995)
Browse files Browse the repository at this point in the history
## Summary

Adds a rule that bans too many positional (i.e. not keyword-only)
parameters in function definitions.

Fixes #8946

Rule ID code taken from pylint-dev/pylint#9278

## Test Plan
1. fixtures file checking multiple OKs/fails
2. parametrized test file
  • Loading branch information
flying-sheep committed Dec 4, 2023
1 parent 060a25d commit b90027d
Show file tree
Hide file tree
Showing 12 changed files with 209 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
def f(x, y, z, t, u, v, w, r): # Too many positional arguments (8/3)
pass


def f(x): # OK
pass


def f(x, y, z, _t, _u, _v, _w, r): # OK (underscore-prefixed names are ignored
pass


def f(x, y, z, *, u=1, v=1, r=1): # OK
pass


def f(x=1, y=1, z=1): # OK
pass


def f(x, y, z, /, u, v, w): # Too many positional arguments (6/3)
pass


def f(x, y, z, *, u, v, w): # OK
pass


def f(x, y, z, a, b, c, *, u, v, w): # Too many positional arguments (6/3)
pass
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Too many positional arguments (7/4) for max_positional=4
# OK for dummy_variable_rgx ~ "skip_.*"
def f(w, x, y, z, skip_t, skip_u, skip_v):
pass


# Too many positional arguments (7/4) for max_args=4
# Too many positional arguments (7/3) for dummy_variable_rgx ~ "skip_.*"
def f(w, x, y, z, t, u, v):
pass
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::TooManyArguments) {
pylint::rules::too_many_arguments(checker, function_def);
}
if checker.enabled(Rule::TooManyPositional) {
pylint::rules::too_many_positional(checker, parameters, stmt);
}
if checker.enabled(Rule::TooManyReturnStatements) {
if let Some(diagnostic) = pylint::rules::too_many_return_statements(
stmt,
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 @@ -254,6 +254,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "R0913") => (RuleGroup::Stable, rules::pylint::rules::TooManyArguments),
(Pylint, "R0915") => (RuleGroup::Stable, rules::pylint::rules::TooManyStatements),
(Pylint, "R0916") => (RuleGroup::Preview, rules::pylint::rules::TooManyBooleanExpressions),
(Pylint, "R0917") => (RuleGroup::Preview, rules::pylint::rules::TooManyPositional),
(Pylint, "R1701") => (RuleGroup::Stable, rules::pylint::rules::RepeatedIsinstanceCalls),
(Pylint, "R1704") => (RuleGroup::Preview, rules::pylint::rules::RedefinedArgumentFromLocal),
(Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn),
Expand Down
17 changes: 17 additions & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ mod tests {
#[test_case(Rule::RedefinedLoopName, Path::new("redefined_loop_name.py"))]
#[test_case(Rule::ReturnInInit, Path::new("return_in_init.py"))]
#[test_case(Rule::TooManyArguments, Path::new("too_many_arguments.py"))]
#[test_case(Rule::TooManyPositional, Path::new("too_many_positional.py"))]
#[test_case(Rule::TooManyBranches, Path::new("too_many_branches.py"))]
#[test_case(
Rule::TooManyReturnStatements,
Expand Down Expand Up @@ -249,6 +250,22 @@ mod tests {
Ok(())
}

#[test]
fn max_positional_args() -> Result<()> {
let diagnostics = test_path(
Path::new("pylint/too_many_positional_params.py"),
&LinterSettings {
pylint: pylint::settings::Settings {
max_positional_args: 4,
..pylint::settings::Settings::default()
},
..LinterSettings::for_rule(Rule::TooManyPositional)
},
)?;
assert_messages!(diagnostics);
Ok(())
}

#[test]
fn max_branches() -> 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_positional::*;
pub(crate) use too_many_public_methods::*;
pub(crate) use too_many_return_statements::*;
pub(crate) use too_many_statements::*;
Expand Down Expand Up @@ -131,6 +132,7 @@ mod sys_exit_alias;
mod too_many_arguments;
mod too_many_boolean_expressions;
mod too_many_branches;
mod too_many_positional;
mod too_many_public_methods;
mod too_many_return_statements;
mod too_many_statements;
Expand Down
79 changes: 79 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/too_many_positional.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{identifier::Identifier, Parameters, Stmt};

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for function definitions that include too many positional arguments.
///
/// By default, this rule allows up to five arguments, as configured by the
/// [`pylint.max-positional-args`] option.
///
/// ## Why is this bad?
/// Functions with many arguments are harder to understand, maintain, and call.
/// This is especially true for functions with many positional arguments, as
/// providing arguments positionally is more error-prone and less clear to
/// readers than providing arguments by name.
///
/// Consider refactoring functions with many arguments into smaller functions
/// with fewer arguments, using objects to group related arguments, or
/// migrating to keyword-only arguments.
///
/// ## Example
/// ```python
/// def plot(x, y, z, color, mark, add_trendline):
/// ...
///
///
/// plot(1, 2, 3, "r", "*", True)
/// ```
///
/// Use instead:
/// ```python
/// def plot(x, y, z, *, color, mark, add_trendline):
/// ...
///
///
/// plot(1, 2, 3, color="r", mark="*", add_trendline=True)
/// ```
///
/// ## Options
/// - `pylint.max-positional-args`
#[violation]
pub struct TooManyPositional {
c_pos: usize,
max_pos: usize,
}

impl Violation for TooManyPositional {
#[derive_message_formats]
fn message(&self) -> String {
let TooManyPositional { c_pos, max_pos } = self;
format!("Too many positional arguments: ({c_pos}/{max_pos})")
}
}

/// PLR0917
pub(crate) fn too_many_positional(checker: &mut Checker, parameters: &Parameters, stmt: &Stmt) {
let num_positional_args = parameters
.args
.iter()
.chain(&parameters.posonlyargs)
.filter(|arg| {
!checker
.settings
.dummy_variable_rgx
.is_match(&arg.parameter.name)
})
.count();
if num_positional_args > checker.settings.pylint.max_positional_args {
checker.diagnostics.push(Diagnostic::new(
TooManyPositional {
c_pos: num_positional_args,
max_pos: checker.settings.pylint.max_positional_args,
},
stmt.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 @@ -39,6 +39,7 @@ pub struct Settings {
pub allow_magic_value_types: Vec<ConstantType>,
pub allow_dunder_method_names: FxHashSet<String>,
pub max_args: usize,
pub max_positional_args: usize,
pub max_returns: usize,
pub max_bool_expr: usize,
pub max_branches: usize,
Expand All @@ -52,6 +53,7 @@ impl Default for Settings {
allow_magic_value_types: vec![ConstantType::Str, ConstantType::Bytes],
allow_dunder_method_names: FxHashSet::default(),
max_args: 5,
max_positional_args: 5,
max_returns: 6,
max_bool_expr: 5,
max_branches: 12,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
too_many_positional.py:1:5: PLR0917 Too many positional arguments: (8/5)
|
1 | def f(x, y, z, t, u, v, w, r): # Too many positional arguments (8/3)
| ^ PLR0917
2 | pass
|

too_many_positional.py:21:5: PLR0917 Too many positional arguments: (6/5)
|
21 | def f(x, y, z, /, u, v, w): # Too many positional arguments (6/3)
| ^ PLR0917
22 | pass
|

too_many_positional.py:29:5: PLR0917 Too many positional arguments: (6/5)
|
29 | def f(x, y, z, a, b, c, *, u, v, w): # Too many positional arguments (6/3)
| ^ PLR0917
30 | pass
|


Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
too_many_positional_params.py:3:5: PLR0917 Too many positional arguments: (7/4)
|
1 | # Too many positional arguments (7/4) for max_positional=4
2 | # OK for dummy_variable_rgx ~ "skip_.*"
3 | def f(w, x, y, z, skip_t, skip_u, skip_v):
| ^ PLR0917
4 | pass
|

too_many_positional_params.py:9:5: PLR0917 Too many positional arguments: (7/4)
|
7 | # Too many positional arguments (7/4) for max_args=4
8 | # Too many positional arguments (7/3) for dummy_variable_rgx ~ "skip_.*"
9 | def f(w, x, y, z, t, u, v):
| ^ PLR0917
10 | pass
|


8 changes: 8 additions & 0 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2635,6 +2635,11 @@ pub struct PylintOptions {
#[option(default = r"5", value_type = "int", example = r"max-args = 5")]
pub max_args: Option<usize>,

/// Maximum number of positional arguments allowed for a function or method definition
/// (see: `PLR0917`).
#[option(default = r"3", value_type = "int", example = r"max-pos-args = 3")]
pub max_positional_args: 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 @@ -2663,6 +2668,9 @@ impl PylintOptions {
.unwrap_or(defaults.allow_magic_value_types),
allow_dunder_method_names: self.allow_dunder_method_names.unwrap_or_default(),
max_args: self.max_args.unwrap_or(defaults.max_args),
max_positional_args: self
.max_positional_args
.unwrap_or(defaults.max_positional_args),
max_bool_expr: self.max_bool_expr.unwrap_or(defaults.max_bool_expr),
max_returns: self.max_returns.unwrap_or(defaults.max_returns),
max_branches: self.max_branches.unwrap_or(defaults.max_branches),
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 b90027d

Please sign in to comment.