feat!: no recursion and Rc for causes (pubgrub-rs#184)

* inline find_shared_ids

* move clone for shorter code

* topological scan not recursion

* perf!: use rc not box

* cargo clippy

* Rc to Arc

Author: Eh2406

src/internal/core.rs

@@ -4,6 +4,7 @@
 //! to write a functional PubGrub algorithm.
 
 use std::error::Error;
+use std::sync::Arc;
 
 use crate::error::PubGrubError;
 use crate::internal::arena::Arena;
@@ -288,11 +289,6 @@ impl<P: Package, VS: VersionSet, Priority: Ord + Clone> State<P, VS, Priority> {
     // Error reporting #########################################################
 
     fn build_derivation_tree(&self, incompat: IncompId<P, VS>) -> DerivationTree<P, VS> {
-        let shared_ids = self.find_shared_ids(incompat);
-        Incompatibility::build_derivation_tree(incompat, &shared_ids, &self.incompatibility_store)
-    }
-
-    fn find_shared_ids(&self, incompat: IncompId<P, VS>) -> Set<IncompId<P, VS>> {
         let mut all_ids = Set::default();
         let mut shared_ids = Set::default();
         let mut stack = vec![incompat];
@@ -301,12 +297,28 @@ impl<P: Package, VS: VersionSet, Priority: Ord + Clone> State<P, VS, Priority> {
                 if all_ids.contains(&i) {
                     shared_ids.insert(i);
                 } else {
-                    all_ids.insert(i);
                     stack.push(id1);
                     stack.push(id2);
                 }
             }
+            all_ids.insert(i);
+        }
+        // To avoid recursion we need to generate trees in topological order.
+        // That is to say we need to ensure that the causes are processed before the incompatibility they effect.
+        // It happens to be that sorting by their ID maintains this property.
+        let mut sorted_ids = all_ids.into_iter().collect::<Vec<_>>();
+        sorted_ids.sort_unstable_by_key(|id| id.into_raw());
+        let mut precomputed = Map::default();
+        for id in sorted_ids {
+            let tree = Incompatibility::build_derivation_tree(
+                id,
+                &shared_ids,
+                &self.incompatibility_store,
+                &precomputed,
+            );
+            precomputed.insert(id, Arc::new(tree));
         }
-        shared_ids
+        // Now the user can refer to the entire tree from its root.
+        Arc::into_inner(precomputed.remove(&incompat).unwrap()).unwrap()
     }
 }

src/internal/incompatibility.rs

@@ -4,6 +4,7 @@
 //! that should never be satisfied all together.
 
 use std::fmt;
+use std::sync::Arc;
 
 use crate::internal::arena::{Arena, Id};
 use crate::internal::small_map::SmallMap;
@@ -12,7 +13,7 @@ use crate::report::{
     DefaultStringReportFormatter, DerivationTree, Derived, External, ReportFormatter,
 };
 use crate::term::{self, Term};
-use crate::type_aliases::Set;
+use crate::type_aliases::{Map, Set};
 use crate::version_set::VersionSet;
 
 /// An incompatibility is a set of terms for different packages
@@ -227,36 +228,36 @@ impl<P: Package, VS: VersionSet> Incompatibility<P, VS> {
         self_id: Id<Self>,
         shared_ids: &Set<Id<Self>>,
         store: &Arena<Self>,
+        precomputed: &Map<Id<Self>, Arc<DerivationTree<P, VS>>>,
     ) -> DerivationTree<P, VS> {
-        match &store[self_id].kind {
+        match store[self_id].kind.clone() {
             Kind::DerivedFrom(id1, id2) => {
-                let cause1 = Self::build_derivation_tree(*id1, shared_ids, store);
-                let cause2 = Self::build_derivation_tree(*id2, shared_ids, store);
                 let derived = Derived {
                     terms: store[self_id].package_terms.as_map(),
                     shared_id: shared_ids.get(&self_id).map(|id| id.into_raw()),
-                    cause1: Box::new(cause1),
-                    cause2: Box::new(cause2),
+                    cause1: precomputed
+                        .get(&id1)
+                        .expect("Non-topological calls building tree")
+                        .clone(),
+                    cause2: precomputed
+                        .get(&id2)
+                        .expect("Non-topological calls building tree")
+                        .clone(),
                 };
                 DerivationTree::Derived(derived)
             }
             Kind::NotRoot(package, version) => {
-                DerivationTree::External(External::NotRoot(package.clone(), version.clone()))
+                DerivationTree::External(External::NotRoot(package, version))
             }
             Kind::NoVersions(package, set) => {
-                DerivationTree::External(External::NoVersions(package.clone(), set.clone()))
+                DerivationTree::External(External::NoVersions(package, set))
             }
-            Kind::UnavailableDependencies(package, set) => DerivationTree::External(
-                External::UnavailableDependencies(package.clone(), set.clone()),
-            ),
-            Kind::FromDependencyOf(package, set, dep_package, dep_set) => {
-                DerivationTree::External(External::FromDependencyOf(
-                    package.clone(),
-                    set.clone(),
-                    dep_package.clone(),
-                    dep_set.clone(),
-                ))
+            Kind::UnavailableDependencies(package, set) => {
+                DerivationTree::External(External::UnavailableDependencies(package, set))
             }
+            Kind::FromDependencyOf(package, set, dep_package, dep_set) => DerivationTree::External(
+                External::FromDependencyOf(package, set, dep_package, dep_set),
+            ),
         }
     }
 }

src/lib.rs

@@ -220,7 +220,6 @@
 //! with a cache, you may want to know that some versions
 //! do not exist in your cache.
 
-#![allow(clippy::rc_buffer)]
 #![warn(missing_docs)]
 
 pub mod error;

src/report.rs

@@ -4,7 +4,8 @@
 //! dependency solving failed.
 
 use std::fmt;
-use std::ops::{Deref, DerefMut};
+use std::ops::Deref;
+use std::sync::Arc;
 
 use crate::package::Package;
 use crate::term::Term;
@@ -64,9 +65,9 @@ pub struct Derived<P: Package, VS: VersionSet> {
     /// and refer to the explanation for the other times.
     pub shared_id: Option<usize>,
     /// First cause.
-    pub cause1: Box<DerivationTree<P, VS>>,
+    pub cause1: Arc<DerivationTree<P, VS>>,
     /// Second cause.
-    pub cause2: Box<DerivationTree<P, VS>>,
+    pub cause2: Arc<DerivationTree<P, VS>>,
 }
 
 impl<P: Package, VS: VersionSet> DerivationTree<P, VS> {
@@ -82,7 +83,10 @@ impl<P: Package, VS: VersionSet> DerivationTree<P, VS> {
         match self {
             DerivationTree::External(_) => {}
             DerivationTree::Derived(derived) => {
-                match (derived.cause1.deref_mut(), derived.cause2.deref_mut()) {
+                match (
+                    Arc::make_mut(&mut derived.cause1),
+                    Arc::make_mut(&mut derived.cause2),
+                ) {
                     (DerivationTree::External(External::NoVersions(p, r)), ref mut cause2) => {
                         cause2.collapse_no_versions();
                         *self = cause2
@@ -98,8 +102,8 @@ impl<P: Package, VS: VersionSet> DerivationTree<P, VS> {
                             .unwrap_or_else(|| self.to_owned());
                     }
                     _ => {
-                        derived.cause1.collapse_no_versions();
-                        derived.cause2.collapse_no_versions();
+                        Arc::make_mut(&mut derived.cause1).collapse_no_versions();
+                        Arc::make_mut(&mut derived.cause2).collapse_no_versions();
                     }
                 }
             }