Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Invert reverse argument regardless of whether it's a boolean #7372

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Invert reverse argument regardless of whether it's a boolean #7372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
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)))
Expand Down
42 changes: 41 additions & 1 deletion crates/ruff/src/cst/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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![],
}))
}
34 changes: 6 additions & 28 deletions crates/ruff/src/rules/flake8_comprehensions/fixes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -728,7 +728,7 @@ pub(crate) fn fix_unnecessary_call_around_sorted(
})
)
}) {
// Negate the `reverse` argument
// Negate the `reverse` argument.
inner_call
.args
.clone()
Expand All @@ -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 {
Expand Down
124 changes: 83 additions & 41 deletions ...ke8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C413_C413.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -122,17 +123,17 @@ 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()`
|
7 | reversed(sorted(x, key=lambda e: e, reverse=True))
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

Expand All @@ -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 |

C413.py:13:1: C413 [*] Unnecessary `reversed` call around `sorted()`
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:11:1: C413 [*] Unnecessary `reversed` call around `sorted()`
|
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()`
|
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))
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):


21 changes: 2 additions & 19 deletions crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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;
Expand Down Expand Up @@ -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![],
}))
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulled this out into a common home + generalized it to True and False.


/// Propagate parentheses from a parent to a child expression, if necessary.
///
/// For example, when splitting:
Expand Down
Loading