Control-flow comments formatting #4578

davidBar-On · 2020-12-06T06:52:04Z

A proposed fix for issue #4549 for control-flow comments formatting. The approach taken is:

"pre-else" comments continue to be in a separate line, to be compatible with the formatting of the "pre-if" comments.
The "post-if/else" comments and "post-condition / pre-block" comments are formatted using combine_strs_with_missing_comments(), instead of just putting them in a separate line.

The general output with these changes is:

    /*pre-if*/
    if /*post-if*/ 5 /*x*/ == /*y*/ 6 /*pre-if-block*/ {
        x = y + 7;
    }
    /*pre-elseif*/
    else if /*post-elseif*/ z == u /*pre-elseif-block*/ {
        y = 5;
    }
    /*pre-else*/
    else /*post-else*/ {
        z = x;
    }

This change requires some changes to existing test cases, but it seems that at least in most of these cases this change is what is desired. For example, currently rustfmt formats the following from else-if-brace-style-closing-next-line.rs:

    if false // lone if comment

as:

    if false
    // lone if comment

With the change the output format is:

    if false // lone if comment

which seem to be better, as the comment remains in the same line of the if as in the source.

calebcartwright · 2020-12-17T02:26:57Z

With the change the output format is:
    if false // lone if comment 
which seem to be better, as the comment remains in the same line of the if as in the source.

Ideally rustfmt would honor the original comment placement when formatting so that the association and meaning is maintained, but we obviously fail to do that in many cases.

Are the changes you're proposing here supporting that, or would they also move next line comments up to the same line as the conditional?

E.g. how would these two end up being formatted (assuming items in the bodies)

    if false // i probably say something about the condition
    {
    }

    if false
    // maybe i wanted my comment here to describe the block and not the condition :)
    {
    }

davidBar-On · 2020-12-17T10:46:23Z

Are the changes you're proposing here supporting that, or would they also move next line comments up to the same line as the conditional?

The change does preserve the same/next line placement of the comment, which is the result of using combine_strs_with_missing_comments(). The formatting output of the given code is:

    if false // i probably say something about the condition
    {}

    if false
    // maybe i wanted my comment here to describe the block and not the condition :)
    {}

calebcartwright · 2020-12-20T18:34:53Z

Could you take a look at the corresponding Style Guide section and see if the proposed changes result in formatting that aligns?

My sense is that regardless of the root causes that requires breaking the control line (be it a comment, width, nature of the expression, etc.) then the line breaking indentation rules should still be applied.

Note that we may already be failing to align with the prescribed formatting in the current state with some of these cases with comments, and if we're going to update the comment handling then we may as well go ahead and ensure full alignment with the style guide.

Also, let's be sure to include a wide variety of tests here, since this formatting applies to all control flow expressions (not just If blocks)

davidBar-On · 2020-12-27T15:21:26Z

Also, let's be sure to include a wide variety of tests here, since this formatting applies to all control flow expressions (not just If blocks)

I have added test cases for all the control flow expressions, including a fix for for which I found while adding these test cases.

Could you take a look at the corresponding Style Guide section and see if the proposed changes result in formatting that aligns? ... Note that we may already be failing to align with the prescribed formatting in the current state with some of these cases with comments, and if we're going to update the comment handling then we may as well go ahead and ensure full alignment with the style guide.

The guidelines doesn't say much about comments formatting and I didn't find any issue with their formatting.

The main issue in the current formatting vs. the guideline seem to be related to "... prefer to break before the = in * let expressions and before in in a for expression". There are several examples in the the test cases that do not comply with these guidelines. For example, current formatting of the following code is:

    if let Some(x) =
        (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)

while according to the guidelines it should be formatted as:

    if let Some(x)
        = (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)

calebcartwright

Thank you for the PR. Have only done a cursory glance through but left a few inline feedback items.

To be fully transparent, I'm not sure when I may have a chance to fully review this one and consider merging because my bandwidth is somewhat limited and there's more pressing/higher priority rustfmt items that need attention

calebcartwright · 2021-01-02T19:03:45Z

src/formatting/expr.rs

-            let missing_comments = match rewrite_missing_comment(comments_span, cond_shape, context)
-            {
-                None => "".to_owned(),
-                Some(comment) if self.connector.is_empty() || comment.is_empty() => comment,
-                // Handle same-line block comments:
-                //     if let Some(foo) = /*bar*/ baz { ... }
-                //     if let Some(ref /*def*/ mut /*abc*/ state)...
-                Some(comment)
-                    if !comment_style(&comment, false).is_line_comment()
-                        && !comment.contains('\n') =>
-                {
-                    format!(" {}", comment)
-                }
-                // Handle sequence of multiple inline comments:
-                //     if let Some(n) =
-                //         // this is a test comment
-                //         // with another
-                //         foo { .... }
-                Some(_) => {
-                    let newline = &cond_shape
-                        .indent
-                        .block_indent(context.config)
-                        .to_string_with_newline(context.config);
-                    let shape = pat_shape.block_indent(context.config.tab_spaces());
-                    let comment = format!(
-                        "{}{}",
-                        newline,
-                        rewrite_missing_comment(comments_span, shape, context)?,
-                    );
-                    let lhs = format!("{}{}{}{}", matcher, pat_string, self.connector, comment);
-                    let orig_rhs = Some(format!("{}{}", newline, expr.rewrite(context, shape)?));
-                    let rhs = choose_rhs(
-                        context,
-                        expr,
-                        cond_shape,
-                        orig_rhs,
-                        RhsTactics::Default,
-                        true,
-                    )?;
-                    return Some(format!("{}{}", lhs, rhs));
-                }
-            };
-            let result = format!(
-                "{}{}{}{}",
-                matcher, pat_string, self.connector, missing_comments
+            return rewrite_assign_rhs_with_comments(
+                context,
+                &format!("{}{}{}", matcher, pat_string, self.connector),
+                expr,
+                cond_shape,
+                RhsTactics::Default,
+                comments_span,
+                true,
            );
-            return rewrite_assign_rhs(context, result, expr, cond_shape);


This seems like it could be extracted out into a separate PR, since it's largely just reusing a comparatively new helper and cleaning up some more complex code written before said helper.

If this can't be landed/merged independently, please let me know why, otherwise I'd ask that you pull this change out into a separate PR so that we can go ahead and push that through (understand that this PR may need/want that separate one to get merged first)

Created PR #4626 with this change. This is the only change from this PR that does not depend on the other changes.

calebcartwright · 2021-01-02T19:09:10Z

src/formatting/expr.rs

+                shape,
+                true,
+            )
+            .unwrap()


Please be sure to use the ? operator instead of explicitly unwrapping in functions that return an Option

Done. Changed unwrap() to ? in 3 places.

calebcartwright · 2021-01-02T19:10:08Z

tests/target/loop.rs

-    {
-    }
+    {}


this is a bit concerning from a stability perspective, could you explain why your proposed changes are modifying the emitted formatting in this case?

That happened when formatting the line:

'a: while loooooooooooooooooooooooooooooooooong_variable_name + another_value > some_other_valu{}

The { is in column 101, so it is moved to the next line. In the original code, rewrite_cond() returns used_width that is calculated based on the length of the code before the {. The line break before the '{' is added only later when calling rewrite_block_with_visitor() with a shape based on used_width. It seems that when rewrite_block_with_visitor() determines that the opening { should start a new line, it also does that for the closing }. This is the reason why in the original code the } is in a new line.

The change I did already adds the new-line indent before calculating used_width, so the shape received by rewrite_block_with_visitor() allows it to keep the the { in the same line and therefore the '}` does not start a new line.

Note that the difference is only when the {} block is empty and when the '{` starts a new line. E.g., the formatting of the following code:

fn main() { 'a: while loooooooooooooooooooooooooooooooooong_variable_name + another_value > some_other_valueLong{} 'a: while loooooooooooooooooooooooooooooooooong_variable_name + another_value > some_other_value{ x } }

Is the same when using both the original and modified code:

fn main() { 'a: while loooooooooooooooooooooooooooooooooong_variable_name + another_value > some_other_valueLong {} 'a: while loooooooooooooooooooooooooooooooooong_variable_name + another_value > some_other_value { x } }

calebcartwright · 2021-10-30T15:28:17Z

Thanks again for this, but going to go ahead and close given both the age and 2.0 target. If this is revisited in the future it'd be best done against the current mainline/master branch of the released and supported versions of rustfmt.

Fix for issue rust-lang#4549 - Cintrol-flow comments formatting

2cc1d82

calebcartwright added the pr-not-reviewed label Dec 7, 2020

davidBar-On added 3 commits December 24, 2020 10:25

Handling comments with debug

c0b320c

removing debug info

eb02dc7

remove change to toml done by mistake

943a097

calebcartwright reviewed Jan 2, 2021

View reviewed changes

calebcartwright removed the pr-not-reviewed label Jan 2, 2021

davidBar-On mentioned this pull request Jan 5, 2021

Use rewrite_assign_rhs_with_comments by ControlFlow rewrite_pat_expr #4626

Merged

Change unwrap() to ? in functions that return Option

5d0f88c

calebcartwright added the pr-on-hold label Jan 9, 2021

calebcartwright changed the base branch from master to rustfmt-2.0.0-rc.2 April 30, 2021 00:22

calebcartwright closed this Oct 30, 2021

calebcartwright removed the pr-on-hold label Oct 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Control-flow comments formatting #4578

Control-flow comments formatting #4578

Uh oh!

davidBar-On commented Dec 6, 2020

Uh oh!

calebcartwright commented Dec 17, 2020

Uh oh!

davidBar-On commented Dec 17, 2020

Uh oh!

calebcartwright commented Dec 20, 2020

Uh oh!

davidBar-On commented Dec 27, 2020

Uh oh!

calebcartwright left a comment

Uh oh!

calebcartwright Jan 2, 2021

Uh oh!

davidBar-On Jan 5, 2021

Uh oh!

calebcartwright Jan 2, 2021

Uh oh!

davidBar-On Jan 5, 2021

Uh oh!

calebcartwright Jan 2, 2021

Uh oh!

davidBar-On Jan 5, 2021

Uh oh!

calebcartwright commented Oct 30, 2021

Uh oh!

Uh oh!

Control-flow comments formatting #4578

Control-flow comments formatting #4578

Uh oh!

Conversation

davidBar-On commented Dec 6, 2020

Uh oh!

calebcartwright commented Dec 17, 2020

Uh oh!

davidBar-On commented Dec 17, 2020

Uh oh!

calebcartwright commented Dec 20, 2020

Uh oh!

davidBar-On commented Dec 27, 2020

Uh oh!

calebcartwright left a comment

Choose a reason for hiding this comment

Uh oh!

calebcartwright Jan 2, 2021

Choose a reason for hiding this comment

Uh oh!

davidBar-On Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

calebcartwright Jan 2, 2021

Choose a reason for hiding this comment

Uh oh!

davidBar-On Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

calebcartwright Jan 2, 2021

Choose a reason for hiding this comment

Uh oh!

davidBar-On Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

calebcartwright commented Oct 30, 2021

Uh oh!

Uh oh!