Fix/Inconsistent Struct Body Opening Brace Placement After Where Clause #5508

jmj0502 · 2022-08-18T18:46:10Z

Fixes #5507

A new function was added to items.rs. set_brace_pos will format generic structs with where clauses appropriately based on the version provided as a parameter.
Different tests case for both indent_styles were added.

…uses was improved for config `version: Two`

jmj0502 · 2022-08-22T19:25:05Z

@ytmimi Done! Thanks for the version gate clue btw c:
Let me know if there's something else we need to add!

ytmimi · 2022-08-22T21:14:54Z

src/items.rs

+fn set_brace_pos(has_fields: bool, has_content: bool, version: Version) -> BracePos {
+    return match version {
+        Version::One => {
+            if has_fields {
+                return BracePos::Auto;
+            }
+            BracePos::ForceSameLine
+        }
+        Version::Two => {
+            match (has_fields, has_content) {
+                // Doesn't have fields, there is nothing between {}, has generics.
+                (true, true) => BracePos::ForceSameLine,
+
+                // Doesn't have fields, has something between { }, has generics.
+                (true, false) => BracePos::Auto,
+
+                // Has fields, has generics.
+                (false, _) => BracePos::Auto,
+            }
+        }
+    };
+}


Couple of notes:

the return in return match version is redundant, since the match is the only statement in this function. remove the trailing semicolon from the match statement to convert it to an expression and have it implicitly return its value.

I'm all for extracting this logic into a stand alone function, but I think we can simplify the logic see my next comment for details.

ytmimi · 2022-08-22T21:15:22Z

src/items.rs

-            if fields.is_empty() {
-                BracePos::ForceSameLine
-            } else {
-                BracePos::Auto
-            },
+            set_brace_pos(
+                !fields.is_empty(),
+                span.hi() <= body_lo + BytePos(5),


To be honest, this check span.hi() <= body_lo + BytePos(5) has got me scratching my head and it's not obvious from reading the code what it's supposed to be doing. I'm all for moving this logic into a standalone function, but I think this check could be simplified to something like:

if fields.is_empty() { let snippet = context.snippet(mk_sp(body_lo, span.hi())); let body_contains_commets = contains_comment(snippet); if context.config.version() == Version::Two && body_contains_commets { BracePos::Auto } else { BracePos::ForceSameLine } } else { BracePos::Auto }

ytmimi · 2022-08-22T21:16:32Z

tests/source/issue-5507/issue-5507-indent-style-block.rs

+struct BlockComment<T>
+    where T: Eq {
+    /* block comment */
+}


Can we also add a MultiLineBlockComment case

struct MultiLineBlockComment<T> where T: Eq { /* block comment * block comment */ }

ytmimi

Thanks for your work on this.

As noted in the original issue we didn't need any configuration options to produce the inconsistent opening brace placement. For that reason we don't need indent_style=Visual and indent_style=Block tests. However, it would be nice to have an explicit version=One and version=Two test.

Also, I feel like it's fine to place the braces on the same line in the empty EmptyBody and single-line BlockComment cases. I'd expect in the MultiLineBlockComment case that we'd put the opening brace on its own line.

jmj0502 · 2022-08-23T01:07:11Z

@ytmimi Cool! I'll start to with the changes now! The contains_comments function will be super helpful (it will allow me to simplify a lot of code) ✌️

…y (based on the comments received on the review process). The test cases were improved, in order to emphasize the fix.

jmj0502 · 2022-08-23T02:55:43Z

@ytmimi The changes are done! Thanks for all the comments and guidance (they'll sure help me improve as a rustacean 🦀), I really appreciate it!

The whole experience contributing to rustfmt has been awesome and I'm really looking forward to keep on making contributions! c:

ytmimi · 2022-09-07T01:29:50Z

tests/target/issue-5507/issue-5507-version-two.rs

+struct EmptyBody<T>
+where
+    T: Eq,
+{}


Subjective, but I think this should stay on the same line as the bound, Essentially following the version=One formatting.

Okok. I'll proceed to fix it

ytmimi · 2022-09-07T01:30:33Z

tests/target/issue-5507/issue-5507-version-two.rs

+struct BlockComment<T>
+where
+    T: Eq,
+{/* block comment */}


Similarly, I think single line block comments should also follow the version=One formatting.

Is there a way to specifically check for block comments? If so, I the solution shouldn't be too complicated.

To my knowledge there isn't a standalone function that can be used for something like that. However you could check that using CommentStyle, which has a is_block_comment method. You'd use the comment_style function to construct one.

@ytmimi Done! Now empty impl blocks and impl blocks that only contain single line block comments have the same format on each version.

Fantastic. Thank you for making those changes. I'll give this another review in the coming days!

… line block comments are formatted equally in V2 and V1.

… in order to avoid unnecesary duplication. [items.rs - 1286].

…tems.rs - 1302].

jmj0502 · 2022-10-08T00:36:59Z

@ytmimi Hey! Hope you're doing great!

I made a little change, in order to remove some redundant code. I was checking for fields.is_empty() and body_has_comments in the match expression located on items.rs - 1302, despite the fact that such match expression is contained on an if block that checks for fields.is_empty(). Now I just check for body_has_comments.

It was just a little change, but I thought that you should know about it. Have an awesome weekend!

ytmimi · 2022-10-11T14:26:40Z

src/items.rs

+fn set_brace_pos(
+    context: &RewriteContext<'_>,
+    struct_parts: &StructParts<'_>,
+    fields: &[ast::FieldDef],
+    body_lo: BytePos,
+) -> BracePos {
+    if fields.is_empty() {
+        let span = struct_parts.span;
+        let generics_lo = if let Some(generics) = struct_parts.generics {
+            generics.span.lo()
+        } else {
+            body_lo
+        };
+        let snippet = context.snippet(mk_sp(generics_lo, span.hi()));
+        let body_contains_comments = contains_comment(snippet);
+        let has_single_line_block_comment = if body_contains_comments {
+            let comment_start = match snippet.find('/') {
+                Some(i) => i,
+                None => 0,
+            };
+            let comment_snippet = &snippet[comment_start..];
+            let comment = comment_style(comment_snippet, false);
+            let is_block_comment = comment.is_block_comment();
+            let is_single_line_comment = if is_block_comment {
+                let comment_end = match find_comment_end(comment_snippet) {
+                    Some(i) => i,
+                    None => comment_start,
+                };
+                is_single_line(&comment_snippet[..=comment_end - 1])
+            } else {
+                false
+            };
+            is_single_line_comment
+        } else {
+            false
+        };
+
+        if context.config.version() == Version::Two {
+            return match body_contains_comments {
+                true if has_single_line_block_comment => BracePos::ForceSameLine,
+                true => BracePos::Auto,
+                false => BracePos::ForceSameLine,
+            };
+        }
+        return BracePos::ForceSameLine;
+    }
+    BracePos::Auto
+}


I took a look, and I think we could simplify the logic here as follows:

Note: I renamed set_brace_pos to set_struct_brace_pos to be more explicit.

fn set_struct_brace_pos( context: &RewriteContext<'_>, fields: &[ast::FieldDef], span: Span, ) -> BracePos { fn set_struct_brace_pos( context: &RewriteContext<'_>, fields: &[ast::FieldDef], span: Span, ) -> BracePos { if !fields.is_empty() { return BracePos::Auto; } // At this point we know there aren't any struct fields // This snippet contains the unformatted, trimmed struct body let snippet = context.snippet(span).trim(); if snippet.is_empty() || context.config.version() == Version::One { return BracePos::ForceSameLine; } // The body isn't empty so there must be comments. // we also know that we're implicitly using Version Two formatting. let is_single_line = is_single_line(snippet); let is_block_comment = comment_style(snippet, false).is_block_comment(); if is_single_line && is_block_comment { BracePos::ForceSameLine } else { BracePos::Auto } }

At the call site we can hoist inner_span from line 1329 so we can use that as the span argument to set_struct_brace_pos. Let me know if all that makes sense.

ytmimi

@jmj0502 apologies for the delay on the follow up review. I've looked this over and I like the approach of encapsulating the brace position logic into a separate function. I've left some thoughts on how we could simplify it. Let me know your thoughts and if you think I've missed anything.

jmj0502 · 2022-10-12T13:40:56Z

@ytmimi Hey! Hope you're doing well!

No worries, I know that you have other occupations, so I'm ok with PRs not being reviewed instantly 👍.

The approach you decided to take seems super cool, in fact, I like the way most of the logic could be stripped down to some simple steps based on the changes made to the function signature.

I'll implement the changes and let you know when I'm done! c:

…_pos function, in order to simplify it.

ytmimi · 2022-10-13T14:23:34Z

src/items.rs

-            set_brace_pos(&context, &struct_parts, &fields, body_lo),
+            set_struct_brace_pos(&context, &fields, mk_sp(body_lo, span.hi() - BytePos(1))),


Instead of creating the inner_span inline with mk_sp(body_lo, span.hi() - BytePos(1)) can we just hoist it up from where it's originally defined.

+ let inner_span = mk_sp(body_lo, span.hi() - BytePos(1)); let generics_str = match struct_parts.generics { Some(g) => format_generics( context, g, context.config.brace_style(), - set_brace_pos(&context, &struct_parts, &fields, body_lo), + set_struct_brace_pos(&context, &fields, inner_span)), .. ) .. }; ... if fields.is_empty() { - let inner_span = mk_sp(body_lo, span.hi() - BytePos(1)); format_empty_struct_or_tuple(context, inner_span, offset, &mut result, "", "}"); return Some(result); }

@ytmimi Hey! Hope you're doing awesome!

I was about to ask you a question about this point hehe (hositing the inner_span on the outermost scope of the function). I'll add the changes in a few minutes 👍.

…st scope of the format_struct_struct function.

jmj0502 · 2022-10-14T01:51:58Z

@ytmimi Donee! thanks again for all the guidance and the help. This fix really allowed me to understand some parts of Rustfmt a little bit better (in regard to the use of some utility functions, structs, etc). I'm really glad that I'm getting to know the project better and I'm definitely looking forward to keeping on making contributions.

I really appreciate all the things you've taught me; they'll sure help me improve the quality of feature contributions. I'll be ready in case any change is needed after the review c:

ytmimi

@jmj0502 thank you for the kind words and for all the effort you're putting in to help solve these issues. The team really appreciates it! I also love to hear that you're feeling more comfortable with the codebase!

I think the PR is good to go, and I'm happy to move forward. We're still waiting on a few PRs to land in the rust-lang/rust repo, and we'll likely merge this after we sync those changes into rustfmt.

jmj0502 · 2023-01-22T17:10:55Z

@ytmimi Hey! Hope you're doing awesome! Btw, happy new year! 🥳

Sorry that I've been off for a while. I wasn't very active over the course of the last two months due to different reasons including a back injury and some other personal stuff. How you've been?

Is it possible to merge this PR? #5460 depends on this one and it's technically done as well. I'll be looking forward to integrate this changes into #5460 once they land, in order to proceed and upload the updated version of the branch.

Looking forward to hearing from you!
Have a nice day!

Improvements: Adding the tests cases for the issue

0ee1308

ytmimi added the pr-not-reviewed label Aug 19, 2022

jmj0502 added 2 commits August 22, 2022 15:12

Improvements: The formatting for generic structs containing where cla…

2e46b4c

…uses was improved for config `version: Two`

Bugfix: Removing unnecesary value.

58e2a54

jmj0502 marked this pull request as ready for review August 22, 2022 19:21

ytmimi reviewed Aug 22, 2022

View reviewed changes

jmj0502 added 3 commits August 22, 2022 22:36

Improvements: A refactor was performed in order to improve readabilit…

b3e3651

…y (based on the comments received on the review process). The test cases were improved, in order to emphasize the fix.

Improvements: Removing unnecessary comments.

b5323ca

Improvements: Removing additional line break.

0c0fb58

ytmimi added pr-follow-up-review-pending and removed pr-not-reviewed labels Aug 25, 2022

ytmimi reviewed Sep 7, 2022

View reviewed changes

ytmimi added pr-waiting-on-author and removed pr-follow-up-review-pending labels Sep 7, 2022

jmj0502 added 2 commits September 17, 2022 12:41

Improvements: Now empty impl blocks and impl blocks containing single…

254c829

… line block comments are formatted equally in V2 and V1.

Improvements: replacing &snippet[comment_start..] for comment_snippet…

088369e

… in order to avoid unnecesary duplication. [items.rs - 1286].

ytmimi added pr-follow-up-review-pending and removed pr-waiting-on-author labels Sep 18, 2022

Improvements: Removing unnecesary match clause (fields.is_empty()) [i…

f526382

…tems.rs - 1302].

ytmimi reviewed Oct 11, 2022

View reviewed changes

ytmimi removed the pr-follow-up-review-pending label Oct 11, 2022

ytmimi added the pr-waiting-on-author label Oct 11, 2022

Improvements: Making changes to the signature of the set_struct_brace…

c5098f8

…_pos function, in order to simplify it.

ytmimi reviewed Oct 13, 2022

View reviewed changes

Improvements: Now the inner_span variable is hoisted on the outter mo…

5cd84e1

…st scope of the format_struct_struct function.

ytmimi approved these changes Oct 14, 2022

View reviewed changes

ytmimi added pr-ready-to-merge and removed pr-waiting-on-author labels Oct 14, 2022

Merge branch 'master' into fix/issue-5507

64a2321

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/Inconsistent Struct Body Opening Brace Placement After Where Clause #5508

Fix/Inconsistent Struct Body Opening Brace Placement After Where Clause #5508

jmj0502 commented Aug 18, 2022 •

edited

Loading

jmj0502 commented Aug 22, 2022 •

edited

Loading

ytmimi Aug 22, 2022

ytmimi Aug 22, 2022

ytmimi Aug 22, 2022

ytmimi left a comment

jmj0502 commented Aug 23, 2022

jmj0502 commented Aug 23, 2022

ytmimi Sep 7, 2022

jmj0502 Sep 8, 2022

ytmimi Sep 7, 2022

jmj0502 Sep 8, 2022

ytmimi Sep 13, 2022

jmj0502 Sep 17, 2022

ytmimi Sep 18, 2022

jmj0502 commented Oct 8, 2022

ytmimi Oct 11, 2022

ytmimi left a comment

jmj0502 commented Oct 12, 2022

ytmimi Oct 13, 2022

jmj0502 Oct 13, 2022

jmj0502 commented Oct 14, 2022 •

edited

Loading

ytmimi left a comment

jmj0502 commented Jan 22, 2023

		set_brace_pos(&context, &struct_parts, &fields, body_lo),
		set_struct_brace_pos(&context, &fields, mk_sp(body_lo, span.hi() - BytePos(1))),

Fix/Inconsistent Struct Body Opening Brace Placement After Where Clause #5508

Are you sure you want to change the base?

Fix/Inconsistent Struct Body Opening Brace Placement After Where Clause #5508

Conversation

jmj0502 commented Aug 18, 2022 • edited Loading

Fixes #5507

jmj0502 commented Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytmimi left a comment

Choose a reason for hiding this comment

jmj0502 commented Aug 23, 2022

jmj0502 commented Aug 23, 2022

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

jmj0502 commented Oct 8, 2022

Choose a reason for hiding this comment

ytmimi left a comment

Choose a reason for hiding this comment

jmj0502 commented Oct 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmj0502 commented Oct 14, 2022 • edited Loading

ytmimi left a comment

Choose a reason for hiding this comment

jmj0502 commented Jan 22, 2023

jmj0502 commented Aug 18, 2022 •

edited

Loading

jmj0502 commented Aug 22, 2022 •

edited

Loading

jmj0502 commented Oct 14, 2022 •

edited

Loading