Fix formatting of chained boolean operations (astral-sh#6394)

Closes astral-sh#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.
durumu · Aug 12, 2023 · 46be79e · 46be79e
1 parent c5898cc
commit 46be79e
Show file tree

Hide file tree

Showing 5 changed files with 177 additions and 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
@@ -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
@@ -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<Parentheses>,
+    chained: bool,
+}
+
+pub struct BoolOpLayout {
+    pub(crate) parentheses: Option<Parentheses>,
+    pub(crate) chained: bool,
 }
 
 impl FormatRuleWithOptions<ExprBoolOp, PyFormatContext<'_>> for FormatExprBoolOp {
-    type Options = Option<Parentheses>;
+    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<ExprBoolOp> 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<ExprBoolOp> 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<PyFormatContext<'_>> 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
@@ -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<Expr, PyFormatContext<'_>> 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/...s/ruff_python_formatter/tests/snapshots/format@expression__binary_implicit_string.py.snap b/...s/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
@@ -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
 ```