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

RustFmt fails to format file with long line within scope of lambda passed to member function #3135

Closed
VeaaC opened this issue Oct 25, 2018 · 17 comments

Comments

@VeaaC
Copy link

VeaaC commented Oct 25, 2018

fn f() {
    something.do_stuff(|| {
                {
                    "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
                }
            }   );
}

I tried to reduce it more, but any change caused it to work again.

@Elarnon
Copy link

Elarnon commented Oct 26, 2018

I hit this today, so here is another example with more info. Notes from my experiments:

  • Rustfmt only fails to format the block containing the long line
  • There needs to be a method call. If calling bar(..) instead of foo.bar(..) things work as expected. Using Foo::bar is a workaround.
  • There needs to be a block inside the method call (so you need foo.bar({ .. }), not just foo.bar(..)).
  • The "long token" doesn't need to be a string, but it needs to be inside some other expression. Curly brackets {xxx} or a function call f(xxx) both trigger the bug; a tuple (xxx, ) does not.
fn main() {
    rustfmt.will(properly + reformat).this();
    foo.bar({
rustfmt        .
will(NOT
+
touch
                        )
.this()
;
{"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"}
    });
}

Edit: By "Using Foo::bar is a workaround", I mean that you can get the right formatting by temporarily using foo::bar instead of foo.bar, running rustfmt, then reverting back to foo.bar. Since rustfmt won't touch the code in the foo.bar case you get to keep the good formatting even after changing back to foo.bar.

@nrc
Copy link
Member

nrc commented Nov 1, 2018

There is no good formatting here because the string lit is too long for the max width. SO I think in this case, doing nothing and let the user fix it is the right thing to do.

@VeaaC
Copy link
Author

VeaaC commented Nov 1, 2018

I would agree about the line in question, but it breaks all the blocks it is in, e.g. in my use case a few hundred lines, and without indication which line is responsible.

Also: It also breaks if the long line consists of symbols that are possible to line-break properly (e.g. I had a longer macro invocation in there, not a long string)

@scampi
Copy link
Contributor

scampi commented Nov 1, 2018

@VeaaC Can you share an example of that long line with a macro ? thanks!

@scampi
Copy link
Contributor

scampi commented Nov 1, 2018

@Elarnon do you have an example of the following ? I tried to replace the curly brackets around the long string from your comment with a tuple but it didn't get formatted either.

The "long token" doesn't need to be a string, but it needs to be inside some other expression. Curly brackets {xxx} or a function call f(xxx) both trigger the bug; a tuple (xxx, ) does not.

@Elarnon
Copy link

Elarnon commented Nov 1, 2018

@scampi Indeed, I checked again and it does trigger the bug as well. I think I got confused because the behavior around tuples and whitespaces is really weird:

fn main() {
    foo.bar({
underindented();
{(  xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx,)}
    });
}

triggers the bug, but if you do any of the following, it no longer triggers:

  • Remove one x
  • Remove one space before the xs
  • Remove the comma
  • Replace the comma with an x
  • Put the spaces before the opening paren ({ (x...).

BTW I will reiterate that rustfmt works properly with :: instead of .:

fn main() {
    foo::bar({
underindented();
{"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"}
    });
}

gets formatted into

fn main() {
    foo::bar({
        underindented();
        {
            "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
        }
    });
}

which is arguably the "least bad formatting", even if no "good" ones exist.

@VeaaC
Copy link
Author

VeaaC commented Nov 2, 2018

This one fails to split a macro (instead of the long string):

fn f() {
    something.do_stuff
    (
    |
    | 
    {
        error!(context.logger, "metric_production_abcdefgere"; "reason_abcdefg" => format!("{}", e));
    }
    );
}

Making the line any shorter will format the rest of the code normally, but still not split the macro:

fn f() {
    something.do_stuff(|| {
        error!(context.logger, "metric_production_abcdefgere"; "reason_abcde" => format!("{}", e));
    });
}

@shepmaster
Copy link
Member

Is this the same root cause?

fn x() {
    let near : Vec<u8> = foo("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
}

Rustfmt 1.0.0-nightly (2018-11-30 43206f4) reports no errors or warnings, the exit code is zero, and the file is not changed:

$ cat original
fn x() {
    let near : Vec<u8> = foo("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
}
$ cp original fmt
$ rustup run nightly rustfmt fmt
$ echo $?
0
$ diff original fmt
$ rustup run nightly rustfmt --version
rustfmt 1.0.0-nightly (43206f41 2018-11-30)

@shika-blyat
Copy link

I faced this issue today, and it tooks me 5 hours to understand the problem was coming from my string which was too long, but rustfmt gave me no warning nor error.
Considering adding a warning/an hard error for such cases instead would be feasible ?

@asv7c2
Copy link

asv7c2 commented Jun 6, 2020

What's the status of this bug? Will it be fixed?

@topecongiro topecongiro added a-chains a-strings String literals labels Jun 29, 2020
@topecongiro topecongiro added this to the 3.0.0 milestone Jun 29, 2020
@timohanke
Copy link

Here's another example of something passing through rustfmt despite underindentation. This example may be sufficiently close to the ones above that it is not worth mentioning. But, however, two things are noticeable: 1) Replacing .bar with ::bar does not fix the issue. 2) Adding a trailing white space at the end of line 2 (after foo) causes rustfmt to terminate with an internal error. (rustfmt 1.4.17-nightly ( ))

fn f() {
    foo
.bar("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
}

@ted-tanner
Copy link

I spent several hours last night trying to figure out why rustfmt wasn't working for a single file in a project. Revisited it this morning and found this issue.

Yes, the static string slice hardcoded into the module was too long to be properly formatted and probably should have been shorter. That is no excuse for why rustfmt wouldn't format anything in the entire module (and was completely silent about it). The expected outcome of running rustfmt on a file containing a long string is that the string itself doesn't get touched (other than to move it to a sensible position) but everything else in the file gets formatted as per usual. A warning about the length of the string would also help.

@ted-tanner
Copy link

ted-tanner commented Dec 31, 2021

I found this unit test in lib.rs (line 611 in commit 4a053f2).

#[test]
fn test_format_code_block_fail() {
    #[rustfmt::skip]
    let code_block = "this_line_is_100_characters_long_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx(x, y, z);";
    assert!(format_code_block(code_block, &Config::default(), false).is_none());
}

From format_code_block (line 406 in commit 4a053f2):

else if line.len() > config.max_width() {
    // If there are lines that are larger than max width, we cannot tell
    // whether we have succeeded but have some comments or strings that
    // are too long, or we have failed to format code block. We will be
    // conservative and just return `None` in this case.
    return None;
}

@calebcartwright
Copy link
Member

I realize this issue has been open longer, but I'm going to close in favor of #3863 because they're duplicative reports but the latter has more focused/actionable discussion.

That thread covers the expected/default behavior, existing workarounds and config options, options you'd need to enable to receive warnings/errors, and request for feedback on some new options under consideration.

For the non-chain related snippets that were tacked on to this thread:

  • singular calls (including macro calls) can set version=Two in their config files if they want those to be formatted
  • macro calls can really only be formatted if all of their args are valid rust code; otherwise the calls are mostly left as-is

@calebcartwright
Copy link
Member

@ted-tanner I appreciate that you're frustrated, but want to note that expressing that frustration across multiple issues doesn't really help move anything forward.

That is no excuse for why rustfmt wouldn't format anything in the entire module

This doesn't sound right. Are you sure you're running rustfmt or cargo fmt directly, as opposed to editor/IDE driven formatting? I don't want to repeat what's already been shared above and in the linked issue, but the main behavior of not reformatting the chain with long content is specific to the chain itself; rustfmt will absolutely format other content. Please note that certain editor/IDE plugins opted to not format anything in the file in these cases, but that's a decision those unrelated solutions made and not anything rustfmt controls.

You can see rustfmt's behavior in action via this simple playground snippet. If you're still seeing the behavior you described then please open a new issue with a complete MCVE that can be reproduced.

Considering adding a warning/an hard error for such cases instead would be feasible ?
(and was completely silent about it)
A warning about the length of the string would also help.

Already covered in #3863 (comment), tl;dr the ability to do so exists but it's turned off by default.

@calebcartwright calebcartwright removed this from the 3.0.0 milestone Dec 31, 2021
@ted-tanner
Copy link

ted-tanner commented Dec 31, 2021

@calebcartwright I apologize for my poor behavior. It was wrong of me to express my frustration here. Thank you for the gracious manner in which you responded to my not-so-gracious comment. Also, thanks for your hard work on rustfmt that has immensely benefited me and thousands of others.

I'm not sure why the rest of the module was not being formatted (it was actually the rest of the file, not the module--my tests in a separate module in the same file also were not being formatted using the command-line, not an editor plugin), but with an adjustment to a long string it is being formatted correctly now.

I'll leave my comment on this issue unedited as an example of how not to post comments.

@calebcartwright
Copy link
Member

No worries, we've all been there 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests