derive attribute is not wrapped if it's exactly max length

[Dzordzu]

Tested configuration

The default one plus cargo +nightly fmt -- --unstable-features --config error_on_line_overflow=true --config imports_granularity=module

Current situation

error[internal]: line formatted, but exceeded maximum width (maximum: 100 (see `max_width` option), found: 101)
  --> /home/dzordzu/400G/work/wcss/service-user-synchro/lib/sus-protocol/src/lib.rs:58:58:101
   |
58 |     Serialize, Deserialize, Debug, Clone, EnumString, EnumIter, strum_macros::Display, PartialEq, Eq,
   |                                                                                                     ^
   |

Suggestion

Add a new option to the merge_derives that will split derives on line overflow

[ytmimi]

@Dzordzu Thanks for reaching out.

Without a minimal reproducible code snippet that we can use to reproduce the issue there's not action that the rustfmt team can take. When you have a moment can you update the description and provide a code snippet, thanks.

[Dzordzu]

Here you go!

Wrong snippet. This code is working

```rust #[derive( Serialize, Deserialize, Debug, Clone, EnumString, EnumIter, strum_macros::Display, PartialEq, Eq, JsonSchema, ValueEnum )] #[serde(rename_all = "snake_case")] pub enum Action { Create, Update, Delete, Undefined, } ```

[ytmimi]

@Dzordzu thanks for following up.

What version of rustfmt are you using?

When running rustfmt 1.5.2-nightly (2d0aa576 2023-06-18) on your input snippet I'm unable to reproduce the internal error that you posted. I get the following formatting.

#[derive(
    Serialize,
    Deserialize,
    Debug,
    Clone,
    EnumString,
    EnumIter,
    strum_macros::Display,
    PartialEq,
    Eq,
    JsonSchema,
    ValueEnum,
)]
#[serde(rename_all = "snake_case")]
pub enum Action {
    Create,
    Update,
    Delete,
    Undefined,
}

[Dzordzu]

Hm. Strange. After update it works. Sorry for wasting your time

[Dzordzu]

UPDATED: Copied wrong snippet

#[derive(
    Serialize, Deserialize, Debug, Clone, EnumString, EnumIter, strum_macros::Display, PartialEq, Eq,
)]
#[serde(rename_all = "snake_case")]
pub enum Action {
    Create,
    Update,
    Delete,
    Undefined,
}

Fails with:

• cargo +nightly fmt -- --config error_on_line_overflow=true
• rustfmt --config error_on_line_overflow=true ./src/long.rs

Spec:

• rustfmt 1.5.2-stable (90c5418 2023-05-31)
• rustfmt 1.5.3-nightly (8084f39 2023-06-25)

Preview:

[ytmimi]

@Dzordzu I'm unable to reproduce the described behavior with the new snippet either. Can you confirm what version of rustfmt you're using and enumerate any non default configs you might be using. rustfmt should be wrapping those derives. I know you mentioned running cargo fmt.

Can you reproduce the issue when running rustfmt path/to/file.rs directly?

[Dzordzu]

@ytmimi updated the previous reply. I must have had copied the wrong snippet

[ytmimi]

Thanks for updating the snippet. I'm now able to reproduce the issue with rustfmt 1.5.1-stable (90743e72 2023-01-10)

Input

#[derive(
    Serialize, Deserialize, Debug, Clone, EnumString, EnumIter, strum_macros::Display, PartialEq, Eq,
)]
#[serde(rename_all = "snake_case")]
pub enum Action {
    Create,
    Update,
    Delete,
    Undefined,
}

run rustfmt --config error_on_line_overflow=true on the input and you'll get

error[internal]: line formatted, but exceeded maximum width (maximum: 100 (see `max_width` option), found: 101)
 --> <stdin>:2:2:101
  |
2 |     Serialize, Deserialize, Debug, Clone, EnumString, EnumIter, strum_macros::Display, PartialEq, Eq,
  |                                                                                                     ^
  |

warning: rustfmt has failed to format. See previous 1 errors.

[ytmimi]

@IVIURRAY this one seems similar to the issue you just worked on. It's possible that your fix might have resolve this. Would you be able to check if this is resolved when setting version=Two? If your fix works then we're all set here, but it might still be worth adding a test case. If there's still and issue here, maybe you can help track down what's going on.

[IVIURRAY]

@ytmimi I just created test cases for both version One and version Two. It looks like when error_on_line_overflow is true we get the following error in the logs.

error[internal]: line formatted, but exceeded maximum width (maximum: 100 (see `max_width` option), found: 101)
 --> tests/target/issue-5796/true.rs:5:5:101
  |
5 |     Serialize, Deserialize, Debug, Clone, EnumString, EnumIter, strum_macros::Display, PartialEq, Eq,
  |                                                                                                     ^
  |

warning: rustfmt has failed to format. See previous 1 errors.
error[internal]: line formatted, but exceeded maximum width (maximum: 100 (see `max_width` option), found: 101)
 --> tests/target/issue-5796/true_v2.rs:5:5:101
  |
5 |     Serialize, Deserialize, Debug, Clone, EnumString, EnumIter, strum_macros::Display, PartialEq, Eq,
  |                                                                                                     ^
  |

warning: rustfmt has failed to format. See previous 1 errors.

I assume we expect this line to be wrapped? - i.e. the line is greater than max_width, therefore the file should look like the below snipper?

    Serialize, 
    Deserialize, 
    Debug, 
    Clone, 
    EnumString, 
    EnumIter, 
    strum_macros::Display, 
    PartialEq, 
    Eq,

[ytmimi]

@IVIURRAY correct! I believe the expectation is that

#[derive(
    Serialize, Deserialize, Debug, Clone, EnumString, EnumIter, strum_macros::Display, PartialEq, Eq,
)]
#[serde(rename_all = "snake_case")]
pub enum Action {
    Create,
    Update,
    Delete,
    Undefined,
}

is formatted as

#[derive(
    Serialize,
    Deserialize,
    Debug,
    Clone,
    EnumString,
    EnumIter,
    strum_macros::Display,
    PartialEq,
    Eq,
)]
#[serde(rename_all = "snake_case")]
pub enum Action {
    Create,
    Update,
    Delete,
    Undefined,
}

This bug might be present in the attribute rewriting code though. We special case how we rewrite derives since we may need to merge multiple derives. You might want to start looking here:

https://github.com/rust-lang/rustfmt/blob/6356fca675bd756d71f5c123cd053d17b16c573e/src/attr.rs#L425

[IVIURRAY]

Ok, I think I've found the root of the issue.

When we go to write the line here...

https://github.com/rust-lang/rustfmt/blob/6356fca675bd756d71f5c123cd053d17b16c573e/src/attr.rs#L150

It is hard coded to subtract 1 from the sep_count. This means that we don't take into account the final , in the example above (I assume this thinks we will always have a trailing seperator to discount).

https://github.com/rust-lang/rustfmt/blob/6356fca675bd756d71f5c123cd053d17b16c573e/src/lists.rs#L246

If I set this value to 0 to take into account the trailing comma then the code works as expected...but breaks a whole bunch of logic with regards to handling lists of Items.

I will see if I can dig into this more tomorrow and think of a sensible solution. Wanted to let you know my findings if you have some sensible suggestions.

[ytmimi]

@IVIURRAY thanks for looking into this. I'd be surprised if we needed to make changes to the list formatting code in order to solve this, but maybe it's possible 🤷. I'd expect that the issue would be higher up the call stack. The list formatting is pretty fundamental to how rustfmt works.

If the issue really is in the list formatting code, then it may be more complicated to solve this than I initially anticipated. Let me know if you'd still want to look into this one. I'm sure I can find something else on the backlog that will be more straightforward.