From 5eae31f08b3fb7e702847eea02d8116c394a959b Mon Sep 17 00:00:00 2001 From: konstin Date: Tue, 5 Nov 2024 08:53:12 +0100 Subject: [PATCH] Use `Ranges::from_iter` over `Ranges::iter_mut` Use astral-sh/pubgrub#34 (pubgrub-rs/pubgrub#273) for safer ranges modification that can't break pubgrub invariants. In the process, I removed the `&mut` references in favor of a direct immutable-to-immutable mapping. --- Cargo.lock | 4 +- Cargo.toml | 4 +- crates/uv-resolver/src/error.rs | 185 ++++++++++++++++++-------------- 3 files changed, 106 insertions(+), 87 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 81269bb53ffb5..19ad251d0cc49 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2520,7 +2520,7 @@ dependencies = [ [[package]] name = "pubgrub" version = "0.2.1" -source = "git+https://github.com/astral-sh/pubgrub?branch=charlie/mut#5aadec2663c058cac0ffdacc73d58b01ebdee5a9" +source = "git+https://github.com/astral-sh/pubgrub?rev=30502ceb17c033be408d9766a32ebc211518033f#30502ceb17c033be408d9766a32ebc211518033f" dependencies = [ "indexmap", "log", @@ -5410,7 +5410,7 @@ checksum = "830b7e5d4d90034032940e4ace0d9a9a057e7a45cd94e6c007832e39edb82f6d" [[package]] name = "version-ranges" version = "0.1.0" -source = "git+https://github.com/astral-sh/pubgrub?branch=charlie/mut#5aadec2663c058cac0ffdacc73d58b01ebdee5a9" +source = "git+https://github.com/astral-sh/pubgrub?rev=30502ceb17c033be408d9766a32ebc211518033f#30502ceb17c033be408d9766a32ebc211518033f" dependencies = [ "smallvec", ] diff --git a/Cargo.toml b/Cargo.toml index 253c53d694440..e885ca4e5acdd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -128,8 +128,8 @@ petgraph = { version = "0.6.5" } platform-info = { version = "2.0.3" } procfs = { version = "0.17.0" , default-features = false, features = ["flate2"] } proc-macro2 = { version = "1.0.86" } -pubgrub = { git = "https://github.com/astral-sh/pubgrub", branch = "charlie/mut" } -version-ranges = { git = "https://github.com/astral-sh/pubgrub", branch = "charlie/mut" } +pubgrub = { git = "https://github.com/astral-sh/pubgrub", rev = "30502ceb17c033be408d9766a32ebc211518033f" } +version-ranges = { git = "https://github.com/astral-sh/pubgrub", rev = "30502ceb17c033be408d9766a32ebc211518033f" } quote = { version = "1.0.37" } rayon = { version = "1.10.0" } reflink-copy = { version = "0.1.19" } diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs index e95ebd586a806..55d308316d06e 100644 --- a/crates/uv-resolver/src/error.rs +++ b/crates/uv-resolver/src/error.rs @@ -230,81 +230,94 @@ impl NoSolutionError { /// implement PEP 440 semantics for local version equality. For example, `1.0.0+foo` needs to /// satisfy `==1.0.0`. pub(crate) fn collapse_local_version_segments(derivation_tree: ErrorTree) -> ErrorTree { - /// Remove local versions sentinels (`+[max]`) from the given version ranges. - fn strip_sentinel(versions: &mut Ranges) { - versions.iter_mut().for_each(|(lower, upper)| { - match (&lower, &upper) { - (Bound::Unbounded, Bound::Unbounded) => {} - (Bound::Unbounded, Bound::Included(v)) => { - // `<=1.0.0+[max]` is equivalent to `<=1.0.0` - if v.local() == LocalVersionSlice::Sentinel { - *upper = Bound::Included(v.clone().without_local()); - } + /// Remove local versions sentinels (`+[max]`) from the interval. + fn strip_sentinel( + mut lower: Bound, + mut upper: Bound, + ) -> (Bound, Bound) { + match (&lower, &upper) { + (Bound::Unbounded, Bound::Unbounded) => {} + (Bound::Unbounded, Bound::Included(v)) => { + // `<=1.0.0+[max]` is equivalent to `<=1.0.0` + if v.local() == LocalVersionSlice::Sentinel { + upper = Bound::Included(v.clone().without_local()); } - (Bound::Unbounded, Bound::Excluded(v)) => { - // `<1.0.0+[max]` is equivalent to `<1.0.0` - if v.local() == LocalVersionSlice::Sentinel { - *upper = Bound::Excluded(v.clone().without_local()); - } + } + (Bound::Unbounded, Bound::Excluded(v)) => { + // `<1.0.0+[max]` is equivalent to `<1.0.0` + if v.local() == LocalVersionSlice::Sentinel { + upper = Bound::Excluded(v.clone().without_local()); } - (Bound::Included(v), Bound::Unbounded) => { - // `>=1.0.0+[max]` is equivalent to `>1.0.0` - if v.local() == LocalVersionSlice::Sentinel { - *lower = Bound::Excluded(v.clone().without_local()); - } + } + (Bound::Included(v), Bound::Unbounded) => { + // `>=1.0.0+[max]` is equivalent to `>1.0.0` + if v.local() == LocalVersionSlice::Sentinel { + lower = Bound::Excluded(v.clone().without_local()); } - (Bound::Included(v), Bound::Included(b)) => { - // `>=1.0.0+[max]` is equivalent to `>1.0.0` - if v.local() == LocalVersionSlice::Sentinel { - *lower = Bound::Excluded(v.clone().without_local()); - } - // `<=1.0.0+[max]` is equivalent to `<=1.0.0` - if b.local() == LocalVersionSlice::Sentinel { - *upper = Bound::Included(b.clone().without_local()); - } + } + (Bound::Included(v), Bound::Included(b)) => { + // `>=1.0.0+[max]` is equivalent to `>1.0.0` + if v.local() == LocalVersionSlice::Sentinel { + lower = Bound::Excluded(v.clone().without_local()); } - (Bound::Included(v), Bound::Excluded(b)) => { - // `>=1.0.0+[max]` is equivalent to `>1.0.0` - if v.local() == LocalVersionSlice::Sentinel { - *lower = Bound::Excluded(v.clone().without_local()); - } - // `<1.0.0+[max]` is equivalent to `<1.0.0` - if b.local() == LocalVersionSlice::Sentinel { - *upper = Bound::Included(b.clone().without_local()); - } + // `<=1.0.0+[max]` is equivalent to `<=1.0.0` + if b.local() == LocalVersionSlice::Sentinel { + upper = Bound::Included(b.clone().without_local()); } - (Bound::Excluded(v), Bound::Unbounded) => { - // `>1.0.0+[max]` is equivalent to `>1.0.0` - if v.local() == LocalVersionSlice::Sentinel { - *lower = Bound::Excluded(v.clone().without_local()); - } + } + (Bound::Included(v), Bound::Excluded(b)) => { + // `>=1.0.0+[max]` is equivalent to `>1.0.0` + if v.local() == LocalVersionSlice::Sentinel { + lower = Bound::Excluded(v.clone().without_local()); } - (Bound::Excluded(v), Bound::Included(b)) => { - // `>1.0.0+[max]` is equivalent to `>1.0.0` - if v.local() == LocalVersionSlice::Sentinel { - *lower = Bound::Excluded(v.clone().without_local()); - } - // `<=1.0.0+[max]` is equivalent to `<=1.0.0` - if b.local() == LocalVersionSlice::Sentinel { - *upper = Bound::Included(b.clone().without_local()); - } + // `<1.0.0+[max]` is equivalent to `<1.0.0` + if b.local() == LocalVersionSlice::Sentinel { + upper = Bound::Included(b.clone().without_local()); } - (Bound::Excluded(v), Bound::Excluded(b)) => { - // `>1.0.0+[max]` is equivalent to `>1.0.0` - if v.local() == LocalVersionSlice::Sentinel { - *lower = Bound::Excluded(v.clone().without_local()); - } - // `<1.0.0+[max]` is equivalent to `<1.0.0` - if b.local() == LocalVersionSlice::Sentinel { - *upper = Bound::Excluded(b.clone().without_local()); - } + } + (Bound::Excluded(v), Bound::Unbounded) => { + // `>1.0.0+[max]` is equivalent to `>1.0.0` + if v.local() == LocalVersionSlice::Sentinel { + lower = Bound::Excluded(v.clone().without_local()); } } - }); + (Bound::Excluded(v), Bound::Included(b)) => { + // `>1.0.0+[max]` is equivalent to `>1.0.0` + if v.local() == LocalVersionSlice::Sentinel { + lower = Bound::Excluded(v.clone().without_local()); + } + // `<=1.0.0+[max]` is equivalent to `<=1.0.0` + if b.local() == LocalVersionSlice::Sentinel { + upper = Bound::Included(b.clone().without_local()); + } + } + (Bound::Excluded(v), Bound::Excluded(b)) => { + // `>1.0.0+[max]` is equivalent to `>1.0.0` + if v.local() == LocalVersionSlice::Sentinel { + lower = Bound::Excluded(v.clone().without_local()); + } + // `<1.0.0+[max]` is equivalent to `<1.0.0` + if b.local() == LocalVersionSlice::Sentinel { + upper = Bound::Excluded(b.clone().without_local()); + } + } + } + (lower, upper) + } + + /// Remove local versions sentinels (`+[max]`) from the version ranges. + // TODO(konsti): Add `impl IntoIterator for Ranges` without leaking the internal + // smallvec to remove the cloning + #[allow(clippy::needless_pass_by_value)] + fn strip_sentinels(versions: Ranges) -> Ranges { + versions + .iter() + .map(|(lower, upper)| strip_sentinel(lower.clone(), upper.clone())) + .collect() } /// Returns `true` if the range appears to be, e.g., `>1.0.0, <1.0.0+[max]`. - fn is_sentinel(versions: &mut Ranges) -> bool { + fn is_sentinel(versions: &Ranges) -> bool { versions.iter().all(|(lower, upper)| { let (Bound::Excluded(lower), Bound::Excluded(upper)) = (lower, upper) else { return false; @@ -319,30 +332,36 @@ impl NoSolutionError { }) } - fn collapse(mut derivation_tree: ErrorTree) -> Option { + fn collapse(derivation_tree: ErrorTree) -> Option { match derivation_tree { DerivationTree::External(External::NotRoot(_, _)) => Some(derivation_tree), - DerivationTree::External(External::NoVersions(_, ref mut versions)) => { - if is_sentinel(versions) { + DerivationTree::External(External::NoVersions(package, versions)) => { + if is_sentinel(&versions) { return None; } - strip_sentinel(versions); - Some(derivation_tree) + let versions = strip_sentinels(versions); + Some(DerivationTree::External(External::NoVersions( + package, versions, + ))) } DerivationTree::External(External::FromDependencyOf( - _, - ref mut versions1, - _, - ref mut versions2, + package1, + versions1, + package2, + versions2, )) => { - strip_sentinel(versions1); - strip_sentinel(versions2); - Some(derivation_tree) + let versions1 = strip_sentinels(versions1); + let versions2 = strip_sentinels(versions2); + Some(DerivationTree::External(External::FromDependencyOf( + package1, versions1, package2, versions2, + ))) } - DerivationTree::External(External::Custom(_, ref mut versions, _)) => { - strip_sentinel(versions); - Some(derivation_tree) + DerivationTree::External(External::Custom(package, versions, reason)) => { + let versions = strip_sentinels(versions); + Some(DerivationTree::External(External::Custom( + package, versions, reason, + ))) } DerivationTree::Derived(mut derived) => { let cause1 = collapse((*derived.cause1).clone()); @@ -353,15 +372,15 @@ impl NoSolutionError { cause2: Arc::new(cause2), terms: std::mem::take(&mut derived.terms) .into_iter() - .map(|(pkg, mut term)| { - match &mut term { + .map(|(pkg, term)| { + let term = match term { Term::Positive(versions) => { - strip_sentinel(versions); + Term::Positive(strip_sentinels(versions)) } Term::Negative(versions) => { - strip_sentinel(versions); + Term::Negative(strip_sentinels(versions)) } - } + }; (pkg, term) }) .collect(),