Merge pull request #2666 from topecongiro/issue-2634

Avoid flip-flopping impl items when reordering them

Author: nrc

src/comment.rs

@@ -1139,6 +1139,7 @@ impl<'a> CommentReducer<'a> {
 
 impl<'a> Iterator for CommentReducer<'a> {
     type Item = char;
+
     fn next(&mut self) -> Option<Self::Item> {
         loop {
             let mut c = self.iter.next()?;

src/config/options.rs

@@ -239,6 +239,7 @@ impl WidthHeuristics {
             single_line_if_else_max_width: 0,
         }
     }
+
     // scale the default WidthHeuristics according to max_width
     pub fn scaled(max_width: usize) -> WidthHeuristics {
         let mut max_width_ratio: f32 = max_width as f32 / 100.0; // 100 is the default width -> default ratio is 1

src/imports.rs

@@ -799,9 +799,11 @@ mod test {
             fn bump(&mut self) {
                 self.input.next().unwrap();
             }
+
             fn eat(&mut self, c: char) {
                 assert!(self.input.next().unwrap() == c);
             }
+
             fn push_segment(
                 result: &mut Vec<UseSegment>,
                 buf: &mut String,
@@ -825,6 +827,7 @@ mod test {
                     }
                 }
             }
+
             fn parse_in_list(&mut self) -> UseTree {
                 let mut result = vec![];
                 let mut buf = String::new();

src/items.rs

@@ -578,6 +578,55 @@ impl<'a> FmtVisitor<'a> {
 
         combine_strs_with_missing_comments(&context, &attrs_str, &variant_body, span, shape, false)
     }
+
+    fn visit_impl_items(&mut self, items: &[ast::ImplItem]) {
+        if self.get_context().config.reorder_impl_items() {
+            // Create visitor for each items, then reorder them.
+            let mut buffer = vec![];
+            for item in items {
+                self.visit_impl_item(item);
+                buffer.push((self.buffer.clone(), item.clone()));
+                self.buffer.clear();
+            }
+            // type -> const -> macro -> method
+            use ast::ImplItemKind::*;
+            fn need_empty_line(a: &ast::ImplItemKind, b: &ast::ImplItemKind) -> bool {
+                match (a, b) {
+                    (Type(..), Type(..)) | (Const(..), Const(..)) => false,
+                    _ => true,
+                }
+            }
+
+            buffer.sort_by(|(_, a), (_, b)| match (&a.node, &b.node) {
+                (Type(..), _) => Ordering::Less,
+                (_, Type(..)) => Ordering::Greater,
+                (Const(..), _) => Ordering::Less,
+                (_, Const(..)) => Ordering::Greater,
+                (Macro(..), _) => Ordering::Less,
+                (_, Macro(..)) => Ordering::Greater,
+                _ => a.span.lo().cmp(&b.span.lo()),
+            });
+            let mut prev_kind = None;
+            for (buf, item) in buffer {
+                // Make sure that there are at least a single empty line between
+                // different impl items.
+                if prev_kind
+                    .as_ref()
+                    .map_or(false, |prev_kind| need_empty_line(prev_kind, &item.node))
+                {
+                    self.push_str("\n");
+                }
+                let indent_str = self.block_indent.to_string_with_newline(self.config);
+                self.push_str(&indent_str);
+                self.push_str(buf.trim());
+                prev_kind = Some(item.node.clone());
+            }
+        } else {
+            for item in items {
+                self.visit_impl_item(item);
+            }
+        }
+    }
 }
 
 pub fn format_impl(
@@ -672,51 +721,7 @@ pub fn format_impl(
             visitor.last_pos = item.span.lo() + BytePos(open_pos as u32);
 
             visitor.visit_attrs(&item.attrs, ast::AttrStyle::Inner);
-            if context.config.reorder_impl_items() {
-                // Create visitor for each items, then reorder them.
-                let mut buffer = vec![];
-                for item in items {
-                    visitor.visit_impl_item(item);
-                    buffer.push((visitor.buffer.clone(), item.clone()));
-                    visitor.buffer.clear();
-                }
-                // type -> const -> macro -> method
-                use ast::ImplItemKind::*;
-                fn need_empty_line(a: &ast::ImplItemKind, b: &ast::ImplItemKind) -> bool {
-                    match (a, b) {
-                        (Type(..), Type(..)) | (Const(..), Const(..)) => false,
-                        _ => true,
-                    }
-                }
-
-                buffer.sort_by(|(_, a), (_, b)| match (&a.node, &b.node) {
-                    (Type(..), _) => Ordering::Less,
-                    (_, Type(..)) => Ordering::Greater,
-                    (Const(..), _) => Ordering::Less,
-                    (_, Const(..)) => Ordering::Greater,
-                    (Macro(..), _) => Ordering::Less,
-                    (_, Macro(..)) => Ordering::Greater,
-                    _ => Ordering::Less,
-                });
-                let mut prev_kind = None;
-                for (buf, item) in buffer {
-                    // Make sure that there are at least a single empty line between
-                    // different impl items.
-                    if prev_kind
-                        .as_ref()
-                        .map_or(false, |prev_kind| need_empty_line(prev_kind, &item.node))
-                    {
-                        visitor.push_str("\n");
-                    }
-                    visitor.push_str(&item_indent.to_string_with_newline(context.config));
-                    visitor.push_str(buf.trim());
-                    prev_kind = Some(item.node.clone());
-                }
-            } else {
-                for item in items {
-                    visitor.visit_impl_item(item);
-                }
-            }
+            visitor.visit_impl_items(items);
 
             visitor.format_missing(item.span.hi() - BytePos(1));
 

src/test/mod.rs

@@ -568,6 +568,7 @@ struct CharsIgnoreNewlineRepr<'a>(Peekable<Chars<'a>>);
 
 impl<'a> Iterator for CharsIgnoreNewlineRepr<'a> {
     type Item = char;
+
     fn next(&mut self) -> Option<char> {
         self.0.next().map(|c| {
             if c == '\r' {

tests/source/reorder-impl-items.rs

@@ -0,0 +1,15 @@
+// rustfmt-reorder_impl_items: true
+
+// The ordering of the folllowing impl items should be idempotent.
+impl<'a> Command<'a> {
+    pub fn send_to(&self, w: &mut io::Write) -> io::Result<()> {
+        match self {
+            &Command::Data(ref c) => c.send_to(w),
+            &Command::Vrfy(ref c) => c.send_to(w),
+        }
+    }
+
+    pub fn parse(arg: &[u8]) -> Result<Command, ParseError> {
+        nom_to_result(command(arg))
+    }
+}

tests/target/reorder-impl-items.rs

@@ -0,0 +1,15 @@
+// rustfmt-reorder_impl_items: true
+
+// The ordering of the folllowing impl items should be idempotent.
+impl<'a> Command<'a> {
+    pub fn send_to(&self, w: &mut io::Write) -> io::Result<()> {
+        match self {
+            &Command::Data(ref c) => c.send_to(w),
+            &Command::Vrfy(ref c) => c.send_to(w),
+        }
+    }
+
+    pub fn parse(arg: &[u8]) -> Result<Command, ParseError> {
+        nom_to_result(command(arg))
+    }
+}