Add use declaration re-ordering (#1104)

* Add config options for combinations of lines and items

* Reordering of import lines implemented.

* Changed nested matches to tuple pattern matching

* Added ordering of path list items to the ordering of use declarations

* Move `format_imports` and `format_import` methods to `imports.rs`

* Add comment to explain how `use` declarations are split off while walking through a module

* Change `ImportReordering` config option to separate boolean options

src/config.rs

@@ -391,6 +391,8 @@ create_config! {
     chain_indent: BlockIndentStyle, BlockIndentStyle::Tabbed, "Indentation of chain";
     chains_overflow_last: bool, true, "Allow last call in method chain to break the line";
     reorder_imports: bool, false, "Reorder import statements alphabetically";
+    reorder_imported_names: bool, false,
+        "Reorder lists of names in import statements alphabetically";
     single_line_if_else_max_width: usize, 50, "Maximum line length for single line if-else \
                                                 expressions. A value of zero means always break \
                                                 if-else expressions.";

src/imports.rs

@@ -9,13 +9,120 @@
 // except according to those terms.
 
 use Indent;
+use utils;
+use syntax::codemap::{self, BytePos, Span};
 use codemap::SpanUtils;
 use lists::{write_list, itemize_list, ListItem, ListFormatting, SeparatorTactic, definitive_tactic};
 use types::rewrite_path;
 use rewrite::{Rewrite, RewriteContext};
+use visitor::FmtVisitor;
+use std::cmp::Ordering;
 
-use syntax::ast;
-use syntax::codemap::Span;
+use syntax::{ast, ptr};
+
+fn path_of(a: &ast::ViewPath_) -> &ast::Path {
+    match a {
+        &ast::ViewPath_::ViewPathSimple(_, ref p) => p,
+        &ast::ViewPath_::ViewPathGlob(ref p) => p,
+        &ast::ViewPath_::ViewPathList(ref p, _) => p,
+    }
+}
+
+fn compare_path_segments(a: &ast::PathSegment, b: &ast::PathSegment) -> Ordering {
+    a.identifier.name.as_str().cmp(&b.identifier.name.as_str())
+}
+
+fn compare_paths(a: &ast::Path, b: &ast::Path) -> Ordering {
+    for segment in a.segments.iter().zip(b.segments.iter()) {
+        let ord = compare_path_segments(segment.0, segment.1);
+        if ord != Ordering::Equal {
+            return ord;
+        }
+    }
+    a.segments.len().cmp(&b.segments.len())
+}
+
+fn compare_path_list_items(a: &ast::PathListItem, b: &ast::PathListItem) -> Ordering {
+    let name_ordering = match a.node.name() {
+        Some(a_name) => {
+            match b.node.name() {
+                Some(b_name) => a_name.name.as_str().cmp(&b_name.name.as_str()),
+                None => Ordering::Greater,
+            }
+        }
+        None => {
+            match b.node.name() {
+                Some(_) => Ordering::Less,
+                None => Ordering::Equal,
+            }
+        }
+    };
+    if name_ordering == Ordering::Equal {
+        match a.node.rename() {
+            Some(a_rename) => {
+                match b.node.rename() {
+                    Some(b_rename) => a_rename.name.as_str().cmp(&b_rename.name.as_str()),
+                    None => Ordering::Greater,
+                }
+            }
+            None => {
+                match b.node.name() {
+                    Some(_) => Ordering::Less,
+                    None => Ordering::Equal,
+                }
+            }
+        }
+    } else {
+        name_ordering
+    }
+}
+
+fn compare_path_list_item_lists(a_items: &Vec<ast::PathListItem>,
+                                b_items: &Vec<ast::PathListItem>)
+                                -> Ordering {
+    let mut a = a_items.clone();
+    let mut b = b_items.clone();
+    a.sort_by(|a, b| compare_path_list_items(a, b));
+    b.sort_by(|a, b| compare_path_list_items(a, b));
+    for comparison_pair in a.iter().zip(b.iter()) {
+        let ord = compare_path_list_items(comparison_pair.0, comparison_pair.1);
+        if ord != Ordering::Equal {
+            return ord;
+        }
+    }
+    a.len().cmp(&b.len())
+}
+
+fn compare_view_path_types(a: &ast::ViewPath_, b: &ast::ViewPath_) -> Ordering {
+    use syntax::ast::ViewPath_::*;
+    match (a, b) {
+        (&ViewPathSimple(..), &ViewPathSimple(..)) => Ordering::Equal,
+        (&ViewPathSimple(..), _) => Ordering::Less,
+        (&ViewPathGlob(_), &ViewPathSimple(..)) => Ordering::Greater,
+        (&ViewPathGlob(_), &ViewPathGlob(_)) => Ordering::Equal,
+        (&ViewPathGlob(_), &ViewPathList(..)) => Ordering::Less,
+        (&ViewPathList(_, ref a_items), &ViewPathList(_, ref b_items)) => {
+            compare_path_list_item_lists(a_items, b_items)
+        }
+        (&ViewPathList(..), _) => Ordering::Greater,
+    }
+}
+
+fn compare_view_paths(a: &ast::ViewPath_, b: &ast::ViewPath_) -> Ordering {
+    match compare_paths(path_of(a), path_of(b)) {
+        Ordering::Equal => compare_view_path_types(a, b),
+        cmp => cmp,
+    }
+}
+
+fn compare_use_items(a: &ast::Item, b: &ast::Item) -> Option<Ordering> {
+    match (&a.node, &b.node) {
+        (&ast::ItemKind::Use(ref a_vp), &ast::ItemKind::Use(ref b_vp)) => {
+            Some(compare_view_paths(&a_vp.node, &b_vp.node))
+        }
+        _ => None,
+    }
+}
 
 // TODO (some day) remove unused imports, expand globs, compress many single
 // imports into a list import.
@@ -50,6 +157,63 @@ impl Rewrite for ast::ViewPath {
     }
 }
 
+impl<'a> FmtVisitor<'a> {
+    pub fn format_imports(&mut self, use_items: &[ptr::P<ast::Item>]) {
+        let mut last_pos =
+            use_items.first().map(|p_i| p_i.span.lo - BytePos(1)).unwrap_or(self.last_pos);
+        let prefix = codemap::mk_sp(self.last_pos, last_pos);
+        let mut ordered_use_items = use_items.iter()
+            .map(|p_i| {
+                let new_item = (&*p_i, last_pos);
+                last_pos = p_i.span.hi;
+                new_item
+            })
+            .collect::<Vec<_>>();
+        // Order the imports by view-path & other import path properties
+        ordered_use_items.sort_by(|a, b| compare_use_items(a.0, b.0).unwrap());
+        // First, output the span before the first import
+        self.format_missing(prefix.hi);
+        for ordered in ordered_use_items {
+            // Fake out the formatter by setting `self.last_pos` to the appropriate location before
+            // each item before visiting it.
+            self.last_pos = ordered.1;
+            self.visit_item(&ordered.0);
+        }
+        self.last_pos = last_pos;
+    }
+
+    pub fn format_import(&mut self, vis: &ast::Visibility, vp: &ast::ViewPath, span: Span) {
+        let vis = utils::format_visibility(vis);
+        let mut offset = self.block_indent;
+        offset.alignment += vis.len() + "use ".len();
+        // 1 = ";"
+        match vp.rewrite(&self.get_context(),
+                         self.config.max_width - offset.width() - 1,
+                         offset) {
+            Some(ref s) if s.is_empty() => {
+                // Format up to last newline
+                let prev_span = codemap::mk_sp(self.last_pos, source!(self, span).lo);
+                let span_end = match self.snippet(prev_span).rfind('\n') {
+                    Some(offset) => self.last_pos + BytePos(offset as u32),
+                    None => source!(self, span).lo,
+                };
+                self.format_missing(span_end);
+                self.last_pos = source!(self, span).hi;
+            }
+            Some(ref s) => {
+                let s = format!("{}use {};", vis, s);
+                self.format_missing_with_indent(source!(self, span).lo);
+                self.buffer.push_str(&s);
+                self.last_pos = source!(self, span).hi;
+            }
+            None => {
+                self.format_missing_with_indent(source!(self, span).lo);
+                self.format_missing(source!(self, span).hi);
+            }
+        }
+    }
+}
+
 fn rewrite_single_use_list(path_str: String, vpi: &ast::PathListItem) -> String {
     let path_item_str = if let ast::PathListItemKind::Ident { name, .. } = vpi.node {
         // A name.
@@ -138,7 +302,7 @@ pub fn rewrite_use_list(width: usize,
     let has_self = move_self_to_front(&mut items);
     let first_index = if has_self { 0 } else { 1 };
 
-    if context.config.reorder_imports {
+    if context.config.reorder_imported_names {
         items[1..].sort_by(|a, b| a.item.cmp(&b.item));
     }
 

src/utils.rs

@@ -224,6 +224,13 @@ macro_rules! msg {
     )
 }
 
+// For format_missing and last_pos, need to use the source callsite (if applicable).
+// Required as generated code spans aren't guaranteed to follow on from the last span.
+macro_rules! source {
+    ($this:ident, $sp: expr) => {
+        $this.codemap.source_callsite($sp)
+    }
+}
 
 // Wraps string-like values in an Option. Returns Some when the string adheres
 // to the Rewrite constraints defined for the Rewrite trait and else otherwise.

src/visitor.rs

@@ -8,7 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use syntax::{ast, visit};
+use syntax::{ast, ptr, visit};
 use syntax::codemap::{self, CodeMap, Span, BytePos};
 use syntax::parse::ParseSess;
 
@@ -23,11 +23,10 @@ use comment::rewrite_comment;
 use macros::rewrite_macro;
 use items::{rewrite_static, rewrite_associated_type, rewrite_type_alias, format_impl, format_trait};
 
-// For format_missing and last_pos, need to use the source callsite (if applicable).
-// Required as generated code spans aren't guaranteed to follow on from the last span.
-macro_rules! source {
-    ($this:ident, $sp: expr) => {
-        $this.codemap.source_callsite($sp)
+fn is_use_item(item: &ast::Item) -> bool {
+    match item.node {
+        ast::ItemKind::Use(_) => true,
+        _ => false,
     }
 }
 
@@ -191,7 +190,7 @@ impl<'a> FmtVisitor<'a> {
         self.visit_block(b)
     }
 
-    fn visit_item(&mut self, item: &ast::Item) {
+    pub fn visit_item(&mut self, item: &ast::Item) {
         // This is where we bail out if there is a skip attribute. This is only
         // complex in the module case. It is complex because the module could be
         // in a seperate file and there might be attributes in both files, but
@@ -502,8 +501,24 @@ impl<'a> FmtVisitor<'a> {
     }
 
     fn walk_mod_items(&mut self, m: &ast::Mod) {
-        for item in &m.items {
-            self.visit_item(&item);
+        let mut items_left: &[ptr::P<ast::Item>] = &m.items;
+        while !items_left.is_empty() {
+            // If the next item is a `use` declaration, then extract it and any subsequent `use`s
+            // to be potentially reordered within `format_imports`. Otherwise, just format the
+            // next item for output.
+            if self.config.reorder_imports && is_use_item(&*items_left[0]) {
+                let use_item_length =
+                    items_left.iter().take_while(|ppi| is_use_item(&***ppi)).count();
+                let (use_items, rest) = items_left.split_at(use_item_length);
+                self.format_imports(use_items);
+                items_left = rest;
+            } else {
+                // `unwrap()` is safe here because we know `items_left`
+                // has elements from the loop condition
+                let (item, rest) = items_left.split_first().unwrap();
+                self.visit_item(&item);
+                items_left = rest;
+            }
         }
     }
 
@@ -547,37 +562,6 @@ impl<'a> FmtVisitor<'a> {
         self.format_missing(filemap.end_pos);
     }
 
-    fn format_import(&mut self, vis: &ast::Visibility, vp: &ast::ViewPath, span: Span) {
-        let vis = utils::format_visibility(vis);
-        let mut offset = self.block_indent;
-        offset.alignment += vis.len() + "use ".len();
-        // 1 = ";"
-        match vp.rewrite(&self.get_context(),
-                         self.config.max_width - offset.width() - 1,
-                         offset) {
-            Some(ref s) if s.is_empty() => {
-                // Format up to last newline
-                let prev_span = codemap::mk_sp(self.last_pos, source!(self, span).lo);
-                let span_end = match self.snippet(prev_span).rfind('\n') {
-                    Some(offset) => self.last_pos + BytePos(offset as u32),
-                    None => source!(self, span).lo,
-                };
-                self.format_missing(span_end);
-                self.last_pos = source!(self, span).hi;
-            }
-            Some(ref s) => {
-                let s = format!("{}use {};", vis, s);
-                self.format_missing_with_indent(source!(self, span).lo);
-                self.buffer.push_str(&s);
-                self.last_pos = source!(self, span).hi;
-            }
-            None => {
-                self.format_missing_with_indent(source!(self, span).lo);
-                self.format_missing(source!(self, span).hi);
-            }
-        }
-    }
-
     pub fn get_context(&self) -> RewriteContext {
         RewriteContext {
             parse_session: self.parse_session,

tests/source/imports-reorder-lines-and-items.rs

@@ -0,0 +1,9 @@
+// rustfmt-reorder_imports: true
+// rustfmt-reorder_imported_names: true
+
+use std::str;
+use std::cmp::{d, c, b, a};
+use std::ddd::aaa;
+use std::ddd::{d as p, c as g, b, a};
+// This comment should stay with `use std::ddd:bbb;`
+use std::ddd::bbb;

tests/source/imports-reorder-lines.rs

@@ -0,0 +1,34 @@
+// rustfmt-reorder_imports: true
+
+use std::str;
+use std::cmp::{d, c, b, a};
+use std::cmp::{b, e, g, f};
+use std::ddd::aaa;
+// This comment should stay with `use std::ddd;`
+use std::ddd;
+use std::ddd::bbb;
+
+mod test {
+}
+
+use aaa::bbb;
+use aaa;
+use aaa::*;
+
+mod test {}
+// If item names are equal, order by rename
+
+use test::{a as bb, b};
+use test::{a as aa, c};
+
+mod test {}
+// If item names are equal, order by rename - no rename comes before a rename
+
+use test::{a as bb, b};
+use test::{a, c};
+
+mod test {}
+// `self` always comes first
+
+use test::{a as aa, c};
+use test::{self as bb, b};

tests/source/imports-reorder.rs

@@ -1,4 +1,4 @@
-// rustfmt-reorder_imports: true
+// rustfmt-reorder_imported_names: true
 
 use path::{C,/*A*/ A, B /* B */, self /* self */};
 

tests/target/imports-reorder-lines-and-items.rs

@@ -0,0 +1,9 @@
+// rustfmt-reorder_imports: true
+// rustfmt-reorder_imported_names: true
+
+use std::cmp::{a, b, c, d};
+use std::ddd::{a, b, c as g, d as p};
+use std::ddd::aaa;
+// This comment should stay with `use std::ddd:bbb;`
+use std::ddd::bbb;
+use std::str;

tests/target/imports-reorder-lines.rs

@@ -0,0 +1,33 @@
+// rustfmt-reorder_imports: true
+
+use std::cmp::{d, c, b, a};
+use std::cmp::{b, e, g, f};
+// This comment should stay with `use std::ddd;`
+use std::ddd;
+use std::ddd::aaa;
+use std::ddd::bbb;
+use std::str;
+
+mod test {}
+
+use aaa;
+use aaa::*;
+use aaa::bbb;
+
+mod test {}
+// If item names are equal, order by rename
+
+use test::{a as aa, c};
+use test::{a as bb, b};
+
+mod test {}
+// If item names are equal, order by rename - no rename comes before a rename
+
+use test::{a, c};
+use test::{a as bb, b};
+
+mod test {}
+// `self` always comes first
+
+use test::{self as bb, b};
+use test::{a as aa, c};

tests/target/imports-reorder.rs

@@ -1,4 +1,4 @@
-// rustfmt-reorder_imports: true
+// rustfmt-reorder_imported_names: true
 
 use path::{self /* self */, /* A */ A, B /* B */, C};