Auto merge of #259 - servo:serde-as-internal, r=Manishearth

Add (de)serialization of internal representation, to avoid re-parsing

 + random drive-by fixes.

The trick used here to avoid a complex `Deserialize` impl (either hand-written on in a cumbersome generated file) is to use existing impls, namely those of `Option` and `IpAddr`.

@Manishearth, another change from your PR is checking invariants unconditionally (including release mode) so there there is no need for separate `(de)serialize_unsafe` methods. Does this run-time cost sound acceptable?

r? @Manishearth

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/259)
<!-- Reviewable:end -->

Cargo.toml

@@ -27,6 +27,7 @@ test = false
 [dev-dependencies]
 rustc-test = "0.1"
 rustc-serialize = "0.3"
+serde_json = ">=0.6.1, <0.9"
 
 [features]
 query_encoding = ["encoding"]

Makefile

@@ -1,6 +1,5 @@
 test:
-	cargo test --features "query_encoding serde rustc-serialize"
-	[ x$$TRAVIS_RUST_VERSION != xnightly ] || cargo test --features heapsize
+	cargo test --features "query_encoding serde rustc-serialize heapsize"
 	(cd idna && cargo test)
 
 .PHONY: test

src/host.rs

@@ -27,6 +27,38 @@ pub enum HostInternal {
 #[cfg(feature = "heapsize")]
 known_heap_size!(0, HostInternal);
 
+#[cfg(feature="serde")]
+impl ::serde::Serialize for HostInternal {
+    fn serialize<S>(&self, serializer: &mut S) -> Result<(), S::Error> where S: ::serde::Serializer {
+        // This doesn’t use `derive` because that involves
+        // large dependencies (that take a long time to build), and
+        // either Macros 1.1 which are not stable yet or a cumbersome build script.
+        //
+        // Implementing `Serializer` correctly for an enum is tricky,
+        // so let’s use existing enums that already do.
+        use std::net::IpAddr;
+        match *self {
+            HostInternal::None => None,
+            HostInternal::Domain => Some(None),
+            HostInternal::Ipv4(addr) => Some(Some(IpAddr::V4(addr))),
+            HostInternal::Ipv6(addr) => Some(Some(IpAddr::V6(addr))),
+        }.serialize(serializer)
+    }
+}
+
+#[cfg(feature="serde")]
+impl ::serde::Deserialize for HostInternal {
+    fn deserialize<D>(deserializer: &mut D) -> Result<Self, D::Error> where D: ::serde::Deserializer {
+        use std::net::IpAddr;
+        Ok(match try!(::serde::Deserialize::deserialize(deserializer)) {
+            None => HostInternal::None,
+            Some(None) => HostInternal::Domain,
+            Some(Some(IpAddr::V4(addr))) => HostInternal::Ipv4(addr),
+            Some(Some(IpAddr::V6(addr))) => HostInternal::Ipv6(addr),
+        })
+    }
+}
+
 impl<S> From<Host<S>> for HostInternal {
     fn from(host: Host<S>) -> HostInternal {
         match host {

src/lib.rs

@@ -348,11 +348,12 @@ impl Url {
     /// This checks each of these invariants and panic if one is not met.
     /// This is for testing rust-url itself.
     #[doc(hidden)]
-    pub fn assert_invariants(&self) {
+    pub fn check_invariants(&self) -> Result<(), String> {
         macro_rules! assert {
             ($x: expr) => {
                 if !$x {
-                    panic!("!( {} ) for URL {:?}", stringify!($x), self.serialization)
+                    return Err(format!("!( {} ) for URL {:?}",
+                                       stringify!($x), self.serialization))
                 }
             }
         }
@@ -363,8 +364,9 @@ impl Url {
                     let a = $a;
                     let b = $b;
                     if a != b {
-                        panic!("{:?} != {:?} ({} != {}) for URL {:?}",
-                               a, b, stringify!($a), stringify!($b), self.serialization)
+                        return Err(format!("{:?} != {:?} ({} != {}) for URL {:?}",
+                                           a, b, stringify!($a), stringify!($b),
+                                           self.serialization))
                     }
                 }
             }
@@ -445,6 +447,7 @@ impl Url {
         assert_eq!(self.path_start, other.path_start);
         assert_eq!(self.query_start, other.query_start);
         assert_eq!(self.fragment_start, other.fragment_start);
+        Ok(())
     }
 
     /// Return the origin of this URL (https://url.spec.whatwg.org/#origin)
@@ -1514,6 +1517,60 @@ impl Url {
         Ok(url)
     }
 
+    /// Serialize with Serde using the internal representation of the `Url` struct.
+    ///
+    /// The corresponding `deserialize_internal` method sacrifices some invariant-checking
+    /// for speed, compared to the `Deserialize` trait impl.
+    ///
+    /// This method is only available if the `serde` Cargo feature is enabled.
+    #[cfg(feature = "serde")]
+    #[deny(unused)]
+    pub fn serialize_internal<S>(&self, serializer: &mut S) -> Result<(), S::Error> where S: serde::Serializer {
+        use serde::Serialize;
+        // Destructuring first lets us ensure that adding or removing fields forces this method
+        // to be updated
+        let Url { ref serialization, ref scheme_end,
+                  ref username_end, ref host_start,
+                  ref host_end, ref host, ref port,
+                  ref path_start, ref query_start,
+                  ref fragment_start} = *self;
+        (serialization, scheme_end, username_end,
+         host_start, host_end, host, port, path_start,
+         query_start, fragment_start).serialize(serializer)
+    }
+
+    /// Serialize with Serde using the internal representation of the `Url` struct.
+    ///
+    /// The corresponding `deserialize_internal` method sacrifices some invariant-checking
+    /// for speed, compared to the `Deserialize` trait impl.
+    ///
+    /// This method is only available if the `serde` Cargo feature is enabled.
+    #[cfg(feature = "serde")]
+    #[deny(unused)]
+    pub fn deserialize_internal<D>(deserializer: &mut D) -> Result<Self, D::Error> where D: serde::Deserializer {
+        use serde::{Deserialize, Error};
+        let (serialization, scheme_end, username_end,
+             host_start, host_end, host, port, path_start,
+             query_start, fragment_start) = try!(Deserialize::deserialize(deserializer));
+        let url = Url {
+            serialization: serialization,
+            scheme_end: scheme_end,
+            username_end: username_end,
+            host_start: host_start,
+            host_end: host_end,
+            host: host,
+            port: port,
+            path_start: path_start,
+            query_start: query_start,
+            fragment_start: fragment_start
+        };
+        if cfg!(debug_assertions) {
+            try!(url.check_invariants().map_err(|ref reason| Error::invalid_value(&reason)))
+        }
+        Ok(url)
+    }
+
+
     /// Assuming the URL is in the `file` scheme or similar,
     /// convert its path to an absolute `std::path::Path`.
     ///

src/path_segments.rs

@@ -177,11 +177,4 @@ impl<'a> PathSegmentsMut<'a> {
         });
         self
     }
-
-    /// For internal testing, not part of the public API.
-    #[doc(hidden)]
-    pub fn assert_url_invariants(&mut self) -> &mut Self {
-        self.url.assert_invariants();
-        self
-    }
 }

tests/data.rs

@@ -15,6 +15,16 @@ extern crate url;
 use rustc_serialize::json::{self, Json};
 use url::{Url, quirks};
 
+fn check_invariants(url: &Url) {
+    url.check_invariants().unwrap();
+    #[cfg(feature="serde")] {
+        extern crate serde_json;
+        let bytes = serde_json::to_vec(url).unwrap();
+        let new_url: Url = serde_json::from_slice(&bytes).unwrap();
+        assert_eq!(url, &new_url);
+    }
+}
+
 
 fn run_parsing(input: String, base: String, expected: Result<ExpectedAttributes, ()>) {
     let base = match Url::parse(&base) {
@@ -28,7 +38,7 @@ fn run_parsing(input: String, base: String, expected: Result<ExpectedAttributes,
         (Ok(_), Err(())) => panic!("Expected a parse error for URL {:?}", input),
     };
 
-    url.assert_invariants();
+    check_invariants(&url);
 
     macro_rules! assert_eq {
         ($expected: expr, $got: expr) => {
@@ -144,11 +154,11 @@ fn collect_setters<F>(add_test: &mut F) where F: FnMut(String, test::TestFn) {
                 let mut expected = test.take("expected").unwrap();
                 add_test(name, test::TestFn::dyn_test_fn(move || {
                     let mut url = Url::parse(&href).unwrap();
-                    url.assert_invariants();
+                    check_invariants(&url);
                     let _ = quirks::$setter(&mut url, &new_value);
                     assert_attributes!(url, expected,
                         href protocol username password host hostname port pathname search hash);
-                    url.assert_invariants();
+                    check_invariants(&url);
                 }))
             }
         }}

tests/unit.rs

@@ -259,13 +259,13 @@ fn test_form_serialize() {
 fn issue_25() {
     let filename = if cfg!(windows) { r"C:\run\pg.sock" } else { "/run/pg.sock" };
     let mut url = Url::from_file_path(filename).unwrap();
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     url.set_scheme("postgres").unwrap();
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     url.set_host(Some("")).unwrap();
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     url.set_username("me").unwrap();
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     let expected = format!("postgres://me@/{}run/pg.sock", if cfg!(windows) { "C:/" } else { "" });
     assert_eq!(url.as_str(), expected);
 }
@@ -277,7 +277,7 @@ fn issue_61() {
     url.set_scheme("https").unwrap();
     assert_eq!(url.port(), None);
     assert_eq!(url.port_or_known_default(), Some(443));
-    url.assert_invariants();
+    url.check_invariants().unwrap();
 }
 
 #[test]
@@ -285,7 +285,7 @@ fn issue_61() {
 /// https://github.com/servo/rust-url/issues/197
 fn issue_197() {
     let mut url = Url::from_file_path("/").expect("Failed to parse path");
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     assert_eq!(url, Url::parse("file:///").expect("Failed to parse path + protocol"));
     url.path_segments_mut().expect("path_segments_mut").pop_if_empty();
 }
@@ -299,9 +299,9 @@ fn issue_241() {
 /// https://github.com/servo/rust-url/issues/222
 fn append_trailing_slash() {
     let mut url: Url = "http://localhost:6767/foo/bar?a=b".parse().unwrap();
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     url.path_segments_mut().unwrap().push("");
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     assert_eq!(url.to_string(), "http://localhost:6767/foo/bar/?a=b");
 }
 
@@ -310,10 +310,10 @@ fn append_trailing_slash() {
 fn extend_query_pairs_then_mutate() {
     let mut url: Url = "http://localhost:6767/foo/bar".parse().unwrap();
     url.query_pairs_mut().extend_pairs(vec![ ("auth", "my-token") ].into_iter());
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     assert_eq!(url.to_string(), "http://localhost:6767/foo/bar?auth=my-token");
     url.path_segments_mut().unwrap().push("some_other_path");
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     assert_eq!(url.to_string(), "http://localhost:6767/foo/bar/some_other_path?auth=my-token");
 }
 
@@ -321,9 +321,9 @@ fn extend_query_pairs_then_mutate() {
 /// https://github.com/servo/rust-url/issues/222
 fn append_empty_segment_then_mutate() {
     let mut url: Url = "http://localhost:6767/foo/bar?a=b".parse().unwrap();
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     url.path_segments_mut().unwrap().push("").pop();
-    url.assert_invariants();
+    url.check_invariants().unwrap();
     assert_eq!(url.to_string(), "http://localhost:6767/foo/bar?a=b");
 }