Skip to content

Handle special-case format! like macros #2219

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

Merged
merged 8 commits into from
Dec 3, 2017
Merged
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
3 changes: 1 addition & 2 deletions src/chains.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
};
debug!(
"child_shapes {:?} {:?}",
first_child_shape,
other_child_shape
first_child_shape, other_child_shape
);

let child_shape_iter = Some(first_child_shape)
Expand Down
3 changes: 1 addition & 2 deletions src/checkstyle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,7 @@ where
writer,
"<error line=\"{}\" severity=\"warning\" message=\"Should be `{}`\" \
/>",
mismatch.line_number,
message
mismatch.line_number, message
)?;
}
}
Expand Down
6 changes: 1 addition & 5 deletions src/comment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,7 @@ pub fn combine_strs_with_missing_comments(
};
Some(format!(
"{}{}{}{}{}",
prev_str,
first_sep,
missing_comment,
second_sep,
next_str,
prev_str, first_sep, missing_comment, second_sep, next_str,
))
}

Expand Down
3 changes: 1 addition & 2 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -714,8 +714,7 @@ mod test {
toml,
format!(
"merge_derives = {}\nskip_children = {}\n",
merge_derives,
skip_children,
merge_derives, skip_children,
)
);
}
Expand Down
138 changes: 108 additions & 30 deletions src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,10 +353,7 @@ where
if one_line_width <= shape.width {
return Some(format!(
"{}{}{}{}",
lhs_result,
pp.infix,
rhs_result,
pp.suffix
lhs_result, pp.infix, rhs_result, pp.suffix
));
}
}
Expand Down Expand Up @@ -390,10 +387,7 @@ where
};
Some(format!(
"{}{}{}{}",
lhs_result,
infix_with_sep,
rhs_result,
pp.suffix
lhs_result, infix_with_sep, rhs_result, pp.suffix
))
}

Expand Down Expand Up @@ -883,10 +877,7 @@ impl<'a> ControlFlow<'a> {

let result = format!(
"{} {} {{ {} }} else {{ {} }}",
self.keyword,
pat_expr_str,
if_str,
else_str
self.keyword, pat_expr_str, if_str, else_str
);

if result.len() <= width {
Expand Down Expand Up @@ -1589,10 +1580,7 @@ fn rewrite_match_body(

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

Expand Down Expand Up @@ -1807,6 +1795,25 @@ fn rewrite_string_lit(context: &RewriteContext, span: Span, shape: Shape) -> Opt
)
}

const FORMAT_LIKE_WHITELIST: &[&str] = &[
// From the Rust Standard Library.
"eprint!",
"eprintln!",
"format!",
"format_args!",
"panic!",
"println!",
"unreachable!",
Copy link
Member

Choose a reason for hiding this comment

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

Could we add debug, info, warn, trace, and error.

// From the `log` crate.
"debug!",
"error!",
"info!",
"panic!",
"warn!",
];

const WRITE_LIKE_WHITELIST: &[&str] = &["assert!", "write!", "writeln!"];

pub fn rewrite_call(
context: &RewriteContext,
callee: &str,
Expand Down Expand Up @@ -1850,9 +1857,6 @@ where
};
let used_width = extra_offset(callee_str, shape);
let one_line_width = shape.width.checked_sub(used_width + 2 * paren_overhead)?;
// 1 = "("
let combine_arg_with_callee =
callee_str.len() + 1 <= context.config.tab_spaces() && args.len() == 1;

// 1 = "(" or ")"
let one_line_shape = shape
Expand All @@ -1877,7 +1881,7 @@ where
one_line_width,
args_max_width,
force_trailing_comma,
combine_arg_with_callee,
callee_str,
)?;

if !context.use_block_indent() && need_block_indent(&list_str, nested_shape) && !extendable {
Expand Down Expand Up @@ -1918,7 +1922,7 @@ fn rewrite_call_args<'a, T>(
one_line_width: usize,
args_max_width: usize,
force_trailing_comma: bool,
combine_arg_with_callee: bool,
callee_str: &str,
) -> Option<(bool, String)>
where
T: Rewrite + Spanned + ToExpr + 'a,
Expand Down Expand Up @@ -1948,7 +1952,7 @@ where
nested_shape,
one_line_width,
args_max_width,
combine_arg_with_callee,
callee_str,
);

let fmt = ListFormatting {
Expand All @@ -1968,7 +1972,8 @@ where
config: context.config,
};

write_list(&item_vec, &fmt).map(|args_str| (tactic != DefinitiveListTactic::Vertical, args_str))
write_list(&item_vec, &fmt)
.map(|args_str| (tactic == DefinitiveListTactic::Horizontal, args_str))
}

fn try_overflow_last_arg<'a, T>(
Expand All @@ -1979,11 +1984,14 @@ fn try_overflow_last_arg<'a, T>(
nested_shape: Shape,
one_line_width: usize,
args_max_width: usize,
combine_arg_with_callee: bool,
callee_str: &str,
) -> DefinitiveListTactic
where
T: Rewrite + Spanned + ToExpr + 'a,
{
// 1 = "("
let combine_arg_with_callee =
callee_str.len() + 1 <= context.config.tab_spaces() && args.len() == 1;
let overflow_last = combine_arg_with_callee || can_be_overflowed(context, args);

// Replace the last item with its first line to see if it fits with
Expand Down Expand Up @@ -2020,6 +2028,16 @@ where
_ if args.len() >= 1 => {
item_vec[args.len() - 1].item = args.last()
.and_then(|last_arg| last_arg.rewrite(context, nested_shape));

let default_tactic = || {
definitive_tactic(
&*item_vec,
ListTactic::LimitedHorizontalVertical(args_max_width),
Separator::Comma,
one_line_width,
)
};

// Use horizontal layout for a function with a single argument as long as
// everything fits in a single line.
if args.len() == 1
Expand All @@ -2030,12 +2048,29 @@ where
{
tactic = DefinitiveListTactic::Horizontal;
} else {
tactic = definitive_tactic(
&*item_vec,
ListTactic::LimitedHorizontalVertical(args_max_width),
Separator::Comma,
one_line_width,
);
tactic = default_tactic();

// For special-case macros, we may want to use different tactics.
let maybe_args_offset = maybe_get_args_offset(callee_str, args);

if tactic == DefinitiveListTactic::Vertical && maybe_args_offset.is_some() {
let args_offset = maybe_args_offset.unwrap();
let args_tactic = definitive_tactic(
&item_vec[args_offset..],
ListTactic::HorizontalVertical,
Separator::Comma,
nested_shape.width,
);

// Every argument is simple and fits on a single line.
if args_tactic == DefinitiveListTactic::Horizontal {
tactic = if args_offset == 1 {
DefinitiveListTactic::FormatCall
} else {
DefinitiveListTactic::WriteCall
};
}
}
}
}
_ => (),
Expand All @@ -2044,6 +2079,49 @@ where
tactic
}

fn is_simple_arg(expr: &ast::Expr) -> bool {
match expr.node {
ast::ExprKind::Lit(..) => true,
ast::ExprKind::Path(ref qself, ref path) => qself.is_none() && path.segments.len() <= 1,
ast::ExprKind::AddrOf(_, ref expr)
| ast::ExprKind::Box(ref expr)
| ast::ExprKind::Cast(ref expr, _)
| ast::ExprKind::Field(ref expr, _)
| ast::ExprKind::Try(ref expr)
| ast::ExprKind::TupField(ref expr, _)
| ast::ExprKind::Unary(_, ref expr) => is_simple_arg(expr),
ast::ExprKind::Index(ref lhs, ref rhs) | ast::ExprKind::Repeat(ref lhs, ref rhs) => {
is_simple_arg(lhs) && is_simple_arg(rhs)
}
_ => false,
}
}

fn is_every_args_simple<T: ToExpr>(lists: &[&T]) -> bool {
lists
.iter()
.all(|arg| arg.to_expr().map_or(false, is_simple_arg))
}

/// In case special-case style is required, returns an offset from which we start horizontal layout.
fn maybe_get_args_offset<T: ToExpr>(callee_str: &str, args: &[&T]) -> Option<usize> {
if FORMAT_LIKE_WHITELIST
.iter()
.find(|s| **s == callee_str)
.is_some() && args.len() >= 1 && is_every_args_simple(args)
{
Some(1)
} else if WRITE_LIKE_WHITELIST
.iter()
.find(|s| **s == callee_str)
.is_some() && args.len() >= 2 && is_every_args_simple(args)
{
Some(2)
} else {
None
}
}

/// Returns a shape for the last argument which is going to be overflowed.
fn last_arg_shape<T>(
lists: &[&T],
Expand Down
12 changes: 3 additions & 9 deletions src/items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,7 @@ impl Rewrite for ast::Local {
fn rewrite(&self, context: &RewriteContext, shape: Shape) -> Option<String> {
debug!(
"Local::rewrite {:?} {} {:?}",
self,
shape.width,
shape.indent
self, shape.width, shape.indent
);

skip_out_of_file_lines_range!(context, self.span);
Expand Down Expand Up @@ -844,9 +842,7 @@ fn rewrite_trait_ref(
if !(retry && trait_ref_str.contains('\n')) {
return Some(format!(
"{} {}{}",
generics_str,
polarity_str,
&trait_ref_str
generics_str, polarity_str, &trait_ref_str
));
}
}
Expand Down Expand Up @@ -1842,9 +1838,7 @@ fn rewrite_fn_base(

debug!(
"rewrite_fn_base: one_line_budget: {}, multi_line_budget: {}, arg_indent: {:?}",
one_line_budget,
multi_line_budget,
arg_indent
one_line_budget, multi_line_budget, arg_indent
);

// Check if vertical layout was forced.
Expand Down
3 changes: 1 addition & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,7 @@ impl fmt::Display for ErrorKind {
ErrorKind::LineOverflow(found, maximum) => write!(
fmt,
"line exceeded maximum width (maximum: {}, found: {})",
maximum,
found
maximum, found
),
ErrorKind::TrailingWhitespace => write!(fmt, "left behind trailing whitespace"),
ErrorKind::BadIssue(issue) => write!(fmt, "found {}", issue),
Expand Down
28 changes: 27 additions & 1 deletion src/lists.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,10 @@ pub enum DefinitiveListTactic {
Vertical,
Horizontal,
Mixed,
// Special case tactic for `format!()` variants.
FormatCall,
// Special case tactic for `write!()` varianta.
WriteCall,
}

impl DefinitiveListTactic {
Expand Down Expand Up @@ -267,7 +271,7 @@ where
I: IntoIterator<Item = T> + Clone,
T: AsRef<ListItem>,
{
let tactic = formatting.tactic;
let mut tactic = formatting.tactic;
let sep_len = formatting.separator.len();

// Now that we know how we will layout, we can decide for sure if there
Expand Down Expand Up @@ -309,6 +313,28 @@ where
DefinitiveListTactic::Horizontal if !first => {
result.push(' ');
}
DefinitiveListTactic::FormatCall if !first => {
result.push('\n');
result.push_str(indent_str);
tactic = DefinitiveListTactic::Horizontal;
}
DefinitiveListTactic::WriteCall => {
let second = i == 1;
let third = i == 2;

if first {
// Nothing
} else if second {
result.push('\n');
result.push_str(indent_str);
} else if third {
result.push('\n');
result.push_str(indent_str);
tactic = DefinitiveListTactic::Horizontal;
} else {
unreachable!();
}
}
DefinitiveListTactic::Vertical if !first => {
result.push('\n');
result.push_str(indent_str);
Expand Down
5 changes: 1 addition & 4 deletions src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -442,10 +442,7 @@ impl Rewrite for ast::WherePredicate {
{
format!(
"for< {} > {}{}{}",
lifetime_str,
type_str,
colon,
bounds_str
lifetime_str, type_str, colon, bounds_str
)
} else {
format!("for<{}> {}{}{}", lifetime_str, type_str, colon, bounds_str)
Expand Down
8 changes: 8 additions & 0 deletions tests/source/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,3 +216,11 @@ make_test!(str_searcher_ascii_haystack, "bb", "abbcbbd", [
Reject(6, 7),
]);
}

fn special_case_macros() {
// format!
let s = format!("Arr! While plunderin' the hold, we got '{}' when given '{}' (we expected '{}')", result, input, expected);

// assert!
assert!(result, "Arr! While plunderin' the hold, we got '{}' when given '{}' (we expected '{}')", result, input, expected);
}
Loading