Zig fmt introduces newline on for-around-if where it formerly didn't

[marnix]

Some recent zig fmt change (introduced in the published zig master after Thu, 25 Feb 2021 03:36:23 GMT before Fri, 26 Feb 2021 03:36:51 GMT) changes

test "" {
    for (undefined) |x| if (false) {
        //...
    };
}

to

test "" {
    for (undefined) |x|
        if (false) {
            //...
        };
}

Was this an intentional change? Or is this a regression?

I very much liked the original format, which looks quite readable in several of my use cases:

           for (expr1) |cvToken1| if (cvToken1.cv == .V) {
               for (expr2) |cvToken2| if (cvToken2.cv == .V) {
                   // note: don't try to check for duplicates, that is probably not worth it
                   try self.dvPairs.append(.{ .var1 = cvToken1.token, .var2 = cvToken2.token });
               };
           };

Adding the extra newlines and indentation really makes this code less readable:

           for (expr1) |cvToken1|
               if (cvToken1.cv == .V) {
                   for (expr2) |cvToken2|
                       if (cvToken2.cv == .V) {
                           // note: don't try to check for duplicates, that is probably not worth it
                           try self.dvPairs.append(.{ .var1 = cvToken1.token, .var2 = cvToken2.token });
                       };
               };

[andrewrk]

Was this an intentional change? Or is this a regression?

Regression. I agree with you with your preferred style.

For some context, we recently had to rewrite zig fmt nearly from scratch in #7920. There is a lot of test coverage, but some stuff slipping through the cracks was inevitable.

Thanks for the report; current plan is to resolve this before the next release.

[LewisGaul]

I might take a look and see if I can fix this over the next couple of days - having watched Andrew's recent zig fmt streams at least I have some context!

This would be my first contribution so will ask questions if I have problems, but equally I won't be offended if someone wants to prioritise this above waiting for me to get going.

[LewisGaul]

I'm using the below as a testcase for canonical 'for-if' forms. Do these all seem correct?

The last case is a little strange with the closing brace at the indentation of the 'if' rather than the 'for', but I think it looks even more weird if it's dedented further (and this is matching the behaviour in 0.7.1).

test "for if" {
    for (a) |x| if (x) f(x);

    for (a) |x| if (x)
        f(x);

    for (a) |x| if (x) {
        f(x);
    };

    for (a) |x|
        if (x)
            f(x);

    for (a) |x|
        if (x) {
            f(x);
        };
}

[matu3ba]

@LewisGaul for loops and if branches do not have semicolon, so those are wrong grammar:

    for (a) |x| if (x) {
        f(x);
    };
    for (a) |x|
        if (x) {
            f(x);
        };

[ifreund]

@matu3ba the test cases are correct, single statement if/for/while expressions require a semicolon.

You may want to consult the grammar if you are not convinced https://github.com/ziglang/zig-spec/blob/master/grammar/grammar.y

[matu3ba]

@ifreund This compiles fine though:

const std = @import("std");
pub fn main() void {
    if (true) {
        std.debug.print("test", .{});
    }
}

and this not

const std = @import("std");
pub fn main() void {
    if (true) {
        std.debug.print("test", .{});
    };
}

[ifreund]

@matu3ba try compiling the test code with and without the semicolon.

Or try this:

const std = @import("std");
pub fn main() void {
    if (true) if (true) {
        std.debug.print("test", .{});
    };
}

[matu3ba]

@matu3ba try compiling the test code with and without the semicolon.

Or try this:

const std = @import("std");
pub fn main() void {
    if (true) if (true) {
        std.debug.print("test", .{});
    };
}

Ah, so only if it is nested. Thats smart. Sorry my bad.