From 86b1ae9383b4dd6ce3fb1e97707a19bbb860ecad Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 9 Jan 2024 01:28:03 +0000 Subject: [PATCH] Add rule to enforce parentheses in `a or b and c` (#9440) 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` --- .../resources/test/fixtures/ruff/RUF021.py | 98 +++++++++++++++++++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/ruff/mod.rs | 1 + .../ruff_linter/src/rules/ruff/rules/mod.rs | 2 + .../rules/parenthesize_logical_operators.rs | 95 ++++++++++++++++++ ..._rules__ruff__tests__RUF021_RUF021.py.snap | 83 ++++++++++++++++ ruff.schema.json | 1 + 8 files changed, 284 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF021.py create mode 100644 crates/ruff_linter/src/rules/ruff/rules/parenthesize_logical_operators.rs create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF021_RUF021.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF021.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF021.py new file mode 100644 index 0000000000..4a77355f10 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF021.py @@ -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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 1a45bedc14..474ec32b20 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -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) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index e3322155ed..43ef44c2bc 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -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), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index d11722ea34..bda96268fd 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -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( diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index ae50ad4221..37bc4e9c91 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -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::*; @@ -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; diff --git a/crates/ruff_linter/src/rules/ruff/rules/parenthesize_logical_operators.rs b/crates/ruff_linter/src/rules/ruff/rules/parenthesize_logical_operators.rs new file mode 100644 index 0000000000..0373a440c2 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/parenthesize_logical_operators.rs @@ -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, + }; + } +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF021_RUF021.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF021_RUF021.py.snap new file mode 100644 index 0000000000..a0c8166a9b --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF021_RUF021.py.snap @@ -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 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 48e5dd16cf..02295d9ba2 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3441,6 +3441,7 @@ "RUF019", "RUF02", "RUF020", + "RUF021", "RUF1", "RUF10", "RUF100",