Repository: ron-rs/ron (rust)

Issue:
Ron does not properly round trip through Value
```rust
#[macro_use]
extern crate serde;

extern crate ron;
use ron::de::*;
use ron::ser;
use ron::value::Value;

#[derive(Serialize)]
struct Scene(Option<(u32, u32)>);

#[derive(Serialize)]
struct Scene2 {
    foo: Option<(u32, u32)>,
}

fn main() {
    {
        let s = ser::to_string(&Scene2 { foo: Some((1, 1)) }).unwrap();
        println!("{}", s);
        let scene: Value = from_str(&s).unwrap();
        println!("{:?}", scene);
    }
    {
        let s = ser::to_string(&Scene( Some((1, 1)) )).unwrap();
        println!("{}", s);
        let scene: Value = from_str(&s).unwrap();
        println!("{:?}", scene);
    }
}
```

The first example gives:
```
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Parser(ExpectedIdentifier, Position { col: 12, line: 1 })', libcore/result.rs:945:5
```

The second gives:
```
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Parser(ExpectedMapColon, Position { col: 6, line: 1 })', libcore/result.rs:945:5
```

Pull request description:
Handle tuple deserialization in deserialize_any properly
Fixes #121 

Needs some more tests before merging. I'm not happy about all the lookahead that this requires, but I don't see a better solution.

Patch:
diff --git a/Cargo.toml b/Cargo.toml
index 61a75b67..5322fa05 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "ron"
-version = "0.3.0"
+version = "0.4.0"
 license = "MIT/Apache-2.0"
 keywords = ["parser", "serde", "serialization"]
 authors = [
diff --git a/src/de/mod.rs b/src/de/mod.rs
index 9d168019..1a347370 100644
--- a/src/de/mod.rs
+++ b/src/de/mod.rs
@@ -92,6 +92,32 @@ impl<'de> Deserializer<'de> {
             self.bytes.err(ParseError::TrailingCharacters)
         }
     }
+
+    /// Called from `deserialze_any` when a struct was detected. Decides if there is a
+    /// unit, tuple or usual struct and deserializes it accordingly.
+    ///
+    /// This method assumes there is no identifier left.
+    fn handle_any_struct<V>(&mut self, visitor: V) -> Result<V::Value>
+    where
+        V: Visitor<'de>
+    {
+        // Create a working copy
+        let mut bytes = self.bytes.clone();
+
+        match bytes.consume("(") {
+            true => {
+                bytes.skip_ws()?;
+
+                match bytes.check_tuple_struct()? {
+                    // first argument is technically incorrect, but ignored anyway
+                    true => self.deserialize_tuple(0, visitor),
+                    // first two arguments are technically incorrect, but ignored anyway
+                    false => self.deserialize_struct("", &[], visitor),
+                }
+            }
+            false => visitor.visit_unit(),
+        }
+    }
 }
 
 impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
@@ -113,14 +139,17 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
             return visitor.visit_unit();
         }
 
-        if self.bytes.identifier().is_ok() {
+        // `identifier` does not change state if it fails
+        let ident = self.bytes.identifier().ok();
+
+        if ident.is_some() {
             self.bytes.skip_ws()?;
 
-            return self.deserialize_struct("", &[], visitor);
+            return self.handle_any_struct(visitor);
         }
 
         match self.bytes.peek_or_eof()? {
-            b'(' => self.deserialize_struct("", &[], visitor),
+            b'(' => self.handle_any_struct(visitor),
             b'[' => self.deserialize_seq(visitor),
             b'{' => self.deserialize_map(visitor),
             b'0'...b'9' | b'+' | b'-' | b'.' => self.deserialize_f64(visitor),
@@ -361,12 +390,6 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
         }
     }
 
-    // Tuples look just like sequences in JSON. Some formats may be able to
-    // represent tuples more efficiently.
-    //
-    // As indicated by the length parameter, the `Deserialize` implementation
-    // for a tuple in the Serde data model is required to know the length of the
-    // tuple before even looking at the input data.
     fn deserialize_tuple<V>(mut self, _len: usize, visitor: V) -> Result<V::Value>
     where
         V: Visitor<'de>,
diff --git a/src/de/value.rs b/src/de/value.rs
index b2538e9c..2f06caa6 100644
--- a/src/de/value.rs
+++ b/src/de/value.rs
@@ -10,7 +10,12 @@ use value::{Number, Value};
 impl Value {
     /// Creates a value from a string reference.
     pub fn from_str(s: &str) -> de::Result<Self> {
-        Value::deserialize(&mut super::Deserializer::from_str(s)?)
+        let mut de = super::Deserializer::from_str(s)?;
+
+        let val = Value::deserialize(&mut de)?;
+        de.end()?;
+
+        Ok(val)
     }
 }
 
@@ -29,7 +34,7 @@ impl<'de> Visitor<'de> for ValueVisitor {
     type Value = Value;
 
     fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "A RON value")
+        write!(f, "a RON value")
     }
 
     fn visit_bool<E>(self, v: bool) -> Result<Self::Value, E>
@@ -177,6 +182,45 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_tuples_basic() {
+        assert_eq!(
+            eval("(3, 4, 5)"),
+            Value::Seq(
+                vec![
+                    Value::Number(Number::new(3.0)),
+                    Value::Number(Number::new(4.0)),
+                    Value::Number(Number::new(5.0)),
+                ],
+            ),
+        );
+    }
+
+    #[test]
+    fn test_tuples_ident() {
+        assert_eq!(
+            eval("(true, 3, 4, 5)"),
+            Value::Seq(
+                vec![
+                    Value::Bool(true),
+                    Value::Number(Number::new(3.0)),
+                    Value::Number(Number::new(4.0)),
+                    Value::Number(Number::new(5.0)),
+                ],
+            ),
+        );
+    }
+
+    #[test]
+    fn test_tuples_error() {
+        use de::{Error, ParseError, Position};
+
+        assert_eq!(
+            Value::from_str("Foo:").unwrap_err(),
+            Error::Parser(ParseError::TrailingCharacters, Position { col: 4, line: 1 }),
+        );
+    }
+
     #[test]
     fn test_complex() {
         assert_eq!(
diff --git a/src/parse.rs b/src/parse.rs
index 1a09e8ca..a6a19bc0 100644
--- a/src/parse.rs
+++ b/src/parse.rs
@@ -150,6 +150,19 @@ impl<'a> Bytes<'a> {
             .unwrap_or(false)
     }
 
+    /// Should only be used on a working copy
+    pub fn check_tuple_struct(mut self) -> Result<bool> {
+        if self.identifier().is_err() {
+            // if there's no field ident, this is a tuple struct
+            return Ok(true);
+        }
+
+        self.skip_ws()?;
+
+        // if there is no colon after the ident, this can only be a unit struct
+        self.eat_byte().map(|c| c != b':')
+    }
+
     /// Only returns true if the char after `ident` cannot belong
     /// to an identifier.
     pub fn consume_ident(&mut self, ident: &str) -> bool {
@@ -277,6 +290,14 @@ impl<'a> Bytes<'a> {
     }
 
     pub fn identifier(&mut self) -> Result<&'a [u8]> {
+        let bytes = self.identifier_len()?;
+        let ident = &self.bytes[..bytes];
+        let _ = self.advance(bytes);
+
+        Ok(ident)
+    }
+
+    pub fn identifier_len(&self) -> Result<usize> {
         let next = self.peek_or_eof()?;
         if IDENT_FIRST.contains(&next) {
             // If the next two bytes signify the start of a raw string literal,
@@ -289,10 +310,8 @@ impl<'a> Bytes<'a> {
             }
 
             let bytes = self.next_bytes_contained_in(IDENT_CHAR);
-            let ident = &self.bytes[..bytes];
-            let _ = self.advance(bytes);
 
-            Ok(ident)
+            Ok(bytes)
         } else {
             self.err(ParseError::ExpectedIdentifier)
         }


Test patch:
diff --git a/tests/value.rs b/tests/value.rs
index 9dae2eba..a537b0b1 100644
--- a/tests/value.rs
+++ b/tests/value.rs
@@ -1,4 +1,6 @@
 extern crate ron;
+#[macro_use]
+extern crate serde;
 
 use std::collections::BTreeMap;
 
@@ -61,5 +63,40 @@ fn seq() {
 
 #[test]
 fn unit() {
+    use ron::de::{Error, ParseError, Position};
+
     assert_eq!(Value::from_str("()"), Ok(Value::Unit));
+    assert_eq!(Value::from_str("Foo"), Ok(Value::Unit));
+
+    assert_eq!(Value::from_str(""), Err(Error::Parser(
+        ParseError::Eof,
+        Position { col: 1, line: 1 }
+    )));
+}
+
+#[derive(Serialize)]
+struct Scene(Option<(u32, u32)>);
+
+#[derive(Serialize)]
+struct Scene2 {
+    foo: Option<(u32, u32)>,
+}
+
+#[test]
+fn roundtrip() {
+    use ron::ser::to_string;
+    use ron::de::from_str;
+
+    {
+        let s = to_string(&Scene2 { foo: Some((122, 13)) }).unwrap();
+        println!("{}", s);
+        let scene: Value = from_str(&s).unwrap();
+        println!("{:?}", scene);
+    }
+    {
+        let s = to_string(&Scene( Some((13, 122)) )).unwrap();
+        println!("{}", s);
+        let scene: Value = from_str(&s).unwrap();
+        println!("{:?}", scene);
+    }
 }