From 72cac8beae1edceef7e4d504b9a5a4dad4dd2b84 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Fri, 1 Dec 2017 13:28:16 +0900 Subject: [PATCH 1/8] Add a test for special case macros like format! and assert! --- tests/source/macros.rs | 8 ++++++++ tests/target/macros.rs | 15 +++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/tests/source/macros.rs b/tests/source/macros.rs index c8f76906254..ff24939668c 100644 --- a/tests/source/macros.rs +++ b/tests/source/macros.rs @@ -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); +} diff --git a/tests/target/macros.rs b/tests/target/macros.rs index 25156ff21fd..8230ae91161 100644 --- a/tests/target/macros.rs +++ b/tests/target/macros.rs @@ -270,3 +270,18 @@ fn issue2214() { ] ); } + +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 + ); +} From ffbe52eb7638f6647d09564d44314b9bf8d76836 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Fri, 1 Dec 2017 13:28:36 +0900 Subject: [PATCH 2/8] Add whitelists of macros that need special-case format --- src/expr.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/expr.rs b/src/expr.rs index afa31983046..ce2ea65b65a 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -1807,6 +1807,18 @@ fn rewrite_string_lit(context: &RewriteContext, span: Span, shape: Shape) -> Opt ) } +const FORMAT_LIKE_WHITELIST: &[&str] = &[ + "eprint!", + "eprintln!", + "format!", + "format_args!", + "panic!", + "println!", + "unreachable!", +]; + +const WRITE_LIKE_WHITELIST: &[&str] = &["assert!", "write!", "writeln!"]; + pub fn rewrite_call( context: &RewriteContext, callee: &str, From 0f5dcc665da4da6af0532bf1ca5e96ac4c9f6286 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Fri, 1 Dec 2017 13:30:04 +0900 Subject: [PATCH 3/8] Handle special-case macros --- src/expr.rs | 95 ++++++++++++++++++++++++++++++++++++++++++++-------- src/lists.rs | 28 +++++++++++++++- 2 files changed, 108 insertions(+), 15 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index ce2ea65b65a..70363f7dd23 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -1862,9 +1862,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 @@ -1889,7 +1886,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 { @@ -1930,7 +1927,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, @@ -1960,7 +1957,7 @@ where nested_shape, one_line_width, args_max_width, - combine_arg_with_callee, + callee_str, ); let fmt = ListFormatting { @@ -1980,7 +1977,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>( @@ -1991,11 +1989,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 @@ -2032,6 +2033,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 @@ -2042,12 +2053,44 @@ where { tactic = DefinitiveListTactic::Horizontal; } else { - tactic = definitive_tactic( - &*item_vec, - ListTactic::LimitedHorizontalVertical(args_max_width), - Separator::Comma, - one_line_width, - ); + tactic = default_tactic(); + let is_simple_enough = + tactic == DefinitiveListTactic::Vertical && is_every_args_simple(args); + if is_simple_enough + && FORMAT_LIKE_WHITELIST + .iter() + .find(|s| **s == callee_str) + .is_some() + { + let args_tactic = definitive_tactic( + &item_vec[1..], + ListTactic::HorizontalVertical, + Separator::Comma, + nested_shape.width, + ); + tactic = if args_tactic == DefinitiveListTactic::Horizontal { + DefinitiveListTactic::FormatCall + } else { + default_tactic() + }; + } else if is_simple_enough && item_vec.len() >= 2 + && WRITE_LIKE_WHITELIST + .iter() + .find(|s| **s == callee_str) + .is_some() + { + let args_tactic = definitive_tactic( + &item_vec[2..], + ListTactic::HorizontalVertical, + Separator::Comma, + nested_shape.width, + ); + tactic = if args_tactic == DefinitiveListTactic::Horizontal { + DefinitiveListTactic::WriteCall + } else { + default_tactic() + }; + } } } _ => (), @@ -2056,6 +2099,30 @@ 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(lists: &[&T]) -> bool { + lists + .iter() + .all(|arg| arg.to_expr().map_or(false, is_simple_arg)) +} + /// Returns a shape for the last argument which is going to be overflowed. fn last_arg_shape( lists: &[&T], diff --git a/src/lists.rs b/src/lists.rs index f3e1c362dc1..a51653b3732 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -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 { @@ -267,7 +271,7 @@ where I: IntoIterator + Clone, T: AsRef, { - 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 @@ -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); From 16184d3e165018fa5b78f5f99cfc844e5af33bd6 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Fri, 1 Dec 2017 13:30:21 +0900 Subject: [PATCH 4/8] Cargo fmt and update a test --- src/checkstyle.rs | 3 +-- src/comment.rs | 6 +----- src/config.rs | 3 +-- src/expr.rs | 20 ++++---------------- src/items.rs | 4 +--- src/lib.rs | 3 +-- src/types.rs | 5 +---- tests/target/macros.rs | 3 +-- 8 files changed, 11 insertions(+), 36 deletions(-) diff --git a/src/checkstyle.rs b/src/checkstyle.rs index 34c1d81d40c..cbb6573b322 100644 --- a/src/checkstyle.rs +++ b/src/checkstyle.rs @@ -57,8 +57,7 @@ where writer, "", - mismatch.line_number, - message + mismatch.line_number, message )?; } } diff --git a/src/comment.rs b/src/comment.rs index 9b9056c4170..8b8112618b4 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -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, )) } diff --git a/src/config.rs b/src/config.rs index 2b4665729c4..d7b5e8c6ee3 100644 --- a/src/config.rs +++ b/src/config.rs @@ -714,8 +714,7 @@ mod test { toml, format!( "merge_derives = {}\nskip_children = {}\n", - merge_derives, - skip_children, + merge_derives, skip_children, ) ); } diff --git a/src/expr.rs b/src/expr.rs index 70363f7dd23..0215975f9a3 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -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 )); } } @@ -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 )) } @@ -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 { @@ -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 )) }; diff --git a/src/items.rs b/src/items.rs index ce0439b127b..b4fd5a340bc 100644 --- a/src/items.rs +++ b/src/items.rs @@ -844,9 +844,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 )); } } diff --git a/src/lib.rs b/src/lib.rs index 1eec30833b9..0b54c71a6eb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -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), diff --git a/src/types.rs b/src/types.rs index 92a1adbbc8e..ef185a3f7d4 100644 --- a/src/types.rs +++ b/src/types.rs @@ -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) diff --git a/tests/target/macros.rs b/tests/target/macros.rs index 8230ae91161..8b22a457463 100644 --- a/tests/target/macros.rs +++ b/tests/target/macros.rs @@ -135,8 +135,7 @@ fn issue_1279() { fn issue_1555() { let hello = &format!( "HTTP/1.1 200 OK\r\nServer: {}\r\n\r\n{}", - "65454654654654654654654655464", - "4" + "65454654654654654654654655464", "4" ); } From 27a540db4709307928f5f5ca0e4b7f3e1ac947c9 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Fri, 1 Dec 2017 13:44:40 +0900 Subject: [PATCH 5/8] Factor out a mess --- src/expr.rs | 70 +++++++++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 31 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index 0215975f9a3..2c878fe16b5 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -2042,42 +2042,27 @@ where tactic = DefinitiveListTactic::Horizontal; } else { tactic = default_tactic(); - let is_simple_enough = - tactic == DefinitiveListTactic::Vertical && is_every_args_simple(args); - if is_simple_enough - && FORMAT_LIKE_WHITELIST - .iter() - .find(|s| **s == callee_str) - .is_some() - { + + // 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[1..], + &item_vec[args_offset..], ListTactic::HorizontalVertical, Separator::Comma, nested_shape.width, ); - tactic = if args_tactic == DefinitiveListTactic::Horizontal { - DefinitiveListTactic::FormatCall - } else { - default_tactic() - }; - } else if is_simple_enough && item_vec.len() >= 2 - && WRITE_LIKE_WHITELIST - .iter() - .find(|s| **s == callee_str) - .is_some() - { - let args_tactic = definitive_tactic( - &item_vec[2..], - ListTactic::HorizontalVertical, - Separator::Comma, - nested_shape.width, - ); - tactic = if args_tactic == DefinitiveListTactic::Horizontal { - DefinitiveListTactic::WriteCall - } else { - default_tactic() - }; + + // 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 + }; + } } } } @@ -2111,6 +2096,29 @@ fn is_every_args_simple(lists: &[&T]) -> bool { .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(callee_str: &str, args: &[&T]) -> Option { + 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( lists: &[&T], From 8f395bd953ad856b03a98836268ca01bebc10733 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Fri, 1 Dec 2017 19:47:56 +0900 Subject: [PATCH 6/8] Cargo fmt --- src/expr.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index 2c878fe16b5..e82d23b21fd 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -2101,17 +2101,13 @@ fn maybe_get_args_offset(callee_str: &str, args: &[&T]) -> Option= 1 - && is_every_args_simple(args) + .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) + .is_some() && args.len() >= 2 && is_every_args_simple(args) { Some(2) } else { From aeb33986b1a5a509bd036e5ee105495b4e894797 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Sun, 3 Dec 2017 11:37:55 +0900 Subject: [PATCH 7/8] Add macros from the log crate to whitelist --- src/expr.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/expr.rs b/src/expr.rs index e82d23b21fd..e3b06b343fe 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -1796,6 +1796,7 @@ fn rewrite_string_lit(context: &RewriteContext, span: Span, shape: Shape) -> Opt } const FORMAT_LIKE_WHITELIST: &[&str] = &[ + // From the Rust Standard Library. "eprint!", "eprintln!", "format!", @@ -1803,6 +1804,12 @@ const FORMAT_LIKE_WHITELIST: &[&str] = &[ "panic!", "println!", "unreachable!", + // From the `log` crate. + "debug!", + "error!", + "info!", + "panic!", + "warn!", ]; const WRITE_LIKE_WHITELIST: &[&str] = &["assert!", "write!", "writeln!"]; From 026c716168963f0fa8739112f91cea3354534edc Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Sun, 3 Dec 2017 11:38:16 +0900 Subject: [PATCH 8/8] Cargo fmt --- src/chains.rs | 3 +-- src/items.rs | 8 ++------ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 5aebc160750..642fb93c3ba 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -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) diff --git a/src/items.rs b/src/items.rs index b4fd5a340bc..a198d93ca76 100644 --- a/src/items.rs +++ b/src/items.rs @@ -48,9 +48,7 @@ impl Rewrite for ast::Local { fn rewrite(&self, context: &RewriteContext, shape: Shape) -> Option { debug!( "Local::rewrite {:?} {} {:?}", - self, - shape.width, - shape.indent + self, shape.width, shape.indent ); skip_out_of_file_lines_range!(context, self.span); @@ -1840,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.