imports_granularity=Item repeats rustdoc comments

[vgaspary]

Formatting this

/// rustdoc comment
use a::{a, b, c};

with imports_granularity = "Item" results in this:

/// rustdoc comment
use a::a;
/// rustdoc comment
use a::b;
/// rustdoc comment
use a::c;

The rustdoc comment should appear once.

[ytmimi]

How is rustfmt supposed to know which item the doc comment goes with?

[vgaspary]

@ytmimi , my fix leaves the rustdoc comment with the first import statement. I think this is a better default at least.

If the comment was about the whole group, the behavior will be expected, since the new, flattened items end up being "grouped" and the comment is on top.

If the comment was about a specific item (or a subset) in the import tree, this behavior is still expected, since rustfmt couldn't magically determine that. The user has to adjust it manually.

[ytmimi]

Brainstorming here; Maybe rustfmt shouldn't separated the import list if there's a doc comment above the entire group. That way it's completely up to the user to remove or move the doc comment before rustfmt can break it up using imports_granularity = "Item".

Trying to apply the doc comment only to the first item seems sensible, but it won't always be the correct thing to do, and I think it makes sense to have the developer step in here.

[ytmimi]

I'd also argue that in this example, the attribute needs to be applied to all items if we break it up:

#[cfg(target_os = "linux")]
use a::{a, b, c};

Formatting with imports_granularity = "Item"

#[cfg(target_os = "linux")]
use a::a;
#[cfg(target_os = "linux")]
use a::b;
#[cfg(target_os = "linux")]
use a::c;

[vgaspary]

I'd also argue that in this example, the attribute needs to be applied to all items if we break it up:

Yes, and my fix makes sure of that. I'll extend the test to make it explicit.

I don't know what rustc_ast::ast::Attribute::is_doc_comment() returns for an attribute like this - #[doc = ""], we'll find out soon.

[vgaspary]

Maybe rustfmt shouldn't separated the import list if there's a doc comment above the entire group.

I agree, doing nothing is also a valid behavior. It's the safe but not helpful approach. With my proposal it might do the wrong thing, as it does today. If we decide that rustfmt is going to be conservative, it should do nothing in case of any attributes (not just rustdoc comments).

But I'd like my rustfmt to do its best in this particular case. In vast majority of cases users would want the rustdoc comment to appear once and the attributes be repeated. Both of these might be wrong, but those are rare and non obvious. rustfmt not doing the "obvious" thing in 99% cases looks like a bug. rustfmt doing the wrong thing in 1% of edge cases doesn't look like a bug.

[ytmimi]

I agree, doing nothing is also a valid behavior. It's the safe but not helpful approach.

Personally, if there's ambiguity then I'd prefer rustfmt does nothing.

If we decide that rustfmt is going to be conservative, it should do nothing in case of any attributes (not just rustdoc comments).

My understanding is that attributes apply to each import in the list so it makes sense that we'd duplicate them for each element when using imports_granularity = "Item".

I'm curious to know how rustdoc handles /// doc comments and #[doc="attributes] on an import list? Does it apply the same doc comment to each element? If so, then the current behavior is probably correct.

[vgaspary]

Personally, if there's ambiguity then I'd prefer rustfmt does nothing.

That means it should also do nothing with attributes, all kinds of attributes.

I'm curious to know how rustdoc handles /// doc comments and #[doc="attributes] on an import list? Does it apply the same doc comment to each element? If so, then the current behavior is probably correct.

#[doc = "..."] is not considered a doc comment, rustc_ast::ast::Attribute::is_doc_comment() is false, so with or without my fix it gets repeated for all items.

My understanding is that attributes apply to each import in the list so it makes sense that we'd duplicate them for each element when using imports_granularity = "Item".

My latest finding about #[doc ...] attribute proves that not all attributes apply to all imports.