Skip to content

impl rewrite_result for ArmWrapper #6239

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

Merged
merged 1 commit into from
Jul 18, 2024
Merged

impl rewrite_result for ArmWrapper #6239

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
2 changes: 1 addition & 1 deletion src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ pub(crate) fn format_expr(
}
}
ast::ExprKind::Match(ref cond, ref arms, kind) => {
rewrite_match(context, cond, arms, shape, expr.span, &expr.attrs, kind)
rewrite_match(context, cond, arms, shape, expr.span, &expr.attrs, kind).ok()
}
ast::ExprKind::Path(ref qself, ref path) => {
rewrite_path(context, PathContext::Expr, qself, path, shape).ok()
Expand Down
104 changes: 59 additions & 45 deletions src/matches.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::expr::{
ExprType, RhsTactics,
};
use crate::lists::{itemize_list, write_list, ListFormatting};
use crate::rewrite::{Rewrite, RewriteContext};
use crate::rewrite::{Rewrite, RewriteContext, RewriteError, RewriteErrorExt, RewriteResult};
use crate::shape::Shape;
use crate::source_map::SpanUtils;
use crate::spanned::Spanned;
Expand Down Expand Up @@ -55,6 +55,10 @@ impl<'a> Spanned for ArmWrapper<'a> {

impl<'a> Rewrite for ArmWrapper<'a> {
fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option<String> {
self.rewrite_result(context, shape).ok()
}

fn rewrite_result(&self, context: &RewriteContext<'_>, shape: Shape) -> RewriteResult {
rewrite_match_arm(
context,
self.arm,
Expand All @@ -73,7 +77,7 @@ pub(crate) fn rewrite_match(
span: Span,
attrs: &[ast::Attribute],
match_kind: MatchKind,
) -> Option<String> {
) -> RewriteResult {
// Do not take the rhs overhead from the upper expressions into account
// when rewriting match condition.
let cond_shape = Shape {
Expand All @@ -82,10 +86,14 @@ pub(crate) fn rewrite_match(
};
// 6 = `match `
let cond_shape = match context.config.indent_style() {
IndentStyle::Visual => cond_shape.shrink_left(6)?,
IndentStyle::Block => cond_shape.offset_left(6)?,
IndentStyle::Visual => cond_shape
.shrink_left(6)
.max_width_error(shape.width, span)?,
IndentStyle::Block => cond_shape
.offset_left(6)
.max_width_error(shape.width, span)?,
};
let cond_str = cond.rewrite(context, cond_shape)?;
let cond_str = cond.rewrite_result(context, cond_shape)?;
let alt_block_sep = &shape.indent.to_string_with_newline(context.config);
let block_sep = match context.config.control_brace_style() {
ControlBraceStyle::AlwaysNextLine => alt_block_sep,
Expand All @@ -109,7 +117,7 @@ pub(crate) fn rewrite_match(
_ => shape.block_indent(context.config.tab_spaces()),
};
inner_attrs
.rewrite(context, shape)
.rewrite_result(context, shape)
.map(|s| format!("{}{}\n", nested_indent_str, s))?
};

Expand All @@ -129,16 +137,16 @@ pub(crate) fn rewrite_match(
if arms.is_empty() {
let snippet = context.snippet(mk_sp(open_brace_pos, span.hi() - BytePos(1)));
if snippet.trim().is_empty() {
Some(format!("match {cond_str} {{}}"))
Ok(format!("match {cond_str} {{}}"))
} else {
// Empty match with comments or inner attributes? We are not going to bother, sorry ;)
Some(context.snippet(span).to_owned())
Ok(context.snippet(span).to_owned())
}
} else {
let span_after_cond = mk_sp(cond.span.hi(), span.hi());

match match_kind {
MatchKind::Prefix => Some(format!(
MatchKind::Prefix => Ok(format!(
"match {}{}{{\n{}{}{}\n{}}}",
cond_str,
block_sep,
Expand All @@ -147,7 +155,7 @@ pub(crate) fn rewrite_match(
rewrite_match_arms(context, arms, shape, span_after_cond, open_brace_pos)?,
shape.indent.to_string(context.config),
)),
MatchKind::Postfix => Some(format!(
MatchKind::Postfix => Ok(format!(
"{}.match{}{{\n{}{}{}\n{}}}",
cond_str,
block_sep,
Expand Down Expand Up @@ -197,7 +205,7 @@ fn rewrite_match_arms(
shape: Shape,
span: Span,
open_brace_pos: BytePos,
) -> Option<String> {
) -> RewriteResult {
let arm_shape = shape
.block_indent(context.config.tab_spaces())
.with_max_width(context.config);
Expand Down Expand Up @@ -228,7 +236,7 @@ fn rewrite_match_arms(
.separator("")
.preserve_newline(true);

write_list(&arms_vec, &fmt)
write_list(&arms_vec, &fmt).unknown_error()
}

fn rewrite_match_arm(
Expand All @@ -237,19 +245,19 @@ fn rewrite_match_arm(
shape: Shape,
is_last: bool,
has_leading_pipe: bool,
) -> Option<String> {
) -> RewriteResult {
let (missing_span, attrs_str) = if !arm.attrs.is_empty() {
if contains_skip(&arm.attrs) {
let (_, body) = flatten_arm_body(context, arm.body.as_deref()?, None);
let (_, body) = flatten_arm_body(context, arm.body.as_deref().unknown_error()?, None);
// `arm.span()` does not include trailing comma, add it manually.
return Some(format!(
return Ok(format!(
"{}{}",
context.snippet(arm.span()),
arm_comma(context.config, body, is_last),
));
}
let missing_span = mk_sp(arm.attrs[arm.attrs.len() - 1].span.hi(), arm.pat.span.lo());
(missing_span, arm.attrs.rewrite(context, shape)?)
(missing_span, arm.attrs.rewrite_result(context, shape)?)
} else {
(mk_sp(arm.span().lo(), arm.span().lo()), String::new())
};
Expand All @@ -263,19 +271,25 @@ fn rewrite_match_arm(
};

// Patterns
let pat_shape = match &arm.body.as_ref()?.kind {
let pat_shape = match &arm.body.as_ref().unknown_error()?.kind {
ast::ExprKind::Block(_, Some(label)) => {
// Some block with a label ` => 'label: {`
// 7 = ` => : {`
let label_len = label.ident.as_str().len();
shape.sub_width(7 + label_len)?.offset_left(pipe_offset)?
shape
.sub_width(7 + label_len)
.and_then(|s| s.offset_left(pipe_offset))
.max_width_error(shape.width, arm.span)?
}
_ => {
// 5 = ` => {`
shape.sub_width(5)?.offset_left(pipe_offset)?
shape
.sub_width(5)
.and_then(|s| s.offset_left(pipe_offset))
.max_width_error(shape.width, arm.span)?
}
};
let pats_str = arm.pat.rewrite(context, pat_shape)?;
let pats_str = arm.pat.rewrite_result(context, pat_shape)?;

// Guard
let block_like_pat = trimmed_last_line_width(&pats_str) <= context.config.tab_spaces();
Expand All @@ -295,12 +309,16 @@ fn rewrite_match_arm(
missing_span,
shape,
false,
)?;
)
.unknown_error()?;

let arrow_span = mk_sp(arm.pat.span.hi(), arm.body.as_ref()?.span().lo());
let arrow_span = mk_sp(
arm.pat.span.hi(),
arm.body.as_ref().unknown_error()?.span().lo(),
);
rewrite_match_body(
context,
arm.body.as_ref()?,
arm.body.as_ref().unknown_error()?,
&lhs_str,
shape,
guard_str.contains('\n'),
Expand Down Expand Up @@ -381,7 +399,7 @@ fn rewrite_match_body(
has_guard: bool,
arrow_span: Span,
is_last: bool,
) -> Option<String> {
) -> RewriteResult {
let (extend, body) = flatten_arm_body(
context,
body,
Expand All @@ -402,7 +420,7 @@ fn rewrite_match_body(
_ => " ",
};

Some(format!("{} =>{}{}{}", pats_str, block_sep, body_str, comma))
Ok(format!("{} =>{}{}{}", pats_str, block_sep, body_str, comma))
};

let next_line_indent = if !is_block || is_empty_block {
Expand All @@ -429,7 +447,7 @@ fn rewrite_match_body(
if comment_str.is_empty() {
String::new()
} else {
rewrite_comment(comment_str, false, shape, context.config)?
rewrite_comment(comment_str, false, shape, context.config).unknown_error()?
}
};

Expand All @@ -446,7 +464,7 @@ fn rewrite_match_body(
result.push_str(&nested_indent_str);
result.push_str(body_str);
result.push_str(comma);
return Some(result);
return Ok(result);
}

let indent_str = shape.indent.to_string_with_newline(context.config);
Expand Down Expand Up @@ -489,7 +507,7 @@ fn rewrite_match_body(
result.push_str(&block_sep);
result.push_str(body_str);
result.push_str(&body_suffix);
Some(result)
Ok(result)
};

// Let's try and get the arm body on the same line as the condition.
Expand Down Expand Up @@ -539,7 +557,7 @@ fn rewrite_match_body(
combine_next_line_body(next_line_str)
}
(None, Some(ref next_line_str)) => combine_next_line_body(next_line_str),
(None, None) => None,
(None, None) => Err(RewriteError::Unknown),
(Some(ref orig_str), _) => combine_orig_body(orig_str),
}
}
Expand All @@ -553,7 +571,7 @@ fn rewrite_guard(
// the arm (excludes offset).
pattern_width: usize,
multiline_pattern: bool,
) -> Option<String> {
) -> RewriteResult {
if let Some(ref guard) = *guard {
// First try to fit the guard string on the same line as the pattern.
// 4 = ` if `, 5 = ` => {`
Expand All @@ -562,9 +580,9 @@ fn rewrite_guard(
.and_then(|s| s.sub_width(5));
if !multiline_pattern {
if let Some(cond_shape) = cond_shape {
if let Some(cond_str) = guard.rewrite(context, cond_shape) {
if let Ok(cond_str) = guard.rewrite_result(context, cond_shape) {
if !cond_str.contains('\n') || pattern_width <= context.config.tab_spaces() {
return Some(format!(" if {cond_str}"));
return Ok(format!(" if {cond_str}"));
}
}
}
Expand All @@ -574,20 +592,16 @@ fn rewrite_guard(
// 3 = `if `, 5 = ` => {`
let cond_shape = Shape::indented(shape.indent.block_indent(context.config), context.config)
.offset_left(3)
.and_then(|s| s.sub_width(5));
if let Some(cond_shape) = cond_shape {
if let Some(cond_str) = guard.rewrite(context, cond_shape) {
return Some(format!(
"{}if {}",
cond_shape.indent.to_string_with_newline(context.config),
cond_str
));
}
}

None
.and_then(|s| s.sub_width(5))
.max_width_error(shape.width, guard.span)?;
let cond_str = guard.rewrite_result(context, cond_shape)?;
Ok(format!(
"{}if {}",
cond_shape.indent.to_string_with_newline(context.config),
cond_str
))
} else {
Some(String::new())
Ok(String::new())
}
Comment on lines 593 to 605
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made some small refactoring here: we can return RewriteError early when cond_shape exceeds the max width and when it fails to rewrite the arm guard. The original source code returns Some(..) only when both conditions are not met.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. I think this change is sound

}

Expand Down
Loading