diff --git a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py index 3947868387..29f381e0d7 100644 --- a/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py +++ b/crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py @@ -7,6 +7,8 @@ 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)) +reversed(sorted(x, reverse=x)) +reversed(sorted(x, reverse=not x)) # Regression test for: https://github.com/astral-sh/ruff/issues/7289 reversed(sorted(i for i in range(42))) diff --git a/crates/ruff/src/cst/helpers.rs b/crates/ruff/src/cst/helpers.rs index a9a5e171eb..e32c6ab887 100644 --- a/crates/ruff/src/cst/helpers.rs +++ b/crates/ruff/src/cst/helpers.rs @@ -1,4 +1,6 @@ -use libcst_native::{Expression, NameOrAttribute, ParenthesizableWhitespace, SimpleWhitespace}; +use libcst_native::{ + Expression, Name, NameOrAttribute, ParenthesizableWhitespace, SimpleWhitespace, UnaryOperation, +}; fn compose_call_path_inner<'a>(expr: &'a Expression, parts: &mut Vec<&'a str>) { match expr { @@ -50,3 +52,41 @@ pub(crate) fn or_space(whitespace: ParenthesizableWhitespace) -> Parenthesizable whitespace } } + +/// Negate a condition, i.e., `a` => `not a` and `not a` => `a`. +pub(crate) fn negate<'a>(expression: &Expression<'a>) -> Expression<'a> { + if let Expression::UnaryOperation(ref expression) = expression { + if matches!(expression.operator, libcst_native::UnaryOp::Not { .. }) { + return *expression.expression.clone(); + } + } + + if let Expression::Name(ref expression) = expression { + match expression.value { + "True" => { + return Expression::Name(Box::new(Name { + value: "False", + lpar: vec![], + rpar: vec![], + })); + } + "False" => { + return Expression::Name(Box::new(Name { + value: "True", + lpar: vec![], + rpar: vec![], + })); + } + _ => {} + } + } + + Expression::UnaryOperation(Box::new(UnaryOperation { + operator: libcst_native::UnaryOp::Not { + whitespace_after: space(), + }, + expression: Box::new(expression.clone()), + lpar: vec![], + rpar: vec![], + })) +} diff --git a/crates/ruff/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff/src/rules/flake8_comprehensions/fixes.rs index 3c4fc055d7..6ede088741 100644 --- a/crates/ruff/src/rules/flake8_comprehensions/fixes.rs +++ b/crates/ruff/src/rules/flake8_comprehensions/fixes.rs @@ -7,17 +7,17 @@ use libcst_native::{ RightCurlyBrace, RightParen, RightSquareBracket, Set, SetComp, SimpleString, SimpleWhitespace, TrailingWhitespace, Tuple, }; -use ruff_python_ast::Expr; -use ruff_text_size::{Ranged, TextRange}; use ruff_diagnostics::{Edit, Fix}; +use ruff_python_ast::Expr; use ruff_python_codegen::Stylist; use ruff_python_semantic::SemanticModel; use ruff_source_file::Locator; +use ruff_text_size::{Ranged, TextRange}; use crate::autofix::codemods::CodegenStylist; use crate::autofix::edits::pad; -use crate::cst::helpers::space; +use crate::cst::helpers::{negate, space}; use crate::rules::flake8_comprehensions::rules::ObjectType; use crate::{ checkers::ast::Checker, @@ -728,7 +728,7 @@ pub(crate) fn fix_unnecessary_call_around_sorted( }) ) }) { - // Negate the `reverse` argument + // Negate the `reverse` argument. inner_call .args .clone() @@ -741,31 +741,9 @@ pub(crate) fn fix_unnecessary_call_around_sorted( .. }) ) { - 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 + arg.value = negate(&arg.value); } + arg }) .collect_vec() } else { 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 043c3f1432..50c61206c2 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 @@ -103,17 +103,18 @@ C413.py:7:1: C413 [*] Unnecessary `reversed` call around `sorted()` 7 |+sorted(x, key=lambda e: e, reverse=False) 8 8 | reversed(sorted(x, reverse=True, key=lambda e: e)) 9 9 | reversed(sorted(x, reverse=False)) -10 10 | +10 10 | reversed(sorted(x, reverse=x)) C413.py:8:1: C413 [*] Unnecessary `reversed` call around `sorted()` - | -6 | reversed(sorted(x, reverse=True)) -7 | reversed(sorted(x, key=lambda e: e, reverse=True)) -8 | reversed(sorted(x, reverse=True, key=lambda e: e)) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413 -9 | reversed(sorted(x, reverse=False)) - | - = help: Remove unnecessary `reversed` call + | + 6 | reversed(sorted(x, reverse=True)) + 7 | reversed(sorted(x, key=lambda e: e, reverse=True)) + 8 | reversed(sorted(x, reverse=True, key=lambda e: e)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413 + 9 | reversed(sorted(x, reverse=False)) +10 | reversed(sorted(x, reverse=x)) + | + = help: Remove unnecessary `reversed` call ℹ Suggested fix 5 5 | reversed(sorted(x, key=lambda e: e)) @@ -122,8 +123,8 @@ C413.py:8:1: C413 [*] Unnecessary `reversed` call around `sorted()` 8 |-reversed(sorted(x, reverse=True, key=lambda e: e)) 8 |+sorted(x, reverse=False, key=lambda e: e) 9 9 | reversed(sorted(x, reverse=False)) -10 10 | -11 11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289 +10 10 | reversed(sorted(x, reverse=x)) +11 11 | reversed(sorted(x, reverse=not x)) C413.py:9:1: C413 [*] Unnecessary `reversed` call around `sorted()` | @@ -131,8 +132,8 @@ C413.py:9:1: C413 [*] Unnecessary `reversed` call around `sorted()` 8 | reversed(sorted(x, reverse=True, key=lambda e: e)) 9 | reversed(sorted(x, reverse=False)) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413 -10 | -11 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289 +10 | reversed(sorted(x, reverse=x)) +11 | reversed(sorted(x, reverse=not x)) | = help: Remove unnecessary `reversed` call @@ -142,46 +143,87 @@ C413.py:9:1: C413 [*] Unnecessary `reversed` call around `sorted()` 8 8 | reversed(sorted(x, reverse=True, key=lambda e: e)) 9 |-reversed(sorted(x, reverse=False)) 9 |+sorted(x, reverse=True) -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))) +10 10 | reversed(sorted(x, reverse=x)) +11 11 | reversed(sorted(x, reverse=not x)) +12 12 | -C413.py:12:1: C413 [*] Unnecessary `reversed` call around `sorted()` +C413.py:10: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)) + 8 | reversed(sorted(x, reverse=True, key=lambda e: e)) + 9 | reversed(sorted(x, reverse=False)) +10 | reversed(sorted(x, reverse=x)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413 +11 | reversed(sorted(x, reverse=not x)) | = help: Remove unnecessary `reversed` call ℹ Suggested fix +7 7 | reversed(sorted(x, key=lambda e: e, reverse=True)) +8 8 | reversed(sorted(x, reverse=True, key=lambda e: e)) 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 | +10 |-reversed(sorted(x, reverse=x)) + 10 |+sorted(x, reverse=not x) +11 11 | reversed(sorted(x, reverse=not x)) +12 12 | +13 13 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289 -C413.py:13:1: C413 [*] Unnecessary `reversed` call around `sorted()` +C413.py:11: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)) + 9 | reversed(sorted(x, reverse=False)) +10 | reversed(sorted(x, reverse=x)) +11 | reversed(sorted(x, reverse=not x)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413 +12 | +13 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289 + | + = help: Remove unnecessary `reversed` call + +ℹ Suggested fix +8 8 | reversed(sorted(x, reverse=True, key=lambda e: e)) +9 9 | reversed(sorted(x, reverse=False)) +10 10 | reversed(sorted(x, reverse=x)) +11 |-reversed(sorted(x, reverse=not x)) + 11 |+sorted(x, reverse=x) +12 12 | +13 13 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289 +14 14 | reversed(sorted(i for i in range(42))) + +C413.py:14:1: C413 [*] Unnecessary `reversed` call around `sorted()` + | +13 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289 +14 | reversed(sorted(i for i in range(42))) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413 +15 | reversed(sorted((i for i in range(42)), reverse=True)) + | + = help: Remove unnecessary `reversed` call + +ℹ Suggested fix +11 11 | reversed(sorted(x, reverse=not x)) +12 12 | +13 13 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289 +14 |-reversed(sorted(i for i in range(42))) + 14 |+sorted((i for i in range(42)), reverse=True) +15 15 | reversed(sorted((i for i in range(42)), reverse=True)) +16 16 | +17 17 | + +C413.py:15:1: C413 [*] Unnecessary `reversed` call around `sorted()` + | +13 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289 +14 | reversed(sorted(i for i in range(42))) +15 | 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): +12 12 | +13 13 | # Regression test for: https://github.com/astral-sh/ruff/issues/7289 +14 14 | reversed(sorted(i for i in range(42))) +15 |-reversed(sorted((i for i in range(42)), reverse=True)) + 15 |+sorted((i for i in range(42)), reverse=False) +16 16 | +17 17 | +18 18 | def reversed(*args, **kwargs): diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs index abdcf85985..e8b5428c13 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs @@ -4,7 +4,7 @@ use anyhow::Result; use anyhow::{bail, Context}; use libcst_native::{ self, Assert, BooleanOp, CompoundStatement, Expression, ParenthesizedNode, SimpleStatementLine, - SimpleWhitespace, SmallStatement, Statement, TrailingWhitespace, UnaryOperation, + SimpleWhitespace, SmallStatement, Statement, TrailingWhitespace, }; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; @@ -21,7 +21,7 @@ use ruff_text_size::Ranged; use crate::autofix::codemods::CodegenStylist; use crate::checkers::ast::Checker; -use crate::cst::helpers::space; +use crate::cst::helpers::negate; use crate::cst::matchers::match_indented_block; use crate::cst::matchers::match_module; use crate::importer::ImportRequest; @@ -567,23 +567,6 @@ fn is_composite_condition(test: &Expr) -> CompositionKind { CompositionKind::None } -/// Negate a condition, i.e., `a` => `not a` and `not a` => `a`. -fn negate<'a>(expression: &Expression<'a>) -> Expression<'a> { - if let Expression::UnaryOperation(ref expression) = expression { - if matches!(expression.operator, libcst_native::UnaryOp::Not { .. }) { - return *expression.expression.clone(); - } - } - Expression::UnaryOperation(Box::new(UnaryOperation { - operator: libcst_native::UnaryOp::Not { - whitespace_after: space(), - }, - expression: Box::new(expression.clone()), - lpar: vec![], - rpar: vec![], - })) -} - /// Propagate parentheses from a parent to a child expression, if necessary. /// /// For example, when splitting: