Merge pull request #2219 from topecongiro/issue-549

Handle special-case format! like macros

Author: nrc

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)

src/checkstyle.rs

@@ -57,8 +57,7 @@ where
                     writer,
                     "<error line=\"{}\" severity=\"warning\" message=\"Should be `{}`\" \
                      />",
-                    mismatch.line_number,
-                    message
+                    mismatch.line_number, message
                 )?;
             }
         }

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,
     ))
 }
 

src/config.rs

@@ -714,8 +714,7 @@ mod test {
             toml,
             format!(
                 "merge_derives = {}\nskip_children = {}\n",
-                merge_derives,
-                skip_children,
+                merge_derives, skip_children,
             )
         );
     }

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
         ))
     };
 
@@ -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!",
+    // 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,
@@ -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
@@ -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 {
@@ -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,
@@ -1948,7 +1952,7 @@ where
         nested_shape,
         one_line_width,
         args_max_width,
-        combine_arg_with_callee,
+        callee_str,
     );
 
     let fmt = ListFormatting {
@@ -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>(
@@ -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
@@ -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
@@ -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
+                        };
+                    }
+                }
             }
         }
         _ => (),
@@ -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],

src/items.rs

@@ -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);
@@ -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
             ));
         }
     }
@@ -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.

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),

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<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
@@ -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);

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)

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);
+}