Upgrade PubGrub (#349)

Upgrades to `fe309ffb63b2f3ce9b35eb7746b2350cd704515e`, with our changes
layered on top.

crates/puffin-cli/tests/snapshots/pip_sync__install_url.snap.new

@@ -1,23 +0,0 @@
----
-source: crates/puffin-cli/tests/pip_sync.rs
-assertion_line: 554
-info:
-  program: puffin
-  args:
-    - pip-sync
-    - requirements.txt
-    - "--cache-dir"
-    - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpZjdQsr
-  env:
-    VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpkhiPUZ/.venv
----
-success: false
-exit_code: 2
------ stdout -----
-
------ stderr -----
-Resolved 1 package in [TIME]
-Downloaded 1 package in [TIME]
-error: Failed to download and unpack wheels
-  Caused by: No such file or directory (os error 2)
-

vendor/pubgrub/Cargo.toml

@@ -8,7 +8,7 @@ authors = [
     "Alex Tokarev <aleksator@gmail.com>",
     "Jacob Finkelman <Eh2406@wayne.edu>",
 ]
-edition = "2018"
+edition = "2021"
 description = "PubGrub version solving algorithm"
 readme = "README.md"
 repository = "https://github.com/pubgrub-rs/pubgrub"
@@ -20,10 +20,10 @@ include = ["Cargo.toml", "LICENSE", "README.md", "src/**", "tests/**", "examples
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-thiserror = "1.0"
-rustc-hash = "1.1.0"
 indexmap = "2.0.2"
 priority-queue = "1.1.1"
+thiserror = "1.0"
+rustc-hash = "1.1.0"
 serde = { version = "1.0", features = ["derive"], optional = true }
 log = "0.4.14" # for debug logs in tests
 

vendor/pubgrub/src/internal/incompatibility.rs

@@ -30,7 +30,7 @@ use crate::version_set::VersionSet;
 /// [PubGrub documentation](https://github.com/dart-lang/pub/blob/master/doc/solver.md#incompatibility).
 #[derive(Debug, Clone)]
 pub struct Incompatibility<P: Package, VS: VersionSet> {
-    pub package_terms: SmallMap<P, Term<VS>>,
+    package_terms: SmallMap<P, Term<VS>>,
     pub kind: Kind<P, VS>,
 }
 

vendor/pubgrub/src/internal/partial_solution.rs

@@ -165,7 +165,13 @@ impl<P: Package, VS: VersionSet, Priority: Ord + Clone> PartialSolution<P, VS, P
                     AssignmentsIntersection::Decision(_) => panic!("Already existing decision"),
                     // Cannot be called if the versions is not contained in the terms intersection.
                     AssignmentsIntersection::Derivations(term) => {
-                        debug_assert!(term.contains(&version))
+                        debug_assert!(
+                            term.contains(&version),
+                            "{}: {} was expected to be contained in {}",
+                            package,
+                            version,
+                            term,
+                        )
                     }
                 },
             }

vendor/pubgrub/src/internal/small_vec.rs

@@ -166,9 +166,9 @@ impl<T> IntoIterator for SmallVec<T> {
     fn into_iter(self) -> Self::IntoIter {
         match self {
             SmallVec::Empty => SmallVecIntoIter::Empty,
-            SmallVec::One(a) => SmallVecIntoIter::One(IntoIterator::into_iter(a)),
+            SmallVec::One(a) => SmallVecIntoIter::One(a.into_iter()),
-            SmallVec::Two(a) => SmallVecIntoIter::Two(IntoIterator::into_iter(a)),
+            SmallVec::Two(a) => SmallVecIntoIter::Two(a.into_iter()),
-            SmallVec::Flexible(v) => SmallVecIntoIter::Flexible(IntoIterator::into_iter(v)),
+            SmallVec::Flexible(v) => SmallVecIntoIter::Flexible(v.into_iter()),
         }
     }
 }

vendor/pubgrub/src/solver.rs

@@ -165,7 +165,7 @@ pub fn resolve<P: Package, VS: VersionSet>(
                 Dependencies::Known(x) if x.contains_key(p) => {
                     return Err(PubGrubError::SelfDependency {
                         package: p.clone(),
-                        version: v.clone(),
+                        version: v,
                     });
                 }
                 Dependencies::Known(x) => x,

vendor/pubgrub/tests/proptest.rs

@@ -5,8 +5,9 @@ use std::{collections::BTreeSet as Set, error::Error};
 use pubgrub::error::PubGrubError;
 use pubgrub::package::Package;
 use pubgrub::range::Range;
-use pubgrub::report::{DefaultStringReporter, Reporter};
+use pubgrub::report::{DefaultStringReporter, DerivationTree, External, Reporter};
 use pubgrub::solver::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider};
+use pubgrub::type_aliases::SelectedDependencies;
 use pubgrub::version::{NumberVersion, SemanticVersion};
 use pubgrub::version_set::VersionSet;
 
@@ -112,6 +113,18 @@ impl<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>> DependencyProvid
     }
 }
 
+fn timeout_resolve<P: Package, VS: VersionSet, DP: DependencyProvider<P, VS>>(
+    dependency_provider: DP,
+    name: P,
+    version: impl Into<VS::V>,
+) -> Result<SelectedDependencies<P, VS::V>, PubGrubError<P, VS>> {
+    resolve(
+        &TimeoutDependencyProvider::new(dependency_provider, 50_000),
+        name,
+        version,
+    )
+}
+
 type NumVS = Range<NumberVersion>;
 type SemVS = Range<SemanticVersion>;
 
@@ -283,6 +296,110 @@ fn meta_test_deep_trees_from_strategy() {
     );
 }
 
+/// Removes versions from the dependency provider where the retain function returns false.
+/// Solutions are constructed as a set of versions.
+/// If there are fewer versions available, there are fewer valid solutions available.
+/// If there was no solution to a resolution in the original dependency provider,
+/// then there must still be no solution with some options removed.
+/// If there was a solution to a resolution in the original dependency provider,
+/// there may not be a solution after versions are removes iif removed versions were critical for all valid solutions.
+fn retain_versions<N: Package + Ord, VS: VersionSet>(
+    dependency_provider: &OfflineDependencyProvider<N, VS>,
+    mut retain: impl FnMut(&N, &VS::V) -> bool,
+) -> OfflineDependencyProvider<N, VS> {
+    let mut smaller_dependency_provider = OfflineDependencyProvider::new();
+
+    for n in dependency_provider.packages() {
+        for v in dependency_provider.versions(n).unwrap() {
+            if !retain(n, v) {
+                continue;
+            }
+            let deps = match dependency_provider.get_dependencies(&n, &v).unwrap() {
+                Dependencies::Unknown => panic!(),
+                Dependencies::Known(deps) => deps,
+            };
+            smaller_dependency_provider.add_dependencies(n.clone(), v.clone(), deps)
+        }
+    }
+    smaller_dependency_provider
+}
+
+/// Removes dependencies from the dependency provider where the retain function returns false.
+/// Solutions are constraned by having to fulfill all the dependencies.
+/// If there are fewer dependencies required, there are more valid solutions.
+/// If there was a solution to a resolution in the original dependency provider,
+/// then there must still be a solution after dependencies are removed.
+/// If there was no solution to a resolution in the original dependency provider,
+/// there may now be a solution after dependencies are removed.
+fn retain_dependencies<N: Package + Ord, VS: VersionSet>(
+    dependency_provider: &OfflineDependencyProvider<N, VS>,
+    mut retain: impl FnMut(&N, &VS::V, &N) -> bool,
+) -> OfflineDependencyProvider<N, VS> {
+    let mut smaller_dependency_provider = OfflineDependencyProvider::new();
+    for n in dependency_provider.packages() {
+        for v in dependency_provider.versions(n).unwrap() {
+            let deps = match dependency_provider.get_dependencies(&n, &v).unwrap() {
+                Dependencies::Unknown => panic!(),
+                Dependencies::Known(deps) => deps,
+            };
+            smaller_dependency_provider.add_dependencies(
+                n.clone(),
+                v.clone(),
+                deps.iter().filter_map(|(dep, range)| {
+                    if !retain(n, v, dep) {
+                        None
+                    } else {
+                        Some((dep.clone(), range.clone()))
+                    }
+                }),
+            );
+        }
+    }
+    smaller_dependency_provider
+}
+
+fn errors_the_same_with_only_report_dependencies<N: Package + Ord>(
+    dependency_provider: OfflineDependencyProvider<N, NumVS>,
+    name: N,
+    ver: NumberVersion,
+) {
+    let Err(PubGrubError::NoSolution(tree)) =
+        timeout_resolve(dependency_provider.clone(), name.clone(), ver)
+    else {
+        return;
+    };
+
+    fn recursive<N: Package + Ord, VS: VersionSet>(
+        to_retain: &mut Vec<(N, VS, N)>,
+        tree: &DerivationTree<N, VS>,
+    ) {
+        match tree {
+            DerivationTree::External(External::FromDependencyOf(n1, vs1, n2, _)) => {
+                to_retain.push((n1.clone(), vs1.clone(), n2.clone()));
+            }
+            DerivationTree::Derived(d) => {
+                recursive(to_retain, &*d.cause1);
+                recursive(to_retain, &*d.cause2);
+            }
+            _ => {}
+        }
+    }
+
+    let mut to_retain = Vec::new();
+    recursive(&mut to_retain, &tree);
+
+    let removed_provider = retain_dependencies(&dependency_provider, |p, v, d| {
+        to_retain
+            .iter()
+            .any(|(n1, vs1, n2)| n1 == p && vs1.contains(v) && n2 == d)
+    });
+
+    assert!(
+        timeout_resolve(removed_provider.clone(), name, ver).is_err(),
+        "The full index errored filtering to only dependencies in the derivation tree succeeded"
+    );
+}
+
 proptest! {
     #![proptest_config(ProptestConfig {
     max_shrink_iters:
@@ -303,7 +420,7 @@ proptest! {
         (dependency_provider, cases) in registry_strategy(string_names())
     )  {
         for (name, ver) in cases {
-            let _ = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver);
+            _ = timeout_resolve(dependency_provider.clone(), name, ver);
         }
     }
 
@@ -313,7 +430,7 @@ proptest! {
         (dependency_provider, cases) in registry_strategy(0u16..665)
     )  {
         for (name, ver) in cases {
-            let _ = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver);
+            _ = timeout_resolve(dependency_provider.clone(), name, ver);
         }
     }
 
@@ -323,11 +440,17 @@ proptest! {
     )  {
         let mut sat = SatResolve::new(&dependency_provider);
         for (name, ver) in cases {
-            if let Ok(s) = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver) {
+            let res = timeout_resolve(dependency_provider.clone(), name, ver);
-                prop_assert!(sat.sat_is_valid_solution(&s));
+            sat.check_resolve(&res, &name, &ver);
-            } else {
+        }
-                prop_assert!(!sat.sat_resolve(&name, &ver));
+    }
-            }
+
+    #[test]
+    fn prop_errors_the_same_with_only_report_dependencies(
+        (dependency_provider, cases) in registry_strategy(0u16..665)
+    )  {
+        for (name, ver) in cases {
+            errors_the_same_with_only_report_dependencies(dependency_provider.clone(), name, ver);
         }
     }
 
@@ -337,9 +460,9 @@ proptest! {
         (dependency_provider, cases) in registry_strategy(0u16..665)
     )  {
         for (name, ver) in cases {
-            let one = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver);
+            let one = timeout_resolve(dependency_provider.clone(), name, ver);
             for _ in 0..3 {
-                match (&one, &resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver)) {
+                match (&one, &timeout_resolve(dependency_provider.clone(), name, ver)) {
                     (Ok(l), Ok(r)) => assert_eq!(l, r),
                     (Err(PubGrubError::NoSolution(derivation_l)), Err(PubGrubError::NoSolution(derivation_r))) => {
                         prop_assert_eq!(
@@ -360,8 +483,8 @@ proptest! {
     )  {
         let reverse_provider = OldestVersionsDependencyProvider(dependency_provider.clone());
         for (name, ver) in cases {
-            let l = resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver);
+            let l = timeout_resolve(dependency_provider.clone(), name, ver);
-            let r = resolve(&TimeoutDependencyProvider::new(reverse_provider.clone(), 50_000), name, ver);
+            let r = timeout_resolve(reverse_provider.clone(), name, ver);
             match (&l, &r) {
                 (Ok(_), Ok(_)) => (),
                 (Err(_), Err(_)) => (),
@@ -376,7 +499,7 @@ proptest! {
         indexes_to_remove in prop::collection::vec((any::<prop::sample::Index>(), any::<prop::sample::Index>(), any::<prop::sample::Index>()), 1..10)
     ) {
         let packages: Vec<_> = dependency_provider.packages().collect();
-        let mut removed_provider = dependency_provider.clone();
+        let mut to_remove = Set::new();
         for (package_idx, version_idx, dep_idx) in indexes_to_remove {
             let package = package_idx.get(&packages);
             let versions: Vec<_> = dependency_provider
@@ -391,29 +514,17 @@ proptest! {
                 Dependencies::Known(d) => d.into_iter().collect(),
             };
             if !dependencies.is_empty() {
-                let dependency = dep_idx.get(&dependencies).0;
+                to_remove.insert((package, **version, dep_idx.get(&dependencies).0));
-                removed_provider.add_dependencies(
-                    **package,
-                    **version,
-                    dependencies.into_iter().filter(|x| x.0 != dependency),
-                )
             }
         }
+        let removed_provider = retain_dependencies(
+            &dependency_provider,
+            |p, v, d| {!to_remove.contains(&(&p, *v, *d))}
+        );
         for (name, ver) in cases {
-            if resolve(
+            if timeout_resolve(dependency_provider.clone(), name, ver).is_ok() {
-                &TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000),
-                name,
-                ver,
-            )
-            .is_ok()
-            {
                 prop_assert!(
-                    resolve(
+                    timeout_resolve(removed_provider.clone(), name, ver).is_ok(),
-                        &TimeoutDependencyProvider::new(removed_provider.clone(), 50_000),
-                        name,
-                        ver
-                    )
-                    .is_ok(),
                     "full index worked for `{} = \"={}\"` but removing some deps broke it!",
                     name,
                     ver,
@@ -438,24 +549,16 @@ proptest! {
             .collect();
         let to_remove: Set<(_, _)> = indexes_to_remove.iter().map(|x| x.get(&all_versions)).cloned().collect();
         for (name, ver) in cases {
-            match resolve(&TimeoutDependencyProvider::new(dependency_provider.clone(), 50_000), name, ver) {
+            match timeout_resolve(dependency_provider.clone(), name, ver) {
                 Ok(used) => {
                     // If resolution was successful, then unpublishing a version of a crate
                     // that was not selected should not change that.
-                    let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumVS>::new();
+                    let smaller_dependency_provider = retain_versions(&dependency_provider, |n, v| {
-                    for &(n, v) in &all_versions {
+                            used.get(&n) == Some(&v) // it was used
-                        if used.get(&n) == Some(&v) // it was used
+                            || to_remove.get(&(*n, *v)).is_none() // or it is not one to be removed
-                           || to_remove.get(&(n, v)).is_none() // or it is not one to be removed
+                        });
-                        {
-                            let deps = match dependency_provider.get_dependencies(&n, &v).unwrap() {
-                                Dependencies::Unknown => panic!(),
-                                Dependencies::Known(deps) => deps,
-                            };
-                            smaller_dependency_provider.add_dependencies(n, v, deps)
-                        }
-                    }
                     prop_assert!(
-                        resolve(&TimeoutDependencyProvider::new(smaller_dependency_provider.clone(), 50_000), name, ver).is_ok(),
+                        timeout_resolve(smaller_dependency_provider.clone(), name, ver).is_ok(),
                         "unpublishing {:?} stopped `{} = \"={}\"` from working",
                         to_remove,
                         name,
@@ -465,19 +568,11 @@ proptest! {
                 Err(_) => {
                     // If resolution was unsuccessful, then it should stay unsuccessful
                     // even if any version of a crate is unpublished.
-                    let mut smaller_dependency_provider = OfflineDependencyProvider::<_, NumVS>::new();
+                    let smaller_dependency_provider = retain_versions(&dependency_provider, |n, v| {
-                    for &(n, v) in &all_versions {
+                        to_remove.get(&(*n, *v)).is_some() // it is one to be removed
-                        if to_remove.get(&(n, v)).is_none() // it is not one to be removed
+                    });
-                        {
-                            let deps = match dependency_provider.get_dependencies(&n, &v).unwrap() {
-                                Dependencies::Unknown => panic!(),
-                                Dependencies::Known(deps) => deps,
-                            };
-                            smaller_dependency_provider.add_dependencies(n, v, deps)
-                        }
-                    }
                     prop_assert!(
-                        resolve(&TimeoutDependencyProvider::new(smaller_dependency_provider.clone(), 50_000), name, ver).is_err(),
+                        timeout_resolve(smaller_dependency_provider.clone(), name, ver).is_err(),
                         "full index did not work for `{} = \"={}\"` but unpublishing {:?} fixed it!",
                         name,
                         ver,
@@ -495,19 +590,17 @@ fn large_case() {
     for case in std::fs::read_dir("test-examples").unwrap() {
         let case = case.unwrap().path();
         let name = case.file_name().unwrap().to_string_lossy();
-        eprintln!("{}", name);
+        eprint!("{} ", name);
         let data = std::fs::read_to_string(&case).unwrap();
+        let start_time = std::time::Instant::now();
         if name.ends_with("u16_NumberVersion.ron") {
             let dependency_provider: OfflineDependencyProvider<u16, NumVS> =
                 ron::de::from_str(&data).unwrap();
             let mut sat = SatResolve::new(&dependency_provider);
             for p in dependency_provider.packages() {
-                for n in dependency_provider.versions(p).unwrap() {
+                for v in dependency_provider.versions(p).unwrap() {
-                    if let Ok(s) = resolve(&dependency_provider, p.clone(), n.clone()) {
+                    let res = resolve(&dependency_provider, p.clone(), v);
-                        assert!(sat.sat_is_valid_solution(&s));
+                    sat.check_resolve(&res, p, v);
-                    } else {
-                        assert!(!sat.sat_resolve(p, &n));
-                    }
                 }
             }
         } else if name.ends_with("str_SemanticVersion.ron") {
@@ -515,14 +608,12 @@ fn large_case() {
                 ron::de::from_str(&data).unwrap();
             let mut sat = SatResolve::new(&dependency_provider);
             for p in dependency_provider.packages() {
-                for n in dependency_provider.versions(p).unwrap() {
+                for v in dependency_provider.versions(p).unwrap() {
-                    if let Ok(s) = resolve(&dependency_provider, p, n.clone()) {
+                    let res = resolve(&dependency_provider, p.clone(), v);
-                        assert!(sat.sat_is_valid_solution(&s));
+                    sat.check_resolve(&res, p, v);
-                    } else {
-                        assert!(!sat.sat_resolve(p, &n));
-                    }
                 }
             }
         }
+        eprintln!(" in {}s", start_time.elapsed().as_secs())
     }
 }

vendor/pubgrub/tests/sat_dependency_provider.rs

@@ -1,23 +1,12 @@
 // SPDX-License-Identifier: MPL-2.0
 
+use pubgrub::error::PubGrubError;
 use pubgrub::package::Package;
 use pubgrub::solver::{Dependencies, DependencyProvider, OfflineDependencyProvider};
 use pubgrub::type_aliases::{Map, SelectedDependencies};
 use pubgrub::version_set::VersionSet;
 use varisat::ExtendFormula;
 
-const fn num_bits<T>() -> usize {
-    std::mem::size_of::<T>() * 8
-}
-
-fn log_bits(x: usize) -> usize {
-    if x == 0 {
-        return 0;
-    }
-    assert!(x > 0);
-    (num_bits::<usize>() as u32 - x.leading_zeros()) as usize
-}
-
 fn sat_at_most_one(solver: &mut impl varisat::ExtendFormula, vars: &[varisat::Var]) {
     if vars.len() <= 1 {
         return;
@@ -32,7 +21,8 @@ fn sat_at_most_one(solver: &mut impl varisat::ExtendFormula, vars: &[varisat::Va
     }
     // use the "Binary Encoding" from
     // https://www.it.uu.se/research/group/astra/ModRef10/papers/Alan%20M.%20Frisch%20and%20Paul%20A.%20Giannoros.%20SAT%20Encodings%20of%20the%20At-Most-k%20Constraint%20-%20ModRef%202010.pdf
-    let bits: Vec<varisat::Var> = solver.new_var_iter(log_bits(vars.len())).collect();
+    let len_bits = vars.len().ilog2() as usize + 1;
+    let bits: Vec<varisat::Var> = solver.new_var_iter(len_bits).collect();
     for (i, p) in vars.iter().enumerate() {
         for (j, &bit) in bits.iter().enumerate() {
             solver.add_clause(&[p.negative(), bit.lit(((1 << j) & i) > 0)]);
@@ -110,7 +100,7 @@ impl<P: Package, VS: VersionSet> SatResolve<P, VS> {
         }
     }
 
-    pub fn sat_resolve(&mut self, name: &P, ver: &VS::V) -> bool {
+    pub fn resolve(&mut self, name: &P, ver: &VS::V) -> bool {
         if let Some(vers) = self.all_versions_by_p.get(name) {
             if let Some((_, var)) = vers.iter().find(|(v, _)| v == ver) {
                 self.solver.assume(&[var.positive()]);
@@ -126,16 +116,13 @@ impl<P: Package, VS: VersionSet> SatResolve<P, VS> {
         }
     }
 
-    pub fn sat_is_valid_solution(&mut self, pids: &SelectedDependencies<P, VS::V>) -> bool {
+    pub fn is_valid_solution(&mut self, pids: &SelectedDependencies<P, VS::V>) -> bool {
         let mut assumption = vec![];
 
         for (p, vs) in &self.all_versions_by_p {
+            let pid_for_p = pids.get(p);
             for (v, var) in vs {
-                assumption.push(if pids.get(p) == Some(v) {
+                assumption.push(var.lit(pid_for_p == Some(v)))
-                    var.positive()
-                } else {
-                    var.negative()
-                })
             }
         }
 
@@ -145,4 +132,20 @@ impl<P: Package, VS: VersionSet> SatResolve<P, VS> {
             .solve()
             .expect("docs say it can't error in default config")
     }
+
+    pub fn check_resolve(
+        &mut self,
+        res: &Result<SelectedDependencies<P, VS::V>, PubGrubError<P, VS>>,
+        p: &P,
+        v: &VS::V,
+    ) {
+        match res {
+            Ok(s) => {
+                assert!(self.is_valid_solution(s));
+            }
+            Err(_) => {
+                assert!(!self.resolve(p, v));
+            }
+        }
+    }
 }