-
Notifications
You must be signed in to change notification settings - Fork 982
Fix rustfmt giving up on formatting when multi-line strings exceed max_width #6781
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
base: main
Are you sure you want to change the base?
Fix rustfmt giving up on formatting when multi-line strings exceed max_width #6781
Conversation
|
Just ran the Diff-Check, and its already started finding breaking formatting changes. There's no way we can move forward with the PR as is. |
|
These changes would need to be style_edition gated, but even then I'm not sure if the formatting changes that they introduce are worth it. @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
This is unfortunate. All checks passed locally so I assumed this was fine but I wasn't aware of diff-check. Thanks for looking into this. I think I'll close this PR then, but someone can probably use this as a starting point. |
|
@ytmimi just took a closer look at that check and it looks like most changes are not due to rustfmt working on those lines where it previously wasn't. 2026-01-26T21:37:12.231227Z ERROR check_diff: Diff found in 'mdBook' when formatting mdBook/tests/testsuite/markdown.rs
--- original
+++ modified
@@ -8,13 +8,15 @@
// Checks custom header id and classes.
#[test]
fn custom_header_attributes() {
- BookTest::from_dir("markdown/custom_header_attributes")
- .check_main_file("book/custom_header_attributes.html", str![[r##"
+ BookTest::from_dir("markdown/custom_header_attributes").check_main_file(
+ "book/custom_header_attributes.html",
+ str![[r##"
<h1 id="attrs"><a class="header" href="#attrs">Heading Attributes</a></h1>
<h2 class="class1 class2" id="heading-with-classes"><a class="header" href="#heading-with-classes">Heading with classes</a></h2>
<h2 id="both" class="class1 class2"><a class="header" href="#both">Heading with id and classes</a></h2>
<h2 myattr="" otherattr="value" id="myh3" class="myclass1 myclass2"><a class="header" href="#myh3">Heading with attribute</a></h2>
-"##]]);
+"##]],
+ );
}If you run rustfmt on the current code block it'd format it correctly (how the The only change I'd think is maybe undesirable is this one (and even then I'm not sure if this is a case of "rustfmt now works when previously it gave up"). 2026-01-26T21:46:34.084616Z ERROR check_diff: Diff found in 'rust' when formatting rust/compiler/rustc_errors/src/tests.rs
--- original
+++ modified
@@ -31,11 +31,13 @@
#[test]
fn wellformed_fluent() {
- let translator = make_translator("mir_build_borrow_of_moved_value = borrow of moved value
+ let translator = make_translator(
+ "mir_build_borrow_of_moved_value = borrow of moved valueWhat do you think? Update I found the source code for the last one: #[test]
fn wellformed_fluent() {
let translator = make_translator("mir_build_borrow_of_moved_value = borrow of moved value
.label = value moved into `{$name}` here
.occurs_because_label = move occurs because `{$name}` has type `{$ty}` which does not implement the `Copy` trait
.value_borrowed_label = value borrowed here after move
.suggestion = borrow this binding in the pattern to avoid moving the value");
}If you remove some chars from the 3rd line ( #[test]
fn wellformed_fluent() {
let translator = make_translator(
"mir_build_borrow_of_moved_value = borrow of moved value
.label = value moved into `{$name}` here
.occurs_because_label = move occurs because `{$name}` has type `{$ty}` which does not implement
.value_borrowed_label = value borrowed here after move
.suggestion = borrow this binding in the pattern to avoid moving the value",
);
}(I removed everything after implement to make it 100 chars total) Therefore this is indeed a case of "rustfmt now works when previously it gave up". I don't think this should be style_edition gated since the behavior is the same, it's just properly working now |
|
I haven't re-reviewed the change but as I mentioned in my earlier comment these changes would need to be style_edition gated before we could consider merging the PR. I'd also want to review the diff more carefully to see what changes are getting produced. |
would that be the case even if this is a bug fix and is just restoring expected rustfmt behavior? I think I can add a gate if that's the case |
|
Yes, there's a strong stability guarantee so these have to be gated |
Summary
Fix rustfmt giving up on formatting when multi-line strings exceed max_width.
rustfmt would refuse to format code containing multi-line string literals that exceed
max_width. The problem occurred becausefiltered_str_fits()insrc/utils.rswould reject valid formatting attempts when string content (which cannot be shortened) exceeded width limits.Added test fixtures for both issues:
tests/source/issue-6769.rs/tests/target/issue-6769.rs- byte string literals in array tuplestests/source/issue-6778.rs/tests/target/issue-6778.rs- multi-line strings in struct fieldsReferences
Note that reverting commit fa5b1e4 "breaks" a bunch of fixtures for the better, allowing formatting of rust code even after very long strings.
Example:
Mismatch at tests/source/string_punctuation.rs:5: "ThisIsAReallyLongStringWithNoSpaces.It_should_prefer_to_break_onpunctuation:\ Likethisssssssssssss" ); - format!("{}__{}__{}ItShouldOnlyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyNoticeSemicolonsPeriodsColonsAndCommasAndResortToMid-CharBreaksAfterPunctuation{}{}",x,y,z,a,b); + format!( + "{}__{}__{}ItShouldOnlyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyNoticeSemicolonsPeriodsColonsAndCommasAndResortToMid-CharBreaksAfterPunctuation{}{}", + x, y, z, a, b + );and
Mismatch at tests/source/issue-2179/one.rs:27: } else { for package in &opts.package { if let None = ws.members().find(|x| x.name() == package) { - warn!("cargo - couldn't find member package `{}` specified in `analyze_package` configuration", package); + warn!( + "cargo - couldn't find member package `{}` specified in `analyze_package` configuration", + package + ); } } }It does, however, break
tests/source/issue-1210/c.rsin a not so desirable way:I'd encourage someone to take a look at this and follow up if possible.
Note: This PR was generated with assistance from an LLM (Claude). While I'm happy to address minor feedback, I may not have bandwidth to drive major revisions or extensive back-and-forth due to time constraints. If this approach looks promising but needs significant changes, feel free to take over the branch or use this as a starting point. I mainly wanted to get a potential fix out there since these issues were affecting my workflow but since the issue was actually fixed I thought it was worth sharing these changes.