Merge pull request #362 from marcusklaas/rewrite-string

Make rewrite_string return `Option<String>`

5 files changed, 134 insertions, 93 deletions

diff --git a/src/comment.rs b/src/comment.rs
index 765d61cfa11..1e22a578eb1 100644
--- a/src/comment.rs
+++ b/src/comment.rs
@@ -21,7 +21,7 @@ pub fn rewrite_comment(orig: &str,
                        width: usize,
                        offset: Indent,
                        config: &Config)
-                       -> String {
+                       -> Option<String> {
     let s = orig.trim();
 
     // Edge case: block comments. Let's not trim their lines (for now).
@@ -47,49 +47,53 @@ pub fn rewrite_comment(orig: &str,
     let indent_str = offset.to_string(config);
     let line_breaks = s.chars().filter(|&c| c == '\n').count();
 
-    let (_, mut s) = s.lines()
-                      .enumerate()
-                      .map(|(i, mut line)| {
-                          line = line.trim();
-                          // Drop old closer.
-                          if i == line_breaks && line.ends_with("*/") && !line.starts_with("//") {
-                              line = &line[..(line.len() - 2)];
-                          }
-
-                          line.trim_right()
-                      })
-                      .map(left_trim_comment_line)
-                      .map(|line| {
-                          if line_breaks == 0 {
-                              line.trim_left()
-                          } else {
-                              line
-                          }
-                      })
-                      .fold((true, opener.to_owned()),
-                            |(first, mut acc), line| {
-                                if !first {
-                                    acc.push('\n');
-                                    acc.push_str(&indent_str);
-                                    acc.push_str(line_start);
-                                }
-
-                                if line.len() > max_chars {
-                                    acc.push_str(&rewrite_string(line, &fmt));
-                                } else {
-                                    if line.len() == 0 {
-                                        acc.pop(); // Remove space if this is an empty comment.
-                                    } else {
-                                        acc.push_str(line);
-                                    }
-                                }
-
-                                (false, acc)
-                            });
-
-    s.push_str(closer);
-
-    s
+    let lines = s.lines()
+                 .enumerate()
+                 .map(|(i, mut line)| {
+                     line = line.trim();
+                     // Drop old closer.
+                     if i == line_breaks && line.ends_with("*/") && !line.starts_with("//") {
+                         line = &line[..(line.len() - 2)];
+                     }
+
+                     line.trim_right()
+                 })
+                 .map(left_trim_comment_line)
+                 .map(|line| {
+                     if line_breaks == 0 {
+                         line.trim_left()
+                     } else {
+                         line
+                     }
+                 });
+
+    let mut result = opener.to_owned();
+    let mut first = true;
+
+    for line in lines {
+        if !first {
+            result.push('\n');
+            result.push_str(&indent_str);
+            result.push_str(line_start);
+        }
+
+        if line.len() > max_chars {
+            let rewrite = try_opt!(rewrite_string(line, &fmt));
+            result.push_str(&rewrite);
+        } else {
+            if line.len() == 0 {
+                result.pop(); // Remove space if this is an empty comment.
+            } else {
+                result.push_str(line);
+            }
+        }
+
+        first = false;
+    }
+
+    result.push_str(closer);
+
+    Some(result)
 }
 
 fn left_trim_comment_line(line: &str) -> &str {
@@ -294,33 +298,33 @@ impl<T> Iterator for CharClasses<T> where T: Iterator, T::Item: RichChar {
 #[cfg(test)]
 mod test {
     use super::{CharClasses, CodeCharKind, contains_comment, rewrite_comment, FindUncommented};
-
     use Indent;
+
     #[test]
     #[rustfmt_skip]
     fn format_comments() {
         let config = Default::default();
         assert_eq!("/* test */", rewrite_comment(" //test", true, 100, Indent::new(0, 100),
-                                                 &config));
+                                                 &config).unwrap());
         assert_eq!("// comment\n// on a", rewrite_comment("// comment on a", false, 10,
-                                                          Indent::empty(), &config));
+                                                          Indent::empty(), &config).unwrap());
 
         assert_eq!("//  A multi line comment\n            // between args.",
                    rewrite_comment("//  A multi line comment\n             // between args.",
                                    false,
                                    60,
                                    Indent::new(0, 12),
-                                   &config));
+                                   &config).unwrap());
 
         let input = "// comment";
         let expected =
             "/* com\n                                                                      \
              * men\n                                                                      \
              * t */";
-        assert_eq!(expected, rewrite_comment(input, true, 9, Indent::new(0, 69), &config));
+        assert_eq!(expected, rewrite_comment(input, true, 9, Indent::new(0, 69), &config).unwrap());
 
         assert_eq!("/* trimmed */", rewrite_comment("/*   trimmed    */", true, 100,
-                                                    Indent::new(0, 100), &config));
+                                                    Indent::new(0, 100), &config).unwrap());
     }
 
     // This is probably intended to be a non-test fn, but it is not used. I'm
diff --git a/src/expr.rs b/src/expr.rs
index c1f211dbf36..b3e5fee176d 100644
--- a/src/expr.rs
+++ b/src/expr.rs
@@ -377,7 +377,11 @@ impl Rewrite for ast::Block {
                     // 9 = "unsafe  {".len(), 7 = "unsafe ".len()
                     let budget = try_opt!(width.checked_sub(9));
                     format!("unsafe {} ",
-                            rewrite_comment(trimmed, true, budget, offset + 7, context.config))
+                            try_opt!(rewrite_comment(trimmed,
+                                                     true,
+                                                     budget,
+                                                     offset + 7,
+                                                     context.config)))
                 } else {
                     "unsafe ".to_owned()
                 };
@@ -661,7 +665,7 @@ fn rewrite_match_arm_comment(context: &RewriteContext,
                              width: usize,
                              arm_indent: Indent,
                              arm_indent_str: &str)
-                             -> String {
+                             -> Option<String> {
     // The leading "," is not part of the arm-comment
     let missed_str = match missed_str.find_uncommented(",") {
         Some(n) => &missed_str[n+1..],
@@ -687,11 +691,17 @@ 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));
         result.push('\n');
         result.push_str(arm_indent_str);
-        result.push_str(&rewrite_comment(&missed_str, false, width, arm_indent, context.config));
+        result.push_str(&comment);
     }
-    return result;
+
+    Some(result)
 }
 
 fn rewrite_match(context: &RewriteContext,
@@ -725,11 +735,12 @@ fn rewrite_match(context: &RewriteContext,
         } else {
             context.snippet(mk_sp(arm_end_pos(&arms[i-1]), arm_start_pos(arm)))
         };
-        result.push_str(&rewrite_match_arm_comment(context,
-                                                   &missed_str,
-                                                   width,
-                                                   arm_indent,
-                                                   &arm_indent_str));
+        let comment = try_opt!(rewrite_match_arm_comment(context,
+                                                         &missed_str,
+                                                         width,
+                                                         arm_indent,
+                                                         &arm_indent_str));
+        result.push_str(&comment);
         result.push('\n');
         result.push_str(&arm_indent_str);
 
@@ -745,11 +756,12 @@ fn rewrite_match(context: &RewriteContext,
         }
     }
     let last_comment = context.snippet(mk_sp(arm_end_pos(&arms[arms.len() - 1]), span.hi));
-    result.push_str(&rewrite_match_arm_comment(context,
-                                               &last_comment,
-                                               width,
-                                               arm_indent,
-                                               &arm_indent_str));
+    let comment = try_opt!(rewrite_match_arm_comment(context,
+                                                     &last_comment,
+                                                     width,
+                                                     arm_indent,
+                                                     &arm_indent_str));
+    result.push_str(&comment);
     result.push('\n');
     result.push_str(&(context.block_indent + context.overflow_indent).to_string(context.config));
     result.push('}');
@@ -1007,7 +1019,7 @@ fn rewrite_string_lit(context: &RewriteContext,
     let string_lit = context.snippet(span);
     let str_lit = &string_lit[1..string_lit.len() - 1]; // Remove the quote characters.
 
-    Some(rewrite_string(str_lit, &fmt))
+    rewrite_string(str_lit, &fmt)
 }
 
 pub fn rewrite_call<R>(context: &RewriteContext,
diff --git a/src/lists.rs b/src/lists.rs
index a6c0e862cce..cccb8d37b08 100644
--- a/src/lists.rs
+++ b/src/lists.rs
@@ -200,11 +200,12 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> Op
             let block_mode = tactic != ListTactic::Vertical;
             // Width restriction is only relevant in vertical mode.
             let max_width = formatting.v_width;
-            result.push_str(&rewrite_comment(comment,
-                                             block_mode,
-                                             max_width,
-                                             formatting.indent,
-                                             formatting.config));
+            let comment = try_opt!(rewrite_comment(comment,
+                                                   block_mode,
+                                                   max_width,
+                                                   formatting.indent,
+                                                   formatting.config));
+            result.push_str(&comment);
 
             if tactic == ListTactic::Vertical {
                 result.push('\n');
@@ -223,11 +224,11 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> Op
         // Post-comments
         if tactic != ListTactic::Vertical && item.post_comment.is_some() {
             let comment = item.post_comment.as_ref().unwrap();
-            let formatted_comment = rewrite_comment(comment,
-                                                    true,
-                                                    formatting.v_width,
-                                                    Indent::empty(),
-                                                    formatting.config);
+            let formatted_comment = try_opt!(rewrite_comment(comment,
+                                                             true,
+                                                             formatting.v_width,
+                                                             Indent::empty(),
+                                                             formatting.config));
 
             result.push(' ');
             result.push_str(&formatted_comment);
@@ -248,11 +249,11 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> Op
                               comment.trim().contains('\n') ||
                               comment.trim().len() > width;
 
-            let formatted_comment = rewrite_comment(comment,
-                                                    block_style,
-                                                    width,
-                                                    offset,
-                                                    formatting.config);
+            let formatted_comment = try_opt!(rewrite_comment(comment,
+                                                             block_style,
+                                                             width,
+                                                             offset,
+                                                             formatting.config));
 
             result.push(' ');
             result.push_str(&formatted_comment);
diff --git a/src/string.rs b/src/string.rs
index 4e40b8e9ede..ffaf52d3380 100644
--- a/src/string.rs
+++ b/src/string.rs
@@ -31,23 +31,23 @@ pub struct StringFormat<'a> {
 }
 
 // TODO: simplify this!
-pub fn rewrite_string<'a>(s: &str, fmt: &StringFormat<'a>) -> String {
+pub fn rewrite_string<'a>(s: &str, fmt: &StringFormat<'a>) -> Option<String> {
     // TODO if lo.col > IDEAL - 10, start a new line (need cur indent for that)
     // Strip line breaks.
     let re = Regex::new(r"(\\[:space:]+)").unwrap();
     let stripped_str = re.replace_all(s, "");
 
     let graphemes = UnicodeSegmentation::graphemes(&*stripped_str, false).collect::<Vec<&str>>();
-
     let indent = fmt.offset.to_string(fmt.config);
-    let indent = &indent;
 
     let mut cur_start = 0;
     let mut result = String::with_capacity(round_up_to_power_of_two(s.len()));
     result.push_str(fmt.opener);
 
     let ender_length = fmt.line_end.len();
-    let max_chars = fmt.width.checked_sub(fmt.opener.len() + ender_length).unwrap_or(1);
+    // If we cannot put at least a single character per line, the rewrite won't
+    // succeed.
+    let max_chars = try_opt!(fmt.width.checked_sub(fmt.opener.len() + ender_length + 1)) + 1;
 
     loop {
         let mut cur_end = cur_start + max_chars;
@@ -57,8 +57,9 @@ pub fn rewrite_string<'a>(s: &str, fmt: &StringFormat<'a>) -> String {
             result.push_str(line);
             break;
         }
+
         // Push cur_end left until we reach whitespace.
-        while !(graphemes[cur_end - 1].trim().len() == 0) {
+        while !graphemes[cur_end - 1].trim().is_empty() {
             cur_end -= 1;
             if cur_end - cur_start < MIN_STRING {
                 // We can't break at whitespace, fall back to splitting
@@ -71,7 +72,7 @@ pub fn rewrite_string<'a>(s: &str, fmt: &StringFormat<'a>) -> String {
             }
         }
         // Make sure there is no whitespace to the right of the break.
-        while cur_end < s.len() && graphemes[cur_end].trim().len() == 0 {
+        while cur_end < s.len() && graphemes[cur_end].trim().is_empty() {
             cur_end += 1;
         }
         let raw_line = graphemes[cur_start..cur_end].join("");
@@ -85,12 +86,34 @@ pub fn rewrite_string<'a>(s: &str, fmt: &StringFormat<'a>) -> String {
         result.push_str(line);
         result.push_str(fmt.line_end);
         result.push('\n');
-        result.push_str(indent);
+        result.push_str(&indent);
         result.push_str(fmt.line_start);
 
         cur_start = cur_end;
     }
     result.push_str(fmt.closer);
 
-    result
+    Some(result)
+}
+
+#[cfg(test)]
+mod test {
+    use super::{StringFormat, rewrite_string};
+
+    #[test]
+    fn issue343() {
+        let config = Default::default();
+        let fmt = StringFormat {
+            opener: "\"",
+            closer: "\"",
+            line_start: " ",
+            line_end: "\\",
+            width: 2,
+            offset: ::Indent::empty(),
+            trim_end: false,
+            config: &config,
+        };
+
+        rewrite_string("eq_", &fmt);
+    }
 }
diff --git a/src/visitor.rs b/src/visitor.rs
index 8c46d69ebfc..1a070a839f5 100644
--- a/src/visitor.rs
+++ b/src/visitor.rs
@@ -407,11 +407,12 @@ impl<'a> Rewrite for [ast::Attribute] {
                 let multi_line = a_str.starts_with("//") && comment.matches('\n').count() > 1;
                 let comment = comment.trim();
                 if !comment.is_empty() {
-                    let comment = rewrite_comment(comment,
-                                                  false,
-                                                  context.config.max_width - offset.width(),
-                                                  offset,
-                                                  context.config);
+                    let comment = try_opt!(rewrite_comment(comment,
+                                                           false,
+                                                           context.config.max_width -
+                                                           offset.width(),
+                                                           offset,
+                                                           context.config));
                     result.push_str(&indent);
                     result.push_str(&comment);
                     result.push('\n');