Better handle comments and newlines around erased imports

Author: nrc

src/lists.rs

@@ -56,7 +56,7 @@ impl AsRef<ListItem> for ListItem {
     }
 }
 
-#[derive(PartialEq, Eq)]
+#[derive(PartialEq, Eq, Debug)]
 pub enum ListItemCommentStyle {
     // Try to keep the comment on the same line with the item.
     SameLine,
@@ -66,6 +66,7 @@ pub enum ListItemCommentStyle {
     None,
 }
 
+#[derive(Debug)]
 pub struct ListItem {
     // None for comments mean that they are not present.
     pub pre_comment: Option<String>,
@@ -118,6 +119,18 @@ impl ListItem {
             new_lines: false,
         }
     }
+
+    // true if the item causes something to be written.
+    fn is_substantial(&self) -> bool {
+        fn empty(s: &Option<String>) -> bool {
+            match *s {
+                Some(ref s) if !s.is_empty() => false,
+                _ => true,
+            }
+        }
+
+        !(empty(&self.pre_comment) && empty(&self.item) && empty(&self.post_comment))
+    }
 }
 
 /// The type of separator for lists.
@@ -220,6 +233,10 @@ where
             item_last_line_width -= indent_str.len();
         }
 
+        if !item.is_substantial() {
+            continue;
+        }
+
         match tactic {
             DefinitiveListTactic::Horizontal if !first => {
                 result.push(' ');
@@ -276,26 +293,28 @@ where
                 rewrite_comment(comment, block_mode, formatting.shape, formatting.config)?;
             result.push_str(&comment);
 
-            if tactic == DefinitiveListTactic::Vertical {
-                // We cannot keep pre-comments on the same line if the comment if normalized.
-                let keep_comment = if formatting.config.normalize_comments()
-                    || item.pre_comment_style == ListItemCommentStyle::DifferentLine
-                {
-                    false
+            if !inner_item.is_empty() {
+                if tactic == DefinitiveListTactic::Vertical {
+                    // We cannot keep pre-comments on the same line if the comment if normalized.
+                    let keep_comment = if formatting.config.normalize_comments()
+                        || item.pre_comment_style == ListItemCommentStyle::DifferentLine
+                    {
+                        false
+                    } else {
+                        // We will try to keep the comment on the same line with the item here.
+                        // 1 = ` `
+                        let total_width = total_item_width(item) + item_sep_len + 1;
+                        total_width <= formatting.shape.width
+                    };
+                    if keep_comment {
+                        result.push(' ');
+                    } else {
+                        result.push('\n');
+                        result.push_str(indent_str);
+                    }
                 } else {
-                    // We will try to keep the comment on the same line with the item here.
-                    // 1 = ` `
-                    let total_width = total_item_width(item) + item_sep_len + 1;
-                    total_width <= formatting.shape.width
-                };
-                if keep_comment {
                     result.push(' ');
-                } else {
-                    result.push('\n');
-                    result.push_str(indent_str);
                 }
-            } else {
-                result.push(' ');
             }
             item_max_width = None;
         }
@@ -304,7 +323,7 @@ where
             result.push_str(formatting.separator.trim());
             result.push(' ');
         }
-        result.push_str(&inner_item[..]);
+        result.push_str(inner_item);
 
         // Post-comments
         if tactic != DefinitiveListTactic::Vertical && item.post_comment.is_some() {

src/missed_spans.rs

@@ -104,19 +104,38 @@ impl<'a> FmtVisitor<'a> {
     }
 
     fn push_vertical_spaces(&mut self, mut newline_count: usize) {
-        // The buffer already has a trailing newline.
-        let offset = if self.buffer.ends_with('\n') { 0 } else { 1 };
-        let newline_upper_bound = self.config.blank_lines_upper_bound() + offset;
-        let newline_lower_bound = self.config.blank_lines_lower_bound() + offset;
-        if newline_count > newline_upper_bound {
-            newline_count = newline_upper_bound;
-        } else if newline_count < newline_lower_bound {
-            newline_count = newline_lower_bound;
+        let offset = self.count_trailing_newlines();
+        let newline_upper_bound = self.config.blank_lines_upper_bound() + 1;
+        let newline_lower_bound = self.config.blank_lines_lower_bound() + 1;
+
+        if newline_count + offset > newline_upper_bound {
+            if offset >= newline_upper_bound {
+                newline_count = 0;
+            } else {
+                newline_count = newline_upper_bound - offset;
+            }
+        } else if newline_count + offset < newline_lower_bound {
+            if offset >= newline_lower_bound {
+                newline_count = 0;
+            } else {
+                newline_count = newline_lower_bound - offset;
+            }
         }
+
         let blank_lines: String = repeat('\n').take(newline_count).collect();
         self.push_str(&blank_lines);
     }
 
+    fn count_trailing_newlines(&self) -> usize {
+        let mut buf = &*self.buffer;
+        let mut result = 0;
+        while buf.ends_with('\n') {
+            buf = &buf[..buf.len() - 1];
+            result += 1;
+        }
+        result
+    }
+
     fn write_snippet<F>(&mut self, span: Span, process_last_snippet: F)
     where
         F: Fn(&mut FmtVisitor, &str, &str),

src/reorder.rs

@@ -121,6 +121,7 @@ fn rewrite_reorderable_items(
         span.hi(),
         false,
     );
+
     let mut item_pair_vec: Vec<_> = items.zip(reorderable_items.iter()).collect();
     item_pair_vec.sort_by(|a, b| compare_items(a.1, b.1));
     let item_vec: Vec<_> = item_pair_vec.into_iter().map(|pair| pair.0).collect();

tests/target/imports.rs

@@ -85,5 +85,4 @@ use fooo::{bar, x, y, z,
 // nested imports with a single sub-tree.
 use a::b::c::d;
 use a::b::c::*;
-
 use a::b::c::{xxx, yyy, zzz};