From 999d88e77371d9a8637a2cf0319d12b75ab3d132 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Mon, 7 Aug 2023 12:22:33 -0500 Subject: [PATCH] Fix formatting of chained boolean operations (#6394) Closes https://github.com/astral-sh/ruff/issues/6068 These commits are kind of a mess as I did some stumbling around here. Unrolls formatting of chained boolean operations to prevent nested grouping which gives us Black-compatible formatting where each boolean operation is on a new line. --- .../ruff/expression/boolean_operation.py | 32 +++++++ .../src/expression/expr_bool_op.rs | 64 ++++++++++---- .../src/expression/mod.rs | 10 ++- ...expression__binary_implicit_string.py.snap | 3 +- ...rmat@expression__boolean_operation.py.snap | 84 +++++++++++++++++++ 5 files changed, 177 insertions(+), 16 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/boolean_operation.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/boolean_operation.py index f976491cd1a42..16391cdf50947 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/boolean_operation.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/boolean_operation.py @@ -62,3 +62,35 @@ and [dddddddddddddd, eeeeeeeeee, fffffffffffffff] ): pass + +# Regression test for https://github.com/astral-sh/ruff/issues/6068 +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) or numpy and isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) and numpy or isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) or xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy and isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) and xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy or isinstance(ccccccccccc, dddddd) +): + pass + + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) or (xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy) and isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) and (xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy) or isinstance(ccccccccccc, dddddd) +): + pass diff --git a/crates/ruff_python_formatter/src/expression/expr_bool_op.rs b/crates/ruff_python_formatter/src/expression/expr_bool_op.rs index e57953371b2af..1717bdc262954 100644 --- a/crates/ruff_python_formatter/src/expression/expr_bool_op.rs +++ b/crates/ruff_python_formatter/src/expression/expr_bool_op.rs @@ -5,18 +5,27 @@ use crate::expression::parentheses::{ }; use crate::prelude::*; use ruff_formatter::{write, FormatOwnedWithRule, FormatRefWithRule, FormatRuleWithOptions}; -use ruff_python_ast::node::AnyNodeRef; -use ruff_python_ast::{BoolOp, ExprBoolOp}; +use ruff_python_ast::node::{AnyNodeRef, AstNode}; +use ruff_python_ast::{BoolOp, Expr, ExprBoolOp}; + +use super::parentheses::is_expression_parenthesized; #[derive(Default)] pub struct FormatExprBoolOp { parentheses: Option, + chained: bool, +} + +pub struct BoolOpLayout { + pub(crate) parentheses: Option, + pub(crate) chained: bool, } impl FormatRuleWithOptions> for FormatExprBoolOp { - type Options = Option; + type Options = BoolOpLayout; fn with_options(mut self, options: Self::Options) -> Self { - self.parentheses = options; + self.parentheses = options.parentheses; + self.chained = options.chained; self } } @@ -37,7 +46,7 @@ impl FormatNodeRule for FormatExprBoolOp { return Ok(()); }; - write!(f, [in_parentheses_only_group(&first.format())])?; + FormatValue { value: first }.fmt(f)?; for value in values { let leading_value_comments = comments.leading_comments(value); @@ -51,20 +60,20 @@ impl FormatNodeRule for FormatExprBoolOp { )?; } - write!( - f, - [ - op.format(), - space(), - in_parentheses_only_group(&value.format()) - ] - )?; + write!(f, [op.format(), space(),])?; + + FormatValue { value }.fmt(f)?; } Ok(()) }); - in_parentheses_only_group(&inner).fmt(f) + if self.chained { + // Chained boolean operations should not be given a new group + inner.fmt(f) + } else { + in_parentheses_only_group(&inner).fmt(f) + } } } @@ -78,6 +87,33 @@ impl NeedsParentheses for ExprBoolOp { } } +struct FormatValue<'a> { + value: &'a Expr, +} + +impl Format> for FormatValue<'_> { + fn fmt(&self, f: &mut PyFormatter) -> FormatResult<()> { + match self.value { + Expr::BoolOp(bool_op) + if !is_expression_parenthesized( + bool_op.as_any_node_ref(), + f.context().source(), + ) => + { + // Mark chained boolean operations e.g. `x and y or z` and avoid creating a new group + write!( + f, + [bool_op.format().with_options(BoolOpLayout { + parentheses: None, + chained: true, + })] + ) + } + _ => write!(f, [in_parentheses_only_group(&self.value.format())]), + } + } +} + #[derive(Copy, Clone)] pub struct FormatBoolOp; diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 971dc0898ebcc..94f4029a4dfc1 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -16,6 +16,8 @@ use crate::expression::parentheses::{ }; use crate::prelude::*; +use self::expr_bool_op::BoolOpLayout; + pub(crate) mod expr_attribute; pub(crate) mod expr_await; pub(crate) mod expr_bin_op; @@ -67,7 +69,13 @@ impl FormatRule> for FormatExpr { let parentheses = self.parentheses; let format_expr = format_with(|f| match expression { - Expr::BoolOp(expr) => expr.format().with_options(Some(parentheses)).fmt(f), + Expr::BoolOp(expr) => expr + .format() + .with_options(BoolOpLayout { + parentheses: Some(parentheses), + chained: false, + }) + .fmt(f), Expr::NamedExpr(expr) => expr.format().fmt(f), Expr::BinOp(expr) => expr.format().fmt(f), Expr::UnaryOp(expr) => expr.format().fmt(f), diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary_implicit_string.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary_implicit_string.py.snap index 8176cc6fdfecc..a87ae3d9d87c0 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary_implicit_string.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary_implicit_string.py.snap @@ -180,7 +180,8 @@ def test(): ", %s unmodified" % unmodified_count if collected["unmodified"] else "" ), "post_processed": ( - collected["post_processed"] and ", %s post-processed" % post_processed_count + collected["post_processed"] + and ", %s post-processed" % post_processed_count or "" ), } diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__boolean_operation.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__boolean_operation.py.snap index 979f6fb80509d..01ae09af34844 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__boolean_operation.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__boolean_operation.py.snap @@ -68,6 +68,38 @@ if ( and [dddddddddddddd, eeeeeeeeee, fffffffffffffff] ): pass + +# Regression test for https://github.com/astral-sh/ruff/issues/6068 +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) or numpy and isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) and numpy or isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) or xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy and isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) and xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy or isinstance(ccccccccccc, dddddd) +): + pass + + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) or (xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy) and isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) and (xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy) or isinstance(ccccccccccc, dddddd) +): + pass ``` ## Output @@ -136,6 +168,58 @@ if ( and [dddddddddddddd, eeeeeeeeee, fffffffffffffff] ): pass + +# Regression test for https://github.com/astral-sh/ruff/issues/6068 +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) + or numpy + and isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) + and numpy + or isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) + or xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy + and isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) + and xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy + or isinstance(ccccccccccc, dddddd) +): + pass + + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) + or ( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy + ) + and isinstance(ccccccccccc, dddddd) +): + pass + +if not ( + isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) + and ( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy + ) + or isinstance(ccccccccccc, dddddd) +): + pass ```