Skip to content

Commit

Permalink
Invert reverse argument regardless of whether it's a boolean
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Sep 13, 2023
1 parent ebe9c03 commit 35d5676
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 64 deletions.
2 changes: 2 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_comprehensions/C413.py
View file
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))

def reversed(*args, **kwargs):
return None
Expand Down
42 changes: 41 additions & 1 deletion crates/ruff/src/cst/helpers.rs
View file
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
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
74 changes: 58 additions & 16 deletions ...ke8_comprehensions/snapshots/ruff__rules__flake8_comprehensions__tests__C413_C413.py.snap
View file
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 | def reversed(*args, **kwargs):
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 | def reversed(*args, **kwargs):
10 | reversed(sorted(x, reverse=x))
11 | reversed(sorted(x, reverse=not x))
|
= help: Remove unnecessary `reversed` call

Expand All @@ -142,8 +143,49 @@ 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 | def reversed(*args, **kwargs):
12 12 | return None
10 10 | reversed(sorted(x, reverse=x))
11 11 | reversed(sorted(x, reverse=not x))
12 12 |

C413.py:10:1: C413 [*] Unnecessary `reversed` call around `sorted()`
|
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 |-reversed(sorted(x, reverse=x))
10 |+sorted(x, reverse=not x)
11 11 | reversed(sorted(x, reverse=not x))
12 12 |
13 13 | def reversed(*args, **kwargs):

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 | def reversed(*args, **kwargs):
|
= 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 | def reversed(*args, **kwargs):
14 14 | return None


21 changes: 2 additions & 19 deletions crates/ruff/src/rules/flake8_pytest_style/rules/assertion.rs
View file
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![],
}))
}

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

0 comments on commit 35d5676

Please sign in to comment.