zig icon indicating copy to clipboard operation
zig copied to clipboard

'zig fmt: on' directive not working inside array declaration

Open LewisGaul opened this issue 4 years ago โ€ข 1 comments

Zig Version

0.10.0-dev.63+6b8e33d14

Steps to Reproduce

const a = .{
    // zig fmt: off
    .{ 0,      1    },
    // zig fmt: on
};

// No formatting here...
const b =   2;
test {
    _ =      1;
}

// This fixes it:
// zig fmt: on
const c = 3;

Expected Behavior

Expected // zig fmt: on to re-enable formatting

Actual Behavior

The zig fmt: on directive seems to be ignored inside a static struct/array declaration, even when zig fmt: off took effect inside it.

LewisGaul avatar Dec 26 '21 23:12 LewisGaul

This happens because renderArrayInit() bypasses the renderToken() function used everywhere else in render.zig. This means that renderComments(), which handles the off/on directives as well as other things, is not called for these comments.

The reason that renderArrayInit() is so complex (in addition to being the only place renderToken() is bypassed, it is the only place in render.zig where dynamic memory allocation is used) is to achieve the automatic alignment of items in array init expressions. For example:

const foo = .{
    "a",   "b",
    "ccc", "d",
};

Sadly, even with this current complexity it fails badly when faced with the much greater complexity of unicode:

const foo = .{
    "a",                         "b",
    "๐Ÿ‘จโ€๐Ÿ‘ฉโ€๐Ÿ‘งโ€๐Ÿ‘ฆ", "d",
};

I propose that we remove this auto alignment code in renderArrayInit() from zig fmt in favor of zig fmt's off/on directives which work at token granularity and the user's text editor to perform the alignment. This is the status quo for everything in the language that one might want to align aside from array init expressions:

// zig fmt: off
const foo = .{
    "a", "b",
    "๐Ÿ‘จโ€๐Ÿ‘ฉโ€๐Ÿ‘งโ€๐Ÿ‘ฆ", "d",
};
// zig fmt: on

ifreund avatar Dec 27 '21 03:12 ifreund

I attempted this but the problem is not fixing the issue itself but rather the migration to the new zig fmt rules. The diff after zig-out/bin/zig fmt . --exclude test/cases is absolutely massive (mainly in lib/std/crypto) and it takes a lot of effort to add // zig fmt: off and // zig fmt: on in every single place. Even if I did it all by hand it probably wouldn't even be reviewed because the diff would be too big.

Also, the zig codebase is not going to be the only place where array inits rely on this auto alignment.

I think what we need to do is for one release we need to detect code array inits like this that rely on this auto alignment code and then add // zig fmt: off and // zig fmt: on at the start and end. I think it'd be ok to do some guessing "oh this is probably a big table with alignment that we want to keep". zig fmt would do this automatically for one release. And then after that release that code is removed and we're done.

https://github.com/r00ster91/zig/commits/fmtlist is my branch. See top 3 commits. Only src/ is formatted with the new rules currently. Anyone is free to continue on this branch and either do some manual formatting or come up with a way to automatically add // zig fmt: off and // zig fmt: on everywhere.


We might also want to change // zig fmt to just // fmt.

wooster0 avatar Jun 02 '23 22:06 wooster0

We might also want to change // zig fmt to just // fmt.

What about being explicit (like with doc_comment and container_doc_comment) and add a new pi_comment token (where pi means Processing Instruction)?

As an example:

//# zig fmt on

will generate a pi_comment token with "zig fmt on" as payload.

perillo avatar Jan 30 '24 10:01 perillo