Merge pull request #2228 from hannobraun/set

Fix panic in `ObjectSet` due to equal but not identical objects

crates/fj-core/src/objects/mod.rs

@@ -33,6 +33,6 @@ pub use self::{
         surface::Surface,
         vertex::Vertex,
     },
-    object_set::ObjectSet,
+    object_set::{ObjectSet, ObjectSetIntoIter, ObjectSetIter},
     stores::{Objects, Surfaces},
 };

crates/fj-core/src/objects/object_set.rs

@@ -1,8 +1,8 @@
-use std::{collections::BTreeSet, fmt::Debug, slice, vec};
+use std::{collections::BTreeSet, fmt::Debug, iter, slice, vec};
 
 use itertools::Itertools;
 
-use crate::storage::Handle;
+use crate::storage::{Handle, HandleWrapper};
 
 /// An ordered set of objects
 ///
@@ -19,7 +19,7 @@ pub struct ObjectSet<T> {
     // structure (since it is used in objects, and objects themselves are
     // immutable). We need to make sure there are no duplicates when this is
     // constructed (see the constructor below), but after that, we're fine.
-    inner: Vec<Handle<T>>,
+    inner: Vec<HandleWrapper<T>>,
 }
 
 impl<T> ObjectSet<T> {
@@ -36,6 +36,8 @@ impl<T> ObjectSet<T> {
         let mut inner = Vec::new();
 
         for handle in handles {
+            let handle = HandleWrapper::from(handle);
+
             if added.contains(&handle) {
                 panic!(
                     "Constructing `ObjectSet` with duplicate handle: {:?}",
@@ -97,7 +99,7 @@ impl<T> ObjectSet<T> {
 
     /// Return the n-th item
     pub fn nth(&self, index: usize) -> Option<&Handle<T>> {
-        self.inner.get(index)
+        self.inner.get(index).map(|wrapper| &wrapper.0)
     }
 
     /// Return the n-th item, treating the index space as circular
@@ -130,8 +132,8 @@ impl<T> ObjectSet<T> {
     }
 
     /// Access an iterator over the objects
-    pub fn iter(&self) -> slice::Iter<Handle<T>> {
-        self.inner.iter()
+    pub fn iter(&self) -> ObjectSetIter<T> {
+        self.inner.iter().map(HandleWrapper::as_handle)
     }
 
     /// Access an iterator over the neighboring pairs of all contained objects
@@ -199,28 +201,63 @@ where
     }
 }
 
+impl<'r, T> IntoIterator for &'r ObjectSet<T> {
+    type Item = &'r Handle<T>;
+    type IntoIter = ObjectSetIter<'r, T>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.iter()
+    }
+}
+
 impl<T> IntoIterator for ObjectSet<T> {
     type Item = Handle<T>;
-    type IntoIter = vec::IntoIter<Handle<T>>;
+    type IntoIter = ObjectSetIntoIter<T>;
 
     fn into_iter(self) -> Self::IntoIter {
-        self.inner.into_iter()
+        self.inner.into_iter().map(HandleWrapper::into_handle)
     }
 }
 
-impl<'r, T> IntoIterator for &'r ObjectSet<T> {
-    // You might wonder why we're returning references to `Handle`s here, when
-    // `Handle` already is a kind of reference, and easily cloned.
-    //
-    // Most of the time that doesn't make a difference, but there are use cases
-    // where dealing with owned `Handle`s is inconvenient, for example when
-    // using iterator adapters. You can't return a reference to the argument of
-    // an adapter's closure, if you own that argument. You can, if you just
-    // reference the argument.
-    type Item = &'r Handle<T>;
-    type IntoIter = slice::Iter<'r, Handle<T>>;
+/// An borrowed iterator over an [`ObjectSet`]
+///
+/// See [`ObjectSet::iter`].
+pub type ObjectSetIter<'r, T> = iter::Map<
+    slice::Iter<'r, HandleWrapper<T>>,
+    fn(&HandleWrapper<T>) -> &Handle<T>,
+>;
 
-    fn into_iter(self) -> Self::IntoIter {
-        self.iter()
+/// An owned iterator over an [`ObjectSet`]
+///
+/// You might wonder why we're returning references to `Handle`s here, when
+/// `Handle` already is a kind of reference, and easily cloned.
+///
+/// Most of the time that doesn't make a difference, but there are use cases
+/// where dealing with owned `Handle`s is inconvenient, for example when using
+/// iterator adapters. You can't return a reference to the argument of an
+/// adapter's closure, if you own that argument. You can, if you just reference
+/// the argument.
+pub type ObjectSetIntoIter<T> = iter::Map<
+    vec::IntoIter<HandleWrapper<T>>,
+    fn(HandleWrapper<T>) -> Handle<T>,
+>;
+
+#[cfg(test)]
+mod tests {
+    use crate::{objects::Cycle, operations::insert::Insert, Core};
+
+    use super::ObjectSet;
+
+    #[test]
+    fn equal_but_not_identical_objects() {
+        let mut core = Core::new();
+
+        let bare_cycle = Cycle::new([]);
+        let cycle_a = bare_cycle.clone().insert(&mut core);
+        let cycle_b = bare_cycle.insert(&mut core);
+
+        let _object_set = ObjectSet::new([cycle_a, cycle_b]);
+        // Nothing more to test. `ObjectSet` panics on duplicate objects, and it
+        // shouldn't do that in this case.
     }
 }

crates/fj-core/src/storage/handle.rs

@@ -235,6 +235,18 @@ impl fmt::Debug for ObjectId {
 /// is the purpose of `HandleWrapper`.
 pub struct HandleWrapper<T>(pub Handle<T>);
 
+impl<T> HandleWrapper<T> {
+    /// Convert `&self` into a `&Handle`
+    pub fn as_handle(&self) -> &Handle<T> {
+        &self.0
+    }
+
+    /// Convert `self` into a `Handle`
+    pub fn into_handle(self) -> Handle<T> {
+        self.0
+    }
+}
+
 impl<T> Deref for HandleWrapper<T> {
     type Target = Handle<T>;