From 97645f3ea7c909915918e8ea468daea793e68716 Mon Sep 17 00:00:00 2001 From: Jonathan Gautheron Date: Fri, 14 Mar 2025 22:44:13 +0100 Subject: [PATCH 1/3] std.zig.render: Fix multiple fmt invocations needed for array initializers with switch expressions --- lib/std/zig/render.zig | 24 ++++++++++++++++++++++- test/tests.zig | 44 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/lib/std/zig/render.zig b/lib/std/zig/render.zig index 586bb9696b08..a3c8546fc9eb 100644 --- a/lib/std/zig/render.zig +++ b/lib/std/zig/render.zig @@ -2076,8 +2076,9 @@ fn renderArrayInit( const contains_comment = hasComment(tree, array_init.ast.lbrace, rbrace); const contains_multiline_string = hasMultilineString(tree, array_init.ast.lbrace, rbrace); + const contains_switch = containsSwitch(tree, array_init.ast.elements); - if (!trailing_comma and !contains_comment and !contains_multiline_string) { + if (!trailing_comma and !contains_comment and !contains_multiline_string and !contains_switch) { // 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 @@ -2260,6 +2261,27 @@ fn renderArrayInit( return renderToken(r, rbrace, space); // rbrace } +/// Returns true if any array element is or contains a switch expression +fn containsSwitch(tree: Ast, elements: []const Ast.Node.Index) bool { + const tags = tree.nodes.items(.tag); + + for (elements) |elem| { + const tag = tags[@intFromEnum(elem)]; + + if (tag == .switch_case or + tag == .switch_case_inline or + tag == .switch_case_one or + tag == .switch_case_inline_one or + tag == .@"switch" or + tag == .switch_comma) + { + return true; + } + } + + return false; +} + fn renderContainerDecl( r: *Render, container_decl_node: Ast.Node.Index, diff --git a/test/tests.zig b/test/tests.zig index 28c6fbc67e40..247ffc0bda5e 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -1299,6 +1299,50 @@ pub fn addCliTests(b: *std.Build) *Step { }); check6.step.dependOn(&run6.step); + // Test `zig fmt` handling switch expressions in array initializers + const switch_in_array_code = + \\const bar = .{ .{ switch ({}) { + \\ else => {}, + \\ } }, .{}, .{}, .{}, + \\}; + \\ + ; + + const fmt7_path = b.pathJoin(&.{ tmp_path, "fmt7.zig" }); + const write7 = b.addUpdateSourceFiles(); + write7.addBytesToSource(switch_in_array_code, fmt7_path); + write7.step.dependOn(&check6.step); + + // Test `zig fmt` handling switch expressions in array initializers + const run7 = b.addSystemCommand(&.{ b.graph.zig_exe, "fmt", "fmt7.zig" }); + run7.setName("run zig fmt on array with switch"); + run7.setCwd(.{ .cwd_relative = tmp_path }); + run7.has_side_effects = true; + run7.expectStdOutEqual("fmt7.zig\n"); + run7.step.dependOn(&write7.step); + + // Check that the file is formatted correctly after a single fmt invocation + const check7 = b.addCheckFile(.{ .cwd_relative = fmt7_path }, .{ + .expected_matches = &.{ + "const bar = .{", + " .{switch ({}) {", + " else => {},", + " }},", + " .{},", + " .{},", + " .{},", + "};", + }, + }); + check7.step.dependOn(&run7.step); + + // Run fmt again to verify that no further changes are made (idempotence) + const run8 = b.addSystemCommand(&.{ b.graph.zig_exe, "fmt", "fmt7.zig" }); + run8.setName("run zig fmt again on array with switch"); + run8.setCwd(.{ .cwd_relative = tmp_path }); + run8.has_side_effects = true; + run8.step.dependOn(&check7.step); + const cleanup = b.addRemoveDirTree(.{ .cwd_relative = tmp_path }); cleanup.step.dependOn(&check6.step); From 8dd7995e2c9ad7c6ff43490682dca7dfa9b17311 Mon Sep 17 00:00:00 2001 From: Jonathan Gautheron Date: Sat, 15 Mar 2025 01:55:21 +0100 Subject: [PATCH 2/3] std.zig.parser_test: add test cases --- lib/std/zig/parser_test.zig | 33 ++++++++++++++++++++++++++++ test/tests.zig | 44 ------------------------------------- 2 files changed, 33 insertions(+), 44 deletions(-) diff --git a/lib/std/zig/parser_test.zig b/lib/std/zig/parser_test.zig index e5961621bdd5..59f203351e88 100644 --- a/lib/std/zig/parser_test.zig +++ b/lib/std/zig/parser_test.zig @@ -6107,6 +6107,39 @@ test "zig fmt: indentation of comments within catch, else, orelse" { ); } +test "zig fmt: array initializer with switch expression - canonical" { + try testCanonical( + \\const bar = .{ + \\ .{switch ({}) { + \\ else => {}, + \\ }}, + \\ .{}, + \\ .{}, + \\ .{}, + \\}; + \\ + ); +} + +test "zig fmt: array initializer with switch expression - transformation" { + try testTransform( + \\const bar = .{ .{ switch ({}) { + \\ else => {}, + \\ } }, .{}, .{}, .{}, + \\}; + \\ + , + \\const bar = .{ + \\ .{switch ({}) { + \\ else => {}, + \\ }}, + \\ .{}, .{}, + \\ .{}, + \\}; + \\ + ); +} + test "recovery: top level" { try testError( \\test "" {inline} diff --git a/test/tests.zig b/test/tests.zig index 247ffc0bda5e..28c6fbc67e40 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -1299,50 +1299,6 @@ pub fn addCliTests(b: *std.Build) *Step { }); check6.step.dependOn(&run6.step); - // Test `zig fmt` handling switch expressions in array initializers - const switch_in_array_code = - \\const bar = .{ .{ switch ({}) { - \\ else => {}, - \\ } }, .{}, .{}, .{}, - \\}; - \\ - ; - - const fmt7_path = b.pathJoin(&.{ tmp_path, "fmt7.zig" }); - const write7 = b.addUpdateSourceFiles(); - write7.addBytesToSource(switch_in_array_code, fmt7_path); - write7.step.dependOn(&check6.step); - - // Test `zig fmt` handling switch expressions in array initializers - const run7 = b.addSystemCommand(&.{ b.graph.zig_exe, "fmt", "fmt7.zig" }); - run7.setName("run zig fmt on array with switch"); - run7.setCwd(.{ .cwd_relative = tmp_path }); - run7.has_side_effects = true; - run7.expectStdOutEqual("fmt7.zig\n"); - run7.step.dependOn(&write7.step); - - // Check that the file is formatted correctly after a single fmt invocation - const check7 = b.addCheckFile(.{ .cwd_relative = fmt7_path }, .{ - .expected_matches = &.{ - "const bar = .{", - " .{switch ({}) {", - " else => {},", - " }},", - " .{},", - " .{},", - " .{},", - "};", - }, - }); - check7.step.dependOn(&run7.step); - - // Run fmt again to verify that no further changes are made (idempotence) - const run8 = b.addSystemCommand(&.{ b.graph.zig_exe, "fmt", "fmt7.zig" }); - run8.setName("run zig fmt again on array with switch"); - run8.setCwd(.{ .cwd_relative = tmp_path }); - run8.has_side_effects = true; - run8.step.dependOn(&check7.step); - const cleanup = b.addRemoveDirTree(.{ .cwd_relative = tmp_path }); cleanup.step.dependOn(&check6.step); From 44a0c4908f22e3615c59cc7f01a45f1c8278b88d Mon Sep 17 00:00:00 2001 From: Jonathan Gautheron Date: Sun, 16 Mar 2025 23:28:54 +0100 Subject: [PATCH 3/3] std.zig.render: better switch statement handling --- lib/std/zig/parser_test.zig | 70 ++++++++++++++++++++++++++++++++----- lib/std/zig/render.zig | 56 +++++++++++++++++------------ 2 files changed, 94 insertions(+), 32 deletions(-) diff --git a/lib/std/zig/parser_test.zig b/lib/std/zig/parser_test.zig index 59f203351e88..91f3c99e6eb3 100644 --- a/lib/std/zig/parser_test.zig +++ b/lib/std/zig/parser_test.zig @@ -6107,8 +6107,11 @@ test "zig fmt: indentation of comments within catch, else, orelse" { ); } -test "zig fmt: array initializer with switch expression - canonical" { - try testCanonical( +test "zig fmt: nested switch in struct initialization" { + try testTransform( + \\const bar = .{ .{ switch ({}) { else => {}, } }, .{}, .{}, .{}, }; + \\ + , \\const bar = .{ \\ .{switch ({}) { \\ else => {}, @@ -6121,19 +6124,68 @@ test "zig fmt: array initializer with switch expression - canonical" { ); } -test "zig fmt: array initializer with switch expression - transformation" { - try testTransform( - \\const bar = .{ .{ switch ({}) { +test "zig fmt: nested switch in struct initialization is deterministic" { + // This test verifies that the formatter produces the same output + // when run multiple times on the same input + const formatted = + \\const bar = .{ + \\ .{switch ({}) { \\ else => {}, - \\ } }, .{}, .{}, .{}, + \\ }}, + \\ .{}, + \\ .{}, + \\ .{}, \\}; \\ + ; + try testCanonical(formatted); +} + +test "zig fmt: nested switch in struct initialization with multiple cases" { + try testTransform( + \\const foo = .{ .{ switch (x) { 0 => {}, 1 => {}, else => {}, } }, .{}, .{}, }; + \\ , - \\const bar = .{ - \\ .{switch ({}) { + \\const foo = .{ + \\ .{switch (x) { + \\ 0 => {}, + \\ 1 => {}, \\ else => {}, \\ }}, - \\ .{}, .{}, + \\ .{}, + \\ .{}, + \\}; + \\ + ); +} + +test "zig fmt: nested switch in middle of struct initialization" { + try testTransform( + \\const baz = .{ .{}, .{ switch (y) { true => 1, false => 2, } }, .{}, }; + \\ + , + \\const baz = .{ + \\ .{}, + \\ .{switch (y) { + \\ true => 1, + \\ false => 2, + \\ }}, + \\ .{}, + \\}; + \\ + ); +} + +test "zig fmt: deeply nested switch in struct initialization" { + try testTransform( + \\const complex = .{ .{ .{ switch (z) { .a => 10, .b => 20, } } }, .{}, }; + \\ + , + \\const complex = .{ + \\ .{.{switch (z) { + \\ .a => 10, + \\ .b => 20, + \\ }}}, \\ .{}, \\}; \\ diff --git a/lib/std/zig/render.zig b/lib/std/zig/render.zig index a3c8546fc9eb..794a1442a929 100644 --- a/lib/std/zig/render.zig +++ b/lib/std/zig/render.zig @@ -2076,9 +2076,8 @@ fn renderArrayInit( const contains_comment = hasComment(tree, array_init.ast.lbrace, rbrace); const contains_multiline_string = hasMultilineString(tree, array_init.ast.lbrace, rbrace); - const contains_switch = containsSwitch(tree, array_init.ast.elements); - if (!trailing_comma and !contains_comment and !contains_multiline_string and !contains_switch) { + if (!trailing_comma and !contains_comment and !contains_multiline_string) { // 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 @@ -2261,27 +2260,6 @@ fn renderArrayInit( return renderToken(r, rbrace, space); // rbrace } -/// Returns true if any array element is or contains a switch expression -fn containsSwitch(tree: Ast, elements: []const Ast.Node.Index) bool { - const tags = tree.nodes.items(.tag); - - for (elements) |elem| { - const tag = tags[@intFromEnum(elem)]; - - if (tag == .switch_case or - tag == .switch_case_inline or - tag == .switch_case_one or - tag == .switch_case_inline_one or - tag == .@"switch" or - tag == .switch_comma) - { - return true; - } - } - - return false; -} - fn renderContainerDecl( r: *Render, container_decl_node: Ast.Node.Index, @@ -3245,6 +3223,12 @@ fn rowSize(tree: Ast, exprs: []const Ast.Node.Index, rtoken: Ast.TokenIndex) usi const maybe_comma = rtoken - 1; if (tree.tokenTag(maybe_comma) == .comma) return 1; + + // Check if the first expression contains a switch statement + // If it does, we want to put it on its own line + if (containsComplexExpression(tree, exprs[0])) + return 1; + return exprs.len; // no newlines } @@ -3253,6 +3237,12 @@ fn rowSize(tree: Ast, exprs: []const Ast.Node.Index, rtoken: Ast.TokenIndex) usi if (i + 1 < exprs.len) { const expr_last_token = tree.lastToken(expr) + 1; if (!tree.tokensOnSameLine(expr_last_token, tree.firstToken(exprs[i + 1]))) return count; + + // If this expression contains a complex expression like a switch, + // we want to start a new row after it + if (containsComplexExpression(tree, expr)) + return count; + count += 1; } else { return count; @@ -3261,6 +3251,26 @@ fn rowSize(tree: Ast, exprs: []const Ast.Node.Index, rtoken: Ast.TokenIndex) usi unreachable; } +// Helper function to check if a node contains a complex expression like a switch +fn containsComplexExpression(tree: Ast, node: Ast.Node.Index) bool { + const tag = tree.nodeTag(node); + return switch (tag) { + .@"switch" => true, + + // For struct initializations, check if any of their fields contain a switch + .struct_init_one, .struct_init_one_comma, .struct_init_dot_two, .struct_init_dot_two_comma, .struct_init_dot, .struct_init_dot_comma, .struct_init, .struct_init_comma => { + var buf: [2]Ast.Node.Index = undefined; + const struct_init = tree.fullStructInit(&buf, node).?; + for (struct_init.ast.fields) |field| { + if (containsComplexExpression(tree, field)) return true; + } + return false; + }, + + else => false, + }; +} + /// Automatically inserts indentation of written data by keeping /// track of the current indentation level ///