Skip to content

Control-flow comments formatting #4578

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.

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
191 changes: 112 additions & 79 deletions src/formatting/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ use crate::formatting::{
chains::rewrite_chain,
closures,
comment::{
combine_strs_with_missing_comments, comment_style, contains_comment,
recover_comment_removed, rewrite_comment, rewrite_missing_comment, CharClasses,
FindUncommented,
combine_strs_with_missing_comments, contains_comment, recover_comment_removed,
rewrite_comment, rewrite_missing_comment, CharClasses, FindUncommented,
},
lists::{
definitive_tactic, itemize_list, shape_for_tactic, struct_lit_formatting, struct_lit_shape,
Expand Down Expand Up @@ -896,54 +895,15 @@ impl<'a> ControlFlow<'a> {
.snippet_provider
.span_after(self.span, self.connector.trim());
let comments_span = mk_sp(comments_lo, expr.span.lo());

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);
Comment on lines -900 to -946
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

let expr_rw = expr.rewrite(context, cond_shape);
Expand Down Expand Up @@ -1045,46 +1005,82 @@ impl<'a> ControlFlow<'a> {
},
);

let between_kwd_cond_comment = extract_comment(between_kwd_cond, context, shape);
let between_kwd_cond_comment = rewrite_missing_comment(between_kwd_cond, shape, context);

let after_cond_comment =
extract_comment(mk_sp(cond_span.hi(), self.block.span.lo()), context, shape);
rewrite_missing_comment(mk_sp(cond_span.hi(), self.block.span.lo()), shape, context);

let block_sep = if self.cond.is_none() && between_kwd_cond_comment.is_some() {
""
} else if context.config.control_brace_style() == ControlBraceStyle::AlwaysNextLine
let block_sep = if context.config.control_brace_style() == ControlBraceStyle::AlwaysNextLine
|| force_newline_brace
{
alt_block_sep
} else {
" "
};

let used_width = if pat_expr_string.contains('\n') {
/* Combine with pre condition comment */
let label_and_keyword = &format!("{}{}", label_string, self.keyword);
let cond_with_between_kwd_cond_comment = if between_kwd_cond_comment
.as_ref()
.map_or(true, |c| c.is_empty())
{
format!(
"{}{}{}",
label_and_keyword,
if pat_expr_string.is_empty() || pat_expr_string.starts_with('\n') {
""
} else {
" "
},
pat_expr_string,
)
} else {
combine_strs_with_missing_comments(
context,
&label_and_keyword,
&pat_expr_string,
between_kwd_cond,
shape,
true,
)?
};

/* Combine with post-condition comment */
let cond_with_after_cond_comment = combine_strs_with_missing_comments(
context,
&cond_with_between_kwd_cond_comment,
"",
mk_sp(cond_span.hi(), self.block.span.lo()),
shape,
true,
)?;

/* Add separator before block */
let last = cond_with_after_cond_comment.lines().last()?.trim_end();
let closing_sep = if last.trim_start().is_empty() {
""
} else if after_cond_comment
.as_ref()
.map_or(false, |s| s.starts_with("//"))
{
alt_block_sep
} else if last.len() + brace_overhead > context.config.max_width() {
alt_block_sep
} else {
block_sep
};

let cond_string = format!("{}{}", cond_with_after_cond_comment, closing_sep);
let used_width = if closing_sep.contains('\n') {
closing_sep.len()
} else if pat_expr_string.contains('\n') {
last_line_width(&pat_expr_string)
} else {
// 2 = spaces after keyword and condition.
label_string.len() + self.keyword.len() + pat_expr_string.len() + 2
};

Some((
format!(
"{}{}{}{}{}",
label_string,
self.keyword,
between_kwd_cond_comment.as_ref().map_or(
if pat_expr_string.is_empty() || pat_expr_string.starts_with('\n') {
""
} else {
" "
},
|s| &**s,
),
pat_expr_string,
after_cond_comment.as_ref().map_or(block_sep, |s| &**s)
),
used_width,
))
Some((cond_string, used_width))
}
}

Expand All @@ -1094,6 +1090,7 @@ impl<'a> Rewrite for ControlFlow<'a> {

let alt_block_sep = &shape.indent.to_string_with_newline(context.config);
let (cond_str, used_width) = self.rewrite_cond(context, shape, alt_block_sep)?;

// If `used_width` is 0, it indicates that whole control flow is written in a single line.
if used_width == 0 {
return Some(cond_str);
Expand Down Expand Up @@ -1169,7 +1166,7 @@ impl<'a> Rewrite for ControlFlow<'a> {
.span_after(mk_sp(self.block.span.hi(), else_block.span.lo()), "else"),
else_block.span.lo(),
);
let after_else_comment = extract_comment(after_else, context, shape);
let after_else_comment = rewrite_missing_comment(after_else, shape, context);

let between_sep = match context.config.control_brace_style() {
ControlBraceStyle::AlwaysNextLine | ControlBraceStyle::ClosingNextLine => {
Expand All @@ -1182,13 +1179,49 @@ impl<'a> Rewrite for ControlFlow<'a> {
_ => " ",
};

result.push_str(&format!(
"{}else{}",
let brace_overhead =
if context.config.control_brace_style() != ControlBraceStyle::AlwaysNextLine {
2
} else {
0
};

// Combine with pre-else comment
let else_with_pre_comment = format!(
"{}else",
between_kwd_else_block_comment
.as_ref()
.map_or(between_sep, |s| &**s),
after_else_comment.as_ref().map_or(after_sep, |s| &**s),
));
);

/* Combine with post-else comment */
let else_with_post_comment = combine_strs_with_missing_comments(
context,
&else_with_pre_comment,
"",
after_else,
shape,
true,
)?;

/* Add separator befor block */
let last = else_with_post_comment.lines().last()?.trim_end();
let closing_sep = if last.trim_start().is_empty() || last.trim_start() == "\n" {
""
} else if after_else_comment
.as_ref()
.map_or(false, |s| s.starts_with("//"))
{
alt_block_sep
} else if last.len() + brace_overhead > context.config.max_width() {
alt_block_sep
} else {
after_sep
};
let else_string = format!("{}{}", else_with_post_comment, closing_sep);

result.push_str(&else_string);

result.push_str(&rewrite?);
}

Expand Down
117 changes: 117 additions & 0 deletions tests/source/comments-control-flow.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
// Issue #4549

// Different `if`/`else` expressions
// NOTE: comments before and after the condition sign are currently not handled,
// so the original code snippet is used.
// Therefore, althorugh such cases are included in some of the tests,
// such as `if x /*x*/ == /*y*/ y`, they are not really part of the tests purpose.
fn main() {
if /*w*/ x /*x*/ == /*y*/ y /*z*/ {}
}

fn main() {
/*pre-if*/ if /*post-if*/ x == y /*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;}
}

fn main() {
/*pre-if*/ if /*post-if*/ x /*x*/ == /*y*/ y /*pre-if-block*/ { x = y +7; }
/*pre-elseif*/ else if /*post-elseif Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggg*/ z == u /*pre-elseif-block*/ { y = 5; }
/*pre-else*/ else /*post-else*/ { z = x; }
}

fn main() {
/*pre-if*/ if // post-if - make sure that x and y states are the same
x == y //pre-if-block - when they are same change y to next state
{ x = y +7; }
}

fn main() {
/*pre-if*/ if // post-if - make sure that x and y states are the sameeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
x /*x*/ == /*y*/ y //pre-if-block - when they are same change y to next stateeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
{ x = y +7; }
}

fn main() {
if 5 == 6 { x = y +7; }
else if z == u { y = 5; }
else { z = x; }
}

fn main() {
if // if comment
5 == 6 { x = y +7; }
}fn main() {
if
// if comment
5 == 6 { x = y +7; }
}

// Tests for `loop`
fn main() {
loop { x = y +7; };

/*pre-statement*/ loop /*pre-block*/ { x = y +7; };

/*pre-statement*/ loop /*pre-block Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg*/ { y = 5; };

/*pre-statement*/ loop /*pre-block Longerrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr*/ { y = 5; };

loop /*pre-block Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg*/ {exppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppp};
}

// Tests for `while`
// NOTE: same coments limitations as for `if`.
fn main() {
/*pre-statement*/ while /*post-cond*/ x /*x*/ == /*y*/ y /*pre-block*/ { x = y +7; };

/*pre-statement*/ while /*pre-cond Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg*/ z == u /*pre-block*/ { y = 5; };

/*pre-statement*/ while /*pre-cond Longerrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr*/ z == u /*pre-block*/ { y = 5; };

/*pre-statement*/ while // pre-cond - make sure that x and y states are the sameeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
x /*x*/ == /*y*/ y //pre-block - when they are same change y to next stateeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
{ x = y +7; }
}

// Tests for `if/while let`
// NOTE: comments before an after the `pat` item that follows `let` (e.g. the `Some(x)`
// in `let Some(x) =`) are currently not handledand, so original code is used.
// E.g., in `while let /*C1*/ Some(x) /*C2*/ = /*C3*/ exp /*C4*/{}`, only the
// `C3` and `C4` comments are handled, and therefore comments such as `C1` AND `C2`
// are not included in the test.
fn main() {
/*pre-statement*/ while let Some(x) = /*pre-epr*/ exp /*pre-block*/ { y = x +7; };

/*pre-statement*/ while let Some(x) = /*pre-expr Longggggggggggggggggggggggggggggggggggggggggggggggggggg*/ exp /*pre-block*/ { y = x; };

/*pre-statement*/ while let Some(x) = /*pre-expr Longerrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr*/ exp /*pre-block*/ { y = x; };

/*pre-statement*/ while let Some(x) = // pre-exprrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr
exp //pre-blockkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk
{ y = x +7; }
}
fn main() {
/*pre-statement*/ if let Some(x) = /*pre-epr*/ exp /*pre-block*/ { y = x +7; };

/*pre-statement*/ if let Some(ref /*def*/ mut /*abc*/ state) = /*pre-epr*/ exp /*pre-block*/ { y = x +7; };

/*pre-statement*/ if let Some(ref /*def*/ mut /*abc*/ state) = /*pre-eprrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr*/ exp /*pre-blockkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk*/ { y = x +7; };
}

// Tests for `for`
// NOTE: comments `in` aare currently not handledand, so original code is used.
// E.g., in `for x /*C1*/ in /*C2*/`, only the `C2` comment is handled,
// and therefore comments such as `C1` are not included in the test.
fn main() {
/*pre-statement*/ for /*post-cond*/ x in /*post-in*/ exp /*pre-block*/ { y = x +7; };

/*pre-statement*/for /*pre-cond Longggggggggggggggggggggggggggggggggggggggggggggggggggg*/ x in exp /*pre-block*/ { y = x; };

/*pre-statement*/ for /*pre-cond Longerrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr*/ x in exp /*pre-block*/ { y = x; };

/*pre-statement*/ for // pre-cond - make sure that x and y states are the sameeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
x in /*post-innnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnn*/ exp //pre-block - when they are same change y to next stateeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
{ y = x +7; }
}
Loading