diff --git a/rust/kcl-lib/src/parsing/ast/types/mod.rs b/rust/kcl-lib/src/parsing/ast/types/mod.rs index af2331495..c5f2b7c08 100644 --- a/rust/kcl-lib/src/parsing/ast/types/mod.rs +++ b/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 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)] diff --git a/rust/kcl-lib/src/unparser.rs b/rust/kcl-lib/src/unparser.rs index 8b2005ad3..0d034b20f 100644 --- a/rust/kcl-lib/src/unparser.rs +++ b/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); + } }