enhance const block formatting #4493

frbimo · 2020-10-25T14:51:34Z

Testing
cargo make test

frbimo · 2020-10-27T07:42:23Z

...
fn foo() -> i32 {
    const // baz
{
        let x = 5 + 10;
        x / 3
    }
}

test snippet above i change it to

...
fn foo() -> i32 {
    const /* qux */
{
        let x = 5 + 10;
        x / 3
    }
}

because i can't find what is expected formatted result from comment with double slash.
@calebcartwright wdyt ?

calebcartwright · 2020-10-30T05:49:15Z

Thank you for the PR @frbimo and apologies for the delay in review, has been a busy week for me. ~~Could you rebase on master to grab the latest and make CI happy?~~

Edit: hold off on rebasing, we have a new/separate breaking rustc issue again that will prevent compiling on nightly 😞

calebcartwright · 2020-11-01T18:46:01Z

because i can't find what is expected formatted result from comment with double slash.

In a case like this with comments between a keyword and an opening block we should try to respect the authors original comment placing as much as possible (combine_strs_with_missing_comments and recover_missing_comment_in_span do this best).

We should strive to maintain a comment placed on the same line as const regardless of whether it's a line or block comment, and the same is true if the author placed the comment on the following line.

So with input like this in a case where the a user has set brace_style to AlwaysNext line:

fn foo() -> i32 {
    const /* qux */ {
        let x = 5 + 10;
        x / 3
    }
    
    const
    /* qux */
    {
        let x = 5 + 10;
        x / 3
    }
    
    const // qux
    {
        let x = 5 + 10;
        x / 3
    }
    
    const
    // qux
    {
        let x = 5 + 10;
        x / 3
    }
}

the emitted formatting would be:

fn foo() -> i32 {
    const /* qux */
    {
        let x = 5 + 10;
        x / 3
    }
    
    const
    /* qux */
    {
        let x = 5 + 10;
        x / 3
    }
    
    const // qux
    {
        let x = 5 + 10;
        x / 3
    }
    
    const
    // qux
    {
        let x = 5 + 10;
        x / 3
    }
}

frbimo · 2020-11-02T01:13:19Z

sorry if i made myself unclear, i forgot to mentioned i don't get the idea when the brace_style is default.
E.g

fn foo() -> i32 {
  const // qux
    {
        let x = 5 + 10;
        x / 3
    }
}

become like this (?)

fn foo() -> i32 {
  const // qux {
        let x = 5 + 10;
        x / 3
    }
}

calebcartwright · 2020-11-02T05:21:39Z

become like this (?)

fn foo() -> i32 {
  const // qux {
        let x = 5 + 10;
        x / 3
    }
}

No, that would be bad for a few reasons. First, moving the opening bracket behind a line comment would result in breaking code as that bracket would be commented out. Second, and perhaps this is just indentation styles getting botched by GitHub, but the block contents and closing bracket for some reason have extra indentation which is also incorrect.

If there's a trailing line comment after the const keyword then the body has to go on the next line, regardless of brace style. You can observe this in practice already today with functions and structs.

Your snippet would be formatted as

fn foo() -> i32 {
    const // qux
    {
        let x = 5 + 10;
        x / 3
    }
}

As I mentioned above, some of the other comment utility functions may be easier to work with than rewrite_missing_comment in this since they will take care of a lot of this for you. rewrite_missing_comment will require piecing the full snippet together by hand, and will also fail to take into account the original comment placement

frbimo · 2020-11-13T00:24:00Z

I can't ask for review or change label

calebcartwright · 2020-11-13T01:19:56Z

I can't ask for review or change label

No worries, we get notifications when PRs are updated 👍

calebcartwright · 2020-11-14T17:51:48Z

Things are looking good here @frbimo, will try to finish a final review over the weekend but think we're pretty close to being ready to merge

calebcartwright

Thanks @frbimo! Apologies for the late review, have had limited bandwidth lately and have been spending most available cycles trying to get in front of some upstream changes.

The proposed changes seem to have the gist of what is needed, just want to refactor and clean up a bit. Have left some specifics inline for your review

src/formatting/expr.rs

calebcartwright · 2020-11-18T05:18:57Z

src/formatting/expr.rs

+            )?;
+
+            match context.config.brace_style() {
+                BraceStyle::AlwaysNextLine => Some(format!("{}", &combined_strs)),


Just a note that we wouldn't need to use format! here since we aren't joining any strings together, so Some(combined_strs) would have been fine here, though I think the body for this arm will need to change anyway.

If there's a comment in the between span then we could potentially utilize combine_strs_with_missing_comments, and could either have a couple arms with the comments check as a guard:

Suggested change

BraceStyle::AlwaysNextLine => Some(format!("{}", &combined_strs)),

BraceStyle::AlwaysNextLine if contains_comments => combine_strs_with_missing_comments(

context,

"const",

&anon_const_str,

between_span,

shape,

false,

),

BraceStyle::AlwaysNextLine => combine_strs_with_missing_comments(

context,

"const",

&anon_const_str,

between_span,

shape,

!anon_const_str.contains('\n'),

),

or just do something like this (could be helpful to include a comment somewhere nearby providing context for the conditional):

Suggested change

BraceStyle::AlwaysNextLine => Some(format!("{}", &combined_strs)),

BraceStyle::AlwaysNextLine => combine_strs_with_missing_comments(

context,

"const",

&anon_const_str,

between_span,

shape,

!contains_comments && !anon_const_str.contains('\n'),

),

a question, under AlwaysNextLine brace style, contains_comments value is constant (in this case is always false) thus i omit it.
Is there a reason to put it into the logic above? or the test case is not extensive enough?

a question, under AlwaysNextLine brace style, contains_comments value is constant (in this case is always false) thus i omit it.

So the two suggestions I provided were mutually exclusive suggestions.

One suggestion was to utilize two arms, with the first arm utilizing contains_comments as a guard, and in the second arm there was of course no need to utilize contains_comments in the arg to combine_strs_with_missing_comments.

The second suggestion, and the one I'd encourage, is a mutually exclusive alternative to the other suggestion, which only has a single arm that matches AlwaysNextLine (with no guard). In this model, the value of contains_comments is unknown, which is why it would need to be used for determining the value of the arg to use for the last/can_extend param in combine_strs_with_missing_comments.

Does that make sense, or were you asking a different question?

src/formatting/expr.rs

frbimo · 2020-11-23T00:26:00Z

src/formatting/expr.rs

+                BraceStyle::AlwaysNextLine => combine_strs_with_missing_comments(
+                    context,
+                    "const",
+                    &anon_const_str,
+                    between_span,
+                    shape,
+                    !anon_const_str.contains('\n'),
+                ),


Does that make sense, or were you asking a different question?

somehow github doesn't show the update.
i have updated this according to your suggestion. one thing different is allow_extend value i put is without contains_comments as decision parameter because its value is constant. I did cargo make test with code as presented above and it passes the test.
My question is do we still need to put contains_comments here or the test is not extensive enough?

contains_comments as decision parameter because its value is constant

What makes you think that this value is constant? Whether this local has a value of true or false is explicitly variable depending on the input code being formatted

the fact that the code above (contains_comments is omitted) passed the test and successfuly built, i guess contains_comments is always false.
this certainly is my doubt whether i didn't put enough test case or contains_comments is constant.

but this is just a question, at the moment i will follow your suggestion and later will update it with contains_comments on it.

the fact that the code above (contains_comments is omitted) passed the test and successfuly built, i guess contains_comments is always false.
this certainly is my doubt whether i didn't put enough test case or contains_comments is constant.

Ah okay, I think I see what you are asking now.

contains_comment is not a constant, but a utility function that provides the ability to cheaply peek into a certain span to see whether or not there are any comments within there. In this case, that span represents the section between the const keyword and the opening {. So with a const block expression like this: const { x }, contains_comment will return false because there are no comments there of course, but with an expression like const /*abc*/ { x }, contains_comment will return true because there is of course a comment within /*abc*/.

The reason why I included this was to try to be consistent with how rustfmt currently handles comments between the unsafe keyword and the associated block when brace_style is set to AlwaysNextLine:

fn foo() { unsafe /*x*/ { y } }

However, and perhaps this is what you were alluding to 😄, it's not necessary to do so here because combine_strs_with_missing_comments already takes this into account!

Sorry about that, I was going back and forth in my head about which util functions to use and got a bit mixed up. If you wouldn't mind reverting that back (removing the ! contains_comments) then we should be good to merge this. Thanks for your patience and persistence!

sorry, i cant formulate a good sentences.

Signed-off-by: frbimo <fr.bimo@gmail.com>

calebcartwright · 2020-11-26T18:59:51Z

Thanks! Don't worry about the CI checks, it's a toolstate issue that's being addressed separately.

frbimo force-pushed the const_block branch 2 times, most recently from 75180ed to a005a6c Compare October 27, 2020 07:17

calebcartwright added the pr-waiting-on-author label Nov 10, 2020

frbimo force-pushed the const_block branch from a005a6c to f383010 Compare November 12, 2020 16:09

calebcartwright removed the pr-waiting-on-author label Nov 13, 2020

calebcartwright reviewed Nov 21, 2020

View reviewed changes

calebcartwright added the pr-waiting-on-author label Nov 21, 2020

frbimo force-pushed the const_block branch from f383010 to bb1e18c Compare November 22, 2020 16:01

frbimo requested a review from calebcartwright November 22, 2020 16:11

calebcartwright removed the pr-waiting-on-author label Nov 22, 2020

frbimo commented Nov 23, 2020

View reviewed changes

frbimo force-pushed the const_block branch from 3b58804 to ccabb60 Compare November 24, 2020 14:24

enhance const block formatting

3e5542c

Signed-off-by: frbimo <fr.bimo@gmail.com>

frbimo force-pushed the const_block branch from ccabb60 to 3e5542c Compare November 26, 2020 12:58

calebcartwright merged commit e358d19 into rust-lang:master Nov 26, 2020

karyon added the backport-triage label Oct 27, 2021

ytmimi added 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release and removed backport-triage labels Apr 1, 2022

ytmimi mentioned this pull request Apr 1, 2022

Backport 4493 #5289

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance const block formatting #4493

enhance const block formatting #4493

frbimo commented Oct 25, 2020 •

edited by calebcartwright

Loading

frbimo commented Oct 27, 2020 •

edited

Loading

calebcartwright commented Oct 30, 2020 •

edited

Loading

calebcartwright commented Nov 1, 2020

frbimo commented Nov 2, 2020

calebcartwright commented Nov 2, 2020

frbimo commented Nov 13, 2020

calebcartwright commented Nov 13, 2020

calebcartwright commented Nov 14, 2020

calebcartwright left a comment

calebcartwright Nov 18, 2020

frbimo Nov 22, 2020

calebcartwright Nov 22, 2020

frbimo Nov 23, 2020

calebcartwright Nov 23, 2020

frbimo Nov 23, 2020

frbimo Nov 23, 2020

calebcartwright Nov 26, 2020

frbimo Nov 26, 2020

calebcartwright commented Nov 26, 2020

enhance const block formatting #4493

enhance const block formatting #4493

Conversation

frbimo commented Oct 25, 2020 • edited by calebcartwright Loading

frbimo commented Oct 27, 2020 • edited Loading

calebcartwright commented Oct 30, 2020 • edited Loading

calebcartwright commented Nov 1, 2020

frbimo commented Nov 2, 2020

calebcartwright commented Nov 2, 2020

frbimo commented Nov 13, 2020

calebcartwright commented Nov 13, 2020

calebcartwright commented Nov 14, 2020

calebcartwright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Nov 26, 2020

frbimo commented Oct 25, 2020 •

edited by calebcartwright

Loading

frbimo commented Oct 27, 2020 •

edited

Loading

calebcartwright commented Oct 30, 2020 •

edited

Loading