Commit 48efa3bcb6

Isaac Freund <ifreund@ifreund.xyz>
2021-03-15 14:41:15
zig fmt: simplify and improve consistency of if/for/while handling
The main realization here was that getting rid of the early returns in renderWhile() and rewriting the logic into a mostly unified execution path took things from ~200 lines to ~100 lines and improved consistency by deduplicating code. Also add several test cases and fix a few issues along the way: Fixes https://github.com/ziglang/zig/issues/6114 Fixes https://github.com/ziglang/zig/issues/8022
master
1 parent b4db03d
Changed files (2)
lib
std
zig
parser_test.zig
render.zig
lib/std/zig/parser_test.zig
@@ -3186,7 +3186,7 @@ test "zig fmt: for" {
 
 test "zig fmt: for if" {
     try testCanonical(
-        \\test "for if" {
+        \\test {
         \\    for (a) |x| if (x) f(x);
         \\
         \\    for (a) |x| if (x)
@@ -3209,6 +3209,131 @@ test "zig fmt: for if" {
     );
 }
 
+test "zig fmt: if for" {
+    try testCanonical(
+        \\test {
+        \\    if (a) for (x) |x| f(x);
+        \\
+        \\    if (a) for (x) |x|
+        \\        f(x);
+        \\
+        \\    if (a) for (x) |x| {
+        \\        f(x);
+        \\    };
+        \\
+        \\    if (a)
+        \\        for (x) |x|
+        \\            f(x);
+        \\
+        \\    if (a)
+        \\        for (x) |x| {
+        \\            f(x);
+        \\        };
+        \\}
+        \\
+    );
+}
+
+test "zig fmt: while if" {
+    try testCanonical(
+        \\test {
+        \\    while (a) if (x) f(x);
+        \\
+        \\    while (a) if (x)
+        \\        f(x);
+        \\
+        \\    while (a) if (x) {
+        \\        f(x);
+        \\    };
+        \\
+        \\    while (a)
+        \\        if (x)
+        \\            f(x);
+        \\
+        \\    while (a)
+        \\        if (x) {
+        \\            f(x);
+        \\        };
+        \\}
+        \\
+    );
+}
+
+test "zig fmt: if while" {
+    try testCanonical(
+        \\test {
+        \\    if (a) while (x) : (cont) f(x);
+        \\
+        \\    if (a) while (x) : (cont)
+        \\        f(x);
+        \\
+        \\    if (a) while (x) : (cont) {
+        \\        f(x);
+        \\    };
+        \\
+        \\    if (a)
+        \\        while (x) : (cont)
+        \\            f(x);
+        \\
+        \\    if (a)
+        \\        while (x) : (cont) {
+        \\            f(x);
+        \\        };
+        \\}
+        \\
+    );
+}
+
+test "zig fmt: while for" {
+    try testCanonical(
+        \\test {
+        \\    while (a) for (x) |x| f(x);
+        \\
+        \\    while (a) for (x) |x|
+        \\        f(x);
+        \\
+        \\    while (a) for (x) |x| {
+        \\        f(x);
+        \\    };
+        \\
+        \\    while (a)
+        \\        for (x) |x|
+        \\            f(x);
+        \\
+        \\    while (a)
+        \\        for (x) |x| {
+        \\            f(x);
+        \\        };
+        \\}
+        \\
+    );
+}
+
+test "zig fmt: for while" {
+    try testCanonical(
+        \\test {
+        \\    for (a) |a| while (x) |x| f(x);
+        \\
+        \\    for (a) |a| while (x) |x|
+        \\        f(x);
+        \\
+        \\    for (a) |a| while (x) |x| {
+        \\        f(x);
+        \\    };
+        \\
+        \\    for (a) |a|
+        \\        while (x) |x|
+        \\            f(x);
+        \\
+        \\    for (a) |a|
+        \\        while (x) |x| {
+        \\            f(x);
+        \\        };
+        \\}
+        \\
+    );
+}
+
 test "zig fmt: if" {
     try testCanonical(
         \\test "if" {
@@ -3258,6 +3383,82 @@ test "zig fmt: if" {
     );
 }
 
+test "zig fmt: fix single statement if/for/while line breaks" {
+    try testTransform(
+        \\test {
+        \\    if (cond) a
+        \\    else b;
+        \\
+        \\    if (cond)
+        \\        a
+        \\    else b;
+        \\
+        \\    for (xs) |x| foo()
+        \\    else bar();
+        \\
+        \\    for (xs) |x|
+        \\        foo()
+        \\    else bar();
+        \\
+        \\    while (a) : (b) foo()
+        \\    else bar();
+        \\
+        \\    while (a) : (b)
+        \\        foo()
+        \\    else bar();
+        \\}
+        \\
+    ,
+        \\test {
+        \\    if (cond) a else b;
+        \\
+        \\    if (cond)
+        \\        a
+        \\    else
+        \\        b;
+        \\
+        \\    for (xs) |x| foo() else bar();
+        \\
+        \\    for (xs) |x|
+        \\        foo()
+        \\    else
+        \\        bar();
+        \\
+        \\    while (a) : (b) foo() else bar();
+        \\
+        \\    while (a) : (b)
+        \\        foo()
+        \\    else
+        \\        bar();
+        \\}
+        \\
+    );
+}
+
+test "zig fmt: anon struct/array literal in if" {
+    try testCanonical(
+        \\test {
+        \\    const a = if (cond) .{
+        \\        1, 2,
+        \\        3, 4,
+        \\    } else .{
+        \\        1,
+        \\        2,
+        \\        3,
+        \\    };
+        \\
+        \\    const rl_and_tag: struct { rl: ResultLoc, tag: zir.Inst.Tag } = if (any_payload_is_ref) .{
+        \\        .rl = .ref,
+        \\        .tag = .switchbr_ref,
+        \\    } else .{
+        \\        .rl = .none,
+        \\        .tag = .switchbr,
+        \\    };
+        \\}
+        \\
+    );
+}
+
 test "zig fmt: defer" {
     try testCanonical(
         \\test "defer" {
@@ -3845,6 +4046,7 @@ test "zig fmt: comments in ternary ifs" {
         \\    // Comment
         \\    1
         \\else
+        \\    // Comment
         \\    0;
         \\
         \\pub extern "c" fn printf(format: [*:0]const u8, ...) c_int;
@@ -3852,6 +4054,20 @@ test "zig fmt: comments in ternary ifs" {
     );
 }
 
+test "zig fmt: while statement in blockless if" {
+    try testCanonical(
+        \\pub fn main() void {
+        \\    const zoom_node = if (focused_node == layout_first)
+        \\        while (it.next()) |node| {
+        \\            if (!node.view.pending.float and !node.view.pending.fullscreen) break node;
+        \\        } else null
+        \\    else
+        \\        focused_node;
+        \\}
+        \\
+    );
+}
+
 test "zig fmt: test comments in field access chain" {
     try testCanonical(
         \\pub const str = struct {
lib/std/zig/render.zig
@@ -1018,170 +1018,95 @@ fn renderWhile(gpa: *Allocator, ais: *Ais, tree: ast.Tree, while_node: ast.full.
         try renderToken(ais, tree, inline_token, .space); // inline
     }
 
-    try renderToken(ais, tree, while_node.ast.while_token, .space); // if
+    try renderToken(ais, tree, while_node.ast.while_token, .space); // if/for/while
     try renderToken(ais, tree, while_node.ast.while_token + 1, .none); // lparen
     try renderExpression(gpa, ais, tree, while_node.ast.cond_expr, .none); // condition
 
-    const then_tag = node_tags[while_node.ast.then_expr];
-    if (nodeIsBlock(then_tag) and !nodeIsIf(then_tag)) {
-        if (while_node.payload_token) |payload_token| {
-            const brace_space = if (while_node.ast.cont_expr == 0 and ais.isLineOverIndented())
-                Space.newline
-            else
-                Space.space;
-            try renderWhilePayload(gpa, ais, tree, payload_token, brace_space);
-        } else {
-            const rparen = tree.lastToken(while_node.ast.cond_expr) + 1;
-            const brace_space = if (while_node.ast.cont_expr == 0 and ais.isLineOverIndented())
-                Space.newline
-            else
-                Space.space;
-            try renderToken(ais, tree, rparen, brace_space); // rparen
-        }
-        if (while_node.ast.cont_expr != 0) {
-            const rparen = tree.lastToken(while_node.ast.cont_expr) + 1;
-            const lparen = tree.firstToken(while_node.ast.cont_expr) - 1;
-            try renderToken(ais, tree, lparen - 1, .space); // :
-            try renderToken(ais, tree, lparen, .none); // lparen
-            try renderExpression(gpa, ais, tree, while_node.ast.cont_expr, .none);
-            const brace_space: Space = if (ais.isLineOverIndented()) .newline else .space;
-            try renderToken(ais, tree, rparen, brace_space); // rparen
-        }
-        if (while_node.ast.else_expr != 0) {
-            try renderExpression(gpa, ais, tree, while_node.ast.then_expr, Space.space);
-            try renderToken(ais, tree, while_node.else_token, .space); // else
-            if (while_node.error_token) |error_token| {
-                try renderToken(ais, tree, error_token - 1, .none); // |
-                try renderToken(ais, tree, error_token, .none); // identifier
-                try renderToken(ais, tree, error_token + 1, .space); // |
-            }
-            return renderExpression(gpa, ais, tree, while_node.ast.else_expr, space);
-        } else {
-            return renderExpression(gpa, ais, tree, while_node.ast.then_expr, space);
-        }
-    }
-
-    const rparen = tree.lastToken(while_node.ast.cond_expr) + 1;
-    const first_then_token = tree.firstToken(while_node.ast.then_expr);
-    const last_then_token = tree.lastToken(while_node.ast.then_expr);
-    const src_has_newline = !tree.tokensOnSameLine(rparen, last_then_token);
+    var last_prefix_token = tree.lastToken(while_node.ast.cond_expr) + 1; // rparen
 
-    if (src_has_newline) {
-        const newline_before_then_token = !tree.tokensOnSameLine(rparen, first_then_token);
-        const space_before_then_token: Space = if (newline_before_then_token) .newline else .space;
-        const indent_expression = !nodeIsIf(then_tag) or newline_before_then_token;
-
-        if (while_node.payload_token) |payload_token| {
-            const after_space: Space = if (while_node.ast.cont_expr != 0) .space else space_before_then_token;
-            try renderWhilePayload(gpa, ais, tree, payload_token, after_space);
-        } else {
-            if (indent_expression) ais.pushIndent();
-            const after_space: Space = if (while_node.ast.cont_expr != 0) .space else space_before_then_token;
-            try renderToken(ais, tree, rparen, after_space); // rparen
-            if (indent_expression) ais.popIndent();
-        }
-        if (while_node.ast.cont_expr != 0) {
-            const cont_rparen = tree.lastToken(while_node.ast.cont_expr) + 1;
-            const cont_lparen = tree.firstToken(while_node.ast.cont_expr) - 1;
-            try renderToken(ais, tree, cont_lparen - 1, .space); // :
-            try renderToken(ais, tree, cont_lparen, .none); // lparen
-            try renderExpression(gpa, ais, tree, while_node.ast.cont_expr, .none);
-            try renderToken(ais, tree, cont_rparen, space_before_then_token); // rparen
-        }
-        if (while_node.ast.else_expr != 0) {
-            if (indent_expression) ais.pushIndent();
-            try renderExpression(gpa, ais, tree, while_node.ast.then_expr, .newline);
-            if (indent_expression) ais.popIndent();
-            const else_is_block = nodeIsBlock(node_tags[while_node.ast.else_expr]);
-            if (else_is_block) {
-                try renderToken(ais, tree, while_node.else_token, .space); // else
-                if (while_node.error_token) |error_token| {
-                    try renderToken(ais, tree, error_token - 1, .none); // |
-                    try renderToken(ais, tree, error_token, .none); // identifier
-                    try renderToken(ais, tree, error_token + 1, .space); // |
-                }
-                return renderExpression(gpa, ais, tree, while_node.ast.else_expr, space);
+    if (while_node.payload_token) |payload_token| {
+        try renderToken(ais, tree, last_prefix_token, .space);
+        try renderToken(ais, tree, payload_token - 1, .none); // |
+        const ident = blk: {
+            if (token_tags[payload_token] == .asterisk) {
+                try renderToken(ais, tree, payload_token, .none); // *
+                break :blk payload_token + 1;
             } else {
-                if (while_node.error_token) |error_token| {
-                    try renderToken(ais, tree, while_node.else_token, .space); // else
-                    try renderToken(ais, tree, error_token - 1, .none); // |
-                    try renderToken(ais, tree, error_token, .none); // identifier
-                    try renderToken(ais, tree, error_token + 1, .space); // |
-                } else {
-                    try renderToken(ais, tree, while_node.else_token, .newline); // else
-                }
-                if (indent_expression) {
-                    return renderExpressionIndented(gpa, ais, tree, while_node.ast.else_expr, space);
-                } else {
-                    return renderExpression(gpa, ais, tree, while_node.ast.else_expr, space);
-                }
+                break :blk payload_token;
             }
-        } else {
-            if (indent_expression) {
-                return renderExpressionIndented(gpa, ais, tree, while_node.ast.then_expr, space);
+        };
+        try renderToken(ais, tree, ident, .none); // identifier
+        const pipe = blk: {
+            if (token_tags[ident + 1] == .comma) {
+                try renderToken(ais, tree, ident + 1, .space); // ,
+                try renderToken(ais, tree, ident + 2, .none); // index
+                break :blk ident + 3;
             } else {
-                return renderExpression(gpa, ais, tree, while_node.ast.then_expr, space);
+                break :blk ident + 1;
             }
-        }
-    }
-
-    // Render everything on a single line.
-
-    if (while_node.payload_token) |payload_token| {
-        assert(payload_token - 2 == rparen);
-        try renderWhilePayload(gpa, ais, tree, payload_token, .space);
-    } else {
-        try renderToken(ais, tree, rparen, .space); // )
+        };
+        last_prefix_token = pipe;
     }
 
     if (while_node.ast.cont_expr != 0) {
-        const cont_rparen = tree.lastToken(while_node.ast.cont_expr) + 1;
-        const cont_lparen = tree.firstToken(while_node.ast.cont_expr) - 1;
-        try renderToken(ais, tree, cont_lparen - 1, .space); // :
-        try renderToken(ais, tree, cont_lparen, .none); // lparen
+        try renderToken(ais, tree, last_prefix_token, .space);
+        const lparen = tree.firstToken(while_node.ast.cont_expr) - 1;
+        try renderToken(ais, tree, lparen - 1, .space); // :
+        try renderToken(ais, tree, lparen, .none); // lparen
         try renderExpression(gpa, ais, tree, while_node.ast.cont_expr, .none);
-        try renderToken(ais, tree, cont_rparen, .space); // rparen
+        last_prefix_token = tree.lastToken(while_node.ast.cont_expr) + 1; // rparen
+    }
+
+    const then_expr_is_block = nodeIsBlock(node_tags[while_node.ast.then_expr]);
+    const indent_then_expr = !then_expr_is_block and
+        !tree.tokensOnSameLine(last_prefix_token, tree.firstToken(while_node.ast.then_expr));
+    if (indent_then_expr or (then_expr_is_block and ais.isLineOverIndented())) {
+        ais.pushIndentNextLine();
+        try renderToken(ais, tree, last_prefix_token, .newline);
+        ais.popIndent();
+    } else {
+        try renderToken(ais, tree, last_prefix_token, .space);
     }
 
     if (while_node.ast.else_expr != 0) {
-        try renderExpression(gpa, ais, tree, while_node.ast.then_expr, .space);
-        try renderToken(ais, tree, while_node.else_token, .space); // else
+        const first_else_expr_tok = tree.firstToken(while_node.ast.else_expr);
+
+        if (indent_then_expr) {
+            ais.pushIndent();
+            try renderExpression(gpa, ais, tree, while_node.ast.then_expr, .newline);
+            ais.popIndent();
+        } else {
+            try renderExpression(gpa, ais, tree, while_node.ast.then_expr, .space);
+        }
+
+        var last_else_token = while_node.else_token;
 
         if (while_node.error_token) |error_token| {
+            try renderToken(ais, tree, while_node.else_token, .space); // else
             try renderToken(ais, tree, error_token - 1, .none); // |
             try renderToken(ais, tree, error_token, .none); // identifier
-            try renderToken(ais, tree, error_token + 1, .space); // |
+            last_else_token = error_token + 1; // |
         }
 
-        return renderExpression(gpa, ais, tree, while_node.ast.else_expr, space);
-    } else {
-        return renderExpression(gpa, ais, tree, while_node.ast.then_expr, space);
-    }
-}
-
-fn renderWhilePayload(gpa: *Allocator, ais: *Ais, tree: ast.Tree, payload_token: ast.TokenIndex, space: Space) Error!void {
-    const token_tags = tree.tokens.items(.tag);
-    try renderToken(ais, tree, payload_token - 2, .space); // rparen
-    try renderToken(ais, tree, payload_token - 1, .none); // |
-    const ident = blk: {
-        if (token_tags[payload_token] == .asterisk) {
-            try renderToken(ais, tree, payload_token, .none); // *
-            break :blk payload_token + 1;
+        const indent_else_expr = indent_then_expr and
+            !nodeIsBlock(node_tags[while_node.ast.else_expr]) and
+            !nodeIsIfForWhileSwitch(node_tags[while_node.ast.else_expr]);
+        if (indent_else_expr) {
+            ais.pushIndentNextLine();
+            try renderToken(ais, tree, last_else_token, .newline);
+            ais.popIndent();
+            try renderExpressionIndented(gpa, ais, tree, while_node.ast.else_expr, space);
         } else {
-            break :blk payload_token;
+            try renderToken(ais, tree, last_else_token, .space);
+            try renderExpression(gpa, ais, tree, while_node.ast.else_expr, space);
         }
-    };
-    try renderToken(ais, tree, ident, .none); // identifier
-    const pipe = blk: {
-        if (token_tags[ident + 1] == .comma) {
-            try renderToken(ais, tree, ident + 1, .space); // ,
-            try renderToken(ais, tree, ident + 2, .none); // index
-            break :blk ident + 3;
+    } else {
+        if (indent_then_expr) {
+            try renderExpressionIndented(gpa, ais, tree, while_node.ast.then_expr, space);
         } else {
-            break :blk ident + 1;
+            try renderExpression(gpa, ais, tree, while_node.ast.then_expr, space);
         }
-    };
-    try renderToken(ais, tree, pipe, space); // |
+    }
 }
 
 fn renderContainerField(
@@ -2469,6 +2394,21 @@ fn nodeIsBlock(tag: ast.Node.Tag) bool {
         .block_semicolon,
         .block_two,
         .block_two_semicolon,
+        .struct_init_dot,
+        .struct_init_dot_comma,
+        .struct_init_dot_two,
+        .struct_init_dot_two_comma,
+        .array_init_dot,
+        .array_init_dot_comma,
+        .array_init_dot_two,
+        .array_init_dot_two_comma,
+        => true,
+        else => false,
+    };
+}
+
+fn nodeIsIfForWhileSwitch(tag: ast.Node.Tag) bool {
+    return switch (tag) {
         .@"if",
         .if_simple,
         .@"for",
@@ -2483,13 +2423,6 @@ fn nodeIsBlock(tag: ast.Node.Tag) bool {
     };
 }
 
-fn nodeIsIf(tag: ast.Node.Tag) bool {
-    return switch (tag) {
-        .@"if", .if_simple => true,
-        else => false,
-    };
-}
-
 fn nodeCausesSliceOpSpace(tag: ast.Node.Tag) bool {
     return switch (tag) {
         .@"catch",