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

Gives up on chains if any line is too long. #3863

Open
Tracked by #83
ehuss opened this issue Oct 14, 2019 · 53 comments
Open
Tracked by #83

Gives up on chains if any line is too long. #3863

ehuss opened this issue Oct 14, 2019 · 53 comments
Labels
a-chains a-strings String literals
Milestone

Comments

@ehuss
Copy link
Contributor

ehuss commented Oct 14, 2019

If there is a method call chain, and just one call exceeds max_width, it seems like rustfmt gives up and doesn't format anything.

Example:

fn f() {
    foo("This text is under the max_width limit, and shouldn't cause any problems on its own.").long("But this line is extra long, and doesn't fit within 100 max_width. 1234567890123456789").baz().collect().unwrap();
}

No format changes will be applied to this. I'm also a bit surprised that error_on_line_overflow does not complain.

I would expect it to wrap each call chain element, even if one of them is too long, such as:

fn f() {
    foo("This text is under the max_width limit, and shouldn't cause any problems on its own.")
        .long("But this line is extra long, and doesn't fit within 100 max_width. 1234567890123456789")
        .baz()
        .collect()
        .unwrap();
}

All default settings.
rustfmt 1.4.8-nightly (afb1ee1 2019-09-08)

@calebcartwright
Copy link
Member

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.

rustfmt/src/chains.rs

Lines 419 to 444 in a15e97f

fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option<String> {
debug!("rewrite chain {:?} {:?}", self, shape);
let mut formatter = match context.config.indent_style() {
IndentStyle::Block => {
Box::new(ChainFormatterBlock::new(self)) as Box<dyn ChainFormatter>
}
IndentStyle::Visual => {
Box::new(ChainFormatterVisual::new(self)) as Box<dyn ChainFormatter>
}
};
formatter.format_root(&self.parent, context, shape)?;
if let Some(result) = formatter.pure_root() {
return wrap_str(result, context.config.max_width(), shape);
}
// Decide how to layout the rest of the chain.
let child_shape = formatter.child_shape(context, shape)?;
formatter.format_children(context, child_shape)?;
formatter.format_last_child(context, shape, child_shape)?;
let result = formatter.join_rewrites(context, child_shape)?;
wrap_str(result, context.config.max_width(), shape)
}

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.

ehuss added a commit to ehuss/cargo that referenced this issue Oct 18, 2019
@scampi
Copy link
Contributor

scampi commented Oct 21, 2019

@calebcartwright sounds good to me!

@agate-pris
Copy link

I think this issue is duplicate of #2970 .

@NilsIrl
Copy link

NilsIrl commented Aug 22, 2020

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

@joshtriplett
Copy link
Member

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.

@calebcartwright
Copy link
Member

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?

@joshtriplett
Copy link
Member

@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.

@calebcartwright
Copy link
Member

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.

@nazar-pc
Copy link

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
Copy link
Member

@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.

@MonliH
Copy link
Contributor

MonliH commented Oct 22, 2020

Another bug caused by this issue, that makes my editor's formatter, which uses rustfmt, anrgy (I'd imagine many more like this, it's happened to me in match statements as well):

Input:

fn main() {
    (
        foo, 
        "this is a long line that breaks rustfmt, let's make it even longer foo bar foo bar foo bar"
    )
}

Note the trailing space after foo,

Output:

error[internal]: left behind trailing whitespace
 --> /playground/src/main.rs:3:3:12
  |
3 |         foo, 
  |             ^
  |

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

Playground Link

@calebcartwright
Copy link
Member

@MonliH - that's not really a separate bug or strictly related to this issue. Your original snippet has a trailing space which is the root cause (rustfmt isn't inserting a random trailing space), rustfmt is just leaving your original snippet as is and highlighting the fact that it did not remove the trailing spaces.

@ilyvion
Copy link

ilyvion commented Jan 5, 2025

Before I make this a new issue, can someone check whether or not this issue I'm running into has the same cause as this issue? This isn't the exact code I'm having trouble with, but it behaves identically, so it's a good MRE: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f52f18c42f46824eb308281c23ed978e

If you run rustfmt on it, you'll see the code doesn't get properly formatted. In particular, ,dynamic_assets remains without a space after the comma, but also any other bad formatting in the same block remains broken if you mess it up further and try to run formatting on that.

To make rustfmt work again, simply shorten the length of the parameters in the closure. You can make it |asset: Vec<AssetServer>, dynamic: Vec<DynamicAssets>|, e.g. and it formats, or |asset_server: Vec<Asset>, dynamic_assets: Vec<Dynamic>| but it feels wrong to me that formatting entirely stops working in a whole block of code just because some names/lines end up too long.

@Sieabah
Copy link

Sieabah commented Jan 6, 2025

This was affecting me for a few days, was getting around it by just manually formatting but I was getting desperate. I installed nightly and added the extra fields which ended upbreaking a string and reflowing another expression. I've found though that if you have highly nested match/if statements the whitespace counts towards the max width. I imagine most use spaces so that's 2-4 characters wasted on each indent instead of 1 per tab. Indents also can't be "reflowed" so it creates a similar problem to strings which are >97 (accounting for the "/' and the trailing comma if used in a function argument.

I think this is an absolutely insane limitation that it doesn't warn or error at all outside of nightly. I had been trying to find any reason this wasn't compiling and it turns out this issue was exactly what I was running into the whole time.

@ilyvion Your snippet clued me into whitespace being an issue as well. For yours I'd expect the formatter to break the closure into multiple lines. You're right that this silently failing is a huge problem.

    {
        {
            {
                {
                    {
                        {
                            once::run(move |asset_server: Vec<AssetServer>,dynamic_assets: Vec<DynamicAssets>| {
                                load_dynamic_asset(
                                    &dynamic_assets,
                                    &asset_server,
                                    "dialogs",
                                    &dialog,
                                )
                                .expect(&dialog)
                            })
                        }
                    }
                }
            }
        }
    }

I'm unsure why this has been allowed to persist for 4-5 years. Especially given that because it's silent and it only ever doesn't run when it can't compile. So when it compiles and doesn't format... It's a game of whack a mole to figure out which fmt rule is breaking, installing nightly and developing within that isn't feasible since it's nightly.

@ytmimi ytmimi marked this as a duplicate of #6438 Jan 8, 2025
@ytmimi ytmimi marked this as a duplicate of #6443 Jan 13, 2025
@ytmimi ytmimi marked this as a duplicate of #6447 Jan 15, 2025
@ytmimi ytmimi marked this as a duplicate of #6455 Jan 25, 2025
@ytmimi ytmimi marked this as a duplicate of #6461 Feb 7, 2025
jhpratt added a commit to jhpratt/rust that referenced this issue Feb 11, 2025
…braheemdev

Fix long lines which rustfmt fails to format

rustfmt fails to format this match expression, because it has several long string literals over the maximum line width. This seems to exhibit rustfmt issues [rust-lang#3863](rust-lang/rustfmt#3863) (Gives up on chains if any line is too long) and [rust-lang#3156](rust-lang/rustfmt#3156) (Fail to format match arm when other arm has long line).

Format it with a large line width (e.g., by setting `max_width = 200` in rustfmt.toml) and, in case the rustfmt bugs are later fixed, mark it with `#[rustfmt::skip]`, as it is more legible with each case on one line.
jhpratt added a commit to jhpratt/rust that referenced this issue Feb 11, 2025
…braheemdev

Fix long lines which rustfmt fails to format

rustfmt fails to format this match expression, because it has several long string literals over the maximum line width. This seems to exhibit rustfmt issues [rust-lang#3863](rust-lang/rustfmt#3863) (Gives up on chains if any line is too long) and [rust-lang#3156](rust-lang/rustfmt#3156) (Fail to format match arm when other arm has long line).

Format it with a large line width (e.g., by setting `max_width = 200` in rustfmt.toml) and, in case the rustfmt bugs are later fixed, mark it with `#[rustfmt::skip]`, as it is more legible with each case on one line.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 11, 2025
Rollup merge of rust-lang#136606 - thaliaarchi:format-long-lines, r=ibraheemdev

Fix long lines which rustfmt fails to format

rustfmt fails to format this match expression, because it has several long string literals over the maximum line width. This seems to exhibit rustfmt issues [rust-lang#3863](rust-lang/rustfmt#3863) (Gives up on chains if any line is too long) and [rust-lang#3156](rust-lang/rustfmt#3156) (Fail to format match arm when other arm has long line).

Format it with a large line width (e.g., by setting `max_width = 200` in rustfmt.toml) and, in case the rustfmt bugs are later fixed, mark it with `#[rustfmt::skip]`, as it is more legible with each case on one line.
@kristoiv
Copy link

kristoiv commented Feb 23, 2025

It seems entire match statements' stop getting formatted if you have a long string-literal (eg. in a todo!(...)), and if you then happen to have a trailing white-space on the end of a list of variants as shown in my example below, then rust-fmt errors out causing the rest of the code to stop formatting too (atleast in vscode/rust-analyzer):

pub enum SomeType {
    A,
    B,
    C,
}

fn some() {
    let ct = SomeType::A;
    // "SomeType::A " <- trailing whitespace below crashes due to long string in todo!(..)
    match ct {
        SomeType::A 
        | SomeType::B => { /* safe to ignore */ }

        SomeType::C => todo!("Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin accumsan orci eget elit posuere varius."),
    }
        let _ill_formatted = 1; /* indent not fixed by rust-analyzer in vscode, but is in rs-playground */
}
error[internal]: left behind trailing whitespace
  --> /playground/src/main.rs:11:11:20
   |
11 |         SomeType::A 
   |                    ^
   |

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

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e9fed6e242d365cef07f21538724e3c4

@yasuo-ozu
Copy link

yasuo-ozu commented Feb 28, 2025

I have more simple example to cause "left behind trailing whitespace" error.

fn test() {
    // ↓ this line has trailing space
    hello.push(|()| 
        abort!("aaaaa aaaaaaa aaaaaa aaaaaaaaaa aaaaaaaaaaa aaaaaaaaaaa aaaaaaaaaaa aaaaaaaaaa aaaaaaa")
    );
}
error_on_unformatted = false
error_on_line_overflow = false
❯ rustfmt --version
rustfmt 1.8.0-nightly (00f245915b 2025-02-26)

❯ rustfmt --config-path ./rustfmt.toml test_bug2.rs
error[internal]: left behind trailing whitespace
 --> /****.rs:3:3:20
  |
3 |     hello.push(|()|
  |                    ^
  |

@lgarron
Copy link

lgarron commented Mar 23, 2025

Here's my suggestion:

Rustfmt should treat this case the same as Prettier, Biome, Black, Ruff, and I assume many other auto-formatters which I'm not familiar with. Namely, treat the max width as a target but not a strict maximum. They try their best to get lines to fit under that length, but if they can't, they can't and that's fine. They still get the line as short as possible.

I strongly agree with this.

I have spent a while being very confused about why rustfmt wasn't formatting some trailing whitespace that broke our CI for our project, as well as not formatting some other lines. Given the robustness and friendly error reporting of Rust tools, I would have expected it to format as much as possible and at least print a warning.

At the moment, it fails silently until something triggers a CI failure or an isolated line change suddenly causes an entire screenful of code to be reformatted.

Now that I know about this issue I can fix it when I see it, but I don't see a way to identify when it happens (other than when it happens to cause trailing whitespace, which is not every time). Does anyone know any arguments to rustfmt that will reveal when it runs into this bug?

@Martomate
Copy link

I just had this issue as well. I have an array which spans a lot of lines and it happens to contain some long strings, and as a result almost the entire file has no formatting. The strings contain URLs, which is why they are so long.

Here is an example:

fn f() {
    let too_long = [
        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
"b"
    ];
    let not_too_long = [
        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
"b",
    ];
}

becomes

fn f() {
    let too_long = [
        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
"b"
    ];
    let not_too_long = [
        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
        "b",
    ];
}

Is this something that is planned to be fixed, or is there some reason to keep this behaviour?

rustfmt 1.7.1-nightly

@Martomate
Copy link

Update regarding the above: apparently raw strings are allowed to be too long, so I will just convert my long strings to raw strings now.

This means that r#"aaa....aaa"# is formatted correctly even it's too long, whereas "aaa....aaa" does not.

@Martomate
Copy link

Unfortunately the solution above did not actually work in my real code. Below is a reduced example with raw strings:

fn f() {
    f1().f2(
            &[],
            f3(
                    r#"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"#,
            ),
        );
    f1().f2(
            &[],
            f3(
                    r#"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"#,
            ),
        );
}

becomes

fn f() {
    f1().f2(
            &[],
            f3(
                    r#"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"#,
            ),
        );
    f1().f2(
        &[],
        f3(r#"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"#),
    );
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-chains a-strings String literals
Projects
None yet