diff --git a/examples/unsat_root_message_no_version.rs b/examples/unsat_root_message_no_version.rs index ac258e34..3a90c4d2 100644 --- a/examples/unsat_root_message_no_version.rs +++ b/examples/unsat_root_message_no_version.rs @@ -2,11 +2,11 @@ use pubgrub::error::PubGrubError; use pubgrub::range::Range; -use pubgrub::report::Reporter; +use pubgrub::report::{DerivationTree, Reporter}; use pubgrub::solver::{resolve, OfflineDependencyProvider}; use pubgrub::version::SemanticVersion; -use pubgrub::report::{DefaultStringReporter, External, ReportFormatter}; +use pubgrub::report::{DefaultStringReporter, ReportFormatter}; use pubgrub::term::Term; use pubgrub::type_aliases::Map; use std::fmt::{self, Display}; @@ -49,10 +49,10 @@ impl ReportFormatter> for CustomReportFormatter format!("{package} {range} is mandatory") } [(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => { - External::FromDependencyOf(p1, r1.clone(), p2, r2.clone()).to_string() + DerivationTree::FromDependencyOf(p1, r1.clone(), p2, r2.clone()).to_string() } [(p1, Term::Negative(r1)), (p2, Term::Positive(r2))] => { - External::FromDependencyOf(p2, r2.clone(), p1, r1.clone()).to_string() + DerivationTree::FromDependencyOf(p2, r2.clone(), p1, r1.clone()).to_string() } slice => { let str_terms: Vec<_> = slice.iter().map(|(p, t)| format!("{p} {t}")).collect(); @@ -61,26 +61,29 @@ impl ReportFormatter> for CustomReportFormatter } } - fn format_external(&self, external: &External>) -> String { + fn format_external( + &self, + external: &DerivationTree>, + ) -> String { match external { - External::NotRoot(package, version) => { + DerivationTree::NotRoot(package, version) => { format!("we are solving dependencies of {package} {version}") } - External::NoVersions(package, set) => { + DerivationTree::NoVersions(package, set) => { if set == &Range::full() { format!("there is no available version for {package}") } else { format!("there is no version of {package} in {set}") } } - External::UnavailableDependencies(package, set) => { + DerivationTree::UnavailableDependencies(package, set) => { if set == &Range::full() { format!("dependencies of {package} are unavailable") } else { format!("dependencies of {package} at version {set} are unavailable") } } - External::FromDependencyOf(package, package_set, dependency, dependency_set) => { + DerivationTree::FromDependencyOf(package, package_set, dependency, dependency_set) => { if package_set == &Range::full() && dependency_set == &Range::full() { format!("{package} depends on {dependency}") } else if package_set == &Range::full() { @@ -99,6 +102,9 @@ impl ReportFormatter> for CustomReportFormatter format!("{package} {package_set} depends on {dependency} {dependency_set}") } } + DerivationTree::Derived(_) => { + panic!("Got derived, while only external can be formatted"); + } } } } diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 864d6822..abc97947 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -9,9 +9,7 @@ use std::sync::Arc; use crate::internal::arena::{Arena, Id}; use crate::internal::small_map::SmallMap; use crate::package::Package; -use crate::report::{ - DefaultStringReportFormatter, DerivationTree, Derived, External, ReportFormatter, -}; +use crate::report::{DefaultStringReportFormatter, DerivationTree, Derived, ReportFormatter}; use crate::term::{self, Term}; use crate::type_aliases::{Map, Set}; use crate::version_set::VersionSet; @@ -249,18 +247,14 @@ impl Incompatibility { }; DerivationTree::Derived(derived) } - Kind::NotRoot(package, version) => { - DerivationTree::External(External::NotRoot(package, version)) - } - Kind::NoVersions(package, set) => { - DerivationTree::External(External::NoVersions(package, set)) - } + Kind::NotRoot(package, version) => DerivationTree::NotRoot(package, version), + Kind::NoVersions(package, set) => DerivationTree::NoVersions(package, set), Kind::UnavailableDependencies(package, set) => { - DerivationTree::External(External::UnavailableDependencies(package, set)) + DerivationTree::UnavailableDependencies(package, set) + } + Kind::FromDependencyOf(package, set, dep_package, dep_set) => { + DerivationTree::FromDependencyOf(package, set, dep_package, dep_set) } - Kind::FromDependencyOf(package, set, dep_package, dep_set) => DerivationTree::External( - External::FromDependencyOf(package, set, dep_package, dep_set), - ), } } } diff --git a/src/report.rs b/src/report.rs index 9f354183..322a4dce 100644 --- a/src/report.rs +++ b/src/report.rs @@ -33,16 +33,8 @@ pub trait Reporter { /// to solve the dependencies of our root package. #[derive(Debug, Clone)] pub enum DerivationTree { - /// External incompatibility. - External(External), /// Incompatibility derived from two others. Derived(Derived), -} - -/// Incompatibilities that are not derived from others, -/// they have their own reason. -#[derive(Debug, Clone)] -pub enum External { /// Initial incompatibility aiming at picking the root package for the first decision. NotRoot(P, VS::V), /// There are no versions in the given set for this package. @@ -80,31 +72,28 @@ impl DerivationTree { /// was not run in some kind of offline mode that may not /// have access to all versions existing. pub fn collapse_no_versions(&mut self) { - match self { - DerivationTree::External(_) => {} - DerivationTree::Derived(derived) => { - match ( - Arc::make_mut(&mut derived.cause1), - Arc::make_mut(&mut derived.cause2), - ) { - (DerivationTree::External(External::NoVersions(p, r)), ref mut cause2) => { - cause2.collapse_no_versions(); - *self = cause2 - .clone() - .merge_no_versions(p.to_owned(), r.to_owned()) - .unwrap_or_else(|| self.to_owned()); - } - (ref mut cause1, DerivationTree::External(External::NoVersions(p, r))) => { - cause1.collapse_no_versions(); - *self = cause1 - .clone() - .merge_no_versions(p.to_owned(), r.to_owned()) - .unwrap_or_else(|| self.to_owned()); - } - _ => { - Arc::make_mut(&mut derived.cause1).collapse_no_versions(); - Arc::make_mut(&mut derived.cause2).collapse_no_versions(); - } + if let DerivationTree::Derived(derived) = self { + match ( + Arc::make_mut(&mut derived.cause1), + Arc::make_mut(&mut derived.cause2), + ) { + (DerivationTree::NoVersions(p, r), ref mut cause2) => { + cause2.collapse_no_versions(); + *self = cause2 + .clone() + .merge_no_versions(p.to_owned(), r.to_owned()) + .unwrap_or_else(|| self.to_owned()); + } + (ref mut cause1, DerivationTree::NoVersions(p, r)) => { + cause1.collapse_no_versions(); + *self = cause1 + .clone() + .merge_no_versions(p.to_owned(), r.to_owned()) + .unwrap_or_else(|| self.to_owned()); + } + _ => { + Arc::make_mut(&mut derived.cause1).collapse_no_versions(); + Arc::make_mut(&mut derived.cause2).collapse_no_versions(); } } } @@ -115,37 +104,27 @@ impl DerivationTree { // TODO: take care of the Derived case. // Once done, we can remove the Option. DerivationTree::Derived(_) => Some(self), - DerivationTree::External(External::NotRoot(_, _)) => { + DerivationTree::NotRoot(_, _) => { panic!("How did we end up with a NoVersions merged with a NotRoot?") } - DerivationTree::External(External::NoVersions(_, r)) => Some(DerivationTree::External( - External::NoVersions(package, set.union(&r)), - )), - DerivationTree::External(External::UnavailableDependencies(_, r)) => Some( - DerivationTree::External(External::UnavailableDependencies(package, set.union(&r))), + DerivationTree::NoVersions(_, r) => { + Some(DerivationTree::NoVersions(package, set.union(&r))) + } + DerivationTree::UnavailableDependencies(_, r) => Some( + DerivationTree::UnavailableDependencies(package, set.union(&r)), ), - DerivationTree::External(External::FromDependencyOf(p1, r1, p2, r2)) => { + DerivationTree::FromDependencyOf(p1, r1, p2, r2) => { if p1 == package { - Some(DerivationTree::External(External::FromDependencyOf( - p1, - r1.union(&set), - p2, - r2, - ))) + Some(DerivationTree::FromDependencyOf(p1, r1.union(&set), p2, r2)) } else { - Some(DerivationTree::External(External::FromDependencyOf( - p1, - r1, - p2, - r2.union(&set), - ))) + Some(DerivationTree::FromDependencyOf(p1, r1, p2, r2.union(&set))) } } } } } -impl fmt::Display for External { +impl fmt::Display for DerivationTree { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::NotRoot(package, version) => { @@ -180,6 +159,9 @@ impl fmt::Display for External { write!(f, "{} {} depends on {} {}", p, set_p, dep, set_dep) } } + Self::Derived(_) => { + panic!("Got derived, while only external can be formatted"); + } } } } @@ -190,7 +172,7 @@ pub trait ReportFormatter { type Output; /// Format an [External] incompatibility. - fn format_external(&self, external: &External) -> Self::Output; + fn format_external(&self, external: &DerivationTree) -> Self::Output; /// Format terms of an incompatibility. fn format_terms(&self, terms: &Map>) -> Self::Output; @@ -203,7 +185,7 @@ pub struct DefaultStringReportFormatter; impl ReportFormatter for DefaultStringReportFormatter { type Output = String; - fn format_external(&self, external: &External) -> String { + fn format_external(&self, external: &DerivationTree) -> String { external.to_string() } @@ -214,12 +196,12 @@ impl ReportFormatter for DefaultStringReportF // TODO: special case when that unique package is root. [(package, Term::Positive(range))] => format!("{} {} is forbidden", package, range), [(package, Term::Negative(range))] => format!("{} {} is mandatory", package, range), - [(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => { - self.format_external(&External::FromDependencyOf(p1, r1.clone(), p2, r2.clone())) - } - [(p1, Term::Negative(r1)), (p2, Term::Positive(r2))] => { - self.format_external(&External::FromDependencyOf(p2, r2.clone(), p1, r1.clone())) - } + [(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => self.format_external( + &DerivationTree::FromDependencyOf(p1, r1.clone(), p2, r2.clone()), + ), + [(p1, Term::Negative(r1)), (p2, Term::Positive(r2))] => self.format_external( + &DerivationTree::FromDependencyOf(p2, r2.clone(), p1, r1.clone()), + ), slice => { let str_terms: Vec<_> = slice.iter().map(|(p, t)| format!("{} {}", p, t)).collect(); str_terms.join(", ") + " are incompatible" @@ -274,24 +256,6 @@ impl DefaultStringReporter { formatter: &F, ) { match (current.cause1.deref(), current.cause2.deref()) { - (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, - formatter, - )); - } - (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, formatter); - } - (DerivationTree::External(external), DerivationTree::Derived(derived)) => { - self.report_one_each(derived, external, ¤t.terms, formatter); - } (DerivationTree::Derived(derived1), DerivationTree::Derived(derived2)) => { // This is the most complex case since both causes are also derived. match ( @@ -356,6 +320,24 @@ impl DefaultStringReporter { } } } + (DerivationTree::Derived(derived), 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, formatter); + } + (external, DerivationTree::Derived(derived)) => { + self.report_one_each(derived, external, ¤t.terms, formatter); + } + (external1, external2) => { + // Simplest case, we just combine two external incompatibilities. + self.lines.push(Self::explain_both_external( + external1, + external2, + ¤t.terms, + formatter, + )); + } } } @@ -366,7 +348,7 @@ impl DefaultStringReporter { fn report_one_each>( &mut self, derived: &Derived, - external: &External, + external: &DerivationTree, current_terms: &Map>, formatter: &F, ) { @@ -390,14 +372,22 @@ impl DefaultStringReporter { >( &mut self, derived: &Derived, - external: &External, + external: &DerivationTree, current_terms: &Map>, formatter: &F, ) { match (derived.cause1.deref(), derived.cause2.deref()) { + (DerivationTree::Derived(_), DerivationTree::Derived(_)) => { + self.build_recursive(derived, formatter); + self.lines.push(Self::and_explain_external( + external, + current_terms, + formatter, + )); + } // If the derived cause has itself one external prior cause, // we can chain the external explanations. - (DerivationTree::Derived(prior_derived), DerivationTree::External(prior_external)) => { + (DerivationTree::Derived(prior_derived), prior_external) => { self.build_recursive(prior_derived, formatter); self.lines.push(Self::and_explain_prior_and_external( prior_external, @@ -408,7 +398,7 @@ impl DefaultStringReporter { } // If the derived cause has itself one external prior cause, // we can chain the external explanations. - (DerivationTree::External(prior_external), DerivationTree::Derived(prior_derived)) => { + (prior_external, DerivationTree::Derived(prior_derived)) => { self.build_recursive(prior_derived, formatter); self.lines.push(Self::and_explain_prior_and_external( prior_external, @@ -436,8 +426,8 @@ impl DefaultStringReporter { VS: VersionSet, F: ReportFormatter, >( - external1: &External, - external2: &External, + external1: &DerivationTree, + external2: &DerivationTree, current_terms: &Map>, formatter: &F, ) -> String { @@ -480,7 +470,7 @@ impl DefaultStringReporter { >( ref_id: usize, derived: &Derived, - external: &External, + external: &DerivationTree, current_terms: &Map>, formatter: &F, ) -> String { @@ -500,7 +490,7 @@ impl DefaultStringReporter { VS: VersionSet, F: ReportFormatter, >( - external: &External, + external: &DerivationTree, current_terms: &Map>, formatter: &F, ) -> String { @@ -532,8 +522,8 @@ impl DefaultStringReporter { VS: VersionSet, F: ReportFormatter, >( - prior_external: &External, - external: &External, + prior_external: &DerivationTree, + external: &DerivationTree, current_terms: &Map>, formatter: &F, ) -> String { @@ -566,12 +556,12 @@ impl Reporter for DefaultStringReporter { fn report(derivation_tree: &DerivationTree) -> Self::Output { let formatter = DefaultStringReportFormatter; match derivation_tree { - DerivationTree::External(external) => formatter.format_external(external), DerivationTree::Derived(derived) => { let mut reporter = Self::new(); reporter.build_recursive(derived, &formatter); reporter.lines.join("\n") } + external => formatter.format_external(external), } } @@ -580,12 +570,12 @@ impl Reporter for DefaultStringReporter { formatter: &impl ReportFormatter, ) -> Self::Output { match derivation_tree { - DerivationTree::External(external) => formatter.format_external(external), DerivationTree::Derived(derived) => { let mut reporter = Self::new(); reporter.build_recursive(derived, formatter); reporter.lines.join("\n") } + external => formatter.format_external(external), } } } diff --git a/tests/proptest.rs b/tests/proptest.rs index e35cd35a..ba5c42fd 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -8,7 +8,7 @@ use std::convert::Infallible; use pubgrub::error::PubGrubError; use pubgrub::package::Package; use pubgrub::range::Range; -use pubgrub::report::{DefaultStringReporter, DerivationTree, External, Reporter}; +use pubgrub::report::{DefaultStringReporter, DerivationTree, Reporter}; use pubgrub::solver::{resolve, Dependencies, DependencyProvider, OfflineDependencyProvider}; use pubgrub::type_aliases::SelectedDependencies; use pubgrub::version::SemanticVersion; @@ -373,7 +373,7 @@ fn errors_the_same_with_only_report_dependencies( tree: &DerivationTree, ) { match tree { - DerivationTree::External(External::FromDependencyOf(n1, vs1, n2, _)) => { + DerivationTree::FromDependencyOf(n1, vs1, n2, _) => { to_retain.push((n1.clone(), vs1.clone(), n2.clone())); } DerivationTree::Derived(d) => {