Merge pull request #893 from marcusklaas/try-shorthand

Try shorthand

Author: nrc

src/chains.rs

@@ -9,7 +9,7 @@
 // except according to those terms.
 
 /// Formatting of chained expressions, i.e. expressions which are chained by
-/// dots: struct and enum field access and method calls.
+/// dots: struct and enum field access, method calls, and try shorthand (?).
 ///
 /// Instead of walking these subexpressions one-by-one, as is our usual strategy
 /// for expression formatting, we collect maximal sequences of these expressions
@@ -81,24 +81,23 @@
 /// true, then we allow the last method call to spill over multiple lines without
 /// forcing the rest of the chain to be split.
 
-
 use Indent;
 use rewrite::{Rewrite, RewriteContext};
 use utils::{wrap_str, first_line_width};
 use expr::rewrite_call;
 use config::BlockIndentStyle;
+use macros::convert_try_mac;
 
 use syntax::{ast, ptr};
 use syntax::codemap::{mk_sp, Span};
 
-
 pub fn rewrite_chain(expr: &ast::Expr,
                      context: &RewriteContext,
                      width: usize,
                      offset: Indent)
                      -> Option<String> {
     let total_span = expr.span;
-    let (parent, subexpr_list) = make_subexpr_list(expr);
+    let (parent, subexpr_list) = make_subexpr_list(expr, context);
 
     // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`.
     let parent_block_indent = chain_base_indent(context, offset);
@@ -107,11 +106,16 @@ pub fn rewrite_chain(expr: &ast::Expr,
 
     // Decide how to layout the rest of the chain. `extend` is true if we can
     // put the first non-parent item on the same line as the parent.
-    let (indent, extend) = if !parent_rewrite.contains('\n') && is_continuable(parent) ||
+    let (indent, extend) = if !parent_rewrite.contains('\n') && is_continuable(&parent) ||
                               parent_rewrite.len() <= context.config.tab_spaces {
-        // Try and put at least the first two items on the same line.
-        (chain_indent(context, offset + Indent::new(0, parent_rewrite.len())), true)
-    } else if is_block_expr(parent, &parent_rewrite) {
+
+        let indent = if let ast::ExprKind::Try(..) = subexpr_list.last().unwrap().node {
+            parent_block_indent.block_indent(context.config)
+        } else {
+            chain_indent(context, offset + Indent::new(0, parent_rewrite.len()))
+        };
+        (indent, true)
+    } else if is_block_expr(&parent, &parent_rewrite) {
         // The parent is a block, so align the rest of the chain with the closing
         // brace.
         (parent_block_indent, false)
@@ -184,12 +188,29 @@ pub fn rewrite_chain(expr: &ast::Expr,
     wrap_str(format!("{}{}{}",
                      parent_rewrite,
                      first_connector,
-                     rewrites.join(&connector)),
+                     join_rewrites(&rewrites, &subexpr_list, &connector)),
              context.config.max_width,
              width,
              offset)
 }
 
+fn join_rewrites(rewrites: &[String], subexps: &[ast::Expr], connector: &str) -> String {
+    let mut rewrite_iter = rewrites.iter();
+    let mut result = rewrite_iter.next().unwrap().clone();
+    let mut subexpr_iter = subexps.iter().rev();
+    subexpr_iter.next();
+
+    for (rewrite, expr) in rewrite_iter.zip(subexpr_iter) {
+        match expr.node {
+            ast::ExprKind::Try(_) => (),
+            _ => result.push_str(connector),
+        };
+        result.push_str(&rewrite[..]);
+    }
+
+    result
+}
+
 // States whether an expression's last line exclusively consists of closing
 // parens, braces, and brackets in its idiomatic formatting.
 fn is_block_expr(expr: &ast::Expr, repr: &str) -> bool {
@@ -213,21 +234,11 @@ fn is_block_expr(expr: &ast::Expr, repr: &str) -> bool {
 
 // Returns the root of the chain and a Vec of the prefixes of the rest of the chain.
 // E.g., for input `a.b.c` we return (`a`, [`a.b.c`, `a.b`])
-fn make_subexpr_list(mut expr: &ast::Expr) -> (&ast::Expr, Vec<&ast::Expr>) {
-    fn pop_expr_chain(expr: &ast::Expr) -> Option<&ast::Expr> {
-        match expr.node {
-            ast::ExprKind::MethodCall(_, _, ref expressions) => Some(&expressions[0]),
-            ast::ExprKind::TupField(ref subexpr, _) |
-            ast::ExprKind::Field(ref subexpr, _) => Some(subexpr),
-            _ => None,
-        }
-    }
-
-    let mut subexpr_list = vec![expr];
+fn make_subexpr_list(expr: &ast::Expr, context: &RewriteContext) -> (ast::Expr, Vec<ast::Expr>) {
+    let mut subexpr_list = vec![expr.clone()];
 
-    while let Some(subexpr) = pop_expr_chain(expr) {
-        subexpr_list.push(subexpr);
-        expr = subexpr;
+    while let Some(subexpr) = pop_expr_chain(subexpr_list.last().unwrap(), context) {
+        subexpr_list.push(subexpr.clone());
     }
 
     let parent = subexpr_list.pop().unwrap();
@@ -293,6 +304,33 @@ fn rewrite_method_call_with_overflow(expr_kind: &ast::ExprKind,
     }
 }
 
+// Returns the expression's subexpression, if it exists. When the subexpr
+// is a try! macro, we'll convert it to shorthand when the option is set.
+fn pop_expr_chain(expr: &ast::Expr, context: &RewriteContext) -> Option<ast::Expr> {
+    match expr.node {
+        ast::ExprKind::MethodCall(_, _, ref expressions) => {
+            Some(convert_try(&expressions[0], context))
+        }
+        ast::ExprKind::TupField(ref subexpr, _) |
+        ast::ExprKind::Field(ref subexpr, _) |
+        ast::ExprKind::Try(ref subexpr) => Some(convert_try(subexpr, context)),
+        _ => None,
+    }
+}
+
+fn convert_try(expr: &ast::Expr, context: &RewriteContext) -> ast::Expr {
+    match expr.node {
+        ast::ExprKind::Mac(ref mac) if context.config.use_try_shorthand => {
+            if let Some(subexpr) = convert_try_mac(mac, context) {
+                subexpr
+            } else {
+                expr.clone()
+            }
+        }
+        _ => expr.clone(),
+    }
+}
+
 // Rewrite the last element in the chain `expr`. E.g., given `a.b.c` we rewrite
 // `.c`.
 fn rewrite_chain_subexpr(expr: &ast::Expr,
@@ -328,6 +366,13 @@ fn rewrite_chain_subexpr(expr: &ast::Expr,
                 None
             }
         }
+        ast::ExprKind::Try(_) => {
+            if width >= 1 {
+                Some("?".into())
+            } else {
+                None
+            }
+        }
         _ => unreachable!(),
     }
 }

src/config.rs

@@ -392,6 +392,7 @@ create_config! {
     match_wildcard_trailing_comma: bool, true, "Put a trailing comma after a wildcard arm";
     closure_block_indent_threshold: isize, 5, "How many lines a closure must have before it is \
                                                block indented. -1 means never use block indent.";
+    use_try_shorthand: bool, false, "Replace uses of the try! macro by the ? shorthand";
     write_mode: WriteMode, WriteMode::Replace,
         "What Write Mode to use when none is supplied: Replace, Overwrite, Display, Diff, Coverage";
 }

src/expr.rs

@@ -136,18 +136,25 @@ impl Rewrite for ast::Expr {
                     Some(ident) => format!(" {}", ident.node),
                     None => String::new(),
                 };
-                wrap_str(format!("continue{}", id_str), context.config.max_width, width, offset)
+                wrap_str(format!("continue{}", id_str),
+                         context.config.max_width,
+                         width,
+                         offset)
             }
             ast::ExprKind::Break(ref opt_ident) => {
                 let id_str = match *opt_ident {
                     Some(ident) => format!(" {}", ident.node),
                     None => String::new(),
                 };
-                wrap_str(format!("break{}", id_str), context.config.max_width, width, offset)
+                wrap_str(format!("break{}", id_str),
+                         context.config.max_width,
+                         width,
+                         offset)
             }
             ast::ExprKind::Closure(capture, ref fn_decl, ref body, _) => {
                 rewrite_closure(capture, fn_decl, body, self.span, context, width, offset)
             }
+            ast::ExprKind::Try(..) |
             ast::ExprKind::Field(..) |
             ast::ExprKind::TupField(..) |
             ast::ExprKind::MethodCall(..) => rewrite_chain(self, context, width, offset),
@@ -199,21 +206,15 @@ impl Rewrite for ast::Expr {
                         rewrite_unary_prefix(context, delim, &**rhs, width, offset)
                     }
                     (Some(ref lhs), None) => {
-                        Some(format!("{}{}",
-                                     try_opt!(lhs.rewrite(context,
-                                                          try_opt!(width.checked_sub(delim.len())),
-                                                          offset)),
-                                     delim))
+                        rewrite_unary_suffix(context, delim, &**lhs, width, offset)
                     }
                     (None, None) => wrap_str(delim.into(), context.config.max_width, width, offset),
                 }
             }
             // We do not format these expressions yet, but they should still
             // satisfy our width restrictions.
             ast::ExprKind::InPlace(..) |
-            ast::ExprKind::InlineAsm(..) |
-            // TODO(#867): Handle try shorthand
-            ast::ExprKind::Try(_) => {
+            ast::ExprKind::InlineAsm(..) => {
                 wrap_str(context.snippet(self.span),
                          context.config.max_width,
                          width,
@@ -689,11 +690,8 @@ fn extract_comment(span: Span,
                    -> Option<String> {
     let comment_str = context.snippet(span);
     if contains_comment(&comment_str) {
-        let comment = try_opt!(rewrite_comment(comment_str.trim(),
-                                               false,
-                                               width,
-                                               offset,
-                                               context.config));
+        let comment =
+            try_opt!(rewrite_comment(comment_str.trim(), false, width, offset, context.config));
         Some(format!("\n{indent}{}\n{indent}",
                      comment,
                      indent = offset.to_string(context.config)))
@@ -793,14 +791,11 @@ fn rewrite_if_else(context: &RewriteContext,
             }
         };
 
-        let between_if_else_block = mk_sp(if_block.span.hi,
-                                          context.codemap.span_before(mk_sp(if_block.span.hi,
-                                                                            else_block.span.lo),
-                                                                      "else"));
-        let between_if_else_block_comment = extract_comment(between_if_else_block,
-                                                            &context,
-                                                            offset,
-                                                            width);
+        let between_if_else_block =
+            mk_sp(if_block.span.hi,
+                  context.codemap.span_before(mk_sp(if_block.span.hi, else_block.span.lo), "else"));
+        let between_if_else_block_comment =
+            extract_comment(between_if_else_block, &context, offset, width);
 
         let after_else = mk_sp(context.codemap
                                    .span_after(mk_sp(if_block.span.hi, else_block.span.lo),
@@ -927,11 +922,8 @@ fn rewrite_match_arm_comment(context: &RewriteContext,
     }
     let missed_str = missed_str[first..].trim();
     if !missed_str.is_empty() {
-        let comment = try_opt!(rewrite_comment(&missed_str,
-                                               false,
-                                               width,
-                                               arm_indent,
-                                               context.config));
+        let comment =
+            try_opt!(rewrite_comment(&missed_str, false, width, arm_indent, context.config));
         result.push('\n');
         result.push_str(arm_indent_str);
         result.push_str(&comment);
@@ -1155,10 +1147,9 @@ impl Rewrite for ast::Arm {
         let body_budget = try_opt!(width.checked_sub(context.config.tab_spaces));
         let indent = context.block_indent.block_indent(context.config);
         let inner_context = &RewriteContext { block_indent: indent, ..*context };
-        let next_line_body = try_opt!(nop_block_collapse(body.rewrite(inner_context,
-                                                                      body_budget,
-                                                                      indent),
-                                                         body_budget));
+        let next_line_body =
+            try_opt!(nop_block_collapse(body.rewrite(inner_context, body_budget, indent),
+                                        body_budget));
         let indent_str = offset.block_indent(context.config).to_string(context.config);
         let (body_prefix, body_suffix) = if context.config.wrap_match_arms {
             if context.config.match_block_trailing_comma {
@@ -1762,6 +1753,21 @@ pub fn rewrite_unary_prefix<R: Rewrite>(context: &RewriteContext,
         .map(|r| format!("{}{}", prefix, r))
 }
 
+// FIXME: this is probably not correct for multi-line Rewrites. we should
+// subtract suffix.len() from the last line budget, not the first!
+pub fn rewrite_unary_suffix<R: Rewrite>(context: &RewriteContext,
+                                        suffix: &str,
+                                        rewrite: &R,
+                                        width: usize,
+                                        offset: Indent)
+                                        -> Option<String> {
+    rewrite.rewrite(context, try_opt!(width.checked_sub(suffix.len())), offset)
+        .map(|mut r| {
+            r.push_str(suffix);
+            r
+        })
+}
+
 fn rewrite_unary_op(context: &RewriteContext,
                     op: &ast::UnOp,
                     expr: &ast::Expr,
@@ -1817,24 +1823,42 @@ pub fn rewrite_assign_rhs<S: Into<String>>(context: &RewriteContext,
     let max_width = try_opt!(width.checked_sub(last_line_width + 1));
     let rhs = ex.rewrite(&context, max_width, offset + last_line_width + 1);
 
+    fn count_line_breaks(src: &str) -> usize {
+        src.chars().filter(|&x| x == '\n').count()
+    }
+
     match rhs {
-        Some(new_str) => {
+        Some(ref new_str) if count_line_breaks(new_str) < 2 => {
             result.push(' ');
-            result.push_str(&new_str)
+            result.push_str(new_str);
         }
-        None => {
-            // Expression did not fit on the same line as the identifier. Retry
-            // on the next line.
+        _ => {
+            // Expression did not fit on the same line as the identifier or is
+            // at least three lines big. Try splitting the line and see
+            // if that works better.
             let new_offset = offset.block_indent(context.config);
-            result.push_str(&format!("\n{}", new_offset.to_string(context.config)));
-
-            // FIXME: we probably should related max_width to width instead of
-            // config.max_width where is the 1 coming from anyway?
-            let max_width = try_opt!(context.config.max_width.checked_sub(new_offset.width() + 1));
+            let max_width = try_opt!((width + offset.width()).checked_sub(new_offset.width()));
             let inner_context = context.nested_context();
-            let rhs = ex.rewrite(&inner_context, max_width, new_offset);
-
-            result.push_str(&&try_opt!(rhs));
+            let new_rhs = ex.rewrite(&inner_context, max_width, new_offset);
+
+            // FIXME: DRY!
+            match (rhs, new_rhs) {
+                (Some(ref orig_rhs), Some(ref replacement_rhs))
+                    if count_line_breaks(orig_rhs) >
+                       count_line_breaks(replacement_rhs) + 1 => {
+                    result.push_str(&format!("\n{}", new_offset.to_string(context.config)));
+                    result.push_str(replacement_rhs);
+                }
+                (None, Some(ref final_rhs)) => {
+                    result.push_str(&format!("\n{}", new_offset.to_string(context.config)));
+                    result.push_str(final_rhs);
+                }
+                (None, None) => return None,
+                (Some(ref orig_rhs), _) => {
+                    result.push(' ');
+                    result.push_str(orig_rhs);
+                }
+            }
         }
     }