refactor: clean-up ownership

src/conflict.rs

@@ -13,10 +13,7 @@ use petgraph::{
 
 use crate::solver::variable_map::VariableOrigin;
 use crate::{
-    internal::{
-        arena::ArenaId,
-        id::{ClauseId, SolvableId, SolvableOrRootId, StringId, VersionSetId},
-    },
+    internal::id::{ClauseId, SolvableId, SolvableOrRootId, StringId, VersionSetId},
     runtime::AsyncRuntime,
     solver::{clause::Clause, Solver},
     DependencyProvider, Interner, Requirement,
@@ -48,6 +45,8 @@ impl Conflict {
         &self,
         solver: &Solver<D, RT>,
     ) -> ConflictGraph {
+        let state = &solver.state;
+
         let mut graph = DiGraph::<ConflictNode, ConflictEdge>::default();
         let mut nodes: HashMap<SolvableOrRootId, NodeIndex> = HashMap::default();
         let mut excluded_nodes: HashMap<StringId, NodeIndex> = HashMap::default();
@@ -56,14 +55,14 @@ impl Conflict {
         let unresolved_node = graph.add_node(ConflictNode::UnresolvedDependency);
         let mut last_node_by_name = HashMap::default();
 
-        for clause_id in &self.clauses {
-            let clause = &solver.clauses.kinds[clause_id.to_usize()];
+        for &clause_id in &self.clauses {
+            let clause = &state.clauses.kinds[clause_id];
             match clause {
                 Clause::InstallRoot => (),
                 Clause::Excluded(solvable, reason) => {
                     tracing::trace!("{solvable:?} is excluded");
                     let solvable = solvable
-                        .as_solvable(&solver.variable_map)
+                        .as_solvable(&state.variable_map)
                         .expect("only solvables can be excluded");
 
                     let package_node = Self::add_node(&mut graph, &mut nodes, solvable.into());
@@ -80,7 +79,7 @@ impl Conflict {
                 Clause::Learnt(..) => unreachable!(),
                 &Clause::Requires(package_id, version_set_id) => {
                     let solvable = package_id
-                        .as_solvable_or_root(&solver.variable_map)
+                        .as_solvable_or_root(&state.variable_map)
                         .expect("only solvables can be excluded");
                     let package_node = Self::add_node(&mut graph, &mut nodes, solvable);
 
@@ -112,10 +111,10 @@ impl Conflict {
                 }
                 &Clause::Lock(locked, forbidden) => {
                     let locked_solvable = locked
-                        .as_solvable(&solver.variable_map)
+                        .as_solvable(&state.variable_map)
                         .expect("only solvables can be excluded");
                     let forbidden_solvable = forbidden
-                        .as_solvable(&solver.variable_map)
+                        .as_solvable(&state.variable_map)
                         .expect("only solvables can be excluded");
                     let node2_id =
                         Self::add_node(&mut graph, &mut nodes, forbidden_solvable.into());
@@ -124,12 +123,12 @@ impl Conflict {
                 }
                 &Clause::ForbidMultipleInstances(instance1_id, instance2_id, _) => {
                     let solvable1 = instance1_id
-                        .as_solvable_or_root(&solver.variable_map)
+                        .as_solvable_or_root(&state.variable_map)
                         .expect("only solvables can be excluded");
                     let node1_id = Self::add_node(&mut graph, &mut nodes, solvable1);
 
                     let VariableOrigin::ForbidMultiple(name) =
-                        solver.variable_map.origin(instance2_id.variable())
+                        state.variable_map.origin(instance2_id.variable())
                     else {
                         unreachable!("expected only forbid variables")
                     };
@@ -145,10 +144,10 @@ impl Conflict {
                 }
                 &Clause::Constrains(package_id, dep_id, version_set_id) => {
                     let package_solvable = package_id
-                        .as_solvable_or_root(&solver.variable_map)
+                        .as_solvable_or_root(&state.variable_map)
                         .expect("only solvables can be excluded");
                     let dependency_solvable = dep_id
-                        .as_solvable_or_root(&solver.variable_map)
+                        .as_solvable_or_root(&state.variable_map)
                         .expect("only solvables can be excluded");
 
                     let package_node = Self::add_node(&mut graph, &mut nodes, package_solvable);
@@ -629,42 +628,6 @@ impl Indenter {
     }
 }
 
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn test_indenter_without_top_level_indent() {
-        let indenter = Indenter::new(false);
-
-        let indenter = indenter.push_level_with_order(ChildOrder::Last);
-        assert_eq!(indenter.get_indent(), "");
-
-        let indenter = indenter.push_level_with_order(ChildOrder::Last);
-        assert_eq!(indenter.get_indent(), "└─ ");
-    }
-
-    #[test]
-    fn test_indenter_with_multiple_siblings() {
-        let indenter = Indenter::new(true);
-
-        let indenter = indenter.push_level_with_order(ChildOrder::Last);
-        assert_eq!(indenter.get_indent(), "└─ ");
-
-        let indenter = indenter.push_level_with_order(ChildOrder::HasRemainingSiblings);
-        assert_eq!(indenter.get_indent(), "   ├─ ");
-
-        let indenter = indenter.push_level_with_order(ChildOrder::Last);
-        assert_eq!(indenter.get_indent(), "   │  └─ ");
-
-        let indenter = indenter.push_level_with_order(ChildOrder::Last);
-        assert_eq!(indenter.get_indent(), "   │     └─ ");
-
-        let indenter = indenter.push_level_with_order(ChildOrder::HasRemainingSiblings);
-        assert_eq!(indenter.get_indent(), "   │        ├─ ");
-    }
-}
-
 /// A struct implementing [`fmt::Display`] that generates a user-friendly
 /// representation of a conflict graph
 pub struct DisplayUnsat<'i, I: Interner> {
@@ -1052,3 +1015,39 @@ impl<'i, I: Interner> fmt::Display for DisplayUnsat<'i, I> {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_indenter_without_top_level_indent() {
+        let indenter = Indenter::new(false);
+
+        let indenter = indenter.push_level_with_order(ChildOrder::Last);
+        assert_eq!(indenter.get_indent(), "");
+
+        let indenter = indenter.push_level_with_order(ChildOrder::Last);
+        assert_eq!(indenter.get_indent(), "└─ ");
+    }
+
+    #[test]
+    fn test_indenter_with_multiple_siblings() {
+        let indenter = Indenter::new(true);
+
+        let indenter = indenter.push_level_with_order(ChildOrder::Last);
+        assert_eq!(indenter.get_indent(), "└─ ");
+
+        let indenter = indenter.push_level_with_order(ChildOrder::HasRemainingSiblings);
+        assert_eq!(indenter.get_indent(), "   ├─ ");
+
+        let indenter = indenter.push_level_with_order(ChildOrder::Last);
+        assert_eq!(indenter.get_indent(), "   │  └─ ");
+
+        let indenter = indenter.push_level_with_order(ChildOrder::Last);
+        assert_eq!(indenter.get_indent(), "   │     └─ ");
+
+        let indenter = indenter.push_level_with_order(ChildOrder::HasRemainingSiblings);
+        assert_eq!(indenter.get_indent(), "   │        ├─ ");
+    }
+}

src/internal/mapping.rs

@@ -15,7 +15,7 @@ pub struct Mapping<TId, TValue> {
     _phantom: PhantomData<TId>,
 }
 
-impl<TId: ArenaId, TValue: Clone> Default for Mapping<TId, TValue> {
+impl<TId: ArenaId, TValue> Default for Mapping<TId, TValue> {
     fn default() -> Self {
         Self::new()
     }
@@ -124,7 +124,7 @@ impl<TId: ArenaId, TValue> Mapping<TId, TValue> {
             .get_unchecked(chunk)
             .get_unchecked(offset)
             .as_ref()
-            .unwrap()
+            .unwrap_unchecked()
     }
 
     /// Get a specific value in the mapping without bound checks
@@ -139,7 +139,7 @@ impl<TId: ArenaId, TValue> Mapping<TId, TValue> {
             .get_unchecked_mut(chunk)
             .get_unchecked_mut(offset)
             .as_mut()
-            .unwrap()
+            .unwrap_unchecked()
     }
 
     /// Returns the number of mapped items

src/internal/mod.rs

@@ -3,6 +3,3 @@ pub mod frozen_copy_map;
 pub mod id;
 pub mod mapping;
 pub mod small_vec;
-mod unwrap_unchecked;
-
-pub use unwrap_unchecked::debug_expect_unchecked;

src/solver/clause.rs

@@ -632,7 +632,7 @@ mod test {
 
     #[test]
     fn test_literal_eval() {
-        let mut decision_map = DecisionMap::new();
+        let mut decision_map = DecisionMap::default();
 
         let literal = VariableId::root().positive();
         let negated_literal = VariableId::root().negative();
@@ -653,7 +653,7 @@ mod test {
 
     #[test]
     fn test_requires_with_and_without_conflict() {
-        let mut decisions = DecisionTracker::new();
+        let mut decisions = DecisionTracker::default();
 
         let parent = VariableId::from_usize(1);
         let candidate1 = VariableId::from_usize(2);
@@ -671,17 +671,11 @@ mod test {
             clause.as_ref().unwrap().watched_literals[0].variable(),
             parent
         );
-        assert_eq!(
-            clause.unwrap().watched_literals[1].variable(),
-            candidate1.into()
-        );
+        assert_eq!(clause.unwrap().watched_literals[1].variable(), candidate1);
 
         // No conflict, still one candidate available
         decisions
-            .try_add_decision(
-                Decision::new(candidate1.into(), false, ClauseId::from_usize(0)),
-                1,
-            )
+            .try_add_decision(Decision::new(candidate1, false, ClauseId::from_usize(0)), 1)
             .unwrap();
         let (clause, conflict, _kind) = WatchedLiterals::requires(
             parent,
@@ -696,13 +690,13 @@ mod test {
         );
         assert_eq!(
             clause.as_ref().unwrap().watched_literals[1].variable(),
-            candidate2.into()
+            candidate2
         );
 
         // Conflict, no candidates available
         decisions
             .try_add_decision(
-                Decision::new(candidate2.into(), false, ClauseId::install_root()),
+                Decision::new(candidate2, false, ClauseId::install_root()),
                 1,
             )
             .unwrap();
@@ -719,7 +713,7 @@ mod test {
         );
         assert_eq!(
             clause.as_ref().unwrap().watched_literals[1].variable(),
-            candidate1.into()
+            candidate1
         );
 
         // Panic
@@ -740,7 +734,7 @@ mod test {
 
     #[test]
     fn test_constrains_with_and_without_conflict() {
-        let mut decisions = DecisionTracker::new();
+        let mut decisions = DecisionTracker::default();
 
         let parent = VariableId::from_usize(1);
         let forbidden = VariableId::from_usize(2);

src/solver/decision_map.rs

@@ -37,17 +37,12 @@ impl DecisionAndLevel {
 }
 
 /// A map of the assignments to solvables.
+#[derive(Default)]
 pub(crate) struct DecisionMap {
     map: Vec<DecisionAndLevel>,
 }
 
 impl DecisionMap {
-    pub fn new() -> Self {
-        Self {
-            map: Default::default(),
-        }
-    }
-
     pub fn reset(&mut self, variable_id: VariableId) {
         let variable_id = variable_id.to_usize();
         if variable_id < self.map.len() {

src/solver/decision_tracker.rs

@@ -3,25 +3,16 @@ use crate::solver::{decision::Decision, decision_map::DecisionMap};
 
 /// Tracks the assignments to solvables, keeping a log that can be used to backtrack, and a map that
 /// can be used to query the current value assigned
+#[derive(Default)]
 pub(crate) struct DecisionTracker {
     map: DecisionMap,
     stack: Vec<Decision>,
     propagate_index: usize,
 }
 
 impl DecisionTracker {
-    pub(crate) fn new() -> Self {
-        Self {
-            map: DecisionMap::new(),
-            stack: Vec::new(),
-            propagate_index: 0,
-        }
-    }
-
     pub(crate) fn clear(&mut self) {
-        self.map = DecisionMap::new();
-        self.stack = Vec::new();
-        self.propagate_index = 0;
+        *self = Default::default();
     }
 
     #[inline(always)]