Repository: ron-rs/ron (rust)

Issue:
Feature request: preserve order in value
Currently, value is based on `BTree` map, which loses the original order of ron document. I think it makes sense to add an option to preserve order. I'd even say that if ron's primary purpose is to be a human-editable format, then order should be preserved by default.  https://crates.io/crates/indexmap might come in handy do actually implement this. 


Here's my specific use-case for ordering: I [use ron](https://github.com/matklad/libsyntax2/blob/de02d2891ec7f7b295e33887d49de954f677387a/crates/libsyntax2/src/grammar.ron) to generate the AST for Rust. As such, I group related things, like types or expressions, together, and ideally I'd love to see the same order in the generated code. Alas, as I use [a map](https://github.com/matklad/libsyntax2/blob/de02d2891ec7f7b295e33887d49de954f677387a/crates/libsyntax2/src/grammar.ron#L218-L235) for some part of my input, it doesn't work. I can swtich to a list of pairs for this case, but a mapping makes more sense.

Pull request description:
Use indexmap to preserve order (optional)
Introduces an optional `indexmap` feature which uses `IndexMap` in `Value::Map` to preserve the order of deserialized elements.

Fixes #129 

Patch:
diff --git a/.travis.yml b/.travis.yml
index 06c4684d..645de1db 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -9,3 +9,7 @@ branches:
     - staging
     - trying
     - master
+
+script:
+  - cargo test
+  - cargo test --all-features
diff --git a/Cargo.toml b/Cargo.toml
index e5919482..6ac572ad 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -22,6 +22,7 @@ name = "ron"
 [dependencies]
 base64 = "0.10"
 bitflags = "1"
+indexmap = { version = "1.0.2", features = ["serde-1"], optional = true }
 serde = { version = "1", features = ["serde_derive"] }
 
 [dev-dependencies]
diff --git a/src/de/value.rs b/src/de/value.rs
index ddd50ca8..7e07c628 100644
--- a/src/de/value.rs
+++ b/src/de/value.rs
@@ -1,4 +1,4 @@
-use std::{collections::BTreeMap, fmt};
+use std::fmt;
 
 use serde::{
     de::{Error, MapAccess, SeqAccess, Visitor},
@@ -7,7 +7,7 @@ use serde::{
 
 use crate::{
     de,
-    value::{Number, Value},
+    value::{Map, Number, Value},
 };
 
 impl Value {
@@ -153,7 +153,7 @@ impl<'de> Visitor<'de> for ValueVisitor {
     where
         A: MapAccess<'de>,
     {
-        let mut res: BTreeMap<Value, Value> = BTreeMap::new();
+        let mut res: Map = Map::new();
 
         while let Some(entry) = map.next_entry()? {
             res.insert(entry.0, entry.1);
diff --git a/src/value.rs b/src/value.rs
index b0f94c31..2d1822da 100644
--- a/src/value.rs
+++ b/src/value.rs
@@ -5,16 +5,132 @@ use serde::{
         DeserializeOwned, DeserializeSeed, Deserializer, Error as SerdeError, MapAccess, SeqAccess,
         Visitor,
     },
-    forward_to_deserialize_any,
+    forward_to_deserialize_any, Deserialize, Serialize,
 };
 use std::{
     cmp::{Eq, Ordering},
-    collections::BTreeMap,
     hash::{Hash, Hasher},
+    iter::FromIterator,
+    ops::{Index, IndexMut},
 };
 
 use crate::de::{Error as RonError, Result};
 
+/// A `Value` to `Value` map.
+///
+/// This structure either uses a [BTreeMap](std::collections::BTreeMap) or the
+/// [IndexMap](indexmap::IndexMap) internally.
+/// The latter can be used by enabling the `indexmap` feature. This can be used
+/// to preserve the order of the parsed map.
+#[derive(Clone, Debug, Default, Deserialize, Serialize)]
+pub struct Map(MapInner);
+
+impl Map {
+    /// Creates a new, empty `Map`.
+    pub fn new() -> Map {
+        Default::default()
+    }
+
+    /// Returns the number of elements in the map.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if `self.len() == 0`, `false` otherwise.
+    pub fn is_empty(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Inserts a new element, returning the previous element with this `key` if
+    /// there was any.
+    pub fn insert(&mut self, key: Value, value: Value) -> Option<Value> {
+        self.0.insert(key, value)
+    }
+
+    /// Removes an element by its `key`.
+    pub fn remove(&mut self, key: &Value) -> Option<Value> {
+        self.0.remove(key)
+    }
+
+    /// Iterate all key-value pairs.
+    pub fn iter(&self) -> impl Iterator<Item = (&Value, &Value)> + DoubleEndedIterator {
+        self.0.iter()
+    }
+
+    /// Iterate all key-value pairs mutably.
+    pub fn iter_mut(&mut self) -> impl Iterator<Item = (&Value, &mut Value)> + DoubleEndedIterator {
+        self.0.iter_mut()
+    }
+
+    /// Iterate all keys.
+    pub fn keys(&self) -> impl Iterator<Item = &Value> + DoubleEndedIterator {
+        self.0.keys()
+    }
+
+    /// Iterate all values.
+    pub fn values(&self) -> impl Iterator<Item = &Value> + DoubleEndedIterator {
+        self.0.values()
+    }
+
+    /// Iterate all values mutably.
+    pub fn values_mut(&mut self) -> impl Iterator<Item = &mut Value> + DoubleEndedIterator {
+        self.0.values_mut()
+    }
+}
+
+impl FromIterator<(Value, Value)> for Map {
+    fn from_iter<T: IntoIterator<Item = (Value, Value)>>(iter: T) -> Self {
+        Map(MapInner::from_iter(iter))
+    }
+}
+
+/// Note: equality is only given if both values and order of values match
+impl Eq for Map {}
+
+impl Hash for Map {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.iter().for_each(|x| x.hash(state));
+    }
+}
+
+impl Index<&Value> for Map {
+    type Output = Value;
+
+    fn index(&self, index: &Value) -> &Self::Output {
+        &self.0[index]
+    }
+}
+
+impl IndexMut<&Value> for Map {
+    fn index_mut(&mut self, index: &Value) -> &mut Self::Output {
+        self.0.get_mut(index).expect("no entry found for key")
+    }
+}
+
+impl Ord for Map {
+    fn cmp(&self, other: &Map) -> Ordering {
+        self.iter().cmp(other.iter())
+    }
+}
+
+/// Note: equality is only given if both values and order of values match
+impl PartialEq for Map {
+    fn eq(&self, other: &Map) -> bool {
+        self.iter().zip(other.iter()).all(|(a, b)| a == b)
+    }
+}
+
+impl PartialOrd for Map {
+    fn partial_cmp(&self, other: &Map) -> Option<Ordering> {
+        self.iter().partial_cmp(other.iter())
+    }
+}
+
+#[cfg(not(feature = "indexmap"))]
+type MapInner = std::collections::BTreeMap<Value, Value>;
+#[cfg(feature = "indexmap")]
+type MapInner = indexmap::IndexMap<Value, Value>;
+
 /// A wrapper for `f64` which guarantees that the inner value
 /// is finite and thus implements `Eq`, `Hash` and `Ord`.
 #[derive(Copy, Clone, Debug)]
@@ -32,10 +148,10 @@ impl Number {
 }
 
 /// Partial equality comparison
-/// In order to be able to use `Number` as a mapping key, NaN floating values wrapped in `Number`
-/// are equals to each other. It is not the case for underlying `f64` values itself.
+/// In order to be able to use `Number` as a mapping key, NaN floating values
+/// wrapped in `Number` are equals to each other. It is not the case for
+/// underlying `f64` values itself.
 impl PartialEq for Number {
-
     fn eq(&self, other: &Self) -> bool {
         if self.0.is_nan() && other.0.is_nan() {
             return true;
@@ -45,8 +161,9 @@ impl PartialEq for Number {
 }
 
 /// Equality comparison
-/// In order to be able to use `Number` as a mapping key, NaN floating values wrapped in `Number`
-/// are equals to each other. It is not the case for underlying `f64` values itself.
+/// In order to be able to use `Number` as a mapping key, NaN floating values
+/// wrapped in `Number` are equals to each other. It is not the case for
+/// underlying `f64` values itself.
 impl Eq for Number {}
 
 impl Hash for Number {
@@ -56,9 +173,9 @@ impl Hash for Number {
 }
 
 /// Partial ordering comparison
-/// In order to be able to use `Number` as a mapping key, NaN floating values wrapped in `Number`
-/// are equals to each other and are less then any other floating value.
-/// It is not the case for underlying `f64` values itself.
+/// In order to be able to use `Number` as a mapping key, NaN floating values
+/// wrapped in `Number` are equals to each other and are less then any other
+/// floating value. It is not the case for underlying `f64` values itself.
 /// ```
 /// use ron::value::Number;
 /// assert!(Number::new(std::f64::NAN) < Number::new(std::f64::NEG_INFINITY));
@@ -70,15 +187,16 @@ impl PartialOrd for Number {
             (true, true) => Some(Ordering::Equal),
             (true, false) => Some(Ordering::Less),
             (false, true) => Some(Ordering::Greater),
-            _ => self.0.partial_cmp(&other.0)
+            _ => self.0.partial_cmp(&other.0),
         }
     }
 }
 
 /// Ordering comparison
-/// In order to be able to use `Number` as a mapping key, NaN floating values wrapped in `Number`
-/// are equals to each other and are less then any other floating value.
-/// It is not the case for underlying `f64` values itself. See the `PartialEq` implementation.
+/// In order to be able to use `Number` as a mapping key, NaN floating values
+/// wrapped in `Number` are equals to each other and are less then any other
+/// floating value. It is not the case for underlying `f64` values itself. See
+/// the `PartialEq` implementation.
 impl Ord for Number {
     fn cmp(&self, other: &Self) -> Ordering {
         self.partial_cmp(other).expect("Bug: Contract violation")
@@ -89,7 +207,7 @@ impl Ord for Number {
 pub enum Value {
     Bool(bool),
     Char(char),
-    Map(BTreeMap<Value, Value>),
+    Map(Map),
     Number(Number),
     Option(Option<Box<Value>>),
     String(String),
@@ -125,7 +243,7 @@ impl<'de> Deserializer<'de> for Value {
         match self {
             Value::Bool(b) => visitor.visit_bool(b),
             Value::Char(c) => visitor.visit_char(c),
-            Value::Map(m) => visitor.visit_map(Map {
+            Value::Map(m) => visitor.visit_map(MapAccessor {
                 keys: m.keys().cloned().rev().collect(),
                 values: m.values().cloned().rev().collect(),
             }),
@@ -204,12 +322,12 @@ impl<'de> Deserializer<'de> for Value {
     }
 }
 
-struct Map {
+struct MapAccessor {
     keys: Vec<Value>,
     values: Vec<Value>,
 }
 
-impl<'de> MapAccess<'de> for Map {
+impl<'de> MapAccess<'de> for MapAccessor {
     type Error = RonError;
 
     fn next_key_seed<K>(&mut self, seed: K) -> Result<Option<K::Value>>
@@ -257,6 +375,7 @@ mod tests {
     use super::*;
     use serde::Deserialize;
     use std::fmt::Debug;
+    use std::collections::BTreeMap;
 
     fn assert_same<'de, T>(s: &'de str)
     where


Test patch:
diff --git a/tests/117_untagged_tuple_variant.rs b/tests/117_untagged_tuple_variant.rs
deleted file mode 100644
index a11654e5..00000000
--- a/tests/117_untagged_tuple_variant.rs
+++ /dev/null
@@ -1,59 +0,0 @@
-use std::borrow::Cow;
-
-use ron::{de::from_str, ser::to_string};
-use serde::{Deserialize, Serialize};
-
-#[derive(Debug, Deserialize, Eq, PartialEq, Serialize)]
-pub struct BuildSystem<'m> {
-    version: Cow<'m, str>,
-    flags: Vec<Flag<'m>>,
-}
-
-#[derive(Debug, Deserialize, Eq, PartialEq, Serialize)]
-#[serde(untagged)]
-pub enum Flag<'m> {
-    Value(Cow<'m, str>),
-    If(Cow<'m, str>, Vec<Cow<'m, str>>),
-}
-
-#[test]
-fn test_ebkalderon_case() {
-    let file = r#"BuildSystem(
-    version: "1.0.0",
-    flags: [
-        "--enable-thing",
-        "--enable-other-thing",
-        If("some-conditional", ["--enable-third-thing"]),
-    ]
-)
-"#;
-
-    assert_eq!(
-        from_str::<BuildSystem>(file).unwrap(),
-        BuildSystem {
-            version: "1.0.0".into(),
-            flags: vec![
-                Flag::Value("--enable-thing".into()),
-                Flag::Value("--enable-other-thing".into()),
-                Flag::If(
-                    "some-conditional".into(),
-                    vec!["--enable-third-thing".into()]
-                )
-            ]
-        },
-    );
-}
-
-#[derive(Debug, Clone, Deserialize, PartialEq, Serialize)]
-#[serde(untagged)]
-enum Foo {
-    Bar(usize),
-}
-
-#[test]
-fn test_vessd_case() {
-    let foo_vec = vec![Foo::Bar(0); 5];
-    let foo_str = to_string(&foo_vec).unwrap();
-    assert_eq!(foo_str.as_str(), "[0,0,0,0,0,]");
-    assert_eq!(from_str::<Vec<Foo>>(&foo_str).unwrap(), foo_vec);
-}
diff --git a/tests/129_indexmap.rs b/tests/129_indexmap.rs
new file mode 100644
index 00000000..a19e351d
--- /dev/null
+++ b/tests/129_indexmap.rs
@@ -0,0 +1,76 @@
+#[cfg(feature = "indexmap")]
+use ron::{de::from_str, Value};
+
+#[test]
+#[cfg(feature = "indexmap")]
+fn test_order_preserved() {
+    let file = r#"(
+tasks: {
+    "debug message": Dbg(
+      msg: "test message. some text after it."
+    ),
+    "shell command": Shell(
+      command: "ls",
+      args: Some([
+        "-l",
+        "-h",
+      ]),
+      ch_dir: Some("/")
+    ),
+}
+)
+"#;
+
+    let value: Value = from_str(file).unwrap();
+    match value {
+        Value::Map(map) => match &map[&Value::String("tasks".to_owned())] {
+            Value::Map(map) => {
+                assert_eq!(
+                    *map.keys().next().unwrap(),
+                    Value::String("debug message".to_string())
+                );
+                assert_eq!(
+                    *map.keys().skip(1).next().unwrap(),
+                    Value::String("shell command".to_string())
+                );
+            }
+            _ => panic!(),
+        },
+        _ => panic!(),
+    }
+
+    let file = r#"(
+tasks: {
+    "shell command": Shell(
+      command: "ls",
+      args: Some([
+        "-l",
+        "-h",
+      ]),
+      ch_dir: Some("/")
+    ),
+    "debug message": Dbg(
+      msg: "test message. some text after it."
+    ),
+}
+)
+"#;
+
+    let value: Value = from_str(file).unwrap();
+    match value {
+        Value::Map(map) => match &map[&Value::String("tasks".to_owned())] {
+            Value::Map(map) => {
+                assert_eq!(
+                    *map.keys().next().unwrap(),
+                    Value::String("shell command".to_string())
+                );
+                assert_eq!(
+                    *map.keys().skip(1).next().unwrap(),
+                    Value::String("debug message".to_string())
+                );
+            }
+            _ => panic!(),
+        },
+        _ => panic!(),
+    }
+}
diff --git a/tests/value.rs b/tests/value.rs
index 6787dcbb..6180e597 100644
--- a/tests/value.rs
+++ b/tests/value.rs
@@ -1,6 +1,5 @@
-use ron::value::{Number, Value};
+use ron::value::{Map, Number, Value};
 use serde::Serialize;
-use std::collections::BTreeMap;
 
 #[test]
 fn bool() {
@@ -15,7 +14,7 @@ fn char() {
 
 #[test]
 fn map() {
-    let mut map = BTreeMap::new();
+    let mut map = Map::new();
     map.insert(Value::Char('a'), Value::Number(Number::new(1f64)));
     map.insert(Value::Char('b'), Value::Number(Number::new(2f64)));
     assert_eq!(Value::from_str("{ 'a': 1, 'b': 2 }"), Ok(Value::Map(map)));