fix(formatter): align binary operator formatting with PER standard

This commit updates the default formatting for binary operators to align with the PER Coding Style standard and fixes
several related indentation bugs that this change revealed.

The default value for `line_before_binary_operator` has now been set to `true`. This enforces the modern style where the
operator is placed at the beginning of the new line in multi-line expressions.

```php
// Old Default
if (
    $longName &&
    $anotherLongName
) {
    // ...
}

// New Default
if (
    $longName
    && $anotherLongName
) {
    // ...
}
````

Changing this default uncovered issues where the formatter would apply an extra, incorrect level of indentation to the
line containing the operator. The logic has been corrected to remove this extra indent, ensuring that chained conditions
in conditions, assignments, and return statements are formatted cleanly and according to the PER standard.

Signed-off-by: azjezz <[email protected]>

Author: azjezz

crates/formatter/src/internal/format/binaryish.rs

@@ -29,18 +29,19 @@ pub(super) fn print_binaryish_expression<'arena>(
 
     let grandparent = f.grandparent_node();
 
-    let is_inside_parenthesis = matches!(
-        grandparent,
-        Some(
-            Node::If(_)
-                | Node::IfStatementBodyElseIfClause(_)
-                | Node::IfColonDelimitedBodyElseIfClause(_)
-                | Node::While(_)
-                | Node::Switch(_)
-                | Node::DoWhile(_)
-                | Node::Match(_)
-        )
-    );
+    let is_inside_parenthesis = f.is_wrapped_in_parens
+        || matches!(
+            grandparent,
+            Some(
+                Node::If(_)
+                    | Node::IfStatementBodyElseIfClause(_)
+                    | Node::IfColonDelimitedBodyElseIfClause(_)
+                    | Node::While(_)
+                    | Node::Switch(_)
+                    | Node::DoWhile(_)
+                    | Node::Match(_)
+            )
+        );
 
     let parts = print_binaryish_expressions(f, left, operator, right, is_inside_parenthesis, false);
 
@@ -55,7 +56,7 @@ pub(super) fn print_binaryish_expression<'arena>(
     //     $this->lookahead()->type === $tt->parenLeft
     //   ) {
     if is_inside_parenthesis {
-        return Document::Indent(parts);
+        return Document::Array(parts);
     }
 
     // Break between the parens in
@@ -74,12 +75,17 @@ pub(super) fn print_binaryish_expression<'arena>(
         ]));
     }
 
-    let should_not_indent = matches!(grandparent, Some(Node::Return(_) | Node::Throw(_)))
-        || matches!(grandparent, Some(Node::ArrowFunction(func)) if func.arrow.is_before(operator.span()))
-        || matches!(grandparent, Some(Node::For(r#for)) if r#for.body.span().is_after(operator.span()))
-        || (matches!(grandparent, Some(Node::Conditional(_)))
-            && !matches!(f.great_grandparent_node(), Some(Node::Return(_) | Node::Throw(_)))
-            && !is_at_call_like_expression(f));
+    let should_not_indent = if let Some(Node::Binary(parent_binary)) = grandparent {
+        (parent_binary.operator.is_comparison() && operator.is_comparison())
+            || (parent_binary.operator.is_logical() && operator.is_logical())
+    } else {
+        matches!(grandparent, Some(Node::Return(_) | Node::Throw(_)))
+            || matches!(grandparent, Some(Node::ArrowFunction(func)) if func.arrow.is_before(operator.span()))
+            || matches!(grandparent, Some(Node::For(r#for)) if r#for.body.span().is_after(operator.span()))
+            || (matches!(grandparent, Some(Node::Conditional(_)))
+                && !matches!(f.great_grandparent_node(), Some(Node::Return(_) | Node::Throw(_)))
+                && !is_at_call_like_expression(f))
+    };
 
     let should_indent_if_inlining =
         matches!(grandparent, Some(Node::Assignment(_) | Node::PropertyItem(_) | Node::ConstantItem(_)))
@@ -197,6 +203,7 @@ pub(super) fn should_inline_binary_expression<'arena>(expression: &'arena Expres
 
 pub(super) fn should_inline_binary_rhs_expression(rhs: &Expression, operator: &BinaryOperator) -> bool {
     match unwrap_parenthesized(rhs) {
+        Expression::Assignment(_) => true,
         Expression::Array(Array { elements, .. })
         | Expression::List(List { elements, .. })
         | Expression::LegacyArray(LegacyArray { elements, .. }) => {

crates/formatter/src/internal/format/return_value.rs

@@ -29,25 +29,35 @@ pub fn format_return_value<'arena>(
     }
 
     if return_argument_has_leading_comment(f, value) {
+        let was_inside_parens = f.is_wrapped_in_parens;
+        f.is_wrapped_in_parens = true;
+        let value_doc = value.format(f);
+        f.is_wrapped_in_parens = was_inside_parens;
+
         return Document::Array(vec![
             in f.arena;
             (Document::String("(")),
-            (Document::Indent(vec![in f.arena; Document::Line(Line::hard()), value.format(f)])),
+            (Document::Indent(vec![in f.arena; Document::Line(Line::hard()), value_doc])),
             (Document::Line(Line::hard())),
             (Document::String(")")),
         ]);
     }
 
     match value {
         Expression::Binary(binary)
-            if (!is_simple_expression(binary.lhs) && !is_simple_expression(binary.rhs))
+            if (!is_simple_expression(binary.lhs) || !is_simple_expression(binary.rhs))
                 || (binary.lhs.is_binary() || binary.rhs.is_binary()) =>
         {
+            let was_inside_parens = f.is_wrapped_in_parens;
+            f.is_wrapped_in_parens = true;
+            let value_doc = value.format(f);
+            f.is_wrapped_in_parens = was_inside_parens;
+
             Document::Group(Group::new(vec![
                 in f.arena;
                 Document::IfBreak(IfBreak::then(f.arena, Document::String("("))),
                 Document::IndentIfBreak(IndentIfBreak::new(
-                    vec![in f.arena; Document::Line(Line::soft()), value.format(f)],
+                    vec![in f.arena; Document::Line(Line::soft()), value_doc],
                 )),
                 Document::Line(Line::soft()),
                 Document::IfBreak(IfBreak::then(f.arena, Document::String(")"))),
@@ -58,11 +68,16 @@ pub fn format_return_value<'arena>(
                 || (matches!(conditional.then.as_ref(), Some(Expression::Conditional(_)))
                     && matches!(conditional.r#else, Expression::Conditional(_))) =>
         {
+            let was_inside_parens = f.is_wrapped_in_parens;
+            f.is_wrapped_in_parens = true;
+            let value_doc = value.format(f);
+            f.is_wrapped_in_parens = was_inside_parens;
+
             Document::Group(Group::new(vec![
                 in f.arena;
                 Document::IfBreak(IfBreak::then(f.arena, Document::String("("))),
                 Document::IndentIfBreak(IndentIfBreak::new(
-                    vec![in f.arena; Document::Line(Line::soft()), value.format(f)],
+                    vec![in f.arena; Document::Line(Line::soft()), value_doc],
                 )),
                 Document::Line(Line::soft()),
                 Document::IfBreak(IfBreak::then(f.arena, Document::String(")"))),

crates/formatter/src/internal/format/statement.rs

@@ -5,6 +5,7 @@ use bumpalo::collections::Vec;
 use bumpalo::vec;
 
 use bumpalo::Bump;
+use mago_span::HasPosition;
 use mago_span::HasSpan;
 use mago_syntax::ast::*;
 
@@ -214,7 +215,7 @@ fn should_add_new_line_or_space_after_stmt<'arena>(
             DeclareBody::ColonDelimited(_) => true,
         },
         Statement::OpeningTag(_) => {
-            if let Some(index) = f.skip_to_line_end(Some(stmt.span().end_position().offset))
+            if let Some(index) = f.skip_to_line_end(Some(stmt.end_position().offset()))
                 && f.has_newline(index, false)
             {
                 return (true, false);
@@ -225,7 +226,7 @@ fn should_add_new_line_or_space_after_stmt<'arena>(
             false
         }
         _ => {
-            if f.has_newline(stmt.span().end_position().offset, false) {
+            if f.has_newline(stmt.end_position().offset(), false) {
                 true
             } else if let Some(Statement::ClosingTag(_)) = stmts.get(i + 1) {
                 should_add_space = !f.has_comment(stmt.span(), CommentFlags::Trailing);

crates/formatter/src/internal/macros.rs

@@ -3,12 +3,17 @@ macro_rules! wrap {
     ($f:ident, $self:expr, $node:ident, $block:block) => {{
         let node = mago_syntax::ast::Node::$node($self);
         $f.enter_node(node);
+
+        let was_wrapped_in_parens = $f.is_wrapped_in_parens;
+        let needed_to_wrap_in_parens = $f.need_parens(node);
+        $f.is_wrapped_in_parens |= needed_to_wrap_in_parens;
         let leading = $f.print_leading_comments(node.span());
         let doc = $block;
-        let doc = $f.wrap_parens(doc, node);
+        let doc = if needed_to_wrap_in_parens { $f.add_parens(doc, node) } else { doc };
         let trailing = $f.print_trailing_comments_for_node(node);
         let doc = $f.print_comments(leading, doc, trailing);
         $f.leave_node();
+        $f.is_wrapped_in_parens = was_wrapped_in_parens;
         doc
     }};
 }

crates/formatter/src/internal/mod.rs

@@ -51,6 +51,7 @@ pub struct FormatterState<'ctx, 'arena> {
     in_pipe_chain_arrow_segment: bool,
     in_script_terminating_statement: bool,
     in_condition: bool,
+    is_wrapped_in_parens: bool,
     halted_compilation: bool,
 }
 
@@ -85,6 +86,7 @@ impl<'ctx, 'arena> FormatterState<'ctx, 'arena> {
             parameter_state: ParameterState::default(),
             in_pipe_chain_arrow_segment: false,
             in_condition: false,
+            is_wrapped_in_parens: false,
             halted_compilation: false,
             in_script_terminating_statement: false,
         }

crates/formatter/src/internal/parens.rs

@@ -8,56 +8,45 @@ use mago_syntax::token::Precedence;
 
 use crate::document::Document;
 use crate::document::Group;
+use crate::document::IndentIfBreak;
 use crate::document::Line;
 use crate::internal::FormatterState;
 
 impl<'ctx, 'arena> FormatterState<'ctx, 'arena> {
-    pub(crate) fn wrap_parens(&mut self, document: Document<'arena>, node: Node<'arena, 'arena>) -> Document<'arena> {
-        if self.need_parens(node) {
-            if self.should_indent(node) {
-                Document::Group(Group::new(vec![
+    pub(crate) fn add_parens(&mut self, document: Document<'arena>, node: Node<'arena, 'arena>) -> Document<'arena> {
+        if self.should_indent(node) {
+            Document::Group(Group::new(vec![
+                in self.arena;
+                Document::String("("),
+                Document::IndentIfBreak(IndentIfBreak::new(vec![
                     in self.arena;
-                    Document::String("("),
-                    Document::Indent(vec![
-                        in self.arena;
-                        if self.settings.space_within_grouping_parenthesis {
-                            Document::Line(Line::default())
-                        } else {
-                            Document::Line(Line::soft())
-                        },
-                        document,
-                    ]),
                     if self.settings.space_within_grouping_parenthesis {
                         Document::Line(Line::default())
                     } else {
                         Document::Line(Line::soft())
                     },
-                    Document::String(")"),
-                ]))
-            } else {
-                Document::Group(Group::new(vec![
-                    in self.arena;
-                    Document::String("("),
-                    if self.settings.space_within_grouping_parenthesis { Document::space() } else { Document::empty() },
                     document,
-                    if self.settings.space_within_grouping_parenthesis { Document::space() } else { Document::empty() },
-                    Document::String(")"),
-                ]))
-            }
+                ])),
+                if self.settings.space_within_grouping_parenthesis {
+                    Document::Line(Line::default())
+                } else {
+                    Document::Line(Line::soft())
+                },
+                Document::String(")"),
+            ]))
         } else {
-            document
-        }
-    }
-
-    fn should_indent(&self, node: Node<'arena, 'arena>) -> bool {
-        if matches!(node, Node::Program(_)) || node.is_statement() {
-            return false;
+            Document::Group(Group::new(vec![
+                in self.arena;
+                Document::String("("),
+                if self.settings.space_within_grouping_parenthesis { Document::space() } else { Document::empty() },
+                document,
+                if self.settings.space_within_grouping_parenthesis { Document::space() } else { Document::empty() },
+                Document::String(")"),
+            ]))
         }
-
-        self.is_unary_or_binary_or_ternary(node)
     }
 
-    fn need_parens(&mut self, node: Node<'arena, 'arena>) -> bool {
+    pub(crate) fn need_parens(&mut self, node: Node<'arena, 'arena>) -> bool {
         if matches!(node, Node::Program(_)) || node.is_statement() {
             return false;
         }
@@ -70,6 +59,14 @@ impl<'ctx, 'arena> FormatterState<'ctx, 'arena> {
             || self.pipe_node_needs_parens(node)
     }
 
+    pub(crate) fn should_indent(&self, node: Node<'arena, 'arena>) -> bool {
+        if matches!(node, Node::Program(_)) || node.is_statement() {
+            return false;
+        }
+
+        self.is_unary_or_binary_or_ternary(node)
+    }
+
     fn literal_needs_parens(&self, node: Node<'arena, 'arena>) -> bool {
         let Node::Literal(Literal::Integer(_) | Literal::Float(_)) = node else {
             return false;

crates/formatter/src/settings.rs

@@ -399,8 +399,8 @@ pub struct FormatSettings {
     ///
     /// Note: If the right side has a leading comment, this setting is always false.
     ///
-    /// Default: false
-    #[serde(default = "default_false")]
+    /// Default: true
+    #[serde(default = "default_true")]
     pub line_before_binary_operator: bool,
 
     /// Whether to always break named argument lists into multiple lines.
@@ -869,7 +869,7 @@ impl Default for FormatSettings {
             null_type_hint: NullTypeHint::default(),
             break_promoted_properties_list: true,
             method_chain_breaking_style: MethodChainBreakingStyle::NextLine,
-            line_before_binary_operator: false,
+            line_before_binary_operator: true,
             sort_uses: true,
             separate_use_types: true,
             expand_use_groups: true,

crates/formatter/tests/cases/arrow_return/after.php

@@ -7,8 +7,8 @@ public function something(): void
         $command = array_find(
             array: $this->consoleConfig->commands,
             callback: fn(ConsoleCommand $consoleCommand) => (
-                $consoleCommand->handler->getDeclaringClass()->getName() === $command[0] &&
-                $consoleCommand->handler->getName() === $command[1]
+                $consoleCommand->handler->getDeclaringClass()->getName() === $command[0]
+                && $consoleCommand->handler->getName() === $command[1]
             ),
         );
 

crates/formatter/tests/cases/assignments/after.php

@@ -59,21 +59,21 @@ function foo()
 {
     if ($write) {
         $writable =
-            str_contains($meta['mode'], 'x') ||
-            str_contains($meta['mode'], 'w') ||
-            str_contains($meta['mode'], 'c') ||
-            str_contains($meta['mode'], 'a') ||
-            str_contains($meta['mode'], '+');
+            str_contains($meta['mode'], 'x')
+            || str_contains($meta['mode'], 'w')
+            || str_contains($meta['mode'], 'c')
+            || str_contains($meta['mode'], 'a')
+            || str_contains($meta['mode'], '+');
     }
 
     $target_directory = Env\temp_dir() . DIRECTORY_SEPARATOR . 'you-shall-not-pass';
 
     if (($i + 1) < $binary_length) {
         $byte1 = $chunk[2];
         $dest .=
-            static::encode6Bits($byte0 >> 2) .
-            static::encode6Bits((($byte0 << 4) | ($byte1 >> 4)) & 63) .
-            static::encode6Bits(($byte1 << 2) & 63);
+            static::encode6Bits($byte0 >> 2)
+            . static::encode6Bits((($byte0 << 4) | ($byte1 >> 4)) & 63)
+            . static::encode6Bits(($byte1 << 2) & 63);
     }
 
     $target_file =

crates/formatter/tests/cases/binary_alignment/after.php

@@ -21,9 +21,9 @@ private function addSearchClause(
 
                 // this complex condition is needed to avoid issues on PostgreSQL databases
                 if (
-                    $propertyConfig['is_small_integer'] && $isSmallIntegerQueryTerm ||
-                        $propertyConfig['is_integer'] && $isIntegerQueryTerm ||
-                        $propertyConfig['is_numeric'] && $isNumericQueryTerm
+                    $propertyConfig['is_small_integer'] && $isSmallIntegerQueryTerm
+                    || $propertyConfig['is_integer'] && $isIntegerQueryTerm
+                    || $propertyConfig['is_numeric'] && $isNumericQueryTerm
                 ) {
                     // ...
                 }
@@ -34,14 +34,14 @@ private function addSearchClause(
     private function isValidBoolean(mixed $value): bool
     {
         return (
-            $value === false ||
-            $value === 'false' ||
-            $value === 0 ||
-            $value === '0' ||
-            $value === true ||
-            $value === 'true' ||
-            $value === 1 ||
-            $value === '1'
+            $value === false
+            || $value === 'false'
+            || $value === 0
+            || $value === '0'
+            || $value === true
+            || $value === 'true'
+            || $value === 1
+            || $value === '1'
         );
     }
 }