Skip to content

Commit

Permalink
Add rule to enforce parentheses in a or b and c (#9440)
Browse files Browse the repository at this point in the history
Fixes #8721

## Summary

This implements the rule proposed in #8721, as RUF021. `and` always
binds more tightly than `or` when chaining the two together.

(This should definitely be autofixable, but I'm leaving that to a
followup PR for now.)

## Test Plan

`cargo test` / `cargo insta review`
  • Loading branch information
AlexWaygood committed Jan 9, 2024
1 parent 84ab21f commit 86b1ae9
Show file tree
Hide file tree
Showing 8 changed files with 284 additions and 0 deletions.
98 changes: 98 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF021.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
# See https://docs.python.org/3/reference/expressions.html#operator-precedence
# for the official docs on operator precedence.
#
# Most importantly, `and` *always* takes precedence over `or`.
#
# `not` (the third boolean/logical operator) takes precedence over both,
# but the rule there is easier to remember,
# so we don't emit a diagnostic if a `not` expression is unparenthesized
# as part of a chain.

a, b, c = 1, 0, 2
x = a or b and c # RUF021: => `a or (b and c)`

a, b, c = 0, 1, 2
y = a and b or c # RUF021: => `(a and b) or c`

a, b, c, d = 1, 2, 0, 3
if a or b or c and d: # RUF021: => `a or b or (c and d)`
pass

a, b, c, d = 0, 0, 2, 3

if bool():
pass
elif a or b and c or d: # RUF021: => `a or (b and c) or d`
pass

a, b, c, d = 0, 1, 0, 2
while a and b or c and d: # RUF021: => `(and b) or (c and d)`
pass

b, c, d, e = 2, 3, 0, 4
z = [a for a in range(5) if a or b or c or d and e] # RUF021: => `a or b or c or (d and e)`

a, b, c, d = 0, 1, 3, 0
assert not a and b or c or d # RUF021: => `(not a and b) or c or d`

if (not a) and b or c or d: # RUF021: => `((not a) and b) or c or d`
if (not a and b) or c or d: # OK
pass

#############################################
# If they're all the same operator, it's fine
#############################################

x = not a and c # OK

if a or b or c: # OK
pass

while a and b and c: # OK
pass

###########################################################
# We don't consider `not` as part of a chain as problematic
###########################################################

x = not a or not b or not c # OK

#####################################
# If they're parenthesized, it's fine
#####################################

a, b, c = 1, 0, 2
x = a or (b and c) # OK
x2 = (a or b) and c # OK
x3 = (a or b) or c # OK
x4 = (a and b) and c # OK

a, b, c = 0, 1, 2
y = (a and b) or c # OK
yy = a and (b or c) # OK

a, b, c, d = 1, 2, 0, 3
if a or b or (c and d): # OK
pass

a, b, c, d = 0, 0, 2, 3

if bool():
pass
elif a or (b and c) or d: # OK
pass

a, b, c, d = 0, 1, 0, 2
while (a and b) or (c and d): # OK
pass

b, c, d, e = 2, 3, 0, 4
z = [a for a in range(5) if a or b or c or (d and e)] # OK

a, b = 1, 2
if (not a) or b: # OK
if (not a) and b: # OK
pass

a, b, c, d = 0, 1, 3, 0
assert ((not a) and b) or c or d # OK
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1492,6 +1492,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::UnnecessaryKeyCheck) {
ruff::rules::unnecessary_key_check(checker, expr);
}
if checker.enabled(Rule::ParenthesizeChainedOperators) {
ruff::rules::parenthesize_chained_logical_operators(checker, bool_op);
}
}
Expr::NamedExpr(..) => {
if checker.enabled(Rule::AssignmentInAssert) {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -923,6 +923,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "018") => (RuleGroup::Preview, rules::ruff::rules::AssignmentInAssert),
(Ruff, "019") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryKeyCheck),
(Ruff, "020") => (RuleGroup::Preview, rules::ruff::rules::NeverUnion),
(Ruff, "021") => (RuleGroup::Preview, rules::ruff::rules::ParenthesizeChainedOperators),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),

Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ mod tests {
#[test_case(Rule::AssignmentInAssert, Path::new("RUF018.py"))]
#[test_case(Rule::UnnecessaryKeyCheck, Path::new("RUF019.py"))]
#[test_case(Rule::NeverUnion, Path::new("RUF020.py"))]
#[test_case(Rule::ParenthesizeChainedOperators, Path::new("RUF021.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ pub(crate) use mutable_class_default::*;
pub(crate) use mutable_dataclass_default::*;
pub(crate) use never_union::*;
pub(crate) use pairwise_over_zipped::*;
pub(crate) use parenthesize_logical_operators::*;
pub(crate) use quadratic_list_summation::*;
pub(crate) use static_key_dict_comprehension::*;
pub(crate) use unnecessary_iterable_allocation_for_first_element::*;
Expand All @@ -34,6 +35,7 @@ mod mutable_class_default;
mod mutable_dataclass_default;
mod never_union;
mod pairwise_over_zipped;
mod parenthesize_logical_operators;
mod static_key_dict_comprehension;
mod unnecessary_iterable_allocation_for_first_element;
mod unnecessary_key_check;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_ast::parenthesize::parenthesized_range;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for chained operators where adding parentheses could improve the
/// clarity of the code.
///
/// ## Why is this bad?
/// `and` always binds more tightly than `or` when chaining the two together,
/// but this can be hard to remember (and sometimes surprising).
/// Adding parentheses in these situations can greatly improve code readability,
/// with no change to semantics or performance.
///
/// For example:
/// ```python
/// a, b, c = 1, 0, 2
/// x = a or b and c
///
/// d, e, f = 0, 1, 2
/// y = d and e or f
/// ```
///
/// Use instead:
/// ```python
/// a, b, c = 1, 0, 2
/// x = a or (b and c)
///
/// d, e, f = 0, 1, 2
/// y = (d and e) or f
/// ````
#[violation]
pub struct ParenthesizeChainedOperators;

impl Violation for ParenthesizeChainedOperators {
#[derive_message_formats]
fn message(&self) -> String {
format!(
"Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear"
)
}
}

/// RUF021
pub(crate) fn parenthesize_chained_logical_operators(
checker: &mut Checker,
expr: &ast::ExprBoolOp,
) {
// We're only interested in `and` expressions inside `or` expressions:
// - `a or b or c` => `BoolOp(values=[Name("a"), Name("b"), Name("c")], op=Or)`
// - `a and b and c` => `BoolOp(values=[Name("a"), Name("b"), Name("c")], op=And)`
// - `a or b and c` => `BoolOp(value=[Name("a"), BoolOp(values=[Name("b"), Name("c")], op=And), op=Or)`
//
// While it is *possible* to get an `Or` node inside an `And` node,
// you can only achieve it by parenthesizing the `or` subexpression
// (since normally, `and` always binds more tightly):
// - `a and (b or c)` => `BoolOp(values=[Name("a"), BoolOp(values=[Name("b"), Name("c"), op=Or), op=And)`
//
// We only care about unparenthesized boolean subexpressions here
// (if they're parenthesized already, that's great!),
// so we can ignore all cases where an `Or` node
// exists inside an `And` node.
if expr.op.is_and() {
return;
}
for condition in &expr.values {
match condition {
ast::Expr::BoolOp(
bool_op @ ast::ExprBoolOp {
op: ast::BoolOp::And,
..
},
) => {
if parenthesized_range(
bool_op.into(),
expr.into(),
checker.indexer().comment_ranges(),
checker.locator().contents(),
)
.is_none()
{
checker.diagnostics.push(Diagnostic::new(
ParenthesizeChainedOperators,
bool_op.range(),
));
}
}
_ => continue,
};
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
---
RUF021.py:12:10: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
|
11 | a, b, c = 1, 0, 2
12 | x = a or b and c # RUF021: => `a or (b and c)`
| ^^^^^^^ RUF021
13 |
14 | a, b, c = 0, 1, 2
|

RUF021.py:15:5: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
|
14 | a, b, c = 0, 1, 2
15 | y = a and b or c # RUF021: => `(a and b) or c`
| ^^^^^^^ RUF021
16 |
17 | a, b, c, d = 1, 2, 0, 3
|

RUF021.py:18:14: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
|
17 | a, b, c, d = 1, 2, 0, 3
18 | if a or b or c and d: # RUF021: => `a or b or (c and d)`
| ^^^^^^^ RUF021
19 | pass
|

RUF021.py:25:11: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
|
23 | if bool():
24 | pass
25 | elif a or b and c or d: # RUF021: => `a or (b and c) or d`
| ^^^^^^^ RUF021
26 | pass
|

RUF021.py:29:7: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
|
28 | a, b, c, d = 0, 1, 0, 2
29 | while a and b or c and d: # RUF021: => `(and b) or (c and d)`
| ^^^^^^^ RUF021
30 | pass
|

RUF021.py:29:18: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
|
28 | a, b, c, d = 0, 1, 0, 2
29 | while a and b or c and d: # RUF021: => `(and b) or (c and d)`
| ^^^^^^^ RUF021
30 | pass
|

RUF021.py:33:44: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
|
32 | b, c, d, e = 2, 3, 0, 4
33 | z = [a for a in range(5) if a or b or c or d and e] # RUF021: => `a or b or c or (d and e)`
| ^^^^^^^ RUF021
34 |
35 | a, b, c, d = 0, 1, 3, 0
|

RUF021.py:36:8: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
|
35 | a, b, c, d = 0, 1, 3, 0
36 | assert not a and b or c or d # RUF021: => `(not a and b) or c or d`
| ^^^^^^^^^^^ RUF021
37 |
38 | if (not a) and b or c or d: # RUF021: => `((not a) and b) or c or d`
|

RUF021.py:38:4: RUF021 Parenthesize `a and b` expressions when chaining `and` and `or` together, to make the precedence clear
|
36 | assert not a and b or c or d # RUF021: => `(not a and b) or c or d`
37 |
38 | if (not a) and b or c or d: # RUF021: => `((not a) and b) or c or d`
| ^^^^^^^^^^^^^ RUF021
39 | if (not a and b) or c or d: # OK
40 | pass
|


1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 86b1ae9

Please sign in to comment.