Fix doc of generic items formmating error by frank-king · Pull Request #5124 · r...

Copy link

Contributor

calebcartwright commented on Dec 4, 2021 •

TBH I've personally always found the usage of doc comments within generics as odd, but agreed we don't want to have the current rustfmt behavior.

The test failures look legitimate so you'll need to address when you get a chance. This doesn't seem like something that should be tracked globally on the rewrite context though, and which should be handled as part of the processing of each param.

frank-king commented on Dec 5, 2021 •

@calebcartwright Thanks for you review.

I used doc comments in generic params where there are a few const params representing some configs. Yet in stable rust, I cannot put consts with user-defined types into generics, and instead I used separate const bool or int types as the generic params, which may help the compiler opt out some if or match branches.

These const params in generics, I think, is necessary to have some doc comments for their usages.

BTW, for this PR, there is still a unresolved problem, that I have precluded the duplicated doc comments of const generic params before rewriting pre-comments of the ListItem, but I'm afraid it might not be the root cause.

I could not figure out why rustfmt regard the doc comments (or other attributes) as its pre-comments. The only thing I have found was, that for a const generic param, (self.get_lo)(&item) returns the byte position at the const keyword, excluding all of its attributes. However, for a type param or a lifetime param, their spans both include attributes.

Here self.get_lo is provided by ast::GenericParam::span.

Then I read the code in ast::GerericParam::span:

https://github.com/rust-lang/rust/blob/5e93f6e318e687c05c8c44517de4586ed75ce3f4/compiler/rustc_ast/src/ast.rs#L409-L420

None of them include the attributes. Are there other places where the spans of OverflowableItem are reassigned?

Thanks for the follow up. Wanted to acknowledge having seen the update and that it's on my list to circle back to, though going to be a bit busy for the next week or two so it may be a little while

Thanks again, and sorry for the delay @frank-king !

I could not figure out why rustfmt regard the doc comments (or other attributes) as its pre-comments. The only thing I have found was, that for a const generic param, (self.get_lo)(&item) returns the byte position at the const keyword, excluding all of its attributes. However, for a type param or a lifetime param, their spans both include attributes.

None of them include the attributes. Are there other places where the spans of OverflowableItem are reassigned?

You've definitely found an existing issue while working on the original one, and that stems from the Spanned impl for GenericSpan

You can find (and if you're still willing) fix that here:

https://github.com/rust-lang/rustfmt/blob/master/src/spanned.rs#L114-L127

Where we basically need to account for the presence of attributes what that kind variant too. Also, feel free to adjust the assignment of lo as I suspect a match or something else may be a bit easier to read given the needed update.

The other change you have is fine, so once the span issue is adjusted this should be good to go. The last thing to note is that there's no need to include both tests/source/... and tests/target/... files when they are identical, so you can either drop the source file altogether or adjust it/make it misformatted if you'd like to keep it to show reformatting occurring without butchering due to the doc comments

frank-king commented 16 days ago •

@calebcartwright I have revised the impl Spanned for ast::GenericParam as your suggestions, and now there is another problem remained. After formatted, this code

// Non-doc pre-comment of Foo
/// doc of Foo
// Non-doc post-comment of Foo
struct Foo<
    // Non-doc pre-comment of 'a
    /// doc of 'a
    // Non-doc post-comment of 'a
    'a,
    // Non-doc pre-comment of T
    /// doc of T
    // Non-doc post-comment of T
    T,
    // Non-doc pre-comment of N
    /// doc of N
    // Non-doc post-comment of N
    const N: item,
>;

becomes

// Non-doc pre-comment of Foo
/// doc of Foo
// Non-doc post-comment of Foo
struct Foo<
    // Non-doc pre-comment of 'a
    /// doc of 'a
    'a,
    // Non-doc pre-comment of T
    /// doc of T
    T,
    // Non-doc pre-comment of N
    /// doc of N
    const N: item,
>;

I didn't include the post-comments (after their doc-comments) of the generic items because of the above reason.

I think this might have lower priority to fix, since only part of the comments are affected, is it?

now there is another problem remained
I didn't include the post-comments (after their doc-comments) of the generic items because of the above reason.

I think this might have lower priority to fix, since only part of the comments are affected, is it?

Yikes. This is a bit of a mess isn't it. These issues already exist so I don't think we need to block the changes here which do provide some fixes and improvements.

If you'd be willing and interested in working on the post comment fix in a follow up PR that'd be most appreciated!