Auto merge of #12849 - Eh2406:Precise, r=epage

Make the precise field of a source an Enum

### What does this PR try to resolve?

The precise field of a source_id is a stringly typed untagged union for storing various kinds of data. This is a series of changes untangle what this field is used for. Eventually leading to it being stored as an Enum with dedicated helpers for setting or getting the values different use cases need.

### How should we test and review this PR?

This is an internal re-factor, and all the tests still pass.
This can be reviewed one PR at a time. The first three look at patterns and how precise is used the throughout the code base and make dedicated helpers for each common use case. The last PR switches to an Enum and make helpers for the one-off use cases.

### Additional information

I remember having an issue to do this work, but I cannot find it now.

It's not clear to me if I went overboard on making helpers, API design iteration is welcome.

It would be nice if the `precise` was entirely an internal of the source, this PR does not get us all the way there. This PR makes the representation of the field and internally detail, but its existence still matters to many other parts of the code. Suggestions are welcome.

Author: bors

src/cargo/core/dependency.rs

@@ -352,9 +352,7 @@ impl Dependency {
         // Only update the `precise` of this source to preserve other
         // information about dependency's source which may not otherwise be
         // tested during equality/hashing.
-        me.source_id = me
-            .source_id
-            .with_precise(id.source_id().precise().map(|s| s.to_string()));
+        me.source_id = me.source_id.with_precise_from(id.source_id());
         self
     }
 

src/cargo/core/package_id.rs

@@ -160,14 +160,6 @@ impl PackageId {
         self.inner.source_id
     }
 
-    pub fn with_precise(self, precise: Option<String>) -> PackageId {
-        PackageId::pure(
-            self.inner.name,
-            self.inner.version.clone(),
-            self.inner.source_id.with_precise(precise),
-        )
-    }
-
     pub fn with_source_id(self, source: SourceId) -> PackageId {
         PackageId::pure(self.inner.name, self.inner.version.clone(), source)
     }

src/cargo/core/registry.rs

@@ -161,7 +161,7 @@ impl<'cfg> PackageRegistry<'cfg> {
 
             // If the previous source was not a precise source, then we can be
             // sure that it's already been updated if we've already loaded it.
-            Some((previous, _)) if previous.precise().is_none() => {
+            Some((previous, _)) if !previous.has_precise() => {
                 debug!("load/precise  {}", namespace);
                 return Ok(());
             }
@@ -170,7 +170,7 @@ impl<'cfg> PackageRegistry<'cfg> {
             // then we're done, otherwise we need to need to move forward
             // updating this source.
             Some((previous, _)) => {
-                if previous.precise() == namespace.precise() {
+                if previous.has_same_precise_as(namespace) {
                     debug!("load/match    {}", namespace);
                     return Ok(());
                 }
@@ -471,9 +471,9 @@ impl<'cfg> PackageRegistry<'cfg> {
         //
         // If we have a precise version, then we'll update lazily during the
         // querying phase. Note that precise in this case is only
-        // `Some("locked")` as other `Some` values indicate a `cargo update
+        // `"locked"` as other values indicate a `cargo update
         // --precise` request
-        if source_id.precise() != Some("locked") {
+        if !source_id.has_locked_precise() {
             self.sources.get_mut(source_id).unwrap().invalidate_cache();
         } else {
             debug!("skipping update due to locked registry");

src/cargo/core/resolver/encode.rs

@@ -776,7 +776,7 @@ pub fn encodable_package_id(
             }
         }
     }
-    let mut source = encodable_source_id(id_to_encode.with_precise(None), resolve_version);
+    let mut source = encodable_source_id(id_to_encode.without_precise(), resolve_version);
     if let Some(counts) = &state.counts {
         let version_counts = &counts[&id.name()];
         if version_counts[&id.version()] == 1 {

src/cargo/core/source_id.rs

@@ -3,7 +3,8 @@ use crate::sources::registry::CRATES_IO_HTTP_INDEX;
 use crate::sources::source::Source;
 use crate::sources::{DirectorySource, CRATES_IO_DOMAIN, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
 use crate::sources::{GitSource, PathSource, RegistrySource};
-use crate::util::{config, CanonicalUrl, CargoResult, Config, IntoUrl, ToSemver};
+use crate::util::interning::InternedString;
+use crate::util::{config, CanonicalUrl, CargoResult, Config, IntoUrl};
 use anyhow::Context;
 use serde::de;
 use serde::ser;
@@ -50,14 +51,37 @@ struct SourceIdInner {
     /// The source kind.
     kind: SourceKind,
     /// For example, the exact Git revision of the specified branch for a Git Source.
-    precise: Option<String>,
+    precise: Option<Precise>,
     /// Name of the remote registry.
     ///
     /// WARNING: this is not always set when the name is not known,
     /// e.g. registry coming from `--index` or Cargo.lock
     registry_key: Option<KeyOf>,
 }
 
+#[derive(Eq, PartialEq, Clone, Debug, Hash)]
+enum Precise {
+    Locked,
+    Updated {
+        name: InternedString,
+        from: semver::Version,
+        to: semver::Version,
+    },
+    GitUrlFragment(String),
+}
+
+impl fmt::Display for Precise {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        match self {
+            Precise::Locked => "locked".fmt(f),
+            Precise::Updated { name, from, to } => {
+                write!(f, "{name}={from}->{to}")
+            }
+            Precise::GitUrlFragment(s) => s.fmt(f),
+        }
+    }
+}
+
 /// The possible kinds of code source.
 /// Along with [`SourceIdInner`], this fully defines the source.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -178,17 +202,15 @@ impl SourceId {
                 let precise = url.fragment().map(|s| s.to_owned());
                 url.set_fragment(None);
                 url.set_query(None);
-                Ok(SourceId::for_git(&url, reference)?.with_precise(precise))
+                Ok(SourceId::for_git(&url, reference)?.with_git_precise(precise))
             }
             "registry" => {
                 let url = url.into_url()?;
-                Ok(SourceId::new(SourceKind::Registry, url, None)?
-                    .with_precise(Some("locked".to_string())))
+                Ok(SourceId::new(SourceKind::Registry, url, None)?.with_locked_precise())
             }
             "sparse" => {
                 let url = string.into_url()?;
-                Ok(SourceId::new(SourceKind::SparseRegistry, url, None)?
-                    .with_precise(Some("locked".to_string())))
+                Ok(SourceId::new(SourceKind::SparseRegistry, url, None)?.with_locked_precise())
             }
             "path" => {
                 let url = url.into_url()?;
@@ -332,10 +354,10 @@ impl SourceId {
     pub fn display_registry_name(self) -> String {
         if let Some(key) = self.inner.registry_key.as_ref().map(|k| k.key()) {
             key.into()
-        } else if self.precise().is_some() {
+        } else if self.has_precise() {
             // We remove `precise` here to retrieve an permissive version of
             // `SourceIdInner`, which may contain the registry name.
-            self.with_precise(None).display_registry_name()
+            self.without_precise().display_registry_name()
         } else {
             url_display(self.url())
         }
@@ -444,37 +466,81 @@ impl SourceId {
         }
     }
 
-    /// Gets the value of the precise field.
-    pub fn precise(self) -> Option<&'static str> {
-        self.inner.precise.as_deref()
+    /// Check if the precise data field has bean set
+    pub fn has_precise(self) -> bool {
+        self.inner.precise.is_some()
+    }
+
+    /// Check if the precise data field has bean set to "locked"
+    pub fn has_locked_precise(self) -> bool {
+        self.inner.precise == Some(Precise::Locked)
+    }
+
+    /// Check if two sources have the same precise data field
+    pub fn has_same_precise_as(self, other: Self) -> bool {
+        self.inner.precise == other.inner.precise
     }
 
     /// Check if the precise data field stores information for this `name`
     /// from a call to [SourceId::with_precise_registry_version].
     ///
     /// If so return the version currently in the lock file and the version to be updated to.
-    /// If specified, our own source will have a precise version listed of the form
-    // `<pkg>=<p_req>-><f_req>` where `<pkg>` is the name of a crate on
-    // this source, `<p_req>` is the version installed and `<f_req>` is the
-    // version requested (argument to `--precise`).
     pub fn precise_registry_version(
         self,
-        name: &str,
-    ) -> Option<(semver::Version, semver::Version)> {
-        self.inner
-            .precise
-            .as_deref()
-            .and_then(|p| p.strip_prefix(name)?.strip_prefix('='))
-            .map(|p| {
-                let (current, requested) = p.split_once("->").unwrap();
-                (current.to_semver().unwrap(), requested.to_semver().unwrap())
-            })
+        pkg: &str,
+    ) -> Option<(&semver::Version, &semver::Version)> {
+        match &self.inner.precise {
+            Some(Precise::Updated { name, from, to }) if name == pkg => Some((from, to)),
+            _ => None,
+        }
+    }
+
+    pub fn precise_git_fragment(self) -> Option<&'static str> {
+        match &self.inner.precise {
+            Some(Precise::GitUrlFragment(s)) => Some(&s[..8]),
+            _ => None,
+        }
+    }
+
+    pub fn precise_git_oid(self) -> CargoResult<Option<git2::Oid>> {
+        Ok(match self.inner.precise.as_ref() {
+            Some(Precise::GitUrlFragment(s)) => {
+                Some(git2::Oid::from_str(s).with_context(|| {
+                    format!("precise value for git is not a git revision: {}", s)
+                })?)
+            }
+            _ => None,
+        })
     }
 
     /// Creates a new `SourceId` from this source with the given `precise`.
-    pub fn with_precise(self, v: Option<String>) -> SourceId {
+    pub fn with_git_precise(self, fragment: Option<String>) -> SourceId {
         SourceId::wrap(SourceIdInner {
-            precise: v,
+            precise: fragment.map(|f| Precise::GitUrlFragment(f)),
+            ..(*self.inner).clone()
+        })
+    }
+
+    /// Creates a new `SourceId` from this source without a `precise`.
+    pub fn without_precise(self) -> SourceId {
+        SourceId::wrap(SourceIdInner {
+            precise: None,
+            ..(*self.inner).clone()
+        })
+    }
+
+    /// Creates a new `SourceId` from this source without a `precise`.
+    pub fn with_locked_precise(self) -> SourceId {
+        SourceId::wrap(SourceIdInner {
+            precise: Some(Precise::Locked),
+            ..(*self.inner).clone()
+        })
+    }
+
+    /// Creates a new `SourceId` from this source with the `precise` from some other `SourceId`.
+    pub fn with_precise_from(self, v: Self) -> SourceId {
+        SourceId::wrap(SourceIdInner {
+            precise: v.inner.precise.clone(),
             ..(*self.inner).clone()
         })
     }
@@ -487,13 +553,21 @@ impl SourceId {
     /// The data can be read with [SourceId::precise_registry_version]
     pub fn with_precise_registry_version(
         self,
-        name: impl fmt::Display,
-        version: &semver::Version,
+        name: InternedString,
+        version: semver::Version,
         precise: &str,
     ) -> CargoResult<SourceId> {
-        semver::Version::parse(precise)
+        let precise = semver::Version::parse(precise)
             .with_context(|| format!("invalid version format for precise version `{precise}`"))?;
-        Ok(self.with_precise(Some(format!("{}={}->{}", name, version, precise))))
+
+        Ok(SourceId::wrap(SourceIdInner {
+            precise: Some(Precise::Updated {
+                name,
+                from: version,
+                to: precise,
+            }),
+            ..(*self.inner).clone()
+        }))
     }
 
     /// Returns `true` if the remote registry is the standard <https://crates.io>.
@@ -625,7 +699,8 @@ impl fmt::Display for SourceId {
                     write!(f, "?{}", pretty)?;
                 }
 
-                if let Some(ref s) = self.inner.precise {
+                if let Some(s) = &self.inner.precise {
+                    let s = s.to_string();
                     let len = cmp::min(s.len(), 8);
                     write!(f, "#{}", &s[..len])?;
                 }

src/cargo/ops/cargo_generate_lockfile.rs

@@ -105,14 +105,14 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
                         if pid.source_id().is_registry() {
                             pid.source_id().with_precise_registry_version(
                                 pid.name(),
-                                pid.version(),
+                                pid.version().clone(),
                                 precise,
                             )?
                         } else {
-                            pid.source_id().with_precise(Some(precise.to_string()))
+                            pid.source_id().with_git_precise(Some(precise.to_string()))
                         }
                     }
-                    None => pid.source_id().with_precise(None),
+                    None => pid.source_id().without_precise(),
                 });
             }
             if let Ok(unused_id) =
@@ -147,7 +147,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
                 format!(
                     "{} -> #{}",
                     removed[0],
-                    &added[0].source_id().precise().unwrap()[..8]
+                    &added[0].source_id().precise_git_fragment().unwrap()
                 )
             } else {
                 format!("{} -> v{}", removed[0], added[0].version())
@@ -230,7 +230,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
                     b[i..]
                         .iter()
                         .take_while(|b| a == b)
-                        .all(|b| a.source_id().precise() != b.source_id().precise())
+                        .all(|b| !a.source_id().has_same_precise_as(b.source_id()))
                 })
                 .cloned()
                 .collect()

src/cargo/ops/common_for_install_and_uninstall.rs

@@ -215,7 +215,7 @@ impl InstallTracker {
                 let precise_equal = if source_id.is_git() {
                     // Git sources must have the exact same hash to be
                     // considered "fresh".
-                    dupe_pkg_id.source_id().precise() == source_id.precise()
+                    dupe_pkg_id.source_id().has_same_precise_as(source_id)
                 } else {
                     true
                 };

src/cargo/ops/resolve.rs

@@ -389,12 +389,8 @@ pub fn resolve_with_previous<'cfg>(
                         }) {
                             Some(id_using_default) => {
                                 let id_using_master = id_using_default.with_source_id(
-                                    dep.source_id().with_precise(
-                                        id_using_default
-                                            .source_id()
-                                            .precise()
-                                            .map(|s| s.to_string()),
-                                    ),
+                                    dep.source_id()
+                                        .with_precise_from(id_using_default.source_id()),
                                 );
 
                                 let mut locked_dep = dep.clone();
@@ -796,7 +792,7 @@ fn master_branch_git_source(id: PackageId, resolve: &Resolve) -> Option<PackageI
             let new_source =
                 SourceId::for_git(source.url(), GitReference::Branch("master".to_string()))
                     .unwrap()
-                    .with_precise(source.precise().map(|s| s.to_string()));
+                    .with_precise_from(source);
             return Some(id.with_source_id(new_source));
         }
     }

src/cargo/ops/vendor.rs

@@ -258,7 +258,7 @@ fn sync(
         } else {
             // Remove `precise` since that makes the source name very long,
             // and isn't needed to disambiguate multiple sources.
-            source_id.with_precise(None).as_url().to_string()
+            source_id.without_precise().as_url().to_string()
         };
 
         let source = if source_id.is_crates_io() {

src/cargo/sources/config.rs

@@ -145,7 +145,7 @@ impl<'cfg> SourceConfigMap<'cfg> {
                 // Attempt to interpret the source name as an alt registry name
                 if let Ok(alt_id) = SourceId::alt_registry(self.config, name) {
                     debug!("following pointer to registry {}", name);
-                    break alt_id.with_precise(id.precise().map(str::to_string));
+                    break alt_id.with_precise_from(id);
                 }
                 bail!(
                     "could not find a configured source with the \
@@ -163,7 +163,7 @@ impl<'cfg> SourceConfigMap<'cfg> {
                 }
                 None if id == cfg.id => return id.load(self.config, yanked_whitelist),
                 None => {
-                    break cfg.id.with_precise(id.precise().map(|s| s.to_string()));
+                    break cfg.id.with_precise_from(id);
                 }
             }
             debug!("following pointer to {}", name);
@@ -199,7 +199,7 @@ a lock file compatible with `{orig}` cannot be generated in this situation
             );
         }
 
-        if old_src.requires_precise() && id.precise().is_none() {
+        if old_src.requires_precise() && !id.has_precise() {
             bail!(
                 "\
 the source {orig} requires a lock file to be present first before it can be