Merge pull request #113 from marcusklaas/import-comments

Update import list formatting

Author: nrc

src/comment.rs

@@ -46,9 +46,16 @@ pub fn rewrite_comment(orig: &str, block_style: bool, width: usize, offset: usiz
                 line = &line[..(line.len() - 2)];
             }
 
-            line.trim_right_matches(' ')
+            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');
@@ -98,6 +105,8 @@ fn format_comments() {
                             * men\n                                                                      \
                             * t */";
     assert_eq!(expected_output, rewrite_comment(input, true, 9, 69));
+
+    assert_eq!("/* trimmed */", rewrite_comment("/*   trimmed    */", true, 100, 100));
 }
 
 
@@ -156,6 +165,7 @@ fn test_find_uncommented() {
     check("/*sup yo? \n sup*/ sup", "p", Some(20));
     check("hel/*lohello*/lo", "hello", None);
     check("acb", "ab", None);
+    check(",/*A*/ ", ",", Some(0));
 }
 
 // Returns the first byte position after the first comment. The given string

src/config.rs

@@ -29,6 +29,7 @@ pub struct Config {
     pub enum_trailing_comma: bool,
     pub report_todo: ReportTactic,
     pub report_fixme: ReportTactic,
+    pub reorder_imports: bool, // Alphabetically, case sensitive.
 }
 
 impl Config {

src/default.toml

@@ -11,3 +11,4 @@ struct_lit_trailing_comma = "Vertical"
 enum_trailing_comma = true
 report_todo = "Always"
 report_fixme = "Never"
+reorder_imports = false

src/imports.rs

@@ -9,12 +9,13 @@
 // except according to those terms.
 
 use visitor::FmtVisitor;
-use lists::{write_list, ListItem, ListFormatting, SeparatorTactic, ListTactic};
-use utils::format_visibility;
+use lists::{write_list, itemize_list, ListItem, ListFormatting, SeparatorTactic, ListTactic};
+use utils::{span_after, format_visibility};
 
 use syntax::ast;
 use syntax::parse::token;
 use syntax::print::pprust;
+use syntax::codemap::Span;
 
 // TODO (some day) remove unused imports, expand globs, compress many single imports into a list import
 
@@ -40,13 +41,14 @@ fn rewrite_single_use_list(path_str: String, vpi: ast::PathListItem, vis: &str)
 impl<'a> FmtVisitor<'a> {
     // Basically just pretty prints a multi-item import.
     // Returns None when the import can be removed.
-    pub fn rewrite_use_list(&mut self,
+    pub fn rewrite_use_list(&self,
                             block_indent: usize,
                             one_line_budget: usize, // excluding indentation
                             multi_line_budget: usize,
                             path: &ast::Path,
                             path_list: &[ast::PathListItem],
-                            visibility: ast::Visibility) -> Option<String> {
+                            visibility: ast::Visibility,
+                            span: Span) -> Option<String> {
         let path_str = pprust::path_to_string(path);
         let vis = format_visibility(visibility);
 
@@ -78,34 +80,53 @@ impl<'a> FmtVisitor<'a> {
             ends_with_newline: true,
         };
 
-        // TODO handle any comments inbetween items.
-        // If `self` is in the list, put it first.
-        let head = if path_list.iter().any(|vpi|
-            if let ast::PathListItem_::PathListMod{ .. } = vpi.node {
-                true
-            } else {
-                false
-            }
-        ) {
-            Some(ListItem::from_str("self"))
-        } else {
-            None
-        };
+        let mut items = itemize_list(self.codemap,
+                                     vec![ListItem::from_str("")], // Dummy value, explanation below
+                                     path_list.iter(),
+                                     ",",
+                                     "}",
+                                     |vpi| vpi.span.lo,
+                                     |vpi| vpi.span.hi,
+                                     |vpi| match vpi.node {
+                                         ast::PathListItem_::PathListIdent{ name, .. } => {
+                                             token::get_ident(name).to_string()
+                                         }
+                                         ast::PathListItem_::PathListMod{ .. } => {
+                                             "self".to_owned()
+                                         }
+                                     },
+                                     span_after(span, "{", self.codemap),
+                                     span.hi);
+
+        // We prefixed the item list with a dummy value so that we can
+        // potentially move "self" to the front of the vector without touching
+        // the rest of the items.
+        // FIXME: Make more efficient by using a linked list? That would
+        // require changes to the signatures of itemize_list and write_list.
+        let has_self = move_self_to_front(&mut items);
+        let first_index = if has_self { 0 } else { 1 };
+
+        if self.config.reorder_imports {
+            items.tail_mut().sort_by(|a, b| a.item.cmp(&b.item));
+        }
 
-        let items: Vec<_> = head.into_iter().chain(path_list.iter().filter_map(|vpi| {
-            match vpi.node {
-                ast::PathListItem_::PathListIdent{ name, .. } => {
-                    Some(ListItem::from_str(token::get_ident(name).to_string()))
-                }
-                // Skip `self`, because we added it above.
-                ast::PathListItem_::PathListMod{ .. } => None,
-            }
-        })).collect();
+        let list = write_list(&items[first_index..], &fmt);
 
         Some(if path_str.len() == 0 {
-            format!("{}use {{{}}};", vis, write_list(&items, &fmt))
+            format!("{}use {{{}}};", vis, list)
         } else {
-            format!("{}use {}::{{{}}};", vis, path_str, write_list(&items, &fmt))
+            format!("{}use {}::{{{}}};", vis, path_str, list)
         })
     }
 }
+
+// Returns true when self item was found.
+fn move_self_to_front(items: &mut Vec<ListItem>) -> bool {
+    match items.iter().position(|item| item.item == "self") {
+        Some(pos) => {
+            items[0] = items.remove(pos);
+            true
+        },
+        None => false
+    }
+}

src/lib.rs

@@ -11,6 +11,7 @@
 #![feature(rustc_private)]
 #![feature(str_escape)]
 #![feature(str_char)]
+#![feature(slice_extras)]
 
 // TODO we're going to allocate a whole bunch of temp Strings, is it worth
 // keeping some scratch mem for this and running our own StrPool?

src/lists.rs

@@ -14,7 +14,6 @@ use syntax::codemap::{self, CodeMap, BytePos};
 
 use utils::{round_up_to_power_of_two, make_indent};
 use comment::{FindUncommented, rewrite_comment, find_comment_end};
-use string::before;
 
 #[derive(Eq, PartialEq, Debug, Copy, Clone)]
 pub enum ListTactic {
@@ -66,6 +65,10 @@ impl ListItem {
         self.post_comment.as_ref().map(|s| s.contains('\n')).unwrap_or(false)
     }
 
+    pub fn has_line_pre_comment(&self) -> bool {
+        self.pre_comment.as_ref().map_or(false, |comment| comment.starts_with("//"))
+    }
+
     pub fn from_str<S: Into<String>>(s: S) -> ListItem {
         ListItem {
             pre_comment: None,
@@ -115,7 +118,7 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> St
     }
 
     // Switch to vertical mode if we find non-block comments.
-    if items.iter().any(has_line_pre_comment) {
+    if items.iter().any(ListItem::has_line_pre_comment) {
         tactic = ListTactic::Vertical;
     }
 
@@ -223,13 +226,6 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> St
     result
 }
 
-fn has_line_pre_comment(item: &ListItem) -> bool {
-    match item.pre_comment {
-        Some(ref comment) => comment.starts_with("//"),
-        None => false
-    }
-}
-
 // Turns a list into a vector of items with associated comments.
 // TODO: we probably do not want to take a terminator any more. Instead, we
 // should demand a proper span end.
@@ -250,6 +246,8 @@ pub fn itemize_list<T, I, F1, F2, F3>(codemap: &CodeMap,
           F3: Fn(&T) -> String
 {
     let mut result = prefix;
+    result.reserve(it.size_hint().0);
+
     let mut new_it = it.peekable();
     let white_space: &[_] = &[' ', '\t'];
 
@@ -276,14 +274,27 @@ pub fn itemize_list<T, I, F1, F2, F3>(codemap: &CodeMap,
 
         let comment_end = match new_it.peek() {
             Some(..) => {
-                if let Some(start) = before(&post_snippet, "/*", "\n") {
+                let block_open_index = post_snippet.find("/*");
+                let newline_index = post_snippet.find('\n');
+                let separator_index = post_snippet.find_uncommented(separator).unwrap();
+
+                match (block_open_index, newline_index) {
+                    // Separator before comment, with the next item on same line.
+                    // Comment belongs to next item.
+                    (Some(i), None) if i > separator_index => { separator_index + separator.len() }
+                    // Block-style post-comment before the separator.
+                    (Some(i), None) => {
+                        cmp::max(find_comment_end(&post_snippet[i..]).unwrap() + i,
+                                 separator_index + separator.len())
+                    }
                     // Block-style post-comment. Either before or after the separator.
-                    cmp::max(find_comment_end(&post_snippet[start..]).unwrap() + start,
-                             post_snippet.find_uncommented(separator).unwrap() + separator.len())
-                } else if let Some(idx) = post_snippet.find('\n') {
-                    idx + 1
-                } else {
-                    post_snippet.len()
+                    (Some(i), Some(j)) if i < j => {
+                        cmp::max(find_comment_end(&post_snippet[i..]).unwrap() + i,
+                                 separator_index + separator.len())
+                    }
+                    // Potential *single* line comment.
+                    (_, Some(j)) => { j + 1 }
+                    _ => post_snippet.len()
                 }
             },
             None => {
@@ -292,6 +303,7 @@ pub fn itemize_list<T, I, F1, F2, F3>(codemap: &CodeMap,
             }
         };
 
+        // Cleanup post-comment: strip separators and whitespace.
         prev_span_end = get_hi(&item) + BytePos(comment_end as u32);
         let mut post_snippet = post_snippet[..comment_end].trim();
 

src/string.rs

@@ -88,16 +88,3 @@ pub fn rewrite_string<'a>(s: &str, fmt: &StringFormat<'a>) -> String {
 
     result
 }
-
-#[inline]
-// Checks if a appears before b in given string and, if so, returns the index of
-// a.
-// FIXME: could be more generic
-pub fn before<'x>(s: &'x str, a: &str, b: &str) -> Option<usize> {
-    s.find(a).and_then(|i| {
-        match s.find(b) {
-            Some(j) if j <= i => None,
-            _ => Some(i)
-        }
-    })
-}

src/visitor.rs

@@ -168,7 +168,8 @@ impl<'a, 'v> visit::Visitor<'v> for FmtVisitor<'a> {
                                                               multi_line_budget,
                                                               path,
                                                               path_list,
-                                                              item.vis);
+                                                              item.vis,
+                                                              item.span);
 
                         if let Some(new_str) = formatted {
                             self.format_missing_with_indent(item.span.lo);
@@ -186,9 +187,12 @@ impl<'a, 'v> visit::Visitor<'v> for FmtVisitor<'a> {
                         self.last_pos = item.span.hi;
                     }
                     ast::ViewPath_::ViewPathGlob(_) => {
+                        self.format_missing_with_indent(item.span.lo);
                         // FIXME convert to list?
                     }
-                    ast::ViewPath_::ViewPathSimple(_,_) => {}
+                    ast::ViewPath_::ViewPathSimple(_,_) => {
+                        self.format_missing_with_indent(item.span.lo);
+                    }
                 }
                 visit::walk_item(self, item);
             }

tests/config/reorder_imports.toml

@@ -0,0 +1,14 @@
+max_width = 100
+ideal_width = 80
+leeway = 5
+tab_spaces = 4
+newline_style = "Unix"
+fn_brace_style = "SameLineWhere"
+fn_return_indent = "WithArgs"
+fn_args_paren_newline = true
+struct_trailing_comma = "Vertical"
+struct_lit_trailing_comma = "Vertical"
+enum_trailing_comma = true
+report_todo = "Always"
+report_fixme = "Never"
+reorder_imports = true

tests/config/small_tabs.toml

@@ -11,3 +11,4 @@ struct_lit_trailing_comma = "Vertical"
 enum_trailing_comma = true
 report_todo = "Always"
 report_fixme = "Never"
+reorder_imports = false

tests/source/imports-reorder.rs

@@ -0,0 +1,5 @@
+// rustfmt-config: reorder_imports.toml
+
+use path::{C,/*A*/ A, B /* B */, self /* self */};
+
+use {ab, ac, aa, Z, b};

tests/source/imports.rs

@@ -0,0 +1,41 @@
+// Imports.
+
+// Long import.
+use syntax::ast::{ItemForeignMod, ItemImpl, ItemMac, ItemMod, ItemStatic, ItemDefaultImpl};
+use exceedingly::looooooooooooooooooooooooooooooooooooooooooooooooooooooooooong::import::path::{ItemA,
+                                                                                                ItemB};
+
+use list::{
+    // Some item
+    SomeItem /* Comment */, /* Another item */ AnotherItem /* Another Comment */, // Last Item
+    LastItem
+};
+
+use test::{  Other          /* C   */  , /*   A   */ self  /*    B     */    };
+
+use syntax::{self};
+use {/* Pre-comment! */
+     Foo, Bar /* comment */};
+use Foo::{Bar, Baz};
+pub use syntax::ast::{Expr_, Expr, ExprAssign, ExprCall, ExprMethodCall, ExprPath};
+use syntax::some::{};
+
+mod Foo {
+    pub use syntax::ast::{
+        ItemForeignMod,
+        ItemImpl, 
+        ItemMac,
+        ItemMod,
+        ItemStatic, 
+        ItemDefaultImpl
+    };
+
+    mod Foo2 {
+        pub use syntax::ast::{ItemForeignMod, ItemImpl, ItemMac, ItemMod, ItemStatic, self, ItemDefaultImpl};
+    }
+}
+
+fn test() {
+use Baz::*;
+        use Qux;
+}

tests/target/imports-reorder.rs

@@ -0,0 +1,5 @@
+// rustfmt-config: reorder_imports.toml
+
+use path::{self /* self */, /* A */ A, B /* B */, C};
+
+use {Z, aa, ab, ac, b};