5801 enum variant doc comments are wrapped before comment_width

[IVIURRAY]

Fixes #5801

[ytmimi]

@IVIURRAY would you mind holding off on creating PRs until the bugfix is ready. You can verify that your code changes work by running the test suite locally with cargo test.

[IVIURRAY]

@ytmimi yeah that is fine, will avoid raising until finished in the future.

[ytmimi]

@IVIURRAY thanks! To clarify, I don't think there's anything wrong with opening draft PRs. However, In this particular case I don't think it's necessary if you're just adding failing test cases.

Let me know if you need any pointers for this one.

[IVIURRAY]

Hey @ytmimi - yes, that is totally fine, I can keep the failing tests local until I'm ready to raise an MR 😄

Also, I managed to get this MR passing with the test case from #5801 !! - Let me know if you've comments, I'm not going to say I know exactly how everything is hanging together here as this is my first MR for rustfmt. Please let me know if you want changes...

[ytmimi]

@IVIURRAY great! I should have some time later today to review this and provide feedback

[ytmimi]

The sub_width(1) is still important for rewriting each enum variant as we need to take the trailing comma into account. Thats what the comment // 1 = ',' was telling us. The bug here is that we're apply sub_width(1) prematurely. We still need to reduce the width by 1 after we rewrite the attributes as the trailing comma doesn't impact the attributes.

Additionally, I think it would be best if these changes were version gated.

probably best to change this to something like:

let attrs_str = if context.config.version() == Version::Two {
    field.attrs.rewrite(&context, shape)?
} else {
   // Version::One formatting that was off by 1. See issue #5801
   field.attrs.rewrite(&context, shape.sub_width(1)?)?
};

// sub_width(1) to take the trailing comma into account
let shape = shape.sub_width(1)?;

[ytmimi]

wait, this is only an issue when wrap_comments=true is set, right? In that case we don't need the version gate since wrap_comments=true is an unstable option. I'd then expect the code change to look something like:

Edit: Disregard this comment.

let attrs_str = field.attrs.rewrite(&context, shape)?;
// sub_width(1) to take the trailing comma into account
let shape = shape.sub_width(1)?;

[ytmimi]

As I was thinking about this more I'm now 100% convinced this change needs to be version gated as attribute formatting is also influenced by stable configuration options like attr_fn_like_width and use_small_heuristics.

Consider this extended example from what you have in the test case.

pub enum Severity {
    /// But here, this comment is 120 columns wide and the formatter wants to split it up onto two separate lines still.
    Error,
    /// This comment is 119 columns wide and works perfectly. Lorem ipsum. lorem ipsum. lorem ipsum. lorem ipsum lorem.
    Warning,
    #[something(AAAAAAAAAAAAA, BBBBBBBBBBBBBB, CCCCCCCCCCCCCCCC, DDDDDDDDDDDDD, EEEEEEEEEEEE, FFFFFFFFFFF, GGGGGGGGGGG)]
    AttrsWillWrap,
    #[something_else(hhhhhhhhhhhhhhhh, iiiiiiiiiiiiiiii, jjjjjjjjjjjjjjj, kkkkkkkkkkkkk, llllllllllll, mmmmmmmmmmmmmm)]
    AttsWontWrap,
}

Using a recent nightly rustfmt 1.7.0-nightly (89e2160c 2023-12-27) with the input above and these configuration options:

comment_width = 120
max_width = 120
wrap_comments = true
version = "One"
attr_fn_like_width = 120

produces this formatting:

pub enum Severity {
    /// But here, this comment is 120 columns wide and the formatter wants to split it up onto two separate lines
    /// still.
    Error,
    /// This comment is 119 columns wide and works perfectly. Lorem ipsum. lorem ipsum. lorem ipsum. lorem ipsum lorem.
    Warning,
    #[something(
        AAAAAAAAAAAAA,
        BBBBBBBBBBBBBB,
        CCCCCCCCCCCCCCCC,
        DDDDDDDDDDDDD,
        EEEEEEEEEEEE,
        FFFFFFFFFFF,
        GGGGGGGGGGG
    )]
    AttrsWillWrap,
    #[something_else(hhhhhhhhhhhhhhhh, iiiiiiiiiiiiiiii, jjjjjjjjjjjjjjj, kkkkkkkkkkkkk, llllllllllll, mmmmmmmmmmmmmm)]
    AttsWontWrap,
}

[IVIURRAY]

I've updated the code to include the version gate. Would you also like me to update the test cases to match what you've as an example here?

[ytmimi]

@IVIURRAY great work on this so far. I hope you're getting a better sense of what it's like to work on rustfmt! I'd like you to add two additional test cases, and move the test cases around.

• a test case for version=One which shows that we prematurely wrap. It's always good to have a test for the stable formatting (even if it's buggy)
• a separate test case for the wrapping attributes. I'd prefer if this was a distinct test file to highlight that we can trigger the issue with just stable options.
• Also, can you move these test cases into a tests/{source | target}/issue_5801 folder. Feel free to name these test cases whatever you think is best. Here's some inspiration on the names:
  • comment_unexpectedly_wraps_before_max_width (Version::One test case for doc comments)
  • comment_does_not_wrap_within_max_width (Version::Two test case for doc comments)
  • attribute_unexpectedly_wraps_before_max_width (Version::One test case for attributes)
  • attribute_does_not_wrap_within_max_width (Version::Two test case for attributes)

[ytmimi]

Also, probably best to squash all of these commits down into a single commit. Feel free to do that on your end, otherwise I'll handle that when merging.

[IVIURRAY]

@ytmimi ok, I'm a little confused here. My test for /attribute_does_not_wrap_within_max_width.rs is unstable and I don't know why.

Running this test in my IDE passes and then fails and then passes. You need to just keep running it a few times. When I pushed from my IDE it passed then running on github actions failed. This caused me to notice the same was happening locally. I'm verey confused as to why this would be. Do you've any insight?

Below is the output I get upon a failed system_tests but sometimes I also just get a failure on idempotence_tests.

Mismatch at tests/source/issue-5801/attribute_does_not_wrap_within_max_width.rs:3:
 // rustfmt-attr_fn_like_width: 120
 
 pub enum Severity {
-    #[something(AAAAAAAAAAAAA, BBBBBBBBBBBBBB, CCCCCCCCCCCCCCCC, DDDDDDDDDDDDD, EEEEEEEEEEEE, FFFFFFFFFFF, GGGGGGGGGGG)]
+    #[something(
+        AAAAAAAAAAAAA,
+        BBBBBBBBBBBBBB,
+        CCCCCCCCCCCCCCCC,
+        DDDDDDDDDDDDD,
+        EEEEEEEEEEEE,
+        FFFFFFFFFFF,
+        GGGGGGGGGGG
+    )]
     AttrsWillWrap,
     #[something_else(hhhhhhhhhhhhhhhh, iiiiiiiiiiiiiiii, jjjjjjjjjjjjjjj, kkkkkkkkkkkkk, llllllllllll, mmmmmmmmmmmmmm)]
     AttsWontWrap,

[IVIURRAY]

Also, just to let you know I squashed the commits, rebased master and created an issue-5801 folder (I know you said issue_5801but the other folders followed that format in /target.

[ytmimi]

Running this test in my IDE passes and then fails and then passes. You need to just keep running it a few times. When I pushed from my IDE it passed then running on github actions failed. This caused me to notice the same was happening locally. I'm very confused as to why this would be. Do you've any insight?

I'm just as confused. That shouldn't happen. I wrote a little script to run cargo test over and over again:

#!/bin/bash
set -e

for i in {1..20};
do
    cargo test -q &>/dev/null
    if [ $? -eq 0 ]; then
        echo "PASS $i"
    else
        echo "FAIL $i"
    fi
done

I'm unable to reproduce the failure locally when running cargo test. How are you running the tests? Maybe there are certain steps that are needed to reproduce the issue.

Also, I find it odd that this is failing in CI for the windows / (x86_64-pc-windows-msvc, nightly) and windows / (x86_64-pc-windows-gnu, nightly) targets.

[IVIURRAY]

Running this test in my IDE passes and then fails and then passes. You need to just keep running it a few times. When I pushed from my IDE it passed then running on github actions failed. This caused me to notice the same was happening locally. I'm very confused as to why this would be. Do you've any insight?

I'm just as confused. That shouldn't happen. I wrote a little script to run cargo test over and over again:

#!/bin/bash
set -e

for i in {1..20};
do
    cargo test -q &>/dev/null
    if [ $? -eq 0 ]; then
        echo "PASS $i"
    else
        echo "FAIL $i"
    fi
done

I'm unable to reproduce the failure locally when running cargo test. How are you running the tests? Maybe there are certain steps that are needed to reproduce the issue.

Also, I find it odd that this is failing in CI for the windows / (x86_64-pc-windows-msvc, nightly) and windows / (x86_64-pc-windows-gnu, nightly) targets.

I've recorded a video of me just running cargo test from the cli and getting different results - see here

Honestly, I think if we re-run the pipeline it won't be consistent. I think I just got lucky there with some build systems passing.

[ytmimi]

@IVIURRAY thanks for the video. I was able to reproduce the error when running the idempotence_tests. My hunch is that the issue is related to setting the configs inline with the rustfmt comments like //rustfmt-{key}: {value} in read_config. From what I can tell read_significant_comments returns a HashMap<String, String>, and when we try to iterate over it we don't necessarily iterate over the configurations in the same order that they're listed in the file. Sometimes we set the max_width=120 before we set attr_fn_like_width=120, and in that case everything works out. In other cases we try to set attr_fn_like_width=120 first, and that fails since we don't allow any width configuration to exceed the max_width, which is still set to its default of 100. so attr_fn_like_width would also get set to 100.

Just to be totally clear, this isn't an issue I want to address in this PR, and I think I need to do some more investigating on my end.

To explain further, In most cases setting the configuration values inline with the rustfmt comments isn't an issue because the configuration values don't depend on each other, but in this case all the width configurations depend on the max_width.

I think the workaround here is to use the // rustfmt-config: comment, where we can specify the name of a configuration file listed in the tests/config directory. When we read the config from a toml file we don't have this same issue.

I think we can create two new files, let's call them tests/config/issue-5801-v1.toml and tests/config/issue-5801-v2.toml.

Then in each test file you can replace the inline configs with // rustfmt-config: issue-5801-v1.toml or// rustfmt-config: issue-5801-v2.toml

[IVIURRAY]

Ah, you genius @ytmimi! - That makes total sense to me, I can totally understand how that is is causing the issue. I thought I had broken something even worse!

I've moved the config into a .toml file now and checks are passing 🎉

Would you like me to write this up as an issue?...maybe this could be the next thing I can work on. You might've something else that would be easier for me though, wait to hear.

[ytmimi]

Thank you for the kind words @IVIURRAY! I thought this one would be simple, but I couldn't have anticipated all the issues that would have popped up. I'm really glad that you continued to work through them all.

Thank you for your first contribution to rustfmt 🎉

It would be great if you could create a new issue to document the testing problem we ran into. I don't think you need to continue looking into it right now. It's a bit of an edge case that I don't think we'll run into soon. I've got a rough idea on how to correct this problem, but as I mentioned, there are a few things I want to double check first.

I don't have another issue for you off the top of my head, but I'm happy to look through the backlog to see if there's another one that could ease you into the codebase.

[IVIURRAY]

Wahoo! - Thanks for the approval and merge @ytmimi, glad I could contribute! I learnt a lot even if most of that was understanding the Rust toolchain 😆

I've created an issue to capture unstable test #6011

I would like another issue to investigate if you've on that you think would be suitable for me 😄 I'll wait to hear from you.

[ytmimi]

As long as you learned something then it was all worth it! I appreciate you following up and creating the issue. I'll checkout the backlog over the next day or so and tag you on an issue that I think would be another good one for you to take on!