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

[style edition 2024] Combine all delimited exprs as last argument #114764

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

pitaj
Copy link
Contributor

@pitaj pitaj commented Aug 12, 2023

Closes rust-lang/style-team#149

If this is merged, the rustfmt option overflow_delimited_expr should be enabled by default in style edition 2024.

Rendered

r? joshtriplett

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-style Relevant to the style team, which will review and decide on the PR/issue. labels Aug 12, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 12, 2023

Some changes occurred in src/doc/style-guide

cc @rust-lang/style

@joshtriplett joshtriplett added the I-style-nominated Nominated for discussion during a style team meeting. label Aug 15, 2023
the arguments and the first line of the closure fit on the first line, use the
same combining behavior:
If the last argument is a multi-line closure with an explicit block,
only apply the combining behavior if there are no other closure arguments.
Copy link
Member

@joshtriplett joshtriplett Aug 15, 2023

Choose a reason for hiding this comment

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

Do we want to keep this property? Here's the last example from this section, reformatted without this restriction:

foo(first_arg, |x| x.bar(), |param| {
    action();
    foo(param)
})

We might want to prohibit this formatting, but let's consider this case specifically before assuming we want to preserve this restriction.

Based on this formatting, I do think we should keep this restriction, to make it easier to read the separate closures.

@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Aug 15, 2023

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 15, 2023
@joshtriplett joshtriplett added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2023
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Aug 16, 2023
@rfcbot
Copy link

rfcbot commented Aug 16, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@calebcartwright
Copy link
Member

@rfcbot concern require all checkboxes

(note I don't have any specific concerns, haven't even had a chance to read yet, it's just that given the small team size we require all 4 approvals)

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 16, 2023
@pitaj
Copy link
Contributor Author

pitaj commented Aug 31, 2023

I recently learned of this issue.

It brings up that there are currently some inconsistencies with how the overflow_delimited_expr option is implemented.

Changing their example from arrays of arrays to arrays of tuples completed changes the formatting behavior, which I don't think should be the case.

There are also cases where applying this option collapses in the wrong position, like the following:

// Expected
vbuf.write(i * 3, Bar(
    Foo { x: 1, y: 2, z: 3 },
    Foo { x: 1, y: 2, z: 3 },
    Foo { x: 1, y: 2, z: 3 },
));

// Actual
vbuf.write(
    i * 3,
    Bar(Foo { x: 1, y: 2, z: 3 }, Foo { x: 1, y: 2, z: 3 }, Foo {
        x: 1,
        y: 2,
        z: 3,
    }),
);

I'm going to think about how to specify the desired behavior for these cases. My first thought is something like this:

Applying this role recursively should use the level of recursion that results in the minimal number of lines. When two levels of recursion have equal numbers of lines, the shallower level should be used.

@pitaj
Copy link
Contributor Author

pitaj commented Aug 31, 2023

That might be a good idea to include regardless, but doesn't cover cases like the following:

vbuf.write(i + 3, &[
    [h - 0.1, v],
    [h - 0.1, v],
    [h - 0.1, v],
    [h - 0.1, v],
]);

vbuf.write(i + 3, (
    Foo { x, y },
    Bar { z, w },
    Thing { a, b },
));

A better idea might be to apply the same rule that closures with explicit blocks get:

If the last argument is a struct literal, only apply the combining behavior if there are no other struct literals.

If the last argument is an array literal, only apply the combining behavior if there are no other array literals.

Etc.

@calebcartwright
Copy link
Member

I'm generally in favor of doing this, though I'm still holding off on providing my ✔️ as I feel there's some unanswered questions/additions (e.g. #114764 (comment)) and it's unclear to me whether those have been implicitly addressed or if perhaps they were accidentally overlooked?

@pitaj
Copy link
Contributor Author

pitaj commented Sep 21, 2023

@calebcartwright good point. I suppose my most recent change kinda implicitly answered Josh's concern, by expanding the rule across all kinds of expressions.

To directly answer it, I agree with him that the rule has value and should stay. The reason I didn't respond to it before is that I thought it was for T-style discussion rather than aimed at me.

It does bring up one more minor question: Any closure can prevent an explicit-body closure from being combined. Should the same apply to tuple vs field structs/enums?

My preemptive answer is that this is undecidable without type information, so we shouldn't even try. Even if it were possible, tuple struct/enum syntax is more similar to a function call than to the field struct/enum. For closures it makes sense, since a closure without an explicit body doesn't have a clear terminator, unlike functions calls and tuple structs/enums which terminate with a closing paren.

@yaahc yaahc removed the I-style-nominated Nominated for discussion during a style team meeting. label Oct 4, 2023
@joshtriplett
Copy link
Member

@pitaj I don't think that last change is as clear-cut as it looks. On the one hand, if you have a few short struct initializers, the examples you give look compelling. On the other hand, with a couple of short struct initializers (1-2 fields) followed by a long initializer (with a lot more fields), I think it makes sense to break the long initializer across lines and keep the short initializers in the same line:

func(Small { a: b }, AlsoSmall { x }, Long {
    field,
    field2: value,
    field3: another_value,
    field4: long_expression(),
    field5,
});

So I would propose that we don't have this restriction. The last commit was also added after the FCP. Could you please drop the last commit?

There's a long history of trying to find some general rule that avoids splitting up matrices or similar things, and tries to visually balance or align similar things; the problem is we've never found a sufficiently general rule that works in the general case. I don't think this specific rule works in the general case.

That said, separate from this, I'd also love to see us having complexity-based rules for when to break lines; by way of example, I do think that func((a, b, c), (d, e, f)) is something we should break across lines, because it's visually hard to parse (with commas at two levels on the same line). That's not a chaining-specific rule, though.

@pitaj
Copy link
Contributor Author

pitaj commented Nov 2, 2023

I'll revert the commit next chance I get, sorry about that.

The purpose of the change is not to improve formatting, but to prevent formatting regressions. These cases are currently formatted well, it would be unfortunate to make them significantly worse with this change.

Even with the "same expression kind" exception, this rule would still be a net positive improvement for most cases. All the exception does is preserve the status quo for a limited subset of cases. And I think it does improve readability, even in your example.

@calebcartwright
Copy link
Member

@rfcbot resolved require all checkboxes

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 15, 2023
@rfcbot
Copy link

rfcbot commented Nov 15, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 25, 2023
@rfcbot
Copy link

rfcbot commented Nov 25, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@pitaj
Copy link
Contributor Author

pitaj commented Nov 25, 2023

I'm on vacation so it may be a few days before I can squash the commits here.

I plan on following up with another PR for the "same expression kind" rule.

@pitaj pitaj force-pushed the style-delimited-expressions branch from d2791fa to bed0c9d Compare November 30, 2023 02:45
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 14, 2023
@pitaj
Copy link
Contributor Author

pitaj commented Dec 18, 2023

FYI, this has been squashed and should be ready to merge.

@ytmimi
Copy link
Contributor

ytmimi commented Jan 19, 2024

I saw this comment on Zulip, which linked to this PR. I just want to make sure I'm not missing anything. Would merging this PR require us to make changes to rustfmt? I'm also noticing that overflow_delimited_expr is currently an unstable configuration option. Does it make sense to change the default give that the option is unstable? @calebcartwright is the plan to stabilize the option in rustfmt?

@pitaj
Copy link
Contributor Author

pitaj commented Jan 19, 2024

It does require rustfmt changes.

I don't think this necessitates stabilizing the feature, but I don't see why you wouldn't given all the work would need to be done for the edition anyways.

@joshtriplett
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 23, 2024

📌 Commit bed0c9d has been approved by joshtriplett

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 23, 2024
@ytmimi
Copy link
Contributor

ytmimi commented Jan 24, 2024

It does require rustfmt changes.

I don't think this necessitates stabilizing the feature, but I don't see why you wouldn't given all the work would need to be done for the edition anyways.

I'd personally feel much better about enabling overflow_delimited_expr by default if it were stabilized. stable rustfmt doesn't allow users to configure unstable options, so it feels off to me that we'd turn on unstable formatting for them.

@pitaj
Copy link
Contributor Author

pitaj commented Jan 24, 2024

To be clear, I think it totally makes sense to stabilize the option as well, I was just saying that technically it's not absolutely required.

fmease added a commit to fmease/rust that referenced this pull request Jan 24, 2024
… r=joshtriplett

[style edition 2024] Combine all delimited exprs as last argument

Closes rust-lang/style-team#149

If this is merged, the rustfmt option `overflow_delimited_expr` should be enabled by default in style edition 2024.

[Rendered](https://github.com/pitaj/rust/blob/style-delimited-expressions/src/doc/style-guide/src/expressions.md#combinable-expressions)

r? joshtriplett
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2024
Rollup of 10 pull requests

Successful merges:

 - rust-lang#114764 ([style edition 2024] Combine all delimited exprs as last argument)
 - rust-lang#118326 (Add `NonZero*::count_ones`)
 - rust-lang#118636 (Add the unstable option  to reduce the binary size of dynamic library…)
 - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions)
 - rust-lang#119616 (Add a new `wasm32-wasi-preview2` target)
 - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions)
 - rust-lang#120265 (Remove no-system-llvm)
 - rust-lang#120284 (privacy: Refactor top-level visiting in `TypePrivacyVisitor`)
 - rust-lang#120285 (Remove extra # from url in suggestion)
 - rust-lang#120299 (Add mw to review rotation and add some owner assignments)

Failed merges:

 - rust-lang#120124 (Split assembly tests for ELF and MachO)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#114764 ([style edition 2024] Combine all delimited exprs as last argument)
 - rust-lang#118326 (Add `NonZero*::count_ones`)
 - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions)
 - rust-lang#119616 (Add a new `wasm32-wasi-preview2` target)
 - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions)
 - rust-lang#120265 (Remove no-system-llvm)
 - rust-lang#120284 (privacy: Refactor top-level visiting in `TypePrivacyVisitor`)
 - rust-lang#120285 (Remove extra # from url in suggestion)
 - rust-lang#120299 (Add mw to review rotation and add some owner assignments)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 61e2b41 into rust-lang:master Jan 24, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2024
Rollup merge of rust-lang#114764 - pitaj:style-delimited-expressions, r=joshtriplett

[style edition 2024] Combine all delimited exprs as last argument

Closes rust-lang/style-team#149

If this is merged, the rustfmt option `overflow_delimited_expr` should be enabled by default in style edition 2024.

[Rendered](https://github.com/pitaj/rust/blob/style-delimited-expressions/src/doc/style-guide/src/expressions.md#combinable-expressions)

r? joshtriplett
@ehuss
Copy link
Contributor

ehuss commented Jan 25, 2024

Is there anything tracking for updating rustfmt to include this change?

@ytmimi
Copy link
Contributor

ytmimi commented Jan 25, 2024

Not to my knowledge, no. I don't think we've added anything on the rustfmt side to specifically to track the work for this change

@calebcartwright
Copy link
Member

responding to the various comments about rustfmt stabilization:

in general, rustfmt having a configuration option that has different default values depending on the specified style edition value is not only permissible but essentially expected given the nature of the style edition as the mechanism to functionally evolve default styles over time

whether or not a configuration option meets all the criteria for stabilization as required by rustfmt's stabilization process strikes me as orthogonal; rustfmt allowing users on stable to specify any supported value is different from rustfmt's default value.

@ytmimi IIRC we already discussed this, but if not and if you've still got concerns then we can review, and if necessary follow up with the style team. Determining what makes the final cut for a style edition is going to be a collaborative process between the style and rustfmt teams, and if there's implementation timeline challenges then we'll let style team know and adjust accordingly.

That being said, I do not personally anticipate any issues with this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-style Relevant to the style team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delimited expressions should be allowed as last argument
10 participants