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

fmt: allow multiline destructuring #22768

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Feb 5, 2025

While unpacking is a niche feature, and you probably shouldn't unpack too many things at the same time, there are cases where this is useful, and it feels easy to support.

The specific motivating example is from TigerBeetle's build system, where we need to parse cli args of a specific known shape:

pub fn main() !void {
    var arena = std.heap.ArenaAllocator.init(std.heap.page_allocator);
    const allocator = arena.allocator();
    const args = try std.process.argsAlloc(allocator);
    if (args.len != 8) @panic();

    const title,
    const author,
    const url_prefix,
    const nav,
    const url_page_source,
    const source_file_path,
    const target_file_path = args;

@matklad
Copy link
Contributor Author

matklad commented Feb 5, 2025

Note: I am only 0.7 sure this is a good idea :P

@matklad matklad force-pushed the matklad/unpack-per-line branch from 9fd71e6 to 19bef31 Compare February 5, 2025 11:19
@castholm
Copy link
Contributor

castholm commented Feb 5, 2025

const multiline = !tree.tokensOnSameLine(tree.firstToken(node), full.ast.equal_token);

It's a minor inconvenience (especially since destructuring is not that common) but I'll just note that this being the rule that decides whether the list is rendered horizontally or vertically means that there's no single-character edit the user can make to transform a multiline list into a single-line one.

Almost all other list constructs base their multiline/single-line-ness on the presence/absence of an optional trailing comma, which makes it easy to go from one form to the other by adding/removing the comma and running zig fmt.

The grammar for destructuring assignments doesn't allow for a trailing comma (why?) but in the absence of language-level changes, perhaps you could relax the rule something like "is there a newline between the N-1th element's comma and the first token of the Nth element?"? That way there's still a simple edit the user can make to wrap/unwrap long lists.

@matklad
Copy link
Contributor Author

matklad commented Feb 5, 2025

means that there's no single-character edit the user can make to transform a multiline list into a single-line one.

Ah, that's a very good observation, I completely missed that, thanks!

@matklad matklad force-pushed the matklad/unpack-per-line branch from 19bef31 to fd45dd0 Compare February 5, 2025 13:20
While unpacking is a niche feature, and you probably shouldn't
unpack too many things at the same time, there _are_ cases
where this is useful, and it feels easy to support.

The specific motivating example is from TigerBeetle's build system,
where we need to parse cli args of a specific known shape:

pub fn main() !void {
    var arena = std.heap.ArenaAllocator.init(std.heap.page_allocator);
    const allocator = arena.allocator();
    const args = try std.process.argsAlloc(allocator);
    if (args.len != 8) @Panic();

    const title,
    const author,
    const url_prefix,
    const nav,
    const url_page_source,
    const source_file_path,
    const target_file_path = args;
@matklad matklad force-pushed the matklad/unpack-per-line branch from fd45dd0 to c018696 Compare February 5, 2025 13:21
@rohlem
Copy link
Contributor

rohlem commented Feb 5, 2025

(Note: The langref consistently names this feature "destructuring", not "unpacking".)

I also think we should instead allow a trailing comma, and use this to decide here as we do everywhere else.

It's pretty unsatisfying that reading const x = f();,
you actually have to look for the end of the previous statement to know whether x holds the entire result,
or is just the last in a list of destructuring destinations.
Therefore even in status-quo I think I would prefer the canonical form to split it like

const a,
// ...
const b
= f();

but I might be in the minority here.

@matklad matklad changed the title fmt: allow multiline unpacking fmt: allow multiline destructuring Feb 5, 2025
@mlugg
Copy link
Member

mlugg commented Feb 6, 2025

I agree that sometimes, due to long type annotations, it can be desirable to split destructuring statements onto many lines. I'm not quite sure what the optimal way to do that is.

The logic for not including a trailing comma here was that we already have a trailing symbol: =! Having a comma too felt redundant; this construct is fundamentally asymmetrical, whereas things like array initializers and argument lists are much cleaner (you "open" the syntax with { or (, have the list, then "close" the syntax with } or )).

I wonder whether the non-initial items should actually be indented:

doSomeStuff()
const foo,
    const bar,
    const baz,
    const qux =
    something;
doSomeOtherStuff();

The benefit is that I think this makes it far more clear that this is a continuation of a preceding statement. The downsides are that it's kind of ugly, and it eliminates a visual distinction between the LHS items and ths RHS if the RHS is on its own line. I probably think this does more harm than good, but I'll throw the idea out there anyway.

@matklad
Copy link
Contributor Author

matklad commented Feb 6, 2025

Yeah, on my side, I also considered if we might want to highlight this more, but, at least for motivating example, it's sort of super-obvious visually what's happening anyway. THere's a giant row of aligned const, with a blank space to the right.

Don't have a super-strong opinion here though!

@castholm
Copy link
Contributor

castholm commented Feb 6, 2025

The logic for not including a trailing comma here was that we already have a trailing symbol: =! Having a comma too felt redundant; this construct is fundamentally asymmetrical, whereas things like array initializers and argument lists are much cleaner (you "open" the syntax with { or (, have the list, then "close" the syntax with } or )).

Apologies in advance if this is veering too much into bikeshedding/unsolicited language proposal territory, but this doesn't seem like a very compelling argument against trailing commas seeing as we already have switch cases, which have very similar visual characteristics to destructuring assignments and which permit a trailing comma:

const result = switch (tag) {
    .one,
    .two,
    .three,
    => something,

    .four,
    .five,
    .six,
    => .{
        4,
        "5",
        "six",
    },
};

I think destructuring assignments formatted in a similar way read quite nicely:

const one: usize,
const two: *const [1]u8,
const three: []const u8,
= something;

const four: usize,
const five: *const [1]u8,
const six: []const u8,
= .{
    4,
    "5",
    "six",
};

The lone unindented = on the new line sticks out just enough visually to make it clear that there's an assignment happening.

There's also an argument to be made that destructuring assignments is the odd one out in terms of language consistency. It is currently the only construct in the language where a trailing comma after an unbounded list of items is not permitted. (Perhaps you could still forbid it after a single item, for clarity since you can't destructure a 1-tuple, if that's a concern.)

@llogick
Copy link
Contributor

llogick commented Feb 6, 2025

I've been making use of // comments in those scenarios, eg

    const title, const author, //
    const url_prefix, //
    const nav, //
    const url_page_source, //
    const source_file_path, //
    const target_file_path //
    = .{ 1, 2, 3, 4, 5, 6, 7 };

Note how they allow for flexibility in how many variables declared on the same line, can group one or more.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    const title,
    const author,
    const url_prefix,
    const nav,
    const url_page_source,
    const source_file_path,
    const target_file_path = args;

What I don't like about this is the last line looks like a statement unless you realize the lines above affect it.

In this one I don't like that something is on the same indentation as const qux:

doSomeStuff()
const foo,
    const bar,
    const baz,
    const qux =
    something;
doSomeOtherStuff();

I find this to be acceptable:

    const title,
    const author,
    const url_prefix,
    const nav,
    const url_page_source,
    const source_file_path,
    const target_file_path,
      = args;

This one I also don't mind, because the
comma above the = is very visible:

    const title,
    const author,
    const url_prefix,
    const nav,
    const url_page_source,
    const source_file_path,
    const target_file_path =
      args;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants