Skip to content

Commit

Permalink
Formatting pre and post cast comments enhancements (#4406)
Browse files Browse the repository at this point in the history
* Fix issues in handling cast pre/post comments and adding test cases

* Add target for cast pre/post comments test cases

* Add test cases related to #2896 and #3528

* Changes per comments to the original PR
  • Loading branch information
davidBar-On committed Sep 6, 2020
1 parent 4e72871 commit 6646248
Show file tree
Hide file tree
Showing 5 changed files with 289 additions and 25 deletions.
38 changes: 29 additions & 9 deletions src/formatting/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,14 +219,34 @@ pub(crate) fn format_expr(
ast::ExprKind::AddrOf(borrow_kind, mutability, ref expr) => {
rewrite_expr_addrof(context, borrow_kind, mutability, expr, shape)
}
ast::ExprKind::Cast(ref expr, ref ty) => rewrite_pair(
&**expr,
&**ty,
PairParts::infix(" as "),
context,
shape,
SeparatorPlace::Front,
),
ast::ExprKind::Cast(ref subexpr, ref ty) => {
/* Retrieving the comments before and after cast */
let prefix_span = mk_sp(
subexpr.span.hi(),
context.snippet_provider.span_before(expr.span, "as"),
);
let suffix_span = mk_sp(
context.snippet_provider.span_after(expr.span, "as"),
ty.span.lo(),
);
let infix_prefix_comments = rewrite_missing_comment(prefix_span, shape, context)?;
let infix_suffix_comments = rewrite_missing_comment(suffix_span, shape, context)?;

rewrite_pair(
&**subexpr,
&**ty,
PairParts::new(
"",
&infix_prefix_comments,
" as ",
&infix_suffix_comments,
"",
),
context,
shape,
SeparatorPlace::Front,
)
}
ast::ExprKind::Type(ref expr, ref ty) => rewrite_pair(
&**expr,
&**ty,
Expand All @@ -241,7 +261,7 @@ pub(crate) fn format_expr(
ast::ExprKind::Repeat(ref expr, ref repeats) => rewrite_pair(
&**expr,
&*repeats.value,
PairParts::new("[", "; ", "]"),
PairParts::new("[", "", "; ", "", "]"),
context,
shape,
SeparatorPlace::Back,
Expand Down
77 changes: 62 additions & 15 deletions src/formatting/pairs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,36 @@ use crate::formatting::{
#[derive(Clone, Copy)]
pub(crate) struct PairParts<'a> {
prefix: &'a str,
infix_prefix: &'a str, /* mainly for pre-infix comments */
infix: &'a str,
infix_suffix: &'a str, /* mainly for post-infix comments */
suffix: &'a str,
}

impl<'a> PairParts<'a> {
/// Constructs a new `PairParts`.
pub(crate) fn new(prefix: &'a str, infix: &'a str, suffix: &'a str) -> Self {
Self {
pub(crate) fn new(
prefix: &'a str,
infix_prefix: &'a str,
infix: &'a str,
infix_suffix: &'a str,
suffix: &'a str,
) -> Self {
PairParts {
prefix,
infix_prefix,
infix,
infix_suffix,
suffix,
}
}

pub(crate) fn infix(infix: &'a str) -> PairParts<'a> {
PairParts {
prefix: "",
infix_prefix: "",
infix,
infix_suffix: "",
suffix: "",
}
}
Expand Down Expand Up @@ -172,22 +184,32 @@ where
RHS: Rewrite,
{
let tab_spaces = context.config.tab_spaces();
let infix_result = format!("{}{}", pp.infix, pp.infix_suffix);
let infix_suffix_separator = if pp.infix_suffix.is_empty() { "" } else { " " };
let infix_prefix_separator = if pp.infix_prefix.is_empty() { "" } else { " " };
let lhs_overhead = match separator_place {
SeparatorPlace::Back => shape.used_width() + pp.prefix.len() + pp.infix.trim_end().len(),
SeparatorPlace::Back => {
shape.used_width() + pp.prefix.len() + pp.infix.trim_end().len() + pp.infix_prefix.len()
}
SeparatorPlace::Front => shape.used_width(),
};
let lhs_shape = Shape {
width: context.budget(lhs_overhead),
..shape
};
let lhs_result = lhs
.rewrite(context, lhs_shape)
.map(|lhs_str| format!("{}{}", pp.prefix, lhs_str))?;
let lhs_result = lhs.rewrite(context, lhs_shape).map(|lhs_str| {
format!(
"{}{}{}{}",
pp.prefix, lhs_str, infix_prefix_separator, pp.infix_prefix
)
})?;

// Try to put both lhs and rhs on the same line.
let rhs_orig_result = shape
.offset_left(last_line_width(&lhs_result) + pp.infix.len())
.and_then(|s| s.sub_width(pp.suffix.len()))
.and_then(|s| {
s.sub_width(pp.suffix.len() + pp.infix_suffix.len() + infix_suffix_separator.len())
})
.and_then(|rhs_shape| rhs.rewrite(context, rhs_shape));
if let Some(ref rhs_result) = rhs_orig_result {
// If the length of the lhs is equal to or shorter than the tab width or
Expand All @@ -201,13 +223,13 @@ where
.unwrap_or(false);
if !rhs_result.contains('\n') || allow_same_line {
let one_line_width = last_line_width(&lhs_result)
+ pp.infix.len()
+ infix_result.len()
+ first_line_width(rhs_result)
+ pp.suffix.len();
if one_line_width <= shape.width {
return Some(format!(
"{}{}{}{}",
lhs_result, pp.infix, rhs_result, pp.suffix
"{}{}{}{}{}",
lhs_result, infix_result, infix_suffix_separator, rhs_result, pp.suffix
));
}
}
Expand All @@ -228,20 +250,45 @@ where
};
let infix = match separator_place {
SeparatorPlace::Back => pp.infix.trim_end(),
SeparatorPlace::Front => pp.infix.trim_start(),
SeparatorPlace::Front => {
if pp.infix_suffix.is_empty() {
pp.infix.trim_start()
} else {
pp.infix
}
}
};
let infix_suffix = if separator_place == SeparatorPlace::Front && !pp.infix_suffix.is_empty() {
pp.infix_suffix.trim_start()
} else {
pp.infix_suffix
};
if separator_place == SeparatorPlace::Front {
rhs_shape = rhs_shape.offset_left(infix.len())?;
}
let rhs_result = rhs.rewrite(context, rhs_shape)?;
let indent_str = rhs_shape.indent.to_string_with_newline(context.config);
let infix_with_sep = match separator_place {
SeparatorPlace::Back => format!("{}{}", infix, indent_str),
SeparatorPlace::Front => format!("{}{}", indent_str, infix),
let mut infix_with_sep = match separator_place {
SeparatorPlace::Back => format!("{}{}{}", infix, infix_suffix.trim_end(), indent_str),
SeparatorPlace::Front => format!(
"{}{}{}{}",
indent_str,
infix.trim_start(),
infix_suffix,
infix_suffix_separator
),
};
let new_line_width = infix_with_sep.len() - 1 + rhs_result.len() + pp.suffix.len();
let rhs_with_sep = if separator_place == SeparatorPlace::Front && new_line_width > shape.width {
let s: String = String::from(infix_with_sep);
infix_with_sep = s.trim_end().to_string();
format!("{}{}", indent_str, rhs_result.trim_start())
} else {
rhs_result
};
Some(format!(
"{}{}{}{}",
lhs_result, infix_with_sep, rhs_result, pp.suffix
lhs_result, infix_with_sep, rhs_with_sep, pp.suffix
))
}

Expand Down
2 changes: 1 addition & 1 deletion src/formatting/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ impl Rewrite for ast::Ty {
ast::TyKind::Array(ref ty, ref repeats) => rewrite_pair(
&**ty,
&*repeats.value,
PairParts::new("[", "; ", "]"),
PairParts::new("[", "", "; ", "", "]"),
context,
shape,
SeparatorPlace::Back,
Expand Down
98 changes: 98 additions & 0 deletions tests/source/issue-4406-cast-comments.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*****
* Tests for proper formatting of pre and post cast ("as") comments
******/

// Test 1
fn main() {
let x = 0f64 /* x as */ as i32;
}

// Test 2
fn main() {
let x = 1 /* foo as */ as i32;
}

// Test 3
fn main() {
let x = 1 as /* bar as */ i32;
}

// Test 4
fn main() {
let x = 1 /* as foo */ as /* as bar */ i32;
}

// Test 5
fn main() {
let x = 1 /* as foo */as/* as bar */ i32;
}

// Test 6
fn main() {
let x = 1 /* as foo */
as/* as bar */
i32;
}

// Test 7
fn main() {
let x = 1 /* as foo yyyyyyyyyyy */as/* as bar xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx*/ i32;
}

// Test 8
fn main() {
let x = 1 /* as foo yyyyyyyyyyy */as/* as bar xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx*/ i32;
}

// Test 9
fn main() {
let x = 1 /* as foo yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy */
as/* as bar xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx*/
i32;
}


/*****
* Tests for not leaving trailing spaces related to cast comments (related to #2896?)
******/
// Test 10 - extra blank after the binary rhs at the 2nd line (comment followws at 3rd line)
fn main() {
if 0 == 1
/* x */ as i32 {} }

// Test 11 - extra blank after the binary rhs at the end of 2nd line
fn main() {
if 0 == ' '
as i32 {} }

// Test 12 - extra blank after the comment at the end of 2nd line
fn main() {
if 0 == ' ' /* x */
as i32 {} }


/*****
* Tests for not moving "as" to new line unnecessarily - from #3528
******/
fn get_old_backends(old_toml_config: &toml::Value) -> Option<Vec<Box<dyn Backend>>> {
old_toml_config.as_table().and_then(|table| {
table
.get("backends")
.and_then(|backends| backends.as_table())
.map(|backends| {
backends
.into_iter()
.filter_map(|(key, value)| match AvailableBackend::from(key.as_str()) {
AvailableBackend::Git => Some(Box::new(Git {
config: value.clone().try_into::<GitConfig>().unwrap(),
})
as Box<dyn Backend>),
AvailableBackend::Github => Some(Box::new(Github {
config: value.clone().try_into::<GithubConfig>().unwrap(),
})
as Box<dyn Backend>),
})
.collect()
})
})
}
Loading

0 comments on commit 6646248

Please sign in to comment.