Skip in struct impl and other cases

[davidBar-On]

Suggest fix for issue #4499 - skip attribute in struct impl and in other cases. The changes are basically to all calls to push_skipped_with_span() where the 2nd parameter was x.span. In all these cases, the 2nd parameter was changed to x.span(). This is because x.span() includes the attributes blobk, while item.span does not (found that based on the other calls to push_skipped_with_span - not sure why is that so).

Currently, per the check done by visit_attrs(), only the first line in an attributs block that contains a skip is indented, regardless of the where the skip attribute is in the block. It seems that it was assumed that a skip will always be the first in a block of attributes. If this functionality is not intentional, further enhancements may be required, e.g. not not format all the attributes block (see this short discussion). Note that per this skip.rs test case, at least the attributes following the skip should not be formatted.

The fix includes a change to a test case for issue #4398. It seems that the fix for that issue was not complete, and therefor a skip line was indented, although it is the second line in the attributes block (Documentation comment is regarded as an Attribute - AST attribute kind DocComment).

[calebcartwright]

Note that currently, per the check done by visit_attrs(), only the first line in a block of attributes is formatted, regardless of the where the skip is in the block

This is a rather confusing statement, especially within the context of the PR (and still so even with the links). It will always be helpful, both for the initial PR review and for posterity, for pertinent topics to be expressed clearly and concisely. That's because folks reviewing the PR at the time as well as those circling back to it months/years later won't necessarily have the same mental context as the developer at the time as the latter was in the technical weeds.

I don't think the intended message there is that only the first attribute in a block is formatted, as that's clearly not true:

      #[foo] #[bar]
fn   lorem  () { Qux // asdf
}

I assume there's specific context about the indentation of the first attribute in a block of attributes which contains a skip attr, but if so let's be explicit and clear about that:

    #[foo]
 #[baz]
    #[rustfmt::skip]
     #[bar]
    fn       foo() {
             Bar
        //
    }

[davidBar-On]

I assume there's specific context about the indentation of the first attribute in a block of attributes which contains a skip attr, but if so let's be explicit and clear about that

Yes, this was the intention. Sorry for the confusion. Somehow I missed mentioning the skip attribute in the block. Fixed the original description to make it clear for reviewers.

[calebcartwright]

Also, have you had a chance to run through the exercise I asked about?

Could you take your current tests/source/issue-4499.rs file and run it through the latest 1.x rustfmt (use rustup-installed rustfmt from stable or nightly, in a separate project) and then run your updated version of rustfmt with these changes against that to ensure it's idempotent? (note there could be some minor unrelated changes between 2.x and 1.x, but still want to run this regardless)

[davidBar-On]

Could you take your current tests/source/issue-4499.rs file and run it through the latest 1.x rustfmt (use rustup-installed rustfmt from stable or nightly, in a separate project) and then run your updated version of rustfmt with these changes against that to ensure it's idempotent?

Sorry, overlooked that request. There is one difference between 1.x (1.4.36-nightly) and the updated 2.x version, which is because of the issue fixed by this PR.

1.x output is:

impl Foo {
    // Comment 1
                        #[rustfmt::skip]
                        #[allow(non_snake_case)]
    // Comment 2
                        #[allow(non_snake_case)]
    pub fn foo(&self) {}
}

while the updated version output is:

impl Foo {
    // Comment 1
    #[rustfmt::skip]
                        #[allow(non_snake_case)]
                        // Comment 2
                        #[allow(non_snake_case)]
                        pub fn foo(&self) {}
}

Note that when impl Foo is replaced by fn foo in the above code then both give the same output which is compatible with the updated version output for impl:

fn Foo() {
    // Comment 1
    #[rustfmt::skip]
                        #[allow(non_snake_case)]
                        // Comment 2
                        #[allow(non_snake_case)]
                        pub fn foo(&self) {}
}

[calebcartwright]

Thank you for checking, although it's not entirely clear to me what the input what as at this point given the many subsequent file changes.

The leftward shift of the skip attribute back to the block indented position it would be placed in if the item wasn't being skipped is not correct. That's an outer attribute on the item that is being skipped, so assuming an input of this:

impl Foo {
                        // Comment 1
                        #[rustfmt::skip]
                        #[allow(non_snake_case)]
                        // Comment 2
                        #[allow(non_snake_case)]
                        pub fn foo(&self) {}
}

I would expect an output of:

impl Foo {
    // Comment 1
                        #[rustfmt::skip]
                        #[allow(non_snake_case)]
                        // Comment 2
                        #[allow(non_snake_case)]
                        pub fn foo(&self) {}
}

The first comment has to be realigned because from the AST and span info it's just an arbitrary comment within the Foo impl that needs to be block indented, but everything that's contained within the full span of the function foo item would be left as-is

[davidBar-On]

... so assuming an input of this:

Yes, this is the input I was referring to.

... everything that's contained within the full span of the function foo item would be left as-is

As far as I understand and checked this is not the current functionality. Currently the first line of the block is properly indented. I assumed this is done with the expectation that the skip is the first block line, and that the intention was that the skip line itself will be indented. Something like that the skip is indicating "please do not format the following lines".

Wasn't that the intention? Should the logic be changed to not format the whole block?

[davidBar-On]

... so assuming an input of this:

Yes, this is the input I was referring to.

... everything that's contained within the full span of the function foo item would be left as-is

As far as I understand and checked this is not the current functionality. Currently the first line of the block is properly indented. I assumed this is done with the expectation that the skip is the first block line, and that the intention was that the skip line itself will be indented. Something like that the skip is indicating "please do not format the following lines".

Wasn't that the intention? Should the logic be changed to not format the whole block?

[calebcartwright]

Something like that the skip is indicating "please do not format the following lines".

Remember that attributes are always applied to the corresponding item, expression, etc. not arbitrary lines in a file. In these cases there's a set of outer attributes, which includes the rustfmt attr, added to the foo function item. So that skip attribute is applied to the entire function item, it does not just apply to the "lines" below which is why it doesn't matter where the skip attribute is placed within the collection of attributes.

The indentation level within the top level impl item is correct yes when reformatting things within that impl, such as inner attributes, comments, items, etc. However, since the user has added skip attribute to the function item that means rustfmt should not touch that function item at all. The reformatting of the first attribute is happening in these most recent snippets with current rustfmt versions because the skipped function item is the very first element within that top level impl.

e.g. compare this (doesn't matter whether there's another item, comment, etc., just something before the skipped item):

impl Foo {
    fn bar(   ) {}
                        #[rustfmt::skip]
                        #[allow(non_snake_case)]
                   // Comment 2
                        #[allow(non_snake_case)]
                                pub fn foo(&self) {}
}

vs the very first node within the top level Foo impl being skipped:

impl Foo {

                        #[rustfmt::skip]
                        #[allow(non_snake_case)]
       // Comment 2
                        #[allow(non_snake_case)]
       pub fn foo(&self) {}
}

This is incorrect and inconsistent (the indentation of the first attribute of a skipped item should not depend on that items position in the sequence of subitems within the containing item), but is an existing issue that can and should be addressed separately. However, my concern here is that you said that the formatting/indentation changes are happening even if there are preceding items/comments/etc., which seems to be trading one problem for another.

It's really important to remember that stable/consistent formatting is a critical concern for rustfmt, so we really want to avoid a scenario where folks would upgrade to the next minor/patch version of rustfmt and have their formatting checks start failing on skipped things because the updated rustfmt now wants to move those skipped elements. That would make for a frustrating user experience and also run afoul of our stability guarantee.

So while the core change here is something we'll certainly want to incorporate, we'll also need to resolve the indentation changes it's currently causing (i.e. as shown in this previous example, and above with the additional bar function item preceding the skipped foo item).

Does that make sense?

[davidBar-On]

With these detailed explanations I believe I now understand much better the approach. However there are still two issues I don't understand. The following explanation of my issues repeat some of the examples above, but I hope it will make the issues more clear.

The main issue is if there is a difference between formatting fn and impl with skip inside. Currently (1.x, 2.x) the following fn with skip inside:

fn foo() {
                        // Comment 1
                        #[rustfmt::skip]
                        #[allow(non_snake_case)]
                        // Comment 2
                        #[allow(non_snake_case)]
                        pub fn foo(&self) {}
}

is formatted with the skip line indented:

fn foo() {
    // Comment 1
    #[rustfmt::skip]
                        #[allow(non_snake_case)]
                        // Comment 2
                        #[allow(non_snake_case)]
                        pub fn foo(&self) {}
}

However, from your example here, replacing fn with impl:

impl Foo {
                        // Comment 1
                        #[rustfmt::skip]
                        #[allow(non_snake_case)]
                        // Comment 2
                        #[allow(non_snake_case)]
                        pub fn foo(&self) {}
}

should be formatted without indenting the skip line:

impl Foo {
    // Comment 1
                        #[rustfmt::skip]
                        #[allow(non_snake_case)]
                        // Comment 2
                        #[allow(non_snake_case)]
                        pub fn foo(&self) {}
}

Why the skip line should be indented for fn but not for impl? Should there be such difference? If yes, is it because of backward compatibility or because there is a difference between fn and impl that I don't understand?

The second issue is the indentation of the first line in attributes block that includes a skip (as in 1.x, 2.x). In the above fn example, the skip line is the first, so it is indented. However, if skip is the second line in the block:

fn foo() {
                        // Comment 1
                        #[allow(non_snake_case)]
                        #[rustfmt::skip]
                        // Comment 2
                        #[allow(non_snake_case)]
                        pub fn foo(&self) {}
}

is formatted with the new first line indented:

fn foo() {
    // Comment 1
    #[allow(non_snake_case)]
                        #[rustfmt::skip]
                        // Comment 2
                        #[allow(non_snake_case)]
                        pub fn foo(&self) {}
}

From your explanations I understand the indentation of the first line in the attributes block may be a bug. Therefore, I am not sure what is the preferred approach for this PR formatting inside impl with skip:

1. Indent the first block line as done for fn?
2. Don't indent the first block line only for impl (because of backward compatibility)?
3. Don't indent the first block line for both fn and impl?

[calebcartwright]

Why the skip line should be indented for fn but not for impl? Should there be such difference?

You've honed in on the important question: should vs. does

Why does it get formatted differently within a function block? Because those are different AST that end up getting processed and formatted by different portions of the codebase; it's not an intentional "feature".

Again, important to think about it from the end user perspective. User says "don't touch this thing", and rustfmt currently says "sure, but I'm going to steal the first attribute and reformat it anyway". The end result can be pretty jarring in these sorts of cases, since the attribute ends up being so disconnected from the element it is attributing. It's not right regardless of whatever the skipped item/expr/whatever is nested within.

So to summarize explicitly:

• (1) rustfmt should not change the indentation of the first attribute on a skipped item nested within an item (e.g. an impl, the original issue driving this pr)
  • (1a) rustfmt does currently do this, but only if the skipped item is the first nested element
• (2) rustfmt should not change the indentation of the first attribute of a skipped element within a block
  • (2a) rustfmt does currently do this
  • (2b) this is a separate, technically unrelated issue, which just happens to present with a nearly identical set of symptoms
  • (2c) addressing this would likely be fairly tricky, and is a very low priority

We're really only focused on 1/1a here in this PR, and the fn aspects, especially 2a which you've raised, are orthogonal.

[davidBar-On]

Changed so first line of attributes block i not indented only inside impl. In all other cases the first line of attributes block is indented, to keep backward compatibility. Initially I thought that first line should not be indented also inside trait, but did not did that as 1.x properly handles skip inside trait.

Also rebased and combined all PR branches into one branch (assuming will help the review, as the previous changes were minor compared to the last change).