Refactor to support stabilized package ids

This refactors the internals, and the public API, to support both the current "opaque" package ids as well as the new package id format stabilized in <rust-lang/cargo#12914>

Author: Jake-Shadle

examples/graph.rs

@@ -28,7 +28,7 @@ pub type Graph = krates::Krates<Simple>;
 impl From<krates::cm::Package> for Simple {
     fn from(pkg: krates::cm::Package) -> Self {
         Self {
-            id: pkg.id,
+            id: pkg.id.into(),
             //features: pkg.fee
         }
     }

src/builder.rs

@@ -48,9 +48,162 @@ pub use builder::{
 };
 pub use errors::Error;
 pub use pkgspec::PkgSpec;
+use std::fmt;
 
 /// A crate's unique identifier
-pub type Kid = cargo_metadata::PackageId;
+#[derive(Clone)]
+pub struct Kid {
+    pub repr: String,
+    // The subslices for each component in name -> version -> source order
+    components: [(usize, usize); 3],
+}
+
+impl Kid {
+    #[inline]
+    pub fn name(&self) -> &str {
+        let (s, e) = self.components[0];
+        &self.repr[s..e]
+    }
+
+    #[inline]
+    pub fn version(&self) -> &str {
+        let (s, e) = self.components[1];
+        &self.repr[s..e]
+    }
+
+    #[inline]
+    pub fn source(&self) -> &str {
+        let (s, e) = self.components[2];
+        &self.repr[s..e]
+    }
+
+    #[inline]
+    pub fn rev(&self) -> Option<&str> {
+        let src = self.source();
+        if src.starts_with("git+") {
+            // In old style ids the rev with always be available, but new ones
+            // only have it if the rev field was actually set, which is unfortunate
+            if let Some((_, rev)) = src.split_once('#') {
+                Some(rev)
+            } else {
+                src.split_once("?rev=").map(|(_, r)| r)
+            }
+        } else {
+            None
+        }
+    }
+}
+
+#[allow(clippy::fallible_impl_from)]
+impl From<cargo_metadata::PackageId> for Kid {
+    fn from(pid: cargo_metadata::PackageId) -> Self {
+        let repr = pid.repr;
+
+        let gen = || {
+            let components = if repr.contains(' ') {
+                let name = (0, repr.find(' ')?);
+                let version = (name.1 + 1, repr[name.1 + 1..].find(' ')? + name.1 + 1);
+                // Note we skip the open and close parentheses as they are superfluous
+                // as every source has them, as well as not being present in the new
+                // stabilized format
+                let source = (version.1 + 2, repr.len() - 1);
+
+                [name, version, source]
+            } else {
+                let vmn = repr.rfind('#')?;
+                let (name, version) = if let Some(split) = repr[vmn..].find('@') {
+                    ((vmn + 1, vmn + split), (vmn + split + 1, repr.len()))
+                } else {
+                    let begin = repr.rfind('/')? + 1;
+                    let end = if repr.starts_with("git+") {
+                        repr[begin..].find('?')? + begin
+                    } else {
+                        vmn
+                    };
+
+                    ((begin, end), (vmn + 1, repr.len()))
+                };
+
+                [name, version, (0, vmn)]
+            };
+
+            Some(components)
+        };
+
+        if let Some(components) = gen() {
+            Self { repr, components }
+        } else {
+            panic!("unable to parse package id '{repr}'");
+        }
+    }
+}
+
+impl fmt::Debug for Kid {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut ds = f.debug_struct("Kid");
+
+        ds.field("name", &self.name())
+            .field("version", &self.version());
+
+        let src = self.source();
+        if src != "registry+https://github.com/rust-lang/crates.io-index" {
+            ds.field("source", &src);
+        }
+
+        ds.finish()
+    }
+}
+
+impl fmt::Display for Kid {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(&self.repr)
+    }
+}
+
+impl Default for Kid {
+    fn default() -> Self {
+        Self {
+            repr: String::new(),
+            components: [(0, 0), (0, 0), (0, 0)],
+        }
+    }
+}
+
+impl std::hash::Hash for Kid {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        state.write(self.repr.as_bytes());
+    }
+}
+
+impl Ord for Kid {
+    fn cmp(&self, o: &Self) -> std::cmp::Ordering {
+        let a = &self.repr;
+        let b = &o.repr;
+
+        for (ar, br) in self.components.into_iter().zip(o.components.into_iter()) {
+            let ord = a[ar.0..ar.1].cmp(&b[br.0..br.1]);
+            if ord != std::cmp::Ordering::Equal {
+                return ord;
+            }
+        }
+
+        std::cmp::Ordering::Equal
+    }
+}
+
+impl PartialOrd for Kid {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl Eq for Kid {}
+
+impl PartialEq for Kid {
+    fn eq(&self, other: &Self) -> bool {
+        self.cmp(other) == std::cmp::Ordering::Equal
+    }
+}
 
 /// The set of features that have been enabled on a crate
 pub type EnabledFeatures = std::collections::BTreeSet<String>;
@@ -84,8 +237,6 @@ impl PartialEq<DK> for DepKind {
     }
 }
 
-use std::fmt;
-
 impl fmt::Display for DepKind {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
@@ -509,29 +660,36 @@ where
     ///
     /// fn print(krates: &Krates, name: &str) {
     ///     let req = VersionReq::parse("=0.2").unwrap();
-    ///     for vs in krates.search_matches(name, req.clone()).map(|(_, krate)| &krate.version) {
-    ///         println!("found version {} matching {}!", vs, req);
+    ///     for vs in krates.search_matches(name, req.clone()).map(|km| &km.krate.version) {
+    ///         println!("found version {vs} matching {req}!");
     ///     }
     /// }
     /// ```
     pub fn search_matches(
         &self,
         name: impl Into<String>,
         req: semver::VersionReq,
-    ) -> impl Iterator<Item = (NodeId, &N)> {
+    ) -> impl Iterator<Item = KrateMatch<'_, N>> {
         let raw_nodes = &self.graph.raw_nodes()[0..self.krates_end];
 
         let name = name.into();
 
-        raw_nodes.iter().enumerate().filter_map(move |(id, node)| {
-            if let Node::Krate { krate, .. } = &node.weight {
-                if krate.name() == name && req.matches(krate.version()) {
-                    return Some((NodeId::new(id), krate));
+        raw_nodes
+            .iter()
+            .enumerate()
+            .filter_map(move |(index, node)| {
+                if let Node::Krate { krate, id, .. } = &node.weight {
+                    if krate.name() == name && req.matches(krate.version()) {
+                        return Some(KrateMatch {
+                            node_id: NodeId::new(index),
+                            krate,
+                            kid: id,
+                        });
+                    }
                 }
-            }
 
-            None
-        })
+                None
+            })
     }
 
     /// Get an iterator over all of the crates in the graph with the given name,
@@ -541,28 +699,44 @@ where
     /// use krates::Krates;
     ///
     /// fn print_all_versions(krates: &Krates, name: &str) {
-    ///     for vs in krates.krates_by_name(name).map(|(_, krate)| &krate.version) {
-    ///         println!("found version {}", vs);
+    ///     for vs in krates.krates_by_name(name).map(|km| &km.krate.version) {
+    ///         println!("found version {vs}");
     ///     }
     /// }
     /// ```
-    pub fn krates_by_name(&self, name: impl Into<String>) -> impl Iterator<Item = (NodeId, &N)> {
+    pub fn krates_by_name(
+        &self,
+        name: impl Into<String>,
+    ) -> impl Iterator<Item = KrateMatch<'_, N>> {
         let raw_nodes = &self.graph.raw_nodes()[0..self.krates_end];
 
         let name = name.into();
 
-        raw_nodes.iter().enumerate().filter_map(move |(id, node)| {
-            if let Node::Krate { krate, .. } = &node.weight {
-                if krate.name() == name {
-                    return Some((NodeId::new(id), krate));
+        raw_nodes
+            .iter()
+            .enumerate()
+            .filter_map(move |(index, node)| {
+                if let Node::Krate { krate, id, .. } = &node.weight {
+                    if krate.name() == name {
+                        return Some(KrateMatch {
+                            node_id: NodeId::new(index),
+                            krate,
+                            kid: id,
+                        });
+                    }
                 }
-            }
 
-            None
-        })
+                None
+            })
     }
 }
 
+pub struct KrateMatch<'graph, N> {
+    pub krate: &'graph N,
+    pub kid: &'graph Kid,
+    pub node_id: NodeId,
+}
+
 impl<N, E> std::ops::Index<NodeId> for Krates<N, E> {
     type Output = N;
 
@@ -586,3 +760,31 @@ impl<N, E> std::ops::Index<usize> for Krates<N, E> {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    #[test]
+    fn converts_package_ids() {
+        let ids = [
+            ("registry+https://github.com/rust-lang/crates.io-index#[email protected]", "ab_glyph", "0.2.22", "registry+https://github.com/rust-lang/crates.io-index", None),
+            ("git+https://github.com/EmbarkStudios/egui-stylist?rev=3900e8aedc5801e42c1bb747cfd025615bf3b832#0.2.0", "egui-stylist", "0.2.0", "git+https://github.com/EmbarkStudios/egui-stylist?rev=3900e8aedc5801e42c1bb747cfd025615bf3b832", Some("3900e8aedc5801e42c1bb747cfd025615bf3b832")),
+            ("path+file:///home/jake/code/ark/components/allocator#[email protected]", "ark-allocator", "0.1.0", "path+file:///home/jake/code/ark/components/allocator", None),
+            ("git+https://github.com/EmbarkStudios/ash?branch=nv-low-latency2#0.38.0+1.3.269", "ash", "0.38.0+1.3.269", "git+https://github.com/EmbarkStudios/ash?branch=nv-low-latency2", None),
+            ("git+https://github.com/EmbarkStudios/fsr-rs?branch=nv-low-latency2#[email protected]", "fsr", "0.1.7", "git+https://github.com/EmbarkStudios/fsr-rs?branch=nv-low-latency2", None),
+            ("fuser 0.4.1 (git+https://github.com/cberner/fuser?branch=master#b2e7622942e52a28ffa85cdaf48e28e982bb6923)", "fuser", "0.4.1", "git+https://github.com/cberner/fuser?branch=master#b2e7622942e52a28ffa85cdaf48e28e982bb6923", Some("b2e7622942e52a28ffa85cdaf48e28e982bb6923")),
+            ("a 0.1.0 (path+file:///home/jake/code/krates/tests/ws/a)", "a", "0.1.0", "path+file:///home/jake/code/krates/tests/ws/a", None),
+            ("bindgen 0.59.2 (registry+https://github.com/rust-lang/crates.io-index)", "bindgen", "0.59.2", "registry+https://github.com/rust-lang/crates.io-index", None),
+        ];
+
+        for (repr, name, version, source, rev) in ids {
+            let kid = super::Kid::from(cargo_metadata::PackageId {
+                repr: repr.to_owned(),
+            });
+
+            assert_eq!(kid.name(), name);
+            assert_eq!(kid.version(), version);
+            assert_eq!(kid.source(), source);
+            assert_eq!(kid.rev(), rev);
+        }
+    }
+}

src/lib.rs

@@ -23,36 +23,18 @@ impl PkgSpec {
             }
         }
 
-        match self.url {
-            Some(ref u) => {
-                // Get the url from the identifier to avoid pointless
-                // allocations.
-                if let Some(mut url) = krate.id.repr.splitn(3, ' ').nth(2) {
-                    // Strip off the the enclosing parens
-                    url = &url[1..url.len() - 1];
-
-                    // Strip off the leading <source>+
-                    if let Some(ind) = url.find('+') {
-                        url = &url[ind + 1..];
-                    }
-
-                    // Strip off any fragments
-                    if let Some(ind) = url.rfind('#') {
-                        url = &url[..ind];
-                    }
+        let Some((url, src)) = self
+            .url
+            .as_ref()
+            .zip(krate.source.as_ref().map(|s| s.repr.as_str()))
+        else {
+            return true;
+        };
 
-                    // Strip off any query parts
-                    if let Some(ind) = url.rfind('?') {
-                        url = &url[..ind];
-                    }
+        let begin = src.find('+').map_or(0, |i| i + 1);
+        let end = src.find('?').or_else(|| src.find('#')).unwrap_or(src.len());
 
-                    u == url
-                } else {
-                    false
-                }
-            }
-            None => true,
-        }
+        url == &src[begin..end]
     }
 }
 

src/pkgspec.rs

@@ -46,13 +46,13 @@ fn prunes_mixed_dependencies() {
 
 macro_rules! assert_features {
     ($graph:expr, $name:expr, $features:expr) => {
-        let (_, krate) = $graph.krates_by_name($name).next().unwrap();
+        let krates::KrateMatch { kid, .. } = $graph.krates_by_name($name).next().unwrap();
 
         let expected_features: std::collections::BTreeSet<_> =
             $features.into_iter().map(|s| s.to_owned()).collect();
 
         assert_eq!(
-            $graph.get_enabled_features(&krate.id).unwrap(),
+            $graph.get_enabled_features(kid).unwrap(),
             &expected_features
         );
     };