remove trailing whitespace from multi-line tuple struct field prefix by ytmimi · Pull Request #5708 · rust-lang/rustfmt

ytmimi · 2023-03-07T05:07:27Z

visibility modifiers always contain a trailing space after them. If the formatted tuple field needs to be written over multiple lines then the extra space will cause issues.

In the best case the space will offset the type name by an extra space and in the worst case it will lead to a "left behind trailing whitespace" error.

These changes were version gated because they cause breaking formatting changes with existing tests.

r? @calebcartwright

ytmimi · 2023-03-07T05:08:35Z

Here are the relevant differences between the v1 and the newly added v2 tests:
git diff --no-index tests/target/struct_field_doc_comment.rs tests/target/struct_field_doc_comment_v2.rs

 struct MyTuple(
     #[cfg(unix)] // some comment
-    pub  u64,
-    #[cfg(not(unix))] /*block comment */ pub(crate)  u32,
+    pub u64,
+    #[cfg(not(unix))] /*block comment */ pub(crate) u32,
 );
 
 struct MyTuple(
     /// Doc Comments
     /* TODO note to add more to Doc Comments */
-    pub  u32,
+    pub u32,
     /// Doc Comments
     // TODO note
-    pub(crate)  u64,
+    pub(crate) u64,
 );

ytmimi · 2023-03-07T05:19:13Z

Just ran the Diff Check Job, and everything looks good ✅

kevinji · 2023-07-30T06:15:03Z

+    while context.config.version() == Version::Two && prefix.ends_with(char::is_whitespace) {
+        // Remove any additional whitespace at the end of the prefix.
+        // For example if there is a space after a visibility modifier.
+        prefix.pop();
+    }


I think this is more straightforward:

Suggested change

while context.config.version() == Version::Two && prefix.ends_with(char::is_whitespace) {

// Remove any additional whitespace at the end of the prefix.

// For example if there is a space after a visibility modifier.

prefix.pop();

}

if context.config.version() == Version::Two {

// Remove trailing whitespace after the prefix, such as a visibility modifier.

prefix.truncate(prefix.trim_end().len());

}

Thanks for helping to simplify the implementation

calebcartwright · 2023-08-13T19:58:15Z

I'd be content merging this as-is because it cleanly fixes an obvious and annoying issue (and I'll add I hate we have to gate this, but agree that we unfortunately must).

However, it's typically a smell for me when we have to remove whitespace that we previously added farther back in the call stack. Do you think it would be feasible to apply a different fix that only adds the whitespace conditionally when necessary? And if so, is that something you think we could turn around quickly or would it be better to move forward with this fix and add a tracking issue and/or fixme comment to try that alternative approach?

ytmimi · 2023-08-13T23:31:12Z

I'll have to revisit this. I can take a look to see what's going on in rewrite_struct_field_prefix to see if we can prevent emitting the trailing space. I imagine that change would touch more files, but I also agree that it would be better not to emit the trailing space in the first place.

ytmimi · 2023-08-31T04:06:40Z

@calebcartwright check out the latest commit. It removes the trailing whitespace when using Version::Two. The main downside is that we need to make several changes in a lot of different places. Let me know if you think this updated change is better and I'll squash the commits.

ytmimi · 2023-08-31T04:09:11Z

+        // format_visibility doesn't have a trailing space in Version::Two
+        result.push_str(&visibility);
+    } else {
+        result.push_str(visibility.trim());


It turns out that there were other places in the codebase that already dealt with trimming the trailing space coming back from format_visibility.

Hm, it feels like a footgun that format_visibility also attaches trailing whitespaces.

Fixes 5703, Fixes 5525 visibility modifiers always contain a trailing space after them. If the formatted tuple field needs to be written over multiple lines then the extra space will cause issues. In the best case the space will offset the type name by an extra space and in the worst case it will lead to a "left behind trailing whitespace" error.

jieyouxu

(Excuse the review 3 years later 🦆)

I think the approach taken in this PR is reasonable enough for a fix that IMO is worth landing. A separate follow-up investigation on the handling of format_visibility is likely warranted though, that it attaches a trailing space I agree feels quite sus.

(Though this PR will need a... rebase)
@rustbot author

View changes since this review

jieyouxu · 2026-03-03T14:51:02Z

@@ -0,0 +1,9 @@
+// rustfmt-version: Two


Suggestion: // rustfmt-style_edition: 2027

(Since style edition 2024 is already stable?)

Likewise in other tests.

jieyouxu · 2026-03-03T14:53:20Z

+        if !vis.is_empty() && context.config.version() == Version::Two {
+            vis += " ";
+        }


Suggestion: && context.config.style_edition() >= StyleEdition::Edition2027 I think

jieyouxu · 2026-03-03T14:55:41Z

+        if !vis.is_empty() && context.config.version() == Version::Two {
+            // format_visibility doesn't have a trailing space in Version::Two
+            result.push(' ');
+        }


Suggestion:

!vis.is_empty() && context.config.version() == Version::Two -> !vis.is_empty() && context.config.style_edition() >= StyleEdition::2027

// format_visibility doesn't have a trailing space in Version::Two -> // format_visibility doesn't have a trailing space in StyleEdition::2027

and elsewhere

jieyouxu · 2026-03-03T14:59:13Z

+        // format_visibility doesn't have a trailing space in Version::Two
+        result.push_str(&visibility);
+    } else {
+        result.push_str(visibility.trim());


Hm, it feels like a footgun that format_visibility also attaches trailing whitespaces.

rustbot · 2026-03-03T15:02:28Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

rustbot · 2026-03-13T15:32:57Z

☔ The latest upstream changes (possibly #6823) made this pull request unmergeable. Please resolve the merge conflicts.

ytmimi added the pr-not-reviewed label Mar 7, 2023

ytmimi requested a review from calebcartwright March 7, 2023 05:07

daprilik mentioned this pull request May 4, 2023

Extra whitespace in nested pub extern #5525

Open

kevinji reviewed Jul 30, 2023

View reviewed changes

ytmimi force-pushed the issue_5703 branch from eaa8e44 to 39387c8 Compare August 7, 2023 13:04

calebcartwright added pr-waiting-on-author and removed pr-not-reviewed labels Aug 13, 2023

ytmimi force-pushed the issue_5703 branch from 39387c8 to e040d96 Compare August 31, 2023 04:04

ytmimi commented Aug 31, 2023

View reviewed changes

ytmimi mentioned this pull request Sep 1, 2023

Initial work on formatting headers #5847

Merged

ytmimi added 3 commits December 28, 2023 21:10

Prevent format_visibility from adding trailing space with Version::Two

09dbeda

Add test case for issue 5997

7894f70

ytmimi mentioned this pull request Dec 29, 2023

Double Space between pub and type in newtype when an attribute is followed by a comment #5997

Open

ytmimi force-pushed the issue_5703 branch from e040d96 to 7894f70 Compare December 29, 2023 02:26

ytmimi added pr-follow-up-review-pending and removed pr-waiting-on-author labels Dec 29, 2023

jieyouxu added the S-waiting-on-review Status: awaiting review from the assignee but also interested parties. label Feb 23, 2026

jieyouxu reviewed Mar 3, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Mar 3, 2026

jieyouxu added A-2027-style-edition Area: style edition 2027 F-impacts-stable-formatted-code Expected formatting impact: affects stable formatted code (caution) F-impacts-stable-default-format Expected formatting impact: affects stable default format configuration (caution) labels Mar 3, 2026

jieyouxu added the F-requires-next-style-edition Expected formatting impact: can only be done over a style edition (caution) label Mar 3, 2026

ytmimi mentioned this pull request May 2, 2026

fix: trim trailing whitespace from lhs when rhs wraps to new line #6881

Open

Uh oh!

Conversation

ytmimi commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ytmimi commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ytmimi commented Mar 7, 2023

Uh oh!

kevinji Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ytmimi Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

calebcartwright commented Aug 13, 2023

Uh oh!

ytmimi commented Aug 13, 2023

Uh oh!

ytmimi commented Aug 31, 2023

Uh oh!

ytmimi Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

jieyouxu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

jieyouxu Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

rustbot commented Mar 3, 2026

Uh oh!

rustbot commented Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ytmimi commented Mar 7, 2023 •

edited

Loading

ytmimi commented Mar 7, 2023 •

edited

Loading

kevinji Jul 30, 2023 •

edited

Loading

jieyouxu left a comment •

edited

Loading

jieyouxu Mar 3, 2026 •

edited

Loading