Wrap expressions in parentheses when negating (#10346)

## 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 https://github.com/astral-sh/ruff/issues/10335.

## Test Plan

`cargo test`
This commit is contained in:
Charlie Marsh 2024-03-11 15:20:55 -07:00 committed by GitHub
parent 4b0666919b
commit 02fc521369
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 64 additions and 13 deletions

View File

@ -14,9 +14,6 @@ reversed(sorted(x, reverse=not x))
reversed(sorted(i for i in range(42))) reversed(sorted(i for i in range(42)))
reversed(sorted((i for i in range(42)), reverse=True)) reversed(sorted((i for i in range(42)), reverse=True))
# Regression test for: https://github.com/astral-sh/ruff/issues/10335
def reversed(*args, **kwargs): reversed(sorted([1, 2, 3], reverse=False or True))
return None reversed(sorted([1, 2, 3], reverse=(False or True)))
reversed(sorted(x, reverse=True))

View File

@ -1,5 +1,6 @@
use libcst_native::{ use libcst_native::{
Expression, Name, ParenthesizableWhitespace, SimpleWhitespace, UnaryOperation, Expression, LeftParen, Name, ParenthesizableWhitespace, ParenthesizedNode, RightParen,
SimpleWhitespace, UnaryOperation,
}; };
/// Return a [`ParenthesizableWhitespace`] containing a single space. /// Return a [`ParenthesizableWhitespace`] containing a single space.
@ -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 { if let Expression::Name(ref expression) = expression {
match expression.value { match expression.value {
"True" => { "True" => {
@ -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 { Expression::UnaryOperation(Box::new(UnaryOperation {
operator: libcst_native::UnaryOp::Not { operator: libcst_native::UnaryOp::Not {
whitespace_after: space(), 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![], lpar: vec![],
rpar: vec![], rpar: vec![],
})) }))

View File

@ -205,7 +205,7 @@ C413.py:14:1: C413 [*] Unnecessary `reversed` call around `sorted()`
14 |+sorted((i for i in range(42)), reverse=True) 14 |+sorted((i for i in range(42)), reverse=True)
15 15 | reversed(sorted((i for i in range(42)), reverse=True)) 15 15 | reversed(sorted((i for i in range(42)), reverse=True))
16 16 | 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()` C413.py:15:1: C413 [*] Unnecessary `reversed` call around `sorted()`
| |
@ -213,6 +213,8 @@ C413.py:15:1: C413 [*] Unnecessary `reversed` call around `sorted()`
14 | reversed(sorted(i for i in range(42))) 14 | reversed(sorted(i for i in range(42)))
15 | reversed(sorted((i for i in range(42)), reverse=True)) 15 | reversed(sorted((i for i in range(42)), reverse=True))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C413
16 |
17 | # Regression test for: https://github.com/astral-sh/ruff/issues/10335
| |
= help: Remove unnecessary `reversed` call = help: Remove unnecessary `reversed` call
@ -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 |-reversed(sorted((i for i in range(42)), reverse=True))
15 |+sorted((i for i in range(42)), reverse=False) 15 |+sorted((i for i in range(42)), reverse=False)
16 16 | 16 16 |
17 17 | 17 17 | # Regression test for: https://github.com/astral-sh/ruff/issues/10335
18 18 | def reversed(*args, **kwargs): 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))

View File

@ -413,5 +413,3 @@ PT018.py:65:5: PT018 [*] Assertion should be broken down into multiple parts
70 72 | 70 72 |
71 73 | assert (not self.find_graph_output(node.output[0]) or 71 73 | assert (not self.find_graph_output(node.output[0]) or
72 74 | self.find_graph_input(node.input[0])) 72 74 | self.find_graph_input(node.input[0]))