rustfmt: Gives up on chains if any line is too long.

I very much agree with Josh here, but I think even bigger issue is that there are no visible errors/warnings that formatting is not possible and why. It just silently gives up, leaving user guessing why this particular part of the code is not formatted correctly.

@calebcartwright In general, I’d expect rustfmt to indent everything to the correct level, and try to wrap appropriately at line-width, but whether it’s successful or not, it should continue on to wrap the rest, yes.

It’s not “except within chains”, it’s “when possible, without violating more important constraints like properly indenting”. If you have an indent-width of 4, and (say) 15 levels of indentation and a 50-character function name, you cannot format that without exceeding 100 characters, and that shouldn’t stop you from continuing on to format more of the line. And if you need to wrap the function parameters, those should still get indented 16 levels, even if one of them is a long string.

I just ran into this as well. My code:

fn main() {
    clap::App::new("foo")
        .subcommand(
            SubCommand::with_name("bar")
                .about("some text here")
                .setting(AppSettings::ArgRequiredElseHelp)
                .arg(
                    Arg::from_usage("-l, --latest 'some very long help text goes here [default: default version]'")
                                               .conflicts_with(  "template_version"  )
                )
        )
        .get_matches();
}

rustfmt seems to completely give up after the line with the long help text, and doesn’t even try to reindent the .conflicts_with, or fix the spacing in its argument, or add a trailing comma in either of the two places that should have trailing commas.

I’d expect rustfmt to do best-effort on the long line, and then continue afterward.

You do not need strings to get this bug:

// this does not format
let offenders = current_validators
    .into_iter()
    .enumerate()
    .filter_map(|(_, id)| 
    <<Runtime as pallet_im_online::Config>::ValidatorSet as ValidatorSetWithIdentification<
        sp_runtime::AccountId32>>::IdentificationOf::convert(id.clone()).map(|full_id| (id, full_id)))
    .collect::<Vec<IdentificationTuple<Runtime>>>();

// this type alias prevents the formatting bug
type XXX =
    <<Runtime as pallet_im_online::Config>::ValidatorSet as ValidatorSetWithIdentification<
        sp_runtime::AccountId32,
    >>::IdentificationOf;

// this does format
let offenders = current_validators
    .into_iter()
    .enumerate()
    .filter_map(|(_, id)| XXX::convert(id.clone()).map(|full_id| (id, full_id)))
    .collect::<Vec<IdentificationTuple<Runtime>>>();

And the giving up propagates to the whole block.

@nazar-pc try running with --config error_on_line_overflow=true which exists for precisely this purpose (make sure you’re on nightly), it’s just disabled by default. If you’d like to make a case for that being enabled by default then #3391 would be the place to do so

example output with that enabled:

error: line formatted, but exceeded maximum width (maximum: 100 (see `max_width` option), found: 115)
 --> <stdin>:8:8:101
  |
8 |                     Arg::from_usage("-l, --latest 'some very long help text goes here [default: default version]'")
  |                                                                                                     ^^^^^^^^^^^^^^^
  |

warning: rustfmt has failed to format. See previous 1 errors.

Using format_strings pretty much fixed it for me as strings get broken up and rustfmt can keep the line length below max_width.

I would like an option to have max_width be the width of the line not including the indentation, or a switch, like relative_max_width

So, i have two questions:

1. Is this the same issue reported here, or something different that looks similar?
2. If a general fix for this is too complicated, could at least the condition of bailing out on long strings be more local? For example, in the snippet above, could it only affect the formatting of the last block on the match, leaving only that block unformatted, but still formatting the rest of the non-offending blocks normally?

1. No, it’s separate. There’s various other comments above reporting different-but-with-similar-symptom issues, but they’re likely buried because of the volume of commentary, see https://github.com/rust-lang/rustfmt/issues/3863#issuecomment-986343312 for more
2. No, no general fix, see same linked comment. Also don’t want to add any additional noise on this thread discussing the reasoning or potential changes for a different issue that would have no impact on this issue.

Seems weird that it tries to put as much as possible on 1 line just to break itself. E.g. here formatting doesn’t work, nothing is reported with error_on_line_overflow

{
    t(|ctx, input: VeryLongInputName| async move {
       None
    })
}

If I shorten the struct name a bit then it starts working and it puts even more stuff on that problematic line. Took me several hours to pinpoint this specific place in the long chain of methods that are needed for rspc to create the procedures handler.

{
    t(|ctx, input: VeryLongInputNam| async move { None })
}

This actually reports the very long line error.

{
    t(|ctx, input: VeryLongInputName| async move { None })
}

BTW, it would be very helpful if there was a mode or cli flags that I could use to identify lines that are too long for rustfmt.

Discussed above (e.g. https://github.com/rust-lang/rustfmt/issues/3863#issuecomment-700441739) and in a few other threads, but you can turn on error_on_line_overflow and error_on_unformatted for this.

rustfmt can’t format the entire file, but it doesn’t give any indication about what line(s) need to be manually shortened.

This is a topic that pops up frequently so I’ll reiterate here that it’s important to distinguish between what rustfmt does vs. what editor plugins do, especially since we have no influence/insight into what those various plugin teams do. The decision to not apply any formatting to an entire file in some scenarios is behavior that some editor plugins have opted to apply, and is not related to rustfmt behavior in any capacity.

In these unformattable scenarios rustfmt only leaves the the top level expression as you wrote it, but will absolutely format everything else in the file.

Additionally, the aforementioned config options, though disabled by default, will highlight your lines that are too long. If you have a scenario where that’s not the case (when running rustfmt directly, not editor behavior) then happy to take a look but would ask a separate issue be filed with steps to reproduce.

No worries! It’s a rather long and fairly old thread at this point so just wanted to note there were some other posts that drilled deeper into the details since I was too lazy to fully recap 😆

Thanks, I confirm that using both error_on_line_overflow and error_on_unformatted does what I want. I’m sorry for not having properly read the previous comments about this, that was wrong of me. I’ve also edited my above comment to clarify my comment about “the entire file”.

Option (b)

Attempt to dynamically derive and then use a formatting shape with an increased width (which exceeds max_width) which will be used for that specific chain element.

The reason this would be necessary is to provide some required constraints when formatting those subexpressions to avoid horrific formatting that would try to collapse/single line everything out lines to infinity. I’d envision this operating along the lines of:

• if impossible to fit the chain element within the max_width of 100
• then create a scaled up width for this particular chain element (we could allow user control over this, e.g. long_chain_scale_factor = 20%, or rustfmt could attempt to guess one based on what it sees in the AST)
• repeat as necessary, with some max number of scaled retry attempts (which again could be user controllable)

This would be more complex, both from a user experience/mental model and implementation, but if we get it right would probably be much helpful for that cases with more complex args within chained calls.

Option (a)

Use the original input for that element (whatever the user originally wrote), with some minor adjustments like indentation of the start of that element, and trimming trailing spaces.

This would directly solve the case originally reported in this issue with the desired format being produced for the provided input, as well as most other input snippets reported throughout the thread. It’s also something we could get out on nightly fairly quickly, potentially before the end of the month. However, it likely wouldn’t help all that much in cases where the exceeding chain item is caused by a more complex arg to a chained call, e.g. a closure with various other statements/expressions/etc., as that sub-content wouldn’t be processed

With the above being said, I have finally managed to set aside some time to dig into this and believe I see a viable path forward.

As noted previously the current behavior will remain the default, but I’m planning on introducing a new config option that will provide some additional variants users can leverage to control chain formatting behavior. That would include one variant that will perform the standard chain formatting behavior, but unlike the default behavior, it will attempt to make a best effort to continue formatting chains which in part exceed the defined max_width value as requested here.

We’d also most likely want to encourage users opting for this non-default behavior to strongly consider pairing it with other options like error_on_line_overflow (which we’ll be adjusting to include off/warn/error mode variants and then hopefully stabilizing) so that you’ll be aware of cases where your formatting result exceeded your defined value for max_width.

There’s only two feasible approaches I can see for handling an element within a chain that must exceed max_width. I’d like to run a poll of sorts to gauge interest on those, so I’ll details those two in separate comments (options a and b below) and ask folks to use reactions on those comments to provide their feedback.

Note that these aren’t mutually exclusive and I’m open to supporting both if there’s interest. As such I’d ask folks to primarily utilize the “thumbs up” to indicate favor/interest, and only use the “thumbs down” if you adamantally feel the option shouldn’t even exist (don’t downvote it just because you wouldn’t use it yourself).

I’d also ask that folks refrain from requesting other options on this issue. The original use case posed in the issue is pretty straightforward and the solution I have in mind will present a resolution to this. It will also create a framework of sorts through which additional variants/formatting behaviors can be added down the road, each of which should be proposed/discussed independently in a separate issue.

Finally, in cases where a chain in a right hand side (rhs) position (e.g. an assignment) is too long fit within the boundary with either shape (i.e. same line as lhs/assignment operator vs. next line block indented), rustfmt will have to try to pick where to put the chain, and I’m thinking that we should base that on which of the two will minimize the amount of formatted chain that ends up on the right of the defined max_width boundary. There’s not a ton of alternatives to that which come to mind for me, but if anyone has questions/alternatives to that particular piece feel free to ask/suggest.

A work around that can be beneficial in certain cases is to use the include, include_str, and include_bytes macros.

more important constraints like properly indenting

What defines the relative importance of one constraint vs. another though? Is there a consensus on which constraints can be violated? Does the style guide have a prescription? The only thing I’m familiar with (and I know you’re far more familiar with the style guide than I @joshtriplett 😄) is https://github.com/rust-dev-tools/fmt-rfcs/blob/7416e12356a55aae959e9629b58eb7c7c3c86f6f/guide/guide.md#indentation-and-line-width

It’s not that we can’t technically make this change, but in these types of scenarios where rustfmt can’t satisfy all the constraints it bails and defers to the programmer to format, or refactor, as needed. This is usually quite manageable, and often accompanied with advice like refactor your code to avoid long/complex expressions, usually by extracting a local variable or using a shorter name

If you have an indent-width of 4, and (say) 15 levels of indentation and a 50-character function name, you cannot format that without exceeding 100 characters

Precisely and agreed, I was just trying to keep the example in the context of chains given the issue.

There’s also portions of the user base that do want to strictly enforce the width limits in their code and would leverage options like error_on_line_overflow to ensure that any violations of the width constraints were indeed raised, and any changes to the behavior would have to continue to honor that (an implementation detail that I’m just noting for future reference).

I also think that if max width were to be changed to more of a soft constraint then we’d need to explicitly convey that rustfmt will format your code within the limit, unless it really can’t, in which case it will format out indentations indefinitely as needed.

Doing something about this is still on my to-do list, though I do think it’s worth expanding a bit on what folks have in mind when we say “best effort”.

It seems this is most often encountered with a single long string, but will also note there are plenty of other scenarios that can run into this where IMO the expected behavior starts to get a little tricky. Consider for example a chain whose parent starts in a heavily indented position, and the chain element that exceeds the max width value is a closure param that itself has a sizeable body with additional nested indentation.

Would users still want rustfmt to format that closure anyway even though it blows way past the max width value? Are we changing the meaning/honoring of max width with a caveat of “except within chains”? Should rustfmt do this by default or should users have to explicitly opt-in to allowing rustfmt to exceed the max width when there are chains involved?

Yeah, AFAICT that’s because rustfmt is expecting to be able to rewrite each of the individual ChainItems to be able to format the Chain. When it attempts to rewrite that long string literal in the Chain it fails due to the length which results in the entire original chain being left as-is.

https://github.com/rust-lang/rustfmt/blob/a15e97f1e92b3621ab8c2f1585f27e8de583f74b/src/chains.rs#L419-L444

I think I see a way we could support still wrapping chain elements like your expected output, though it would mean rustfmt emitting a line that exceeds the max_width (although the original line does too, and the wrapped version is more human friendly IMO even with the one longer line).

@topecongiro @scampi - any concerns with that? if not, then I’ll take a look at implementing something for consideration.