From 3beff290267407b7eef461ebaf4a0c3bffe23287 Mon Sep 17 00:00:00 2001 From: Trevor McCulloch Date: Sat, 6 May 2023 20:14:14 -0700 Subject: [PATCH] [`pylint`] Implement `nested-min-max` (`W3301`) (#4200) --- .../test/fixtures/pylint/nested_min_max.py | 21 ++ crates/ruff/src/checkers/ast/mod.rs | 3 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/pylint/mod.rs | 1 + crates/ruff/src/rules/pylint/rules/mod.rs | 2 + .../src/rules/pylint/rules/nested_min_max.rs | 131 ++++++++++++ ...int__tests__PLW3301_nested_min_max.py.snap | 196 ++++++++++++++++++ ruff.schema.json | 4 + 9 files changed, 360 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/pylint/nested_min_max.py create mode 100644 crates/ruff/src/rules/pylint/rules/nested_min_max.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW3301_nested_min_max.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/nested_min_max.py b/crates/ruff/resources/test/fixtures/pylint/nested_min_max.py new file mode 100644 index 0000000000..9ae58c1241 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/nested_min_max.py @@ -0,0 +1,21 @@ +min(1, 2, 3) +min(1, min(2, 3)) +min(1, min(2, min(3, 4))) +min(1, foo("a", "b"), min(3, 4)) +min(1, max(2, 3)) +max(1, 2, 3) +max(1, max(2, 3)) +max(1, max(2, max(3, 4))) +max(1, foo("a", "b"), max(3, 4)) + +# These should not trigger; we do not flag cases with keyword args. +min(1, min(2, 3), key=test) +min(1, min(2, 3, key=test)) +# This will still trigger, to merge the calls without keyword args. +min(1, min(2, 3, key=test), min(4, 5)) + +# Don't provide a fix if there are comments within the call. +min( + 1, # This is a comment. + min(2, 3), +) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 693a8c759c..0d9e73574b 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -3000,6 +3000,9 @@ where if self.settings.rules.enabled(Rule::InvalidEnvvarValue) { pylint::rules::invalid_envvar_value(self, func, args, keywords); } + if self.settings.rules.enabled(Rule::NestedMinMax) { + pylint::rules::nested_min_max(self, expr, func, args, keywords); + } // flake8-pytest-style if self.settings.rules.enabled(Rule::PytestPatchWithLambda) { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 9ae775ba70..ff97071073 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -212,6 +212,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Pylint, "W1508") => Rule::InvalidEnvvarDefault, (Pylint, "W2901") => Rule::RedefinedLoopName, (Pylint, "E0302") => Rule::UnexpectedSpecialMethodSignature, + (Pylint, "W3301") => Rule::NestedMinMax, // flake8-builtins (Flake8Builtins, "001") => Rule::BuiltinVariableShadowing, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index cc342b9d0e..9124057733 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -189,6 +189,7 @@ ruff_macros::register_rules!( rules::pylint::rules::LoggingTooFewArgs, rules::pylint::rules::LoggingTooManyArgs, rules::pylint::rules::UnexpectedSpecialMethodSignature, + rules::pylint::rules::NestedMinMax, // flake8-builtins rules::flake8_builtins::rules::BuiltinVariableShadowing, rules::flake8_builtins::rules::BuiltinArgumentShadowing, diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index 2a06e513d2..be74e2b068 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -71,6 +71,7 @@ mod tests { #[test_case(Rule::UselessImportAlias, Path::new("import_aliasing.py"); "PLC0414")] #[test_case(Rule::UselessReturn, Path::new("useless_return.py"); "PLR1711")] #[test_case(Rule::YieldInInit, Path::new("yield_in_init.py"); "PLE0100")] + #[test_case(Rule::NestedMinMax, Path::new("nested_min_max.py"); "PLW3301")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index ed332ee4db..3eedc61243 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -25,6 +25,7 @@ pub use load_before_global_declaration::{ pub use logging::{logging_call, LoggingTooFewArgs, LoggingTooManyArgs}; pub use magic_value_comparison::{magic_value_comparison, MagicValueComparison}; pub use manual_import_from::{manual_from_import, ManualFromImport}; +pub use nested_min_max::{nested_min_max, NestedMinMax}; pub use nonlocal_without_binding::NonlocalWithoutBinding; pub use property_with_parameters::{property_with_parameters, PropertyWithParameters}; pub use redefined_loop_name::{redefined_loop_name, RedefinedLoopName}; @@ -68,6 +69,7 @@ mod load_before_global_declaration; mod logging; mod magic_value_comparison; mod manual_import_from; +mod nested_min_max; mod nonlocal_without_binding; mod property_with_parameters; mod redefined_loop_name; diff --git a/crates/ruff/src/rules/pylint/rules/nested_min_max.rs b/crates/ruff/src/rules/pylint/rules/nested_min_max.rs new file mode 100644 index 0000000000..09eb5d078a --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/nested_min_max.rs @@ -0,0 +1,131 @@ +use ruff_text_size::TextSize; +use rustpython_parser::ast::{Expr, ExprKind, Keyword}; + +use ruff_diagnostics::{Diagnostic, Edit, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::{has_comments, unparse_expr}; +use ruff_python_semantic::context::Context; + +use crate::{checkers::ast::Checker, registry::AsRule}; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum MinMax { + Min, + Max, +} + +#[violation] +pub struct NestedMinMax { + func: MinMax, + fixable: bool, +} + +impl Violation for NestedMinMax { + #[derive_message_formats] + fn message(&self) -> String { + format!("Nested `{}` calls can be flattened", self.func) + } + + fn autofix_title_formatter(&self) -> Option String> { + self.fixable + .then_some(|NestedMinMax { func, .. }| format!("Flatten nested `{func}` calls")) + } +} + +impl MinMax { + /// Converts a function call [`Expr`] into a [`MinMax`] if it is a call to `min` or `max`. + fn try_from_call(func: &Expr, keywords: &[Keyword], context: &Context) -> Option { + if !keywords.is_empty() { + return None; + } + let ExprKind::Name { id, .. } = func.node() else { + return None; + }; + if id == "min" && context.is_builtin("min") { + Some(MinMax::Min) + } else if id == "max" && context.is_builtin("max") { + Some(MinMax::Max) + } else { + None + } + } +} + +impl std::fmt::Display for MinMax { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + MinMax::Min => write!(f, "min"), + MinMax::Max => write!(f, "max"), + } + } +} + +/// Collect a new set of arguments to by either accepting existing args as-is or +/// collecting child arguments, if it's a call to the same function. +fn collect_nested_args(context: &Context, min_max: MinMax, args: &[Expr]) -> Vec { + fn inner(context: &Context, min_max: MinMax, args: &[Expr], new_args: &mut Vec) { + for arg in args { + if let ExprKind::Call { + func, + args, + keywords, + } = arg.node() + { + if MinMax::try_from_call(func, keywords, context) == Some(min_max) { + inner(context, min_max, args, new_args); + continue; + } + } + new_args.push(arg.clone()); + } + } + + let mut new_args = Vec::with_capacity(args.len()); + inner(context, min_max, args, &mut new_args); + new_args +} + +/// W3301 +pub fn nested_min_max( + checker: &mut Checker, + expr: &Expr, + func: &Expr, + args: &[Expr], + keywords: &[Keyword], +) { + let Some(min_max) = MinMax::try_from_call(func, keywords, &checker.ctx) else { + return; + }; + + if args.iter().any(|arg| { + let ExprKind::Call { func, keywords, ..} = arg.node() else { + return false; + }; + MinMax::try_from_call(func, keywords, &checker.ctx) == Some(min_max) + }) { + let fixable = !has_comments(expr, checker.locator); + let mut diagnostic = Diagnostic::new( + NestedMinMax { + func: min_max, + fixable, + }, + expr.range(), + ); + if fixable && checker.patch(diagnostic.kind.rule()) { + let flattened_expr = Expr::new( + TextSize::default(), + TextSize::default(), + ExprKind::Call { + func: Box::new(func.clone()), + args: collect_nested_args(&checker.ctx, min_max, args), + keywords: keywords.to_owned(), + }, + ); + diagnostic.set_fix(Edit::range_replacement( + unparse_expr(&flattened_expr, checker.stylist), + expr.range(), + )); + } + checker.diagnostics.push(diagnostic); + } +} diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW3301_nested_min_max.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW3301_nested_min_max.py.snap new file mode 100644 index 0000000000..73e80d040f --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW3301_nested_min_max.py.snap @@ -0,0 +1,196 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +--- +nested_min_max.py:2:1: PLW3301 [*] Nested `min` calls can be flattened + | +2 | min(1, 2, 3) +3 | min(1, min(2, 3)) + | ^^^^^^^^^^^^^^^^^ PLW3301 +4 | min(1, min(2, min(3, 4))) +5 | min(1, foo("a", "b"), min(3, 4)) + | + = help: Flatten nested `min` calls + +ℹ Suggested fix +1 1 | min(1, 2, 3) +2 |-min(1, min(2, 3)) + 2 |+min(1, 2, 3) +3 3 | min(1, min(2, min(3, 4))) +4 4 | min(1, foo("a", "b"), min(3, 4)) +5 5 | min(1, max(2, 3)) + +nested_min_max.py:3:1: PLW3301 [*] Nested `min` calls can be flattened + | +3 | min(1, 2, 3) +4 | min(1, min(2, 3)) +5 | min(1, min(2, min(3, 4))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ PLW3301 +6 | min(1, foo("a", "b"), min(3, 4)) +7 | min(1, max(2, 3)) + | + = help: Flatten nested `min` calls + +ℹ Suggested fix +1 1 | min(1, 2, 3) +2 2 | min(1, min(2, 3)) +3 |-min(1, min(2, min(3, 4))) + 3 |+min(1, 2, 3, 4) +4 4 | min(1, foo("a", "b"), min(3, 4)) +5 5 | min(1, max(2, 3)) +6 6 | max(1, 2, 3) + +nested_min_max.py:3:8: PLW3301 [*] Nested `min` calls can be flattened + | +3 | min(1, 2, 3) +4 | min(1, min(2, 3)) +5 | min(1, min(2, min(3, 4))) + | ^^^^^^^^^^^^^^^^^ PLW3301 +6 | min(1, foo("a", "b"), min(3, 4)) +7 | min(1, max(2, 3)) + | + = help: Flatten nested `min` calls + +ℹ Suggested fix +1 1 | min(1, 2, 3) +2 2 | min(1, min(2, 3)) +3 |-min(1, min(2, min(3, 4))) + 3 |+min(1, min(2, 3, 4)) +4 4 | min(1, foo("a", "b"), min(3, 4)) +5 5 | min(1, max(2, 3)) +6 6 | max(1, 2, 3) + +nested_min_max.py:4:1: PLW3301 [*] Nested `min` calls can be flattened + | +4 | min(1, min(2, 3)) +5 | min(1, min(2, min(3, 4))) +6 | min(1, foo("a", "b"), min(3, 4)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW3301 +7 | min(1, max(2, 3)) +8 | max(1, 2, 3) + | + = help: Flatten nested `min` calls + +ℹ Suggested fix +1 1 | min(1, 2, 3) +2 2 | min(1, min(2, 3)) +3 3 | min(1, min(2, min(3, 4))) +4 |-min(1, foo("a", "b"), min(3, 4)) + 4 |+min(1, foo("a", "b"), 3, 4) +5 5 | min(1, max(2, 3)) +6 6 | max(1, 2, 3) +7 7 | max(1, max(2, 3)) + +nested_min_max.py:7:1: PLW3301 [*] Nested `max` calls can be flattened + | + 7 | min(1, max(2, 3)) + 8 | max(1, 2, 3) + 9 | max(1, max(2, 3)) + | ^^^^^^^^^^^^^^^^^ PLW3301 +10 | max(1, max(2, max(3, 4))) +11 | max(1, foo("a", "b"), max(3, 4)) + | + = help: Flatten nested `max` calls + +ℹ Suggested fix +4 4 | min(1, foo("a", "b"), min(3, 4)) +5 5 | min(1, max(2, 3)) +6 6 | max(1, 2, 3) +7 |-max(1, max(2, 3)) + 7 |+max(1, 2, 3) +8 8 | max(1, max(2, max(3, 4))) +9 9 | max(1, foo("a", "b"), max(3, 4)) +10 10 | + +nested_min_max.py:8:1: PLW3301 [*] Nested `max` calls can be flattened + | + 8 | max(1, 2, 3) + 9 | max(1, max(2, 3)) +10 | max(1, max(2, max(3, 4))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ PLW3301 +11 | max(1, foo("a", "b"), max(3, 4)) + | + = help: Flatten nested `max` calls + +ℹ Suggested fix +5 5 | min(1, max(2, 3)) +6 6 | max(1, 2, 3) +7 7 | max(1, max(2, 3)) +8 |-max(1, max(2, max(3, 4))) + 8 |+max(1, 2, 3, 4) +9 9 | max(1, foo("a", "b"), max(3, 4)) +10 10 | +11 11 | # These should not trigger; we do not flag cases with keyword args. + +nested_min_max.py:8:8: PLW3301 [*] Nested `max` calls can be flattened + | + 8 | max(1, 2, 3) + 9 | max(1, max(2, 3)) +10 | max(1, max(2, max(3, 4))) + | ^^^^^^^^^^^^^^^^^ PLW3301 +11 | max(1, foo("a", "b"), max(3, 4)) + | + = help: Flatten nested `max` calls + +ℹ Suggested fix +5 5 | min(1, max(2, 3)) +6 6 | max(1, 2, 3) +7 7 | max(1, max(2, 3)) +8 |-max(1, max(2, max(3, 4))) + 8 |+max(1, max(2, 3, 4)) +9 9 | max(1, foo("a", "b"), max(3, 4)) +10 10 | +11 11 | # These should not trigger; we do not flag cases with keyword args. + +nested_min_max.py:9:1: PLW3301 [*] Nested `max` calls can be flattened + | + 9 | max(1, max(2, 3)) +10 | max(1, max(2, max(3, 4))) +11 | max(1, foo("a", "b"), max(3, 4)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW3301 +12 | +13 | # These should not trigger; we do not flag cases with keyword args. + | + = help: Flatten nested `max` calls + +ℹ Suggested fix +6 6 | max(1, 2, 3) +7 7 | max(1, max(2, 3)) +8 8 | max(1, max(2, max(3, 4))) +9 |-max(1, foo("a", "b"), max(3, 4)) + 9 |+max(1, foo("a", "b"), 3, 4) +10 10 | +11 11 | # These should not trigger; we do not flag cases with keyword args. +12 12 | min(1, min(2, 3), key=test) + +nested_min_max.py:15:1: PLW3301 [*] Nested `min` calls can be flattened + | +15 | min(1, min(2, 3, key=test)) +16 | # This will still trigger, to merge the calls without keyword args. +17 | min(1, min(2, 3, key=test), min(4, 5)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW3301 +18 | +19 | # Don't provide a fix if there are comments within the call. + | + = help: Flatten nested `min` calls + +ℹ Suggested fix +12 12 | min(1, min(2, 3), key=test) +13 13 | min(1, min(2, 3, key=test)) +14 14 | # This will still trigger, to merge the calls without keyword args. +15 |-min(1, min(2, 3, key=test), min(4, 5)) + 15 |+min(1, min(2, 3, key=test), 4, 5) +16 16 | +17 17 | # Don't provide a fix if there are comments within the call. +18 18 | min( + +nested_min_max.py:18:1: PLW3301 Nested `min` calls can be flattened + | +18 | # Don't provide a fix if there are comments within the call. +19 | / min( +20 | | 1, # This is a comment. +21 | | min(2, 3), +22 | | ) + | |_^ PLW3301 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index a39b5d603d..3bb39c2611 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2051,6 +2051,10 @@ "PLW29", "PLW290", "PLW2901", + "PLW3", + "PLW33", + "PLW330", + "PLW3301", "PT", "PT0", "PT00",