Remove double-proxy nodes in error reporting

astral-sh · Aug 2, 2024 · d4d8869 · d4d8869
1 parent 4096bce
commit d4d8869
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 45 deletions.
diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs
@@ -2,7 +2,7 @@ use std::collections::{BTreeMap, BTreeSet};
 use std::fmt::Formatter;
 use std::sync::Arc;
 
-use pubgrub::{DefaultStringReporter, DerivationTree, External, Range, Reporter};
+use pubgrub::{DefaultStringReporter, DerivationTree, Derived, External, Range, Reporter};
 use rustc_hash::FxHashMap;
 
 use distribution_types::{BuiltDist, IndexLocations, InstalledDist, SourceDist};
@@ -13,9 +13,7 @@ use uv_normalize::PackageName;
 use crate::candidate_selector::CandidateSelector;
 use crate::dependency_provider::UvDependencyProvider;
 use crate::fork_urls::ForkUrls;
-use crate::pubgrub::{
-    PubGrubPackage, PubGrubPackageInner, PubGrubReportFormatter, PubGrubSpecifierError,
-};
+use crate::pubgrub::{PubGrubPackage, PubGrubReportFormatter, PubGrubSpecifierError};
 use crate::python_requirement::PythonRequirement;
 use crate::resolver::{IncompletePackage, ResolverMarkers, UnavailablePackage, UnavailableReason};
 
@@ -166,48 +164,49 @@ impl NoSolutionError {
     /// Given a [`DerivationTree`], collapse any [`External::FromDependencyOf`] incompatibilities
     /// wrap an [`PubGrubPackageInner::Extra`] package.
     pub(crate) fn collapse_proxies(
-        derivation_tree: &mut DerivationTree<PubGrubPackage, Range<Version>, UnavailableReason>,
-    ) {
-        match derivation_tree {
-            DerivationTree::External(_) => {}
-            DerivationTree::Derived(derived) => {
-                match (
-                    Arc::make_mut(&mut derived.cause1),
-                    Arc::make_mut(&mut derived.cause2),
-                ) {
-                    (
-                        DerivationTree::External(External::FromDependencyOf(package, ..)),
-                        ref mut cause,
-                    ) if matches!(
-                        &**package,
-                        PubGrubPackageInner::Extra { .. }
-                            | PubGrubPackageInner::Marker { .. }
-                            | PubGrubPackageInner::Dev { .. }
-                    ) =>
-                    {
-                        Self::collapse_proxies(cause);
-                        *derivation_tree = cause.clone();
-                    }
-                    (
-                        ref mut cause,
-                        DerivationTree::External(External::FromDependencyOf(package, ..)),
-                    ) if matches!(
-                        &**package,
-                        PubGrubPackageInner::Extra { .. }
-                            | PubGrubPackageInner::Marker { .. }
-                            | PubGrubPackageInner::Dev { .. }
-                    ) =>
-                    {
-                        Self::collapse_proxies(cause);
-                        *derivation_tree = cause.clone();
-                    }
-                    _ => {
-                        Self::collapse_proxies(Arc::make_mut(&mut derived.cause1));
-                        Self::collapse_proxies(Arc::make_mut(&mut derived.cause2));
+        derivation_tree: DerivationTree<PubGrubPackage, Range<Version>, UnavailableReason>,
+    ) -> DerivationTree<PubGrubPackage, Range<Version>, UnavailableReason> {
+        fn collapse(
+            derivation_tree: DerivationTree<PubGrubPackage, Range<Version>, UnavailableReason>,
+        ) -> Option<DerivationTree<PubGrubPackage, Range<Version>, UnavailableReason>> {
+            match derivation_tree {
+                DerivationTree::Derived(derived) => {
+                    match (&*derived.cause1, &*derived.cause2) {
+                        (
+                            DerivationTree::External(External::FromDependencyOf(package1, ..)),
+                            DerivationTree::External(External::FromDependencyOf(package2, ..)),
+                        ) if package1.is_proxy() && package2.is_proxy() => None,
+                        (
+                            DerivationTree::External(External::FromDependencyOf(package, ..)),
+                            cause,
+                        ) if package.is_proxy() => collapse(cause.clone()),
+                        (
+                            cause,
+                            DerivationTree::External(External::FromDependencyOf(package, ..)),
+                        ) if package.is_proxy() => collapse(cause.clone()),
+                        (cause1, cause2) => {
+                            let cause1 = collapse(cause1.clone());
+                            let cause2 = collapse(cause2.clone());
+                            match (cause1, cause2) {
+                                (Some(cause1), Some(cause2)) => {
+                                    Some(DerivationTree::Derived(Derived {
+                                        cause1: Arc::new(cause1),
+                                        cause2: Arc::new(cause2),
+                                        ..derived
+                                    }))
+                                }
+                                (Some(cause), None) | (None, Some(cause)) => Some(cause),
+                                _ => None,
+                            }
+                        }
                     }
                 }
+                DerivationTree::External(_) => Some(derivation_tree),
             }
         }
+
+        collapse(derivation_tree)
+            .expect("derivation tree should contain at least one external term")
     }
 }
 

diff --git a/crates/uv-resolver/src/pubgrub/package.rs b/crates/uv-resolver/src/pubgrub/package.rs
@@ -159,6 +159,16 @@ impl PubGrubPackage {
             PubGrubPackageInner::Marker { marker, .. } => Some(marker),
         }
     }
+
+    /// Returns `true` if this PubGrub package is a proxy package.
+    pub fn is_proxy(&self) -> bool {
+        matches!(
+            &**self,
+            PubGrubPackageInner::Extra { .. }
+                | PubGrubPackageInner::Dev { .. }
+                | PubGrubPackageInner::Marker { .. }
+        )
+    }
 }
 
 #[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)]

diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs
@@ -1905,7 +1905,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
         visited: &FxHashSet<PackageName>,
         index_locations: &IndexLocations,
     ) -> ResolveError {
-        NoSolutionError::collapse_proxies(&mut err);
+        err = NoSolutionError::collapse_proxies(err);
 
         let mut unavailable_packages = FxHashMap::default();
         for package in err.packages() {

diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs
@@ -6803,8 +6803,7 @@ fn universal_multi_version() -> Result<()> {
 
     ----- stderr -----
       × No solution found when resolving dependencies:
-      ╰─▶ Because iniconfig{python_version > '3.12'}==2.0.0 depends on iniconfig==2.0.0 and you require iniconfig{python_version > '3.12'}==2.0.0, we can conclude that your requirements and iniconfig{python_version == '3.12'}==1.0.0 are incompatible.
-          And because you require iniconfig{python_version == '3.12'}==1.0.0, we can conclude that the requirements are unsatisfiable.
+      ╰─▶ Because you require iniconfig{python_version > '3.12'}==2.0.0 and iniconfig{python_version == '3.12'}==1.0.0, we can conclude that the requirements are unsatisfiable.
     "###
     );