[pylint] Implement nested-min-max (W3301) (#4200)

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),
+)

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) {

crates/ruff/src/codes.rs

@@ -212,6 +212,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
         (Pylint, "W1508") => Rule::InvalidEnvvarDefault,
         (Pylint, "W2901") => Rule::RedefinedLoopName,
         (Pylint, "E0302") => Rule::UnexpectedSpecialMethodSignature,
+        (Pylint, "W3301") => Rule::NestedMinMax,
 
         // flake8-builtins
         (Flake8Builtins, "001") => Rule::BuiltinVariableShadowing,

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,

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(

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;

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<fn(&Self) -> 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<MinMax> {
+        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<Expr> {
+    fn inner(context: &Context, min_max: MinMax, args: &[Expr], new_args: &mut Vec<Expr>) {
+        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);
+    }
+}