From 2dccb7611a23a6881bc90a691d8734f6a8e4876f Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Sun, 12 Feb 2023 16:56:07 +0100 Subject: [PATCH] [`flake8-comprehensions`] bugfix for C413 autofix (#2804) --- README.md | 2 +- .../fixtures/flake8_comprehensions/C413.py | 4 +- .../src/rules/flake8_comprehensions/fixes.rs | 42 ++++++++++++- .../rules/unnecessary_call_around_sorted.rs | 23 +++++++ ...8_comprehensions__tests__C413_C413.py.snap | 61 ++++++++++++++++++- docs/rules/unnecessary-call-around-sorted.md | 29 +++++++++ 6 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 docs/rules/unnecessary-call-around-sorted.md diff --git a/README.md b/README.md index faef76d6a4..fe71ab8081 100644 --- a/README.md +++ b/README.md @@ -1047,7 +1047,7 @@ For more, see [flake8-comprehensions](https://pypi.org/project/flake8-comprehens | C409 | unnecessary-literal-within-tuple-call | Unnecessary `{literal}` literal passed to `tuple()` (rewrite as a `tuple` literal) | 🛠 | | C410 | unnecessary-literal-within-list-call | Unnecessary `{literal}` literal passed to `list()` (remove the outer call to `list()`) | 🛠 | | C411 | unnecessary-list-call | Unnecessary `list` call (remove the outer call to `list()`) | 🛠 | -| C413 | unnecessary-call-around-sorted | Unnecessary `{func}` call around `sorted()` | 🛠 | +| C413 | [unnecessary-call-around-sorted](https://github.com/charliermarsh/ruff/blob/main/docs/rules/unnecessary-call-around-sorted.md) | Unnecessary `{func}` call around `sorted()` | 🛠 | | C414 | [unnecessary-double-cast-or-process](https://github.com/charliermarsh/ruff/blob/main/docs/rules/unnecessary-double-cast-or-process.md) | Unnecessary `{inner}` call within `{outer}()` | 🛠 | | C415 | unnecessary-subscript-reversal | Unnecessary subscript reversal of iterable within `{func}()` | | | C416 | unnecessary-comprehension | Unnecessary `{obj_type}` comprehension (rewrite using `{obj_type}()`) | 🛠 | diff --git a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py index da72dc94ba..b11fbb8ba0 100644 --- a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py +++ b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py @@ -4,7 +4,9 @@ list(sorted(x)) reversed(sorted(x)) reversed(sorted(x, key=lambda e: e)) reversed(sorted(x, reverse=True)) - +reversed(sorted(x, key=lambda e: e, reverse=True)) +reversed(sorted(x, reverse=True, key=lambda e: e)) +reversed(sorted(x, reverse=False)) 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 90b93e73aa..0f7e340b1c 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/fixes.rs +++ b/crates/ruff/src/rules/flake8_comprehensions/fixes.rs @@ -743,6 +743,7 @@ pub fn fix_unnecessary_call_around_sorted( if outer_name.value == "list" { body.value = Expression::Call(inner_call.clone()); } else { + // If the `reverse` argument is used let args = if inner_call.args.iter().any(|arg| { matches!( arg.keyword, @@ -752,7 +753,46 @@ pub fn fix_unnecessary_call_around_sorted( }) ) }) { - inner_call.args.clone() + // Negate the `reverse` argument + inner_call + .args + .clone() + .into_iter() + .map(|mut arg| { + if matches!( + arg.keyword, + Some(Name { + value: "reverse", + .. + }) + ) { + if let Expression::Name(ref val) = arg.value { + if val.value == "True" { + // TODO: even better would be to drop the argument, as False is the default + arg.value = Expression::Name(Box::new(Name { + value: "False", + lpar: vec![], + rpar: vec![], + })); + arg + } else if val.value == "False" { + arg.value = Expression::Name(Box::new(Name { + value: "True", + lpar: vec![], + rpar: vec![], + })); + arg + } else { + arg + } + } else { + arg + } + } else { + arg + } + }) + .collect_vec() } else { let mut args = inner_call.args.clone(); args.push(Arg { diff --git a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_call_around_sorted.rs b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_call_around_sorted.rs index 3f4405cb3d..efd43fd9b3 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_call_around_sorted.rs +++ b/crates/ruff/src/rules/flake8_comprehensions/rules/unnecessary_call_around_sorted.rs @@ -10,6 +10,29 @@ use crate::rules::flake8_comprehensions::fixes; use crate::violation::AlwaysAutofixableViolation; define_violation!( + /// ## What it does + /// Checks for unnecessary `list` or `reversed` calls around `sorted` + /// calls. + /// + /// ## Why is this bad? + /// It is unnecessary to use `list` around `sorted`, as the latter already + /// returns a list. + /// + /// It is also unnecessary to use `reversed` around `sorted`, as the latter + /// has a `reverse` argument that can be used in lieu of an additional + /// `reversed` call. + /// + /// In both cases, it's clearer to avoid the redundant call. + /// + /// ## Examples + /// ```python + /// reversed(sorted(iterable)) + /// ``` + /// + /// Use instead: + /// ```python + /// sorted(iterable, reverse=True) + /// ``` pub struct UnnecessaryCallAroundSorted { pub func: String, } 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 f3c07d4178..4b4d5b978e 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 @@ -1,5 +1,5 @@ --- -source: src/rules/flake8_comprehensions/mod.rs +source: crates/ruff/src/rules/flake8_comprehensions/mod.rs expression: diagnostics --- - kind: @@ -70,7 +70,7 @@ expression: diagnostics column: 33 fix: content: - - "sorted(x, reverse=True)" + - "sorted(x, reverse=False)" location: row: 6 column: 0 @@ -78,4 +78,61 @@ expression: diagnostics row: 6 column: 33 parent: ~ +- kind: + UnnecessaryCallAroundSorted: + func: reversed + location: + row: 7 + column: 0 + end_location: + row: 7 + column: 50 + fix: + content: + - "sorted(x, key=lambda e: e, reverse=False)" + location: + row: 7 + column: 0 + end_location: + row: 7 + column: 50 + parent: ~ +- kind: + UnnecessaryCallAroundSorted: + func: reversed + location: + row: 8 + column: 0 + end_location: + row: 8 + column: 50 + fix: + content: + - "sorted(x, reverse=False, key=lambda e: e)" + location: + row: 8 + column: 0 + end_location: + row: 8 + column: 50 + parent: ~ +- kind: + UnnecessaryCallAroundSorted: + func: reversed + location: + row: 9 + column: 0 + end_location: + row: 9 + column: 34 + fix: + content: + - "sorted(x, reverse=True)" + location: + row: 9 + column: 0 + end_location: + row: 9 + column: 34 + parent: ~ diff --git a/docs/rules/unnecessary-call-around-sorted.md b/docs/rules/unnecessary-call-around-sorted.md new file mode 100644 index 0000000000..890ea0dc3e --- /dev/null +++ b/docs/rules/unnecessary-call-around-sorted.md @@ -0,0 +1,29 @@ +# unnecessary-call-around-sorted (C413) + +Derived from the **flake8-comprehensions** linter. + +Autofix is always available. + +## What it does +Checks for unnecessary `list` or `reversed` calls around `sorted` +calls. + +## Why is this bad? +It is unnecessary to use `list` around `sorted`, as the latter already +returns a list. + +It is also unnecessary to use `reversed` around `sorted`, as the latter +has a `reverse` argument that can be used in lieu of an additional +`reversed` call. + +In both cases, it's clearer to avoid the redundant call. + +## Examples +```python +reversed(sorted(iterable)) +``` + +Use instead: +```python +sorted(iterable, reverse=True) +``` \ No newline at end of file