mirror of https://github.com/astral-sh/ruff
Invert reverse argument regardless of whether it's a boolean (#7372)
## Summary When fixing `reversed(sorted(x, reverse=False))`, we rewrite as `sorted(x, reverse=True)`. However, if the `reverse` argument isn't `True` or `False`, we leave it as-is, which is incorrect. Now, given `reversed(sorted(x, reverse=y))`, we rewrite as `sorted(x, reverse=not y)`.
This commit is contained in:
Charlie Marsh
GitHub
parent
ebd1b296fd
commit
6e625bd93d
|
|
@ -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)))
|
||||
|
|
|
|||
|
|
@ -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![],
|
||||
}))
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
Loading…
Reference in New Issue