Fix #50 - formatter should preserve parentheses for binary or logical expressions

server/src/parser/expressions.ts

@@ -77,13 +77,15 @@ export type BinaryExpression = ASTNode & {
   operator: string,
   left: Expression,
   right: Expression,
+  parens?: boolean,
 };
 
 export type LogicalExpression = ASTNode & {
   type: 'LogicalExpression',
   operator: 'and' | 'or',
   left: Expression,
   right: Expression,
+  parens?: boolean,
 };
 
 export type UnaryExpression = ASTNode & {
@@ -191,9 +193,12 @@ export type StringCallExpression = ASTNode & {
   argument: StringLiteral,
 };
 
-export type Expression = TableConstructorExpression | BinaryExpression | LogicalExpression | UnaryExpression
+export type Expression = (TableConstructorExpression | BinaryExpression | LogicalExpression | UnaryExpression
   | MemberExpression | IndexExpression | CallExpression | TableCallExpression | StringCallExpression | Literal
-  | Identifier | FunctionDeclaration | Whitespace;
+  | Identifier | FunctionDeclaration | Whitespace) & {
+  /** Whether this expression is surrounded by parentheses. */
+  parens?: boolean;
+};
 
 export type Comment_ = ASTNode & {
   type: 'Comment',

server/src/parser/formatter.ts

@@ -872,6 +872,7 @@ export default class Formatter {
         parentOperator: childContext.parentOperator,
         currentOperator: node.operator,
         extraIndent: true,
+        parens: node.parens,
       },
       () => {
         let ret = '';
@@ -1043,36 +1044,45 @@ export default class Formatter {
       currentOperator?: string,
       isRightSideOfAnExpression?: boolean,
       extraIndent?: boolean,
+      parens?: boolean,
     },
     expressionToWrap: () => string,
   ): string {
-    let shouldWrap = false;
-
-    const parentPrecedence = params.parentOperator
-      ? Operators.binaryPrecedenceOf(params.parentOperator)
+    const {
+      parentOperator,
+      currentOperator,
+      isRightSideOfAnExpression,
+      extraIndent,
+      parens,
+    } = params;
+
+    let shouldWrap: boolean = !!parens;
+
+    const parentPrecedence = parentOperator
+      ? Operators.binaryPrecedenceOf(parentOperator)
       : Operators.minPrecedenceValue;
-    const currentPrecedence = params.currentOperator
-      ? Operators.binaryPrecedenceOf(params.currentOperator)
+    const currentPrecedence = currentOperator
+      ? Operators.binaryPrecedenceOf(currentOperator)
       : Operators.minPrecedenceValue;
     if (currentPrecedence < parentPrecedence) {
       shouldWrap = true;
     }
 
     if (
-      params.parentOperator &&
-      params.parentOperator === params.currentOperator &&
-      params.isRightSideOfAnExpression &&
+      parentOperator &&
+      parentOperator === currentOperator &&
+      isRightSideOfAnExpression &&
       // eslint-disable-next-line @typescript-eslint/no-unsafe-call
-      Operators.doesNeedParenthesesIfOnTheRightSide(params.parentOperator)
+      Operators.doesNeedParenthesesIfOnTheRightSide(parentOperator)
     ) {
       shouldWrap = true;
     }
 
-    if (shouldWrap && params.extraIndent) {
+    if (shouldWrap && extraIndent) {
       this.increaseDepth();
     }
     const expression = expressionToWrap();
-    if (shouldWrap && params.extraIndent) {
+    if (shouldWrap && extraIndent) {
       this.decreaseDepth();
     }
     return shouldWrap ? `(${expression})` : expression;

server/src/parser/parser.ts

@@ -592,6 +592,11 @@ export default class Parser {
 
     if (!this.lexer.consume('then') && !this.lexer.consume('do')) {
       if (canBeOneLiner) {
+
+        // Tell formatter not to use outer parens on the condition expression, since they'll
+        // be added anyway for a one-line if statement
+        condition.parens = false;
+
         // Handle special PICO-8 one-line if statement
         this.lexer.withSignificantNewline((cancelSignificantNewline) => {
           // pseudo-block for the if statement contents
@@ -1260,20 +1265,22 @@ export default class Parser {
     return null;
   }
 
-  parsePrefixExpression(flowContext: FlowContext) {
-    let base, name;
+  parsePrefixExpression(flowContext: FlowContext): Expression | null {
+    let base : Expression;
 
     const marker = this.createLocationMarker();
 
     // The prefix
     if (TokenType.Identifier === this.token.type) {
-      name = this.token.value as string;
+      const name = this.token.value as string;
       base = this.parseIdentifier();
       // Set the parent scope.
       this.attachScope(base, this.scopeHasName(name));
     } else if (this.lexer.consume('(')) {
+      // Parentheses surrounding a new expression.
       base = this.parseExpectedExpression(flowContext);
       this.lexer.expect(')');
+      base.parens = true;
     } else {
       return null;
     }

server/src/parser/test/formatter.test.ts

@@ -224,9 +224,12 @@ f = (some_var_3 + 111) % 222
 g = some_table.some_fn(
   some_var_4 * (rnd() - .5),
   some_var_5 * (rnd() - .5)
-)`.trim();
+)
+h = 20 - (2 + 3 * 4)
+i = not (true and false)
+j = 8 / (8 \\ 8)
+`.trim();
         const formatted = formatLua(input);
-        // TODO: ideally we would assert presence of parentheses, while ignoring if new lines are there or not, since they are not a subject of this test
         eq(formatted, `
 a = some_var_1 - (some_var_2 - some_var_3)
 b = some_var_1 / (some_var_2 / some_var_3)
@@ -237,10 +240,14 @@ f = (some_var_3 + 111) % 222
 g = some_table.some_fn(
   some_var_4 * (rnd() - .5),
   some_var_5 * (rnd() - .5)
-)`.trim());
+)
+h = 20 - (2 + 3 * 4)
+i = not (true and false)
+j = 8 / (8 \\ 8)  
+`.trim());
       });
 
-      it('removes parentheses from calculations when they are unnecessary', () => {
+      it('preserves parentheses on calculations even if unnecessary', () => {
         const input = `
 a = some_var_1 + (some_var_2 + some_var_3)
 b = some_var_1 * (some_var_2 * some_var_3)
@@ -250,17 +257,22 @@ e = (some_var_1 + some_var_2 + some_var_3)
 f = (some_var_1) * some_var_2 * (some_var_3)
 g = (some_var_1 - some_var_2) - some_var_3
 h = (some_var_1 / some_var_2) / some_var_3`.trim();
-        const formatted = formatLua(input);
-        eq(formatted, `
-a = some_var_1 + some_var_2 + some_var_3
-b = some_var_1 * some_var_2 * some_var_3
-c = some_var_1 + some_var_2 + some_var_3
-d = some_var_1 * some_var_2 * some_var_3
-e = some_var_1 + some_var_2 + some_var_3
+
+        const expected = `
+a = some_var_1 + (some_var_2 + some_var_3)
+b = some_var_1 * (some_var_2 * some_var_3)
+c = (some_var_1 + some_var_2) + some_var_3
+d = (some_var_1 * some_var_2) * some_var_3
+e = (some_var_1 + some_var_2 + some_var_3)
 f = some_var_1 * some_var_2 * some_var_3
-g = some_var_1 - some_var_2 - some_var_3
-h = some_var_1 / some_var_2 / some_var_3`.trim());
+g = (some_var_1 - some_var_2) - some_var_3
+h = (some_var_1 / some_var_2) / some_var_3`.trim();
+
+        const formatted = formatLua(input);
+        eq(formatted, expected);
       });
+
+      it('');
     });
 
     it('pads begin/end of single-line table constructors', () => {
@@ -950,7 +962,7 @@ __lua__
         formattedRange: {
           start: { line: 3, character: 0 },
           end: { line: Number.MAX_VALUE, character: 0 },
-        }
+        },
       });
     });