From ecb35f48f40ba7aa7cf85692cab80f19e0575740 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Thu, 30 Nov 2023 16:46:22 -0600 Subject: [PATCH] feat: add report formatter concept for customization of report output (#158) * Add report formatter concept for customization of report output * Add default implementation * Use `format_external` in `format_terms` * Add previous example * Use new formatter * Add docs to trait method * Clippy * Remove outdated comment in example * Use generic --- examples/unsat_root_message_no_version.rs | 146 +++++++++++++ src/internal/incompatibility.rs | 6 +- src/report.rs | 245 +++++++++++++++------- 3 files changed, 325 insertions(+), 72 deletions(-) create mode 100644 examples/unsat_root_message_no_version.rs diff --git a/examples/unsat_root_message_no_version.rs b/examples/unsat_root_message_no_version.rs new file mode 100644 index 00000000..7c45b9ee --- /dev/null +++ b/examples/unsat_root_message_no_version.rs @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: MPL-2.0 + +use pubgrub::error::PubGrubError; +use pubgrub::range::Range; +use pubgrub::report::Reporter; +use pubgrub::solver::{resolve, OfflineDependencyProvider}; +use pubgrub::version::SemanticVersion; + +use pubgrub::report::{DefaultStringReporter, External, ReportFormatter}; +use pubgrub::term::Term; +use pubgrub::type_aliases::Map; +use std::fmt::{self, Display}; + +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub enum Package { + Root, + Package(String), +} + +impl Display for Package { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Package::Root => write!(f, "root"), + Package::Package(name) => write!(f, "{}", name), + } + } +} + +#[derive(Debug, Default)] +struct CustomReportFormatter; + +impl ReportFormatter> for CustomReportFormatter { + type Output = String; + + fn format_terms(&self, terms: &Map>>) -> String { + let terms_vec: Vec<_> = terms.iter().collect(); + match terms_vec.as_slice() { + [] => "version solving failed".into(), + [(package @ Package::Root, Term::Positive(_))] => { + format!("{package} is forbidden") + } + [(package @ Package::Root, Term::Negative(_))] => { + format!("{package} is mandatory") + } + [(package @ Package::Package(_), Term::Positive(range))] => { + format!("{package} {range} is forbidden") + } + [(package @ Package::Package(_), Term::Negative(range))] => { + format!("{package} {range} is mandatory") + } + [(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => { + External::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() + } + slice => { + let str_terms: Vec<_> = slice.iter().map(|(p, t)| format!("{p} {t}")).collect(); + str_terms.join(", ") + " are incompatible" + } + } + } + + fn format_external(&self, external: &External>) -> String { + match external { + External::NotRoot(package, version) => { + format!("we are solving dependencies of {package} {version}") + } + External::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) => { + 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) => { + if package_set == &Range::full() && dependency_set == &Range::full() { + format!("{package} depends on {dependency}") + } else if package_set == &Range::full() { + format!("{package} depends on {dependency} {dependency_set}") + } else if dependency_set == &Range::full() { + if matches!(package, Package::Root) { + // Exclude the dummy version for root packages + format!("{package} depends on {dependency}") + } else { + format!("{package} {package_set} depends on {dependency}") + } + } else { + if matches!(package, Package::Root) { + // Exclude the dummy version for root packages + format!("{package} depends on {dependency} {dependency_set}") + } else { + format!("{package} {package_set} depends on {dependency} {dependency_set}") + } + } + } + } + } +} + +fn main() { + let mut dependency_provider = + OfflineDependencyProvider::>::new(); + // Define the root package with a dependency on a package we do not provide + dependency_provider.add_dependencies( + Package::Root, + (0, 0, 0), + vec![( + Package::Package("foo".to_string()), + Range::singleton((1, 0, 0)), + )], + ); + + // Run the algorithm + match resolve(&dependency_provider, Package::Root, (0, 0, 0)) { + Ok(sol) => println!("{:?}", sol), + Err(PubGrubError::NoSolution(derivation_tree)) => { + eprintln!("No solution.\n"); + + eprintln!("### Default report:"); + eprintln!("```"); + eprintln!("{}", DefaultStringReporter::report(&derivation_tree)); + eprintln!("```\n"); + + eprintln!("### Report with custom formatter:"); + eprintln!("```"); + eprintln!( + "{}", + DefaultStringReporter::report_with_formatter( + &derivation_tree, + &CustomReportFormatter + ) + ); + eprintln!("```"); + std::process::exit(1); + } + Err(err) => panic!("{:?}", err), + }; +} diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index b56a3c44..49037e79 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -9,7 +9,9 @@ use std::fmt; use crate::internal::arena::{Arena, Id}; use crate::internal::small_map::SmallMap; use crate::package::Package; -use crate::report::{DefaultStringReporter, DerivationTree, Derived, External}; +use crate::report::{ + DefaultStringReportFormatter, DerivationTree, Derived, External, ReportFormatter, +}; use crate::term::{self, Term}; use crate::version_set::VersionSet; @@ -251,7 +253,7 @@ impl fmt::Display for Incompatibility { write!( f, "{}", - DefaultStringReporter::string_terms(&self.package_terms.as_map()) + DefaultStringReportFormatter.format_terms(&self.package_terms.as_map()) ) } } diff --git a/src/report.rs b/src/report.rs index ff0b2d3f..af423d40 100644 --- a/src/report.rs +++ b/src/report.rs @@ -17,8 +17,15 @@ pub trait Reporter { type Output; /// Generate a report from the derivation tree - /// describing the resolution failure. + /// describing the resolution failure using the default formatter. fn report(derivation_tree: &DerivationTree) -> Self::Output; + + /// Generate a report from the derivation tree + /// describing the resolution failure using a custom formatter. + fn report_with_formatter( + derivation_tree: &DerivationTree, + formatter: &impl ReportFormatter, + ) -> Self::Output; } /// Derivation tree resulting in the impossibility @@ -173,6 +180,50 @@ impl fmt::Display for External { } } +/// Trait for formatting outputs in the reporter. +pub trait ReportFormatter { + /// Output type of the report. + type Output; + + /// Format an [External] incompatibility. + fn format_external(&self, external: &External) -> Self::Output; + + /// Format terms of an incompatibility. + fn format_terms(&self, terms: &Map>) -> Self::Output; +} + +/// Default formatter for the default reporter. +#[derive(Default, Debug)] +pub struct DefaultStringReportFormatter; + +impl ReportFormatter for DefaultStringReportFormatter { + type Output = String; + + fn format_external(&self, external: &External) -> String { + external.to_string() + } + + fn format_terms(&self, terms: &Map>) -> Self::Output { + let terms_vec: Vec<_> = terms.iter().collect(); + match terms_vec.as_slice() { + [] => "version solving failed".into(), + // 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())) + } + slice => { + let str_terms: Vec<_> = slice.iter().map(|(p, t)| format!("{} {}", p, t)).collect(); + str_terms.join(", ") + " are incompatible" + } + } + } +} + /// Default reporter able to generate an explanation as a [String]. pub struct DefaultStringReporter { /// Number of explanations already with a line reference. @@ -194,8 +245,12 @@ impl DefaultStringReporter { } } - fn build_recursive(&mut self, derived: &Derived) { - self.build_recursive_helper(derived); + fn build_recursive>( + &mut self, + derived: &Derived, + formatter: &F, + ) { + self.build_recursive_helper(derived, formatter); if let Some(id) = derived.shared_id { if self.shared_with_ref.get(&id).is_none() { self.add_line_ref(); @@ -204,7 +259,15 @@ impl DefaultStringReporter { }; } - fn build_recursive_helper(&mut self, current: &Derived) { + fn build_recursive_helper< + P: Package, + VS: VersionSet, + F: ReportFormatter, + >( + &mut self, + current: &Derived, + formatter: &F, + ) { match (current.cause1.deref(), current.cause2.deref()) { (DerivationTree::External(external1), DerivationTree::External(external2)) => { // Simplest case, we just combine two external incompatibilities. @@ -212,16 +275,17 @@ impl DefaultStringReporter { 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); + self.report_one_each(derived, external, ¤t.terms, formatter); } (DerivationTree::External(external), DerivationTree::Derived(derived)) => { - self.report_one_each(derived, external, ¤t.terms); + 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. @@ -237,19 +301,28 @@ impl DefaultStringReporter { ref2, derived2, ¤t.terms, + formatter, )), // 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)); + self.build_recursive(derived2, formatter); + self.lines.push(Self::and_explain_ref( + ref1, + derived1, + ¤t.terms, + formatter, + )); } (None, Some(ref2)) => { - self.build_recursive(derived1); - self.lines - .push(Self::and_explain_ref(ref2, derived2, ¤t.terms)); + self.build_recursive(derived1, formatter); + self.lines.push(Self::and_explain_ref( + ref2, + derived2, + ¤t.terms, + formatter, + )); } // Finally, if no line reference exists yet, // we call recursively the first one and then, @@ -259,17 +332,21 @@ impl DefaultStringReporter { // recursively call on the second node, // and finally conclude. (None, None) => { - self.build_recursive(derived1); + self.build_recursive(derived1, formatter); if derived1.shared_id.is_some() { self.lines.push("".into()); - self.build_recursive(current); + self.build_recursive(current, formatter); } else { self.add_line_ref(); let ref1 = self.ref_count; self.lines.push("".into()); - self.build_recursive(derived2); - self.lines - .push(Self::and_explain_ref(ref1, derived1, ¤t.terms)); + self.build_recursive(derived2, formatter); + self.lines.push(Self::and_explain_ref( + ref1, + derived1, + ¤t.terms, + formatter, + )); } } } @@ -281,11 +358,12 @@ impl DefaultStringReporter { /// /// The result will depend on the fact that the derived incompatibility /// has already been explained or not. - fn report_one_each( + fn report_one_each>( &mut self, derived: &Derived, external: &External, current_terms: &Map>, + formatter: &F, ) { match self.line_ref_of(derived.shared_id) { Some(ref_id) => self.lines.push(Self::explain_ref_and_external( @@ -293,43 +371,54 @@ impl DefaultStringReporter { derived, external, current_terms, + formatter, )), - None => self.report_recurse_one_each(derived, external, current_terms), + None => self.report_recurse_one_each(derived, external, current_terms, formatter), } } /// Report one derived (without a line ref yet) and one external. - fn report_recurse_one_each( + fn report_recurse_one_each< + P: Package, + VS: VersionSet, + F: ReportFormatter, + >( &mut self, derived: &Derived, external: &External, current_terms: &Map>, + formatter: &F, ) { match (derived.cause1.deref(), derived.cause2.deref()) { // 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.build_recursive(prior_derived, formatter); self.lines.push(Self::and_explain_prior_and_external( prior_external, external, current_terms, + formatter, )); } // 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.build_recursive(prior_derived, formatter); self.lines.push(Self::and_explain_prior_and_external( prior_external, external, current_terms, + formatter, )); } _ => { - self.build_recursive(derived); - self.lines - .push(Self::and_explain_external(external, current_terms)); + self.build_recursive(derived, formatter); + self.lines.push(Self::and_explain_external( + external, + current_terms, + formatter, + )); } } } @@ -337,119 +426,120 @@ impl DefaultStringReporter { // String explanations ##################################################### /// Simplest case, we just combine two external incompatibilities. - fn explain_both_external( + fn explain_both_external< + P: Package, + VS: VersionSet, + F: ReportFormatter, + >( external1: &External, external2: &External, current_terms: &Map>, + formatter: &F, ) -> String { // TODO: order should be chosen to make it more logical. format!( "Because {} and {}, {}.", - external1, - external2, - Self::string_terms(current_terms) + formatter.format_external(external1), + formatter.format_external(external2), + formatter.format_terms(current_terms) ) } /// Both causes have already been explained so we use their refs. - fn explain_both_ref( + fn explain_both_ref>( ref_id1: usize, derived1: &Derived, ref_id2: usize, derived2: &Derived, current_terms: &Map>, + formatter: &F, ) -> String { // TODO: order should be chosen to make it more logical. format!( "Because {} ({}) and {} ({}), {}.", - Self::string_terms(&derived1.terms), + formatter.format_terms(&derived1.terms), ref_id1, - Self::string_terms(&derived2.terms), + formatter.format_terms(&derived2.terms), ref_id2, - Self::string_terms(current_terms) + formatter.format_terms(current_terms) ) } /// 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( + fn explain_ref_and_external< + P: Package, + VS: VersionSet, + F: ReportFormatter, + >( ref_id: usize, derived: &Derived, external: &External, current_terms: &Map>, + formatter: &F, ) -> String { // TODO: order should be chosen to make it more logical. format!( "Because {} ({}) and {}, {}.", - Self::string_terms(&derived.terms), + formatter.format_terms(&derived.terms), ref_id, - external, - Self::string_terms(current_terms) + formatter.format_external(external), + formatter.format_terms(current_terms) ) } /// Add an external cause to the chain of explanations. - fn and_explain_external( + fn and_explain_external< + P: Package, + VS: VersionSet, + F: ReportFormatter, + >( external: &External, current_terms: &Map>, + formatter: &F, ) -> String { format!( "And because {}, {}.", - external, - Self::string_terms(current_terms) + formatter.format_external(external), + formatter.format_terms(current_terms) ) } /// Add an already explained incompat to the chain of explanations. - fn and_explain_ref( + fn and_explain_ref>( ref_id: usize, derived: &Derived, current_terms: &Map>, + formatter: &F, ) -> String { format!( "And because {} ({}), {}.", - Self::string_terms(&derived.terms), + formatter.format_terms(&derived.terms), ref_id, - Self::string_terms(current_terms) + formatter.format_terms(current_terms) ) } /// Add an already explained incompat to the chain of explanations. - fn and_explain_prior_and_external( + fn and_explain_prior_and_external< + P: Package, + VS: VersionSet, + F: ReportFormatter, + >( prior_external: &External, external: &External, current_terms: &Map>, + formatter: &F, ) -> String { format!( "And because {} and {}, {}.", - prior_external, - external, - Self::string_terms(current_terms) + formatter.format_external(prior_external), + formatter.format_external(external), + formatter.format_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() { - [] => "version solving failed".into(), - // 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))] => { - External::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() - } - 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) { @@ -469,11 +559,26 @@ impl Reporter for DefaultStringReporter { type Output = String; 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") + } + } + } + + fn report_with_formatter( + derivation_tree: &DerivationTree, + formatter: &impl ReportFormatter, + ) -> Self::Output { match derivation_tree { - DerivationTree::External(external) => external.to_string(), + DerivationTree::External(external) => formatter.format_external(external), DerivationTree::Derived(derived) => { let mut reporter = Self::new(); - reporter.build_recursive(derived); + reporter.build_recursive(derived, formatter); reporter.lines.join("\n") } }