Poor formatting for function with attribute and doc comment

[cdisselkoen]

rustfmt un-indents the attribute in this small example. Interestingly, if either the doc comment or the inline comment are removed, rustfmt lets the attribute stay indented, which I believe to be the intuitive behavior.

Input

pub struct Struct {}

impl Struct {
    /// Documentation for `foo`
    #[rustfmt::skip] // comment on why use a skip here
    pub fn foo(&self) -> usize {
        0
    }
}

Output

pub struct Struct {}

impl Struct {
    /// Documentation for `foo`
#[rustfmt::skip] // comment on why use a skip here
    pub fn foo(&self) -> usize {
        0
    }
}

Expected output

(no change)

Meta

• rustfmt version: Observed this behavior on both 1.4.17-stable (2020-07-20) and 1.4.20-nightly (2020-08-09)
• From where did you install rustfmt?: rustup

[cdisselkoen]

I should mention that I can't easily reproduce this with other kinds of attributes, such as #[cfg()], #[allow()], #[cold], etc. So this may be specific to the #[rustfmt::skip] attribute. This may be more of a bug than a poor-formatting issue

[davidBar-On]

It may be that PR #4399 that fixed this issue is a patch that does not fix the root cause. The root cause seem to be because Documentation comment is regarded as an Attribute (AST attribute kind DocComment). Currently, per the check done by visit_attrs(), only the first line in a block of attributes is formatted. It seems that it was assumed that a skip will always be the first in a block of attributes. As in this issue example skip is not the first attribute in the attributes block (the Documentation line and the skip) it is not indented.

The resolution should probably be one of the following:

1. Format a block of attributes as any other line of code. I.e. if the block contains skip, only the lines after it will not be formatted.
2. Always format the full block of attributes, even if it includes a skip
3. Do not handle Documentation line as an attribute,.
4. Do nothing - i.e. the issue example is not an issue. Recommend users to always put the skip as the first attribute.

I am currently trying to resolve issue #4499, and a decision about the above is needed, as currently Documentation is handled as any other attributes, and PR #4399 change is causing a conflict.

[cdisselkoen]

Myself, I think I find solution (2) most intuitive. If I put the skip attribute on a function or impl or struct, I expect the skip to apply to the body of the function or impl or struct, but not necessarily the other attributes in the attribute block, and certainly not the attributes above the skip - that would be unintuitive to me. But, I think I could also be convinced that (1) is better, if I understand correctly that it means the skip applies to attributes after it but not attributes before it. I think (4) is not what we want (obviously I'm biased as I was OP for this issue) - in general the recommended style AFAIK is to put documentation before all other attributes, and it would be weird to recommend that skip goes before documentation.

[davidBar-On]

Myself, I think I find solution (2) most intuitive

An issue with (2) is that comments within the attributes block will also be formatted. Issue #4499 is about a comment between skip and allow attributes that is wrongly indented. If (2) is the right approach, then the indentation of comments such as presented in #4499 is o.k. Is this the case?

I expect the skip to apply to the body of the function or impl or struct, but not necessarily the other attributes in the attribute block

Just to make sure, I assume that the first line after the skip should also not be formatted. I am asking because rustfmt does not change the format of the following code, which seem to be o.k:

#[rustfmt::skip]
        pub fn foo1(&self) -> usize {
        0 // fn body
        }

However, the following code:

impl Struct {
    #[rustfmt::skip]
                    pub fn foo2(&self) -> usize {
                    0 // fn body
                    }
}

is formatted as:

impl Struct {
    #[rustfmt::skip]
    pub fn foo2(&self) -> usize {
                    0 // fn body
                    }
}

I assume that line pub fn foo2(&self) -> usize { formatting is a bug.

[cdisselkoen]

I agree with you on all counts. So as a result, seems like (1) is the best solution? (Note that I'm not any kind of authority, not a contributor to rustfmt, etc.)