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: overflow_delimited_expr behavior should be reconsidered before stabilization #136224

Closed
kpreid opened this issue Jan 29, 2025 · 1 comment
Labels
A-edition-2024 Area: The 2024 edition A-rustfmt Area: Rustfmt I-style-nominated Nominated for discussion during a style team meeting. T-style Relevant to the style team, which will review and decide on the PR/issue.

Comments

@kpreid
Copy link
Contributor

kpreid commented Jan 29, 2025

(The following is a continuation of this Zulip thread.)

Style edition 2024, as implemented in rustfmt via implying the option overflow_delimited_expr = true, causes certain code to be wrapped very oddly. While this formatting is “working as designed”, I believe it should be reconsidered — at least its stabilization in its current form in the 2024 edition. It will affect any code which uses nested arrays, particularly to write literal tables/matrices. Example from my code:

// 2021
const FACE_TABLE: [[Face7; 2]; 3] = [
    [Face7::PX, Face7::NX],
    [Face7::PY, Face7::NY],
    [Face7::PZ, Face7::NZ],
];

// 2024
const FACE_TABLE: [[Face7; 2]; 3] = [[Face7::PX, Face7::NX], [Face7::PY, Face7::NY], [
    Face7::PZ,
    Face7::NZ,
]];

It will also affect any function or macro call which accepts two semantically parallel arrays, structs, or literals. This is more debatable, but I think it is often worse:

// 2021
let bounds = GridAab::from_lower_upper(
    [-2, -1, -2],
    [(n_x - 1) * spacing_x + 3, 20, (n_z - 1) * spacing_z + 3],
);

// 2024
let bounds = GridAab::from_lower_upper([-2, -1, -2], [
    (n_x - 1) * spacing_x + 3,
    20,
    (n_z - 1) * spacing_z + 3,
]);
// 2021
assert_eq!(
    Rgba::new(0.125, 0.25, 0.5, 0.75).to_srgb8(),
    [99, 137, 188, 191]
);

// 2024
assert_eq!(Rgba::new(0.125, 0.25, 0.5, 0.75).to_srgb8(), [
    99, 137, 188, 191
]);

See rust-lang/rustfmt#6452 for more examples of bad formatting from someone else’s code.

Some codebases will be greatly affected by this because they have large numbers of such calls — depending on their problem domain and API design. It is possible to prevent rustfmt from changing to overflow style by adding a strategic comment:

const FACE_TABLE: [[Face7; 2]; 3] = [
    [Face7::PX, Face7::NX], //
    [Face7::PY, Face7::NY],
    [Face7::PZ, Face7::NZ],
];

but this is an unobvious trick, and applying it to existing code is an unpleasant experience to have as part of edition migration.

I'm not saying that this is a bad change overall. In fact, I am very pleased by its reduction in indentation levels and vertical space occupied solely by brackets, which I think will aid readability by allowing more code to fit on screen at once. However, I think that in its current form, its effect on arrays and other parallel constructs is a significant regression in Rust’s quality of “having good default behavior”, which is going to affect a lot of people who choose to attempt 2024 edition migration as soon as Rust 1.85 is released.

Therefore, I believe that the stabilization of the 2024 style edition with overflow_delimited_expr=true behavior, in its current form and without also stabilizing the ability to control overflow_delimited_expr, is unwise. Comments on the PR adding this style change suggested that it would be desirable to stabilize the option too, but no action has currently been taken. I’m filing this issue in order to bring attention to the matter and provide an opportunity to discuss whether it is wise to stabilize this formatting in its current form.

I believe that one of the following actions should be taken:

  • Remove overflow_delimited_expr=true from the 2024 style edition, deferring it until it can be revised. (Least-effort option.)
  • Change overflow_delimited_expr=true so that it does not do this.
    • Perhaps it does not overflow a final array if the preceding element is also an array.
    • ia0 had another scheme.
  • Stabilize the overflow_delimited_expr rustfmt option so that negatively affected codebases can disable it.

Per TC's request, nominating this for discussion:

@rustbot label +A-edition-2024 +T-style +T-rustfmt +I-style-nominated

CC @traviscross @ytmimi @pitaj @rust-lang/style @rust-lang/rustfmt @ehuss

Tracking:

Alternate rule proposal:

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 29, 2025
@pitaj pitaj added A-rustfmt Area: Rustfmt I-style-nominated Nominated for discussion during a style team meeting. T-style Relevant to the style team, which will review and decide on the PR/issue. A-edition-2024 Area: The 2024 edition and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 29, 2025
@compiler-errors
Copy link
Member

#136312 was landed on nightly and backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition A-rustfmt Area: Rustfmt I-style-nominated Nominated for discussion during a style team meeting. T-style Relevant to the style team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants