diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/too_many_positional.py b/crates/ruff_linter/resources/test/fixtures/pylint/too_many_positional.py new file mode 100644 index 0000000000000..3c0fe2a2f5fb4 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/too_many_positional.py @@ -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 diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/too_many_positional_params.py b/crates/ruff_linter/resources/test/fixtures/pylint/too_many_positional_params.py new file mode 100644 index 0000000000000..dae87b5737a2c --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/too_many_positional_params.py @@ -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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 4b79a9ec7da14..0f05d6165b22e 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -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, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index cb1f907b658e0..fd0b85727bbec 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 20920043f97b6..3933e07bf9e02 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -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, @@ -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( diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 02ae6796a5dfb..aaedd82e2f790 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_positional::*; pub(crate) use too_many_public_methods::*; pub(crate) use too_many_return_statements::*; pub(crate) use too_many_statements::*; @@ -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; diff --git a/crates/ruff_linter/src/rules/pylint/rules/too_many_positional.rs b/crates/ruff_linter/src/rules/pylint/rules/too_many_positional.rs new file mode 100644 index 0000000000000..369b6c63bc6e9 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/too_many_positional.rs @@ -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(¶meters.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(), + )); + } +} diff --git a/crates/ruff_linter/src/rules/pylint/settings.rs b/crates/ruff_linter/src/rules/pylint/settings.rs index cb9846b11e056..020390704c7f8 100644 --- a/crates/ruff_linter/src/rules/pylint/settings.rs +++ b/crates/ruff_linter/src/rules/pylint/settings.rs @@ -39,6 +39,7 @@ pub struct Settings { pub allow_magic_value_types: Vec, pub allow_dunder_method_names: FxHashSet, pub max_args: usize, + pub max_positional_args: usize, pub max_returns: usize, pub max_bool_expr: usize, pub max_branches: usize, @@ -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, diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0917_too_many_positional.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0917_too_many_positional.py.snap new file mode 100644 index 0000000000000..4578419f43545 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0917_too_many_positional.py.snap @@ -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 + | + + diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__max_positional_args.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__max_positional_args.snap new file mode 100644 index 0000000000000..98c964820770c --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__max_positional_args.snap @@ -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 + | + + diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 05f4522710cd9..f87a5223d9aed 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -2635,6 +2635,11 @@ pub struct PylintOptions { #[option(default = r"5", value_type = "int", example = r"max-args = 5")] pub max_args: Option, + /// 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, + /// 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")] @@ -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), diff --git a/ruff.schema.json b/ruff.schema.json index b0584cad84735..d962edc15e141 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2369,6 +2369,15 @@ "format": "uint", "minimum": 0.0 }, + "max-positional-args": { + "description": "Maximum number of positional arguments allowed for a function or method definition (see: `PLR0917`).", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 + }, "max-public-methods": { "description": "Maximum number of public methods allowed for a class (see: `PLR0904`).", "type": [ @@ -3117,6 +3126,7 @@ "PLR0913", "PLR0915", "PLR0916", + "PLR0917", "PLR1", "PLR17", "PLR170",