Consistent format of struct post-comments in new line - with and without trailing comma

[davidBar-On]

Fixes #4654
Resolves #4757

Suggested resolution to the inconsistency of formatting struct post-comment that are in new line, depending on whether there is a trailing comma after the struct item. Without this change, this code:

struct Foo {
    bar_with_trailing_comma: (),
    // Comment
}
struct Foo {
    bar_no_trailing_comma: ()
    // Comment
}

Is formatted as:

struct Foo {
    bar_with_trailing_comma: (),
    // Comment
}
struct Foo {
    bar_no_trailing_comma: (), // Comment
}

The suggested change is that the post-comment will always start in a new line, so the output of both structs will be the same. As far as I understand, keeping the new line is the general approach of rustfmt.

Note that this change contradicts some of the existing test cases (that I changed). However, there should not be any backward compatibility issue. This is because already formatted code should not change. The change suggested in the PR should affect only code that was not formatted before.

The suggested approach practically makes #4757 a non-issue, because if it is accepted that comments in a new line should remain in a new line, than the "Expected output" of the issue is wrong.

[ytmimi]

Thanks for the PR! I haven't had a chance to look at this yet, but just wanted to note that #4655 and #4768 were earlier attempt at resolving this issue.

[davidBar-On]

... #4655 and #4768 were earlier attempt at resolving this issue.

Oops! I don't know how I missed #4655 .... Usually I don't try to resolve issues with open PRs. Regarding #4768, I did see it, but I am not sure it is the right approach. As I mentioned in the description, it may be that #4757 fixed by this PR is actually a non-issue.

Regarding #4655, since it was for R2, I tried now to back-port it, to compare its output to my PR output. I am not sure that the back-port is good enough as extract_post_comment() was changed (including some of the changes suggested by this PR), but following are two possible issues with #4655 changes:

1. It does not keep the alignment in struct comments (maybe this was R2 approach so this was done intentionally). E.g.:

struct Foo {
    bar: (),    // Comment
    bazbaz: (), // Comment
}

is formatted as:

struct Foo {
    bar: (), // Comment
    bazbaz: (), // Comment
}

Instead of remaining as is.

2. Comments are discarded when they follow a terminator that starts new line. E.g.:

struct Foo {
    bar: ()
    ,
    // Comment
}

struct Baz {
    bar: ()
    , // Comment
}

is formatted as:

struct Foo {
    bar: (),
}

struct Baz {
    bar: (),
}

[ytmimi]

Still haven't had a chance to look at this, but these changes will need to be version gated to prevent breaking our backwards compatibility guarantees. You can likely revert all the changes made to existing tests once the version gate is added.

[davidBar-On]

... these changes will need to be version gated to prevent breaking our backwards compatibility guarantees.

Just to make sure, before I add the version gate. As I wrote in the description, there will be no formatting change to code that was already formatted. The changes to the test cases are because the source code is non-formatted. Therefore, backward compatibility is required only if the assumption is that users are expecting that for new (or not formatted) code the related new lines will be ignored. Is this the case?

[davidBar-On]

... these changes will need to be version gated to prevent breaking our backwards compatibility guarantees.

I tried to add the version gating, but I don't find an easy way to pass context as a parameter to extract_post_comment(). The problem is that it is called from ListItems next().

Is there another way to deduce the context.config.version() value? Alternatively, is it possible to label the PR as "Version Two Candidate"?

[ytmimi]

Got it! Thanks for looking into this. Given that existing target tests were changed it likely means that these code changes introduce some change in behavior and those need to be version gated. Is it possible to add a version field to the ListItem struct? Then you could pass context.config.version() or self.config.version() when constructing one with itemize_list.

Let me know if that's something you've already tried and if we need to brainstorm another approach.

[davidBar-On]

Is it possible to add a version field to the ListItem struct?

Added the version gating by adding the version item as suggested. As I expected, it required many changes - to all itemize_list() calls, but there was no problem of context availability for these calls.

The main change was to add version parameter to itemize_list(). A better approach could be to replace the snippet_provider parameter with context parameter, and take both snippet_provider and version from the context. However, that would require changes to FmtVisitor struct: remove snippet_provider item and replace the config item with context. That would allow using self.context.snippet_provider instead of self.snippet_provider. However, I am not sure whether the two snippet_provider are the same, so I didn't take this approach.

[ytmimi]

Hey @davidBar-On I just noticed that #4768 was a previous attempt at solving this issue. It's currently targeting the rustfmt-2.0.0-rc.2 branch, but I wanted to point it out. No rush, but I was wondering if you could compare and contrast the two implementations.

[davidBar-On]

I just noticed that #4768 was a previous attempt at solving this issue ... I was wondering if you could compare and contrast the two implementations.

Per PR #4768 test cases, it combines comment that start a new line with the previous line. I.e. the code:

    match 1 {
        _ => {}
        // hello!
    }
    match 2 {
        _ => {},
        // hello!
    }

is formatted as:

    match 1 {
        _ => {} // hello!
    }
    match 2 {
        _ => {} // hello!
    }

This is fixing the issue described in #4757, but my understanding is that a comment that starts a new line should not be combined with the previous line, i.e. that #4757 is actually a non-issue. For both #4654 and #4757 I took the approach that comments that start a new line will remain in a new line. Therefore my PR output output of the code above is:

    match 1 {
        _ => {}
        // hello!
    }
    match 2 {
        _ => {}
        // hello!
    }

[ytmimi]

Thanks for taking a look! Just double checking, but if comments are written on the same line to begin with your PR will force them onto the next line, right? or am I misunderstanding something?

Given the following input:

    match 1 {
        _ => {} // hello!
    }
    match 2 {
        _ => {}, // hello!
    }

Will your PR produce this output?

    match 1 {
        _ => {}
        // hello!
    }
    match 2 {
        _ => {}
        // hello!
    }

[davidBar-On]

Lines on the same line remain on the same line. For the given code, the PR produce this output:

    match 1 {
        _ => {} // hello!
    }
    match 2 {
        _ => {} // hello!