From 5347df47281f157752ded4948cfc71549b364eb1 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 13 Sep 2023 15:40:45 -0400 Subject: [PATCH] Parenthesize single-generator arguments when adding reverse keyword (#7365) Closes https://github.com/astral-sh/ruff/issues/7289. --- .../fixtures/flake8_comprehensions/C413.py | 5 ++ .../src/rules/flake8_comprehensions/fixes.rs | 24 +++++++++- ...8_comprehensions__tests__C413_C413.py.snap | 46 +++++++++++++++++-- 3 files changed, 70 insertions(+), 5 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py index b11fbb8ba0..3947868387 100644 --- 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, key=lambda e: e, reverse=True)) 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 index dfd60f2d4d..3c4fc055d7 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/fixes.rs +++ b/crates/ruff/src/rules/flake8_comprehensions/fixes.rs @@ -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/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C413_C413.py.snap b/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C413_C413.py.snap index 734d403452..043c3f1432 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C413_C413.py.snap +++ b/crates/ruff/src/rules/flake8_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):