std/zig/render: Rewrite indentation

[87flowers]

Closes #7363. Closes #21102.

This PR rewrites how indentation is handed in std.zig.render, which results in better indentation for zig fmt.

PR is intended to be reviewed one commit at time. It will likely be easier to review without the final commit.

Formatting changes

Please see f972539 for the result of running zig fmt src for a more complete demonstration.

Some examples:

// Before
test {
     if (first_param_type.isGenericPoison() or
         (first_param_type.zigTypeTag(zcu) == .pointer and
         (first_param_type.ptrSize(zcu) == .One or
         first_param_type.ptrSize(zcu) == .C) and
         first_param_type.childType(zcu).eql(concrete_ty, zcu)))
     {
         f(x);
     }
}

// After
test {
    if (first_param_type.isGenericPoison() or
        (first_param_type.zigTypeTag(zcu) == .pointer and
            (first_param_type.ptrSize(zcu) == .One or
                first_param_type.ptrSize(zcu) == .C) and
            first_param_type.childType(zcu).eql(concrete_ty, zcu)))
    {
        f(x);
    }
}

// Before
test {
    const foo =
        if (1 == 2)
        1
    else if (3 > 4)
        2
    else
        0;

    const foo, const bar =
        if (1 == 2)
        .{ 0, 0 }
    else if (3 > 4)
        .{ 1, 1 }
    else
        .{ 2, 2 };

// After
test {
    const foo =
        if (1 == 2)
            1
        else if (3 > 4)
            2
        else
            0;

    const foo, const bar =
        if (1 == 2)
            .{ 0, 0 }
        else if (3 > 4)
            .{ 1, 1 }
        else
            .{ 2, 2 };
}

Design

indent_stack

Indentation should only ever increment by one from one line to the next, no matter how many new indentation scopes are introduced.

This is implemented by keeping track of indentation scopes in a stack (indent_stack), and only realizing the latest one when a newline occurs.

As an example:

while (foo) if (bar)
    f(x);

The body of while introduces a new indentation scope, but the body of if also introduces a new indentation scope. When the newline is seen, only the indentation scope of the if is realized, and the while is not.

This is a lot more straightforward to reason about, and removes the necessity of "oneshot"s.

space_stack

In order to ensure the correct indentation of comments, keeping track of appropriate indentation levels of comments after a semicolon or a comma is required in this new scheme. space_stack does this by keeping track of the indentation level whenever a context that ends in a .semicolon or a .comma is introduced.

.after_equals and .binop indentation collapse

This was implemented in order to keep the prettiness of a particular indentation style:

test {
    const is_alphanum =
        (ch >= 'a' and ch <= 'z') or
        (ch >= 'A' and ch <= 'Z') or
        (ch >= '0' and ch <= '9');
}

If you remove this commit, the above would instead be formatted like this:

test {
    const is_alphanum =
        (ch >= 'a' and ch <= 'z') or
            (ch >= 'A' and ch <= 'Z') or
            (ch >= '0' and ch <= '9');
}

which i feel is less desirable.

[87flowers]

CC @castholm

[castholm]

CC @castholm

(Just so there's no confusion, I have no affiliation with the Zig compiler team and am just a random sporadic contributor.)

If you can find a way to fix the failing zig fmt test cases (the ones that don't need to have their expectations updated) while preserving the new behavior this should be a net positive. The indent stack approach with a realized flag is roughly what I ended up with during my own rewrite experiments in the past.

I quickly copy/pasted your code into my local copy of render.zig to test it and I found one tricky case that it doesn't appear to fix (but it's equally broken with the status quo renderer):

comptime {
    // if blah blah blah then we should ...
    const result = if (foo)
        one()
    // otherwise, if blah blah then we need to ...
    else if (bar)
        two()
    // otherwise, fall back to ...
    else
        three();
}

// becomes

comptime {
    // if blah blah blah then we should ...
    const result = if (foo)
        one()
        // otherwise, if blah blah then we need to ...
    else if (bar)
        two()
        // otherwise, fall back to ...
    else
        three();
}

The comments one the lines before else become indented instead of remaining at the same level of indentation as the else keywords.

[87flowers]

I'm not sure if I would expect the former to be rendered in that example, in the way one can imagine invisible braces:

comptime {
    // if blah blah blah then we should ...
    const result = if (foo) {
        one()
        // otherwise, if blah blah then we need to ...
    } else if (bar) {
        two()
        // otherwise, fall back to ...
    } else {
        three();
    }
}

[mlugg]

I would consider the behavior @castholm suggests to be unexpected and unintuitive, and this PR's behavior to therefore be correct in this instance.

[87flowers]

Fixed tests locally.

[ifreund]

Very nice work! This approach seems obviously superior to the status quo in my opinion. I've read through render.zig pretty carefully and nothing jumped out at me as obvious code smell.

I think it would be good to add examples from your PR description as test cases in parser_test.zig to ensure that the new behavior does not regress.

Also, it would be nice to include the nice high level design notes in your PR description as doc comments in AutoIndentingStream to help readers orient themselves. I found the examples in the PR description particularly helpful to understand the approach taken.

[87flowers]

I think it would be good to add examples from your PR description as test cases in parser_test.zig to ensure that the new behavior does not regress.

Also, it would be nice to include the nice high level design notes in your PR description as doc comments in AutoIndentingStream to help readers orient themselves. I found the examples in the PR description particularly helpful to understand the approach taken.

Both excellent suggestions and will do so.

[andrewrk]

Merging this will require a little bit of strategy since it will cause just about every other open PR to need a rebase. I apologize in advance but I will save it for a few weeks from now when I will try to get the PR count lower, and then choose that time to land it.

[87flowers]

@andrewrk Understandable! Happy to rebase whenever you're ready.

[87flowers]

rebased onto latest master

[andrewrk]

Thanks. Planning to land this before the release for sure.

[ifreund]

This probably closes #5777 as well.

[andrewrk]

Thank you for your patience on this one. I merged it by checking out your branch, deleting the zig build fmt commit (thank you for keeping that one separate), fixing merge conflicts, and building the compiler, then re-running zig build fmt.

FYI I have made many changes to render.zig in an active branch however I will take responsibility for dealing with those merge conflicts when I resume working on it.

[87flowers]

Many thanks!

[andrewrk]

This probably closes #5777 as well.

I checked just now. Issue remains