Not removing commas from end of multiple attributes list

[davidBar-On]

Fix for #3277 - not removing commas from end of multiple attributes list.

The comma was originally removed since otherwise redundant commas would have been added after consecutive closing delimiters. E.g. .... attr,)]) would have been formatted as .... attr,),],). To prevent that, a logic was added to prevent the comma removal after the last attribute, but without adding the redundant commas.

UPDATE: per this comment, also resolves issue #3228.

[jplatte]

I think the trailing comma should still be removed if it isn't followed by a line break, same as for function calls.

edit: with less negatives: I think the trailing comma should only be kept if it's followed by a line break.

[calebcartwright]

@jplatte I appreciate that you'd prefer different behavior around commas in macro calls, but we're not going to rehash the same, settled posture again on this PR.

The current behavior is a bug which this PR is intended to address, so please refrain from attempts to modify or increase scope.

[jplatte]

Whoa okay, sorry! I thought this was simply a case that wasn't considered properly, rather than an intentional design decision. I didn't see it discussed in #3277 at all.

[Nemo157]

So, to clarify: the behaviour after this PR is that commas at the end of multi-item metalists will not be added/removed at all. If the attribute is converted between multi-line/single-line the user will have to add/remove the final comma to have it consistent with function arguments, but then running rustfmt again will leave it as the user wrote it?

[calebcartwright]

Whoa okay, sorry! I thought this was simply a case that wasn't considered properly, rather than an intentional design decision. I didn't see it discussed in #3277 at all.

No worries. It was implicit vs explicit, but yes it's intentional. If there's a comma in the input source we need to maintain it regardless of leading/trailing whitespace characters.

[calebcartwright]

but then running rustfmt again will leave it as the user wrote it?

yup

[ytmimi]

Overall these changes look good to me. I was curious why the Cargo.lock needed to change. Also, If the user wrote .... attr,),],) would we maintain the commas that they wrote or remove them?

[davidBar-On]

I was curious why the Cargo.lock needed to change.

I don't know. This is beyond my understanding of caro ... It seems that at least the first cargo make after cargo update generates a new lock file.

If the user wrote .... attr,),],) would we maintain the commas that they wrote or remove them?

Adding such commas seem to cause parsing/compilation errors. Are there cases where such commas are valid?

[ytmimi]

I don't know. This is beyond my understanding of cargo ... It seems that at least the first cargo make after cargo update generates a new lock file.

Ah I see. I don't think you need to use cargo make to build rustfmt anymore. At least I haven't had to use it on the 1.x branch. cargo build has worked just fine for me.

Are there cases where such commas are valid?

Maybe not in the case of all attribute macros, but you could conceivably write a macro that requires them and removing those commas would then lead to compilation errors. Ultimately I think our goal for macro rewriting is to not add or remove any tokens so if the developer wrote .... attr,),],) that's what it should be formatted to.

[davidBar-On]

... cargo build has worked just fine for me.

Switched to cargo build and it works fine also for me. Thanks.

Ultimately I think our goal for macro rewriting is to not add or remove any tokens so if the developer wrote .... attr,),],) that's what it should be formatted to.

This PR changes should not affect such macro case. However, I was not able to write code example that will prove that.

[davidBar-On]

... cargo build has worked just fine for me.

Switched to cargo build and it works fine also for me. Thanks.

Ultimately I think our goal for macro rewriting is to not add or remove any tokens so if the developer wrote .... attr,),],) that's what it should be formatted to.

This PR changes should not affect such macro case. However, I was not able to write code example that will prove that.

[ytmimi]

Switched to cargo build and it works fine also for me. Thanks.

Fantastic! I assume with the switch you wont see any other changes with the Cargo.lock

This PR changes should not affect such macro case. However, I was not able to write code example that will prove that.

Taking an existing test case, could we just add trailing commas and make sure that those commas aren't removed. For example:

#[cfg(all(feature1 = "std1",feature2 = "std2",),),]

[davidBar-On]

#[cfg(all(feature1 = "std1",feature2 = "std2",),),]

This is a good example! I found that my initial fix was wrong, and did not properly handle the commas between the parenthesis ... (Note that the last comma before the ] in the example is illegal and causes compilation error.)

I re-implemented the fix and added test cases. The root cause of the issue is that for a list in an attribute, the last AST item includes both the list closing ')' and the attribute closing ']'. Therefore, the check in span_ends_with_comma() of whether there is a comma before the last closing parenthesis did not handle such case properly.

[ytmimi]

@davidBar-On I've been doing some backlog triage and came across #3228, which this PR solves. If you don't mind could you add a test case for the #![allow(nline_always,)] case mentioned in that issue.

[davidBar-On]

... #3228, which this PR solves. If you don't mind could you add a test case for the #![allow(nline_always,)] case mentioned in that issue.

Added tests/target/issue-3228 folder with this and two other test cases. Did not add source tests since the tests are just to make sure no commas are removed.

[ytmimi]

Thank you!!

[djc]

What's the status of this PR? Would be nice to get this merged.

[klensy]

Came here from #6439. PR looks reviewed. I rebased locally (small easy conflict) and it passes tests.

Can it be merged in that case? Thanks.