Improve Errors (#152)

* Added checks for child_index out of bounds

* slightly better descriptions

* updated RELEASES.md

* Changed TaffyError back to Error

* split error type into two

* fixed error in git merge

* updated RELEASES.md file

* cargo fmt

* moved errors into error module, and gave them better names

* Split InvalidChild::InvalidNode into two

* updated RELEASES.md

* cargo fmt

* more changes to RELEASES.md

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

RELEASES.md

@@ -4,14 +4,16 @@
 
 ### 0.2.0 Added
 
-- Nothing yet
+- Added `taffy::error::InvalidChild` Error type
 
 ### 0.2.0 Changed
 
 - `Sprawl::remove` now returns a `Result<usize, Error>`, to indicate if the operation was sucessful, and if it was, which ID was invalidated.
 - renamed `taffy::forest::Forest.new-node(..)` `taffy::forest::Forest.new_with_children(..)`
 - renamed `taffy::node::Taffy.new-node(..)` -> `taffy::node::Taffy.new_with_children(..)`
 - renamed `taffy::style::Style` -> `taffy::style::FlexboxLayout` to more precicely indicate its purpose
+- renamed `taffy::Error` -> `taffy::error::InvalidNode`
+- `taffy::Taffy::remove_child_at_index`, `taffy::Taffy::replace_child_at_index`, and `taffy::Taffy::child_at_index` now return `taffy::InvalidChild::ChildIndexOutOfBounds` instead of panicing
 
 ### 0.2.0 Fixed
 

examples/basic.rs

@@ -1,6 +1,6 @@
 use taffy::prelude::*;
 
-fn main() -> Result<(), Error> {
+fn main() -> Result<(), taffy::error::InvalidNode> {
     let mut taffy = Taffy::new();
 
     let child = taffy.new_with_children(

examples/nested.rs

@@ -1,6 +1,6 @@
 use taffy::prelude::*;
 
-fn main() -> Result<(), Error> {
+fn main() -> Result<(), taffy::error::InvalidNode> {
     let mut taffy = Taffy::new();
 
     // left

src/error.rs

@@ -0,0 +1,57 @@
+//! The Error types produced by Taffy.
+#[cfg(feature = "std")]
+use core::fmt::{Display, Formatter, Result};
+
+use crate::node::Node;
+
+/// The [`Node`] was not found in the [`Taffy`](crate::Taffy) instance
+#[derive(Debug)]
+pub struct InvalidNode(pub Node);
+
+#[cfg(feature = "std")]
+impl Display for InvalidNode {
+    fn fmt(&self, f: &mut Formatter) -> Result {
+        write!(f, "Node {:?} is not in the Taffy instance", self.0)
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for InvalidNode {}
+
+/// An error that occurs while trying to access or modify a [`Node`]'s children by index.
+#[derive(Debug)]
+pub enum InvalidChild {
+    /// The parent [`Node`] does not have a child at `child_index`. It only has `child_count` children
+    ChildIndexOutOfBounds {
+        /// The parent node whose child was being looked up
+        parent: Node,
+        /// The index that was looked up
+        child_index: usize,
+        /// The total number of children the parent has
+        child_count: usize,
+    },
+    /// The parent [`Node`] was not found in the [`Taffy`](crate::Taffy) instance.
+    InvalidParentNode(Node),
+    /// The child [`Node`] was not found in the [`Taffy`](crate::Taffy) instance.
+    InvalidChildNode(Node),
+}
+
+#[cfg(feature = "std")]
+impl Display for InvalidChild {
+    fn fmt(&self, f: &mut Formatter) -> Result {
+        match self {
+            InvalidChild::ChildIndexOutOfBounds { parent, child_index, child_count } => write!(
+                f,
+                "Index (is {}) should be < child_count ({}) for parent node {:?}",
+                child_index, child_count, parent
+            ),
+            InvalidChild::InvalidParentNode(parent) => {
+                write!(f, "Parent Node {:?} is not in the Taffy instance", parent)
+            }
+            InvalidChild::InvalidChildNode(child) => write!(f, "Child Node {:?} is not in the Taffy instance", child),
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for InvalidChild {}

src/lib.rs

@@ -11,6 +11,7 @@ extern crate alloc;
 #[cfg(feature = "serde")]
 extern crate serde;
 
+pub mod error;
 pub mod geometry;
 pub mod layout;
 pub mod node;
@@ -25,31 +26,3 @@ mod indexmap;
 mod sys;
 
 pub use crate::node::Taffy;
-
-#[cfg(feature = "std")]
-use core::fmt::{Display, Formatter, Result};
-
-/// An error that can occur when performing layout
-#[derive(Debug)]
-pub enum Error {
-    /// The [`Node`](node::Node) was invalid
-    InvalidNode(node::Node),
-}
-
-#[cfg(feature = "std")]
-impl Display for Error {
-    fn fmt(&self, f: &mut Formatter) -> Result {
-        match *self {
-            Error::InvalidNode(ref node) => write!(f, "Invalid node {:?}", node),
-        }
-    }
-}
-
-#[cfg(feature = "std")]
-impl std::error::Error for Error {
-    fn description(&self) -> &str {
-        match *self {
-            Error::InvalidNode(_) => "The node is not part of the Taffy instance",
-        }
-    }
-}

src/node.rs

@@ -1,6 +1,7 @@
 //! UI [`Node`] types and related data structures.
 //!
 //! Layouts are composed of multiple nodes, which live in a forest-like data structure.
+use crate::error;
 use crate::forest::Forest;
 use crate::geometry::Size;
 use crate::layout::Layout;
@@ -9,7 +10,6 @@ use crate::style::FlexboxLayout;
 #[cfg(any(feature = "std", feature = "alloc"))]
 use crate::sys::Box;
 use crate::sys::{new_map_with_capacity, ChildrenVec, Map, Vec};
-use crate::Error;
 use core::sync::atomic::{AtomicUsize, Ordering};
 
 /// A function that can be applied to a `Size<Number>` to obtain a `Size<f32>`
@@ -87,26 +87,28 @@ impl Taffy {
     }
 
     /// Returns the `NodeId` of the provided node within the forest
-    fn find_node(&self, node: Node) -> Result<NodeId, Error> {
+    fn find_node(&self, node: Node) -> Result<NodeId, error::InvalidNode> {
         match self.nodes_to_ids.get(&node) {
             Some(id) => Ok(*id),
-            None => Err(Error::InvalidNode(node)),
+            None => Err(error::InvalidNode(node)),
         }
     }
 
     /// Adds a new leaf node, which does not have any children
-    pub fn new_leaf(&mut self, style: FlexboxLayout, measure: MeasureFunc) -> Result<Node, Error> {
+    pub fn new_leaf(&mut self, style: FlexboxLayout, measure: MeasureFunc) -> Result<Node, error::InvalidNode> {
         let node = self.allocate_node();
         let id = self.forest.new_leaf(style, measure);
         self.add_node(node, id);
         Ok(node)
     }
 
     /// Adds a new node, which may have any number of `children`
-    pub fn new_with_children(&mut self, style: FlexboxLayout, children: &[Node]) -> Result<Node, Error> {
+    pub fn new_with_children(&mut self, style: FlexboxLayout, children: &[Node]) -> Result<Node, error::InvalidNode> {
         let node = self.allocate_node();
-        let children =
-            children.iter().map(|child| self.find_node(*child)).collect::<Result<ChildrenVec<_>, Error>>()?;
+        let children = children
+            .iter()
+            .map(|child| self.find_node(*child))
+            .collect::<Result<ChildrenVec<_>, error::InvalidNode>>()?;
         let id = self.forest.new_with_children(style, children);
         self.add_node(node, id);
         Ok(node)
@@ -124,7 +126,7 @@ impl Taffy {
     /// Remove a specific [`Node`] from the tree
     ///
     /// Its [`Id`] is marked as invalid. Returns the id of the node removed.
-    pub fn remove(&mut self, node: Node) -> Result<usize, Error> {
+    pub fn remove(&mut self, node: Node) -> Result<usize, error::InvalidNode> {
         let id = self.find_node(node)?;
 
         self.nodes_to_ids.remove(&node);
@@ -140,15 +142,15 @@ impl Taffy {
     }
 
     /// Sets the [`MeasureFunc`] of the associated node
-    pub fn set_measure(&mut self, node: Node, measure: Option<MeasureFunc>) -> Result<(), Error> {
+    pub fn set_measure(&mut self, node: Node, measure: Option<MeasureFunc>) -> Result<(), error::InvalidNode> {
         let id = self.find_node(node)?;
         self.forest.nodes[id].measure = measure;
         self.forest.mark_dirty(id);
         Ok(())
     }
 
     /// Adds a `child` [`Node`] under the supplied `parent`
-    pub fn add_child(&mut self, parent: Node, child: Node) -> Result<(), Error> {
+    pub fn add_child(&mut self, parent: Node, child: Node) -> Result<(), error::InvalidNode> {
         let node_id = self.find_node(parent)?;
         let child_id = self.find_node(child)?;
 
@@ -157,7 +159,7 @@ impl Taffy {
     }
 
     /// Directly sets the `children` of the supplied `parent`
-    pub fn set_children(&mut self, parent: Node, children: &[Node]) -> Result<(), Error> {
+    pub fn set_children(&mut self, parent: Node, children: &[Node]) -> Result<(), error::InvalidNode> {
         let node_id = self.find_node(parent)?;
         let children_id = children.iter().map(|child| self.find_node(*child)).collect::<Result<ChildrenVec<_>, _>>()?;
 
@@ -179,7 +181,7 @@ impl Taffy {
     /// Removes the `child` of the parent `node`
     ///
     /// The child is not removed from the forest entirely, it is simply no longer attached to its previous parent.
-    pub fn remove_child(&mut self, parent: Node, child: Node) -> Result<Node, Error> {
+    pub fn remove_child(&mut self, parent: Node, child: Node) -> Result<Node, error::InvalidNode> {
         let node_id = self.find_node(parent)?;
         let child_id = self.find_node(child)?;
 
@@ -190,9 +192,13 @@ impl Taffy {
     /// Removes the child at the given `index` from the `parent`
     ///
     /// The child is not removed from the forest entirely, it is simply no longer attached to its previous parent.
-    pub fn remove_child_at_index(&mut self, parent: Node, child_index: usize) -> Result<Node, Error> {
-        let node_id = self.find_node(parent)?;
-        // TODO: index check
+    pub fn remove_child_at_index(&mut self, parent: Node, child_index: usize) -> Result<Node, error::InvalidChild> {
+        let node_id = self.find_node(parent).map_err(|e| error::InvalidChild::InvalidParentNode(e.0))?;
+
+        let child_count = self.forest.children[node_id].len();
+        if child_index >= child_count {
+            return Err(error::InvalidChild::ChildIndexOutOfBounds { parent, child_index, child_count });
+        }
 
         let prev_id = self.forest.remove_child_at_index(node_id, child_index);
         Ok(self.ids_to_nodes[&prev_id])
@@ -201,10 +207,19 @@ impl Taffy {
     /// Replaces the child at the given `child_index` from the `parent` node with the new `child` node
     ///
     /// The child is not removed from the forest entirely, it is simply no longer attached to its previous parent.
-    pub fn replace_child_at_index(&mut self, parent: Node, child_index: usize, new_child: Node) -> Result<Node, Error> {
-        let node_id = self.find_node(parent)?;
-        let child_id = self.find_node(new_child)?;
+    pub fn replace_child_at_index(
+        &mut self,
+        parent: Node,
+        child_index: usize,
+        new_child: Node,
+    ) -> Result<Node, error::InvalidChild> {
+        let node_id = self.find_node(parent).map_err(|e| error::InvalidChild::InvalidParentNode(e.0))?;
+        let child_id = self.find_node(new_child).map_err(|e| error::InvalidChild::InvalidChildNode(e.0))?;
         // TODO: index check
+        let child_count = self.forest.children[node_id].len();
+        if child_index >= child_count {
+            return Err(error::InvalidChild::ChildIndexOutOfBounds { parent, child_index, child_count });
+        }
 
         self.forest.parents[child_id].push(node_id);
         let old_child = core::mem::replace(&mut self.forest.children[node_id][child_index], child_id);
@@ -216,58 +231,64 @@ impl Taffy {
     }
 
     /// Returns the child [`Node`] of the parent `node` at the provided `child_index`
-    pub fn child_at_index(&self, parent: Node, child_index: usize) -> Result<Node, Error> {
-        let id = self.find_node(parent)?;
+    pub fn child_at_index(&self, parent: Node, child_index: usize) -> Result<Node, error::InvalidChild> {
+        let id = self.find_node(parent).map_err(|e| error::InvalidChild::InvalidParentNode(e.0))?;
+
+        let child_count = self.forest.children[id].len();
+        if child_index >= child_count {
+            return Err(error::InvalidChild::ChildIndexOutOfBounds { parent, child_index, child_count });
+        }
+
         Ok(self.ids_to_nodes[&self.forest.children[id][child_index]])
     }
 
     /// Returns the number of children of the `parent` [`Node`]
-    pub fn child_count(&self, parent: Node) -> Result<usize, Error> {
+    pub fn child_count(&self, parent: Node) -> Result<usize, error::InvalidNode> {
         let id = self.find_node(parent)?;
         Ok(self.forest.children[id].len())
     }
 
     /// Returns a list of children that belong to the [`Parent`]
-    pub fn children(&self, parent: Node) -> Result<Vec<Node>, Error> {
+    pub fn children(&self, parent: Node) -> Result<Vec<Node>, error::InvalidNode> {
         let id = self.find_node(parent)?;
         Ok(self.forest.children[id].iter().map(|child| self.ids_to_nodes[child]).collect())
     }
 
     /// Sets the [`Style`] of the provided `node`
-    pub fn set_style(&mut self, node: Node, style: FlexboxLayout) -> Result<(), Error> {
+    pub fn set_style(&mut self, node: Node, style: FlexboxLayout) -> Result<(), error::InvalidNode> {
         let id = self.find_node(node)?;
         self.forest.nodes[id].style = style;
         self.forest.mark_dirty(id);
         Ok(())
     }
 
     /// Gets the [`Style`] of the provided `node`
-    pub fn style(&self, node: Node) -> Result<&FlexboxLayout, Error> {
+    pub fn style(&self, node: Node) -> Result<&FlexboxLayout, error::InvalidNode> {
         let id = self.find_node(node)?;
         Ok(&self.forest.nodes[id].style)
     }
 
     /// Return this node layout relative to its parent
-    pub fn layout(&self, node: Node) -> Result<&Layout, Error> {
+    pub fn layout(&self, node: Node) -> Result<&Layout, error::InvalidNode> {
         let id = self.find_node(node)?;
         Ok(&self.forest.nodes[id].layout)
     }
 
     /// Marks the layout computation of this node and its children as outdated
-    pub fn mark_dirty(&mut self, node: Node) -> Result<(), Error> {
+    pub fn mark_dirty(&mut self, node: Node) -> Result<(), error::InvalidNode> {
         let id = self.find_node(node)?;
         self.forest.mark_dirty(id);
         Ok(())
     }
 
     /// Indicates whether the layout of this node (and its children) need to be recomputed
-    pub fn dirty(&self, node: Node) -> Result<bool, Error> {
+    pub fn dirty(&self, node: Node) -> Result<bool, error::InvalidNode> {
         let id = self.find_node(node)?;
         Ok(self.forest.nodes[id].is_dirty)
     }
 
     /// Updates the stored layout of the provided `node` and its children
-    pub fn compute_layout(&mut self, node: Node, size: Size<Number>) -> Result<(), Error> {
+    pub fn compute_layout(&mut self, node: Node, size: Size<Number>) -> Result<(), error::InvalidNode> {
         let id = self.find_node(node)?;
         self.forest.compute_layout(id, size);
         Ok(())

src/prelude.rs

@@ -9,5 +9,4 @@ pub use crate::{
         AlignContent, AlignItems, AlignSelf, Dimension, Display, FlexDirection, FlexWrap, FlexboxLayout,
         JustifyContent, PositionType,
     },
-    Error,
 };