Skip to content

Commit b90027d

Browse files
authored
[pylint] Implement too-many-positional (PLR0917) (#8995)
## 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
1 parent 060a25d commit b90027d

12 files changed

+209
-0
lines changed
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
def f(x, y, z, t, u, v, w, r): # Too many positional arguments (8/3)
2+
pass
3+
4+
5+
def f(x): # OK
6+
pass
7+
8+
9+
def f(x, y, z, _t, _u, _v, _w, r): # OK (underscore-prefixed names are ignored
10+
pass
11+
12+
13+
def f(x, y, z, *, u=1, v=1, r=1): # OK
14+
pass
15+
16+
17+
def f(x=1, y=1, z=1): # OK
18+
pass
19+
20+
21+
def f(x, y, z, /, u, v, w): # Too many positional arguments (6/3)
22+
pass
23+
24+
25+
def f(x, y, z, *, u, v, w): # OK
26+
pass
27+
28+
29+
def f(x, y, z, a, b, c, *, u, v, w): # Too many positional arguments (6/3)
30+
pass
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
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):
4+
pass
5+
6+
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):
10+
pass

crates/ruff_linter/src/checkers/ast/analyze/statement.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
250250
if checker.enabled(Rule::TooManyArguments) {
251251
pylint::rules::too_many_arguments(checker, function_def);
252252
}
253+
if checker.enabled(Rule::TooManyPositional) {
254+
pylint::rules::too_many_positional(checker, parameters, stmt);
255+
}
253256
if checker.enabled(Rule::TooManyReturnStatements) {
254257
if let Some(diagnostic) = pylint::rules::too_many_return_statements(
255258
stmt,

crates/ruff_linter/src/codes.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
254254
(Pylint, "R0913") => (RuleGroup::Stable, rules::pylint::rules::TooManyArguments),
255255
(Pylint, "R0915") => (RuleGroup::Stable, rules::pylint::rules::TooManyStatements),
256256
(Pylint, "R0916") => (RuleGroup::Preview, rules::pylint::rules::TooManyBooleanExpressions),
257+
(Pylint, "R0917") => (RuleGroup::Preview, rules::pylint::rules::TooManyPositional),
257258
(Pylint, "R1701") => (RuleGroup::Stable, rules::pylint::rules::RepeatedIsinstanceCalls),
258259
(Pylint, "R1704") => (RuleGroup::Preview, rules::pylint::rules::RedefinedArgumentFromLocal),
259260
(Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn),

crates/ruff_linter/src/rules/pylint/mod.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ mod tests {
9494
#[test_case(Rule::RedefinedLoopName, Path::new("redefined_loop_name.py"))]
9595
#[test_case(Rule::ReturnInInit, Path::new("return_in_init.py"))]
9696
#[test_case(Rule::TooManyArguments, Path::new("too_many_arguments.py"))]
97+
#[test_case(Rule::TooManyPositional, Path::new("too_many_positional.py"))]
9798
#[test_case(Rule::TooManyBranches, Path::new("too_many_branches.py"))]
9899
#[test_case(
99100
Rule::TooManyReturnStatements,
@@ -249,6 +250,22 @@ mod tests {
249250
Ok(())
250251
}
251252

253+
#[test]
254+
fn max_positional_args() -> Result<()> {
255+
let diagnostics = test_path(
256+
Path::new("pylint/too_many_positional_params.py"),
257+
&LinterSettings {
258+
pylint: pylint::settings::Settings {
259+
max_positional_args: 4,
260+
..pylint::settings::Settings::default()
261+
},
262+
..LinterSettings::for_rule(Rule::TooManyPositional)
263+
},
264+
)?;
265+
assert_messages!(diagnostics);
266+
Ok(())
267+
}
268+
252269
#[test]
253270
fn max_branches() -> Result<()> {
254271
let diagnostics = test_path(

crates/ruff_linter/src/rules/pylint/rules/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ pub(crate) use sys_exit_alias::*;
5555
pub(crate) use too_many_arguments::*;
5656
pub(crate) use too_many_boolean_expressions::*;
5757
pub(crate) use too_many_branches::*;
58+
pub(crate) use too_many_positional::*;
5859
pub(crate) use too_many_public_methods::*;
5960
pub(crate) use too_many_return_statements::*;
6061
pub(crate) use too_many_statements::*;
@@ -131,6 +132,7 @@ mod sys_exit_alias;
131132
mod too_many_arguments;
132133
mod too_many_boolean_expressions;
133134
mod too_many_branches;
135+
mod too_many_positional;
134136
mod too_many_public_methods;
135137
mod too_many_return_statements;
136138
mod too_many_statements;
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
use ruff_diagnostics::{Diagnostic, Violation};
2+
use ruff_macros::{derive_message_formats, violation};
3+
use ruff_python_ast::{identifier::Identifier, Parameters, Stmt};
4+
5+
use crate::checkers::ast::Checker;
6+
7+
/// ## What it does
8+
/// Checks for function definitions that include too many positional arguments.
9+
///
10+
/// By default, this rule allows up to five arguments, as configured by the
11+
/// [`pylint.max-positional-args`] option.
12+
///
13+
/// ## Why is this bad?
14+
/// Functions with many arguments are harder to understand, maintain, and call.
15+
/// This is especially true for functions with many positional arguments, as
16+
/// providing arguments positionally is more error-prone and less clear to
17+
/// readers than providing arguments by name.
18+
///
19+
/// Consider refactoring functions with many arguments into smaller functions
20+
/// with fewer arguments, using objects to group related arguments, or
21+
/// migrating to keyword-only arguments.
22+
///
23+
/// ## Example
24+
/// ```python
25+
/// def plot(x, y, z, color, mark, add_trendline):
26+
/// ...
27+
///
28+
///
29+
/// plot(1, 2, 3, "r", "*", True)
30+
/// ```
31+
///
32+
/// Use instead:
33+
/// ```python
34+
/// def plot(x, y, z, *, color, mark, add_trendline):
35+
/// ...
36+
///
37+
///
38+
/// plot(1, 2, 3, color="r", mark="*", add_trendline=True)
39+
/// ```
40+
///
41+
/// ## Options
42+
/// - `pylint.max-positional-args`
43+
#[violation]
44+
pub struct TooManyPositional {
45+
c_pos: usize,
46+
max_pos: usize,
47+
}
48+
49+
impl Violation for TooManyPositional {
50+
#[derive_message_formats]
51+
fn message(&self) -> String {
52+
let TooManyPositional { c_pos, max_pos } = self;
53+
format!("Too many positional arguments: ({c_pos}/{max_pos})")
54+
}
55+
}
56+
57+
/// PLR0917
58+
pub(crate) fn too_many_positional(checker: &mut Checker, parameters: &Parameters, stmt: &Stmt) {
59+
let num_positional_args = parameters
60+
.args
61+
.iter()
62+
.chain(&parameters.posonlyargs)
63+
.filter(|arg| {
64+
!checker
65+
.settings
66+
.dummy_variable_rgx
67+
.is_match(&arg.parameter.name)
68+
})
69+
.count();
70+
if num_positional_args > checker.settings.pylint.max_positional_args {
71+
checker.diagnostics.push(Diagnostic::new(
72+
TooManyPositional {
73+
c_pos: num_positional_args,
74+
max_pos: checker.settings.pylint.max_positional_args,
75+
},
76+
stmt.identifier(),
77+
));
78+
}
79+
}

crates/ruff_linter/src/rules/pylint/settings.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ pub struct Settings {
3939
pub allow_magic_value_types: Vec<ConstantType>,
4040
pub allow_dunder_method_names: FxHashSet<String>,
4141
pub max_args: usize,
42+
pub max_positional_args: usize,
4243
pub max_returns: usize,
4344
pub max_bool_expr: usize,
4445
pub max_branches: usize,
@@ -52,6 +53,7 @@ impl Default for Settings {
5253
allow_magic_value_types: vec![ConstantType::Str, ConstantType::Bytes],
5354
allow_dunder_method_names: FxHashSet::default(),
5455
max_args: 5,
56+
max_positional_args: 5,
5557
max_returns: 6,
5658
max_bool_expr: 5,
5759
max_branches: 12,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
---
2+
source: crates/ruff_linter/src/rules/pylint/mod.rs
3+
---
4+
too_many_positional.py:1:5: PLR0917 Too many positional arguments: (8/5)
5+
|
6+
1 | def f(x, y, z, t, u, v, w, r): # Too many positional arguments (8/3)
7+
| ^ PLR0917
8+
2 | pass
9+
|
10+
11+
too_many_positional.py:21:5: PLR0917 Too many positional arguments: (6/5)
12+
|
13+
21 | def f(x, y, z, /, u, v, w): # Too many positional arguments (6/3)
14+
| ^ PLR0917
15+
22 | pass
16+
|
17+
18+
too_many_positional.py:29:5: PLR0917 Too many positional arguments: (6/5)
19+
|
20+
29 | def f(x, y, z, a, b, c, *, u, v, w): # Too many positional arguments (6/3)
21+
| ^ PLR0917
22+
30 | pass
23+
|
24+
25+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
source: crates/ruff_linter/src/rules/pylint/mod.rs
3+
---
4+
too_many_positional_params.py:3:5: PLR0917 Too many positional arguments: (7/4)
5+
|
6+
1 | # Too many positional arguments (7/4) for max_positional=4
7+
2 | # OK for dummy_variable_rgx ~ "skip_.*"
8+
3 | def f(w, x, y, z, skip_t, skip_u, skip_v):
9+
| ^ PLR0917
10+
4 | pass
11+
|
12+
13+
too_many_positional_params.py:9:5: PLR0917 Too many positional arguments: (7/4)
14+
|
15+
7 | # Too many positional arguments (7/4) for max_args=4
16+
8 | # Too many positional arguments (7/3) for dummy_variable_rgx ~ "skip_.*"
17+
9 | def f(w, x, y, z, t, u, v):
18+
| ^ PLR0917
19+
10 | pass
20+
|
21+
22+

crates/ruff_workspace/src/options.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2635,6 +2635,11 @@ pub struct PylintOptions {
26352635
#[option(default = r"5", value_type = "int", example = r"max-args = 5")]
26362636
pub max_args: Option<usize>,
26372637

2638+
/// Maximum number of positional arguments allowed for a function or method definition
2639+
/// (see: `PLR0917`).
2640+
#[option(default = r"3", value_type = "int", example = r"max-pos-args = 3")]
2641+
pub max_positional_args: Option<usize>,
2642+
26382643
/// Maximum number of statements allowed for a function or method body (see:
26392644
/// `PLR0915`).
26402645
#[option(default = r"50", value_type = "int", example = r"max-statements = 50")]
@@ -2663,6 +2668,9 @@ impl PylintOptions {
26632668
.unwrap_or(defaults.allow_magic_value_types),
26642669
allow_dunder_method_names: self.allow_dunder_method_names.unwrap_or_default(),
26652670
max_args: self.max_args.unwrap_or(defaults.max_args),
2671+
max_positional_args: self
2672+
.max_positional_args
2673+
.unwrap_or(defaults.max_positional_args),
26662674
max_bool_expr: self.max_bool_expr.unwrap_or(defaults.max_bool_expr),
26672675
max_returns: self.max_returns.unwrap_or(defaults.max_returns),
26682676
max_branches: self.max_branches.unwrap_or(defaults.max_branches),

ruff.schema.json

Lines changed: 10 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)