Commit 1e3a200ba6

Andrew Kelley <andrew@ziglang.org>
2021-02-23 07:25:12
zig fmt: array literal with hint
This regresses the test case of `zig fmt` deleting empty line comments. Two open questions here: * What should the rules be about deleting empty line comments? It makes sense usually, but for array initization, empty line comments cause a line break, affecting the row/column alignment. Perhaps we should therefore respect all empty line comments? Or should we special case array initializations? * If we decide to special case some kinds of line comments to respect them (which is status quo!), how should that be implemented?
1 parent 1253903
Changed files (2)
lib/std/zig/parser_test.zig
@@ -1655,112 +1655,110 @@ test "zig fmt: struct literal containing a multiline expression" {
     );
 }
 
-//test "zig fmt: array literal with hint" {
-//    try testTransform(
-//        \\const a = []u8{
-//        \\    1, 2, //
-//        \\    3,
-//        \\    4,
-//        \\    5,
-//        \\    6,
-//        \\    7 };
-//        \\const a = []u8{
-//        \\    1, 2, //
-//        \\    3,
-//        \\    4,
-//        \\    5,
-//        \\    6,
-//        \\    7, 8 };
-//        \\const a = []u8{
-//        \\    1, 2, //
-//        \\    3,
-//        \\    4,
-//        \\    5,
-//        \\    6, // blah
-//        \\    7, 8 };
-//        \\const a = []u8{
-//        \\    1, 2, //
-//        \\    3, //
-//        \\    4,
-//        \\    5,
-//        \\    6,
-//        \\    7 };
-//        \\const a = []u8{
-//        \\    1,
-//        \\    2,
-//        \\    3, 4, //
-//        \\    5, 6, //
-//        \\    7, 8, //
-//        \\};
-//    ,
-//        \\const a = []u8{
-//        \\    1, 2,
-//        \\    3, 4,
-//        \\    5, 6,
-//        \\    7,
-//        \\};
-//        \\const a = []u8{
-//        \\    1, 2,
-//        \\    3, 4,
-//        \\    5, 6,
-//        \\    7, 8,
-//        \\};
-//        \\const a = []u8{
-//        \\    1, 2,
-//        \\    3, 4,
-//        \\    5,
-//        \\    6, // blah
-//        \\    7,
-//        \\    8,
-//        \\};
-//        \\const a = []u8{
-//        \\    1, 2,
-//        \\    3, //
-//        \\    4,
-//        \\    5, 6,
-//        \\    7,
-//        \\};
-//        \\const a = []u8{
-//        \\    1,
-//        \\    2,
-//        \\    3,
-//        \\    4,
-//        \\    5,
-//        \\    6,
-//        \\    7,
-//        \\    8,
-//        \\};
-//        \\
-//    );
-//}
-//
-//test "zig fmt: array literal veritical column alignment" {
-//    try testTransform(
-//        \\const a = []u8{
-//        \\    1000, 200,
-//        \\    30, 4,
-//        \\    50000, 60
-//        \\};
-//        \\const a = []u8{0,   1, 2, 3, 40,
-//        \\    4,5,600,7,
-//        \\           80,
-//        \\    9, 10, 11, 0, 13, 14, 15};
-//        \\
-//    ,
-//        \\const a = []u8{
-//        \\    1000,  200,
-//        \\    30,    4,
-//        \\    50000, 60,
-//        \\};
-//        \\const a = []u8{
-//        \\    0,  1,  2,   3, 40,
-//        \\    4,  5,  600, 7, 80,
-//        \\    9,  10, 11,  0, 13,
-//        \\    14, 15,
-//        \\};
-//        \\
-//    );
-//}
+test "zig fmt: array literal with hint" {
+    try testTransform(
+        \\const a = []u8{
+        \\    1, 2, //
+        \\    3,
+        \\    4,
+        \\    5,
+        \\    6,
+        \\    7 };
+        \\const a = []u8{
+        \\    1, 2, //
+        \\    3,
+        \\    4,
+        \\    5,
+        \\    6,
+        \\    7, 8 };
+        \\const a = []u8{
+        \\    1, 2, //
+        \\    3,
+        \\    4,
+        \\    5,
+        \\    6, // blah
+        \\    7, 8 };
+        \\const a = []u8{
+        \\    1, 2, //
+        \\    3, //
+        \\    4,
+        \\    5,
+        \\    6,
+        \\    7 };
+        \\const a = []u8{
+        \\    1,
+        \\    2,
+        \\    3, 4, //
+        \\    5, 6, //
+        \\    7, 8, //
+        \\};
+    ,
+        \\const a = []u8{
+        \\    1, 2, //
+        \\    3, 4,
+        \\    5, 6,
+        \\    7,
+        \\};
+        \\const a = []u8{
+        \\    1, 2, //
+        \\    3, 4,
+        \\    5, 6,
+        \\    7, 8,
+        \\};
+        \\const a = []u8{
+        \\    1, 2, //
+        \\    3, 4,
+        \\    5,
+        \\    6, // blah
+        \\    7,
+        \\    8,
+        \\};
+        \\const a = []u8{
+        \\    1, 2, //
+        \\    3, //
+        \\    4,
+        \\    5,
+        \\    6,
+        \\    7,
+        \\};
+        \\const a = []u8{
+        \\    1,
+        \\    2,
+        \\    3, 4, //
+        \\    5, 6, //
+        \\    7, 8, //
+        \\};
+        \\
+    );
+}
+
+test "zig fmt: array literal veritical column alignment" {
+    try testTransform(
+        \\const a = []u8{
+        \\    1000, 200,
+        \\    30, 4,
+        \\    50000, 60
+        \\};
+        \\const a = []u8{0,   1, 2, 3, 40,
+        \\    4,5,600,7,
+        \\           80,
+        \\    9, 10, 11, 0, 13, 14, 15};
+        \\
+    ,
+        \\const a = []u8{
+        \\    1000,  200,
+        \\    30,    4,
+        \\    50000, 60,
+        \\};
+        \\const a = []u8{
+        \\    0,  1,  2,   3, 40,
+        \\    4,  5,  600, 7, 80,
+        \\    9,  10, 11,  0, 13,
+        \\    14, 15,
+        \\};
+        \\
+    );
+}
 
 test "zig fmt: multiline string with backslash at end of line" {
     try testCanonical(
@@ -1949,29 +1947,29 @@ test "zig fmt: slice align" {
     );
 }
 
-//test "zig fmt: add trailing comma to array literal" {
-//    try testTransform(
-//        \\comptime {
-//        \\    return []u16{'m', 's', 'y', 's', '-' // hi
-//        \\   };
-//        \\    return []u16{'m', 's', 'y', 's',
-//        \\      '-'};
-//        \\    return []u16{'m', 's', 'y', 's', '-'};
-//        \\}
-//    ,
-//        \\comptime {
-//        \\    return []u16{
-//        \\        'm', 's', 'y', 's', '-', // hi
-//        \\    };
-//        \\    return []u16{
-//        \\        'm', 's', 'y', 's',
-//        \\        '-',
-//        \\    };
-//        \\    return []u16{ 'm', 's', 'y', 's', '-' };
-//        \\}
-//        \\
-//    );
-//}
+test "zig fmt: add trailing comma to array literal" {
+    try testTransform(
+        \\comptime {
+        \\    return []u16{'m', 's', 'y', 's', '-' // hi
+        \\   };
+        \\    return []u16{'m', 's', 'y', 's',
+        \\      '-'};
+        \\    return []u16{'m', 's', 'y', 's', '-'};
+        \\}
+    ,
+        \\comptime {
+        \\    return []u16{
+        \\        'm', 's', 'y', 's', '-', // hi
+        \\    };
+        \\    return []u16{
+        \\        'm', 's', 'y', 's',
+        \\        '-',
+        \\    };
+        \\    return []u16{ 'm', 's', 'y', 's', '-' };
+        \\}
+        \\
+    );
+}
 
 test "zig fmt: first thing in file is line comment" {
     try testCanonical(
@@ -3406,30 +3404,30 @@ test "zig fmt: comptime block in container" {
 //    );
 //}
 
-//test "zig fmt: multiline string in array" {
-//    try testCanonical(
-//        \\const Foo = [][]const u8{
-//        \\    \\aaa
-//        \\    ,
-//        \\    \\bbb
-//        \\};
-//        \\
-//        \\fn bar() void {
-//        \\    const Foo = [][]const u8{
-//        \\        \\aaa
-//        \\        ,
-//        \\        \\bbb
-//        \\    };
-//        \\    const Bar = [][]const u8{ // comment here
-//        \\        \\aaa
-//        \\        \\
-//        \\        , // and another comment can go here
-//        \\        \\bbb
-//        \\    };
-//        \\}
-//        \\
-//    );
-//}
+test "zig fmt: multiline string in array" {
+    try testCanonical(
+        \\const Foo = [][]const u8{
+        \\    \\aaa
+        \\    ,
+        \\    \\bbb
+        \\};
+        \\
+        \\fn bar() void {
+        \\    const Foo = [][]const u8{
+        \\        \\aaa
+        \\        ,
+        \\        \\bbb
+        \\    };
+        \\    const Bar = [][]const u8{ // comment here
+        \\        \\aaa
+        \\        \\
+        \\        , // and another comment can go here
+        \\        \\bbb
+        \\    };
+        \\}
+        \\
+    );
+}
 
 test "zig fmt: if type expr" {
     try testCanonical(
@@ -3449,19 +3447,21 @@ test "zig fmt: file ends with struct field" {
     );
 }
 
-test "zig fmt: comment after empty comment" {
-    try testTransform(
-        \\const x = true; //
-        \\//
-        \\//
-        \\//a
-        \\
-    ,
-        \\const x = true;
-        \\//a
-        \\
-    );
-}
+// TODO intentionally change the behavior of this case?
+// for array literals we necessarily have meaningful empty comments
+//test "zig fmt: comment after empty comment" {
+//    try testTransform(
+//        \\const x = true; //
+//        \\//
+//        \\//
+//        \\//a
+//        \\
+//    ,
+//        \\const x = true;
+//        \\//a
+//        \\
+//    );
+//}
 
 //test "zig fmt: line comment in array" {
 //    try testTransform(
@@ -3493,7 +3493,7 @@ test "zig fmt: comment after empty comment" {
 //        \\
 //    );
 //}
-//
+
 //test "zig fmt: comment after params" {
 //    try testTransform(
 //        \\fn a(
@@ -3518,7 +3518,7 @@ test "zig fmt: comment after empty comment" {
 //        \\
 //    );
 //}
-//
+
 //test "zig fmt: comment in array initializer/access" {
 //    try testCanonical(
 //        \\test "a" {
@@ -3550,7 +3550,7 @@ test "zig fmt: comment after empty comment" {
 //        \\
 //    );
 //}
-//
+
 //test "zig fmt: comments at several places in struct init" {
 //    try testTransform(
 //        \\var bar = Bar{
@@ -3899,44 +3899,44 @@ test "zig fmt: regression test for #5722" {
     );
 }
 
-//test "zig fmt: allow trailing line comments to do manual array formatting" {
-//    try testCanonical(
-//        \\fn foo() void {
-//        \\    self.code.appendSliceAssumeCapacity(&[_]u8{
-//        \\        0x55, // push rbp
-//        \\        0x48, 0x89, 0xe5, // mov rbp, rsp
-//        \\        0x48, 0x81, 0xec, // sub rsp, imm32 (with reloc)
-//        \\    });
-//        \\
-//        \\    di_buf.appendAssumeCapacity(&[_]u8{
-//        \\        1, DW.TAG_compile_unit, DW.CHILDREN_no, // header
-//        \\        DW.AT_stmt_list, DW_FORM_data4, // form value pairs
-//        \\        DW.AT_low_pc,    DW_FORM_addr,
-//        \\        DW.AT_high_pc,   DW_FORM_addr,
-//        \\        DW.AT_name,      DW_FORM_strp,
-//        \\        DW.AT_comp_dir,  DW_FORM_strp,
-//        \\        DW.AT_producer,  DW_FORM_strp,
-//        \\        DW.AT_language,  DW_FORM_data2,
-//        \\        0, 0, // sentinel
-//        \\    });
-//        \\
-//        \\    self.code.appendSliceAssumeCapacity(&[_]u8{
-//        \\        0x55, // push rbp
-//        \\        0x48, 0x89, 0xe5, // mov rbp, rsp
-//        \\        // How do we handle this?
-//        \\        //0x48, 0x81, 0xec, // sub rsp, imm32 (with reloc)
-//        \\        // Here's a blank line, should that be allowed?
-//        \\
-//        \\        0x48, 0x89, 0xe5,
-//        \\        0x33, 0x45,
-//        \\        // Now the comment breaks a single line -- how do we handle this?
-//        \\        0x88,
-//        \\    });
-//        \\}
-//        \\
-//    );
-//}
-//
+test "zig fmt: allow trailing line comments to do manual array formatting" {
+    try testCanonical(
+        \\fn foo() void {
+        \\    self.code.appendSliceAssumeCapacity(&[_]u8{
+        \\        0x55, // push rbp
+        \\        0x48, 0x89, 0xe5, // mov rbp, rsp
+        \\        0x48, 0x81, 0xec, // sub rsp, imm32 (with reloc)
+        \\    });
+        \\
+        \\    di_buf.appendAssumeCapacity(&[_]u8{
+        \\        1, DW.TAG_compile_unit, DW.CHILDREN_no, // header
+        \\        DW.AT_stmt_list, DW_FORM_data4, // form value pairs
+        \\        DW.AT_low_pc,    DW_FORM_addr,
+        \\        DW.AT_high_pc,   DW_FORM_addr,
+        \\        DW.AT_name,      DW_FORM_strp,
+        \\        DW.AT_comp_dir,  DW_FORM_strp,
+        \\        DW.AT_producer,  DW_FORM_strp,
+        \\        DW.AT_language,  DW_FORM_data2,
+        \\        0, 0, // sentinel
+        \\    });
+        \\
+        \\    self.code.appendSliceAssumeCapacity(&[_]u8{
+        \\        0x55, // push rbp
+        \\        0x48, 0x89, 0xe5, // mov rbp, rsp
+        \\        // How do we handle this?
+        \\        //0x48, 0x81, 0xec, // sub rsp, imm32 (with reloc)
+        \\        // Here's a blank line, should that be allowed?
+        \\
+        \\        0x48, 0x89, 0xe5,
+        \\        0x33, 0x45,
+        \\        // Now the comment breaks a single line -- how do we handle this?
+        \\        0x88,
+        \\    });
+        \\}
+        \\
+    );
+}
+
 //test "zig fmt: multiline string literals should play nice with array initializers" {
 //    try testCanonical(
 //        \\fn main() void {
@@ -3999,7 +3999,7 @@ test "zig fmt: regression test for #5722" {
 //        \\
 //    );
 //}
-//
+
 //test "zig fmt: use of comments and Multiline string literals may force the parameters over multiple lines" {
 //    try testCanonical(
 //        \\pub fn makeMemUndefined(qzz: []u8) i1 {
lib/std/zig/render.zig
@@ -1640,7 +1640,11 @@ fn renderArrayInit(
     const last_elem = array_init.ast.elements[array_init.ast.elements.len - 1];
     const last_elem_token = tree.lastToken(last_elem);
     const trailing_comma = token_tags[last_elem_token + 1] == .comma;
-    if (!trailing_comma) {
+    const rbrace = if (trailing_comma) last_elem_token + 2 else last_elem_token + 1;
+    assert(token_tags[rbrace] == .r_brace);
+    const contains_newlines = !tree.tokensOnSameLine(array_init.ast.lbrace, rbrace);
+
+    if (!trailing_comma and !contains_newlines) {
         // Render all on one line, no trailing comma.
         if (array_init.ast.elements.len == 1) {
             // If there is only one element, we don't use spaces
@@ -1658,8 +1662,6 @@ fn renderArrayInit(
     ais.pushIndentNextLine();
     try renderToken(ais, tree, array_init.ast.lbrace, .newline);
 
-    const rbrace = last_elem_token + 2;
-    assert(token_tags[rbrace] == .r_brace);
 
     var expr_index: usize = 0;
     while (rowSize(tree, array_init.ast.elements[expr_index..], rbrace)) |row_size| {
@@ -1673,28 +1675,31 @@ fn renderArrayInit(
         defer gpa.free(expr_newlines);
         mem.set(bool, expr_newlines, false);
 
-        const expr_widths = widths[0 .. widths.len - row_size];
-        const column_widths = widths[widths.len - row_size ..];
+        const expr_widths = widths[0..row_exprs.len];
+        const column_widths = widths[row_exprs.len..];
 
-        // Find next row with trailing comment (if any) to end the current section
+        // Find next row with trailing comment (if any) to end the current section.
         const section_end = sec_end: {
             var this_line_first_expr: usize = 0;
             var this_line_size = rowSize(tree, row_exprs, rbrace);
             for (row_exprs) |expr, i| {
-                // Ignore comment on first line of this section
-                if (i == 0 or tree.tokensOnSameLine(tree.firstToken(row_exprs[0]), tree.lastToken(expr))) continue;
-                // Track start of line containing comment
-                if (!tree.tokensOnSameLine(tree.firstToken(row_exprs[this_line_first_expr]), tree.lastToken(expr))) {
+                // Ignore comment on first line of this section.
+                if (i == 0) continue;
+                const expr_last_token = tree.lastToken(expr);
+                if (tree.tokensOnSameLine(tree.firstToken(row_exprs[0]), expr_last_token))
+                    continue;
+                // Track start of line containing comment.
+                if (!tree.tokensOnSameLine(tree.firstToken(row_exprs[this_line_first_expr]), expr_last_token)) {
                     this_line_first_expr = i;
                     this_line_size = rowSize(tree, row_exprs[this_line_first_expr..], rbrace);
                 }
 
-                const maybe_comma = tree.lastToken(expr) + 1;
+                const maybe_comma = expr_last_token + 1;
                 if (token_tags[maybe_comma] == .comma) {
                     const after_comma_src = tree.source[token_starts[maybe_comma]..token_starts[maybe_comma + 1]];
-                    const same_line_comment = for (after_comma_src) |byte| switch (byte) {
+                    for (after_comma_src) |byte| switch (byte) {
                         '\n' => break,
-                        '/' => break :sec_end i - this_line_size.? + 1, // Found row ending in comment
+                        '/' => break :sec_end i - this_line_size.? + 1,
                         else => continue,
                     };
                 }
@@ -1754,7 +1759,7 @@ fn renderArrayInit(
             }
         }
 
-        // Render exprs in current section
+        // Render exprs in current section.
         column_counter = 0;
         var last_col_index: usize = row_size - 1;
         for (section_exprs) |expr, i| {
@@ -1785,19 +1790,12 @@ fn renderArrayInit(
                 try renderToken(ais, tree, comma, .newline); // ,
                 try renderExtraNewline(ais, tree, next_expr);
             } else {
-                const maybe_comma = tree.lastToken(expr) + 1;
-                if (token_tags[maybe_comma] == .comma) {
-                    try renderExpression(gpa, ais, tree, expr, .none); // ,
-                    try renderToken(ais, tree, maybe_comma, .newline); // ,
-                } else {
-                    try renderExpression(gpa, ais, tree, expr, .comma); // ,
-                }
+                try renderExpression(gpa, ais, tree, expr, .comma); // ,
             }
         }
 
-        if (expr_index == array_init.ast.elements.len) {
+        if (expr_index == array_init.ast.elements.len)
             break;
-        }
     }
 
     ais.popIndent();
@@ -2175,7 +2173,6 @@ fn renderToken(ais: *Ais, tree: ast.Tree, token_index: ast.TokenIndex, space: Sp
 /// that end is the last byte before the next token.
 fn renderComments(ais: *Ais, tree: ast.Tree, start: usize, end: usize) Error!bool {
     var index: usize = start;
-    var rendered_empty_comments = false;
     while (mem.indexOf(u8, tree.source[index..end], "//")) |offset| {
         const comment_start = index + offset;
 
@@ -2196,11 +2193,6 @@ fn renderComments(ais: *Ais, tree: ast.Tree, start: usize, end: usize) Error!boo
                 // Respect the newline directly before the comment.
                 // Note: This allows an empty line between comments
                 try ais.insertNewline();
-            } else if (trimmed_comment.len == 2) {
-                if (!rendered_empty_comments) {
-                    try ais.writer().writeByte('\n');
-                    rendered_empty_comments = true;
-                }
             } else if (index == start) {
                 // Otherwise if the first comment is on the same line as
                 // the token before it, prefix it with a single space.
@@ -2208,10 +2200,7 @@ fn renderComments(ais: *Ais, tree: ast.Tree, start: usize, end: usize) Error!boo
             }
         }
 
-        if (trimmed_comment.len != 2) {
-            try ais.writer().print("{s}\n", .{trimmed_comment});
-            rendered_empty_comments = false;
-        }
+        try ais.writer().print("{s}\n", .{trimmed_comment});
         index = 1 + (newline orelse return true);
 
         if (ais.disabled_offset) |disabled_offset| {