Replace () error types in several functions.

This is an initial pass at #299.

It does not change `ParseError` to the more idiomatic `Error` name, or
change `with_default_port`'s return type.

Author: tmccombs

src/lib.rs

@@ -1467,10 +1467,10 @@ impl Url {
     /// # }
     /// # run().unwrap();
     /// ```
-    pub fn set_port(&mut self, mut port: Option<u16>) -> Result<(), ()> {
+    pub fn set_port(&mut self, mut port: Option<u16>) -> Result<(), ParseError> {
         // has_host implies !cannot_be_a_base
         if !self.has_host() || self.host() == Some(Host::Domain("")) || self.scheme() == "file" {
-            return Err(())
+            return Err(ParseError::EmptyHost)
         }
         if port.is_some() && port == parser::default_port(self.scheme()) {
             port = None
@@ -1697,9 +1697,9 @@ impl Url {
     /// # run().unwrap();
     /// ```
     ///
-    pub fn set_ip_host(&mut self, address: IpAddr) -> Result<(), ()> {
+    pub fn set_ip_host(&mut self, address: IpAddr) -> Result<(), ParseError> {
         if self.cannot_be_a_base() {
-            return Err(())
+            return Err(ParseError::SetHostOnCannotBeABaseUrl)
         }
 
         let address = match address {
@@ -1736,10 +1736,10 @@ impl Url {
     /// # }
     /// # run().unwrap();
     /// ```
-    pub fn set_password(&mut self, password: Option<&str>) -> Result<(), ()> {
+    pub fn set_password(&mut self, password: Option<&str>) -> Result<(), ParseError> {
         // has_host implies !cannot_be_a_base
         if !self.has_host() || self.host() == Some(Host::Domain("")) || self.scheme() == "file" {
-            return Err(())
+            return Err(ParseError::EmptyHost)
         }
         if let Some(password) = password {
             let host_and_after = self.slice(self.host_start..).to_owned();
@@ -1818,10 +1818,10 @@ impl Url {
     /// # }
     /// # run().unwrap();
     /// ```
-    pub fn set_username(&mut self, username: &str) -> Result<(), ()> {
+    pub fn set_username(&mut self, username: &str) -> Result<(), ParseError> {
         // has_host implies !cannot_be_a_base
         if !self.has_host() || self.host() == Some(Host::Domain("")) || self.scheme() == "file" {
-            return Err(())
+            return Err(ParseError::EmptyHost)
         }
         let username_start = self.scheme_end + 3;
         debug_assert!(self.slice(self.scheme_end..username_start) == "://");
@@ -1922,12 +1922,12 @@ impl Url {
     /// # }
     /// # run().unwrap();
     /// ```
-    pub fn set_scheme(&mut self, scheme: &str) -> Result<(), ()> {
+    pub fn set_scheme(&mut self, scheme: &str) -> Result<(), ParseError> {
         let mut parser = Parser::for_setter(String::new());
         let remaining = parser.parse_scheme(parser::Input::new(scheme))?;
         if !remaining.is_empty() ||
                 (!self.has_host() && SchemeType::from(&parser.serialization).is_special()) {
-            return Err(())
+            return Err(ParseError::InvalidScheme)
         }
         let old_scheme_end = self.scheme_end;
         let new_scheme_end = to_u32(parser.serialization.len()).unwrap();
@@ -1962,7 +1962,7 @@ impl Url {
     /// # if cfg!(unix) {
     /// use url::Url;
     ///
-    /// # fn run() -> Result<(), ()> {
+    /// # fn run() -> Result<(), url::ParseError> {
     /// let url = Url::from_file_path("/tmp/foo.txt")?;
     /// assert_eq!(url.as_str(), "file:///tmp/foo.txt");
     ///
@@ -1977,7 +1977,7 @@ impl Url {
     /// # }
     /// ```
     #[cfg(any(unix, windows, target_os="redox"))]
-    pub fn from_file_path<P: AsRef<Path>>(path: P) -> Result<Url, ()> {
+    pub fn from_file_path<P: AsRef<Path>>(path: P) -> Result<Url, ParseError> {
         let mut serialization = "file://".to_owned();
         let host_start = serialization.len() as u32;
         let (host_end, host) = path_to_file_url_segments(path.as_ref(), &mut serialization)?;
@@ -2013,7 +2013,7 @@ impl Url {
     /// Note that `std::path` does not consider trailing slashes significant
     /// and usually does not include them (e.g. in `Path::parent()`).
     #[cfg(any(unix, windows, target_os="redox"))]
-    pub fn from_directory_path<P: AsRef<Path>>(path: P) -> Result<Url, ()> {
+    pub fn from_directory_path<P: AsRef<Path>>(path: P) -> Result<Url, ParseError> {
         let mut url = Url::from_file_path(path)?;
         if !url.serialization.ends_with('/') {
             url.serialization.push('/')
@@ -2088,26 +2088,26 @@ impl Url {
     /// let path = url.to_file_path();
     /// ```
     ///
-    /// Returns `Err` if the host is neither empty nor `"localhost"` (except on Windows, where
+    /// Returns `Err(ParseError::InvalidLocalPath)` if the host is neither empty nor `"localhost"` (except on Windows, where
     /// `file:` URLs may have a non-local host),
     /// or if `Path::new_opt()` returns `None`.
     /// (That is, if the percent-decoded path contains a NUL byte or,
     /// for a Windows path, is not UTF-8.)
     #[inline]
     #[cfg(any(unix, windows, target_os="redox"))]
-    pub fn to_file_path(&self) -> Result<PathBuf, ()> {
+    pub fn to_file_path(&self) -> Result<PathBuf, ParseError> {
         if let Some(segments) = self.path_segments() {
             let host = match self.host() {
                 None | Some(Host::Domain("localhost")) => None,
                 Some(_) if cfg!(windows) && self.scheme() == "file" => {
                     Some(&self.serialization[self.host_start as usize .. self.host_end as usize])
                 },
-                _ => return Err(())
+                _ => return Err(ParseError::InvalidLocalPath)
             };
 
             return file_url_segments_to_pathbuf(host, segments);
         }
-        Err(())
+        Err(ParseError::InvalidLocalPath)
     }
 
     // Private helper methods:
@@ -2268,10 +2268,10 @@ impl serde::Deserialize for Url {
 
 #[cfg(any(unix, target_os = "redox"))]
 fn path_to_file_url_segments(path: &Path, serialization: &mut String)
-                             -> Result<(u32, HostInternal), ()> {
+                             -> Result<(u32, HostInternal), ParseError> {
     use std::os::unix::prelude::OsStrExt;
     if !path.is_absolute() {
-        return Err(())
+        return Err(ParseError::PathNotAbsolute)
     }
     let host_end = to_u32(serialization.len()).unwrap();
     let mut empty = true;
@@ -2343,13 +2343,13 @@ fn path_to_file_url_segments_windows(path: &Path, serialization: &mut String)
 
 
 #[cfg(any(unix, target_os = "redox"))]
-fn file_url_segments_to_pathbuf(host: Option<&str>, segments: str::Split<char>) -> Result<PathBuf, ()> {
+fn file_url_segments_to_pathbuf(host: Option<&str>, segments: str::Split<char>) -> Result<PathBuf, ParseError> {
     use std::ffi::OsStr;
     use std::os::unix::prelude::OsStrExt;
     use std::path::PathBuf;
 
     if host.is_some() {
-        return Err(());
+        return Err(ParseError::InvalidLocalPath);
     }
 
     let mut bytes = if cfg!(target_os = "redox") {

src/parser.rs

@@ -56,13 +56,16 @@ simple_enum_error! {
     EmptyHost => "empty host",
     IdnaError => "invalid international domain name",
     InvalidPort => "invalid port number",
+    InvalidScheme => "invalid scheme",
     InvalidIpv4Address => "invalid IPv4 address",
     InvalidIpv6Address => "invalid IPv6 address",
     InvalidDomainCharacter => "invalid domain character",
     RelativeUrlWithoutBase => "relative URL without a base",
     RelativeUrlWithCannotBeABaseBase => "relative URL with a cannot-be-a-base base",
     SetHostOnCannotBeABaseUrl => "a cannot-be-a-base URL doesn’t have a host to set",
     Overflow => "URLs more than 4 GB are not supported",
+    InvalidLocalPath => "the url is not a valid local path",
+    PathNotAbsolute => "path component of url is not absolute",
 }
 
 #[cfg(feature = "heapsize")]
@@ -372,9 +375,9 @@ impl<'a> Parser<'a> {
         }
     }
 
-    pub fn parse_scheme<'i>(&mut self, mut input: Input<'i>) -> Result<Input<'i>, ()> {
+    pub fn parse_scheme<'i>(&mut self, mut input: Input<'i>) -> Result<Input<'i>, ParseError> {
         if input.is_empty() || !input.starts_with(ascii_alpha) {
-            return Err(())
+            return Err(ParseError::InvalidScheme)
         }
         debug_assert!(self.serialization.is_empty());
         while let Some(c) = input.next() {
@@ -385,7 +388,7 @@ impl<'a> Parser<'a> {
                 ':' => return Ok(input),
                 _ => {
                     self.serialization.clear();
-                    return Err(())
+                    return Err(ParseError::InvalidScheme)
                 }
             }
         }
@@ -394,7 +397,7 @@ impl<'a> Parser<'a> {
             Ok(input)
         } else {
             self.serialization.clear();
-            Err(())
+            Err(ParseError::InvalidScheme)
         }
     }
 

src/quirks.rs

@@ -56,7 +56,7 @@ pub fn protocol(url: &Url) -> &str {
 }
 
 /// Setter for https://url.spec.whatwg.org/#dom-url-protocol
-pub fn set_protocol(url: &mut Url, mut new_protocol: &str) -> Result<(), ()> {
+pub fn set_protocol(url: &mut Url, mut new_protocol: &str) -> Result<(), ParseError> {
     // The scheme state in the spec ignores everything after the first `:`,
     // but `set_scheme` errors if there is more.
     if let Some(position) = new_protocol.find(':') {
@@ -72,7 +72,7 @@ pub fn username(url: &Url) -> &str {
 }
 
 /// Setter for https://url.spec.whatwg.org/#dom-url-username
-pub fn set_username(url: &mut Url, new_username: &str) -> Result<(), ()> {
+pub fn set_username(url: &mut Url, new_username: &str) -> Result<(), ParseError> {
     url.set_username(new_username)
 }
 
@@ -83,7 +83,7 @@ pub fn password(url: &Url) -> &str {
 }
 
 /// Setter for https://url.spec.whatwg.org/#dom-url-password
-pub fn set_password(url: &mut Url, new_password: &str) -> Result<(), ()> {
+pub fn set_password(url: &mut Url, new_password: &str) -> Result<(), ParseError> {
     url.set_password(if new_password.is_empty() { None } else { Some(new_password) })
 }
 

tests/unit.rs

@@ -16,7 +16,7 @@ use std::borrow::Cow;
 use std::cell::{Cell, RefCell};
 use std::net::{Ipv4Addr, Ipv6Addr};
 use std::path::{Path, PathBuf};
-use url::{Host, HostAndPort, Url, form_urlencoded};
+use url::{Host, HostAndPort, Url, form_urlencoded, ParseError};
 
 #[test]
 fn size() {
@@ -39,14 +39,14 @@ macro_rules! assert_from_file_path {
 #[test]
 fn new_file_paths() {
     if cfg!(unix) {
-        assert_eq!(Url::from_file_path(Path::new("relative")), Err(()));
-        assert_eq!(Url::from_file_path(Path::new("../relative")), Err(()));
+        assert_eq!(Url::from_file_path(Path::new("relative")), Err(ParseError::PathNotAbsolute));
+        assert_eq!(Url::from_file_path(Path::new("../relative")), Err(ParseError::PathNotAbsolute));
     }
     if cfg!(windows) {
-        assert_eq!(Url::from_file_path(Path::new("relative")), Err(()));
-        assert_eq!(Url::from_file_path(Path::new(r"..\relative")), Err(()));
-        assert_eq!(Url::from_file_path(Path::new(r"\drive-relative")), Err(()));
-        assert_eq!(Url::from_file_path(Path::new(r"\\ucn\")), Err(()));
+        assert_eq!(Url::from_file_path(Path::new("relative")), Err(ParseError::PathNotAbsolute));
+        assert_eq!(Url::from_file_path(Path::new(r"..\relative")), Err(ParseError::PathNotAbsolute));
+        assert_eq!(Url::from_file_path(Path::new(r"\drive-relative")), Err(ParseError::PathNotAbsolute));
+        assert_eq!(Url::from_file_path(Path::new(r"\\ucn\")), Err(ParseError::PathNotAbsolute));
     }
 
     if cfg!(unix) {
@@ -89,19 +89,20 @@ fn new_path_windows_fun() {
 
 #[test]
 fn new_directory_paths() {
+    use url::ParseError;
     if cfg!(unix) {
-        assert_eq!(Url::from_directory_path(Path::new("relative")), Err(()));
-        assert_eq!(Url::from_directory_path(Path::new("../relative")), Err(()));
+        assert_eq!(Url::from_directory_path(Path::new("relative")), Err(ParseError::PathNotAbsolute));
+        assert_eq!(Url::from_directory_path(Path::new("../relative")), Err(ParseError::PathNotAbsolute));
 
         let url = Url::from_directory_path(Path::new("/foo/bar")).unwrap();
         assert_eq!(url.host(), None);
         assert_eq!(url.path(), "/foo/bar/");
     }
     if cfg!(windows) {
-        assert_eq!(Url::from_directory_path(Path::new("relative")), Err(()));
-        assert_eq!(Url::from_directory_path(Path::new(r"..\relative")), Err(()));
-        assert_eq!(Url::from_directory_path(Path::new(r"\drive-relative")), Err(()));
-        assert_eq!(Url::from_directory_path(Path::new(r"\\ucn\")), Err(()));
+        assert_eq!(Url::from_directory_path(Path::new("relative")), Err(ParseError::PathNotAbsolute));
+        assert_eq!(Url::from_directory_path(Path::new(r"..\relative")), Err(ParseError::PathNotAbsolute));
+        assert_eq!(Url::from_directory_path(Path::new(r"\drive-relative")), Err(ParseError::PathNotAbsolute));
+        assert_eq!(Url::from_directory_path(Path::new(r"\\ucn\")), Err(ParseError::PathNotAbsolute));
 
         let url = Url::from_directory_path(Path::new(r"C:\foo\bar")).unwrap();
         assert_eq!(url.host(), None);