[refurb] New rule to suggest min/max over sorted() (FURB192)

crates/ruff_linter/resources/test/fixtures/refurb/FURB192.py

@@ -0,0 +1,39 @@
+# Errors
+
+sorted(l)[0]
+
+sorted(l)[-1]
+
+sorted(l, reverse=True)[0]
+
+sorted(l, reverse=True)[-1]
+
+sorted(l, reverse=False)[-1]
+
+sorted(l, key=lambda x: x)[0]
+
+sorted(l, key=key_fn, reverse=True)[-1]
+
+sorted(l, key=key_fn)[0]
+
+# Non-errors
+
+sorted(l, reverse=foo())[0]
+
+sorted([1, 2, 3])[0]
+
+sorted(l)[1]
+
+sorted(get_list())[1]
+
+sorted()[0]
+
+sorted(l)[1]
+
+sorted(l)[-2]
+
+b = True
+
+sorted(l, reverse=b)[0]
+0
+sorted(l, invalid_kwarg=True)[0]

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

@@ -125,6 +125,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
             if checker.enabled(Rule::PotentialIndexError) {
                 pylint::rules::potential_index_error(checker, value, slice);
             }
+            if checker.enabled(Rule::SortedMinMax) {
+                refurb::rules::sorted_min_max(checker, subscript);
+            }
 
             pandas_vet::rules::subscript(checker, value, expr);
         }

crates/ruff_linter/src/codes.rs

@@ -1066,6 +1066,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Refurb, "180") => (RuleGroup::Preview, rules::refurb::rules::MetaClassABCMeta),
         (Refurb, "181") => (RuleGroup::Preview, rules::refurb::rules::HashlibDigestHex),
         (Refurb, "187") => (RuleGroup::Preview, rules::refurb::rules::ListReverseCopy),
+        (Refurb, "192") => (RuleGroup::Preview, rules::refurb::rules::SortedMinMax),
 
         // flake8-logging
         (Flake8Logging, "001") => (RuleGroup::Stable, rules::flake8_logging::rules::DirectLoggerInstantiation),

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

@@ -41,6 +41,7 @@ mod tests {
     #[test_case(Rule::MetaClassABCMeta, Path::new("FURB180.py"))]
     #[test_case(Rule::HashlibDigestHex, Path::new("FURB181.py"))]
     #[test_case(Rule::ListReverseCopy, Path::new("FURB187.py"))]
+    #[test_case(Rule::SortedMinMax, Path::new("FURB192.py"))]
     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_linter/src/rules/refurb/rules/mod.rs

@@ -21,6 +21,7 @@ pub(crate) use reimplemented_starmap::*;
 pub(crate) use repeated_append::*;
 pub(crate) use single_item_membership_test::*;
 pub(crate) use slice_copy::*;
+pub(crate) use sorted_min_max::*;
 pub(crate) use type_none_comparison::*;
 pub(crate) use unnecessary_enumerate::*;
 pub(crate) use unnecessary_from_float::*;
@@ -49,6 +50,7 @@ mod reimplemented_starmap;
 mod repeated_append;
 mod single_item_membership_test;
 mod slice_copy;
+mod sorted_min_max;
 mod type_none_comparison;
 mod unnecessary_enumerate;
 mod unnecessary_from_float;

crates/ruff_linter/src/rules/refurb/rules/sorted_min_max.rs

@@ -0,0 +1,201 @@
+use ruff_diagnostics::Diagnostic;
+use ruff_diagnostics::Edit;
+use ruff_diagnostics::Fix;
+use ruff_diagnostics::FixAvailability;
+use ruff_diagnostics::Violation;
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{self as ast, Expr};
+
+use crate::checkers::ast::Checker;
+use ruff_python_ast::Number;
+use ruff_text_size::Ranged;
+
+/// ## What it does
+/// Checks for uses of `sorted()` to get the min and max values of a list.
+///
+/// ## Why is this bad?
+/// Using `sorted()` to get the min and max values of a list is inefficient.
+///
+/// ## Example
+/// ```python
+/// nums = [3, 1, 4, 1, 5]
+/// lowest = sorted(nums)[0]
+/// highest = sorted(nums)[-1]
+/// highest = sorted(nums, reverse=True)[0]
+/// ```
+///
+/// Use instead:
+/// ```python
+/// nums = [3, 1, 4, 1, 5]
+/// lowest = min(nums)
+/// highest = max(nums)
+/// ```
+///
+/// ## Note
+///
+/// If the original statement uses `reverse=True`, the `min` and `max` replacement will not
+/// be equivalent if the intended result is to get a non-stable min and max.
+///
+/// In other words, `sorted(data, key=itemgetter(0), reverse=True)[0]` is not a stable min,
+/// but `min(data, key=itemgetter(0))` is a stable min.
+///
+/// ## References
+/// - [Python documentation: `min`](https://docs.python.org/3/library/functions.html#min)
+/// - [Python documentation: `max`](https://docs.python.org/3/library/functions.html#max)
+#[violation]
+pub struct SortedMinMax;
+
+impl Violation for SortedMinMax {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Always;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Prefer `min` or `max` over `sorted()` to get the min and max values of a list")
+    }
+
+    fn fix_title(&self) -> Option<String> {
+        Some(format!("Replace with `min` or `max`"))
+    }
+}
+
+/// FURB192
+pub(crate) fn sorted_min_max(checker: &mut Checker, subscript: &ast::ExprSubscript) {
+    if subscript.ctx.is_store() || subscript.ctx.is_del() {
+        return;
+    }
+
+    let ast::ExprSubscript { slice, value, .. } = &subscript;
+
+    // Early return if index is not unary or a number literal.
+    if !(slice.is_number_literal_expr() || slice.is_unary_op_expr()) {
+        return;
+    }
+
+    let Expr::Call(ast::ExprCall {
+        func, arguments, ..
+    }) = value.as_ref()
+    else {
+        return;
+    };
+
+    // Check if the value is a call to `sorted()`.
+    if !matches!(func.as_ref(), Expr::Name(name) if name.id == "sorted" && checker.semantic().is_builtin(name.id.as_str()))
+    {
+        return;
+    };
+
+    let index = match slice.as_ref() {
+        // [0]
+        Expr::NumberLiteral(ast::ExprNumberLiteral {
+            value: Number::Int(index),
+            ..
+        }) if *index == 0 => Index::First,
+
+        // [-1]
+        Expr::UnaryOp(ast::ExprUnaryOp {
+            op: ast::UnaryOp::USub,
+            operand,
+            ..
+        }) => {
+            match operand.as_ref() {
+                // [-1]
+                Expr::NumberLiteral(ast::ExprNumberLiteral {
+                    value: Number::Int(index),
+                    ..
+                }) if *index == 1 => Index::Last,
+                _ => return,
+            }
+        }
+        _ => return,
+    };
+
+    let mut reverse_keyword = None;
+    let mut key_keyword_expr = None;
+
+    // Check if the call to `sorted()` has the `reverse` and `key` keywords.
+    for keyword in arguments.keywords.iter() {
+        if reverse_keyword.is_none()
+            && keyword
+                .arg
+                .as_ref()
+                .map_or(false, |arg| arg.as_str() == "reverse")
+        {
+            reverse_keyword = Some(keyword);
+        } else if key_keyword_expr.is_none()
+            && keyword
+                .arg
+                .as_ref()
+                .map_or(false, |arg| arg.as_str() == "key")
+        {
+            key_keyword_expr = Some(&keyword.value);
+        } else {
+            // If unexpected keyword is found, return.
+            return;
+        }
+    }
+
+    let is_reversed = if let Some(reverse_keyword) = reverse_keyword {
+        match reverse_keyword.value.as_boolean_literal_expr() {
+            Some(ast::ExprBooleanLiteral { value, .. }) => *value,
+            // If the value is not a boolean literal, we can't determine if it is reversed.
+            _ => return,
+        }
+    } else {
+        // No reverse keyword, so it is not reversed.
+        false
+    };
+
+    let min_max = match (index, is_reversed) {
+        (Index::First, false) => MinMax::Min,
+        (Index::First, true) => MinMax::Max,
+        (Index::Last, false) => MinMax::Max,
+        (Index::Last, true) => MinMax::Min,
+    };
+
+    // Get the list expression.
+    let Some(Expr::Name(list_expr)) = arguments.args.first() else {
+        return;
+    };
+
+    let replacement = if let Some(key) = &key_keyword_expr {
+        format!(
+            "{}({}, key={})",
+            min_max.as_str(),
+            list_expr.id,
+            checker.generator().expr(key)
+        )
+    } else {
+        format!("{}({})", min_max.as_str(), list_expr.id)
+    };
+
+    let mut diagnostic = Diagnostic::new(SortedMinMax, subscript.range());
+    diagnostic.set_fix(Fix::safe_edit(Edit::replacement(
+        replacement,
+        subscript.start(),
+        subscript.end(),
+    )));
+    checker.diagnostics.push(diagnostic);
+}
+
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+enum MinMax {
+    Min,
+    Max,
+}
+
+impl MinMax {
+    fn as_str(self) -> &'static str {
+        match self {
+            Self::Min => "min",
+            Self::Max => "max",
+        }
+    }
+}
+
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+enum Index {
+    // 0
+    First,
+    // -1
+    Last,
+}