Properly respect associativity when reformatting (#7486)

Signed-off-by: Nick Cameron <nrc@ncameron.org>

rust/kcl-lib/src/parsing/ast/types/mod.rs

@@ -3005,6 +3005,8 @@ impl BinaryOperator {
         }
     }
 
+    /// The operator associativity of the operator (as in the parsing sense, not the mathematical sense of associativity).
+    ///
     /// Follow JS definitions of each operator.
     /// Taken from <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_precedence#table>
     pub fn associativity(&self) -> Associativity {
@@ -3015,6 +3017,12 @@ impl BinaryOperator {
             Self::And | Self::Or => Associativity::Left,
         }
     }
+
+    /// Whether an operator is mathematically associative. If it is, then the operator associativity (given by the
+    /// `associativity` method) is mostly irrelevant.
+    pub fn associative(&self) -> bool {
+        matches!(self, Self::Add | Self::Mul | Self::And | Self::Or)
+    }
 }
 #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema)]
 #[ts(export)]

rust/kcl-lib/src/unparser.rs

@@ -3,8 +3,8 @@ use std::fmt::Write;
 use crate::{
     parsing::{
         ast::types::{
-            Annotation, ArrayExpression, ArrayRangeExpression, AscribedExpression, BinaryExpression, BinaryOperator,
-            BinaryPart, BodyItem, CallExpressionKw, CommentStyle, DefaultParamVal, Expr, FormatOptions,
+            Annotation, ArrayExpression, ArrayRangeExpression, AscribedExpression, Associativity, BinaryExpression,
+            BinaryOperator, BinaryPart, BodyItem, CallExpressionKw, CommentStyle, DefaultParamVal, Expr, FormatOptions,
             FunctionExpression, IfExpression, ImportSelector, ImportStatement, ItemVisibility, LabeledArg, Literal,
             LiteralIdentifier, LiteralValue, MemberExpression, Node, NonCodeNode, NonCodeValue, ObjectExpression,
             Parameter, PipeExpression, Program, TagDeclarator, TypeDeclaration, UnaryExpression, VariableDeclaration,
@@ -710,17 +710,28 @@ impl BinaryExpression {
             }
         };
 
-        let should_wrap_right = match &self.right {
+        // It would be better to always preserve the user's parentheses but since we've dropped that
+        // info from the AST, we bracket expressions as necessary.
+        let should_wrap_left = match &self.left {
             BinaryPart::BinaryExpression(bin_exp) => {
                 self.precedence() > bin_exp.precedence()
-                    || self.operator == BinaryOperator::Sub
-                    || self.operator == BinaryOperator::Div
+                    || ((self.precedence() == bin_exp.precedence())
+                        && (!(self.operator.associative() && self.operator == bin_exp.operator)
+                            && self.operator.associativity() == Associativity::Right))
             }
             _ => false,
         };
 
-        let should_wrap_left = match &self.left {
-            BinaryPart::BinaryExpression(bin_exp) => self.precedence() > bin_exp.precedence(),
+        let should_wrap_right = match &self.right {
+            BinaryPart::BinaryExpression(bin_exp) => {
+                self.precedence() > bin_exp.precedence()
+                    // These two lines preserve previous reformatting behaviour.
+                    || self.operator == BinaryOperator::Sub
+                    || self.operator == BinaryOperator::Div
+                    || ((self.precedence() == bin_exp.precedence())
+                        && (!(self.operator.associative() && self.operator == bin_exp.operator)
+                            && self.operator.associativity() == Associativity::Left))
+            }
             _ => false,
         };
 
@@ -2820,4 +2831,36 @@ yo = 'bing'
         let recasted = ast.recast(&FormatOptions::new(), 0);
         assert_eq!(recasted, code);
     }
+
+    #[test]
+    fn paren_precedence() {
+        let code = r#"x = 1 - 2 - 3
+x = (1 - 2) - 3
+x = 1 - (2 - 3)
+x = 1 + 2 + 3
+x = (1 + 2) + 3
+x = 1 + (2 + 3)
+x = 2 * (y % 2)
+x = (2 * y) % 2
+x = 2 % (y * 2)
+x = (2 % y) * 2
+x = 2 * y % 2
+"#;
+
+        let expected = r#"x = 1 - 2 - 3
+x = 1 - 2 - 3
+x = 1 - (2 - 3)
+x = 1 + 2 + 3
+x = 1 + 2 + 3
+x = 1 + 2 + 3
+x = 2 * (y % 2)
+x = 2 * y % 2
+x = 2 % (y * 2)
+x = 2 % y * 2
+x = 2 * y % 2
+"#;
+        let ast = crate::parsing::top_level_parse(code).unwrap();
+        let recasted = ast.recast(&FormatOptions::new(), 0);
+        assert_eq!(recasted, expected);
+    }
 }