Parenthesize single-generator arguments when adding reverse keyword

astral-sh · Sep 13, 2023 · eb9e8e1 · eb9e8e1
1 parent ebe9c03
commit eb9e8e1
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 6 deletions.
diff --git a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py
@@ -8,6 +8,11 @@
 reversed(sorted(x, reverse=True, key=lambda e: e))
 reversed(sorted(x, reverse=False))
 
+# Regression test for: https://github.com/astral-sh/ruff/issues/7289
+reversed(sorted(i for i in range(42)))
+reversed(sorted((i for i in range(42)), reverse=True))
+
+
 def reversed(*args, **kwargs):
     return None
 

diff --git a/crates/ruff/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff/src/rules/flake8_comprehensions/fixes.rs
@@ -5,7 +5,7 @@ use libcst_native::{
     Element, EmptyLine, Expression, GeneratorExp, LeftCurlyBrace, LeftParen, LeftSquareBracket,
     List, ListComp, Name, ParenthesizableWhitespace, ParenthesizedNode, ParenthesizedWhitespace,
     RightCurlyBrace, RightParen, RightSquareBracket, Set, SetComp, SimpleString, SimpleWhitespace,
-    TrailingWhitespace, Tuple,
+    TrailingWhitespace, Tuple, UnaryOp, UnaryOperation,
 };
 use ruff_python_ast::Expr;
 use ruff_text_size::{Ranged, TextRange};
@@ -718,7 +718,7 @@ pub(crate) fn fix_unnecessary_call_around_sorted(
         if outer_name.value == "list" {
             tree = Expression::Call(Box::new((*inner_call).clone()));
         } else {
-            // If the `reverse` argument is used
+            // If the `reverse` argument is used...
             let args = if inner_call.args.iter().any(|arg| {
                 matches!(
                     arg.keyword,
@@ -770,6 +770,28 @@ pub(crate) fn fix_unnecessary_call_around_sorted(
                     .collect_vec()
             } else {
                 let mut args = inner_call.args.clone();
+
+                // If necessary, parenthesize a generator expression, as a generator expression must
+                // be parenthesized if it's not a solitary argument. For example, given:
+                // ```python
+                // reversed(sorted(i for i in range(42)))
+                // ```
+                // Rewrite as:
+                // ```python
+                // sorted((i for i in range(42)), reverse=True)
+                // ```
+                if let [arg] = args.as_mut_slice() {
+                    if matches!(arg.value, Expression::GeneratorExp(_)) {
+                        if arg.value.lpar().is_empty() && arg.value.rpar().is_empty() {
+                            arg.value = arg
+                                .value
+                                .clone()
+                                .with_parens(LeftParen::default(), RightParen::default());
+                        }
+                    }
+                }
+
+                // Add the `reverse=True` argument.
                 args.push(Arg {
                     value: Expression::Name(Box::new(Name {
                         value: "True",

diff --git a/...ke8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C413_C413.py.snap b/...ke8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C413_C413.py.snap
@@ -123,7 +123,7 @@ C413.py:8:1: C413 [*] Unnecessary `reversed` call around `sorted()`
   8 |+sorted(x, reverse=False, key=lambda e: e)
 9 9 | reversed(sorted(x, reverse=False))
 10 10 | 
-11 11 | def reversed(*args, **kwargs):
+11 11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
 
 C413.py:9:1: C413 [*] Unnecessary `reversed` call around `sorted()`
    |
@@ -132,7 +132,7 @@ C413.py:9:1: C413 [*] Unnecessary `reversed` call around `sorted()`
  9 | reversed(sorted(x, reverse=False))
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413
 10 | 
-11 | def reversed(*args, **kwargs):
+11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
    |
    = help: Remove unnecessary `reversed` call
 
@@ -143,7 +143,45 @@ C413.py:9:1: C413 [*] Unnecessary `reversed` call around `sorted()`
 9     |-reversed(sorted(x, reverse=False))
    9  |+sorted(x, reverse=True)
 10 10 | 
-11 11 | def reversed(*args, **kwargs):
-12 12 |     return None
+11 11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
+12 12 | reversed(sorted(i for i in range(42)))
+
+C413.py:12:1: C413 [*] Unnecessary `reversed` call around `sorted()`
+   |
+11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
+12 | reversed(sorted(i for i in range(42)))
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413
+13 | reversed(sorted((i for i in range(42)), reverse=True))
+   |
+   = help: Remove unnecessary `reversed` call
+
+ℹ Suggested fix
+9  9  | reversed(sorted(x, reverse=False))
+10 10 | 
+11 11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
+12    |-reversed(sorted(i for i in range(42)))
+   12 |+sorted((i for i in range(42)), reverse=True)
+13 13 | reversed(sorted((i for i in range(42)), reverse=True))
+14 14 | 
+15 15 | 
+
+C413.py:13:1: C413 [*] Unnecessary `reversed` call around `sorted()`
+   |
+11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
+12 | reversed(sorted(i for i in range(42)))
+13 | reversed(sorted((i for i in range(42)), reverse=True))
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413
+   |
+   = help: Remove unnecessary `reversed` call
+
+ℹ Suggested fix
+10 10 | 
+11 11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289
+12 12 | reversed(sorted(i for i in range(42)))
+13    |-reversed(sorted((i for i in range(42)), reverse=True))
+   13 |+sorted((i for i in range(42)), reverse=False)
+14 14 | 
+15 15 | 
+16 16 | def reversed(*args, **kwargs):