feat(prettier): binaryish expressions with parens (#1597)

crates/oxc_ast/src/ast_kind.rs

@@ -527,7 +527,9 @@ impl<'a> AstKind<'a> {
             Self::ArrowExpression(_) => "ArrowExpression".into(),
             Self::AssignmentExpression(_) => "AssignmentExpression".into(),
             Self::AwaitExpression(_) => "AwaitExpression".into(),
-            Self::BinaryExpression(b) => format!("BinaryExpression{}", b.operator.as_str()).into(),
+            Self::BinaryExpression(b) => {
+                format!("BinaryExpression({})", b.operator.as_str()).into()
+            }
             Self::CallExpression(_) => "CallExpression".into(),
             Self::ChainExpression(_) => "ChainExpression".into(),
             Self::ConditionalExpression(_) => "ConditionalExpression".into(),

crates/oxc_ast/src/precedence.rs

@@ -1,7 +1,4 @@
-use oxc_syntax::{
-    operator::{BinaryOperator, LogicalOperator},
-    precedence::{GetPrecedence, Precedence},
-};
+use oxc_syntax::precedence::{GetPrecedence, Precedence};
 
 use crate::ast::{
     ArrowExpression, AssignmentExpression, AwaitExpression, BinaryExpression, CallExpression,
@@ -62,39 +59,13 @@ impl<'a> GetPrecedence for AssignmentExpression<'a> {
 
 impl<'a> GetPrecedence for LogicalExpression<'a> {
     fn precedence(&self) -> Precedence {
-        match self.operator {
-            LogicalOperator::Or => Precedence::LogicalOr,
-            LogicalOperator::And => Precedence::LogicalAnd,
-            LogicalOperator::Coalesce => Precedence::Coalesce,
-        }
+        self.operator.precedence()
     }
 }
 
 impl<'a> GetPrecedence for BinaryExpression<'a> {
     fn precedence(&self) -> Precedence {
-        match self.operator {
-            BinaryOperator::BitwiseOR => Precedence::BitwiseOr,
-            BinaryOperator::BitwiseXOR => Precedence::BitwiseXor,
-            BinaryOperator::BitwiseAnd => Precedence::BitwiseAnd,
-            BinaryOperator::Equality
-            | BinaryOperator::Inequality
-            | BinaryOperator::StrictEquality
-            | BinaryOperator::StrictInequality => Precedence::Equality,
-            BinaryOperator::LessThan
-            | BinaryOperator::LessEqualThan
-            | BinaryOperator::GreaterThan
-            | BinaryOperator::GreaterEqualThan
-            | BinaryOperator::Instanceof
-            | BinaryOperator::In => Precedence::Relational,
-            BinaryOperator::ShiftLeft
-            | BinaryOperator::ShiftRight
-            | BinaryOperator::ShiftRightZeroFill => Precedence::Shift,
-            BinaryOperator::Subtraction | BinaryOperator::Addition => Precedence::Add,
-            BinaryOperator::Multiplication
-            | BinaryOperator::Remainder
-            | BinaryOperator::Division => Precedence::Multiply,
-            BinaryOperator::Exponential => Precedence::Exponential,
-        }
+        self.operator.precedence()
     }
 }
 

crates/oxc_prettier/src/binaryish.rs

@@ -0,0 +1,121 @@
+use oxc_ast::ast::*;
+use oxc_syntax::{
+    operator::{BinaryOperator, LogicalOperator},
+    precedence::{GetPrecedence, Precedence},
+};
+
+use crate::{Doc, Format, Prettier};
+
+#[derive(Clone, Copy)]
+pub enum BinaryishLeft<'a, 'b> {
+    Expression(&'b Expression<'a>),
+    PrivateIdentifier(&'b PrivateIdentifier),
+}
+
+impl<'a, 'b> From<&'b Expression<'a>> for BinaryishLeft<'a, 'b> {
+    fn from(e: &'b Expression<'a>) -> Self {
+        Self::Expression(e)
+    }
+}
+
+impl<'a, 'b> From<&'b PrivateIdentifier> for BinaryishLeft<'a, 'b> {
+    fn from(e: &'b PrivateIdentifier) -> Self {
+        Self::PrivateIdentifier(e)
+    }
+}
+
+impl<'a, 'b> BinaryishLeft<'a, 'b> {
+    pub fn operator(&self) -> Option<BinaryishOperator> {
+        match self {
+            Self::Expression(Expression::BinaryExpression(e)) => {
+                Some(BinaryishOperator::BinaryOperator(e.operator))
+            }
+            Self::Expression(Expression::LogicalExpression(e)) => {
+                Some(BinaryishOperator::LogicalOperator(e.operator))
+            }
+            _ => None,
+        }
+    }
+}
+
+impl<'a, 'b> Format<'a> for BinaryishLeft<'a, 'b> {
+    fn format(&self, p: &mut Prettier<'a>) -> Doc<'a> {
+        match self {
+            Self::Expression(expr) => expr.format(p),
+            Self::PrivateIdentifier(ident) => ident.format(p),
+        }
+    }
+}
+
+#[derive(Debug, Clone, Copy, Eq, PartialEq)]
+pub enum BinaryishOperator {
+    BinaryOperator(BinaryOperator),
+    LogicalOperator(LogicalOperator),
+}
+
+impl From<BinaryOperator> for BinaryishOperator {
+    fn from(op: BinaryOperator) -> Self {
+        Self::BinaryOperator(op)
+    }
+}
+
+impl From<LogicalOperator> for BinaryishOperator {
+    fn from(op: LogicalOperator) -> Self {
+        Self::LogicalOperator(op)
+    }
+}
+
+impl GetPrecedence for BinaryishOperator {
+    fn precedence(&self) -> Precedence {
+        match self {
+            Self::BinaryOperator(op) => op.precedence(),
+            Self::LogicalOperator(op) => op.precedence(),
+        }
+    }
+}
+
+impl BinaryishOperator {
+    pub fn is_binary(self) -> bool {
+        matches!(self, Self::BinaryOperator(_))
+    }
+
+    pub fn should_flatten(self, parent_op: Self) -> bool {
+        if self.precedence() != parent_op.precedence() {
+            return false;
+        }
+
+        if matches!(parent_op, Self::BinaryOperator(op) if op == BinaryOperator::Exponential) {
+            return false;
+        }
+
+        if matches!(parent_op, Self::BinaryOperator(op) if op.is_equality())
+            && matches!(self, Self::BinaryOperator(op) if op.is_equality())
+        {
+            return false;
+        }
+
+        if self != parent_op
+            && matches!(parent_op, Self::BinaryOperator(op) if op.is_multiplicative())
+            && matches!(self, Self::BinaryOperator(op) if op.is_multiplicative())
+        {
+            return false;
+        }
+
+        if matches!(parent_op, Self::BinaryOperator(op) if op.is_bitshift())
+            && matches!(self, Self::BinaryOperator(op) if op.is_bitshift())
+        {
+            return false;
+        }
+
+        true
+    }
+}
+
+impl BinaryishOperator {
+    pub fn as_str(self) -> &'static str {
+        match self {
+            Self::BinaryOperator(op) => op.as_str(),
+            Self::LogicalOperator(op) => op.as_str(),
+        }
+    }
+}

crates/oxc_prettier/src/format/binaryish.rs

@@ -1,78 +1,31 @@
 use oxc_ast::ast::*;
-use oxc_syntax::operator::{BinaryOperator, LogicalOperator};
 
 use crate::{
+    binaryish::{BinaryishLeft, BinaryishOperator},
     doc::{Doc, DocBuilder, Group},
     group, line, ss, Format, Prettier,
 };
 
-pub enum BinaryishLeft<'a, 'b> {
-    Expression(&'b Expression<'a>),
-    PrivateIdentifier(&'b PrivateIdentifier),
-}
-
-impl<'a, 'b> Format<'a> for BinaryishLeft<'a, 'b> {
-    fn format(&self, p: &mut Prettier<'a>) -> Doc<'a> {
-        match self {
-            Self::Expression(expr) => expr.format(p),
-            Self::PrivateIdentifier(ident) => ident.format(p),
-        }
-    }
-}
-
-#[derive(Clone, Copy)]
-pub enum BinaryishOperator {
-    BinaryOperator(BinaryOperator),
-    LogicalOperator(LogicalOperator),
-}
-
-impl BinaryishOperator {
-    fn is_binary(self) -> bool {
-        matches!(self, Self::BinaryOperator(_))
-    }
-}
-
-impl BinaryishOperator {
-    fn as_str(self) -> &'static str {
-        match self {
-            Self::BinaryOperator(op) => op.as_str(),
-            Self::LogicalOperator(op) => op.as_str(),
-        }
-    }
-}
-
 pub(super) fn print_binaryish_expression<'a>(
     p: &mut Prettier<'a>,
-    left: &BinaryishLeft<'a, '_>,
+    left: BinaryishLeft<'a, '_>,
     operator: BinaryishOperator,
     right: &Expression<'a>,
 ) -> Doc<'a> {
     let mut parts = p.vec();
-    match &left {
-        BinaryishLeft::Expression(expr) => match expr {
-            Expression::LogicalExpression(logical) => {
-                parts.push(print_binaryish_expression(
-                    p,
-                    &BinaryishLeft::Expression(&logical.left),
-                    BinaryishOperator::LogicalOperator(logical.operator),
-                    &logical.right,
-                ));
+
+    if left.operator().is_some_and(|left_operator| operator.should_flatten(left_operator)) {
+        parts.push(match left {
+            BinaryishLeft::Expression(Expression::BinaryExpression(e)) => {
+                print_binaryish_expression(p, (&e.left).into(), e.operator.into(), &e.right)
             }
-            Expression::BinaryExpression(binary) => {
-                parts.push(print_binaryish_expression(
-                    p,
-                    &BinaryishLeft::Expression(&binary.left),
-                    BinaryishOperator::BinaryOperator(binary.operator),
-                    &binary.right,
-                ));
+            BinaryishLeft::Expression(Expression::LogicalExpression(e)) => {
+                print_binaryish_expression(p, (&e.left).into(), e.operator.into(), &e.right)
             }
-            _ => {
-                parts.push(left.format(p));
-            }
-        },
-        BinaryishLeft::PrivateIdentifier(ident) => {
-            parts.push(left.format(p));
-        }
+            _ => unreachable!(),
+        });
+    } else {
+        parts.push(group!(p, left.format(p)));
     }
     parts.push(ss!(" "));
 

crates/oxc_prettier/src/format/mod.rs

@@ -36,12 +36,7 @@ use crate::{
     format, group, hardline, indent, line, softline, ss, string, wrap, Prettier,
 };
 
-use self::{
-    array::Array,
-    binaryish::{BinaryishLeft, BinaryishOperator},
-    object::ObjectLike,
-    template_literal::TemplateLiteralPrinter,
-};
+use self::{array::Array, object::ObjectLike, template_literal::TemplateLiteralPrinter};
 
 pub trait Format<'a> {
     #[must_use]
@@ -1597,8 +1592,8 @@ impl<'a> Format<'a> for BinaryExpression<'a> {
         wrap!(p, self, BinaryExpression, {
             let doc = binaryish::print_binaryish_expression(
                 p,
-                &BinaryishLeft::Expression(&self.left),
-                BinaryishOperator::BinaryOperator(self.operator),
+                (&self.left).into(),
+                self.operator.into(),
                 &self.right,
             );
             if misc::in_parentheses(p.parent_kind(), p.source_text, self.span) {
@@ -1615,8 +1610,8 @@ impl<'a> Format<'a> for PrivateInExpression<'a> {
         wrap!(p, self, PrivateInExpression, {
             binaryish::print_binaryish_expression(
                 p,
-                &BinaryishLeft::PrivateIdentifier(&self.left),
-                BinaryishOperator::BinaryOperator(self.operator),
+                (&self.left).into(),
+                self.operator.into(),
                 &self.right,
             )
         })
@@ -1628,8 +1623,8 @@ impl<'a> Format<'a> for LogicalExpression<'a> {
         wrap!(p, self, LogicalExpression, {
             let doc = binaryish::print_binaryish_expression(
                 p,
-                &BinaryishLeft::Expression(&self.left),
-                BinaryishOperator::LogicalOperator(self.operator),
+                (&self.left).into(),
+                self.operator.into(),
                 &self.right,
             );
 

crates/oxc_prettier/src/lib.rs

@@ -4,6 +4,7 @@
 
 #![allow(clippy::wildcard_imports)]
 
+mod binaryish;
 mod comments;
 mod doc;
 mod format;

crates/oxc_prettier/src/needs_parens.rs

@@ -17,9 +17,12 @@ use oxc_ast::{
     AstKind,
 };
 use oxc_span::{GetSpan, Span};
-use oxc_syntax::operator::{BinaryOperator, UnaryOperator, UpdateOperator};
+use oxc_syntax::{
+    operator::{BinaryOperator, UnaryOperator, UpdateOperator},
+    precedence::GetPrecedence,
+};
 
-use crate::{array, doc::Doc, ss, Prettier};
+use crate::{array, binaryish::BinaryishOperator, doc::Doc, ss, Prettier};
 
 impl<'a> Prettier<'a> {
     pub(crate) fn wrap_parens(&mut self, doc: Doc<'a>, kind: AstKind<'a>) -> Doc<'a> {
@@ -120,9 +123,7 @@ impl<'a> Prettier<'a> {
             AstKind::LogicalExpression(e) => self.check_binarish(e.span),
             AstKind::BinaryExpression(e) => match parent_kind {
                 AstKind::UpdateExpression(_) => true,
-                _ if e.operator == BinaryOperator::In
-                    && self.is_path_in_for_statement_initializer(e.span) =>
-                {
+                _ if e.operator.is_in() && self.is_path_in_for_statement_initializer(e.span) => {
                     true
                 }
                 _ => self.check_binarish(e.span),
@@ -385,13 +386,17 @@ impl<'a> Prettier<'a> {
     }
 
     fn check_binarish(&self, span: Span) -> bool {
-        match self.parent_kind() {
-            AstKind::TSAsExpression(_) => !self.is_binary_cast_expression(span),
-            AstKind::TSSatisfiesExpression(_) => !self.is_binary_cast_expression(span),
-            AstKind::ConditionalExpression(_) => self.is_binary_cast_expression(span),
-            AstKind::NewExpression(new_expr) => new_expr.callee.span() == span,
-            AstKind::CallExpression(new_expr) => new_expr.callee.span() == span,
-            AstKind::Class(class) => class.super_class.as_ref().is_some_and(|e| e.span() == span),
+        let current_kind = self.current_kind();
+        let parent_kind = self.parent_kind();
+        match parent_kind {
+            AstKind::TSAsExpression(_) => return !self.is_binary_cast_expression(span),
+            AstKind::TSSatisfiesExpression(_) => return !self.is_binary_cast_expression(span),
+            AstKind::ConditionalExpression(_) => return self.is_binary_cast_expression(span),
+            AstKind::NewExpression(new_expr) => return new_expr.callee.span() == span,
+            AstKind::CallExpression(new_expr) => return new_expr.callee.span() == span,
+            AstKind::Class(class) => {
+                return class.super_class.as_ref().is_some_and(|e| e.span() == span)
+            }
             AstKind::TSTypeAssertion(_)
             | AstKind::TaggedTemplateExpression(_)
             | AstKind::UnaryExpression(_)
@@ -399,16 +404,53 @@ impl<'a> Prettier<'a> {
             | AstKind::SpreadElement(_)
             | AstKind::AwaitExpression(_)
             | AstKind::TSNonNullExpression(_)
-            | AstKind::UpdateExpression(_) => true,
-            AstKind::MemberExpression(member_expr) => member_expr.object().span() == span,
+            | AstKind::UpdateExpression(_) => return true,
+            AstKind::MemberExpression(member_expr) => return member_expr.object().span() == span,
             AstKind::AssignmentExpression(assign_expr) => {
-                assign_expr.left.span() == span && self.is_binary_cast_expression(span)
+                return assign_expr.left.span() == span && self.is_binary_cast_expression(span)
             }
             AstKind::AssignmentPattern(assign_pat) => {
-                assign_pat.left.span() == span && self.is_binary_cast_expression(span)
+                return assign_pat.left.span() == span && self.is_binary_cast_expression(span)
             }
-            _ => false,
+            AstKind::LogicalExpression(parent_logical_expr) => {
+                if let AstKind::LogicalExpression(logical_expr) = current_kind {
+                    return parent_logical_expr.operator != logical_expr.operator;
+                }
+            }
+            _ => {}
         }
+
+        let operator = match self.current_kind() {
+            AstKind::LogicalExpression(e) => BinaryishOperator::from(e.operator),
+            AstKind::BinaryExpression(e) => BinaryishOperator::from(e.operator),
+            _ => return false,
+        };
+
+        let parent_operator = match parent_kind {
+            AstKind::LogicalExpression(e) => BinaryishOperator::from(e.operator),
+            AstKind::BinaryExpression(e) => BinaryishOperator::from(e.operator),
+            _ => return false,
+        };
+
+        let precedence = operator.precedence();
+        let parent_precedence = parent_operator.precedence();
+
+        if parent_precedence > precedence {
+            return true;
+        }
+
+        if parent_precedence == precedence && !parent_operator.should_flatten(operator) {
+            return true;
+        }
+
+        // Add parenthesis when working with bitwise operators
+        // It's not strictly needed but helps with code understanding
+        if matches!(parent_kind, AstKind::BinaryExpression(binary_expr) if binary_expr.operator.is_bitwise())
+        {
+            return true;
+        }
+
+        false
     }
 
     fn check_member_call(&self, span: Span) -> bool {

crates/oxc_syntax/src/operator.rs

@@ -1,6 +1,8 @@
 #[cfg(feature = "serde")]
 use serde::Serialize;
 
+use crate::precedence::{GetPrecedence, Precedence};
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize))]
 pub enum AssignmentOperator {
@@ -129,46 +131,41 @@ pub enum BinaryOperator {
 }
 
 impl BinaryOperator {
+    #[rustfmt::skip]
     pub fn is_equality(self) -> bool {
-        matches!(
-            self,
-            Self::Equality | Self::Inequality | Self::StrictEquality | Self::StrictInequality
-        )
+        matches!(self, Self::Equality | Self::Inequality | Self::StrictEquality | Self::StrictInequality)
     }
 
+    #[rustfmt::skip]
     pub fn is_compare(self) -> bool {
-        matches!(
-            self,
-            Self::LessThan | Self::LessEqualThan | Self::GreaterThan | Self::GreaterEqualThan
-        )
+        matches!(self, Self::LessThan | Self::LessEqualThan | Self::GreaterThan | Self::GreaterEqualThan)
     }
 
+    #[rustfmt::skip]
     pub fn is_arithmetic(self) -> bool {
-        matches!(
-            self,
-            Self::Addition
-                | Self::Subtraction
-                | Self::Multiplication
-                | Self::Division
-                | Self::Remainder
-                | Self::Exponential
-        )
+        matches!(self, Self::Addition | Self::Subtraction | Self::Multiplication
+                | Self::Division | Self::Remainder | Self::Exponential)
+    }
+
+    pub fn is_multiplicative(self) -> bool {
+        matches!(self, Self::Multiplication | Self::Division | Self::Remainder)
     }
 
     pub fn is_relational(self) -> bool {
         matches!(self, Self::In | Self::Instanceof)
     }
 
+    pub fn is_in(self) -> bool {
+        matches!(self, Self::In)
+    }
+
+    #[rustfmt::skip]
     pub fn is_bitwise(self) -> bool {
-        matches!(
-            self,
-            Self::BitwiseOR
-                | Self::BitwiseXOR
-                | Self::BitwiseAnd
-                | Self::ShiftLeft
-                | Self::ShiftRight
-                | Self::ShiftRightZeroFill,
-        )
+        self.is_bitshift() || matches!(self, Self::BitwiseOR | Self::BitwiseXOR | Self::BitwiseAnd)
+    }
+
+    pub fn is_bitshift(self) -> bool {
+        matches!(self, Self::ShiftLeft | Self::ShiftRight | Self::ShiftRightZeroFill)
     }
 
     pub fn is_numeric_or_string_binary_operator(self) -> bool {
@@ -227,6 +224,29 @@ impl BinaryOperator {
     }
 }
 
+impl GetPrecedence for BinaryOperator {
+    fn precedence(&self) -> Precedence {
+        match self {
+            Self::BitwiseOR => Precedence::BitwiseOr,
+            Self::BitwiseXOR => Precedence::BitwiseXor,
+            Self::BitwiseAnd => Precedence::BitwiseAnd,
+            Self::Equality | Self::Inequality | Self::StrictEquality | Self::StrictInequality => {
+                Precedence::Equality
+            }
+            Self::LessThan
+            | Self::LessEqualThan
+            | Self::GreaterThan
+            | Self::GreaterEqualThan
+            | Self::Instanceof
+            | Self::In => Precedence::Relational,
+            Self::ShiftLeft | Self::ShiftRight | Self::ShiftRightZeroFill => Precedence::Shift,
+            Self::Subtraction | Self::Addition => Precedence::Add,
+            Self::Multiplication | Self::Remainder | Self::Division => Precedence::Multiply,
+            Self::Exponential => Precedence::Exponential,
+        }
+    }
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize))]
 pub enum LogicalOperator {
@@ -248,6 +268,16 @@ impl LogicalOperator {
     }
 }
 
+impl GetPrecedence for LogicalOperator {
+    fn precedence(&self) -> Precedence {
+        match self {
+            Self::Or => Precedence::LogicalOr,
+            Self::And => Precedence::LogicalAnd,
+            Self::Coalesce => Precedence::Coalesce,
+        }
+    }
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize))]
 pub enum UnaryOperator {

tasks/prettier_conformance/prettier.snap.md

@@ -1,4 +1,4 @@
-Compatibility: 209/561 (37.25%)
+Compatibility: 211/561 (37.61%)
 
 # Failed
 
@@ -71,8 +71,6 @@ Compatibility: 209/561 (37.25%)
 * binary-expressions/arrow.js
 * binary-expressions/call.js
 * binary-expressions/comment.js
-* binary-expressions/equality.js
-* binary-expressions/exp.js
 * binary-expressions/in_instanceof.js
 * binary-expressions/inline-jsx.js
 * binary-expressions/inline-object-array.js