Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhance const block formatting #4493

Merged
merged 1 commit into from
Nov 26, 2020
Merged

enhance const block formatting #4493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion src/formatting/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,32 @@ pub(crate) fn format_expr(
| ast::ExprKind::While(..) => to_control_flow(expr, expr_type)
.and_then(|control_flow| control_flow.rewrite(context, shape)),
ast::ExprKind::ConstBlock(ref anon_const) => {
Some(format!("const {}", anon_const.rewrite(context, shape)?))
let between_span = mk_sp(
context.snippet_provider.span_after(expr.span, "const"),
anon_const.value.span.lo(),
);
let anon_const_str = anon_const.rewrite(context, shape)?;
let contains_comments =
contains_comment(context.snippet_provider.span_to_snippet(between_span)?);
match context.config.brace_style() {
BraceStyle::AlwaysNextLine => combine_strs_with_missing_comments(
context,
"const",
&anon_const_str,
between_span,
shape,
!anon_const_str.contains('\n'),
),
Comment on lines +136 to +143
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, i cant formulate a good sentences.

_ if !contains_comments => Some(format!("const {}", &anon_const_str)),
_ => combine_strs_with_missing_comments(
context,
"const",
&anon_const_str,
between_span,
shape,
true,
),
}
}
ast::ExprKind::Block(ref block, opt_label) => {
match expr_type {
Expand Down
45 changes: 45 additions & 0 deletions tests/source/configs/brace_style/const_block_always_next_line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// rustfmt-brace_style: AlwaysNextLine
// AlwaysNextLine brace style for const blocks

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

fn bar() -> i32 {
const { 4 }
}

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

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

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

fn foo() -> i32 {
const
// baz
{
let x = 5 + 10;
x / 3
}
}
45 changes: 45 additions & 0 deletions tests/source/configs/brace_style/const_block_same_line_where.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// rustfmt-brace_style: SameLineWhere
// SameLineWhere brace style for const blocks

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

fn bar() -> i32 {
const { 4 }
}

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

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

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

fn foo() -> i32 {
const
// baz
{
let x = 5 + 10;
x / 3
}
}
53 changes: 53 additions & 0 deletions tests/target/configs/brace_style/const_block_always_next_line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// rustfmt-brace_style: AlwaysNextLine
// AlwaysNextLine brace style for const blocks

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

fn bar() -> i32
{
const { 4 }
}

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

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

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

fn foo() -> i32
{
const
// baz
{
let x = 5 + 10;
x / 3
}
}
45 changes: 45 additions & 0 deletions tests/target/configs/brace_style/const_block_same_line_where.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// rustfmt-brace_style: SameLineWhere
// SameLineWhere brace style for const blocks

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

fn bar() -> i32 {
const { 4 }
}

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

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

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

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