Trailing comma removed from multiline attribute

[TimNN]

I originally had an overly long attribute like this:

#![allow(clippy::needless_pass_by_value, clippy::new_ret_no_self, clippy::new_without_default_derive)]

This got formatted to:

#![allow(
    clippy::needless_pass_by_value,
    clippy::new_ret_no_self,
    clippy::new_without_default_derive
)]

I manually added a comma after the third line, because I expect this list to change in the future.

Expected behavior:

rustfmt keeps the trailing comma.

Actual behavior:

rustfmt removes the trailing comma.

---

Note that, ideally (IMO), rustfmt would have automatically inserted a trailing comma, or had a setting to specify that behavior. However I don't know if there are situation where the lack of (or presence) of a trailing comma is of semantic significance in (custom) attributes.

If a trailing comma can be significant it should be neither added nor removed automatically, I think. (Unless rustfmt recognizes the specific attribute).

[matthiaskrgr]

Looks like a duplicate of #3228

[elidupree]

I've just run into this bug myself. It doesn't seem like a duplicate of #3228 to me (although it might be related).

In my code, I want to have:

  #[live_prop_test(
    precondition = "inputs.input_flows.len() == self.num_inputs()",
    postcondition = "result.len() == self.num_outputs()",
    postcondition = "output_times_valid(inputs, &result)",
  )]
  fn output_flows(
    &self,
    inputs: MachineObservedInputs,
    future: &Self::Future,
  ) -> Inputs<Option<MaterialFlow>> {
    inputs![]
  }

But rustfmt is insisting on:

  #[live_prop_test(
    precondition = "inputs.input_flows.len() == self.num_inputs()",
    postcondition = "result.len() == self.num_outputs()",
    postcondition = "output_times_valid(inputs, &result)"
  )]
  fn output_flows(
    &self,
    inputs: MachineObservedInputs,
    future: &Self::Future,
  ) -> Inputs<Option<MaterialFlow>> {
    inputs![]
  }

It seems clear that rustfmt should default to requiring the trailing comma (probably respecting the new trailing_commas setting).

It's technically true that an attribute proc-macro like this is able to behave differently based on the presence or absence of the trailing comma, so you might think rustfmt shouldn't touch it. But a macro generally shouldn't do that, and for the very weird case where it does, we have #[rustfmt::skip::attributes()] available. (Similarly, a proc macro can technically behave differently based on the presence or absence of trailing commas inside the function it's modifying, and we sure wouldn't want to inconvenience regular users to accommodate that.)

[xpe]

It's technically true that an attribute proc-macro like this is able to behave differently based on the presence or absence of the trailing comma, so you might think rustfmt shouldn't touch it. But a macro generally shouldn't do that, and for the very weird case where it does, we have #[rustfmt::skip::attributes()] available.

Is such an approach (allowing Rustfmt to have any chance of breaking code) considered acceptable? (If so, that would surprise me.)

It is my expectation that rustfmt not make any text changes that could change program compilation. Is this written out somewhere more precisely?

[elidupree]

Proc macros have access to raw line and column numbers of all tokens (a feature which has been used in practice), so it must be considered acceptable to break potential proc macros, or rustfmt wouldn't be allowed to make any changes at all. The only question is where to draw the line.

[calebcartwright]

so it must be considered acceptable to break potential proc macros, or rustfmt wouldn't be allowed to make any changes at all

This is not correct. Obviously bugs happen, but we never have any leeway to implement formatting behavior that is known to be a potential source issues, regardless of "shoulds" or "best practices". This is why we've been unable to implement certain features historically (e.g. derive sorting), and is in fact why the commas are not inserted by default.

However, the discussion about default insertion of trailing commas is really getting off track the core topic here of the removal of a user-added comma.

[elidupree]

I recognize that this is a bit of a side note, but I'd like to clarify:

If rustfmt truly has no leeway to break potential proc macros, then it is a bug that rustfmt changes any formatting within attribute arguments or within any token tree that has an attribute macro applied to it. Currently:

1. Even if we fix this issue, rustfmt still changes whitespace within attribute arguments.
2. Naturally, rustfmt adjusts both whitespace and commas/etc in functions, regardless of whether they have attribute macros applied to them.

Are you saying that one or both of these is a bug in rustfmt? (If they aren't bugs, it seems like the line rustfmt is drawing is "Assume that all macros ignore whitespace, and assume that attribute macros don't care about formatting changes in the token trees they modify". I think those are reasonable lines to draw for practical reasons.)

It seems to me that the issue you linked is responding to a practical concern where it is common for macros to reject trailing commas, rather than a formal concern about whether it would be possible for a macro to reject trailing commas.

[calebcartwright]

I recognize that this is a bit of a side note, but I'd like to clarify:

@elidupree - I appreciate your passion for precision, but this is entirely off topic and I don't want this thread to continue to be side tracked for an exercise in pedantry on an unrelated front. Definitely feel free to find/create a more appropriate issue if you'd like to continue the discussion or otherwise make your case, but please refrain from doing so here as this will be my last comment on the topic on this thread.

If rustfmt truly has no leeway to break potential proc macros, then it is a bug that rustfmt changes any formatting within attribute arguments or within any token tree that has an attribute macro applied to it.

Are you saying that one or both of these is a bug in rustfmt? (If they aren't bugs, it seems like the line rustfmt is drawing is "Assume that all macros ignore whitespace, and assume that attribute macros don't care about formatting changes in the token trees they modify". I think those are reasonable lines to draw for practical reasons.)

No, we unequivocally do not consider that a bug. Yes macros have access to spans and source file contents, but we follow Rust's posture that whitespace between tokens does not carry semantic significance. The types of things we consider bugs include things like removing/inserting tokens from the stream, and even formatting changes around calls that can cause issues with expansion.

We would of course re-evaluate if the semantics of whitespace changes, or on a case by case basis if a reasonably prominent ecosystem macro for some reason opts to makes whitespace semantically significant in a way which the Rust Style Guide's prescriptions fail to uphold. However, you should not conflate that with meaning rustfmt must either do nothing or can do absolutely anything.

[elidupree]

but please refrain from doing so here

Got it - it sounds like we are mostly in agreement on what rustfmt's priorities should be, in any case.

[davidBar-On]

Submitted PR #5394 with a proposed fix.