Auto merge of #13727 - weihanglo:patch, r=epage

refactor: make `resolve_with_previous` clearer

Author: bors

src/cargo/core/registry.rs

@@ -911,7 +911,12 @@ fn lock(
     })
 }
 
-/// This is a helper for selecting the summary, or generating a helpful error message.
+/// A helper for selecting the summary, or generating a helpful error message.
+///
+/// Returns a tuple that the first element is the summary selected. The second
+/// is a package ID indicating that the patch entry should be unlocked. This
+/// happens when a match cannot be found with the `locked` one, but found one
+/// via the original patch, so we need to inform the resolver to "unlock" it.
 fn summary_for_patch(
     orig_patch: &Dependency,
     locked: &Option<LockedPatchDependency>,
@@ -961,9 +966,6 @@ fn summary_for_patch(
         let orig_matches = orig_matches.into_iter().map(|s| s.into_summary()).collect();
 
         let summary = ready!(summary_for_patch(orig_patch, &None, orig_matches, source))?;
-
-        // The unlocked version found a match. This returns a value to
-        // indicate that this entry should be unlocked.
         return Poll::Ready(Ok((summary.0, Some(locked.package_id))));
     }
     // Try checking if there are *any* packages that match this by name.

src/cargo/ops/cargo_generate_lockfile.rs

@@ -144,13 +144,28 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
         registry.add_sources(sources)?;
     }
 
+    // Here we place an artificial limitation that all non-registry sources
+    // cannot be locked at more than one revision. This means that if a Git
+    // repository provides more than one package, they must all be updated in
+    // step when any of them are updated.
+    //
+    // TODO: this seems like a hokey reason to single out the registry as being
+    // different.
+    let to_avoid_sources: HashSet<_> = to_avoid
+        .iter()
+        .map(|p| p.source_id())
+        .filter(|s| !s.is_registry())
+        .collect();
+
+    let keep = |p: &PackageId| !to_avoid_sources.contains(&p.source_id()) && !to_avoid.contains(p);
+
     let mut resolve = ops::resolve_with_previous(
         &mut registry,
         ws,
         &CliFeatures::new_all(true),
         HasDevUnits::Yes,
         Some(&previous_resolve),
-        Some(&to_avoid),
+        Some(&keep),
         &[],
         true,
     )?;

src/cargo/ops/resolve.rs

@@ -264,9 +264,10 @@ fn resolve_with_registry<'gctx>(
 /// Resolves all dependencies for a package using an optional previous instance
 /// of resolve to guide the resolution process.
 ///
-/// This also takes an optional hash set, `to_avoid`, which is a list of package
-/// IDs that should be avoided when consulting the previous instance of resolve
-/// (often used in pairings with updates).
+/// This also takes an optional filter `keep_previous`, which informs the `registry`
+/// which package ID should be locked to the previous instance of resolve
+/// (often used in pairings with updates). See comments in [`register_previous_locks`]
+/// for scenarios that might override this.
 ///
 /// The previous resolve normally comes from a lock file. This function does not
 /// read or write lock files from the filesystem.
@@ -283,7 +284,7 @@ pub fn resolve_with_previous<'gctx>(
     cli_features: &CliFeatures,
     has_dev_units: HasDevUnits,
     previous: Option<&Resolve>,
-    to_avoid: Option<&HashSet<PackageId>>,
+    keep_previous: Option<&dyn Fn(&PackageId) -> bool>,
     specs: &[PackageIdSpec],
     register_patches: bool,
 ) -> CargoResult<Resolve> {
@@ -293,29 +294,8 @@ pub fn resolve_with_previous<'gctx>(
         .gctx()
         .acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?;
 
-    // Here we place an artificial limitation that all non-registry sources
-    // cannot be locked at more than one revision. This means that if a Git
-    // repository provides more than one package, they must all be updated in
-    // step when any of them are updated.
-    //
-    // TODO: this seems like a hokey reason to single out the registry as being
-    // different.
-    let to_avoid_sources: HashSet<SourceId> = to_avoid
-        .map(|set| {
-            set.iter()
-                .map(|p| p.source_id())
-                .filter(|s| !s.is_registry())
-                .collect()
-        })
-        .unwrap_or_default();
-
-    let pre_patch_keep = |p: &PackageId| {
-        !to_avoid_sources.contains(&p.source_id())
-            && match to_avoid {
-                Some(set) => !set.contains(p),
-                None => true,
-            }
-    };
+    // Try to keep all from previous resolve if no instruction given.
+    let keep_previous = keep_previous.unwrap_or(&|_| true);
 
     // While registering patches, we will record preferences for particular versions
     // of various packages.
@@ -327,117 +307,14 @@ pub fn resolve_with_previous<'gctx>(
         version_prefs.max_rust_version(ws.rust_version().cloned().map(RustVersion::into_partial));
     }
 
-    // This is a set of PackageIds of `[patch]` entries, and some related locked PackageIds, for
-    // which locking should be avoided (but which will be preferred when searching dependencies,
-    // via prefer_patch_deps below)
-    let mut avoid_patch_ids = HashSet::new();
-
-    if register_patches {
-        for (url, patches) in ws.root_patch()?.iter() {
-            for patch in patches {
-                version_prefs.prefer_dependency(patch.clone());
-            }
-            let Some(previous) = previous else {
-                let patches: Vec<_> = patches.iter().map(|p| (p, None)).collect();
-                let unlock_ids = registry.patch(url, &patches)?;
-                // Since nothing is locked, this shouldn't possibly return anything.
-                assert!(unlock_ids.is_empty());
-                continue;
-            };
-
-            // This is a list of pairs where the first element of the pair is
-            // the raw `Dependency` which matches what's listed in `Cargo.toml`.
-            // The second element is, if present, the "locked" version of
-            // the `Dependency` as well as the `PackageId` that it previously
-            // resolved to. This second element is calculated by looking at the
-            // previous resolve graph, which is primarily what's done here to
-            // build the `registrations` list.
-            let mut registrations = Vec::new();
-            for dep in patches {
-                let candidates = || {
-                    previous
-                        .iter()
-                        .chain(previous.unused_patches().iter().cloned())
-                        .filter(&pre_patch_keep)
-                };
-
-                let lock = match candidates().find(|id| dep.matches_id(*id)) {
-                    // If we found an exactly matching candidate in our list of
-                    // candidates, then that's the one to use.
-                    Some(package_id) => {
-                        let mut locked_dep = dep.clone();
-                        locked_dep.lock_to(package_id);
-                        Some(LockedPatchDependency {
-                            dependency: locked_dep,
-                            package_id,
-                            alt_package_id: None,
-                        })
-                    }
-                    None => {
-                        // If the candidate does not have a matching source id
-                        // then we may still have a lock candidate. If we're
-                        // loading a v2-encoded resolve graph and `dep` is a
-                        // git dep with `branch = 'master'`, then this should
-                        // also match candidates without `branch = 'master'`
-                        // (which is now treated separately in Cargo).
-                        //
-                        // In this scenario we try to convert candidates located
-                        // in the resolve graph to explicitly having the
-                        // `master` branch (if they otherwise point to
-                        // `DefaultBranch`). If this works and our `dep`
-                        // matches that then this is something we'll lock to.
-                        match candidates().find(|&id| {
-                            match master_branch_git_source(id, previous) {
-                                Some(id) => dep.matches_id(id),
-                                None => false,
-                            }
-                        }) {
-                            Some(id_using_default) => {
-                                let id_using_master = id_using_default.with_source_id(
-                                    dep.source_id()
-                                        .with_precise_from(id_using_default.source_id()),
-                                );
-
-                                let mut locked_dep = dep.clone();
-                                locked_dep.lock_to(id_using_master);
-                                Some(LockedPatchDependency {
-                                    dependency: locked_dep,
-                                    package_id: id_using_master,
-                                    // Note that this is where the magic
-                                    // happens, where the resolve graph
-                                    // probably has locks pointing to
-                                    // DefaultBranch sources, and by including
-                                    // this here those will get transparently
-                                    // rewritten to Branch("master") which we
-                                    // have a lock entry for.
-                                    alt_package_id: Some(id_using_default),
-                                })
-                            }
-
-                            // No locked candidate was found
-                            None => None,
-                        }
-                    }
-                };
-
-                registrations.push((dep, lock));
-            }
-
-            let canonical = CanonicalUrl::new(url)?;
-            for (orig_patch, unlock_id) in registry.patch(url, &registrations)? {
-                // Avoid the locked patch ID.
-                avoid_patch_ids.insert(unlock_id);
-                // Also avoid the thing it is patching.
-                avoid_patch_ids.extend(previous.iter().filter(|id| {
-                    orig_patch.matches_ignoring_source(*id)
-                        && *id.source_id().canonical_url() == canonical
-                }));
-            }
-        }
-    }
-    debug!("avoid_patch_ids={:?}", avoid_patch_ids);
+    let avoid_patch_ids = if register_patches {
+        register_patch_entries(registry, ws, previous, &mut version_prefs, keep_previous)?
+    } else {
+        HashSet::new()
+    };
 
-    let keep = |p: &PackageId| pre_patch_keep(p) && !avoid_patch_ids.contains(p);
+    // Refine `keep` with patches that should avoid locking.
+    let keep = |p: &PackageId| keep_previous(p) && !avoid_patch_ids.contains(p);
 
     let dev_deps = ws.require_optional_deps() || has_dev_units == HasDevUnits::Yes;
 
@@ -880,3 +757,121 @@ fn emit_warnings_of_unused_patches(
 
     return Ok(());
 }
+
+/// Informs `registry` and `version_pref` that `[patch]` entries are available
+/// and preferable for the dependency resolution.
+///
+/// This returns a set of PackageIds of `[patch]` entries, and some related
+/// locked PackageIds, for which locking should be avoided (but which will be
+/// preferred when searching dependencies, via [`VersionPreferences::prefer_patch_deps`]).
+#[tracing::instrument(level = "debug", skip_all, ret)]
+fn register_patch_entries(
+    registry: &mut PackageRegistry<'_>,
+    ws: &Workspace<'_>,
+    previous: Option<&Resolve>,
+    version_prefs: &mut VersionPreferences,
+    keep_previous: &dyn Fn(&PackageId) -> bool,
+) -> CargoResult<HashSet<PackageId>> {
+    let mut avoid_patch_ids = HashSet::new();
+    for (url, patches) in ws.root_patch()?.iter() {
+        for patch in patches {
+            version_prefs.prefer_dependency(patch.clone());
+        }
+        let Some(previous) = previous else {
+            let patches: Vec<_> = patches.iter().map(|p| (p, None)).collect();
+            let unlock_ids = registry.patch(url, &patches)?;
+            // Since nothing is locked, this shouldn't possibly return anything.
+            assert!(unlock_ids.is_empty());
+            continue;
+        };
+
+        // This is a list of pairs where the first element of the pair is
+        // the raw `Dependency` which matches what's listed in `Cargo.toml`.
+        // The second element is, if present, the "locked" version of
+        // the `Dependency` as well as the `PackageId` that it previously
+        // resolved to. This second element is calculated by looking at the
+        // previous resolve graph, which is primarily what's done here to
+        // build the `registrations` list.
+        let mut registrations = Vec::new();
+        for dep in patches {
+            let candidates = || {
+                previous
+                    .iter()
+                    .chain(previous.unused_patches().iter().cloned())
+                    .filter(&keep_previous)
+            };
+
+            let lock = match candidates().find(|id| dep.matches_id(*id)) {
+                // If we found an exactly matching candidate in our list of
+                // candidates, then that's the one to use.
+                Some(package_id) => {
+                    let mut locked_dep = dep.clone();
+                    locked_dep.lock_to(package_id);
+                    Some(LockedPatchDependency {
+                        dependency: locked_dep,
+                        package_id,
+                        alt_package_id: None,
+                    })
+                }
+                None => {
+                    // If the candidate does not have a matching source id
+                    // then we may still have a lock candidate. If we're
+                    // loading a v2-encoded resolve graph and `dep` is a
+                    // git dep with `branch = 'master'`, then this should
+                    // also match candidates without `branch = 'master'`
+                    // (which is now treated separately in Cargo).
+                    //
+                    // In this scenario we try to convert candidates located
+                    // in the resolve graph to explicitly having the
+                    // `master` branch (if they otherwise point to
+                    // `DefaultBranch`). If this works and our `dep`
+                    // matches that then this is something we'll lock to.
+                    match candidates().find(|&id| match master_branch_git_source(id, previous) {
+                        Some(id) => dep.matches_id(id),
+                        None => false,
+                    }) {
+                        Some(id_using_default) => {
+                            let id_using_master = id_using_default.with_source_id(
+                                dep.source_id()
+                                    .with_precise_from(id_using_default.source_id()),
+                            );
+
+                            let mut locked_dep = dep.clone();
+                            locked_dep.lock_to(id_using_master);
+                            Some(LockedPatchDependency {
+                                dependency: locked_dep,
+                                package_id: id_using_master,
+                                // Note that this is where the magic
+                                // happens, where the resolve graph
+                                // probably has locks pointing to
+                                // DefaultBranch sources, and by including
+                                // this here those will get transparently
+                                // rewritten to Branch("master") which we
+                                // have a lock entry for.
+                                alt_package_id: Some(id_using_default),
+                            })
+                        }
+
+                        // No locked candidate was found
+                        None => None,
+                    }
+                }
+            };
+
+            registrations.push((dep, lock));
+        }
+
+        let canonical = CanonicalUrl::new(url)?;
+        for (orig_patch, unlock_id) in registry.patch(url, &registrations)? {
+            // Avoid the locked patch ID.
+            avoid_patch_ids.insert(unlock_id);
+            // Also avoid the thing it is patching.
+            avoid_patch_ids.extend(previous.iter().filter(|id| {
+                orig_patch.matches_ignoring_source(*id)
+                    && *id.source_id().canonical_url() == canonical
+            }));
+        }
+    }
+
+    Ok(avoid_patch_ids)
+}