Merge toml-rs#63

63: Avoid reordering tables r=ordian a=alsuren

Adds Document::to_string_in_original_order()

This is intended to help with killercup/cargo-edit#218
    
We save .position = Some(i) on each Table, starting with i = 0 for
the root of the Document, and then incrementing each time we see a
table header.

When serialising, ~we start looking for a Table with position 0,
which we should find at the root of the document. We then continue
scanning, looking for position 1 and so on. If the document doesn't
need re-ordering then we will on need to do a single scan.~ we start by 
doing a walk over the tree and working out which positions exist
(storing them in a Vec and then sorting them). We then do subsequent
walks to find each of the known positions in turn. Each walk of the tree
is guaranteed to yield at least one table, but will probably yield more.
If the document doesn't need re-ordering then we will on need to do a single scan.

If a Table has been added using the API rather than parsed then it
will have position = None. We only want to serialise each Table
once, so we only print tables with position = None if the table
we visited immediately before it had the right position.

If we didn't manage to serialise all Tables after a single loop
then we keep looping until we are done. ~There is code to make sure
we don't get stuck when someone causes a gap in the numbers by
deleting a Table.~ It is impossible for the loop to get stuck, because
it only searches for position values that it already knows are in the tree.

Known issues:
- [x] ~If you have created your Document by parsing two .toml files and
  merging the results together, this method may silently skip some
  tables. Please use Document.to_string() instead, which doesn't have
  this problem.~ fixed
- [ ] The best case performance of this function is a bit worse than
  Document.to_string(). It will be significantly worse if you have lots of
  tables that are in strange orders.


Also adds `pretty_assertions` in some tests and color-backtrace (as separate commits) because I found debugging test failures more tricky without them.

Things to do before merging:
- [x] make a version of cargo-edit which uses this patch, and make sure that it actually works on my test samples at https://github.com/alsuren/rust-repos/tree/cargo.toml-samples
  - see killercup/cargo-edit#312
  - went from 1457 funky files down to 13 (and I think that some of them are because I didn't clean up the unix/windows line endings correctly)
- [x] Write a few more integration tests, describing interesting edge cases from the repo. For example:
  - [x] An example with trailing comments after the end of the document.
  - [x] An example where `[package.metadata.deb]` is at the bottom of the file and it stays there.
    * The table_reordering test already does this.
  - [x] A test with the following document, where you add `[dependencies.somethingelse]` and it is added after `[dependencies.opencl]` rather than before `[[example]]`.
```
[package]
[dependencies]
[[example]]
[dependencies.opencl]
[dev-dependencies]
```
- [x] Is the API okay? Display.to_string() seems to swallow the error case (possibly by panicking). Should we do the same? (answer: yes)
- [x] (edit: added a commit to use this new algorithm) Is it enough to document the issue where it can silently skip tables that are parsed from multiple files, or should I check for it? If I want to fix it properly then the best way to do it is to:
  * walk the tree once, gathering all of the known `position` numbers unto a vec
  * Sort the positions vec
  * Walk the tree and pop off items from the positions vec as I go (rather than incrementing an integer)
  * Stop looping when the vec is empty.
  * This is probably simpler than my current implementation, but forces me to allocate another vec, and then do at least two passes of the tree, even if the tables are in the correct order already.
- [x] update the limitations section of the readme to explain that you should use Document::to_string_in_original_order() if you care about not forcing nested tables to be grouped next to their parents.
  - [ ] Once this PR is merged, make another PR to point `https://github.com/ordian/toml_edit/blob/f09bd5d075fdb7d2ef8d9bb3270a34506c276753/tests/test_valid.rs#L84` at the updated test.

Co-authored-by: David Laban <[email protected]>

Cargo.toml

@@ -22,6 +22,7 @@ combine = "3.5.1"
 
 [dev-dependencies]
 serde_json = "1.0.27"
+pretty_assertions = "0.6.1"
 
 [profile.dev]
 opt-level = 1

README.md

@@ -43,7 +43,7 @@ c = { d = "hello" }
 
 ## Limitations
 
-*Things it does not preserve:
+Things it does not preserve:
 * Different quotes and spaces around the same table key, e.g.
 ```toml
 [ 'a'. b]
@@ -56,15 +56,17 @@ will be represented as (spaces are removed, the first encountered quote type is
 ['a'.c]
 ['a'.d]
 ``` 
-* Children tables before parent table (tables are reordered, see [test]).
-* Scattered array of tables (tables are reordered, see [test]).
+* Children tables before parent table (tables are reordered by default, see [test]).
+* Scattered array of tables (tables are reordered by default, see [test]).
 
 The reason behind the first limitation is that `Table` does not store its header, 
 allowing us to safely swap two tables (we store a mapping in each table: child key -> child table).
 
 This last two limitations allow us to represent a toml document as a tree-like data structure, 
 which enables easier implementation of editing operations 
-and an easy to use and type-safe API.
+and an easy to use and type-safe API. If you care about the above two cases, you
+can use `Document::to_string_in_original_order()` to reconstruct tables in their
+original order.
 
 ## License
 

src/display.rs

@@ -2,7 +2,7 @@ use crate::decor::{Formatted, Repr};
 use crate::document::Document;
 use crate::table::{Item, Table};
 use crate::value::{Array, DateTime, InlineTable, Value};
-use std::fmt::{Display, Formatter, Result};
+use std::fmt::{Display, Formatter, Result, Write};
 
 impl Display for Repr {
     fn fmt(&self, f: &mut Formatter) -> Result {
@@ -110,12 +110,7 @@ impl Table {
     }
 }
 
-fn visit_table(
-    f: &mut Formatter,
-    table: &Table,
-    path: &[&str],
-    is_array_of_tables: bool,
-) -> Result {
+fn visit_table(f: &mut Write, table: &Table, path: &[&str], is_array_of_tables: bool) -> Result {
     if path.is_empty() {
         // don't print header for the root node
     } else if is_array_of_tables {
@@ -147,6 +142,35 @@ impl Display for Table {
     }
 }
 
+impl Document {
+    /// Returns a string representation of the TOML document, attempting to keep
+    /// the table headers in their original order.
+    pub fn to_string_in_original_order(&self) -> String {
+        let mut string = String::new();
+        let mut path = Vec::new();
+        let mut last_position = 0;
+        let mut tables = Vec::new();
+        self.as_table()
+            .visit_nested_tables(&mut path, false, &mut |t, p, is_array| {
+                if let Some(pos) = t.position {
+                    last_position = pos;
+                }
+                let mut s = String::new();
+                visit_table(&mut s, t, p, is_array)?;
+                tables.push((last_position, s));
+                Ok(())
+            })
+            .unwrap();
+
+        tables.sort_by_key(|&(id, _)| id);
+        for (_, table) in tables {
+            string.push_str(&table);
+        }
+        string.push_str(&self.trailing);
+        string
+    }
+}
+
 impl Display for Document {
     fn fmt(&self, f: &mut Formatter) -> Result {
         write!(f, "{}", self.as_table())?;

src/document.rs

@@ -15,7 +15,7 @@ pub struct Document {
 impl Default for Document {
     fn default() -> Self {
         Self {
-            root: Item::Table(Table::default()),
+            root: Item::Table(Table::with_pos(Some(0))),
             trailing: Default::default(),
         }
     }

src/parser/mod.rs

@@ -25,6 +25,7 @@ use crate::table::Table;
 pub struct TomlParser {
     document: Box<Document>,
     current_table: *mut Table,
+    current_table_position: usize,
 }
 
 impl Default for TomlParser {
@@ -34,6 +35,7 @@ impl Default for TomlParser {
         Self {
             document: doc,
             current_table: table,
+            current_table_position: 0,
         }
     }
 }
@@ -43,6 +45,21 @@ mod tests {
     use crate::parser::*;
     use combine::stream::state::State;
     use combine::*;
+    use pretty_assertions::assert_eq;
+    use std;
+    use std::fmt;
+    // Copied from https://github.com/colin-kiegel/rust-pretty-assertions/issues/24
+    /// Wrapper around string slice that makes debug output `{:?}` to print string same way as `{}`.
+    /// Used in different `assert*!` macros in combination with `pretty_assertions` crate to make
+    /// test failures to show nice diffs.
+    #[derive(PartialEq, Eq)]
+    struct PrettyString<'a>(pub &'a str);
+    /// Make diff to display string as multi-line string
+    impl<'a> fmt::Debug for PrettyString<'a> {
+        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+            f.write_str(self.0)
+        }
+    }
 
     macro_rules! parsed_eq {
         ($parsed:ident, $expected:expr) => {{
@@ -461,7 +478,7 @@ that
             assert!(doc.is_ok());
             let doc = doc.unwrap();
 
-            assert_eq!(&doc.to_string(), document);
+            assert_eq!(PrettyString(document), PrettyString(&doc.to_string()));
         }
 
         let invalid_inputs = [r#" hello = 'darkness' # my old friend

src/parser/table.rs

@@ -130,6 +130,7 @@ impl TomlParser {
 
         let leading = mem::replace(&mut self.document.trailing, InternalString::new());
         let table = self.document.as_table_mut();
+        self.current_table_position += 1;
 
         let table = Self::descend_path(table, &path[..path.len() - 1], 0);
         let key = &path[path.len() - 1];
@@ -140,7 +141,10 @@ impl TomlParser {
 
                 let entry = table.entry(key.raw());
                 if entry.is_none() {
-                    *entry = Item::Table(Table::with_decor(decor));
+                    *entry = Item::Table(Table::with_decor_and_pos(
+                        decor,
+                        Some(self.current_table_position),
+                    ));
                     self.current_table = entry.as_table_mut().unwrap();
                     return Ok(());
                 }
@@ -149,6 +153,7 @@ impl TomlParser {
                     Item::Table(ref mut t) if t.implicit => {
                         debug_assert!(t.values_len() == 0);
                         t.decor = decor;
+                        t.position = Some(self.current_table_position);
                         t.set_implicit(false);
                         self.current_table = t;
                         return Ok(());
@@ -179,7 +184,12 @@ impl TomlParser {
                         .entry(key.raw())
                         .or_insert(Item::ArrayOfTables(ArrayOfTables::new()));
                     let array = entry.as_array_of_tables_mut().unwrap();
-                    self.current_table = array.append(Table::with_decor(decor));
+
+                    self.current_table_position += 1;
+                    self.current_table = array.append(Table::with_decor_and_pos(
+                        decor,
+                        Some(self.current_table_position),
+                    ));
 
                     Ok(())
                 } else {

src/table.rs

@@ -15,6 +15,9 @@ pub struct Table {
     pub(crate) decor: Decor,
     // whether to hide an empty table
     pub(crate) implicit: bool,
+    // used for putting tables back in their original order when serialising.
+    // Will be None when the Table wasn't parsed from a file.
+    pub(crate) position: Option<usize>,
 }
 
 pub(crate) type KeyValuePairs = LinkedHashMap<InternalString, TableKeyValue>;
@@ -58,12 +61,20 @@ pub type Iter<'a> = Box<dyn Iterator<Item = (&'a str, &'a Item)> + 'a>;
 impl Table {
     /// Creates an empty table.
     pub fn new() -> Self {
-        Self::with_decor(Decor::new("\n", ""))
+        Self::with_decor_and_pos(Decor::new("\n", ""), None)
     }
 
-    pub(crate) fn with_decor(decor: Decor) -> Self {
+    pub(crate) fn with_pos(position: Option<usize>) -> Self {
+        Self {
+            position,
+            ..Default::default()
+        }
+    }
+
+    pub(crate) fn with_decor_and_pos(decor: Decor, position: Option<usize>) -> Self {
         Self {
             decor,
+            position,
             ..Default::default()
         }
     }
@@ -386,6 +397,8 @@ impl TableLike for Table {
 /// # Examples
 /// ```rust
 /// # extern crate toml_edit;
+/// # extern crate pretty_assertions;
+/// # use pretty_assertions::assert_eq;
 /// # use toml_edit::*;
 /// # fn main() {
 /// let mut table = Table::default();

tests/test_edit.rs

@@ -1,3 +1,4 @@
+extern crate pretty_assertions;
 extern crate toml_edit;
 
 macro_rules! parse_key {
@@ -21,6 +22,21 @@ macro_rules! as_table {
 mod tests {
     use toml_edit::{Document, Key, Value, Table, Item, value, table, array};
     use std::iter::FromIterator;
+    use std::fmt;
+    use pretty_assertions::assert_eq;
+
+    // Copied from https://github.com/colin-kiegel/rust-pretty-assertions/issues/24
+    /// Wrapper around string slice that makes debug output `{:?}` to print string same way as `{}`.
+    /// Used in different `assert*!` macros in combination with `pretty_assertions` crate to make
+    /// test failures to show nice diffs.
+    #[derive(PartialEq, Eq)]
+    struct PrettyString<'a>(pub &'a str);
+    /// Make diff to display string as multi-line string
+    impl<'a> fmt::Debug for PrettyString<'a> {
+        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+            f.write_str(self.0)
+        }
+    }
 
     struct Test {
         doc: Document,
@@ -45,8 +61,23 @@ mod tests {
             self
         }
 
-        fn produces(&self, expected: &str) {
-            assert_eq!(self.doc.to_string(), expected);
+        fn produces_display(&self, expected: &str) -> &Self {
+            assert_eq!(
+                PrettyString(expected),
+                PrettyString(&self.doc.to_string()));
+            self
+        }
+
+        fn produces_in_original_order(&self, expected: &str) -> &Self {
+            assert_eq!(
+                PrettyString(expected),
+                PrettyString(&self.doc.to_string_in_original_order()));
+            self
+        }
+
+        fn produces(&self, expected: &str) -> &Self {
+            self.produces_display(expected).produces_in_original_order(expected);
+            self
         }
     }
 
@@ -82,6 +113,46 @@ dc = "eqdc10"
     );
 }
 
+#[test]
+fn test_inserted_leaf_table_goes_after_last_sibling() {
+    given(r#"
+        [package]
+        [dependencies]
+        [[example]]
+        [dependencies.opencl]
+        [dev-dependencies]"#
+    ).running(|root| {
+        root["dependencies"]["newthing"] = table();
+    }).produces_display(r#"
+        [package]
+        [dependencies]
+        [dependencies.opencl]
+
+[dependencies.newthing]
+        [[example]]
+        [dev-dependencies]
+"#).produces_in_original_order(r#"
+        [package]
+        [dependencies]
+        [[example]]
+        [dependencies.opencl]
+
+[dependencies.newthing]
+        [dev-dependencies]
+"#);
+}
+
+#[test]
+fn test_inserting_tables_from_different_parsed_docs() {
+    given(
+        "[a]"
+    ).running(|root| {
+        let other = "[b]".parse::<Document>().unwrap();
+        root["b"] = other["b"].clone();
+    }).produces(
+        "[a]\n[b]\n"
+    );
+}
 #[test]
 fn test_insert_nonleaf_table() {
     given(r#"
@@ -358,7 +429,7 @@ fn test_sort_values() {
         let a = root.entry("a");
         let a = as_table!(a);
         a.sort_values();
-    }).produces(r#"
+    }).produces_display(r#"
         [a]
         a = 1
         # this comment is attached to b
@@ -369,7 +440,17 @@ fn test_sort_values() {
 
         [a.y]
 "#
-    );
+    ).produces_in_original_order(r#"
+        [a.z]
+
+        [a]
+        a = 1
+        # this comment is attached to b
+        b = 2 # as well as this
+        c = 3
+
+        [a.y]
+"#);
 }
 
 macro_rules! as_array {