[Reason] Semicolon is not required after final item in sequence.

Summary:Per request of @jberdine, we no longer require the final
semicolon. There's a small tradeoff that had to be made (no punned,
single item records) - but that case is very rare, and would surely be
caught by the type system.

I also took the opportunity to start a clean directory of type checked
formatting test cases. Many bugs in parsing/printing would be caught by
simple type checking of the AST.

Thanks for the suggesion, Josh. I like this much better.

The next step is to make it so when pretty printing, the final semicolon
is ommitted.

Similarly, it is easy to make it such that the final item in a
module/signature does not require a semicolon.

Test Plan:Added type-checked formatting tests.

Reviewers:@jberdine, @cristianoc

CC:

formatTest/.gitignore

@@ -0,0 +1,3 @@
+typeCheckedTests/*.out
+typeCheckedTests/*.cmo
+typeCheckedTests/*.cmi

formatTest/formatOutput.re

@@ -4763,7 +4763,8 @@ type hasA = {a: int};
 
 let a = 10;
 
-let thisReturnsARecord () => {a};
+let returnsASequenceExpressionWithASingleIdentifier 
+    () => a;
 
 let thisReturnsA () => a;
 
@@ -6123,3 +6124,69 @@ let module M = Something.Create {
   type resource1 = MyModule.MySubmodule.t;
   type resource2 = MyModule.MySubmodule.t;
 };
+let result = {
+  let twenty = 20;
+  let result = twenty;
+  result;
+};
+
+/* Final semicolon is not required */
+let result = {
+  let twenty = result;
+  twenty;
+};
+
+let anInt = result + 20;
+
+let twenty = 20;
+
+/**
+ * Each of these are a sequence with a single item - they will be
+ * printed in reduced form because sequences are a *parse* time construct.
+ * To ensure these are parsed correctly, adding to an integer.
+ */
+let result = 0 + twenty;
+
+let result = 0 + twenty;
+
+let result = 0 + twenty;
+
+let unitValue = ();
+
+/* While loops/for loops merely accept a "simple expression" (which means
+ * it is either a simple token or balanced with parens/braces). However,
+ * the formatter ensures that the bodies are printed in "sequence" form even if
+ * it's not required.
+ */
+while false {
+  unitValue;
+};
+
+while false {
+  print_string "test";
+};
+
+while false {
+  print_string "test";
+};
+
+type myRecord = {number: int};
+
+let x = {number: 20};
+
+let number = 20;
+
+/*
+ * The (mild) consequence of not requiring a final semi in a sequence,
+ * is that we can no longer "pun" a single field record (which would)
+ * be very rare anyways.
+ */
+let cannotPunASingleFieldRecord = {
+  number: number
+};
+
+let fourty = 20 + cannotPunASingleFieldRecord.number;
+
+let thisIsASequenceNotPunedRecord = number;
+
+let fourty = 20 + thisIsASequenceNotPunedRecord;

formatTest/syntax.re

@@ -350,7 +350,7 @@ let onlyDoingThisTopLevelLetToBypassTopLevelSequence = {
 
 type hasA = {a:int};
 let a = 10;
-let thisReturnsARecord () => {a};
+let returnsASequenceExpressionWithASingleIdentifier () => {a};
 let thisReturnsA () => {a;};
 let thisReturnsAAsWell () => a;
 

formatTest/test.sh

@@ -22,3 +22,12 @@
 ../reasonfmt_impl.native -print-width 50 -print re ./trailingSpaces.re 2>&1 >>./formatOutput.re
 
 ../reasonfmt_impl.native -print-width 50 -print re ./wrappingTest.rei 2>&1 >./formatOutput.rei
+
+
+
+# Let's start creating formatting test cases that must type check.
+# Errors in parsing/printing often are caught via the type system.
+ocamlc -c -pp ../reasonfmt_impl.native -intf-suffix rei -impl ./typeCheckedTests/sequences.re
+rm ./typeCheckedTests/sequences.cmi
+rm ./typeCheckedTests/sequences.cmo
+../reasonfmt_impl.native -print-width 50 -print re ./typeCheckedTests/sequences.re 2>&1 >>./formatOutput.re

formatTest/typeCheckedTests/sequences.re

@@ -0,0 +1,52 @@
+let result = {
+  let twenty = 20;
+  let result = twenty;
+  result;
+};
+
+/* Final semicolon is not required */
+let result = {
+  let twenty = result;
+  twenty
+};
+let anInt = result + 20;
+
+let twenty = 20;
+
+/**
+ * Each of these are a sequence with a single item - they will be
+ * printed in reduced form because sequences are a *parse* time construct.
+ * To ensure these are parsed correctly, adding to an integer.
+ */
+let result = 0 + {twenty};
+let result = 0 + {twenty;};
+let result = 0 + twenty;
+
+let unitValue = ();
+/* While loops/for loops merely accept a "simple expression" (which means
+ * it is either a simple token or balanced with parens/braces). However,
+ * the formatter ensures that the bodies are printed in "sequence" form even if
+ * it's not required.
+ */
+while false unitValue;
+while (false) {
+  print_string "test"
+};
+while (false) {
+  print_string "test";
+};
+
+type myRecord = {
+  number: int
+};
+let x = {number:20};
+let number = 20;
+/*
+ * The (mild) consequence of not requiring a final semi in a sequence,
+ * is that we can no longer "pun" a single field record (which would)
+ * be very rare anyways.
+ */
+let cannotPunASingleFieldRecord = {number: number};
+let fourty = 20 + cannotPunASingleFieldRecord.number;
+let thisIsASequenceNotPunedRecord = {number};
+let fourty = 20 + thisIsASequenceNotPunedRecord;

src/reason_parser.mly

@@ -1444,7 +1444,7 @@ semi_terminated_seq_expr:
    *    a + b;
    *  };
    */
-  | expr SEMI  { reloc_exp $1 }
+  | expr opt_semi  { reloc_exp $1 }
   | LET ext_attributes rec_flag let_bindings_no_attrs SEMI semi_terminated_seq_expr
       { mkexp_attrs (Pexp_let($3, List.rev $4, $6)) $2 }
   | LET MODULE ext_attributes UIDENT module_binding_body SEMI semi_terminated_seq_expr
@@ -2129,7 +2129,7 @@ expr_optional_constraint:
 
 record_expr:
     DOTDOTDOT expr_optional_constraint COMMA lbl_expr_list   { (Some $2, $4) }
-  | lbl_expr_list                               { (None, $1) }
+  | lbl_expr_list_with_at_least_one_non_punned_field         { (None, $1) }
 ;
 lbl_expr_list:
      lbl_expr { [$1] }
@@ -2143,14 +2143,37 @@ lbl_expr:
       { (mkrhs $1 1, exp_of_label $1 1) }
 ;
 
-field_expr:
-  LIDENT COLON expr
-    { (mkrhs $1 1, $3) }
- | LIDENT
-    { (mkrhs $1 1, mkexp (Pexp_ident(mkrhs (Lident $1) 1))) }
+non_punned_lbl_expression:
+  label_longident COLON expr
+      { (mkrhs $1 1, $3) }
 ;
 
-field_expr_list:
+/**
+ * To allow sequence expressions to not *require* a final semicolon, we make a small tradeoff:
+ * {label} would normally be an ambiguity: Is it a punned record, or a sequence expression
+ * with only one item? It makes more sense to break the tie by interpreting it as a sequence
+ * expression with a single item, as opposed to a punned record with one field. Justification:
+ *
+ * - Constructing single field records is very rare.
+ * - Block sequences should not require a semicolon after their final item:
+ *   - let something = {print "hi"; foo};
+ *   - We often want to be able to print *single* values in block form, without a trailing semicolon.
+ *   - You should be able to delete `print "hi"` in the above statement and still parse intuitively.
+ *   - An equivalent scenario when you delete all but a single field from a punned record is more rare:
+ *     -  let myRecord = {deleteThisField, butLeaveThisOne};
+ * - For whatever tiny remaining confusion would occur, virtually all of them would be caught by the type system.
+ */
+lbl_expr_list_with_at_least_one_non_punned_field:
+  | non_punned_lbl_expression
+     { [$1] }
+  | non_punned_lbl_expression COMMA lbl_expr_list
+      { $1::$3 }
+;
+
+/**
+ * field_expr is distinct from record_expr because labels cannot/shouldn't be scoped.
+ */
+field_expr:
   /* Using LIDENT instead of label here, because a reduce/reduce conflict occurs on:
    *   {blah:x}
    *
@@ -2159,12 +2182,20 @@ field_expr_list:
    * Another approach would have been to place the `label` rule at at a precedence
    * of below_COLON or something.
    */
+  LIDENT COLON expr
+    { (mkrhs $1 1, $3) }
+ | LIDENT
+    { (mkrhs $1 1, mkexp (Pexp_ident(mkrhs (Lident $1) 1))) }
+;
+
+field_expr_list:
   field_expr
      { [$1] }
   | field_expr_list COMMA field_expr
       { $3::$1 }
 ;
 
+
 type_constraint_right_of_colon:
     core_type                             { (Some $1, None) }
   | core_type COLONGREATER core_type      { (Some $1, Some $3) }

src/reason_pprint_ast.ml

@@ -1235,6 +1235,7 @@ let layoutEasy x = x
 
 let semiTerminated term = makeList [term; atom ";"]
 
+(* TODO: Do not print the final semicolon *)
 let makeLetSequence letItems =
   makeList ~wrap:("{", "}") ~break:Always ~inline:(true, false) letItems
 
@@ -3289,7 +3290,7 @@ class printer  ()= object(self:'self)
         | Pexp_variant (l, None) ->
             ensureSingleTokenSticksToLabel (atom ("`" ^ l))
         | Pexp_record (l, eo) ->
-            let makeRow (li, e) appendComma =
+            let makeRow (li, e) appendComma shouldPun =
               let comma = atom "," in
               let totalRowLoc = {
                 loc_start = li.Asttypes.loc.loc_start;
@@ -3299,7 +3300,7 @@ class printer  ()= object(self:'self)
               let theRow =
                 match e.pexp_desc with
                   (* Punning *)
-                  |  Pexp_ident {txt} when li.txt = txt ->
+                  |  Pexp_ident {txt} when li.txt = txt && shouldPun ->
                       makeList ((self#longident_loc li)::(if appendComma then [comma] else []))
                   | _ ->
                      let (argsList, return) = self#curriedPatternsAndReturnVal e in (
@@ -3328,12 +3329,18 @@ class printer  ()= object(self:'self)
             let rec getRows l =
               match l with
                 | [] -> []
-                | hd::[] -> [makeRow hd false]
-                | hd::hd2::tl -> (makeRow hd true)::(getRows (hd2::tl))
+                | hd::[] -> [makeRow hd false true]
+                | hd::hd2::tl -> (makeRow hd true true)::(getRows (hd2::tl))
             in
 
             let allRows = match eo with
-              | None -> getRows l
+              | None -> (
+                match l with
+                  (* No punning (or comma) for records with only a single field. *)
+                  (* See comment in parser.mly for lbl_expr_list_with_at_least_one_non_punned_field *)
+                  | [hd] -> [makeRow hd false false]
+                  | _ -> getRows l
+                )
               | Some withRecord ->
                 let firstRow = (
                   match self#expressionToFormattedApplicationItems withRecord with