Merge pull request #186 from marcusklaas/closures

Format closures

src/config.rs

@@ -78,4 +78,5 @@ create_config! {
     report_fixme: ReportTactic,
     reorder_imports: bool, // Alphabetically, case sensitive.
     expr_indent_style: BlockIndentStyle,
+    closure_indent_style: BlockIndentStyle,
 }

src/default.toml

@@ -14,3 +14,4 @@ report_todo = "Always"
 report_fixme = "Never"
 reorder_imports = false
 expr_indent_style = "Tabbed"
+closure_indent_style = "Visual"

src/expr.rs

@@ -17,6 +17,7 @@ use visitor::FmtVisitor;
 use config::BlockIndentStyle;
 use comment::{FindUncommented, rewrite_comment};
 use types::rewrite_path;
+use items::{span_lo_for_arg, span_hi_for_arg, rewrite_fn_input};
 
 use syntax::{ast, ptr};
 use syntax::codemap::{Pos, Span, BytePos, mk_sp};
@@ -115,11 +116,86 @@ impl Rewrite for ast::Expr {
                 };
                 Some(format!("continue{}", id_str))
             }
+            ast::Expr_::ExprClosure(capture, ref fn_decl, ref body) => {
+                rewrite_closure(capture, fn_decl, body, self.span, context, width, offset)
+            }
             _ => context.codemap.span_to_snippet(self.span).ok(),
         }
     }
 }
 
+// This functions is pretty messy because of the wrapping and unwrapping of
+// expressions into and from blocks. See rust issue #27872.
+fn rewrite_closure(capture: ast::CaptureClause,
+                   fn_decl: &ast::FnDecl,
+                   body: &ast::Block,
+                   span: Span,
+                   context: &RewriteContext,
+                   width: usize,
+                   offset: usize)
+                   -> Option<String> {
+    let mover = if capture == ast::CaptureClause::CaptureByValue {
+        "move "
+    } else {
+        ""
+    };
+    let offset = offset + mover.len();
+
+    // 4 = "|| {".len(), which is overconservative when the closure consists of
+    // a single expression.
+    let argument_budget = try_opt!(width.checked_sub(4 + mover.len()));
+    // 1 = |
+    let argument_offset = offset + 1;
+
+    let arg_items = itemize_list(context.codemap,
+                                 fn_decl.inputs.iter(),
+                                 "|",
+                                 |arg| span_lo_for_arg(arg),
+                                 |arg| span_hi_for_arg(arg),
+                                 |arg| rewrite_fn_input(arg),
+                                 span_after(span, "|", context.codemap),
+                                 body.span.lo);
+
+    let fmt = ListFormatting::for_fn(argument_budget, argument_offset);
+    let prefix = format!("{}|{}|", mover, write_list(&arg_items.collect::<Vec<_>>(), &fmt));
+    let block_indent = closure_block_indent(context, offset);
+
+    // Try to format closure body as a single line expression without braces.
+    if body.stmts.is_empty() {
+        let expr = body.expr.as_ref().unwrap();
+        // All closure bodies are blocks in the eyes of the AST, but we may not
+        // want to unwrap them when they only contain a single expression.
+        let inner_expr = match expr.node {
+            ast::Expr_::ExprBlock(ref inner) if inner.stmts.is_empty() && inner.expr.is_some() => {
+                inner.expr.as_ref().unwrap()
+            }
+            _ => expr,
+        };
+
+        // 1 = the separating space between arguments and the body.
+        let extra_offset = extra_offset(&prefix, offset) + 1;
+        let rewrite = inner_expr.rewrite(context, width - extra_offset, offset + extra_offset);
+
+        // Checks if rewrite succeeded and fits on a single line.
+        let accept_rewrite = rewrite.as_ref().map(|result| !result.contains('\n')).unwrap_or(false);
+
+        if accept_rewrite {
+            return Some(format!("{} {}", prefix, rewrite.unwrap()));
+        }
+    }
+
+    // We couldn't format the closure body as a single line expression; fall
+    // back to block formatting.
+    let inner_context = &RewriteContext { block_indent: block_indent, ..*context };
+    let body_rewrite = if let ast::Expr_::ExprBlock(ref inner) = body.expr.as_ref().unwrap().node {
+        inner.rewrite(inner_context, 0, 0)
+    } else {
+        body.rewrite(inner_context, 0, 0)
+    };
+
+    Some(format!("{} {}", prefix, try_opt!(body_rewrite)))
+}
+
 impl Rewrite for ast::Block {
     fn rewrite(&self, context: &RewriteContext, width: usize, offset: usize) -> Option<String> {
         let user_str = context.codemap.span_to_snippet(self.span).unwrap();
@@ -674,33 +750,33 @@ fn rewrite_call(context: &RewriteContext,
                              |item| item.span.lo,
                              |item| item.span.hi,
                              // Take old span when rewrite fails.
-                             |item| item.rewrite(inner_context, remaining_width, offset)
-                                        .unwrap_or(context.codemap.span_to_snippet(item.span)
-                                                                  .unwrap()),
+                             |item| {
+                                 item.rewrite(inner_context, remaining_width, offset)
+                                     .unwrap_or(context.codemap.span_to_snippet(item.span).unwrap())
+                             },
                              callee.span.hi + BytePos(1),
                              span.hi);
 
-    let fmt = ListFormatting {
-        tactic: ListTactic::HorizontalVertical,
-        separator: ",",
-        trailing_separator: SeparatorTactic::Never,
-        indent: offset,
-        h_width: remaining_width,
-        v_width: remaining_width,
-        ends_with_newline: false,
-    };
+    let fmt = ListFormatting::for_fn(remaining_width, offset);
 
     Some(format!("{}({})", callee_str, write_list(&items.collect::<Vec<_>>(), &fmt)))
 }
 
-fn expr_block_indent(context: &RewriteContext, offset: usize) -> usize {
-    match context.config.expr_indent_style {
-        BlockIndentStyle::Inherit => context.block_indent,
-        BlockIndentStyle::Tabbed => context.block_indent + context.config.tab_spaces,
-        BlockIndentStyle::Visual => offset,
-    }
+macro_rules! block_indent_helper {
+    ($name:ident, $option:ident) => (
+        fn $name(context: &RewriteContext, offset: usize) -> usize {
+            match context.config.$option {
+                BlockIndentStyle::Inherit => context.block_indent,
+                BlockIndentStyle::Tabbed => context.block_indent + context.config.tab_spaces,
+                BlockIndentStyle::Visual => offset,
+            }
+        }
+    );
 }
 
+block_indent_helper!(expr_block_indent, expr_indent_style);
+block_indent_helper!(closure_block_indent, closure_indent_style);
+
 fn rewrite_paren(context: &RewriteContext,
                  subexpr: &ast::Expr,
                  width: usize,
@@ -760,13 +836,13 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext,
                                  match *item {
                                      StructLitField::Regular(ref field) => field.span.lo,
                                      // 2 = ..
-                                     StructLitField::Base(ref expr) => expr.span.lo - BytePos(2)
+                                     StructLitField::Base(ref expr) => expr.span.lo - BytePos(2),
                                  }
                              },
                              |item| {
                                  match *item {
                                      StructLitField::Regular(ref field) => field.span.hi,
-                                     StructLitField::Base(ref expr) => expr.span.hi
+                                     StructLitField::Base(ref expr) => expr.span.hi,
                                  }
                              },
                              |item| {
@@ -775,7 +851,7 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext,
                                          rewrite_field(inner_context, &field, h_budget, indent)
                                             .unwrap_or(context.codemap.span_to_snippet(field.span)
                                                                       .unwrap())
-                                     },
+                                     }
                                      StructLitField::Base(ref expr) => {
                                          // 2 = ..
                                          expr.rewrite(inner_context, h_budget - 2, indent + 2)
@@ -846,23 +922,15 @@ fn rewrite_tuple_lit(context: &RewriteContext,
                              ")",
                              |item| item.span.lo,
                              |item| item.span.hi,
-                             |item| item.rewrite(context,
-                                                 context.config.max_width - indent - 1,
-                                                 indent)
-                                        .unwrap_or(context.codemap.span_to_snippet(item.span)
-                                                                  .unwrap()),
+                             |item| {
+                                 let inner_width = context.config.max_width - indent - 1;
+                                 item.rewrite(context, inner_width, indent)
+                                     .unwrap_or(context.codemap.span_to_snippet(item.span).unwrap())
+                             },
                              span.lo + BytePos(1), // Remove parens
                              span.hi - BytePos(1));
 
-    let fmt = ListFormatting {
-        tactic: ListTactic::HorizontalVertical,
-        separator: ",",
-        trailing_separator: SeparatorTactic::Never,
-        indent: indent,
-        h_width: width - 2,
-        v_width: width - 2,
-        ends_with_newline: false,
-    };
+    let fmt = ListFormatting::for_fn(width - 2, indent);
 
     Some(format!("({})", write_list(&items.collect::<Vec<_>>(), &fmt)))
 }

src/imports.rs

@@ -117,13 +117,15 @@ pub fn rewrite_use_list(width: usize,
                                 "}",
                                 |vpi| vpi.span.lo,
                                 |vpi| vpi.span.hi,
-                                |vpi| match vpi.node {
-                                     ast::PathListItem_::PathListIdent{ name, .. } => {
-                                         name.to_string()
-                                     }
-                                     ast::PathListItem_::PathListMod{ .. } => {
-                                         "self".to_owned()
-                                     }
+                                |vpi| {
+                                    match vpi.node {
+                                        ast::PathListItem_::PathListIdent{ name, .. } => {
+                                            name.to_string()
+                                        }
+                                        ast::PathListItem_::PathListMod{ .. } => {
+                                            "self".to_owned()
+                                        }
+                                    }
                                 },
                                 span_after(span, "{", context.codemap),
                                 span.hi);

src/items.rs

@@ -232,7 +232,7 @@ impl<'a> FmtVisitor<'a> {
                     arg_indent: usize,
                     span: Span)
                     -> String {
-        let mut arg_item_strs: Vec<_> = args.iter().map(|a| self.rewrite_fn_input(a)).collect();
+        let mut arg_item_strs: Vec<_> = args.iter().map(rewrite_fn_input).collect();
         // Account for sugary self.
         // FIXME: the comment for the self argument is dropped. This is blocked
         // on rust issue #27522.
@@ -512,13 +512,12 @@ impl<'a> FmtVisitor<'a> {
                                  struct_def.fields.iter(),
                                  terminator,
                                  |field| {
-                                      // Include attributes and doc comments,
-                                      // if present
-                                      if field.node.attrs.len() > 0 {
-                                          field.node.attrs[0].span.lo
-                                      } else {
-                                          field.span.lo
-                                      }
+                                     // Include attributes and doc comments, if present
+                                     if field.node.attrs.len() > 0 {
+                                         field.node.attrs[0].span.lo
+                                     } else {
+                                         field.span.lo
+                                     }
                                  },
                                  |field| field.node.ty.span.hi,
                                  |field| self.format_field(field),
@@ -650,16 +649,14 @@ impl<'a> FmtVisitor<'a> {
     fn rewrite_generics(&self, generics: &ast::Generics, offset: usize, span: Span) -> String {
         // FIXME convert bounds to where clauses where they get too big or if
         // there is a where clause at all.
-        let mut result = String::new();
         let lifetimes: &[_] = &generics.lifetimes;
         let tys: &[_] = &generics.ty_params;
         if lifetimes.len() + tys.len() == 0 {
-            return result;
+            return String::new();
         }
 
         let budget = self.config.max_width - offset - 2;
         // TODO might need to insert a newline if the generics are really long
-        result.push('<');
 
         // Strings for the generics.
         // 1 = <
@@ -697,20 +694,9 @@ impl<'a> FmtVisitor<'a> {
             item.item = ty;
         }
 
-        let fmt = ListFormatting {
-            tactic: ListTactic::HorizontalVertical,
-            separator: ",",
-            trailing_separator: SeparatorTactic::Never,
-            indent: offset + 1,
-            h_width: budget,
-            v_width: budget,
-            ends_with_newline: false,
-        };
-        result.push_str(&write_list(&items, &fmt));
+        let fmt = ListFormatting::for_fn(budget, offset + 1);
 
-        result.push('>');
-
-        result
+        format!("<{}>", write_list(&items, &fmt))
     }
 
     fn rewrite_where_clause(&self,
@@ -719,15 +705,10 @@ impl<'a> FmtVisitor<'a> {
                             indent: usize,
                             span_end: BytePos)
                             -> String {
-        let mut result = String::new();
         if where_clause.predicates.len() == 0 {
-            return result;
+            return String::new();
         }
 
-        result.push('\n');
-        result.push_str(&make_indent(indent + config.tab_spaces));
-        result.push_str("where ");
-
         let context = self.get_context();
         // 6 = "where ".len()
         let offset = indent + config.tab_spaces + 6;
@@ -752,11 +733,12 @@ impl<'a> FmtVisitor<'a> {
             indent: offset,
             h_width: budget,
             v_width: budget,
-            ends_with_newline: false,
+            ends_with_newline: true,
         };
-        result.push_str(&write_list(&items.collect::<Vec<_>>(), &fmt));
 
-        result
+        format!("\n{}where {}",
+                make_indent(indent + config.tab_spaces),
+                write_list(&items.collect::<Vec<_>>(), &fmt))
     }
 
     fn rewrite_return(&self, ret: &ast::FunctionRetTy) -> String {
@@ -766,17 +748,21 @@ impl<'a> FmtVisitor<'a> {
             ast::FunctionRetTy::Return(ref ty) => "-> ".to_owned() + &pprust::ty_to_string(ty),
         }
     }
+}
 
-    // TODO we farm this out, but this could spill over the column limit, so we
-    // ought to handle it properly.
-    fn rewrite_fn_input(&self, arg: &ast::Arg) -> String {
-        if is_named_arg(arg) {
-            format!("{}: {}",
-                    pprust::pat_to_string(&arg.pat),
-                    pprust::ty_to_string(&arg.ty))
+// TODO we farm this out, but this could spill over the column limit, so we
+// ought to handle it properly.
+pub fn rewrite_fn_input(arg: &ast::Arg) -> String {
+    if is_named_arg(arg) {
+        if let ast::Ty_::TyInfer = arg.ty.node {
+            pprust::pat_to_string(&arg.pat)
         } else {
-            pprust::ty_to_string(&arg.ty)
+            format!("{}: {}",
+                pprust::pat_to_string(&arg.pat),
+                pprust::ty_to_string(&arg.ty))
         }
+    } else {
+        pprust::ty_to_string(&arg.ty)
     }
 }
 
@@ -810,7 +796,7 @@ fn rewrite_explicit_self(explicit_self: &ast::ExplicitSelf, args: &[ast::Arg]) -
     }
 }
 
-fn span_lo_for_arg(arg: &ast::Arg) -> BytePos {
+pub fn span_lo_for_arg(arg: &ast::Arg) -> BytePos {
     if is_named_arg(arg) {
         arg.pat.span.lo
     } else {
@@ -818,6 +804,13 @@ fn span_lo_for_arg(arg: &ast::Arg) -> BytePos {
     }
 }
 
+pub fn span_hi_for_arg(arg: &ast::Arg) -> BytePos {
+    match arg.ty.node {
+        ast::Ty_::TyInfer if is_named_arg(arg) => arg.pat.span.hi,
+        _ => arg.ty.span.hi,
+    }
+}
+
 fn is_named_arg(arg: &ast::Arg) -> bool {
     if let ast::Pat_::PatIdent(_, ident, _) = arg.pat.node {
         ident.node != token::special_idents::invalid

src/lists.rs

@@ -52,6 +52,20 @@ pub struct ListFormatting<'a> {
     pub ends_with_newline: bool,
 }
 
+impl<'a> ListFormatting<'a> {
+    pub fn for_fn(width: usize, offset: usize) -> ListFormatting<'a> {
+        ListFormatting {
+            tactic: ListTactic::HorizontalVertical,
+            separator: ",",
+            trailing_separator: SeparatorTactic::Never,
+            indent: offset,
+            h_width: width,
+            v_width: width,
+            ends_with_newline: false,
+        }
+    }
+}
+
 pub struct ListItem {
     pub pre_comment: Option<String>,
     // Item should include attributes and doc comments

src/types.rs

@@ -14,7 +14,7 @@ use syntax::ast;
 use syntax::print::pprust;
 use syntax::codemap::{self, Span, BytePos, CodeMap};
 
-use lists::{itemize_list, write_list, ListTactic, SeparatorTactic, ListFormatting};
+use lists::{itemize_list, write_list, ListFormatting};
 use rewrite::{Rewrite, RewriteContext};
 use utils::{extra_offset, span_after};
 
@@ -218,16 +218,7 @@ fn rewrite_segment(segment: &ast::PathSegment,
             let extra_offset = 1 + separator.len();
             // 1 for >
             let list_width = try_opt!(width.checked_sub(extra_offset + 1));
-
-            let fmt = ListFormatting {
-                tactic: ListTactic::HorizontalVertical,
-                separator: ",",
-                trailing_separator: SeparatorTactic::Never,
-                indent: offset + extra_offset,
-                h_width: list_width,
-                v_width: list_width,
-                ends_with_newline: false,
-            };
+            let fmt = ListFormatting::for_fn(list_width, offset + extra_offset);
 
             // update pos
             *span_lo = next_span_lo;
@@ -253,16 +244,8 @@ fn rewrite_segment(segment: &ast::PathSegment,
             // 2 for ()
             let budget = try_opt!(width.checked_sub(output.len() + 2));
 
-            let fmt = ListFormatting {
-                tactic: ListTactic::HorizontalVertical,
-                separator: ",",
-                trailing_separator: SeparatorTactic::Never,
-                // 1 for (
-                indent: offset + 1,
-                h_width: budget,
-                v_width: budget,
-                ends_with_newline: false,
-            };
+            // 1 for (
+            let fmt = ListFormatting::for_fn(budget, offset + 1);
 
             // update pos
             *span_lo = data.inputs.last().unwrap().span.hi + BytePos(1);

src/visitor.rs

@@ -68,9 +68,18 @@ impl<'a, 'v> visit::Visitor<'v> for FmtVisitor<'a> {
                self.codemap.lookup_char_pos(b.span.lo),
                self.codemap.lookup_char_pos(b.span.hi));
 
-        self.buffer.push_str("{");
-        self.last_pos = self.last_pos + BytePos(1);
+        // Check if this block has braces.
+        let snippet = self.snippet(b.span);
+        let has_braces = snippet.chars().next().unwrap() == '{' || &snippet[..6] == "unsafe";
+        let brace_compensation = if has_braces {
+            BytePos(1)
+        } else {
+            BytePos(0)
+        };
+
+        self.last_pos = self.last_pos + brace_compensation;
         self.block_indent += self.config.tab_spaces;
+        self.buffer.push_str("{");
 
         for stmt in &b.stmts {
             self.visit_stmt(&stmt)
@@ -86,7 +95,7 @@ impl<'a, 'v> visit::Visitor<'v> for FmtVisitor<'a> {
 
         self.block_indent -= self.config.tab_spaces;
         // TODO we should compress any newlines here to just one
-        self.format_missing_with_indent(b.span.hi - BytePos(1));
+        self.format_missing_with_indent(b.span.hi - brace_compensation);
         self.buffer.push_str("}");
         self.last_pos = b.span.hi;
     }

tests/config/small_tabs.toml

@@ -14,3 +14,4 @@ report_todo = "Always"
 report_fixme = "Never"
 reorder_imports = false
 expr_indent_style = "Tabbed"
+closure_indent_style = "Visual"

tests/source/closure.rs

@@ -0,0 +1,39 @@
+// Closures
+
+fn main() {
+    let square = ( |i:  i32 | i  *  i );
+
+    let commented = |/* first */ a /*argument*/, /* second*/ b: WithType /* argument*/, /* ignored */ _ |
+        (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbbbbbbbbbbbbbbbbbbbb);
+
+    let commented = |/* first */ a /*argument*/, /* second*/ b: WithType /* argument*/, /* ignored */ _ |
+        (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb);
+
+    let block_body = move   |xxxxxxxxxxxxxxxxxxxxxxxxxxxxx,  ref  yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy| {
+            xxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
+        };
+
+    let loooooooooooooong_name = |field| {
+             // TODO(#27): format comments.
+             if field.node.attrs.len() > 0 { field.node.attrs[0].span.lo
+             } else {
+                 field.span.lo
+             }};
+
+    let block_me = |field| if true_story() { 1 } else { 2 };
+
+    let unblock_me = |trivial| {
+                         closure()
+                     };
+
+    let empty = |arg|    {};
+
+    let test = |  | { do_something(); do_something_else(); };
+
+    let arg_test = |big_argument_name, test123| looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame();
+
+    let arg_test = |big_argument_name, test123| {looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame()};
+
+    |arg1, arg2, _, _, arg3, arg4| { let temp = arg4 + arg3;
+                                     arg2 * arg1 - temp }
+}

tests/target/closure.rs

@@ -0,0 +1,66 @@
+// Closures
+
+fn main() {
+    let square = (|i: i32| i * i);
+
+    let commented = |// first
+                     a, // argument
+                     // second
+                     b: WithType, // argument
+                     // ignored
+                     _| (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbbbbbbbbbbbbbbbbbbbb);
+
+    let commented = |// first
+                     a, // argument
+                     // second
+                     b: WithType, // argument
+                     // ignored
+                     _| {
+                        (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,
+                         bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb)
+                    };
+
+    let block_body = move |xxxxxxxxxxxxxxxxxxxxxxxxxxxxx,
+                           ref yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy| {
+                              xxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
+                          };
+
+    let loooooooooooooong_name = |field| {
+             // TODO(#27): format comments.
+                                     if field.node.attrs.len() > 0 {
+                                         field.node.attrs[0].span.lo
+                                     } else {
+                                         field.span.lo
+                                     }
+                                 };
+
+    let block_me = |field| {
+                       if true_story() {
+                           1
+                       } else {
+                           2
+                       }
+                   };
+
+    let unblock_me = |trivial| closure();
+
+    let empty = |arg| {};
+
+    let test = || {
+                   do_something();
+                   do_something_else();
+               };
+
+    let arg_test = |big_argument_name, test123| {
+                       looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame()
+                   };
+
+    let arg_test = |big_argument_name, test123| {
+                       looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame()
+                   };
+
+    |arg1, arg2, _, _, arg3, arg4| {
+        let temp = arg4 + arg3;
+        arg2 * arg1 - temp
+    }
+}

tests/target/comments-fn.rs

@@ -11,7 +11,7 @@ fn foo<F, G>(a: aaaaaaaaaaaaa, // A comment
              e: eeeeeeeeeeeee /* comment before paren */)
              -> bar
     where F: Foo, // COmment after where clause
-          G: Goo /* final comment */
+          G: Goo // final comment
 {
 
 }