Skip to content

Commit

Permalink
Wrap expressions in parentheses when negating (#10346)
Browse files Browse the repository at this point in the history
## Summary

When negating an expression like `a or b`, we need to wrap it in
parentheses, e.g., `not (a or b)` instead of `not a or b`, due to
operator precedence.

Closes #10335.

## Test Plan

`cargo test`
  • Loading branch information
charliermarsh committed Mar 11, 2024
1 parent 4b06669 commit 02fc521
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 13 deletions.
Expand Up @@ -14,9 +14,6 @@
reversed(sorted(i for i in range(42)))
reversed(sorted((i for i in range(42)), reverse=True))


def reversed(*args, **kwargs):
return None


reversed(sorted(x, reverse=True))
# Regression test for: https://github.com/astral-sh/ruff/issues/10335
reversed(sorted([1, 2, 3], reverse=False or True))
reversed(sorted([1, 2, 3], reverse=(False or True)))
27 changes: 25 additions & 2 deletions crates/ruff_linter/src/cst/helpers.rs
@@ -1,5 +1,6 @@
use libcst_native::{
Expression, Name, ParenthesizableWhitespace, SimpleWhitespace, UnaryOperation,
Expression, LeftParen, Name, ParenthesizableWhitespace, ParenthesizedNode, RightParen,
SimpleWhitespace, UnaryOperation,
};

/// Return a [`ParenthesizableWhitespace`] containing a single space.
Expand All @@ -24,6 +25,7 @@ pub(crate) fn negate<'a>(expression: &Expression<'a>) -> Expression<'a> {
}
}

// If the expression is `True` or `False`, return the opposite.
if let Expression::Name(ref expression) = expression {
match expression.value {
"True" => {
Expand All @@ -44,11 +46,32 @@ pub(crate) fn negate<'a>(expression: &Expression<'a>) -> Expression<'a> {
}
}

// If the expression is higher precedence than the unary `not`, we need to wrap it in
// parentheses.
//
// For example: given `a and b`, we need to return `not (a and b)`, rather than `not a and b`.
//
// See: <https://docs.python.org/3/reference/expressions.html#operator-precedence>
let needs_parens = matches!(
expression,
Expression::BooleanOperation(_)
| Expression::IfExp(_)
| Expression::Lambda(_)
| Expression::NamedExpr(_)
);
let has_parens = !expression.lpar().is_empty() && !expression.rpar().is_empty();
// Otherwise, wrap in a `not` operator.
Expression::UnaryOperation(Box::new(UnaryOperation {
operator: libcst_native::UnaryOp::Not {
whitespace_after: space(),
},
expression: Box::new(expression.clone()),
expression: Box::new(if needs_parens && !has_parens {
expression
.clone()
.with_parens(LeftParen::default(), RightParen::default())
} else {
expression.clone()
}),
lpar: vec![],
rpar: vec![],
}))
Expand Down
Expand Up @@ -205,14 +205,16 @@ C413.py:14:1: C413 [*] Unnecessary `reversed` call around `sorted()`
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 |
17 17 | # Regression test for: https://github.com/astral-sh/ruff/issues/10335

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
16 |
17 | # Regression test for: https://github.com/astral-sh/ruff/issues/10335
|
= help: Remove unnecessary `reversed` call

Expand All @@ -223,7 +225,38 @@ C413.py:15:1: C413 [*] Unnecessary `reversed` call around `sorted()`
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):
17 17 | # Regression test for: https://github.com/astral-sh/ruff/issues/10335
18 18 | reversed(sorted([1, 2, 3], reverse=False or True))

C413.py:18:1: C413 [*] Unnecessary `reversed` call around `sorted()`
|
17 | # Regression test for: https://github.com/astral-sh/ruff/issues/10335
18 | reversed(sorted([1, 2, 3], reverse=False or True))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413
19 | reversed(sorted([1, 2, 3], reverse=(False or True)))
|
= help: Remove unnecessary `reversed` call

Unsafe fix
15 15 | reversed(sorted((i for i in range(42)), reverse=True))
16 16 |
17 17 | # Regression test for: https://github.com/astral-sh/ruff/issues/10335
18 |-reversed(sorted([1, 2, 3], reverse=False or True))
18 |+sorted([1, 2, 3], reverse=not (False or True))
19 19 | reversed(sorted([1, 2, 3], reverse=(False or True)))

C413.py:19:1: C413 [*] Unnecessary `reversed` call around `sorted()`
|
17 | # Regression test for: https://github.com/astral-sh/ruff/issues/10335
18 | reversed(sorted([1, 2, 3], reverse=False or True))
19 | reversed(sorted([1, 2, 3], reverse=(False or True)))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413
|
= help: Remove unnecessary `reversed` call

Unsafe fix
16 16 |
17 17 | # Regression test for: https://github.com/astral-sh/ruff/issues/10335
18 18 | reversed(sorted([1, 2, 3], reverse=False or True))
19 |-reversed(sorted([1, 2, 3], reverse=(False or True)))
19 |+sorted([1, 2, 3], reverse=not (False or True))
Expand Up @@ -413,5 +413,3 @@ PT018.py:65:5: PT018 [*] Assertion should be broken down into multiple parts
70 72 |
71 73 | assert (not self.find_graph_output(node.output[0]) or
72 74 | self.find_graph_input(node.input[0]))


0 comments on commit 02fc521

Please sign in to comment.