diff --git a/src/internal/core.rs b/src/internal/core.rs index 4dc5d447..7b152225 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -116,7 +116,7 @@ impl State { &mut self, package: Id, ) -> Result, IncompDpId)>, NoSolutionError> { - let mut root_causes = SmallVec::default(); + let mut satisfier_causes = SmallVec::default(); self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package); while let Some(current_package) = self.unit_propagation_buffer.pop() { @@ -169,12 +169,11 @@ impl State { } } if let Some(incompat_id) = conflict_id { - let (package_almost, root_cause) = - self.conflict_resolution(incompat_id) - .map_err(|terminal_incompat_id| { - self.build_derivation_tree(terminal_incompat_id) - })?; - root_causes.push((package, root_cause)); + let (package_almost, root_cause) = self + .conflict_resolution(incompat_id, &mut satisfier_causes) + .map_err(|terminal_incompat_id| { + self.build_derivation_tree(terminal_incompat_id) + })?; self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package_almost); // Add to the partial solution with incompat as cause. @@ -190,16 +189,46 @@ impl State { } } // If there are no more changed packages, unit propagation is done. - Ok(root_causes) + Ok(satisfier_causes) } /// Return the root cause or the terminal incompatibility. /// CF + /// + /// Usually by the time we have a conflict `unit_propagation` has done a lot of work. + /// So the actual conflict we find is important, but not particularly actionable. + /// It says something like "the dependency on package X and the dependency on package Y are incompatible". + /// To make it actionable we want to track it back to decisions that made the dependency required. + /// "The decision on B is incompatible with the decision on C, + /// because unit propagation from just those decisions will lead to the conflict about X and Y" + /// is much more actionable, backtrack until one of those decisions can be revisited. + /// To make a practical, we really only need one of the terms to be a decision. + /// We may as well leave the other terms general. Something like + /// "the dependency on the package X is incompatible with the decision on C" tends to work out pretty well. + /// Then if A turns out to also have a dependency on X the resulting root cause is still useful. + /// Of course, this is more heuristics than science. If the output is too general, then `unit_propagation` will + /// handle the confusion by calling us again with the next most specific conflict it comes across. + /// If the output is to specific, then the outer `solver` loop will eventually end up calling us again + /// until all possibilities are enumerated. + /// + /// This function combines incompatibilities with things that make the problem inevitable to end up with a + /// more useful incompatibility. For the correctness of the PubGrub algorithm only the final output is required. + /// By banning the final output, unit propagation will prevent the intermediate steps from occurring again, + /// at least prevent the exact same way. However, the statistics collected for `prioritize`may want + /// to analyze those intermediate steps. For example we might start with "there is no version 1 of Z", + /// and `conflict_resolution` may be able to determine that "that was inevitable when we picked version 1 of X" + /// which was inevitable when picked W and ... and version 1 of B, which was depended on by version 1 of A. + /// Therefore the root cause may simplify all the way down to "we cannot pick version 1 of A". + /// This will prevent us going down this path again. However when we start looking at version 2 of A, + /// and discover that it depends on version 2 of B, we will want to prioritize the chain of intermediate steps + /// to confirm if it has a problem with the same shape. + /// The `satisfier_causes` argument keeps track of these intermediate steps so that the caller can use. #[allow(clippy::type_complexity)] #[cold] fn conflict_resolution( &mut self, incompatibility: IncompDpId, + satisfier_causes: &mut SmallVec<(Id, IncompDpId)>, ) -> Result<(Id, IncompDpId), IncompDpId> { let mut current_incompat_id = incompatibility; let mut current_incompat_changed = false; @@ -223,6 +252,7 @@ impl State { previous_satisfier_level, ); log::info!("backtrack to {:?}", previous_satisfier_level); + satisfier_causes.push((package, current_incompat_id)); return Ok((package, current_incompat_id)); } SatisfierSearch::SameDecisionLevels { satisfier_cause } => { @@ -234,6 +264,7 @@ impl State { ); log::info!("prior cause: {}", prior_cause.display(&self.package_store)); current_incompat_id = self.incompatibility_store.alloc(prior_cause); + satisfier_causes.push((package, current_incompat_id)); current_incompat_changed = true; } } diff --git a/src/lib.rs b/src/lib.rs index 1d906c27..87123373 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -71,7 +71,8 @@ //! and [SemanticVersion] for versions. //! This may be done quite easily by implementing the three following functions. //! ``` -//! # use pubgrub::{DependencyProvider, Dependencies, SemanticVersion, Ranges, DependencyConstraints, Map, PackageResolutionStatistics}; +//! # use pubgrub::{DependencyProvider, Dependencies, SemanticVersion, Ranges, +//! # DependencyConstraints, Map, PackageResolutionStatistics}; //! # use std::error::Error; //! # use std::borrow::Borrow; //! # use std::convert::Infallible; diff --git a/src/provider.rs b/src/provider.rs index aad87175..ec3dd384 100644 --- a/src/provider.rs +++ b/src/provider.rs @@ -2,8 +2,10 @@ use std::cmp::Reverse; use std::collections::BTreeMap; use std::convert::Infallible; -use crate::solver::PackageResolutionStatistics; -use crate::{Dependencies, DependencyConstraints, DependencyProvider, Map, Package, VersionSet}; +use crate::{ + Dependencies, DependencyConstraints, DependencyProvider, Map, Package, + PackageResolutionStatistics, VersionSet, +}; /// A basic implementation of [DependencyProvider]. #[derive(Debug, Clone, Default)] @@ -102,15 +104,15 @@ impl DependencyProvider for OfflineDependencyProvide range: &Self::VS, package_statistics: &PackageResolutionStatistics, ) -> Self::Priority { - ( - package_statistics.conflict_count(), - Reverse( - self.dependencies - .get(package) - .map(|versions| versions.keys().filter(|v| range.contains(v)).count()) - .unwrap_or(0), - ), - ) + let version_count = self + .dependencies + .get(package) + .map(|versions| versions.keys().filter(|v| range.contains(v)).count()) + .unwrap_or(0); + if version_count == 0 { + return (u32::MAX, Reverse(0)); + } + (package_statistics.conflict_count(), Reverse(version_count)) } #[inline] diff --git a/src/solver.rs b/src/solver.rs index 7c4ed0f8..07c7825b 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -126,8 +126,8 @@ pub fn resolve( "unit_propagation: {:?} = '{}'", &next, state.package_store[next] ); - let root_causes = state.unit_propagation(next)?; - for (affected, incompat) in root_causes { + let satisfier_causes = state.unit_propagation(next)?; + for (affected, incompat) in satisfier_causes { conflict_tracker .entry(affected) .or_default()