Indentation of multi post-comments to a list item

[davidBar-On]

Suggested fix for issue #3847.
The issue is caused when several one-lone post-comments follow a list item. Because rustfmt is moving the , separator in front of the list item and before the comments, after the format only the first line comment is regarded as post-comment to list item. Therefore these non-first comments should be indented as comments between list items and not as item post-comments.

[calebcartwright]

Thanks for the PR! General approach seems sound but will need to consider single line block comments as well.

[calebcartwright]

What's the thinking for scoping this solely to line comments? Won't the same issue described in #3847 exist with block comments too?

e.g. this snippet has the same idempotency issue

type T1 = Result<
    u32 /* Second comment with some info*/
        /* First longgggggggggggggggggggggggggggggggggg Comment */
        /* First abcd Comment */
        ,
    bool,
>;

[davidBar-On]

What's the thinking for scoping this solely to line comments? Won't the same issue described in #3847 exist with block comments too?

Missed that case 😞 Added it now with handling first one-line block comment /* ... */ the same as one-line open-comment // .... Also added related test cases.

[calebcartwright]

No worries! From a rustfmt POV comments tend to involve myriad edge cases and often end up being a bit annoying 😆

The other case to consider here is when the first line starts a multiline block comment, but there are also additional comments between the end of that block comment and the next list item:

type T1 = Result<
    u32 /* Second comment with some info 
        * First longgggggggggggggggggggggggggggggggggg Comment */
        /* First abcd Comment */
        ,
    bool,
>;

This is starting to feel like a scenario where we may need a more comprehensive picture/processing of the collection of comments within the span, since the typical start/end inspections are not going to be sufficient to cover all cases.

Please also remember that there are various other utilities within the comments module that could potentially be helpful (such as comment_style). Don't feel obligated to use any of them though, just mentioning it as a reminder in case

[davidBar-On]

The other case to consider here is when the first line starts a multiline block comment, but there are also additional comments between the end of that block comment and the next list item:

Added handling of this case. It is more complicated, as formatting of the a first multiline comment should use different shape than the other comments.

(While evaluating this change I found an issue with formatting multiline comments. Since this is a general issue I submitted the proposed fix in a separate PR #4617.)

<This is starting to feel like a scenario where we may need a more comprehensive picture/processing of the collection of comments within the span, since the typical start/end inspections are not going to be sufficient to cover all cases.

The changes should make the solution more comprehensive. I added more test cases, but I am not sure the current solution covers all reasonable possibilities.

Please also remember that there are various other utilities within the comments module that could potentially be helpful (such as comment_style). Don't feel obligated to use any of them though, just mentioning it as a reminder in case

Made some changes to use the available utilities when possible.

[calebcartwright]

Thanks for the updates. Is my understanding correct that in the referenced cases (comments between an item and separator) that come rewriting time we've incorrectly got the entirety of the comments as the post comment on the current item instead of correctly attaching part of those comments as pre comments on the next item in the list?

Maybe that's unavoidable given the general utility of various *list functions, but if so I'd like to expand on the inline comment in the rustfmt source explaining that such comments within the post_comment actually need to be formatted with indentation that aligns as pre-comments with the next item.

If we do need to retroactively "move" part of a post-comment into the logical pre-comment position of the next item as we're doing here then that's fine, though I think we could clean up some of the complexity by refactoring it a bit, perhaps by leveraging things like early returns and reusing locals or adding some internal closures within this closure to minimize duplication.

Could you take a look at refactoring this some more? As an example, we don't need to have path where comment_style gets called twice. I also feel like we should be able to return early if the post comment is not multiline or opens with a line comment, which should reduce some of the readability complexity.

[davidBar-On]

While adding test cases to test first post-comment that start a new line (although I am not sure why someone will do that - such comment is usually written as pre-comment of the next item), I found existing bug in get_comment_end() that is not related to the changes I did. To allow adding the relevant test cases I fixed the bug in this PR and did not open a new PR. I hope this is o.k.

Is my understanding correct that in the referenced cases (comments between an item and separator) that come rewriting time we've incorrectly got the entirety of the comments as the post comment on the current item instead of correctly attaching part of those comments as pre comments on the next item in the list?

Yes, this is correct. After the "front , separator", only the first comment is regarded as post-comment. All the following comments before the next item are regarded as per-comments.

Note that currently the first post-comment is always added to the same line of the item. As I also wrote above, that seem to be o.k., since otherwise, a if the first comment after the item is in a new line, it should follow the separator and be a pre-comment of the next item.

Maybe that's unavoidable given the general utility of various *list functions, but if so I'd like to expand on the inline comment in the rustfmt source explaining that such comments within the post_comment actually need to be formatted with indentation that aligns as per-comments with the next item.

Added a comment before the "post-comment" section.

If we do need to retroactively "move" part of a post-comment into the logical pre-comment position of the next item as we're doing here then that's fine, though I think we could clean up some of the complexity by refactoring it a bit, perhaps by leveraging things like early returns and reusing locals or adding some internal closures within this closure to minimize duplication.

Could you take a look at refactoring this some more? As an example, we don't need to have path where comment_style gets called twice. I also feel like we should be able to return early if the post comment is not multiline or opens with a line comment, which should reduce some of the readability complexity.

Did some refactoring. Hope it is better now.

[calebcartwright]

Did some refactoring. Hope it is better now.

Thanks! Will try to review over the weekend

[calebcartwright]

Thank you for the updates. While I'd still prefer an approach that didn't utilize a match with an unreachable arm, I think this is good enough and don't feel a need to bikeshed.

I know this was a tricky one with lots of annoying edge cases, so thanks for the persistence!