Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
LewisGaul opened this issue Dec 26, 2021 · 3 comments
Open

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

LewisGaul opened this issue Dec 26, 2021 · 3 comments
Labels
bug Observed behavior contradicts documented or intended behavior zig fmt
Milestone

Comments

@LewisGaul
Copy link
Contributor

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 LewisGaul added the bug Observed behavior contradicts documented or intended behavior label Dec 26, 2021
@ifreund ifreund added this to the 0.10.0 milestone Dec 27, 2021
@ifreund
Copy link
Member

ifreund commented Dec 27, 2021

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

curtiswilkinson added a commit to curtiswilkinson/zig that referenced this issue Mar 20, 2022
column-cased alignment.

This fixes multiple issues caused by bypassing the renderToken()
infrastructure, as described in ziglang#10418
magurotuna added a commit to shiguredo/quic-server-zig that referenced this issue Aug 25, 2022
If we put the directive inside the array declarations, `zig fmt: on`
does not work. For detail see ziglang/zig#10418
@andrewrk andrewrk modified the milestones: 0.10.0, 0.11.0 Sep 14, 2022
wooster0 added a commit to wooster0/zig that referenced this issue Jun 2, 2023
This had a number of problems, including:

1. Complexity: the code was complicated and it was the only place where allocation is used.
   Now zig token rendering is free of heap allocation.
2. Unicode awareness: handling Unicode properly will probably look like
   this: https://github.com/unicode-rs/unicode-width/blob/f5a9b4efa5142d20d5f23070542e46e76f426a30/src/tables.rs
   I'm not sure we want that too. Or do we only want basic Unicode awareness then?
   Problem is there will always be new issues "oh for these characters the width is wrong though".
   And I don't think adding those tables alone would be the end of this story, either.
3. Inconsistency: if I can do
   ```
   _ = .{
       x, y,
       z, w,
   };
   ```
   , why can't I also do
   ```
   func(
       1, 1,
       1, 1,
   );
   ```
   ?
   These rules weren't applied consistently at all.

Fixes ziglang#10418
Fixes ziglang#15929
@wooster0
Copy link
Contributor

wooster0 commented Jun 2, 2023

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.

@perillo
Copy link
Contributor

perillo commented Jan 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior zig fmt
Projects
None yet
Development

No branches or pull requests

5 participants