diff --git a/Cargo.lock b/Cargo.lock index f456cf95c1ad..7f7043205b15 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2229,7 +2229,7 @@ dependencies = [ [[package]] name = "pubgrub" version = "0.2.1" -source = "git+https://github.com/zanieb/pubgrub?rev=725b9a8e323c9ae0727fed2d045bad64eb212167#725b9a8e323c9ae0727fed2d045bad64eb212167" +source = "git+https://github.com/zanieb/pubgrub?rev=c4ffd26715df9f21067983b0eb176dfd14876133#c4ffd26715df9f21067983b0eb176dfd14876133" dependencies = [ "indexmap 2.1.0", "log", diff --git a/Cargo.toml b/Cargo.toml index 94ed2feaf8fe..1daffae48493 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,7 +50,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 = "725b9a8e323c9ae0727fed2d045bad64eb212167" } +pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "c4ffd26715df9f21067983b0eb176dfd14876133" } pyo3 = { version = "0.20.0" } pyo3-log = { version = "0.9.0"} pyproject-toml = { version = "0.8.0" } diff --git a/crates/puffin-resolver/src/error.rs b/crates/puffin-resolver/src/error.rs index 18e6ea07d4f0..60fe3229856b 100644 --- a/crates/puffin-resolver/src/error.rs +++ b/crates/puffin-resolver/src/error.rs @@ -1,7 +1,7 @@ use std::fmt::Formatter; use pubgrub::range::Range; -use pubgrub::report::Reporter; +use pubgrub::report::{DefaultStringReporter, Reporter}; use thiserror::Error; use url::Url; @@ -11,7 +11,7 @@ use puffin_distribution::DistributionDatabaseError; use puffin_normalize::PackageName; use crate::pubgrub::{PubGrubPackage, PubGrubVersion}; -use crate::ResolutionFailureReporter; +use crate::PubGrubReportFormatter; #[derive(Error, Debug)] pub enum ResolveError { @@ -78,7 +78,8 @@ 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 report = ResolutionFailureReporter::report(derivation_tree); + let formatter = PubGrubReportFormatter; + let report = DefaultStringReporter::report_with_formatter(derivation_tree, &formatter); write!(f, "{report}") } else { write!(f, "{}", self.source) diff --git a/crates/puffin-resolver/src/lib.rs b/crates/puffin-resolver/src/lib.rs index 642d0ae2dfb7..8713d3bc0753 100644 --- a/crates/puffin-resolver/src/lib.rs +++ b/crates/puffin-resolver/src/lib.rs @@ -2,7 +2,7 @@ pub use error::ResolveError; pub use finder::{DistFinder, Reporter as FinderReporter}; pub use manifest::Manifest; pub use prerelease_mode::PreReleaseMode; -pub use pubgrub::ResolutionFailureReporter; +pub use pubgrub::PubGrubReportFormatter; pub use resolution::Graph; pub use resolution_mode::ResolutionMode; pub use resolution_options::ResolutionOptions; diff --git a/crates/puffin-resolver/src/pubgrub/mod.rs b/crates/puffin-resolver/src/pubgrub/mod.rs index 1f52e13ed556..2e58d52621bc 100644 --- a/crates/puffin-resolver/src/pubgrub/mod.rs +++ b/crates/puffin-resolver/src/pubgrub/mod.rs @@ -1,6 +1,6 @@ pub(crate) use crate::pubgrub::package::PubGrubPackage; pub(crate) use crate::pubgrub::priority::{PubGrubPriorities, PubGrubPriority}; -pub use crate::pubgrub::report::ResolutionFailureReporter; +pub use crate::pubgrub::report::PubGrubReportFormatter; pub(crate) use crate::pubgrub::version::{PubGrubVersion, MIN_VERSION}; diff --git a/crates/puffin-resolver/src/pubgrub/report.rs b/crates/puffin-resolver/src/pubgrub/report.rs index 14f648916d03..9908e42f1e01 100644 --- a/crates/puffin-resolver/src/pubgrub/report.rs +++ b/crates/puffin-resolver/src/pubgrub/report.rs @@ -1,448 +1,102 @@ -use std::fmt; - use pubgrub::range::Range; -use pubgrub::report::{DerivationTree, Derived, External, Reporter}; +use pubgrub::report::{External, ReportFormatter}; use pubgrub::term::Term; use pubgrub::type_aliases::Map; use super::{PubGrubPackage, PubGrubVersion}; -/// Puffin derivative of [`pubgrub::report::DefaultStringReporter`] for customized display -/// of package resolution errors. -pub struct ResolutionFailureReporter { - /// Number of explanations already with a line reference. - ref_count: usize, - /// Shared nodes that have already been marked with a line reference. - /// The incompatibility ids are the keys, and the line references are the values. - shared_with_ref: Map, - /// Accumulated lines of the report already generated. - lines: Vec, -} - -impl ResolutionFailureReporter { - /// Initialize the reporter. - fn new() -> Self { - Self { - ref_count: 0, - shared_with_ref: Map::default(), - lines: Vec::new(), - } - } - - fn build_recursive(&mut self, derived: &Derived>) { - self.build_recursive_helper(derived); - if let Some(id) = derived.shared_id { - if self.shared_with_ref.get(&id).is_none() { - self.add_line_ref(); - self.shared_with_ref.insert(id, self.ref_count); - } - }; - } - - fn build_recursive_helper(&mut self, current: &Derived>) { - match (&*current.cause1, &*current.cause2) { - (DerivationTree::External(external1), DerivationTree::External(external2)) => { - // Simplest case, we just combine two external incompatibilities. - self.lines.push(Self::explain_both_external( - external1, - external2, - ¤t.terms, - )); - } - (DerivationTree::Derived(derived), DerivationTree::External(external)) => { - // One cause is derived, so we explain this first - // then we add the one-line external part - // and finally conclude with the current incompatibility. - self.report_one_each(derived, external, ¤t.terms); - } - (DerivationTree::External(external), DerivationTree::Derived(derived)) => { - self.report_one_each(derived, external, ¤t.terms); - } - (DerivationTree::Derived(derived1), DerivationTree::Derived(derived2)) => { - // This is the most complex case since both causes are also derived. - match ( - self.line_ref_of(derived1.shared_id), - self.line_ref_of(derived2.shared_id), - ) { - // If both causes already have been referenced (shared_id), - // the explanation simply uses those references. - (Some(ref1), Some(ref2)) => self.lines.push(Self::explain_both_ref( - ref1, - derived1, - ref2, - derived2, - ¤t.terms, - )), - // Otherwise, if one only has a line number reference, - // we recursively call the one without reference and then - // add the one with reference to conclude. - (Some(ref1), None) => { - self.build_recursive(derived2); - self.lines - .push(Self::and_explain_ref(ref1, derived1, ¤t.terms)); - } - (None, Some(ref2)) => { - self.build_recursive(derived1); - self.lines - .push(Self::and_explain_ref(ref2, derived2, ¤t.terms)); - } - // Finally, if no line reference exists yet, - // we call recursively the first one and then, - // - if this was a shared node, it will get a line ref - // and we can simply recall this with the current node. - // - otherwise, we add a line reference to it, - // recursively call on the second node, - // and finally conclude. - (None, None) => { - self.build_recursive(derived1); - if derived1.shared_id.is_some() { - self.lines.push(String::new()); - self.build_recursive(current); - } else { - self.add_line_ref(); - let ref1 = self.ref_count; - self.lines.push(String::new()); - self.build_recursive(derived2); - self.lines - .push(Self::and_explain_ref(ref1, derived1, ¤t.terms)); - } - } - } - } - } - } - - /// Report a derived and an external incompatibility. - /// - /// The result will depend on the fact that the derived incompatibility - /// has already been explained or not. - fn report_one_each( - &mut self, - derived: &Derived>, - external: &External>, - current_terms: &Map>>, - ) { - match self.line_ref_of(derived.shared_id) { - Some(ref_id) => self.lines.push(Self::explain_ref_and_external( - ref_id, - derived, - external, - current_terms, - )), - None => self.report_recurse_one_each(derived, external, current_terms), - } - } - - /// Report one derived (without a line ref yet) and one external. - fn report_recurse_one_each( - &mut self, - derived: &Derived>, - external: &External>, - current_terms: &Map>>, - ) { - match (&*derived.cause1, &*derived.cause2) { - // If the derived cause has itself one external prior cause, - // we can chain the external explanations. - (DerivationTree::Derived(prior_derived), DerivationTree::External(prior_external)) => { - self.build_recursive(prior_derived); - self.lines.push(Self::and_explain_prior_and_external( - prior_external, - external, - current_terms, - )); - } - // If the derived cause has itself one external prior cause, - // we can chain the external explanations. - (DerivationTree::External(prior_external), DerivationTree::Derived(prior_derived)) => { - self.build_recursive(prior_derived); - self.lines.push(Self::and_explain_prior_and_external( - prior_external, - external, - current_terms, - )); - } - _ => { - self.build_recursive(derived); - self.lines - .push(Self::and_explain_external(external, current_terms)); - } - } - } - - // String explanations ##################################################### - - /// Simplest case, we just combine two external incompatibilities. - fn explain_both_external( - external1: &External>, - external2: &External>, - current_terms: &Map>>, - ) -> String { - // TODO: order should be chosen to make it more logical. - format!( - "Because {} and {}, {}.", - PuffinExternal::from_pubgrub(external1.clone()), - PuffinExternal::from_pubgrub(external2.clone()), - Self::string_terms(current_terms) - ) - } - - /// Both causes have already been explained so we use their refs. - fn explain_both_ref( - ref_id1: usize, - derived1: &Derived>, - ref_id2: usize, - derived2: &Derived>, - current_terms: &Map>>, - ) -> String { - // TODO: order should be chosen to make it more logical. - format!( - "Because {} ({}) and {} ({}), {}.", - Self::string_terms(&derived1.terms), - ref_id1, - Self::string_terms(&derived2.terms), - ref_id2, - Self::string_terms(current_terms) - ) - } +#[derive(Debug, Default)] +pub struct PubGrubReportFormatter; - /// One cause is derived (already explained so one-line), - /// the other is a one-line external cause, - /// and finally we conclude with the current incompatibility. - fn explain_ref_and_external( - ref_id: usize, - derived: &Derived>, - external: &External>, - current_terms: &Map>>, - ) -> String { - // TODO: order should be chosen to make it more logical. - format!( - "Because {} ({}) and {}, {}.", - Self::string_terms(&derived.terms), - ref_id, - PuffinExternal::from_pubgrub(external.clone()), - Self::string_terms(current_terms) - ) - } - - /// Add an external cause to the chain of explanations. - fn and_explain_external( - external: &External>, - current_terms: &Map>>, - ) -> String { - format!( - "And because {}, {}.", - PuffinExternal::from_pubgrub(external.clone()), - Self::string_terms(current_terms) - ) - } - - /// Add an already explained incompat to the chain of explanations. - fn and_explain_ref( - ref_id: usize, - derived: &Derived>, - current_terms: &Map>>, - ) -> String { - format!( - "And because {} ({}), {}.", - Self::string_terms(&derived.terms), - ref_id, - Self::string_terms(current_terms) - ) - } - - /// Add an already explained incompat to the chain of explanations. - fn and_explain_prior_and_external( - prior_external: &External>, - external: &External>, - current_terms: &Map>>, - ) -> String { - format!( - "And because {} and {}, {}.", - prior_external, - external, - Self::string_terms(current_terms) - ) - } - - /// Try to print terms of an incompatibility in a human-readable way. - pub fn string_terms(terms: &Map>>) -> String { - let terms_vec: Vec<_> = terms.iter().collect(); - match terms_vec.as_slice() { - [] | [(PubGrubPackage::Root(_), _)] => "version solving failed".into(), - [(package @ PubGrubPackage::Package(..), Term::Positive(range))] => { - format!("{package}{range} is forbidden") - } - [(package @ PubGrubPackage::Package(..), Term::Negative(range))] => { - format!("{package}{range} is mandatory") - } - [(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => { - PuffinExternal::FromDependencyOf( - (*p1).clone(), - r1.clone(), - (*p2).clone(), - r2.clone(), - ) - .to_string() - } - [(p1, Term::Negative(r1)), (p2, Term::Positive(r2))] => { - PuffinExternal::FromDependencyOf( - (*p2).clone(), - r2.clone(), - (*p1).clone(), - r1.clone(), - ) - .to_string() - } - slice => { - let str_terms: Vec<_> = slice.iter().map(|(p, t)| format!("{p} {t}")).collect(); - str_terms.join(", ") + " are incompatible" - } - } - } - - // Helper functions ######################################################## - - fn add_line_ref(&mut self) { - let new_count = self.ref_count + 1; - self.ref_count = new_count; - if let Some(line) = self.lines.last_mut() { - *line = format!("{line} ({new_count})"); - } - } - - fn line_ref_of(&self, shared_id: Option) -> Option { - shared_id.and_then(|id| self.shared_with_ref.get(&id).copied()) - } -} - -impl Reporter> for ResolutionFailureReporter { +impl ReportFormatter> for PubGrubReportFormatter { type Output = String; - fn report( - derivation_tree: &DerivationTree>, + fn format_external( + &self, + external: &External>, ) -> Self::Output { - match derivation_tree { - DerivationTree::External(external) => { - PuffinExternal::from_pubgrub(external.clone()).to_string() - } - DerivationTree::Derived(derived) => { - let mut reporter = Self::new(); - reporter.build_recursive(derived); - reporter.lines.join("\n") - } - } - } -} - -/// Puffin derivative of [`pubgrub::report::External`] for customized display -/// for Puffin internal [`PubGrubPackage`]. -#[allow(clippy::large_enum_variant)] -#[derive(Debug, Clone)] -enum PuffinExternal { - /// Initial incompatibility aiming at picking the root package for the first decision. - NotRoot(PubGrubPackage, PubGrubVersion), - /// There are no versions in the given set for this package. - NoVersions(PubGrubPackage, Range), - /// Dependencies of the package are unavailable for versions in that set. - UnavailableDependencies(PubGrubPackage, Range), - /// Dependencies of the package are unusable for versions in that set. - UnusableDependencies(PubGrubPackage, Range, Option), - /// Incompatibility coming from the dependencies of a given package. - FromDependencyOf( - PubGrubPackage, - Range, - PubGrubPackage, - Range, - ), -} - -impl PuffinExternal { - fn from_pubgrub(external: External>) -> Self { match external { - External::NotRoot(p, v) => PuffinExternal::NotRoot(p, v), - External::NoVersions(p, vs) => PuffinExternal::NoVersions(p, vs), - External::UnavailableDependencies(p, vs) => { - PuffinExternal::UnavailableDependencies(p, vs) - } - External::UnusableDependencies(p, vs, reason) => { - PuffinExternal::UnusableDependencies(p, vs, reason) - } - External::FromDependencyOf(p, vs, p_dep, vs_dep) => { - PuffinExternal::FromDependencyOf(p, vs, p_dep, vs_dep) + External::NotRoot(package, version) => { + format!("we are solving dependencies of {package} {version}") } - } - } -} - -impl fmt::Display for PuffinExternal { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::NotRoot(package, version) => { - write!(f, "we are solving dependencies of {package} {version}") - } - Self::NoVersions(package, set) => { + External::NoVersions(package, set) => { if set == &Range::full() { - write!(f, "there is no available version for {package}") + format!("there is no available version for {package}") } else { - write!( - f, - "there is no version of {package} available matching {set}" - ) + format!("there is no version of {package} available matching {set}") } } - Self::UnavailableDependencies(package, set) => { + External::UnavailableDependencies(package, set) => { if set == &Range::full() { - write!(f, "dependencies of {package} are unavailable") + format!("dependencies of {package} are unavailable") } else { - write!( - f, - "dependencies of {package} at version {set} are unavailable" - ) + format!("dependencies of {package} at version {set} are unavailable") } } - Self::UnusableDependencies(package, set, reason) => { + External::UnusableDependencies(package, set, reason) => { if let Some(reason) = reason { if matches!(package, PubGrubPackage::Root(_)) { - write!(f, "{package} dependencies are unusable: {reason}") + format!("{package} dependencies are unusable: {reason}") } else { if set == &Range::full() { - write!(f, "dependencies of {package} are unusable: {reason}") + format!("dependencies of {package} are unusable: {reason}") } else { - write!(f, "dependencies of {package}{set} are unusable: {reason}",) + format!("dependencies of {package}{set} are unusable: {reason}",) } } } else { if set == &Range::full() { - write!(f, "dependencies of {package} are unusable") + format!("dependencies of {package} are unusable") } else { - write!(f, "dependencies of {package}{set} are unusable") + format!("dependencies of {package}{set} are unusable") } } } - Self::FromDependencyOf(package, package_set, dependency, dependency_set) => { + External::FromDependencyOf(package, package_set, dependency, dependency_set) => { if package_set == &Range::full() && dependency_set == &Range::full() { - write!(f, "{package} depends on {dependency}") + format!("{package} depends on {dependency}") } else if package_set == &Range::full() { - write!(f, "{package} depends on {dependency}{dependency_set}") + 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 - write!(f, "{package} depends on {dependency}") + format!("{package} depends on {dependency}") } else { - write!(f, "{package}{package_set} depends on {dependency}") + format!("{package}{package_set} depends on {dependency}") } } else { if matches!(package, PubGrubPackage::Root(_)) { // Exclude the dummy version for root packages - write!(f, "{package} depends on {dependency}{dependency_set}") + format!("{package} depends on {dependency}{dependency_set}") } else { - write!( - f, - "{package}{package_set} depends on {dependency}{dependency_set}" - ) + format!("{package}{package_set} depends on {dependency}{dependency_set}") } } } } } + + /// Try to print terms of an incompatibility in a human-readable way. + fn format_terms(&self, terms: &Map>>) -> String { + let terms_vec: Vec<_> = terms.iter().collect(); + match terms_vec.as_slice() { + [] | [(PubGrubPackage::Root(_), _)] => "version solving failed".into(), + [(package @ PubGrubPackage::Package(..), Term::Positive(range))] => { + format!("{package}{range} is forbidden") + } + [(package @ PubGrubPackage::Package(..), Term::Negative(range))] => { + format!("{package}{range} is mandatory") + } + [(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => self.format_external( + &External::FromDependencyOf((*p1).clone(), r1.clone(), (*p2).clone(), r2.clone()), + ), + [(p1, Term::Negative(r1)), (p2, Term::Positive(r2))] => self.format_external( + &External::FromDependencyOf((*p2).clone(), r2.clone(), (*p1).clone(), r1.clone()), + ), + slice => { + let str_terms: Vec<_> = slice.iter().map(|(p, t)| format!("{p} {t}")).collect(); + str_terms.join(", ") + " are incompatible" + } + } + } }