Fix formatting of chained boolean operations (#6394)

Closes #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.

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

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;
 

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),

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 ""
  ),
  }

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
 ```