[unstable option] overflow_delimited_expr

[scampi]

Tracking issue for unstable option: overflow_delimited_expr

[athre0z]

I feel like this could also apply to function calls in the tail argument, rewriting

market.open_order(
    upos,
    Order::limit(
        market.pair().clone(),
        direction,
        price,
        amount,
        self.user_id,
    ),
)?;

to

market.open_order(upos, Order::limit(
    market.pair().clone(),
    direction,
    price,
    amount,
    self.user_id,
))?;

This would allow for symmetry between structs instantiated via their constructor and constructor methods. What do you think?

[pitaj]

Why is this unstable and what needs to be done to stabilize it? Any options that reduce unnecessary indentation are wonderful IMO

[calebcartwright]

Why is this unstable

When new config options are added to rustfmt, they are almost always added as unstable options. I'm not sure whether there are any outstanding items with this particular option, but it could actually be ready for stabilization: it's entirely possible that like many of the other currently-unstable options that it's only unstable because it hasn't been stablized yet.

what needs to be done to stabilize it?

The process for stablizing a config option is documented here: https://github.com/rust-lang/rustfmt/blob/master/Processes.md#stabilising-an-option

If indeed the enumerated stabilzation conditions are true for this option, then the act of stabilizing would involve changing:

rustfmt/rustfmt-core/rustfmt-config/src/lib.rs

Line 83 in e8da49a

overflow_delimited_expr: bool, false, false,

to:

    overflow_delimited_expr: bool, false, true,

And then updating the stable flag in the configuration docs
https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#overflow_delimited_expr

[pitaj]

I should have looked around a bit more to save you the trouble, but thanks for answering!

Considering the conditions:

1. Is the default value correct ? 🚫
2. The design and implementation of the option are sound and clean. ❓
3. The option is well tested, both in unit tests and, optimally, in real usage. ❓
4. There is no open bug about the option that prevents its use. ✔️

I'll have to look into 2 and 3. The only trouble I see now is that I think the default is incorrect.

I'd propose to change it to have a version-gate as outlined here and make it enabled by default in future versions, should it be stabilized.

I've made a comment asking why not here: rust-lang/style-team#61 (comment)

I'll make a PR for whatever is necessary to stabilize this option pending the conclusion of that discussion.

[topecongiro]

@pitaj Thank you for bringing this up. We will review this option once again, and if it looks good, we will change the default value and stabilize it in the next release.

[tmandry]

I'd like to see similar option for string literals, e.g.

    gen!(f, "
        trait $trait_name {
            fn $func_name(self, $args_sig);
        }
    ")?;

Should that be configured separately, or in the same option?

In my case I can skip the macro, but it might be useful in other cases.

[jmillikin]

The current implementation of this option produces strange output when the last value is a struct, and overflowing causes the line to exceed max_width.

For example, in this diff I think breaking after the = should cause the ovreflow_delimited_expr format to not be applied. It's documented as "allow to overflow" rather than "force to overflow", so I think this would be consistent with the docs.

Alternatively, changing it to a tri-valued string would be fine.

        let mut channel = FakeChannel::new();
-       let encoder = ResponseEncoder::new(
-               &mut channel,
-               REQUEST_ID,
-               crate::Version { major: 7, minor: 9 },
-       );
+       let encoder =
+               ResponseEncoder::new(&mut channel, REQUEST_ID, crate::Version {
+                       major: 7,
+                       minor: 9,
+               });
        resp.encode_response(encoder).unwrap();

[dmitmel]

Hi, is it possible to apply this option to try blocks as well? E.g. currently overflowing try blocks are formatted as follows:

handle_broken_pipe(
  try {
    serde_json::to_writer(&mut stdout, &out_msg)?;
    stdout.write_all(b"\n")?;
  },
)
.context("Failed to serialize to stdout")?;

And if this option was applied to them, then I'd expect the output to be as follows:

handle_broken_pipe(try {
  serde_json::to_writer(&mut stdout, &out_msg)?;
  stdout.write_all(b"\n")?;
})
.context("Failed to serialize to stdout")?;

[outfoxxed]

Hello, is it possible to change this behavior with nested arrays?
Currently rustfmt changes

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

to

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

which isn't really desirable.

Consider always forcing arrays onto the next line if they would wrap at all, like the first example.

[calebcartwright]

I think this option and the existing variants are ready for stabilization, and both will be default behavior in different Style Editions as enabled by RFC 3338.

However, after spending some time thinking on this one in prep for the 2024 edition write up on it, I'm now of the opinion that we should modify this to be a multi-variant option as opposed to the current bool (likely with a rename of the option as well).

I believe doing that will allow for naming that better reflects the actual behavior, and it will give room for new (initially unstable) variants to be added at any time. Furthermore, I do think additional variants/enhancements to this will need to be added (better behavior on array/matrix scenarios is something the style team will probably target for the 2027 Style Edition)

@ytmimi thoughts?

[ytmimi]

I'm definitely in favor of stabilizing this! I'm also interested in learning more about the new name / variants you're thinking of.

[calebcartwright]

let the great bikeshed begin 😅

would something like combinable_expr_overflow_style make sense, albeit being even more of a mouthful then it already is?

edit: i'm actually not even sure we need the "overflow" nomenclature in there.

the only other place where "overflow" is exposed publicly is error_on_line_overflow where overflow is used to denote "exceeding max width", which definitely isn't what we're talking about here.

it's really more about how "args" are laid out in call-like contexts, and whether they can be combined on the same line