Address soundness hole in cycle prevention

src/builder.rs

@@ -63,11 +63,11 @@ impl IsUnitType for () {}
 /// let b = dag.add_task(Multiplier::new(2));
 /// // b is TaskBuilder<_, Pending> until we call depends_on()
 ///
-/// let b = b.depends_on(&a);
+/// let b = b.depends_on(a);
 /// // Now b is a TaskHandle<i32>
 ///
 /// dag.run(|fut| tokio::spawn(fut).map(Result::unwrap)).await.unwrap();
-/// assert_eq!(dag.get(&b).unwrap(), 20);
+/// assert_eq!(dag.get(b).unwrap(), 20);
 /// # };
 /// ```
 pub struct TaskBuilder<'a, Tk: Task, Deps> {
@@ -118,14 +118,14 @@ impl<'a, Tk: Task, Deps> TaskBuilder<'a, Tk, Deps> {
     /// # async {
     /// let dag = DagRunner::new();
     ///
-    /// let x = dag.add_task(Value(2));
+    /// let x = dag.add_task(Value(2)).into();
     /// let y = dag.add_task(Value(3));
     ///
     /// // Single dependency
     /// let double = dag.add_task(Scale(2)).depends_on(&x);
     ///
     /// // Multiple dependencies: tuple form
-    /// let sum = dag.add_task(Add).depends_on((&x, &y));
+    /// let sum = dag.add_task(Add).depends_on((&x, y));
     ///
     /// dag.run(|fut| tokio::spawn(fut).map(Result::unwrap)).await.unwrap();
     /// # };
@@ -169,7 +169,3 @@ impl<'a, Tk: Task, Deps> TaskBuilder<'a, Tk, Deps> {
 
 #[cfg(test)]
 mod tests;
-
-// Note: We cannot safely implement AsRef<TaskHandle<Tk::Output>> for TaskBuilder<Tk, Deps>
-// because Handle is Copy and returning a reference to a temporary would be unsound.
-// Instead, users should use From/Into conversions or call .into() explicitly.

src/builder/tests.rs

@@ -76,7 +76,7 @@ fn test_task_builder_depends_on_returns_handle() {
     let dag = DagRunner::new();
 
     let source = dag.add_task(TestTask { value: 10 });
-    let dependent = dag.add_task(TestTaskWithInput).depends_on(&source);
+    let dependent = dag.add_task(TestTaskWithInput).depends_on(source);
 
     // depends_on should return a TaskHandle
     let _handle: TaskHandle<i32> = dependent;
@@ -87,12 +87,12 @@ fn test_task_builder_chain() {
     let dag = DagRunner::new();
 
     // Test chaining multiple tasks
-    let t1 = dag.add_task(TestTask { value: 1 });
-    let t2 = dag.add_task(TestTaskWithInput).depends_on(&t1);
+    let t1 = dag.add_task(TestTask { value: 1 }).into();
+    let t2 = dag.add_task(TestTaskWithInput).depends_on(t1);
     let t3 = dag.add_task(TestTaskWithInput).depends_on(t2);
 
     // All should return TaskHandles
-    let _: TaskHandle<i32> = t1.into();
+    let _: TaskHandle<i32> = t1;
     let _: TaskHandle<i32> = t2;
     let _: TaskHandle<i32> = t3;
 }
@@ -115,7 +115,7 @@ fn test_multiple_dependencies() {
     let b = dag.add_task(TestTask { value: 20 });
 
     // Test multiple dependencies
-    let sum = dag.add_task(AddTask).depends_on((&a, &b));
+    let sum = dag.add_task(AddTask).depends_on((a, b));
 
     // Should return a TaskHandle
     let _: TaskHandle<i32> = sum;

src/deps.rs

@@ -11,52 +11,60 @@ use crate::builder::TaskBuilder;
 use crate::task::Task;
 use crate::types::{NodeId, TaskHandle};
 
+trait IntoNodeRef {
+    type Output;
+    fn into_node_ref(self) -> NodeId;
+}
+
+impl<T> IntoNodeRef for &TaskHandle<T> {
+    type Output = T;
+    fn into_node_ref(self) -> NodeId {
+        self.id
+    }
+}
+
+impl<T> IntoNodeRef for TaskHandle<T> {
+    type Output = T;
+    fn into_node_ref(self) -> NodeId {
+        self.id
+    }
+}
+
+impl<'runner, Tk: Task, Deps> IntoNodeRef for TaskBuilder<'runner, Tk, Deps> {
+    type Output = Tk::Output;
+
+    fn into_node_ref(self) -> NodeId {
+        self.id
+    }
+}
+
 pub(crate) trait DepsTuple<Input> {
-    fn to_node_ids(&self) -> Vec<NodeId>;
+    fn to_node_ids(self) -> Vec<NodeId>;
 }
 
 // Implementation for unit (no dependencies)
 impl DepsTuple<()> for () {
-    fn to_node_ids(&self) -> Vec<NodeId> {
+    fn to_node_ids(self) -> Vec<NodeId> {
         Vec::new()
     }
 }
 
 // Implementation for single dependency (all forms)
-impl<T> DepsTuple<T> for &TaskHandle<T> {
-    fn to_node_ids(&self) -> Vec<NodeId> {
-        vec![self.id]
+impl<T> DepsTuple<T::Output> for T
+where
+    T: IntoNodeRef,
+{
+    fn to_node_ids(self) -> Vec<NodeId> {
+        vec![self.into_node_ref()]
     }
 }
 
-impl<T> DepsTuple<T> for TaskHandle<T> {
-    fn to_node_ids(&self) -> Vec<NodeId> {
-        vec![self.id]
-    }
-}
-
-impl<T> DepsTuple<T> for (&TaskHandle<T>,) {
-    fn to_node_ids(&self) -> Vec<NodeId> {
-        vec![self.0.id]
-    }
-}
-
-impl<T> DepsTuple<T> for (TaskHandle<T>,) {
-    fn to_node_ids(&self) -> Vec<NodeId> {
-        vec![self.0.id]
-    }
-}
-
-// Support for &TaskBuilder
-impl<'a, Tk: Task, Deps> DepsTuple<Tk::Output> for &TaskBuilder<'a, Tk, Deps> {
-    fn to_node_ids(&self) -> Vec<NodeId> {
-        vec![self.id]
-    }
-}
-
-impl<'a, Tk: Task, Deps> DepsTuple<Tk::Output> for (&TaskBuilder<'a, Tk, Deps>,) {
-    fn to_node_ids(&self) -> Vec<NodeId> {
-        vec![self.0.id]
+impl<T> DepsTuple<T::Output> for (T,)
+where
+    T: IntoNodeRef,
+{
+    fn to_node_ids(self) -> Vec<NodeId> {
+        vec![self.0.into_node_ref()]
     }
 }
 
@@ -65,10 +73,6 @@ impl<'a, Tk: Task, Deps> DepsTuple<Tk::Output> for (&TaskBuilder<'a, Tk, Deps>,)
 /// This macro exists because Rust lacks variadic generics - we need separate implementations
 /// for each tuple size.
 ///
-/// For each tuple size, generates two implementations:
-/// - `(&TaskHandle<A>, &TaskHandle<B>, ...)` → `DepsTuple<(A, B, ...)>`
-/// - `(&TaskBuilder<TkA, _>, &TaskBuilder<TkB, _>, ...)` → `DepsTuple<(TkA::Output, TkB::Output, ...)>`
-///
 /// This allows flexible dependency specification:
 /// ```ignore
 /// // Using handles
@@ -78,37 +82,27 @@ impl<'a, Tk: Task, Deps> DepsTuple<Tk::Output> for (&TaskBuilder<'a, Tk, Deps>,)
 /// task.depends_on((&dag.add_task(TaskA), &dag.add_task(TaskB)))
 /// ```
 macro_rules! impl_deps_tuple {
-    ($($T:ident : $Tk:ident : $D:ident),+) => {
-        // For &TaskHandle tuples
-        impl<$($T),+> DepsTuple<($($T,)+)> for ($(&TaskHandle<$T>,)+) {
+    ($($T:ident),+) => {
+        // For tuples of any combination of &TaskHandle, TaskHandle, and TaskBuilder
+        impl<$($T: IntoNodeRef),+> DepsTuple<($($T::Output,)+)> for ($($T,)+) {
             #[allow(non_snake_case)]
-            fn to_node_ids(&self) -> Vec<NodeId> {
+            fn to_node_ids(self) -> Vec<NodeId> {
                 let ($($T,)+) = self;
-                vec![$($T.id,)+]
-            }
-        }
-
-        // For &TaskBuilder tuples
-        impl<'a, $($Tk: Task, $D),+> DepsTuple<($($Tk::Output,)+)> for ($(&TaskBuilder<'a, $Tk, $D>,)+) {
-            #[allow(non_snake_case)]
-            fn to_node_ids(&self) -> Vec<NodeId> {
-                let ($($T,)+) = self;
-                vec![$($T.id,)+]
+                vec![$($T.into_node_ref(),)+]
             }
         }
     };
 }
 
 // Generate DepsTuple implementations for tuples of size 2-8.
-// Each invocation generates 2 implementations (TaskHandle refs and TaskBuilder refs).
 // Supporting up to 8 elements covers the vast majority of use cases.
-impl_deps_tuple!(A:TkA:DA, B:TkB:DB);
-impl_deps_tuple!(A:TkA:DA, B:TkB:DB, C:TkC:DC);
-impl_deps_tuple!(A:TkA:DA, B:TkB:DB, C:TkC:DC, D:TkD:DD);
-impl_deps_tuple!(A:TkA:DA, B:TkB:DB, C:TkC:DC, D:TkD:DD, E:TkE:DE);
-impl_deps_tuple!(A:TkA:DA, B:TkB:DB, C:TkC:DC, D:TkD:DD, E:TkE:DE, F:TkF:DF);
-impl_deps_tuple!(A:TkA:DA, B:TkB:DB, C:TkC:DC, D:TkD:DD, E:TkE:DE, F:TkF:DF, G:TkG:DG);
-impl_deps_tuple!(A:TkA:DA, B:TkB:DB, C:TkC:DC, D:TkD:DD, E:TkE:DE, F:TkF:DF, G:TkG:DG, H:TkH:DH);
+impl_deps_tuple!(T1, T2);
+impl_deps_tuple!(T1, T2, T3);
+impl_deps_tuple!(T1, T2, T3, T4);
+impl_deps_tuple!(T1, T2, T3, T4, T5);
+impl_deps_tuple!(T1, T2, T3, T4, T5, T6);
+impl_deps_tuple!(T1, T2, T3, T4, T5, T6, T7);
+impl_deps_tuple!(T1, T2, T3, T4, T5, T6, T7, T8);
 
 #[cfg(test)]
 mod tests;

src/deps/tests.rs

@@ -91,12 +91,13 @@ fn test_deps_tuple_task_builder_ref() {
     // Test lines 51-55 in deps.rs - &TaskBuilder
     let dag = DagRunner::new();
     let builder = dag.add_task(TestTask { value: 42 });
+    let builder_id = builder.id;
 
     // We need to test the DepsTuple implementation directly
     // The builder has an id field we can check
-    let node_ids = (&builder).to_node_ids();
+    let node_ids = builder.to_node_ids();
     assert_eq!(node_ids.len(), 1);
-    assert_eq!(node_ids[0], builder.id);
+    assert_eq!(node_ids[0], builder_id);
 }
 
 #[test]
@@ -104,10 +105,11 @@ fn test_deps_tuple_task_builder_tuple() {
     // Test lines 57-61 in deps.rs - (&TaskBuilder,)
     let dag = DagRunner::new();
     let builder = dag.add_task(TestTask { value: 100 });
+    let builder_id = builder.id;
 
-    let node_ids = (&builder,).to_node_ids();
+    let node_ids = (builder,).to_node_ids();
     assert_eq!(node_ids.len(), 1);
-    assert_eq!(node_ids[0], builder.id);
+    assert_eq!(node_ids[0], builder_id);
 }
 
 #[test]
@@ -145,21 +147,18 @@ fn test_deps_tuple_multiple_builders() {
     // Test macro-generated implementations for multiple builders
     let dag = DagRunner::new();
     let builder1 = dag.add_task(TestTask { value: 10 });
+    let builder1_id = builder1.id;
     let builder2 = dag.add_task(TestTask { value: 20 });
+    let builder2_id = builder2.id;
     let builder3 = dag.add_task(TestTask { value: 30 });
-
-    // Test 2-tuple of builders
-    let node_ids = (&builder1, &builder2).to_node_ids();
-    assert_eq!(node_ids.len(), 2);
-    assert_eq!(node_ids[0], builder1.id);
-    assert_eq!(node_ids[1], builder2.id);
+    let builder3_id = builder3.id;
 
     // Test 3-tuple of builders
-    let node_ids = (&builder1, &builder2, &builder3).to_node_ids();
+    let node_ids = (builder1, builder2, builder3).to_node_ids();
     assert_eq!(node_ids.len(), 3);
-    assert_eq!(node_ids[0], builder1.id);
-    assert_eq!(node_ids[1], builder2.id);
-    assert_eq!(node_ids[2], builder3.id);
+    assert_eq!(node_ids[0], builder1_id);
+    assert_eq!(node_ids[1], builder2_id);
+    assert_eq!(node_ids[2], builder3_id);
 }
 
 #[test]

src/lib.rs

@@ -23,7 +23,7 @@
 //! # Quick Start
 //!
 //! ```no_run
-//! use dagx::{task, DagRunner, Task};
+//! use dagx::{task, DagRunner, Task, TaskHandle};
 //! use futures::FutureExt; // for FutureExt::map
 //!
 //! // Source task with read-only state (tuple struct)
@@ -47,10 +47,10 @@
 //!
 //! // Add source tasks
 //! let x = dag.add_task(Value(2));
-//! let y = dag.add_task(Value(3));
+//! let y: TaskHandle<_> = dag.add_task(Value(3)).into(); // Allows y to be reused as a dependency
 //!
 //! // Add task with dependencies
-//! let sum = dag.add_task(Add).depends_on((&x, &y));
+//! let sum = dag.add_task(Add).depends_on((x, &y));
 //!
 //! // Execute and retrieve results
 //! dag.run(|fut| tokio::spawn(fut).map(Result::unwrap)).await.unwrap();
@@ -255,7 +255,7 @@
 //! # async {
 //! # let dag = DagRunner::new();
 //! # let source = dag.add_task(Value(42));
-//! let doubled = dag.add_task(Double).depends_on(&source);
+//! let doubled = dag.add_task(Double).depends_on(source);
 //! # };
 //! ```
 //!
@@ -279,7 +279,7 @@
 //! # let dag = DagRunner::new();
 //! # let x = dag.add_task(Value(2));
 //! # let y = dag.add_task(Value(3));
-//! let sum = dag.add_task(Add).depends_on((&x, &y));
+//! let sum = dag.add_task(Add).depends_on((x, y));
 //! # };
 //! ```
 //!
@@ -320,7 +320,7 @@
 //! # async {
 //! let dag = DagRunner::new();
 //!
-//! let base = dag.add_task(Value(10));
+//! let base = dag.add_task(Value(10)).into();
 //! let plus1 = dag.add_task(Add(1)).depends_on(&base);
 //! let times2 = dag.add_task(Scale(2)).depends_on(&base);
 //!
@@ -379,7 +379,7 @@
 //! let name = dag.add_task(Name("Alice".to_string()));
 //! let age = dag.add_task(Age(30));
 //! let active = dag.add_task(Active(true));
-//! let result = dag.add_task(FormatUser).depends_on((&name, &age, &active));
+//! let result = dag.add_task(FormatUser).depends_on((name, age, active));
 //!
 //! dag.run(|fut| tokio::spawn(fut).map(Result::unwrap)).await.unwrap();
 //!
@@ -424,12 +424,12 @@
 //!
 //! // Layer 1: Sources
 //! let x = dag.add_task(Value(2));
-//! let y = dag.add_task(Value(3));
+//! let y = dag.add_task(Value(3)).into();
 //! let z = dag.add_task(Value(5));
 //!
 //! // Layer 2: Intermediate computations
-//! let sum_xy = dag.add_task(Add).depends_on((&x, &y));  // 2 + 3 = 5
-//! let prod_yz = dag.add_task(Multiply).depends_on((&y, &z)); // 3 * 5 = 15
+//! let sum_xy = dag.add_task(Add).depends_on((x, &y));  // 2 + 3 = 5
+//! let prod_yz = dag.add_task(Multiply).depends_on((&y, z)); // 3 * 5 = 15
 //!
 //! // Layer 3: Final result
 //! let total = dag.add_task(Add).depends_on((&sum_xy, &prod_yz)); // 5 + 15 = 20
@@ -476,7 +476,7 @@
 //! let dag = DagRunner::new();
 //!
 //! let user = dag.add_task(CreateUser);
-//! let formatted = dag.add_task(FormatUser).depends_on(&user);
+//! let formatted = dag.add_task(FormatUser).depends_on(user);
 //!
 //! dag.run(|fut| tokio::spawn(fut).map(Result::unwrap)).await.unwrap();
 //!
@@ -656,7 +656,7 @@
 //!
 //! # async {
 //! let dag = DagRunner::new();
-//! let data = dag.add_task(FetchData);
+//! let data = dag.add_task(FetchData).into();
 //!
 //! // All three tasks get efficient Arc-wrapped sharing automatically
 //! let task1 = dag.add_task(ProcessData).depends_on(&data);

src/runner.rs

@@ -83,7 +83,7 @@ impl<'a> Drop for RunGuard<'a> {
 /// // Construct instances using ::new() pattern
 /// let x = dag.add_task(LoadValue::new(2));
 /// let y = dag.add_task(LoadValue::new(3));
-/// let sum = dag.add_task(Add).depends_on((&x, &y));
+/// let sum = dag.add_task(Add).depends_on((x, y));
 ///
 /// dag.run(|fut| tokio::spawn(fut).map(Result::unwrap)).await.unwrap();
 ///
@@ -190,10 +190,10 @@ impl DagRunner {
     /// let base = dag.add_task(LoadValue::new(10));
     ///
     /// // Construct task with offset of 1
-    /// let inc = dag.add_task(AddOffset::new(1)).depends_on(&base);
+    /// let inc = dag.add_task(AddOffset::new(1)).depends_on(base);
     ///
     /// dag.run(|fut| tokio::spawn(fut).map(Result::unwrap)).await.unwrap();
-    /// assert_eq!(dag.get(&inc).unwrap(), 11);
+    /// assert_eq!(dag.get(inc).unwrap(), 11);
     /// # };
     /// ```
     pub fn add_task<Tk>(&self, task: Tk) -> TaskBuilder<'_, Tk, Pending>
@@ -278,7 +278,7 @@ impl DagRunner {
     ///
     /// let a = dag.add_task(Value(1));
     /// let b = dag.add_task(Value(2));
-    /// let sum = dag.add_task(Add).depends_on((&a, &b));
+    /// let sum = dag.add_task(Add).depends_on((a, b));
     ///
     /// dag.run(|fut| tokio::spawn(fut).map(Result::unwrap)).await.unwrap(); // Executes all tasks
     /// # };

src/runner/tests.rs

@@ -190,16 +190,16 @@ async fn test_multiple_get_calls() {
     init_tracing();
     // Test that get() can be called multiple times for the same handle
     let dag = DagRunner::new();
-    let handle = dag.add_task(TestTask { value: 100 });
+    let handle: TaskHandle<_> = dag.add_task(TestTask { value: 100 }).into();
 
     dag.run(|fut| tokio::spawn(fut).map(Result::unwrap))
         .await
         .unwrap();
 
     // Multiple get calls should all return the same value
-    let result1 = dag.get(&handle).unwrap();
-    let result2 = dag.get(&handle).unwrap();
-    let result3 = dag.get(&handle).unwrap();
+    let result1 = dag.get(handle).unwrap();
+    let result2 = dag.get(handle).unwrap();
+    let result3 = dag.get(handle).unwrap();
 
     assert_eq!(result1, 100);
     assert_eq!(result2, 100);
@@ -264,11 +264,11 @@ async fn test_task_panic_in_multi_task_layer() {
     }
 
     let dag = DagRunner::new();
-    let source = dag.add_task(Source);
+    let source: TaskHandle<_> = dag.add_task(Source).into();
 
     // Create two tasks in the same layer - one panics, one doesn't
-    let _panic_task = dag.add_task(PanicTask).depends_on(&source);
-    let _good_task = dag.add_task(GoodTask).depends_on(&source);
+    let _panic_task = dag.add_task(PanicTask).depends_on(source);
+    let _good_task = dag.add_task(GoodTask).depends_on(source);
 
     // Run the DAG - the panic should be caught
     let result = dag.run(|fut| tokio::spawn(fut).map(Result::unwrap)).await;

src/types.rs

@@ -75,18 +75,6 @@ where
     }
 }
 
-impl<'a, Tk: Task, Deps> From<&TaskBuilder<'a, Tk, Deps>> for TaskHandle<Tk::Output>
-where
-    Tk::Input: IsUnitType,
-{
-    fn from(node: &TaskBuilder<'a, Tk, Deps>) -> Self {
-        TaskHandle {
-            id: node.id,
-            _phantom: std::marker::PhantomData,
-        }
-    }
-}
-
 // TaskHandle can be converted from &TaskHandle (for .get() calls)
 impl<T> From<&TaskHandle<T>> for TaskHandle<T> {
     fn from(handle: &TaskHandle<T>) -> Self {

src/types/tests.rs

@@ -147,25 +147,3 @@ fn test_task_handle_ref_from_conversion() {
     assert_eq!(converted.id.0, 123);
     assert_eq!(handle.id, converted.id);
 }
-
-#[test]
-fn test_task_builder_ref_from_conversion() {
-    use crate::runner::DagRunner;
-
-    // Test From<&TaskBuilder> for TaskHandle (line 77-87, specifically line 83)
-    struct SimpleTask2;
-    #[crate::task]
-    impl SimpleTask2 {
-        async fn run(&self) -> String {
-            "test".to_string()
-        }
-    }
-
-    let dag = DagRunner::new();
-    let builder = dag.add_task(SimpleTask2);
-    let builder_id = builder.id;
-
-    // This conversion uses the From<&TaskBuilder> implementation
-    let handle: TaskHandle<String> = (&builder).into();
-    assert_eq!(handle.id, builder_id);
-}