Prevent duplicating comments and tokens when rewriting trait headers by ytmimi · Pull Request #6834 · rust-lang/rustfmt

ytmimi · 2026-03-20T21:19:58Z

Expand the HeaderPart struct to handle some new header parts like, const, unsafe, and auto and refactor trait header formatting to use HeaderPart.

r? @fee1-dead since you originally added HeaderPart

matthewhughes934 · 2026-03-22T14:58:55Z

src/header.rs

+        Self::new("", DUMMY_SP)
+    }
+
+    /// Find the `keyword` for the header within the give `span`


I'm a bit confused by this doc comment: is this function finding a keyword, or setting/creating one? (extra context: I'm not too familiar with how Spans work, so I may just be completely misunderstanding)

A span refers to a byte range in the source code, and each AST node has a span that denotes where it begins and ends. So we're actually searching for the keyword within the span and finding out what it's sub-span is.

With that in mind, do you have any suggestions to make that clearer?

A span refers to a byte range in the source code, and each AST node has a span that denotes where it begins and ends. So we're actually searching for the keyword within the span and finding out what it's sub-span is.

Thanks! I only just now realised span was added as part of this changeset 🤦

With that in mind, do you have any suggestions to make that clearer?

Maybe the method could be span_at or span_on following the pattern of span_before/span_after?

span_at Sounds good to me. I'll make that update.

I think the entire set of method names is not so great. I would support the following set of naming:

subspan - currently named as span

subspan_from - currently named as span_after

subspan_until - currently named as span_before

Appreciate other options as well!

on second glance, after seems better than from, since from appears to include the given needle. Hmm I will have to think more on this.

@fee1-dead i agree. I think renaming those methods would be ideal, though I'm not sure that should happen in this PR. I'd say span_before and span_after should probably be renamed to pos_before and pos_after since they return BytePos objects instead of Spans

I can open up a follow up PR with any renames based on what we decide to move forward with

renamed span -> subspan

matthewhughes934 · 2026-03-22T22:59:56Z

I think comments between the literal trait and the identity will now be removed:

$ echo 'pub /* first */ trait /* second */ MyTrait /* third */ {}' | cargo run --bin rustfmt --quiet
pub /* first */ trait MyTrait /* third */ {}

Because that second comment is now in a gap not covered by this change:

The first comment is captured by format_header
The third comment is captured by the updated else block in this change
But the second comment (covered by the old 'else' block) is now skipped

src/header.rs

fee1-dead · 2026-03-23T00:54:53Z

src/utils.rs

+// FIXME(ytmimi) might want to get rid of `format_auto` since we now have `HeaderPart::auto`
+#[allow(unused)]


I would encourage you to remove this in this PR :) of course it will still be stored in git history, but cleanup would be nice!

ytmimi · 2026-03-23T16:21:14Z

I think comments between the literal trait and the identity will now be removed:
$ echo 'pub /* first */ trait /* second */ MyTrait /* third */ {}' | cargo run --bin rustfmt --quiet
pub /* first */ trait MyTrait /* third */ {}
Because that second comment is now in a gap not covered by this change:

The first comment is captured by format_header

The third comment is captured by the updated else block in this change

But the second comment (covered by the old 'else' block) is now skipped

Will Look into this case

ytmimi · 2026-03-23T23:49:52Z

I think comments between the literal trait and the identity will now be removed:
$ echo 'pub /* first */ trait /* second */ MyTrait /* third */ {}' | cargo run --bin rustfmt --quiet
pub /* first */ trait MyTrait /* third */ {}
Because that second comment is now in a gap not covered by this change:

The first comment is captured by format_header

The third comment is captured by the updated else block in this change

But the second comment (covered by the old 'else' block) is now skipped
Will Look into this case

@matthewhughes934 I'm not sure your example currently works with the latest main: rustfmt 1.9.0-nightly (bdab101 2026-03-20)

Running your example produces invalid code.

pub trait MyTrait /* first */ trait /* second */ MyTrait /* third */ {}

ytmimi · 2026-03-24T01:06:02Z

I think comments between the literal trait and the identity will now be removed:
$ echo 'pub /* first */ trait /* second */ MyTrait /* third */ {}' | cargo run --bin rustfmt --quiet
pub /* first */ trait MyTrait /* third */ {}
Because that second comment is now in a gap not covered by this change:

The first comment is captured by format_header

The third comment is captured by the updated else block in this change

But the second comment (covered by the old 'else' block) is now skipped
Will Look into this case
@matthewhughes934 I'm not sure your example currently works with the latest main: rustfmt 1.9.0-nightly (bdab101 2026-03-20)

Running your example produces invalid code.
pub trait MyTrait /* first */ trait /* second */ MyTrait /* third */ {}

Okay, the latest commit addresses this case.

fee1-dead · 2026-03-26T14:45:29Z

src/items.rs

+    result = combine_strs_with_missing_comments(
+        context,
+        &result,
+        &generics_str,
+        comment_span,
+        // infinite_width so we don't put the `ident + generics_str` on the next line
+        shape.infinite_width(),
+        true,
+    )?;


In the future I might want to make the headers mechanism to also do this part of the job too.

I was hoping to add the generics_str code to the HeaderPart vec as well to simplify things, but because format_header always uses shape.infinite_width() generic's weren't getting properly wrapped.

Maybe updating HeaderPart to something like this in the future is something we could consider:

pub(crate) struct HeaderPart<'a> { kind: HeaderPartKind<'a>, span: Span, } #[derive(Debug)] enum HeaderPartKind<'a> { /// snippet of this part without surrounding space Snippet(Cow<'a, str>), /// A closure that will be used to dynamically compute the header part Computed(fn (&RewriteContext<'_>, Shape) -> RewriteResult) }

Then we could modify format_header to dynamically compute any HeaderPartKind::Computed header parts with an appropriate shape offset.

fee1-dead · 2026-03-26T14:46:31Z

tests/source/trait.rs

+pub /* first */ trait /* second */ MyTrait /* third */ {}
+


Please also add a test for something like this:

pub /* a */ trait /* b */ Foo /* c */ {}

Just to ensure that we don't accidentally format the comments (aligning them etc?).

So I think after my own testing and looking at the original issue, it looks like pub /* a */ trait had the issue, but it looks like latest rustfmt also chokes on this one:

trait /* a */ X {}

which gets reformatted to

trait X /* a */ X {}

but in here, it gets re-aligned (thus formatted):

trait /* b */ Foo

My recollection of the discussion at #5847 (comment) is that we don't want to introduce comment formatting for now.

I'm obviously for us being bold and all, and I disagree with postponing these things because I doubt it will get done in a reasonable time. I also don't think rustfmt breakages should be done using "editions", it should just be through a normal upgrade process (remind me some months later to look into whether rustup can pin rustfmt versions, whether via rust-toolchain.toml or something else).

But I just wanted to let you know in case you wanted to be consistent with the original decision (which applied here would require you to preserve the original comment formatting), and note that trait Foo /* a */ {} would be fine because that behavior is pre-existing.

Otherwise, feel free to r=me.

rustbot · 2026-03-27T02:48:27Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Helper method to get the `Span` of a known substring within a span.

Simplify macro header formatting by using the new `HeaderPart::keyword` associated function.

Keep track of the `lo` span when rewriting parts the header so that we can correctly recover comments between the last rewritten part of the trait definition and the body of the trait.

Using `HeaderPart::auto` for formatting instead.

fee1-dead · 2026-03-29T03:55:23Z

Marking as waiting on author per latest comment :)

rustbot added the S-waiting-on-review Status: awaiting review from the assignee but also interested parties. label Mar 20, 2026

rustbot assigned fee1-dead Mar 20, 2026

ytmimi force-pushed the issue_6160 branch from 33800a7 to 58c0bec Compare March 20, 2026 21:24

matthewhughes934 reviewed Mar 22, 2026

View reviewed changes

fee1-dead reviewed Mar 23, 2026

View reviewed changes

src/header.rs Outdated Show resolved Hide resolved

fee1-dead reviewed Mar 23, 2026

View reviewed changes

ytmimi force-pushed the issue_6160 branch from 58c0bec to 3d8117a Compare March 23, 2026 17:01

This comment has been minimized.

Sign in to view

fee1-dead reviewed Mar 26, 2026

View reviewed changes

ytmimi force-pushed the issue_6160 branch from ae7b41a to fa2a273 Compare March 27, 2026 02:48

ytmimi added 12 commits March 26, 2026 23:35

feat: Add subspan helper method to SpanUtil

46a7452

Helper method to get the `Span` of a known substring within a span.

refactor: Add HeaderPart::keyword helper and refactor macro formatting

c2cfb41

Simplify macro header formatting by using the new `HeaderPart::keyword` associated function.

feat: add HeaderPart methods to format constness, safety, and auto

2a42154

refactor: use HeaderPart to simplify trait header formatting

6b3dcf4

fix: prevent duplicating comments and tokens in trait header

0634576

Keep track of the `lo` span when rewriting parts the header so that we can correctly recover comments between the last rewritten part of the trait definition and the body of the trait.

test: Add formatting test case for issue 6160

3394929

test: add test case for issue 5841

af27f2c

test: Add more trait header test cases with comments

b46276f

chore: remove unused crate::utils::format_auto

59105d2

Using `HeaderPart::auto` for formatting instead.

chore: fix typo give -> given

e22cd2b

fix: recover comments between trait keyword and ident

df00702

test: add test case with indented trait header comments

f0db1f8

ytmimi force-pushed the issue_6160 branch from fa2a273 to f0db1f8 Compare March 27, 2026 03:36

fee1-dead 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 29, 2026

		// FIXME(ytmimi) might want to get rid of `format_auto` since we now have `HeaderPart::auto`
		#[allow(unused)]

Conversation

ytmimi commented Mar 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ytmimi commented Mar 23, 2026

Uh oh!

This comment has been minimized.

ytmimi commented Mar 23, 2026

Uh oh!

ytmimi commented Mar 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Mar 27, 2026

Uh oh!

fee1-dead commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

matthewhughes934 commented Mar 22, 2026 •

edited

Loading

fee1-dead commented Mar 29, 2026 •

edited

Loading