From 4e479e20620c2165b5e97cf419051d082afd2bde Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 29 Jun 2026 02:07:29 +0000 Subject: [PATCH 1/4] Avoid cloning dependency version sets --- src/internal/core.rs | 2 +- src/internal/incompatibility.rs | 361 ++++++++++++++++++++++++++++++-- 2 files changed, 349 insertions(+), 14 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index d4b9e197..720d6840 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -400,7 +400,7 @@ impl State { if let Some((p1, p2, dependency_range)) = self.incompatibility_store[id].as_dependency() { // Self-dependencies cannot be merged. if p1 != p2 { - let deps_lookup = self.merged_dependencies.bucket(p1, p2, dependency_range); + let deps_lookup = self.merged_dependencies.bucket(p1, p2, &dependency_range); if let Some((past, merged)) = deps_lookup.as_mut_slice().iter_mut().find_map(|past| { self.incompatibility_store[id] diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index a8723de5..9fc097c4 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -87,11 +87,11 @@ pub enum Kind { /// Incompatibility coming from the dependencies of a given package. /// /// If a@1 depends on b>=1,<2, we create an incompatibility with terms `{a 1, b <1,>=2}` with - /// kind `FromDependencyOf(a, 1, b, >=1,<2)`. + /// kind `FromDependencyOf(a, b)`. The version sets are stored in the incompatibility terms. /// /// We can merge multiple dependents with the same version. For example, if a@1 depends on b and /// a@2 depends on b, we can say instead a@1||2 depends on b. - FromDependencyOf(Id

, VS, Id

, VS), + FromDependencyOf(Id

, Id

), /// Derived from two causes. Stores cause ids. /// /// For example, if a -> b and b -> c, we can derive a -> c. @@ -177,21 +177,39 @@ impl Incompatibilit let (p2, set2) = dep; Self { package_terms: if set2 == VS::empty() { - SmallMap::One([(package, Term::Positive(versions.clone()))]) + SmallMap::One([(package, Term::Positive(versions))]) } else { SmallMap::Two([ - (package, Term::Positive(versions.clone())), - (p2, Term::Negative(set2.clone())), + (package, Term::Positive(versions)), + (p2, Term::Negative(set2)), ]) }, - kind: Kind::FromDependencyOf(package, versions, p2, set2), + kind: Kind::FromDependencyOf(package, p2), contradiction_cache: ContradictionCache::not_contradicted(), } } - pub(crate) fn as_dependency(&self) -> Option<(Id

, Id

, &VS)> { + pub(crate) fn as_dependency(&self) -> Option<(Id

, Id

, Option<&VS>)> { match &self.kind { - Kind::FromDependencyOf(p1, _, p2, range) => Some((*p1, *p2, range)), + Kind::FromDependencyOf(p1, p2) => { + let mut terms = self.package_terms.iter(); + match terms.next() { + Some((term_package, Term::Positive(_))) if term_package == p1 => {} + _ => panic!("dependency incompatibility must start with its positive term"), + } + let dependency_range = match terms.next() { + None => None, + Some((term_package, Term::Negative(range))) if term_package == p2 => { + Some(range) + } + _ => panic!("dependency incompatibility must end with its negative term"), + }; + assert!( + terms.next().is_none(), + "dependency incompatibility must contain at most two terms" + ); + Some((*p1, *p2, dependency_range)) + } _ => None, } } @@ -361,12 +379,32 @@ impl Incompatibilit package_store[package].clone(), set.clone(), )), - Kind::FromDependencyOf(package, set, dep_package, dep_set) => { + Kind::FromDependencyOf(package, dep_package) => { + let mut terms = store[self_id].package_terms.iter(); + let package_versions = match terms.next() { + Some((&term_package, Term::Positive(versions))) if term_package == package => { + versions + } + _ => panic!("dependency incompatibility must start with its positive term"), + }; + let dependency_versions = match terms.next() { + None => VS::empty(), + Some((&term_package, Term::Negative(versions))) + if term_package == dep_package => + { + versions.clone() + } + _ => panic!("dependency incompatibility must end with its negative term"), + }; + assert!( + terms.next().is_none(), + "dependency incompatibility must contain at most two terms" + ); DerivationTree::External(External::FromDependencyOf( package_store[package].clone(), - set.clone(), + package_versions.clone(), package_store[dep_package].clone(), - dep_set.clone(), + dependency_versions, )) } Kind::Custom(package, set, metadata) => DerivationTree::External(External::Custom( @@ -452,12 +490,56 @@ pub(crate) mod tests { use proptest::prelude::*; use std::cmp::Reverse; use std::collections::BTreeMap; + use std::fmt::{self, Formatter}; + use std::sync::atomic::{AtomicUsize, Ordering}; use super::*; use crate::internal::State; use crate::term::tests::strategy as term_strat; use crate::{OfflineDependencyProvider, Ranges}; + #[derive(Debug, Eq, Hash, PartialEq)] + struct CloneCountingRanges(Ranges); + + static RANGE_CLONES: AtomicUsize = AtomicUsize::new(0); + + impl Clone for CloneCountingRanges { + fn clone(&self) -> Self { + RANGE_CLONES.fetch_add(1, Ordering::Relaxed); + Self(self.0.clone()) + } + } + + impl Display for CloneCountingRanges { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + Display::fmt(&self.0, f) + } + } + + impl VersionSet for CloneCountingRanges { + type V = usize; + + fn empty() -> Self { + Self(Ranges::empty()) + } + + fn singleton(v: Self::V) -> Self { + Self(Ranges::singleton(v)) + } + + fn complement(&self) -> Self { + Self(self.0.complement()) + } + + fn intersection(&self, other: &Self) -> Self { + Self(self.0.intersection(&other.0)) + } + + fn contains(&self, v: &Self::V) -> bool { + self.0.contains(v) + } + } + #[test] fn contradiction_cache_tracks_backtrack_generations() { let current_generation = ContradictionCache { @@ -502,13 +584,13 @@ pub(crate) mod tests { let p3 = package_store.alloc("p3"); let i1 = store.alloc(Incompatibility { package_terms: SmallMap::Two([(p1, t1.clone()), (p2, t2.negate())]), - kind: Kind::<_, _, String>::FromDependencyOf(p1, Ranges::full(), p2, Ranges::full()), + kind: Kind::<_, _, String>::FromDependencyOf(p1, p2), contradiction_cache: ContradictionCache::not_contradicted(), }); let i2 = store.alloc(Incompatibility { package_terms: SmallMap::Two([(p2, t2), (p3, t3.clone())]), - kind: Kind::<_, _, String>::FromDependencyOf(p2, Ranges::full(), p3, Ranges::full()), + kind: Kind::<_, _, String>::FromDependencyOf(p2, p3), contradiction_cache: ContradictionCache::not_contradicted(), }); @@ -522,6 +604,259 @@ pub(crate) mod tests { } + #[test] + fn from_dependency_does_not_clone_version_sets() { + let mut package_store = HashArena::new(); + let package = package_store.alloc("package".to_string()); + let dependency = package_store.alloc("dependency".to_string()); + + let versions = CloneCountingRanges(Ranges::singleton(1usize)); + let dependency_versions = CloneCountingRanges(Ranges::singleton(2usize)); + RANGE_CLONES.store(0, Ordering::Relaxed); + let nonempty: Incompatibility = + Incompatibility::from_dependency(package, versions, (dependency, dependency_versions)); + assert_eq!(RANGE_CLONES.load(Ordering::Relaxed), 0); + assert!(matches!( + nonempty.kind, + Kind::FromDependencyOf(actual_package, actual_dependency) + if actual_package == package && actual_dependency == dependency + )); + + let versions = CloneCountingRanges(Ranges::singleton(1usize)); + let dependency_versions = CloneCountingRanges(Ranges::empty()); + RANGE_CLONES.store(0, Ordering::Relaxed); + let empty: Incompatibility = + Incompatibility::from_dependency(package, versions, (dependency, dependency_versions)); + assert_eq!(RANGE_CLONES.load(Ordering::Relaxed), 0); + assert!(matches!( + empty.kind, + Kind::FromDependencyOf(actual_package, actual_dependency) + if actual_package == package && actual_dependency == dependency + )); + } + + #[test] + fn dependency_terms_preserve_order_duplicates_and_self_dependencies() { + let mut package_store = HashArena::new(); + let package = package_store.alloc("package".to_string()); + let dependency = package_store.alloc("dependency".to_string()); + let versions = Ranges::between(1usize, 4usize); + let dependency_versions = Ranges::between(7usize, 10usize); + + let ordinary: Incompatibility, String> = + Incompatibility::from_dependency( + package, + versions.clone(), + (dependency, dependency_versions.clone()), + ); + let entries: Vec<_> = ordinary.iter().collect(); + assert_eq!(entries.len(), 2); + assert_eq!(entries[0].0, package); + assert_eq!(entries[0].1, &Term::Positive(versions.clone())); + assert_eq!(entries[1].0, dependency); + assert_eq!(entries[1].1, &Term::Negative(dependency_versions.clone())); + + let empty: Incompatibility, String> = + Incompatibility::from_dependency( + package, + versions.clone(), + (dependency, Ranges::empty()), + ); + let entries: Vec<_> = empty.iter().collect(); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].0, package); + assert_eq!(entries[0].1, &Term::Positive(versions.clone())); + + let duplicate1: Incompatibility, String> = + Incompatibility::from_dependency( + package, + versions.clone(), + (dependency, dependency_versions.clone()), + ); + let duplicate2: Incompatibility, String> = + Incompatibility::from_dependency( + package, + versions.clone(), + (dependency, dependency_versions.clone()), + ); + for duplicate in [&duplicate1, &duplicate2] { + let entries: Vec<_> = duplicate.iter().collect(); + assert_eq!(entries.len(), 2); + assert_eq!(entries[0].0, package); + assert_eq!(entries[0].1, &Term::Positive(versions.clone())); + assert_eq!(entries[1].0, dependency); + assert_eq!(entries[1].1, &Term::Negative(dependency_versions.clone())); + } + + let self_dependency: Incompatibility, String> = + Incompatibility::from_dependency( + package, + versions.clone(), + (package, dependency_versions.clone()), + ); + let entries: Vec<_> = self_dependency.iter().collect(); + assert_eq!(entries.len(), 2); + assert_eq!(entries[0].0, package); + assert_eq!(entries[0].1, &Term::Positive(versions)); + assert_eq!(entries[1].0, package); + assert_eq!(entries[1].1, &Term::Negative(dependency_versions)); + } + + #[test] + fn dependency_derivation_trees_reconstruct_ranges_positionally() { + let mut package_store = HashArena::new(); + let package = package_store.alloc("package".to_string()); + let dependency = package_store.alloc("dependency".to_string()); + let versions: Ranges = Ranges::between(1usize, 4usize); + let dependency_versions = Ranges::between(7usize, 10usize); + + let mut store = Arena::new(); + let ordinary = store.alloc(Incompatibility::<_, _, String>::from_dependency( + package, + versions.clone(), + (dependency, dependency_versions.clone()), + )); + let empty = store.alloc(Incompatibility::<_, _, String>::from_dependency( + package, + versions.clone(), + (dependency, Ranges::empty()), + )); + let duplicate1 = store.alloc(Incompatibility::<_, _, String>::from_dependency( + package, + versions.clone(), + (dependency, dependency_versions.clone()), + )); + let duplicate2 = store.alloc(Incompatibility::<_, _, String>::from_dependency( + package, + versions.clone(), + (dependency, dependency_versions.clone()), + )); + let self_dependency = store.alloc(Incompatibility::<_, _, String>::from_dependency( + package, + versions.clone(), + (package, dependency_versions.clone()), + )); + + let shared_ids = Set::default(); + let precomputed = Map::default(); + let cases = [ + ( + "ordinary", + ordinary, + "package", + versions.clone(), + "dependency", + dependency_versions.clone(), + ), + ( + "empty", + empty, + "package", + versions.clone(), + "dependency", + Ranges::empty(), + ), + ( + "duplicate 1", + duplicate1, + "package", + versions.clone(), + "dependency", + dependency_versions.clone(), + ), + ( + "duplicate 2", + duplicate2, + "package", + versions.clone(), + "dependency", + dependency_versions.clone(), + ), + ( + "self dependency", + self_dependency, + "package", + versions, + "package", + dependency_versions, + ), + ]; + + for (name, id, expected_package, expected_versions, expected_dependency, expected_set) in + cases + { + let tree = Incompatibility::build_derivation_tree( + id, + &shared_ids, + &store, + &package_store, + &precomputed, + ); + let DerivationTree::External(External::FromDependencyOf( + actual_package, + actual_versions, + actual_dependency, + actual_set, + )) = tree + else { + panic!("{name}: expected a dependency external") + }; + assert_eq!(actual_package, expected_package, "{name}"); + assert_eq!(actual_versions, expected_versions, "{name}"); + assert_eq!(actual_dependency, expected_dependency, "{name}"); + assert_eq!(actual_set, expected_set, "{name}"); + } + } + + #[test] + fn merged_dependency_derivation_tree_preserves_ranges() { + let mut package_store = HashArena::new(); + let package = package_store.alloc("package".to_string()); + let dependency = package_store.alloc("dependency".to_string()); + let versions1 = Ranges::singleton(1usize); + let versions2 = Ranges::singleton(2usize); + let dependency_versions = Ranges::between(7usize, 10usize); + + let incompatibility1: Incompatibility, String> = + Incompatibility::from_dependency( + package, + versions1.clone(), + (dependency, dependency_versions.clone()), + ); + let incompatibility2 = Incompatibility::from_dependency( + package, + versions2.clone(), + (dependency, dependency_versions.clone()), + ); + let merged = incompatibility1 + .merge_dependents(&incompatibility2) + .unwrap(); + let expected_versions = versions1.union(&versions2); + + let mut store = Arena::new(); + let merged = store.alloc(merged); + let tree = Incompatibility::build_derivation_tree( + merged, + &Set::default(), + &store, + &package_store, + &Map::default(), + ); + let DerivationTree::External(External::FromDependencyOf( + actual_package, + actual_versions, + actual_dependency, + actual_dependency_versions, + )) = tree + else { + panic!("expected a dependency external") + }; + assert_eq!(actual_package, "package"); + assert_eq!(actual_versions, expected_versions); + assert_eq!(actual_dependency, "dependency"); + assert_eq!(actual_dependency_versions, dependency_versions); + } + /// Check that multiple self-dependencies are supported. /// /// The current public API deduplicates dependencies through a map, so we test them here From 8f31b20e3def5dbd1f68a6911f19190c87adf53d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 28 Jun 2026 22:32:53 -0400 Subject: [PATCH 2/4] Simplify dependency version set clone removal --- src/internal/incompatibility.rs | 366 +++++++++----------------------- 1 file changed, 106 insertions(+), 260 deletions(-) diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 9fc097c4..69cdec08 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -189,25 +189,28 @@ impl Incompatibilit } } + fn dependency_terms(&self, p1: Id

, p2: Id

) -> (&VS, Option<&VS>) { + let mut terms = self.package_terms.iter(); + let versions = match terms.next() { + Some((term_package, Term::Positive(versions))) if *term_package == p1 => versions, + _ => panic!("dependency incompatibility must start with its positive term"), + }; + let dependency_versions = match terms.next() { + None => None, + Some((term_package, Term::Negative(versions))) if *term_package == p2 => Some(versions), + _ => panic!("dependency incompatibility must end with its negative term"), + }; + assert!( + terms.next().is_none(), + "dependency incompatibility must contain at most two terms" + ); + (versions, dependency_versions) + } + pub(crate) fn as_dependency(&self) -> Option<(Id

, Id

, Option<&VS>)> { match &self.kind { Kind::FromDependencyOf(p1, p2) => { - let mut terms = self.package_terms.iter(); - match terms.next() { - Some((term_package, Term::Positive(_))) if term_package == p1 => {} - _ => panic!("dependency incompatibility must start with its positive term"), - } - let dependency_range = match terms.next() { - None => None, - Some((term_package, Term::Negative(range))) if term_package == p2 => { - Some(range) - } - _ => panic!("dependency incompatibility must end with its negative term"), - }; - assert!( - terms.next().is_none(), - "dependency incompatibility must contain at most two terms" - ); + let (_, dependency_range) = self.dependency_terms(*p1, *p2); Some((*p1, *p2, dependency_range)) } _ => None, @@ -225,10 +228,11 @@ impl Incompatibilit /// is the common dependant in the two incompatibilities expressing dependencies. pub(crate) fn merge_dependents(&self, other: &Self) -> Option { // It is almost certainly a bug to call this method without checking that self is a dependency - debug_assert!(self.as_dependency().is_some()); + let dependency = self.as_dependency(); + debug_assert!(dependency.is_some()); // Check that both incompatibilities are of the shape p1 depends on p2, // with the same p1 and p2. - let (p1, p2, _) = self.as_dependency()?; + let (p1, p2, _) = dependency?; let (other_p1, other_p2, _) = other.as_dependency()?; if (p1, p2) != (other_p1, other_p2) { return None; @@ -380,31 +384,13 @@ impl Incompatibilit set.clone(), )), Kind::FromDependencyOf(package, dep_package) => { - let mut terms = store[self_id].package_terms.iter(); - let package_versions = match terms.next() { - Some((&term_package, Term::Positive(versions))) if term_package == package => { - versions - } - _ => panic!("dependency incompatibility must start with its positive term"), - }; - let dependency_versions = match terms.next() { - None => VS::empty(), - Some((&term_package, Term::Negative(versions))) - if term_package == dep_package => - { - versions.clone() - } - _ => panic!("dependency incompatibility must end with its negative term"), - }; - assert!( - terms.next().is_none(), - "dependency incompatibility must contain at most two terms" - ); + let (package_versions, dependency_versions) = + store[self_id].dependency_terms(package, dep_package); DerivationTree::External(External::FromDependencyOf( package_store[package].clone(), package_versions.clone(), package_store[dep_package].clone(), - dependency_versions, + dependency_versions.cloned().unwrap_or_else(VS::empty), )) } Kind::Custom(package, set, metadata) => DerivationTree::External(External::Custom( @@ -491,7 +477,6 @@ pub(crate) mod tests { use std::cmp::Reverse; use std::collections::BTreeMap; use std::fmt::{self, Formatter}; - use std::sync::atomic::{AtomicUsize, Ordering}; use super::*; use crate::internal::State; @@ -499,24 +484,21 @@ pub(crate) mod tests { use crate::{OfflineDependencyProvider, Ranges}; #[derive(Debug, Eq, Hash, PartialEq)] - struct CloneCountingRanges(Ranges); - - static RANGE_CLONES: AtomicUsize = AtomicUsize::new(0); + struct PanicOnCloneRanges(Ranges); - impl Clone for CloneCountingRanges { + impl Clone for PanicOnCloneRanges { fn clone(&self) -> Self { - RANGE_CLONES.fetch_add(1, Ordering::Relaxed); - Self(self.0.clone()) + panic!("version set was cloned") } } - impl Display for CloneCountingRanges { + impl Display for PanicOnCloneRanges { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { Display::fmt(&self.0, f) } } - impl VersionSet for CloneCountingRanges { + impl VersionSet for PanicOnCloneRanges { type V = usize; fn empty() -> Self { @@ -610,251 +592,115 @@ pub(crate) mod tests { let package = package_store.alloc("package".to_string()); let dependency = package_store.alloc("dependency".to_string()); - let versions = CloneCountingRanges(Ranges::singleton(1usize)); - let dependency_versions = CloneCountingRanges(Ranges::singleton(2usize)); - RANGE_CLONES.store(0, Ordering::Relaxed); - let nonempty: Incompatibility = - Incompatibility::from_dependency(package, versions, (dependency, dependency_versions)); - assert_eq!(RANGE_CLONES.load(Ordering::Relaxed), 0); - assert!(matches!( - nonempty.kind, - Kind::FromDependencyOf(actual_package, actual_dependency) - if actual_package == package && actual_dependency == dependency - )); - - let versions = CloneCountingRanges(Ranges::singleton(1usize)); - let dependency_versions = CloneCountingRanges(Ranges::empty()); - RANGE_CLONES.store(0, Ordering::Relaxed); - let empty: Incompatibility = - Incompatibility::from_dependency(package, versions, (dependency, dependency_versions)); - assert_eq!(RANGE_CLONES.load(Ordering::Relaxed), 0); - assert!(matches!( - empty.kind, - Kind::FromDependencyOf(actual_package, actual_dependency) - if actual_package == package && actual_dependency == dependency - )); + for dependency_versions in [ + PanicOnCloneRanges(Ranges::singleton(2usize)), + PanicOnCloneRanges(Ranges::empty()), + ] { + let _: Incompatibility = + Incompatibility::from_dependency( + package, + PanicOnCloneRanges(Ranges::singleton(1usize)), + (dependency, dependency_versions), + ); + } + } + + fn assert_dependency_tree( + package_store: &HashArena, + incompatibility: Incompatibility, String>, + expected_versions: &Ranges, + expected_dependency: &str, + expected_dependency_versions: &Ranges, + ) { + let mut store = Arena::new(); + let id = store.alloc(incompatibility); + let tree = Incompatibility::build_derivation_tree( + id, + &Set::default(), + &store, + package_store, + &Map::default(), + ); + let DerivationTree::External(External::FromDependencyOf( + actual_package, + actual_versions, + actual_dependency, + actual_dependency_versions, + )) = tree + else { + panic!("expected a dependency external") + }; + assert_eq!(actual_package, "package"); + assert_eq!(&actual_versions, expected_versions); + assert_eq!(actual_dependency, expected_dependency); + assert_eq!(&actual_dependency_versions, expected_dependency_versions); } #[test] - fn dependency_terms_preserve_order_duplicates_and_self_dependencies() { + fn dependency_derivation_trees_reconstruct_ranges() { let mut package_store = HashArena::new(); let package = package_store.alloc("package".to_string()); let dependency = package_store.alloc("dependency".to_string()); let versions = Ranges::between(1usize, 4usize); + let other_versions = Ranges::singleton(4usize); let dependency_versions = Ranges::between(7usize, 10usize); - let ordinary: Incompatibility, String> = + assert_dependency_tree( + &package_store, Incompatibility::from_dependency( package, versions.clone(), (dependency, dependency_versions.clone()), - ); - let entries: Vec<_> = ordinary.iter().collect(); - assert_eq!(entries.len(), 2); - assert_eq!(entries[0].0, package); - assert_eq!(entries[0].1, &Term::Positive(versions.clone())); - assert_eq!(entries[1].0, dependency); - assert_eq!(entries[1].1, &Term::Negative(dependency_versions.clone())); - - let empty: Incompatibility, String> = - Incompatibility::from_dependency( - package, - versions.clone(), - (dependency, Ranges::empty()), - ); - let entries: Vec<_> = empty.iter().collect(); - assert_eq!(entries.len(), 1); - assert_eq!(entries[0].0, package); - assert_eq!(entries[0].1, &Term::Positive(versions.clone())); + ), + &versions, + "dependency", + &dependency_versions, + ); - let duplicate1: Incompatibility, String> = - Incompatibility::from_dependency( - package, - versions.clone(), - (dependency, dependency_versions.clone()), - ); - let duplicate2: Incompatibility, String> = + let empty = Ranges::empty(); + assert_dependency_tree( + &package_store, Incompatibility::from_dependency( package, versions.clone(), - (dependency, dependency_versions.clone()), - ); - for duplicate in [&duplicate1, &duplicate2] { - let entries: Vec<_> = duplicate.iter().collect(); - assert_eq!(entries.len(), 2); - assert_eq!(entries[0].0, package); - assert_eq!(entries[0].1, &Term::Positive(versions.clone())); - assert_eq!(entries[1].0, dependency); - assert_eq!(entries[1].1, &Term::Negative(dependency_versions.clone())); - } + (dependency, empty.clone()), + ), + &versions, + "dependency", + &empty, + ); - let self_dependency: Incompatibility, String> = + assert_dependency_tree( + &package_store, Incompatibility::from_dependency( package, versions.clone(), (package, dependency_versions.clone()), - ); - let entries: Vec<_> = self_dependency.iter().collect(); - assert_eq!(entries.len(), 2); - assert_eq!(entries[0].0, package); - assert_eq!(entries[0].1, &Term::Positive(versions)); - assert_eq!(entries[1].0, package); - assert_eq!(entries[1].1, &Term::Negative(dependency_versions)); - } - - #[test] - fn dependency_derivation_trees_reconstruct_ranges_positionally() { - let mut package_store = HashArena::new(); - let package = package_store.alloc("package".to_string()); - let dependency = package_store.alloc("dependency".to_string()); - let versions: Ranges = Ranges::between(1usize, 4usize); - let dependency_versions = Ranges::between(7usize, 10usize); - - let mut store = Arena::new(); - let ordinary = store.alloc(Incompatibility::<_, _, String>::from_dependency( - package, - versions.clone(), - (dependency, dependency_versions.clone()), - )); - let empty = store.alloc(Incompatibility::<_, _, String>::from_dependency( - package, - versions.clone(), - (dependency, Ranges::empty()), - )); - let duplicate1 = store.alloc(Incompatibility::<_, _, String>::from_dependency( - package, - versions.clone(), - (dependency, dependency_versions.clone()), - )); - let duplicate2 = store.alloc(Incompatibility::<_, _, String>::from_dependency( - package, - versions.clone(), - (dependency, dependency_versions.clone()), - )); - let self_dependency = store.alloc(Incompatibility::<_, _, String>::from_dependency( - package, - versions.clone(), - (package, dependency_versions.clone()), - )); - - let shared_ids = Set::default(); - let precomputed = Map::default(); - let cases = [ - ( - "ordinary", - ordinary, - "package", - versions.clone(), - "dependency", - dependency_versions.clone(), - ), - ( - "empty", - empty, - "package", - versions.clone(), - "dependency", - Ranges::empty(), - ), - ( - "duplicate 1", - duplicate1, - "package", - versions.clone(), - "dependency", - dependency_versions.clone(), ), - ( - "duplicate 2", - duplicate2, - "package", - versions.clone(), - "dependency", - dependency_versions.clone(), - ), - ( - "self dependency", - self_dependency, - "package", - versions, - "package", - dependency_versions, - ), - ]; - - for (name, id, expected_package, expected_versions, expected_dependency, expected_set) in - cases - { - let tree = Incompatibility::build_derivation_tree( - id, - &shared_ids, - &store, - &package_store, - &precomputed, - ); - let DerivationTree::External(External::FromDependencyOf( - actual_package, - actual_versions, - actual_dependency, - actual_set, - )) = tree - else { - panic!("{name}: expected a dependency external") - }; - assert_eq!(actual_package, expected_package, "{name}"); - assert_eq!(actual_versions, expected_versions, "{name}"); - assert_eq!(actual_dependency, expected_dependency, "{name}"); - assert_eq!(actual_set, expected_set, "{name}"); - } - } - - #[test] - fn merged_dependency_derivation_tree_preserves_ranges() { - let mut package_store = HashArena::new(); - let package = package_store.alloc("package".to_string()); - let dependency = package_store.alloc("dependency".to_string()); - let versions1 = Ranges::singleton(1usize); - let versions2 = Ranges::singleton(2usize); - let dependency_versions = Ranges::between(7usize, 10usize); + &versions, + "package", + &dependency_versions, + ); - let incompatibility1: Incompatibility, String> = + let first: Incompatibility, String> = Incompatibility::from_dependency( package, - versions1.clone(), + versions.clone(), (dependency, dependency_versions.clone()), ); - let incompatibility2 = Incompatibility::from_dependency( + let second = Incompatibility::from_dependency( package, - versions2.clone(), + other_versions.clone(), (dependency, dependency_versions.clone()), ); - let merged = incompatibility1 - .merge_dependents(&incompatibility2) - .unwrap(); - let expected_versions = versions1.union(&versions2); - - let mut store = Arena::new(); - let merged = store.alloc(merged); - let tree = Incompatibility::build_derivation_tree( - merged, - &Set::default(), - &store, + let merged = first.merge_dependents(&second).unwrap(); + assert_dependency_tree( &package_store, - &Map::default(), + merged, + &versions.union(&other_versions), + "dependency", + &dependency_versions, ); - let DerivationTree::External(External::FromDependencyOf( - actual_package, - actual_versions, - actual_dependency, - actual_dependency_versions, - )) = tree - else { - panic!("expected a dependency external") - }; - assert_eq!(actual_package, "package"); - assert_eq!(actual_versions, expected_versions); - assert_eq!(actual_dependency, "dependency"); - assert_eq!(actual_dependency_versions, dependency_versions); } /// Check that multiple self-dependencies are supported. From b8dd822dee1f94499787d2245dfe51e963bb411f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 29 Jun 2026 09:18:08 -0400 Subject: [PATCH 3/4] Expose dependency version sets --- src/internal/incompatibility.rs | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 69cdec08..56be737d 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -207,6 +207,18 @@ impl Incompatibilit (versions, dependency_versions) } + /// Returns the version sets for a dependency incompatibility. + /// + /// Returns `None` if this is not a dependency incompatibility. The dependency version set in + /// the returned pair is `None` when it is empty because empty dependencies are stored without a + /// negative term. + pub fn dependency_version_sets(&self) -> Option<(&VS, Option<&VS>)> { + match &self.kind { + Kind::FromDependencyOf(p1, p2) => Some(self.dependency_terms(*p1, *p2)), + _ => None, + } + } + pub(crate) fn as_dependency(&self) -> Option<(Id

, Id

, Option<&VS>)> { match &self.kind { Kind::FromDependencyOf(p1, p2) => { @@ -384,8 +396,9 @@ impl Incompatibilit set.clone(), )), Kind::FromDependencyOf(package, dep_package) => { - let (package_versions, dependency_versions) = - store[self_id].dependency_terms(package, dep_package); + let (package_versions, dependency_versions) = store[self_id] + .dependency_version_sets() + .expect("matched dependency incompatibility"); DerivationTree::External(External::FromDependencyOf( package_store[package].clone(), package_versions.clone(), @@ -612,6 +625,17 @@ pub(crate) mod tests { expected_dependency: &str, expected_dependency_versions: &Ranges, ) { + let (versions, dependency_versions) = incompatibility + .dependency_version_sets() + .expect("expected a dependency incompatibility"); + assert_eq!(versions, expected_versions); + match dependency_versions { + Some(dependency_versions) => { + assert_eq!(dependency_versions, expected_dependency_versions); + } + None => assert_eq!(expected_dependency_versions, &Ranges::empty()), + } + let mut store = Arena::new(); let id = store.alloc(incompatibility); let tree = Incompatibility::build_derivation_tree( From 8fd9709ea671b1b3ead53e6ec847da5fcad7c03d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 29 Jun 2026 09:26:07 -0400 Subject: [PATCH 4/4] Use the private dependency term helper internally --- src/internal/incompatibility.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 56be737d..0552a72c 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -396,9 +396,8 @@ impl Incompatibilit set.clone(), )), Kind::FromDependencyOf(package, dep_package) => { - let (package_versions, dependency_versions) = store[self_id] - .dependency_version_sets() - .expect("matched dependency incompatibility"); + let (package_versions, dependency_versions) = + store[self_id].dependency_terms(package, dep_package); DerivationTree::External(External::FromDependencyOf( package_store[package].clone(), package_versions.clone(),