Fix nested calls to sorted with differing arguments (#5761)

## Summary Nested calls to `sorted` can only be collapsed if the calls are identical (i.e., they have the exact same keyword arguments). Update C414 to only flag such cases. Fixes #5712 ## Test Plan Updated snapshots. Tested against flake8-comprehensions. It incorrectly flags these cases.
astral-sh · Jul 19, 2023 · a1aedfe · a1aedfe
1 parent fad919a
commit a1aedfe
Show file tree

Hide file tree

Showing 4 changed files with 123 additions and 68 deletions.
diff --git a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C414.py b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C414.py
@@ -12,7 +12,8 @@
 sorted(list(x))
 sorted(tuple(x))
 sorted(sorted(x))
-sorted(sorted(x, key=lambda y: y))
+sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+sorted(sorted(x, reverse=True), reverse=True)
 sorted(reversed(x))
 sorted(list(x), key=lambda y: y)
 tuple(
@@ -21,3 +22,9 @@
         "o"]
     )
 )
+
+# Nested sorts with differing keyword arguments. Not flagged.
+sorted(sorted(x, key=lambda y: y))
+sorted(sorted(x, key=lambda y: y), key=lambda x: x)
+sorted(sorted(x), reverse=True)
+sorted(sorted(x, reverse=False), reverse=True)
diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs
@@ -2772,7 +2772,7 @@ where
                 }
                 if self.enabled(Rule::UnnecessaryDoubleCastOrProcess) {
                     flake8_comprehensions::rules::unnecessary_double_cast_or_process(
-                        self, expr, func, args,
+                        self, expr, func, args, keywords,
                     );
                 }
                 if self.enabled(Rule::UnnecessarySubscriptReversal) {

diff --git a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_double_cast_or_process.rs b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_double_cast_or_process.rs
@@ -1,7 +1,8 @@
-use rustpython_parser::ast::{self, Expr, Ranged};
+use rustpython_parser::ast::{self, Expr, Keyword, Ranged};
 
 use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic};
 use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::comparable::ComparableKeyword;
 
 use crate::checkers::ast::Checker;
 use crate::registry::AsRule;
@@ -69,6 +70,7 @@ pub(crate) fn unnecessary_double_cast_or_process(
     expr: &Expr,
     func: &Expr,
     args: &[Expr],
+    outer_kw: &[Keyword],
 ) {
     let Some(outer) = helpers::expr_name(func) else {
         return;
@@ -84,7 +86,12 @@ pub(crate) fn unnecessary_double_cast_or_process(
     let Some(arg) = args.first() else {
         return;
     };
-    let Expr::Call(ast::ExprCall { func, .. }) = arg else {
+    let Expr::Call(ast::ExprCall {
+        func,
+        keywords: inner_kw,
+        ..
+    }) = arg
+    else {
         return;
     };
     let Some(inner) = helpers::expr_name(func) else {
@@ -94,6 +101,21 @@ pub(crate) fn unnecessary_double_cast_or_process(
         return;
     }
 
+    // Avoid collapsing nested `sorted` calls with non-identical keyword arguments
+    // (i.e., `key`, `reverse`).
+    if inner == "sorted" && outer == "sorted" {
+        if inner_kw.len() != outer_kw.len() {
+            return;
+        }
+        if !inner_kw.iter().all(|inner| {
+            outer_kw
+                .iter()
+                .any(|outer| ComparableKeyword::from(inner) == ComparableKeyword::from(outer))
+        }) {
+            return;
+        }
+    }
+
     // Ex) set(tuple(...))
     // Ex) list(tuple(...))
     // Ex) set(set(...))

diff --git a/...ke8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C414_C414.py.snap b/...ke8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C414_C414.py.snap
@@ -226,7 +226,7 @@ C414.py:12:1: C414 [*] Unnecessary `list` call within `sorted()`
    12 |+sorted(x)
 13 13 | sorted(tuple(x))
 14 14 | sorted(sorted(x))
-15 15 | sorted(sorted(x, key=lambda y: y))
+15 15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
 
 C414.py:13:1: C414 [*] Unnecessary `tuple` call within `sorted()`
    |
@@ -235,7 +235,7 @@ C414.py:13:1: C414 [*] Unnecessary `tuple` call within `sorted()`
 13 | sorted(tuple(x))
    | ^^^^^^^^^^^^^^^^ C414
 14 | sorted(sorted(x))
-15 | sorted(sorted(x, key=lambda y: y))
+15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
    |
    = help: Remove the inner `tuple` call
 
@@ -246,17 +246,17 @@ C414.py:13:1: C414 [*] Unnecessary `tuple` call within `sorted()`
 13    |-sorted(tuple(x))
    13 |+sorted(x)
 14 14 | sorted(sorted(x))
-15 15 | sorted(sorted(x, key=lambda y: y))
-16 16 | sorted(reversed(x))
+15 15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+16 16 | sorted(sorted(x, reverse=True), reverse=True)
 
 C414.py:14:1: C414 [*] Unnecessary `sorted` call within `sorted()`
    |
 12 | sorted(list(x))
 13 | sorted(tuple(x))
 14 | sorted(sorted(x))
    | ^^^^^^^^^^^^^^^^^ C414
-15 | sorted(sorted(x, key=lambda y: y))
-16 | sorted(reversed(x))
+15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+16 | sorted(sorted(x, reverse=True), reverse=True)
    |
    = help: Remove the inner `sorted` call
 
@@ -266,96 +266,122 @@ C414.py:14:1: C414 [*] Unnecessary `sorted` call within `sorted()`
 13 13 | sorted(tuple(x))
 14    |-sorted(sorted(x))
    14 |+sorted(x)
-15 15 | sorted(sorted(x, key=lambda y: y))
-16 16 | sorted(reversed(x))
-17 17 | sorted(list(x), key=lambda y: y)
+15 15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+16 16 | sorted(sorted(x, reverse=True), reverse=True)
+17 17 | sorted(reversed(x))
 
 C414.py:15:1: C414 [*] Unnecessary `sorted` call within `sorted()`
    |
 13 | sorted(tuple(x))
 14 | sorted(sorted(x))
-15 | sorted(sorted(x, key=lambda y: y))
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C414
-16 | sorted(reversed(x))
-17 | sorted(list(x), key=lambda y: y)
+15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C414
+16 | sorted(sorted(x, reverse=True), reverse=True)
+17 | sorted(reversed(x))
    |
    = help: Remove the inner `sorted` call
 
 ℹ Suggested fix
 12 12 | sorted(list(x))
 13 13 | sorted(tuple(x))
 14 14 | sorted(sorted(x))
-15    |-sorted(sorted(x, key=lambda y: y))
-   15 |+sorted(x, )
-16 16 | sorted(reversed(x))
-17 17 | sorted(list(x), key=lambda y: y)
-18 18 | tuple(
+15    |-sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+   15 |+sorted(x, reverse=False, key=foo)
+16 16 | sorted(sorted(x, reverse=True), reverse=True)
+17 17 | sorted(reversed(x))
+18 18 | sorted(list(x), key=lambda y: y)
 
-C414.py:16:1: C414 [*] Unnecessary `reversed` call within `sorted()`
+C414.py:16:1: C414 [*] Unnecessary `sorted` call within `sorted()`
    |
 14 | sorted(sorted(x))
-15 | sorted(sorted(x, key=lambda y: y))
-16 | sorted(reversed(x))
+15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+16 | sorted(sorted(x, reverse=True), reverse=True)
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C414
+17 | sorted(reversed(x))
+18 | sorted(list(x), key=lambda y: y)
+   |
+   = help: Remove the inner `sorted` call
+
+ℹ Suggested fix
+13 13 | sorted(tuple(x))
+14 14 | sorted(sorted(x))
+15 15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+16    |-sorted(sorted(x, reverse=True), reverse=True)
+   16 |+sorted(x, reverse=True)
+17 17 | sorted(reversed(x))
+18 18 | sorted(list(x), key=lambda y: y)
+19 19 | tuple(
+
+C414.py:17:1: C414 [*] Unnecessary `reversed` call within `sorted()`
+   |
+15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+16 | sorted(sorted(x, reverse=True), reverse=True)
+17 | sorted(reversed(x))
    | ^^^^^^^^^^^^^^^^^^^ C414
-17 | sorted(list(x), key=lambda y: y)
-18 | tuple(
+18 | sorted(list(x), key=lambda y: y)
+19 | tuple(
    |
    = help: Remove the inner `reversed` call
 
 ℹ Suggested fix
-13 13 | sorted(tuple(x))
 14 14 | sorted(sorted(x))
-15 15 | sorted(sorted(x, key=lambda y: y))
-16    |-sorted(reversed(x))
-   16 |+sorted(x)
-17 17 | sorted(list(x), key=lambda y: y)
-18 18 | tuple(
-19 19 |     list(
-
-C414.py:17:1: C414 [*] Unnecessary `list` call within `sorted()`
+15 15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+16 16 | sorted(sorted(x, reverse=True), reverse=True)
+17    |-sorted(reversed(x))
+   17 |+sorted(x)
+18 18 | sorted(list(x), key=lambda y: y)
+19 19 | tuple(
+20 20 |     list(
+
+C414.py:18:1: C414 [*] Unnecessary `list` call within `sorted()`
    |
-15 | sorted(sorted(x, key=lambda y: y))
-16 | sorted(reversed(x))
-17 | sorted(list(x), key=lambda y: y)
+16 | sorted(sorted(x, reverse=True), reverse=True)
+17 | sorted(reversed(x))
+18 | sorted(list(x), key=lambda y: y)
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C414
-18 | tuple(
-19 |     list(
+19 | tuple(
+20 |     list(
    |
    = help: Remove the inner `list` call
 
 ℹ Suggested fix
-14 14 | sorted(sorted(x))
-15 15 | sorted(sorted(x, key=lambda y: y))
-16 16 | sorted(reversed(x))
-17    |-sorted(list(x), key=lambda y: y)
-   17 |+sorted(x, key=lambda y: y)
-18 18 | tuple(
-19 19 |     list(
-20 20 |         [x, 3, "hell"\
-
-C414.py:18:1: C414 [*] Unnecessary `list` call within `tuple()`
+15 15 | sorted(sorted(x, key=foo, reverse=False), reverse=False, key=foo)
+16 16 | sorted(sorted(x, reverse=True), reverse=True)
+17 17 | sorted(reversed(x))
+18    |-sorted(list(x), key=lambda y: y)
+   18 |+sorted(x, key=lambda y: y)
+19 19 | tuple(
+20 20 |     list(
+21 21 |         [x, 3, "hell"\
+
+C414.py:19:1: C414 [*] Unnecessary `list` call within `tuple()`
    |
-16 |   sorted(reversed(x))
-17 |   sorted(list(x), key=lambda y: y)
-18 | / tuple(
-19 | |     list(
-20 | |         [x, 3, "hell"\
-21 | |         "o"]
-22 | |     )
-23 | | )
+17 |   sorted(reversed(x))
+18 |   sorted(list(x), key=lambda y: y)
+19 | / tuple(
+20 | |     list(
+21 | |         [x, 3, "hell"\
+22 | |         "o"]
+23 | |     )
+24 | | )
    | |_^ C414
+25 |   
+26 |   # Nested sorts with differing keyword arguments. Not flagged.
    |
    = help: Remove the inner `list` call
 
 ℹ Suggested fix
-16 16 | sorted(reversed(x))
-17 17 | sorted(list(x), key=lambda y: y)
-18 18 | tuple(
-19    |-    list(
-20    |-        [x, 3, "hell"\
-   19 |+    [x, 3, "hell"\
-21 20 |         "o"]
-22 21 |     )
-23    |-)
+17 17 | sorted(reversed(x))
+18 18 | sorted(list(x), key=lambda y: y)
+19 19 | tuple(
+20    |-    list(
+21    |-        [x, 3, "hell"\
+   20 |+    [x, 3, "hell"\
+22 21 |         "o"]
+23 22 |     )
+24    |-)
+25 23 | 
+26 24 | # Nested sorts with differing keyword arguments. Not flagged.
+27 25 | sorted(sorted(x, key=lambda y: y))