From 9c88b89db4ce1d7a256cbde51d74225513b76a3e Mon Sep 17 00:00:00 2001 From: Marcus Roberts Date: Tue, 14 Sep 2021 07:35:51 +0100 Subject: [PATCH] fix(grainfmt): Resolve issues with comments within braces (#888) Fix up multiple formatter issues, Tidied up the tests, remove unsafe examples from tests --- compiler/grainformat/debug.re | 8 +- compiler/grainformat/reformat.re | 211 ++++++++++++++---- compiler/grainformat/walktree.re | 13 +- .../test/formatter_inputs/brace_comments.gr | 17 ++ compiler/test/formatter_inputs/enums.gr | 2 +- .../test/formatter_inputs/function_params.gr | 12 +- compiler/test/formatter_inputs/records.gr | 52 ++++- .../test/formatter_outputs/brace_comments.gr | 17 ++ compiler/test/formatter_outputs/enums.gr | 2 +- .../test/formatter_outputs/function_params.gr | 6 +- compiler/test/formatter_outputs/records.gr | 30 ++- compiler/test/suites/formatter.re | 1 + 12 files changed, 295 insertions(+), 76 deletions(-) create mode 100644 compiler/test/formatter_inputs/brace_comments.gr create mode 100644 compiler/test/formatter_outputs/brace_comments.gr diff --git a/compiler/grainformat/debug.re b/compiler/grainformat/debug.re index ed1d9859e8..0c1c34b937 100644 --- a/compiler/grainformat/debug.re +++ b/compiler/grainformat/debug.re @@ -10,10 +10,10 @@ let print_loc_string = (msg: string, loc: Grain_parsing.Location.t) => { if (startchar >= 0) { if (line == endline) { - Printf.sprintf("%s %d:%d,%d", msg, line, startchar, endchar); + Printf.sprintf("%s %d:%d,%d\n", msg, line, startchar, endchar); } else { Printf.sprintf( - "%s %d:%d - %d:%d", + "%s %d:%d - %d:%d\n", msg, line, startchar, @@ -32,10 +32,10 @@ let print_loc = (msg: string, loc: Grain_parsing.Location.t) => { if (startchar >= 0) { if (line == endline) { - Printf.printf("%s %d:%d,%d", msg, line, startchar, endchar); + Printf.printf("%s %d:%d,%d\n", msg, line, startchar, endchar); } else { Printf.printf( - "%s %d:%d - %d:%d", + "%s %d:%d - %d:%d\n", msg, line, startchar, diff --git a/compiler/grainformat/reformat.re b/compiler/grainformat/reformat.re index b289e65834..1844916d7b 100644 --- a/compiler/grainformat/reformat.re +++ b/compiler/grainformat/reformat.re @@ -82,7 +82,23 @@ let make_line_start = (loc: Grain_parsing.Location.t) => { let merged_loc: Grain_parsing.Location.t = { loc_start: startpos, loc_end: loc.loc_start, - loc_ghost: loc.loc_ghost, + loc_ghost: true, + }; + merged_loc; +}; + +let make_to_line_end = (loc: Grain_parsing.Location.t) => { + let endpos: Stdlib__lexing.position = { + pos_fname: "", + pos_lnum: loc.loc_end.pos_lnum, + pos_cnum: max_int, + pos_bol: 0, + }; + + let merged_loc: Grain_parsing.Location.t = { + loc_start: loc.loc_end, + loc_end: endpos, + loc_ghost: true, }; merged_loc; }; @@ -359,14 +375,6 @@ let add_parens = doc => Doc.rparen, ]); -let add_braces = doc => - Doc.concat([ - Doc.lbrace, - Doc.indent(Doc.concat([Doc.softLine, doc])), - Doc.softLine, - Doc.rbrace, - ]); - let infixop = (op: string) => { switch (op) { | "+" @@ -568,6 +576,7 @@ and print_record_pattern = ~closedflag: Grain_parsing__Asttypes.closed_flag, ~parent_loc: Grain_parsing__Location.t, ~original_source: array(string), + patloc: Grain_parsing__Location.t, ) => { let close = switch (closedflag) { @@ -576,43 +585,48 @@ and print_record_pattern = }; Doc.concat([ Doc.lbrace, - Doc.space, - Doc.join( - Doc.concat([Doc.comma, Doc.space]), - List.map( - ( - patternloc: ( - Grain_parsing__Location.loc(Grain_parsing__Identifier.t), - Grain_parsing__Parsetree.pattern, - ), - ) => { - let (loc, pat) = patternloc; + get_trailing_comments_to_end_of_line(patloc), + Doc.indent( + Doc.concat([ + Doc.line, + Doc.join( + Doc.concat([Doc.comma, Doc.line]), + List.map( + ( + patternloc: ( + Grain_parsing__Location.loc(Grain_parsing__Identifier.t), + Grain_parsing__Parsetree.pattern, + ), + ) => { + let (loc, pat) = patternloc; - let printed_ident: Doc.t = print_ident(loc.txt); - let printed_pat = - print_pattern(~pat, ~parent_loc, ~original_source); + let printed_ident: Doc.t = print_ident(loc.txt); + let printed_pat = + print_pattern(~pat, ~parent_loc, ~original_source); - let pun = - switch (printed_ident, printed_pat: Doc.t) { - | (Text(i), Text(e)) => i == e - | _ => false - }; - if (pun) { - printed_ident; - } else { - Doc.concat([ - printed_ident, - Doc.text(":"), - Doc.space, - printed_pat, - ]); - }; - }, - patternlocs, - ), + let pun = + switch (printed_ident, printed_pat: Doc.t) { + | (Text(i), Text(e)) => i == e + | _ => false + }; + if (pun) { + printed_ident; + } else { + Doc.concat([ + printed_ident, + Doc.text(":"), + Doc.space, + printed_pat, + ]); + }; + }, + patternlocs, + ), + ), + close, + ]), ), - close, - Doc.space, + Doc.line, Doc.rbrace, ]); } @@ -692,6 +706,7 @@ and print_pattern = ~closedflag, ~parent_loc, ~original_source, + pat.ppat_loc, ), false, ) @@ -845,9 +860,11 @@ and print_record = ), ~parent_loc: Grain_parsing__Location.t, ~original_source: array(string), + recloc: Grain_parsing__Location.t, ) => Doc.concat([ Doc.lbrace, + get_trailing_comments_to_end_of_line(recloc), Doc.indent( Doc.concat([ Doc.line, @@ -1186,6 +1203,73 @@ and check_for_pun = (expr: Parsetree.expression) => | _ => Doc.nil } +and get_trailing_comments_to_next_code = (loc: Grain_parsing__Location.t) => { + // just looking for any comments left on this line as they will come + // after the lbrace + + let (_leading_comments, trailing_comments) = + Walktree.partition_comments(loc, None); + + let expr_line = get_loc_line(loc); + + let trailing_comment_docs = + if (List.length(trailing_comments) > 0) { + Doc.concat([ + print_multi_comments(trailing_comments, expr_line), + Doc.ifBreaks(Doc.nil, Doc.hardLine), + ]); + } else { + Doc.nil; + }; + + Walktree.remove_used_comments([], trailing_comments); + trailing_comment_docs; +} + +and get_trailing_comments_to_end_of_line = + (block_location: Grain_parsing__Location.t) => { + let loc = make_start_loc(block_location); // partition from the start location of the expression + let range = make_to_line_end(loc); + + let (_leading_comments, trailing_comments) = + Walktree.partition_comments(loc, Some(range)); + + let expr_line = get_end_loc_line(range); + + let trailing_comment_docs = + if (List.length(trailing_comments) > 0) { + Doc.concat([ + print_multi_comments(trailing_comments, expr_line), + Doc.ifBreaks(Doc.nil, Doc.hardLine), + ]); + } else { + Doc.nil; + }; + + Walktree.remove_used_comments([], trailing_comments); + trailing_comment_docs; +} + +and get_trailing_top_level_comments = + (block_location: Grain_parsing__Location.t) => { + let loc = make_end_loc(block_location); // partition from the start location of the expression + let range = make_to_line_end(loc); + + let (_leading_comments, trailing_comments) = + Walktree.partition_comments(loc, Some(range)); + + let expr_line = get_end_loc_line(range); + + let trailing_comment_docs = + if (List.length(trailing_comments) > 0) { + Doc.concat([print_multi_comments(trailing_comments, expr_line)]); + } else { + Doc.nil; + }; + + Walktree.remove_used_comments([], trailing_comments); + trailing_comment_docs; +} and print_expression = ( ~parentIsArrow: bool, @@ -1340,7 +1424,12 @@ and print_expression = ) | PExpRecord(record) => - print_record(~fields=record, ~parent_loc, ~original_source) + print_record( + ~fields=record, + ~parent_loc, + ~original_source, + expr.pexp_loc, + ) | PExpRecordGet(expression, {txt, _}) => Doc.concat([ print_expression( @@ -1384,7 +1473,7 @@ and print_expression = ~parentIsArrow=false, ~endChar=None, ~original_source, - ~parent_loc, + ~parent_loc=expression.pexp_loc, expression, ), ), @@ -1395,7 +1484,8 @@ and print_expression = ~forceBreak=false, Doc.concat([ Doc.concat([Doc.text("match "), arg, Doc.space]), - Doc.text("{"), + Doc.lbrace, + get_trailing_comments_to_end_of_line(expression.pexp_loc), Doc.indent( Doc.concat([ Doc.hardLine, @@ -1511,7 +1601,7 @@ and print_expression = ]), ), Doc.line, - Doc.text("}"), + Doc.rbrace, ]), ); @@ -1529,7 +1619,7 @@ and print_expression = Doc.text(originalCode); | PExpIf(condition, trueExpr, falseExpr) => let (leading_condition_comments, _trailing_comments) = - Walktree.partition_comments(condition.pexp_loc, Some(parent_loc)); + Walktree.partition_comments(condition.pexp_loc, None); Walktree.remove_used_comments(leading_condition_comments, []); let expr_line = get_end_loc_line(condition.pexp_loc); @@ -1568,6 +1658,7 @@ and print_expression = | _ => Doc.concat([ Doc.lbrace, + // no comment to add here as this was a single line expression Doc.indent( Doc.concat([ Doc.hardLine, @@ -1623,6 +1714,7 @@ and print_expression = Doc.concat([ Doc.space, Doc.lbrace, + // no comments to add here as original was single line Doc.indent( Doc.concat([ Doc.hardLine, @@ -1658,6 +1750,7 @@ and print_expression = ~parent_loc, condition, ), + get_trailing_comments_to_next_code(condition.pexp_loc), Doc.rparen, Doc.space, ]), @@ -1681,6 +1774,7 @@ and print_expression = ~parent_loc, condition, ), + get_trailing_comments_to_next_code(condition.pexp_loc), Doc.rparen, Doc.space, ]), @@ -1910,6 +2004,8 @@ and print_expression = | PExpBlock(expressions) => if (List.length(expressions) > 0) { let previous_line = ref(get_loc_line(expr.pexp_loc)); + let after_brace_comments = + get_trailing_comments_to_end_of_line(expr.pexp_loc); let block = Doc.join( Doc.hardLine, @@ -1919,6 +2015,9 @@ and print_expression = let (leading_comments, _trailing_comments) = Walktree.partition_comments(e.pexp_loc, None); + let line_end_comments = + get_trailing_comments_to_end_of_line(e.pexp_loc); + let disable_formatting = List.exists( is_disable_formatting_comment, @@ -1949,6 +2048,12 @@ and print_expression = get_original_code(e.pexp_loc, original_source); Walktree.remove_comments_in_ignore_block(e.pexp_loc); + // get any trailing comments as the find doesn't look past the end of the + // statement itself + // we throw these away + let _after_brace_comments = + get_trailing_comments_to_next_code(e.pexp_loc); + Doc.concat([ stmt_leading_comment_docs, blank_line_above, @@ -1971,6 +2076,7 @@ and print_expression = stmt_leading_comment_docs, blank_line_above, Doc.group(printed_expression), + line_end_comments, ]), ); }; @@ -1996,6 +2102,7 @@ and print_expression = ~forceBreak=true, Doc.concat([ Doc.lbrace, + after_brace_comments, Doc.indent(Doc.concat([Doc.line, block, blockEndCommentDocs])), Doc.line, Doc.rbrace, @@ -2022,6 +2129,7 @@ and print_expression = ~forceBreak=true, Doc.concat([ Doc.lbrace, + // no post brace here as this was a single line statement Doc.indent(Doc.concat([Doc.line, blockCommentDocs])), Doc.line, Doc.rbrace, @@ -2261,6 +2369,7 @@ and print_value_bind = Doc.space, Doc.equal, Doc.group(expressionGrp), + get_trailing_comments_to_end_of_line(vb.pvb_expr.pexp_loc), ]); }, vbs, @@ -2373,6 +2482,7 @@ let rec print_data = Doc.space; }, Doc.lbrace, + get_trailing_comments_to_end_of_line(data.pdata_name.loc), Doc.indent(Doc.concat([Doc.line, joinedDecls])), if (!commaTerminated) { Doc.ifBreaks(Doc.comma, Doc.nil); @@ -2458,6 +2568,7 @@ let rec print_data = }, Doc.concat([ Doc.lbrace, + get_trailing_comments_to_end_of_line(data.pdata_name.loc), Doc.indent(Doc.concat([Doc.line, joinedDecls])), if (!commaTerminated) { Doc.ifBreaks(Doc.comma, Doc.nil); @@ -2573,6 +2684,7 @@ let import_print = (imp: Parsetree.import_declaration) => { let numVals = List.length(identlocsopts); Doc.concat([ Doc.lbrace, + // get_trailing_comments_to_end_of_line(imp.pimp_loc), Doc.indent( Doc.concat([ Doc.line, @@ -2778,6 +2890,8 @@ let toplevel_print = let (leading_comments, _trailing_comments) = Walktree.partition_comments(data.ptop_loc, None); + let line_end_comments = get_trailing_top_level_comments(data.ptop_loc); + // check to see if we have a comment to disable formatting let disable_formatting = @@ -2960,6 +3074,7 @@ let toplevel_print = blank_line_above, attribute_text, Doc.group(without_comments), + line_end_comments, ]); }; }; diff --git a/compiler/grainformat/walktree.re b/compiler/grainformat/walktree.re index d98fc24153..5147bc398d 100644 --- a/compiler/grainformat/walktree.re +++ b/compiler/grainformat/walktree.re @@ -69,7 +69,7 @@ let is_first_inside_second = let begins_inside = if (raw1l > raw2l) { true; - } else if (raw1c >= raw2c) { + } else if (raw1c >= raw2c && raw1c <= raw2ce) { true; } else { false; @@ -77,9 +77,15 @@ let is_first_inside_second = let ends_inside = if (raw1le < raw2le) { true; + } else if (raw1le == raw2le) { + if (raw1ce <= raw2ce) { + true; + } else { + false; + }; } else { - true; - }; // ignore the end of line so we catch trailing comments + false; + }; begins_inside && ends_inside; }; @@ -201,7 +207,6 @@ let partition_comments = ([], []), all_locations^, ); - (preceeding, following); }; diff --git a/compiler/test/formatter_inputs/brace_comments.gr b/compiler/test/formatter_inputs/brace_comments.gr new file mode 100644 index 0000000000..0276f2eb77 --- /dev/null +++ b/compiler/test/formatter_inputs/brace_comments.gr @@ -0,0 +1,17 @@ +match (1) { // fails + // fails2 + 1 => "1", + _ => "*", // x +} + +if (true) { // block 1 + // block 2 + 1 // block 2a + // block 2b +} else { // block3 + // block 4 + 2 // block 5 + // block 6 +} + +let space = "" diff --git a/compiler/test/formatter_inputs/enums.gr b/compiler/test/formatter_inputs/enums.gr index 687eeb51fb..b9c130a505 100644 --- a/compiler/test/formatter_inputs/enums.gr +++ b/compiler/test/formatter_inputs/enums.gr @@ -1,4 +1,4 @@ -export enum Instructions { +export enum Instructions { // brace trailer // leading MoveNorth(Number), // north // below north diff --git a/compiler/test/formatter_inputs/function_params.gr b/compiler/test/formatter_inputs/function_params.gr index 0c4e73c1cb..0951bdce45 100644 --- a/compiler/test/formatter_inputs/function_params.gr +++ b/compiler/test/formatter_inputs/function_params.gr @@ -4,10 +4,12 @@ let unit_arg = () => 3 let two_args = (x,y) => 4 -export let fd_write: (WasmI32, WasmI32, WasmI32, WasmI32) -> WasmI32 = - (fd, +export let fake_write: (int, int, int, string) -> string = + ( + fd, iovs, iovs_len, - nwritten) => { - 0n -} \ No newline at end of file + nwritten, + ) => { + "ok" +} diff --git a/compiler/test/formatter_inputs/records.gr b/compiler/test/formatter_inputs/records.gr index 25a4e97e85..11c825310b 100644 --- a/compiler/test/formatter_inputs/records.gr +++ b/compiler/test/formatter_inputs/records.gr @@ -3,15 +3,21 @@ let x = {foo: 4, bar: 9} x.foo + x.bar +record Rec1 { foo2: Number, bar2: Number } +record Rec2 { baz2: Rec1 } +let x = { baz2: { foo2: 4, bar2: 9 }, } +x.baz2.bar2 + +record Rec3 { foo3: Number, bar3: String, baz3: Bool } +let { foo3, _ } = { foo3: 4, bar3: "boo", baz3: true } +foo3 + +record Long {longField1: Number,longField2: Number,longField3: String, + longField4: String, + longField5: String, +} - -record Rec1 {foo2: Number, bar2: Number}; record Rec2 {baz2: Rec1}; let x = {baz2: {foo2: 4, bar2: 9}}; x.baz2.bar2 - -record Rec3 {foo3: Number, bar3: String, baz3: Bool}; let { foo3, _ } = {foo3: 4, bar3: "boo", baz3: true}; foo3 - -record Long {longField1: Number, longField2: Number, longField3:String, longField4:String, longField5:String} - -record Commented { +record Commented { // brace comment // leading name: String, // trail 1 // under 1 @@ -24,5 +30,33 @@ record Commented { record Bucket { mut key: k, mut value: v, - mut nextkey: Option> + mut nextkey: Option>, +} + +record Commented { // brace comment + // leading + longlonglongnamenamename1: String, // trail 1 + // under 1 + longlonglongnamenamename2: String, /* + stupid block */ + longlonglongnamenamename3: Number, + // trailing } + +let x: Commented = { // comment 1 + longlonglongnamenamename1: "A", + longlonglongnamenamename2: "B", + longlonglongnamenamename3: 42, +} + +let { // a comment 2 + longlonglongnamenamename1, + longlonglongnamenamename2, + longlonglongnamenamename3 +} = x + +let { // a comment 3 + l1, + l2, + l3 +} = x diff --git a/compiler/test/formatter_outputs/brace_comments.gr b/compiler/test/formatter_outputs/brace_comments.gr new file mode 100644 index 0000000000..0276f2eb77 --- /dev/null +++ b/compiler/test/formatter_outputs/brace_comments.gr @@ -0,0 +1,17 @@ +match (1) { // fails + // fails2 + 1 => "1", + _ => "*", // x +} + +if (true) { // block 1 + // block 2 + 1 // block 2a + // block 2b +} else { // block3 + // block 4 + 2 // block 5 + // block 6 +} + +let space = "" diff --git a/compiler/test/formatter_outputs/enums.gr b/compiler/test/formatter_outputs/enums.gr index 145066f931..ac6c013971 100644 --- a/compiler/test/formatter_outputs/enums.gr +++ b/compiler/test/formatter_outputs/enums.gr @@ -1,4 +1,4 @@ -export enum Instructions { +export enum Instructions { // brace trailer // leading MoveNorth(Number), // north // below north diff --git a/compiler/test/formatter_outputs/function_params.gr b/compiler/test/formatter_outputs/function_params.gr index b6884921df..c363d53771 100644 --- a/compiler/test/formatter_outputs/function_params.gr +++ b/compiler/test/formatter_outputs/function_params.gr @@ -4,12 +4,12 @@ let unit_arg = () => 3 let two_args = (x, y) => 4 -export let fd_write: (WasmI32, WasmI32, WasmI32, WasmI32) -> WasmI32 = +export let fake_write: (int, int, int, string) -> string = ( fd, iovs, iovs_len, nwritten, ) => { - 0n -} \ No newline at end of file + "ok" +} diff --git a/compiler/test/formatter_outputs/records.gr b/compiler/test/formatter_outputs/records.gr index c5f08dda6f..3ff361f209 100644 --- a/compiler/test/formatter_outputs/records.gr +++ b/compiler/test/formatter_outputs/records.gr @@ -20,7 +20,7 @@ record Long { longField5: String, } -record Commented { +record Commented { // brace comment // leading name: String, // trail 1 // under 1 @@ -35,3 +35,31 @@ record Bucket { mut value: v, mut nextkey: Option>, } + +record Commented { // brace comment + // leading + longlonglongnamenamename1: String, // trail 1 + // under 1 + longlonglongnamenamename2: String, /* + stupid block */ + longlonglongnamenamename3: Number, + // trailing +} + +let x: Commented = { // comment 1 + longlonglongnamenamename1: "A", + longlonglongnamenamename2: "B", + longlonglongnamenamename3: 42, +} + +let { // a comment 2 + longlonglongnamenamename1, + longlonglongnamenamename2, + longlonglongnamenamename3 +} = x + +let { // a comment 3 + l1, + l2, + l3 +} = x diff --git a/compiler/test/suites/formatter.re b/compiler/test/suites/formatter.re index 4cbef87c35..cb8f07385e 100644 --- a/compiler/test/suites/formatter.re +++ b/compiler/test/suites/formatter.re @@ -28,4 +28,5 @@ describe("formatter", ({test}) => { assertFormatOutput("ignores", "ignores"); assertFormatOutput("list_sugar", "list_sugar"); assertFormatOutput("values", "values"); + assertFormatOutput("brace_comments", "brace_comments"); });