From 3a722c29dd030c58665ac431797e209b155d903b Mon Sep 17 00:00:00 2001 From: Jordan Walke Date: Fri, 27 Nov 2015 14:57:19 -0800 Subject: [PATCH] [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 | 3 ++ formatTest/formatOutput.re | 69 +++++++++++++++++++++++- formatTest/syntax.re | 2 +- formatTest/test.sh | 9 ++++ formatTest/typeCheckedTests/sequences.re | 52 ++++++++++++++++++ src/reason_parser.mly | 47 +++++++++++++--- src/reason_pprint_ast.ml | 17 ++++-- 7 files changed, 184 insertions(+), 15 deletions(-) create mode 100644 formatTest/.gitignore create mode 100644 formatTest/typeCheckedTests/sequences.re diff --git a/formatTest/.gitignore b/formatTest/.gitignore new file mode 100644 index 000000000..374959514 --- /dev/null +++ b/formatTest/.gitignore @@ -0,0 +1,3 @@ +typeCheckedTests/*.out +typeCheckedTests/*.cmo +typeCheckedTests/*.cmi diff --git a/formatTest/formatOutput.re b/formatTest/formatOutput.re index bc6c9a22a..9b759ec42 100644 --- a/formatTest/formatOutput.re +++ b/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; diff --git a/formatTest/syntax.re b/formatTest/syntax.re index 97c766223..dd508289e 100644 --- a/formatTest/syntax.re +++ b/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; diff --git a/formatTest/test.sh b/formatTest/test.sh index 3f48df58a..9b20f042e 100755 --- a/formatTest/test.sh +++ b/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 diff --git a/formatTest/typeCheckedTests/sequences.re b/formatTest/typeCheckedTests/sequences.re new file mode 100644 index 000000000..11cb4290e --- /dev/null +++ b/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; \ No newline at end of file diff --git a/src/reason_parser.mly b/src/reason_parser.mly index 269a8b1ff..1f3329317 100644 --- a/src/reason_parser.mly +++ b/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) } diff --git a/src/reason_pprint_ast.ml b/src/reason_pprint_ast.ml index fa3756ee1..d90fb1f42 100644 --- a/src/reason_pprint_ast.ml +++ b/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