Merge #39

39: Make it explicit that serialization is in u32 r=cuviper a=cuviper

Add explicit type annotations to make sure we serialize `BigUint` like a
`Vec<u32>`.  If we ever change the internal representation, we still
need to remain compatible with serialized data.

Also, deserializing `BigUint` and `BigInt` now uses their normal
constructor methods, to make sure they are normalized.  We shouldn't
assume that all incoming data is already well-formed.

Co-authored-by: Josh Stone <[email protected]>

Author: bors[bot]

Cargo.toml

@@ -41,6 +41,9 @@ version = "0.4"
 optional = true
 version = "1.0"
 
+[dev-dependencies]
+serde_test = "1.0"
+
 [dev-dependencies.rand]
 version = "0.4"
 

src/bigint.rs

@@ -73,6 +73,8 @@ impl serde::Serialize for Sign {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         where S: serde::Serializer
     {
+        // Note: do not change the serialization format, or it may break
+        // forward and backward compatibility of serialized data!
         match *self {
             Sign::Minus => (-1i8).serialize(serializer),
             Sign::NoSign => 0i8.serialize(serializer),
@@ -1701,6 +1703,8 @@ impl serde::Serialize for BigInt {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         where S: serde::Serializer
     {
+        // Note: do not change the serialization format, or it may break
+        // forward and backward compatibility of serialized data!
         (self.sign, &self.data).serialize(serializer)
     }
 }
@@ -1711,10 +1715,7 @@ impl<'de> serde::Deserialize<'de> for BigInt {
         where D: serde::Deserializer<'de>
     {
         let (sign, data) = serde::Deserialize::deserialize(deserializer)?;
-        Ok(BigInt {
-            sign: sign,
-            data: data,
-        })
+        Ok(BigInt::from_biguint(sign, data))
     }
 }
 

src/biguint.rs

@@ -1745,7 +1745,11 @@ impl serde::Serialize for BigUint {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         where S: serde::Serializer
     {
-        self.data.serialize(serializer)
+        // Note: do not change the serialization format, or it may break forward
+        // and backward compatibility of serialized data!  If we ever change the
+        // internal representation, we should still serialize in base-`u32`.
+        let data: &Vec<u32> = &self.data;
+        data.serialize(serializer)
     }
 }
 
@@ -1754,8 +1758,8 @@ impl<'de> serde::Deserialize<'de> for BigUint {
     fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
         where D: serde::Deserializer<'de>
     {
-        let data = try!(Vec::deserialize(deserializer));
-        Ok(BigUint { data: data })
+        let data: Vec<u32> = try!(Vec::deserialize(deserializer));
+        Ok(BigUint::new(data))
     }
 }
 

tests/serde.rs

@@ -0,0 +1,103 @@
+//! Test serialization and deserialization of `BigUint` and `BigInt`
+//!
+//! The serialized formats should not change, even if we change our
+//! internal representation, because we want to preserve forward and
+//! backward compatibility of serialized data!
+
+#![cfg(feature = "serde")]
+
+extern crate num_bigint;
+extern crate num_traits;
+extern crate serde_test;
+
+use num_bigint::{BigInt, BigUint};
+use num_traits::{One, Zero};
+use serde_test::{assert_tokens, Token};
+
+#[test]
+fn biguint_zero() {
+    let tokens = [Token::Seq { len: Some(0) }, Token::SeqEnd];
+    assert_tokens(&BigUint::zero(), &tokens);
+}
+
+#[test]
+fn bigint_zero() {
+    let tokens = [
+        Token::Tuple { len: 2 },
+        Token::I8(0),
+        Token::Seq { len: Some(0) },
+        Token::SeqEnd,
+        Token::TupleEnd,
+    ];
+    assert_tokens(&BigInt::zero(), &tokens);
+}
+
+#[test]
+fn biguint_one() {
+    let tokens = [Token::Seq { len: Some(1) }, Token::U32(1), Token::SeqEnd];
+    assert_tokens(&BigUint::one(), &tokens);
+}
+
+#[test]
+fn bigint_one() {
+    let tokens = [
+        Token::Tuple { len: 2 },
+        Token::I8(1),
+        Token::Seq { len: Some(1) },
+        Token::U32(1),
+        Token::SeqEnd,
+        Token::TupleEnd,
+    ];
+    assert_tokens(&BigInt::one(), &tokens);
+}
+
+#[test]
+fn bigint_negone() {
+    let tokens = [
+        Token::Tuple { len: 2 },
+        Token::I8(-1),
+        Token::Seq { len: Some(1) },
+        Token::U32(1),
+        Token::SeqEnd,
+        Token::TupleEnd,
+    ];
+    assert_tokens(&-BigInt::one(), &tokens);
+}
+
+// Generated independently from python `hex(factorial(100))`
+const FACTORIAL_100: &'static [u32] = &[
+    0x00000000, 0x00000000, 0x00000000, 0x2735c61a, 0xee8b02ea, 0xb3b72ed2, 0x9420c6ec, 0x45570cca,
+    0xdf103917, 0x943a321c, 0xeb21b5b2, 0x66ef9a70, 0xa40d16e9, 0x28d54bbd, 0xdc240695, 0x964ec395,
+    0x1b30,
+];
+
+#[test]
+fn biguint_factorial_100() {
+    let n: BigUint = (1u8..101).product();
+
+    let mut tokens = vec![];
+    tokens.push(Token::Seq {
+        len: Some(FACTORIAL_100.len()),
+    });
+    tokens.extend(FACTORIAL_100.iter().map(|&u| Token::U32(u)));
+    tokens.push(Token::SeqEnd);
+
+    assert_tokens(&n, &tokens);
+}
+
+#[test]
+fn bigint_factorial_100() {
+    let n: BigInt = (1i8..101).product();
+
+    let mut tokens = vec![];
+    tokens.push(Token::Tuple { len: 2 });
+    tokens.push(Token::I8(1));
+    tokens.push(Token::Seq {
+        len: Some(FACTORIAL_100.len()),
+    });
+    tokens.extend(FACTORIAL_100.iter().map(|&u| Token::U32(u)));
+    tokens.push(Token::SeqEnd);
+    tokens.push(Token::TupleEnd);
+
+    assert_tokens(&n, &tokens);
+}