fix(grainfmt): Remove extraneous parens around infix function applica…

…tion (grain-lang#902)

Check operator precedence when adding parens for infix operations and remove redundant parens

compiler/grainformat/reformat.re

@@ -7,6 +7,34 @@ open Grain_diagnostics;
 module Doc = Res_doc;
 
 let exception_primitives = [|"throw", "fail", "assert"|];
+
+let op_precedence = fn =>
+  switch (fn) {
+  | "*"
+  | "/"
+  | "%" => 120
+  | "+"
+  | "-"
+  | "++" => 110
+  | "<<"
+  | ">>"
+  | ">>>" => 100
+  | "<"
+  | "<="
+  | ">"
+  | ">=" => 90
+  | "=="
+  | "!="
+  | "is"
+  | "isnt" => 80
+  | "&" => 70
+  | "^" => 60
+  | "|" => 50
+  | "&&" => 40
+  | "||" => 30
+  | "_" => 10
+  | _ => 9999
+  };
 let list_cons = "[...]";
 
 type error =
@@ -998,10 +1026,10 @@ and print_type =
 }
 and print_application =
     (
-      ~func: Parsetree.expression,
       ~expressions: list(Parsetree.expression),
       ~parent_loc: Location.t,
       ~original_source: array(string),
+      func: Parsetree.expression,
     ) => {
   let function_name = get_function_name(func);
 
@@ -1036,87 +1064,96 @@ and print_application =
     }
 
   | [first, second] when infixop(function_name) =>
-    let first_brackets =
+    let left_expr =
+      print_expression(
+        ~parentIsArrow=false,
+        ~endChar=None,
+        ~original_source,
+        ~parent_loc,
+        first,
+      );
+
+    let right_expr =
+      print_expression(
+        ~parentIsArrow=false,
+        ~endChar=None,
+        ~original_source,
+        ~parent_loc,
+        second,
+      );
+
+    // wrap if in parens
+
+    let left_is_if =
       switch (first.pexp_desc) {
-      | PExpIf(_) =>
-        Doc.concat([
-          Doc.lparen,
-          print_expression(
-            ~parentIsArrow=false,
-            ~endChar=None,
-            ~original_source,
-            ~parent_loc,
-            first,
-          ),
-          Doc.rparen,
-        ])
-      | PExpApp(fn, _) =>
-        let leftfn = get_function_name(fn);
-        if (infixop(leftfn)) {
-          Doc.concat([
-            Doc.lparen,
-            print_expression(
-              ~parentIsArrow=false,
-              ~endChar=None,
-              ~original_source,
-              ~parent_loc,
-              first,
-            ),
-            Doc.rparen,
-          ]);
+      | PExpIf(_) => true
+      | _ => false
+      };
+
+    let right_is_if =
+      switch (second.pexp_desc) {
+      | PExpIf(_) => true
+      | _ => false
+      };
+
+    let (left_grouping_required, right_grouping_required) =
+      switch (first.pexp_desc, second.pexp_desc) {
+      | (PExpApp(fn1, _), PExpApp(fn2, _)) =>
+        let left_prec = op_precedence(get_function_name(fn1));
+        let right_prec = op_precedence(get_function_name(fn2));
+        let parent_prec = op_precedence(function_name);
+
+        // the equality check is needed for the function on the right
+        // as we process from the left by default when the same prededence
+
+        let neededLeft = left_prec < parent_prec;
+        let neededRight = right_prec <= parent_prec;
+
+        (neededLeft, neededRight);
+
+      | (PExpApp(fn1, _), _) =>
+        let left_prec = op_precedence(get_function_name(fn1));
+        let parent_prec = op_precedence(function_name);
+        if (left_prec < parent_prec) {
+          (true, false);
         } else {
-          Doc.concat([
-            Doc.lparen,
-            print_expression(
-              ~parentIsArrow=false,
-              ~endChar=None,
-              ~original_source,
-              ~parent_loc,
-              first,
-            ),
-            Doc.rparen,
-          ]);
+          (false, false);
         };
-      | _ =>
-        print_expression(
-          ~parentIsArrow=false,
-          ~endChar=None,
-          ~original_source,
-          ~parent_loc,
-          first,
-        )
+      | (_, PExpApp(fn2, _)) =>
+        let parent_prec = op_precedence(function_name);
+        let right_prec = op_precedence(get_function_name(fn2));
+        if (right_prec <= parent_prec) {
+          (false, true);
+        } else {
+          (false, false);
+        };
+
+      | _ => (false, false)
       };
 
-    let second_brackets =
-      switch (second.pexp_desc) {
-      | PExpIf(_)
-      | PExpApp(_) =>
-        Doc.concat([
-          Doc.lparen,
-          print_expression(
-            ~parentIsArrow=false,
-            ~endChar=None,
-            ~original_source,
-            ~parent_loc,
-            second,
-          ),
-          Doc.rparen,
-        ])
-      | _ =>
-        print_expression(
-          ~parentIsArrow=false,
-          ~endChar=None,
-          ~original_source,
-          ~parent_loc,
-          second,
-        )
+    let left_needs_parens = false || left_is_if || left_grouping_required;
+    let right_needs_parens = false || right_is_if || right_grouping_required;
+
+    let wrapped_left =
+      if (left_needs_parens) {
+        Doc.concat([Doc.lparen, left_expr, Doc.rparen]);
+      } else {
+        left_expr;
+      };
+
+    let wrapped_right =
+      if (right_needs_parens) {
+        Doc.concat([Doc.lparen, right_expr, Doc.rparen]);
+      } else {
+        right_expr;
       };
+
     Doc.concat([
-      first_brackets,
+      wrapped_left,
       Doc.space,
       Doc.text(function_name),
       Doc.space,
-      second_brackets,
+      wrapped_right,
     ]);
 
   | _ when prefixop(function_name) || infixop(function_name) =>
@@ -2018,7 +2055,7 @@ and print_expression =
       Doc.concat(followsArrow);
 
     | PExpApp(func, expressions) =>
-      print_application(~func, ~expressions, ~parent_loc, ~original_source)
+      print_application(~expressions, ~parent_loc, ~original_source, func)
     | PExpBlock(expressions) =>
       if (List.length(expressions) > 0) {
         let previous_line = ref(get_loc_line(expr.pexp_loc));

compiler/test/formatter_inputs/application.gr

@@ -10,6 +10,9 @@ let sillyFunction = (a,b,c,d,e,f,g,h,j,k) => 10
 
 sillyFunction(123456789,123456789,123456789,123456789,123456789,123456789,123456789,123456789,123456789,123456789)
 
+let filteri = (x,y) => []
+let findIndex =  (x,y) => None
+
 /**
  * Produces a new array with any duplicates removed.
  * Uses the generic `==` structural equality operator.

compiler/test/formatter_inputs/operators.gr

@@ -15,4 +15,4 @@ if (!b) print("false")
 
 if (a && b) print("true")
 
-if (a && b || c) print("who knows")
+if (a && (b || c)) print("who knows")

compiler/test/formatter_inputs/parens.gr

@@ -0,0 +1,36 @@
+let x = y => y + 0
+
+x(3) + 4
+
+let a = true
+let b = false
+let c = false
+
+if ((a && b) || c) {
+  print("who knows")
+}
+
+if (a && (b || c)) {
+  print("who knows")
+}
+
+3 + (4 * 5)
+
+(3 + 4) * 5
+
+3 - 4 + 5
+3 - (4 + 5)
+
+"a" ++ "b" ++ "c"
+
+
+
+  let zz = (3 - 4) + (5 - 6);
+  let zz2 = (3 - 4) * (5 - 6);
+  let zz3 = (3 * 4) * (5 - 6);
+
+print (x(3)+x(4))
+print (x(3)-x(4)+x(5))
+print (x(3)-(x(4)+x(5)))
+print ((x(3)-x(4))+x(5))
+

compiler/test/formatter_outputs/application.gr

@@ -25,6 +25,9 @@ sillyFunction(
   123456789,
 )
 
+let filteri = (x, y) => []
+let findIndex = (x, y) => None
+
 /**
  * Produces a new array with any duplicates removed.
  * Uses the generic `==` structural equality operator.
@@ -35,7 +38,7 @@ sillyFunction(
 export let unique = array => {
   filteri(
     (el, index) =>
-      (findIndex(value => value == el, array)) == (Some(index)),
+      findIndex(value => value == el, array) == Some(index),
     array,
   )
 }

compiler/test/formatter_outputs/arrays.gr

@@ -16,7 +16,7 @@ let instructions = Array.map(
       [> "acc", arg] => Acc(parseIntWithSign(arg)),
       [> "jmp", arg] => Jmp(parseIntWithSign(arg)),
       [> "nop", _] => Nop,
-      x => fail "Can't handle: " ++ (toString(x)),
+      x => fail "Can't handle: " ++ toString(x),
     }
   },
   String.split("\n", data),

compiler/test/formatter_outputs/nested_matches.gr

@@ -5,7 +5,7 @@ let join = (separator: String, items: List<String>) => {
       [hd, ...tl] => {
         let newAcc = match (acc) {
           None => Some(hd),
-          Some(s) => Some((hd ++ sep) ++ s),
+          Some(s) => Some(hd ++ sep ++ s),
         }
         iter(sep, newAcc, tl)
       },

compiler/test/formatter_outputs/operators.gr

@@ -18,6 +18,6 @@ if (a && b) {
   print("true")
 }
 
-if ((a && b) || c) {
+if (a && (b || c)) {
   print("who knows")
 }

compiler/test/formatter_outputs/parens.gr

@@ -0,0 +1,33 @@
+let x = y => y + 0
+
+x(3) + 4
+
+let a = true
+let b = false
+let c = false
+
+if (a && b || c) {
+  print("who knows")
+}
+
+if (a && (b || c)) {
+  print("who knows")
+}
+
+3 + 4 * 5
+
+(3 + 4) * 5
+
+3 - 4 + 5
+3 - (4 + 5)
+
+"a" ++ "b" ++ "c"
+
+let zz = 3 - 4 + (5 - 6)
+let zz2 = (3 - 4) * (5 - 6)
+let zz3 = 3 * 4 * (5 - 6)
+
+print(x(3) + x(4))
+print(x(3) - x(4) + x(5))
+print(x(3) - (x(4) + x(5)))
+print(x(3) - x(4) + x(5))

compiler/test/suites/formatter.re

@@ -30,4 +30,5 @@ describe("formatter", ({test}) => {
   assertFormatOutput("values", "values");
   assertFormatOutput("brace_comments", "brace_comments");
   assertFormatOutput("while_loops", "while_loops");
+  assertFormatOutput("parens", "parens");
 });