Add dedicated `PubGrubPackage::UrlPackage` for URL dependencies

Instead of using `PubGrubPackage::Package(.., Option<Url>)`

crates/puffin-resolver/src/pubgrub/dependencies.rs

@@ -1,10 +1,9 @@
-use fxhash::FxHashMap;
 use itertools::Itertools;
 use pubgrub::range::Range;
 use tracing::warn;
 
 use pep508_rs::{MarkerEnvironment, Requirement, VersionOrUrl};
-use puffin_cache::CanonicalUrl;
+
 use puffin_normalize::{ExtraName, PackageName};
 
 use crate::pubgrub::specifier::PubGrubSpecifier;
@@ -57,21 +56,6 @@ impl PubGrubDependencies {
                 let (package, version) = result?;
 
                 dependencies.push((package.clone(), version.clone()));
-
-                // if let Some(entry) = dependencies.get_key_value(&package) {
-                //     // Merge the versions.
-                //     let version = merge_versions(entry.1, &version);
-
-                //     // Merge the package.
-                //     if let Some(package) = merge_package(entry.0, &package)? {
-                //         dependencies.remove(&package);
-                //         dependencies.insert(package, version);
-                //     } else {
-                //         dependencies.insert(package, version);
-                //     }
-                // } else {
-                //     dependencies.insert(package.clone(), version.clone());
-                // }
             }
         }
 
@@ -107,18 +91,6 @@ impl PubGrubDependencies {
                 let (package, version) = result?;
 
                 dependencies.push((package.clone(), version.clone()));
-
-                // if let Some(entry) = dependencies.get_key_value(&package) {
-                //     // Merge the versions.
-                //     let version = merge_versions(entry.1, &version);
-
-                //     // Merge the package.
-                //     if let Some(package) = merge_package(entry.0, &package)? {
-                //         dependencies.insert(package, version);
-                //     } else {
-                //         dependencies.insert(package, version);
-                //     }
-                // }
             }
         }
 
@@ -151,12 +123,12 @@ fn to_pubgrub(
     match requirement.version_or_url.as_ref() {
         // The requirement has no specifier (e.g., `flask`).
         None => Ok((
-            PubGrubPackage::Package(requirement.name.clone(), extra, None),
+            PubGrubPackage::Package(requirement.name.clone(), extra),
             Range::full(),
         )),
         // The requirement has a URL (e.g., `flask @ file:///path/to/flask`).
         Some(VersionOrUrl::Url(url)) => Ok((
-            PubGrubPackage::Package(requirement.name.clone(), extra, Some(url.clone())),
+            PubGrubPackage::UrlPackage(requirement.name.clone(), extra, url.clone()),
             Range::full(),
         )),
         // The requirement has a specifier (e.g., `flask>=1.0`).
@@ -168,67 +140,9 @@ fn to_pubgrub(
                     range.intersection(&specifier.into())
                 })?;
             Ok((
-                PubGrubPackage::Package(requirement.name.clone(), extra, None),
+                PubGrubPackage::Package(requirement.name.clone(), extra),
                 version,
             ))
         }
     }
 }
-
-/// Merge two [`PubGrubVersion`] ranges.
-fn merge_versions(
-    left: &Range<PubGrubVersion>,
-    right: &Range<PubGrubVersion>,
-) -> Range<PubGrubVersion> {
-    left.intersection(right)
-}
-
-/// Merge two [`PubGrubPackage`] instances.
-fn merge_package(
-    left: &PubGrubPackage,
-    right: &PubGrubPackage,
-) -> Result<Option<PubGrubPackage>, ResolveError> {
-    match (left, right) {
-        // Either package is `root`.
-        (PubGrubPackage::Root(_), _) | (_, PubGrubPackage::Root(_)) => Ok(None),
-
-        // Left package has a URL. Propagate the URL.
-        (PubGrubPackage::Package(name, extra, Some(url)), PubGrubPackage::Package(.., None)) => {
-            Ok(Some(PubGrubPackage::Package(
-                name.clone(),
-                extra.clone(),
-                Some(url.clone()),
-            )))
-        }
-
-        // Right package has a URL.
-        (PubGrubPackage::Package(.., None), PubGrubPackage::Package(name, extra, Some(url))) => {
-            Ok(Some(PubGrubPackage::Package(
-                name.clone(),
-                extra.clone(),
-                Some(url.clone()),
-            )))
-        }
-
-        // Neither package has a URL.
-        (PubGrubPackage::Package(_name, _extra, None), PubGrubPackage::Package(.., None)) => {
-            Ok(None)
-        }
-
-        // Both packages have a URL.
-        (
-            PubGrubPackage::Package(name, _extra, Some(left)),
-            PubGrubPackage::Package(.., Some(right)),
-        ) => {
-            if CanonicalUrl::new(left) == CanonicalUrl::new(right) {
-                Ok(None)
-            } else {
-                Err(ResolveError::ConflictingUrls(
-                    name.clone(),
-                    left.to_string(),
-                    right.to_string(),
-                ))
-            }
-        }
-    }
-}

crates/puffin-resolver/src/pubgrub/package.rs

@@ -14,7 +14,8 @@ use puffin_normalize::{ExtraName, PackageName};
 #[derivative(PartialEq, Hash)]
 pub enum PubGrubPackage {
     Root(Option<PackageName>),
-    Package(
+    Package(PackageName, Option<ExtraName>),
+    UrlPackage(
         PackageName,
         Option<ExtraName>,
         /// The URL of the package, if it was specified in the requirement.
@@ -66,7 +67,7 @@ pub enum PubGrubPackage {
         #[derivative(PartialEq = "ignore")]
         #[derivative(PartialOrd = "ignore")]
         #[derivative(Hash = "ignore")]
-        Option<Url>,
+        Url,
     ),
 }
 
@@ -80,8 +81,13 @@ impl std::fmt::Display for PubGrubPackage {
                     write!(f, "root")
                 }
             }
-            PubGrubPackage::Package(name, None, ..) => write!(f, "{name}"),
+            PubGrubPackage::Package(name, None) => write!(f, "{name}"),
-            PubGrubPackage::Package(name, Some(extra), ..) => {
+            PubGrubPackage::Package(name, Some(extra)) => {
+                write!(f, "{name}[{extra}]")
+            }
+
+            PubGrubPackage::UrlPackage(name, None, ..) => write!(f, "{name}"),
+            PubGrubPackage::UrlPackage(name, Some(extra), ..) => {
                 write!(f, "{name}[{extra}]")
             }
         }

crates/puffin-resolver/src/pubgrub/priority.rs

@@ -20,7 +20,7 @@ impl PubGrubPriorities {
     pub(crate) fn get(&self, package: &PubGrubPackage) -> Option<PubGrubPriority> {
         match package {
             PubGrubPackage::Root(_) => Some(Reverse(0)),
-            PubGrubPackage::Package(name, _, _) => self
+            PubGrubPackage::Package(name, ..) | PubGrubPackage::UrlPackage(name, ..) => self
                 .0
                 .get(name)
                 .copied()

crates/puffin-resolver/src/pubgrub/report.rs

@@ -269,10 +269,16 @@ impl ResolutionFailureReporter {
         let terms_vec: Vec<_> = terms.iter().collect();
         match terms_vec.as_slice() {
             [] | [(PubGrubPackage::Root(_), _)] => "version solving failed".into(),
-            [(package @ PubGrubPackage::Package(..), Term::Positive(range))] => {
+            [(
+                package @ (PubGrubPackage::Package(..) | PubGrubPackage::UrlPackage(..)),
+                Term::Positive(range),
+            )] => {
                 format!("{package}{range} is forbidden")
             }
-            [(package @ PubGrubPackage::Package(..), Term::Negative(range))] => {
+            [(
+                package @ (PubGrubPackage::Package(..) | PubGrubPackage::UrlPackage(..)),
+                Term::Negative(range),
+            )] => {
                 format!("{package}{range} is mandatory")
             }
             [(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => {

crates/puffin-resolver/src/resolution.rs

@@ -70,7 +70,7 @@ impl Graph {
             FxHashMap::with_capacity_and_hasher(selection.len(), BuildHasherDefault::default());
         for (package, version) in selection {
             match package {
-                PubGrubPackage::Package(package_name, None, None) => {
+                PubGrubPackage::Package(package_name, None) => {
                     let version = Version::from(version.clone());
                     let file = pins
                         .get(package_name)
@@ -83,7 +83,7 @@ impl Graph {
                     let index = graph.add_node(pinned_package);
                     inverse.insert(package_name, index);
                 }
-                PubGrubPackage::Package(package_name, None, Some(url)) => {
+                PubGrubPackage::UrlPackage(package_name, None, url) => {
                     let url = redirects
                         .get(url)
                         .map_or_else(|| url.clone(), |url| url.value().clone());
@@ -103,10 +103,13 @@ impl Graph {
                 if let Kind::FromDependencyOf(self_package, self_version, dependency_package, _) =
                     &state.incompatibility_store[*id].kind
                 {
-                    let PubGrubPackage::Package(self_package, None, _) = self_package else {
+                    let (PubGrubPackage::Package(self_package, None)
+                    | PubGrubPackage::UrlPackage(self_package, None, _)) = self_package
+                    else {
                         continue;
                     };
-                    let PubGrubPackage::Package(dependency_package, None, _) = dependency_package
+                    let (PubGrubPackage::Package(dependency_package, None)
+                    | PubGrubPackage::UrlPackage(dependency_package, None, _)) = dependency_package
                     else {
                         continue;
                     };

crates/puffin-resolver/src/resolver.rs

@@ -9,6 +9,7 @@ use futures::channel::mpsc::UnboundedReceiver;
 use futures::{pin_mut, FutureExt, StreamExt, TryFutureExt};
 use fxhash::{FxHashMap, FxHashSet};
 use pubgrub::error::PubGrubError;
+
 use pubgrub::range::Range;
 use pubgrub::solver::{Incompatibility, State};
 use tokio::select;
@@ -281,14 +282,14 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
     ) -> Result<(), ResolveError> {
         match package {
             PubGrubPackage::Root(_) => {}
-            PubGrubPackage::Package(package_name, _extra, None) => {
+            PubGrubPackage::Package(package_name, _extra) => {
                 // Emit a request to fetch the metadata for this package.
                 if in_flight.insert_package(package_name) {
                     priorities.add(package_name.clone());
                     request_sink.unbounded_send(Request::Package(package_name.clone()))?;
                 }
             }
-            PubGrubPackage::Package(package_name, _extra, Some(url)) => {
+            PubGrubPackage::UrlPackage(package_name, _extra, url) => {
                 // Emit a request to fetch the metadata for this package.
                 if in_flight.insert_url(url) {
                     priorities.add(package_name.clone());
@@ -318,7 +319,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
         // Iterate over the potential packages, and fetch file metadata for any of them. These
         // represent our current best guesses for the versions that we _might_ select.
         for (package, range) in packages {
-            let PubGrubPackage::Package(package_name, _extra, None) = package else {
+            let PubGrubPackage::Package(package_name, _extra) = package else {
                 continue;
             };
 
@@ -372,7 +373,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
         return match package {
             PubGrubPackage::Root(_) => Ok(Some(MIN_VERSION.clone())),
 
-            PubGrubPackage::Package(package_name, _extra, Some(url)) => {
+            PubGrubPackage::UrlPackage(package_name, _extra, url) => {
                 debug!("Searching for a compatible version of {package_name} @ {url} ({range})",);
 
                 // If the URL wasn't declared in the direct dependencies or constraints, reject it.
@@ -404,7 +405,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
                 }
             }
 
-            PubGrubPackage::Package(package_name, _extra, None) => {
+            PubGrubPackage::Package(package_name, _extra) => {
                 // Wait for the metadata to be available.
                 let entry = self.index.packages.wait(package_name).await.unwrap();
                 let version_map = entry.value();
@@ -470,7 +471,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
         in_flight: &mut InFlight,
         request_sink: &futures::channel::mpsc::UnboundedSender<Request>,
     ) -> Result<Dependencies, ResolveError> {
-        match package {
+        let (package_name, extra, entry) = match package {
             PubGrubPackage::Root(_) => {
                 // Add the root requirements.
                 let constraints = PubGrubDependencies::try_from_requirements(
@@ -488,19 +489,21 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
                     Self::visit_package(package, priorities, in_flight, request_sink)?;
                 }
 
-                Ok(Dependencies::Known(constraints.into()))
+                return Ok(Dependencies::Known(constraints.into()));
             }
-
+            PubGrubPackage::Package(package_name, extra) => {
-            PubGrubPackage::Package(package_name, extra, url) => {
                 // Wait for the metadata to be available.
-                let entry = match url {
-                    Some(url) => self.index.versions.wait(url.as_str()).await.unwrap(),
-                    None => {
                 let versions = pins.get(package_name).unwrap();
                 let file = versions.get(version.into()).unwrap();
-                        self.index.versions.wait(&file.hashes.sha256).await.unwrap()
+                let entry = self.index.versions.wait(&file.hashes.sha256).await.unwrap();
+                (package_name, extra, entry)
+            }
+            PubGrubPackage::UrlPackage(package_name, extra, url) => {
+                let entry = self.index.versions.wait(url.as_str()).await.unwrap();
+                (package_name, extra, entry)
             }
         };
+
         let metadata = entry.value();
 
         let mut constraints = PubGrubDependencies::try_from_requirements(
@@ -527,15 +530,13 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
                 return Ok(Dependencies::Unknown);
             }
             constraints.insert(
-                        PubGrubPackage::Package(package_name.clone(), None, None),
+                PubGrubPackage::Package(package_name.clone(), None),
                 Range::singleton(version.clone()),
             );
         }
 
         Ok(Dependencies::Known(constraints.into()))
     }
-        }
-    }
 
     /// Fetch the metadata for a stream of packages and versions.
     async fn fetch(&self, request_stream: UnboundedReceiver<Request>) -> Result<(), ResolveError> {
@@ -808,10 +809,10 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
         if let Some(reporter) = self.reporter.as_ref() {
             match package {
                 PubGrubPackage::Root(_) => {}
-                PubGrubPackage::Package(package_name, extra, Some(url)) => {
+                PubGrubPackage::UrlPackage(package_name, extra, url) => {
                     reporter.on_progress(package_name, extra.as_ref(), VersionOrUrl::Url(url));
                 }
-                PubGrubPackage::Package(package_name, extra, None) => {
+                PubGrubPackage::Package(package_name, extra) => {
                     reporter.on_progress(
                         package_name,
                         extra.as_ref(),