zig fmt: ternary if inside switch could be improved

[bens]

pub fn main() void {
    return switch (foo) {
        0 => if (xyz)
            abc
        else
            def,
        else => ghi,
    };
}

To me, the if (foo) ending up being to the right of the "then" expression seems awkward, and even more so to have the else keyword further left again so that it lines up with the switch cases, which I find confusing.

Something that I'd find more natural would be along the lines of

pub fn main() void {
    return switch (foo) {
        0 =>
            if (xyz)
                abc
            else
                def,
        else => ghi,
    };
}

Where the if is moved to a new line since it's a multi-line construction and indented along with its else.

[dbandstra]

I'm undecided, but want to point out it's not just inside switches, zig fmt also does this:

    const variable = if (xyz)
        abc
    else
        def;

[ifreund]

My ideal here would be

const variable = if (xyz) abc
    else def;

If blocks are used we could keep the status quo:

const variable = if (xyz) blk: {
    break :blk abc;
} else blk: {
    break :blk def;
};

For the example from the OP this would yield:

pub fn main() void {
    return switch (foo) {
        0 => if (xyz) abc
            else def,
        else => ghi,
    };
}

[bens]

@ifreund That seems fine, my main issue is how confusing having the else from the if line up with the switch cases was when expressions are larger and more nested. In your first example the test and the "then" expression are both short, how would the "then" expression be indented if they become too large to fit on a line? Possibly I'm doing it wrong and these expressions should be kept shorter with functions that should inline but zig fmt will need to handle it one way or another.

pub fn main() void {
    return switch (foo) {
        .MyThing => if (checkMyXYZ(xyz))
            parseAsABC(xyz)
        else if (anotherCheck(xyz))
            parseAsDEF(xyz)
        else if (isItOK(uvw)) someFunction(123) + anotherFunction(456) else if (checkerFunction(pqr)) switch (stu) {
            .SomeOtherThing => this,
            else => that,
        } else
            ghi,
        else => jkl,
    };
}

This is a zig fmt example closer to my code where I find the indentation quite unhelpful, I have to fall back to counting braces and keywords. The way it enforces the else if (...) else if (...) switch (...) { to all go on one line is also unhelpful I find. It's not obvious to me that the else ghi is the last else of the if (checkMyXYZ) and not attached to the switch (stu) without looking carefully, and it also makes for quite a long line.

Apologies for opening a bikeshedding issue, but if non-zig fmt style code is going to be a compile error it's motivating to raise these sorts of things.

[ifreund]

With my proposed change your snippet would look like this which I think is fairly readable:

pub fn main() void {
    return switch (foo) {
        .MyThing => if (checkMyXYZ(xyz)) parseAsABC(xyz)
            else if (anotherCheck(xyz)) parseAsDEF(xyz)
            else if (isItOK(uvw)) someFunction(123) + anotherFunction(456)
            else if (checkerFunction(pqr)) switch (stu) {
                .SomeOtherThing => this,
                else => that,
            } else ghi,
        else => jkl,
    };
}

In your first example the test and the "then" expression are both short, how would the "then" expression be indented if they become too large to fit on a line?

zig fmt doesn't know or care about line length. The way it handles line breaks is driven by semantics and trailing commas. It would for example accept the following snippet no matter how long the lines were:

const variable = if (condition) std.thingo.how_long_can_you_read.TypeThing(u32, i32, std.HashMap(u32, u32)).init()
    else foobar;

If you were to insert a trailing comma into the function call, it would then give you:

const variable = if (condition) std.thingo.how_long_can_you_read.TypeThing(
            u32,
            i32,
            std.HashMap(u32, u32),
        ).init()
    else foobar;

To give another real world example from my code, this is the status quo:

pub fn next(self: *Iterator) ?*Node {
    while (self.it) |node| : (self.it = if (self.reverse) node.prev else node.next) {
        if (if (self.pending)
            self.tags & node.view.pending.tags != 0
        else
            self.tags & node.view.current.tags != 0) {
            self.it = if (self.reverse) node.prev else node.next;
            return node;
        }
    }
    return null;
}

and with the proposed changes:

pub fn next(self: *Iterator) ?*Node {
    while (self.it) |node| : (self.it = if (self.reverse) node.prev else node.next) {
        if (if (self.pending) self.tags & node.view.pending.tags != 0
            else self.tags & node.view.current.tags != 0)
        {
            self.it = if (self.reverse) node.prev else node.next;
            return node;
        }
    }
    return null;
}

I think that this is significantly more readable.

You're right that this is bikeshedding, but it is also important to get right before zig fmt stabilizes. (FWIW I don't think compile errors for zig fmt non-compliance are planned, just some indentation requirements to prevent goto-fail).

[daurnimator]

Possibly related: #3978 #4580

[bens]

Yes, that style helps a lot, I'd be quite happy to have your suggestion. The example of the mismatched if and else in your example is quite jarring.

Thanks for pointing out that commas can help with the line breaking, it's a little hard to get used to. Putting a comma after 456 helps quite a bit.

We could close this issue with @ifreund's proposal as input to #4580?

[LakeByTheWoods]

It looks like behaviour changed at some point so that the else is put on the same line as the if. Is the above proposed solution still desirable?

[LakeByTheWoods]

While I think this is certainly possible it reworking the code to render else if chains to be iterative instead of recursive, this is due to elses being part of the previous else's body