Use available versions to simplify unsat error reports (#547)

Uses https://github.com/pubgrub-rs/pubgrub/pull/156 to consolidate
version ranges in error reports using the actual available versions for
each package.

Alternative to https://github.com/zanieb/pubgrub/pull/8 which implements
this behavior as a method in the `Reporter` — here it's implemented in
our custom report formatter (#521) instead which requires no upstream
changes.

Requires https://github.com/zanieb/pubgrub/pull/11 to only retrieve the
versions for packages that will be used in the report.

This is a work in progress. Some things to do:
- ~We may want to allow lazy retrieval of the version maps from the
formatter~
- [x] We should probably create a separate error type for no solution
instead of mixing them with other resolve errors
- ~We can probably do something smarter than creating vectors to hold
the versions~
- [x] This degrades error messages when a single version is not
available, we'll need to special case that
- [x] It seems safer to coerce the error type in `resolve` instead of
`solve` if feasible

Cargo.lock

@@ -2229,7 +2229,7 @@ dependencies = [
 [[package]]
 name = "pubgrub"
 version = "0.2.1"
-source = "git+https://github.com/zanieb/pubgrub?rev=a1d584a5e506b8f0a600269373190c4a35b298d5#a1d584a5e506b8f0a600269373190c4a35b298d5"
+source = "git+https://github.com/zanieb/pubgrub?rev=8fff95d6a233a9fa4ee5ab5429033f0f0d3ddb20#8fff95d6a233a9fa4ee5ab5429033f0f0d3ddb20"
 dependencies = [
  "indexmap 2.1.0",
  "log",

Cargo.toml

@@ -51,7 +51,7 @@ once_cell = { version = "1.18.0" }
 petgraph = { version = "0.6.4" }
 platform-info = { version = "2.0.2" }
 plist = { version = "1.6.0" }
-pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "a1d584a5e506b8f0a600269373190c4a35b298d5" }
+pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "8fff95d6a233a9fa4ee5ab5429033f0f0d3ddb20" }
 pyo3 = { version = "0.20.0" }
 pyo3-log = { version = "0.9.0"}
 pyproject-toml = { version = "0.8.0" }

crates/puffin-cli/src/commands/pip_compile.rs

@@ -152,7 +152,7 @@ pub(crate) async fn pip_compile(
     let resolver = Resolver::new(manifest, options, &markers, &tags, &client, &build_dispatch)
         .with_reporter(ResolverReporter::from(printer));
     let resolution = match resolver.resolve().await {
-        Err(puffin_resolver::ResolveError::PubGrub(err)) => {
+        Err(puffin_resolver::ResolveError::NoSolution(err)) => {
             #[allow(clippy::print_stderr)]
             {
                 let report = miette::Report::msg(format!("{err}"))

crates/puffin-cli/src/commands/pip_install.rs

@@ -237,14 +237,14 @@ async fn resolve(
     let resolver = Resolver::new(manifest, options, markers, &tags, &client, &build_dispatch)
         .with_reporter(ResolverReporter::from(printer));
     let resolution = match resolver.resolve().await {
-        Err(puffin_resolver::ResolveError::PubGrub(err)) => {
+        Err(puffin_resolver::ResolveError::NoSolution(err)) => {
             #[allow(clippy::print_stderr)]
             {
                 let report = miette::Report::msg(format!("{err}"))
                     .context("No solution found when resolving dependencies:");
                 eprint!("{report:?}");
             }
-            return Err(puffin_resolver::ResolveError::PubGrub(err).into());
+            return Err(puffin_resolver::ResolveError::NoSolution(err).into());
         }
         result => result,
     }?;

crates/puffin-resolver/src/error.rs

@@ -1,7 +1,9 @@
 use std::fmt::Formatter;
 
 use pubgrub::range::Range;
-use pubgrub::report::{DefaultStringReporter, Reporter};
+use pubgrub::report::{DefaultStringReporter, DerivationTree, Reporter};
+use pypi_types::IndexUrl;
+use rustc_hash::FxHashMap;
 use thiserror::Error;
 use url::Url;
 
@@ -9,8 +11,10 @@ use distribution_types::{BuiltDist, PathBuiltDist, PathSourceDist, SourceDist};
 use pep508_rs::Requirement;
 use puffin_distribution::DistributionDatabaseError;
 use puffin_normalize::PackageName;
+use puffin_traits::OnceMap;
 
 use crate::pubgrub::{PubGrubPackage, PubGrubVersion};
+use crate::version_map::VersionMap;
 use crate::PubGrubReportFormatter;
 
 #[derive(Error, Debug)]
@@ -30,9 +34,6 @@ pub enum ResolveError {
     #[error(transparent)]
     Join(#[from] tokio::task::JoinError),
 
-    #[error(transparent)]
-    PubGrub(#[from] RichPubGrubError),
-
     #[error("Package metadata name `{metadata}` does not match given name `{given}`")]
     NameMismatch {
         given: PackageName,
@@ -65,6 +66,37 @@ pub enum ResolveError {
 
     #[error("Failed to build: {0}")]
     Build(Box<PathSourceDist>, #[source] DistributionDatabaseError),
+
+    #[error(transparent)]
+    NoSolution(#[from] NoSolutionError),
+
+    #[error("Retrieving dependencies of {package} {version} failed")]
+    ErrorRetrievingDependencies {
+        /// Package whose dependencies we want.
+        package: Box<PubGrubPackage>,
+        /// Version of the package for which we want the dependencies.
+        version: Box<PubGrubVersion>,
+        /// Error raised by the implementer of [DependencyProvider](crate::solver::DependencyProvider).
+        source: Box<dyn std::error::Error + Send + Sync>,
+    },
+
+    #[error("{package} {version} depends on itself")]
+    SelfDependency {
+        /// Package whose dependencies we want.
+        package: Box<PubGrubPackage>,
+        /// Version of the package for which we want the dependencies.
+        version: Box<PubGrubVersion>,
+    },
+
+    #[error("Decision making failed")]
+    ErrorChoosingPackageVersion(Box<dyn std::error::Error + Send + Sync>),
+
+    #[error("We should cancel")]
+    ErrorInShouldCancel(Box<dyn std::error::Error + Send + Sync>),
+
+    /// Something unexpected happened.
+    #[error("{0}")]
+    Failure(String),
 }
 
 impl<T> From<futures::channel::mpsc::TrySendError<T>> for ResolveError {
@@ -73,28 +105,83 @@ impl<T> From<futures::channel::mpsc::TrySendError<T>> for ResolveError {
     }
 }
 
-/// A wrapper around [`pubgrub::error::PubGrubError`] that displays a resolution failure report.
-#[derive(Debug)]
-pub struct RichPubGrubError {
-    source: pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>>,
-}
-
-impl std::error::Error for RichPubGrubError {}
-
-impl std::fmt::Display for RichPubGrubError {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        if let pubgrub::error::PubGrubError::NoSolution(derivation_tree) = &self.source {
-            let formatter = PubGrubReportFormatter;
-            let report = DefaultStringReporter::report_with_formatter(derivation_tree, &formatter);
-            write!(f, "{report}")
-        } else {
-            write!(f, "{}", self.source)
+impl From<pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>>> for ResolveError {
+    fn from(value: pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>>) -> Self {
+        match value {
+            pubgrub::error::PubGrubError::ErrorChoosingPackageVersion(inner) => {
+                ResolveError::ErrorChoosingPackageVersion(inner)
+            }
+            pubgrub::error::PubGrubError::ErrorInShouldCancel(inner) => {
+                ResolveError::ErrorInShouldCancel(inner)
+            }
+            pubgrub::error::PubGrubError::ErrorRetrievingDependencies {
+                package,
+                version,
+                source,
+            } => ResolveError::ErrorRetrievingDependencies {
+                package: Box::new(package),
+                version: Box::new(version),
+                source,
+            },
+            pubgrub::error::PubGrubError::Failure(inner) => ResolveError::Failure(inner),
+            pubgrub::error::PubGrubError::NoSolution(derivation_tree) => {
+                ResolveError::NoSolution(NoSolutionError {
+                    derivation_tree,
+                    available_versions: FxHashMap::default(),
+                })
+            }
+            pubgrub::error::PubGrubError::SelfDependency { package, version } => {
+                ResolveError::SelfDependency {
+                    package: Box::new(package),
+                    version: Box::new(version),
+                }
+            }
         }
     }
 }
 
-impl From<pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>>> for ResolveError {
-    fn from(value: pubgrub::error::PubGrubError<PubGrubPackage, Range<PubGrubVersion>>) -> Self {
-        ResolveError::PubGrub(RichPubGrubError { source: value })
+/// A wrapper around [`pubgrub::error::PubGrubError::NoSolution`] that displays a resolution failure report.
+#[derive(Debug)]
+pub struct NoSolutionError {
+    derivation_tree: DerivationTree<PubGrubPackage, Range<PubGrubVersion>>,
+    available_versions: FxHashMap<PubGrubPackage, Vec<PubGrubVersion>>,
+}
+
+impl std::error::Error for NoSolutionError {}
+
+impl std::fmt::Display for NoSolutionError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        let formatter = PubGrubReportFormatter {
+            available_versions: &self.available_versions,
+        };
+        let report =
+            DefaultStringReporter::report_with_formatter(&self.derivation_tree, &formatter);
+        write!(f, "{report}")
+    }
+}
+
+impl NoSolutionError {
+    /// Update the available versions attached to the error using the given package version index.
+    ///
+    /// Only packages used in the error's deriviation tree will be retrieved.
+    pub(crate) fn update_available_versions(
+        mut self,
+        package_versions: &OnceMap<PackageName, (IndexUrl, VersionMap)>,
+    ) -> Self {
+        for package in self.derivation_tree.packages() {
+            if let PubGrubPackage::Package(name, ..) = package {
+                if let Some(entry) = package_versions.get(name) {
+                    let (_, version_map) = entry.value();
+                    self.available_versions.insert(
+                        package.clone(),
+                        version_map
+                            .iter()
+                            .map(|(version, _)| version.clone())
+                            .collect(),
+                    );
+                }
+            }
+        }
+        self
     }
 }

crates/puffin-resolver/src/pubgrub/report.rs

@@ -2,13 +2,32 @@ use pubgrub::range::Range;
 use pubgrub::report::{External, ReportFormatter};
 use pubgrub::term::Term;
 use pubgrub::type_aliases::Map;
+use rustc_hash::FxHashMap;
 
 use super::{PubGrubPackage, PubGrubVersion};
 
-#[derive(Debug, Default)]
-pub struct PubGrubReportFormatter;
+#[derive(Debug)]
+pub struct PubGrubReportFormatter<'a> {
+    /// The versions that were available for each package
+    pub available_versions: &'a FxHashMap<PubGrubPackage, Vec<PubGrubVersion>>,
+}
 
-impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFormatter {
+impl PubGrubReportFormatter<'_> {
+    fn simplify_set(
+        &self,
+        set: &Range<PubGrubVersion>,
+        package: &PubGrubPackage,
+    ) -> Range<PubGrubVersion> {
+        set.simplify(
+            self.available_versions
+                .get(package)
+                .unwrap_or(&vec![])
+                .iter(),
+        )
+    }
+}
+
+impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFormatter<'_> {
     type Output = String;
 
     fn format_external(
@@ -23,6 +42,7 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
                 if set == &Range::full() {
                     format!("there is no available version for {package}")
                 } else {
+                    let set = self.simplify_set(set, package);
                     format!("there is no version of {package} available matching {set}")
                 }
             }
@@ -30,6 +50,7 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
                 if set == &Range::full() {
                     format!("dependencies of {package} are unavailable")
                 } else {
+                    let set = self.simplify_set(set, package);
                     format!("dependencies of {package} at version {set} are unavailable")
                 }
             }
@@ -41,6 +62,7 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
                         if set == &Range::full() {
                             format!("dependencies of {package} are unusable: {reason}")
                         } else {
+                            let set = self.simplify_set(set, package);
                             format!("dependencies of {package}{set} are unusable: {reason}",)
                         }
                     }
@@ -48,6 +70,7 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
                     if set == &Range::full() {
                         format!("dependencies of {package} are unusable")
                     } else {
+                        let set = self.simplify_set(set, package);
                         format!("dependencies of {package}{set} are unusable")
                     }
                 }
@@ -56,19 +79,23 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
                 if package_set == &Range::full() && dependency_set == &Range::full() {
                     format!("{package} depends on {dependency}")
                 } else if package_set == &Range::full() {
+                    let dependency_set = self.simplify_set(dependency_set, package);
                     format!("{package} depends on {dependency}{dependency_set}")
                 } else if dependency_set == &Range::full() {
                     if matches!(package, PubGrubPackage::Root(_)) {
                         // Exclude the dummy version for root packages
                         format!("{package} depends on {dependency}")
                     } else {
+                        let package_set = self.simplify_set(package_set, package);
                         format!("{package}{package_set} depends on {dependency}")
                     }
                 } else {
+                    let dependency_set = self.simplify_set(dependency_set, package);
                     if matches!(package, PubGrubPackage::Root(_)) {
                         // Exclude the dummy version for root packages
                         format!("{package} depends on {dependency}{dependency_set}")
                     } else {
+                        let package_set = self.simplify_set(package_set, package);
                         format!("{package}{package_set} depends on {dependency}{dependency_set}")
                     }
                 }
@@ -82,9 +109,21 @@ impl ReportFormatter<PubGrubPackage, Range<PubGrubVersion>> for PubGrubReportFor
         match terms_vec.as_slice() {
             [] | [(PubGrubPackage::Root(_), _)] => "version solving failed".into(),
             [(package @ PubGrubPackage::Package(..), Term::Positive(range))] => {
+                let range = range.simplify(
+                    self.available_versions
+                        .get(package)
+                        .unwrap_or(&vec![])
+                        .iter(),
+                );
                 format!("{package}{range} is forbidden")
             }
             [(package @ PubGrubPackage::Package(..), Term::Negative(range))] => {
+                let range = range.simplify(
+                    self.available_versions
+                        .get(package)
+                        .unwrap_or(&vec![])
+                        .iter(),
+                );
                 format!("{package}{range} is mandatory")
             }
             [(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => self.format_external(

crates/puffin-resolver/src/resolver.rs

@@ -247,7 +247,14 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
                 return Err(ResolveError::StreamTermination);
             }
             resolution = resolve_fut => {
-                resolution?
+                resolution.map_err(|err| {
+                    // Add version information to improve unsat error messages
+                    if let ResolveError::NoSolution(err) = err {
+                        ResolveError::NoSolution(err.update_available_versions(&self.index.packages))
+                    } else {
+                        err
+                    }
+                })?
             }
         };