Refactor error handling in Store.

* Fix infinite recursion in `fmt::Display::fmt()` for `Error`.
* Make `Store` generic over the error type. This allows callers to
  provide their own error types that unify with Dogear errors,
  `nsresult`s, and others, instead of requiring them to wrap their
  errors into `ErrorKind::Storage(...)`.
* Forward decoding errors from `Guid::from{utf8, uft16}()`.
* Rename `ErrorKind::GenerateGuid` to `ErrorKind::InvalidGuid`.
* Move `dogear::merge` into `Store::merge`.

src/error.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::{error, fmt, result};
+use std::{error, fmt, result, str::Utf8Error, string::FromUtf16Error};
 
 use guid::Guid;
 use tree::Kind;
@@ -28,36 +28,36 @@ impl Error {
     }
 }
 
-impl error::Error for Error {}
+impl error::Error for Error {
+    fn source(&self) -> Option<&(dyn error::Error + 'static)> {
+        match self.kind() {
+            ErrorKind::MalformedString(err) => Some(err.as_ref()),
+            _ => None
+        }
+    }
+}
 
 impl From<ErrorKind> for Error {
     fn from(kind: ErrorKind) -> Error {
         Error(kind)
     }
 }
 
-impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "{}", self)
+impl From<FromUtf16Error> for Error {
+    fn from(error: FromUtf16Error) -> Error {
+        Error(ErrorKind::MalformedString(error.into()))
     }
 }
 
-#[derive(Debug)]
-pub enum ErrorKind {
-    MismatchedItemKind(Kind, Kind),
-    DuplicateItem(Guid),
-    InvalidParent(Guid, Guid),
-    MissingParent(Guid, Guid),
-    Storage(&'static str, u32),
-    MergeConflict,
-    UnmergedLocalItems,
-    UnmergedRemoteItems,
-    GenerateGuid(Guid),
+impl From<Utf8Error> for Error {
+    fn from(error: Utf8Error) -> Error {
+        Error(ErrorKind::MalformedString(error.into()))
+    }
 }
 
-impl fmt::Display for ErrorKind {
+impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match self {
+        match self.kind() {
             ErrorKind::MismatchedItemKind(local_kind, remote_kind) => {
                 write!(f, "Can't merge local kind {} and remote kind {}",
                        local_kind, remote_kind)
@@ -73,9 +73,6 @@ impl fmt::Display for ErrorKind {
                 write!(f, "Can't insert item {} into nonexistent parent {}",
                        child_guid, parent_guid)
             },
-            ErrorKind::Storage(message, result) => {
-                write!(f, "Storage error: {} ({})", message, result)
-            },
             ErrorKind::MergeConflict => {
                 write!(f, "Local tree changed during merge")
             },
@@ -85,9 +82,27 @@ impl fmt::Display for ErrorKind {
             ErrorKind::UnmergedRemoteItems => {
                 write!(f, "Merged tree doesn't mention all items from remote tree")
             },
-            ErrorKind::GenerateGuid(invalid_guid) => {
-                write!(f, "Failed to generate new GUID for {}", invalid_guid)
-            }
+            ErrorKind::InvalidGuid(invalid_guid) => {
+                write!(f, "Merged tree contains invalid GUID {}", invalid_guid)
+            },
+            ErrorKind::InvalidByte(b) => {
+                write!(f, "Invalid byte {} in UTF-16 encoding", b)
+            },
+            ErrorKind::MalformedString(err) => err.fmt(f),
         }
     }
 }
+
+#[derive(Debug)]
+pub enum ErrorKind {
+    MismatchedItemKind(Kind, Kind),
+    DuplicateItem(Guid),
+    InvalidParent(Guid, Guid),
+    MissingParent(Guid, Guid),
+    MergeConflict,
+    UnmergedLocalItems,
+    UnmergedRemoteItems,
+    InvalidGuid(Guid),
+    InvalidByte(u16),
+    MalformedString(Box<dyn error::Error + Send + Sync + 'static>),
+}

src/guid.rs

@@ -14,6 +14,8 @@
 
 use std::{fmt, str, ops};
 
+use error::{Result, ErrorKind};
+
 #[derive(Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
 pub struct Guid(Repr);
 
@@ -54,37 +56,37 @@ const VALID_GUID_BYTES: [u8; 255] =
      0, 0, 0, 0, 0, 0, 0];
 
 impl Guid {
-    pub fn from_utf8(b: &[u8]) -> Option<Guid> {
+    pub fn from_utf8(b: &[u8]) -> Result<Guid> {
         let repr = if Guid::is_valid(b) {
             let mut bytes = [0u8; 12];
             bytes.copy_from_slice(b);
             Repr::Fast(bytes)
         } else {
             match str::from_utf8(b) {
                 Ok(s) => Repr::Slow(s.into()),
-                Err(_) => return None
+                Err(err) => return Err(err.into()),
             }
         };
-        Some(Guid(repr))
+        Ok(Guid(repr))
     }
 
-    pub fn from_utf16(b: &[u16]) -> Option<Guid> {
+    pub fn from_utf16(b: &[u16]) -> Result<Guid> {
         let repr = if Guid::is_valid(b) {
             let mut bytes = [0u8; 12];
             for (index, byte) in b.iter().enumerate() {
                 match byte.into_byte() {
                     Some(byte) => bytes[index] = byte,
-                    None => return None
+                    None => return Err(ErrorKind::InvalidByte(*byte).into()),
                 };
             }
             Repr::Fast(bytes)
         } else {
             match String::from_utf16(b) {
                 Ok(s) => Repr::Slow(s),
-                Err(_) => return None
+                Err(err) => return Err(err.into()),
             }
         };
-        Some(Guid(repr))
+        Ok(Guid(repr))
     }
 
     #[inline]

src/lib.rs

@@ -27,5 +27,5 @@ mod tests;
 pub use error::{Error, ErrorKind, Result};
 pub use guid::{Guid, ROOT_GUID, USER_CONTENT_ROOTS};
 pub use merge::{Deletion, Merger, StructureCounts};
-pub use store::{Store, merge};
-pub use tree::{Content, Item, Kind, MergeState, MergedNode, Tree};
+pub use store::Store;
+pub use tree::{Child, Content, Item, Kind, MergeState, MergedNode, ParentGuidFrom, Tree};

src/merge.rs

@@ -90,7 +90,7 @@ impl Driver for DefaultDriver {
     /// The default implementation returns an error and fails the merge if any
     /// items have invalid GUIDs.
     fn generate_new_guid(&self, invalid_guid: &Guid) -> Result<Guid> {
-        Err(ErrorKind::GenerateGuid(invalid_guid.clone()).into())
+        Err(ErrorKind::InvalidGuid(invalid_guid.clone()).into())
     }
 }
 

src/store.rs

@@ -1,28 +1,28 @@
 use std::collections::HashMap;
 
-use error::{ErrorKind, Result};
+use error::{Error, ErrorKind};
 use guid::Guid;
 use merge::{Merger, Deletion};
 use tree::{Content, MergedNode, Tree};
 
-pub trait Store {
+pub trait Store<E: From<Error>> {
     /// Builds a fully rooted, consistent tree from the items and tombstones in
     /// the local store.
-    fn fetch_local_tree(&self, local_time_millis: i64) -> Result<Tree>;
+    fn fetch_local_tree(&self, local_time_millis: i64) -> Result<Tree, E>;
 
     /// Fetches content info for all new local items that haven't been uploaded
     /// or merged yet. We'll try to dedupe them to remotely changed items with
     /// similar contents and different GUIDs.
-    fn fetch_new_local_contents(&self) -> Result<HashMap<Guid, Content>>;
+    fn fetch_new_local_contents(&self) -> Result<HashMap<Guid, Content>, E>;
 
     /// Builds a fully rooted, consistent tree from the items and tombstones in
     /// the mirror.
-    fn fetch_remote_tree(&self, remote_time_millis: i64) -> Result<Tree>;
+    fn fetch_remote_tree(&self, remote_time_millis: i64) -> Result<Tree, E>;
 
     /// Fetches content info for all items in the mirror that changed since the
     /// last sync and don't exist locally. We'll try to match new local items to
     /// these.
-    fn fetch_new_remote_contents(&self) -> Result<HashMap<Guid, Content>>;
+    fn fetch_new_remote_contents(&self) -> Result<HashMap<Guid, Content>, E>;
 
     /// Applies the merged tree and stages items to upload. We keep this
     /// generic: on Desktop, we'll insert the merged tree into a temp
@@ -31,29 +31,28 @@ pub trait Store {
     /// this flow might be simpler.
     fn apply<D: Iterator<Item = Deletion>>(&mut self,
                                            merged_root: &MergedNode,
-                                           deletions: D) -> Result<()>;
-}
+                                           deletions: D) -> Result<(), E>;
 
-pub fn merge<S: Store>(store: &mut S, local_time_millis: i64,
-                       remote_time_millis: i64) -> Result<()> {
-    let local_tree = store.fetch_local_tree(local_time_millis)?;
-    let new_local_contents = store.fetch_new_local_contents()?;
+    fn merge(&mut self, local_time_millis: i64, remote_time_millis: i64) -> Result<(), E> {
+        let local_tree = self.fetch_local_tree(local_time_millis)?;
+        let new_local_contents = self.fetch_new_local_contents()?;
 
-    let remote_tree = store.fetch_remote_tree(remote_time_millis)?;
-    let new_remote_contents = store.fetch_new_remote_contents()?;
+        let remote_tree = self.fetch_remote_tree(remote_time_millis)?;
+        let new_remote_contents = self.fetch_new_remote_contents()?;
 
-    let mut merger = Merger::with_contents(&local_tree, &new_local_contents,
-                                           &remote_tree, &new_remote_contents);
-    let merged_root = merger.merge()?;
+        let mut merger = Merger::with_contents(&local_tree, &new_local_contents,
+                                               &remote_tree, &new_remote_contents);
+        let merged_root = merger.merge()?;
 
-    if !merger.subsumes(&local_tree) {
-        return Err(ErrorKind::UnmergedLocalItems.into());
-    }
-    if !merger.subsumes(&remote_tree) {
-        return Err(ErrorKind::UnmergedRemoteItems.into());
-    }
+        if !merger.subsumes(&local_tree) {
+            Err(E::from(ErrorKind::UnmergedLocalItems.into()))?;
+        }
+        if !merger.subsumes(&remote_tree) {
+            Err(E::from(ErrorKind::UnmergedRemoteItems.into()))?;
+        }
 
-    store.apply(&merged_root, merger.deletions())?;
+        self.apply(&merged_root, merger.deletions())?;
 
-    Ok(())
+        Ok(())
+    }
 }