Auto merge of #8248 - ehuss:patch-err-help, r=alexcrichton

Provide better error messages for a bad `patch`.

This attempts to provide more user-friendly error messages for some situations with a bad `patch`.  This is a follow-up to #8243.

I think this more or less covers all the issues from #4678.  I imagine there are other corner cases, but those will need to wait for another day. The main one I can think of is when the patch location is missing required features.  Today you get a "blah was not used in the crate graph." warning, with some suggestions added in #6470, but it doesn't actually check if there is a feature mismatch.

Closes #4678

src/cargo/core/registry.rs

@@ -222,16 +222,35 @@ impl<'cfg> PackageRegistry<'cfg> {
     /// the manifest.
     ///
     /// Here the `deps` will be resolved to a precise version and stored
-    /// internally for future calls to `query` below. It's expected that `deps`
-    /// have had `lock_to` call already, if applicable. (e.g., if a lock file was
-    /// already present).
+    /// internally for future calls to `query` below. `deps` should be a tuple
+    /// where the first element is the patch definition straight from the
+    /// manifest, and the second element is an optional variant where the
+    /// patch has been locked. This locked patch is the patch locked to
+    /// a specific version found in Cargo.lock. This will be `None` if
+    /// `Cargo.lock` doesn't exist, or the patch did not match any existing
+    /// entries in `Cargo.lock`.
     ///
     /// Note that the patch list specified here *will not* be available to
     /// `query` until `lock_patches` is called below, which should be called
     /// once all patches have been added.
-    pub fn patch(&mut self, url: &Url, deps: &[Dependency]) -> CargoResult<()> {
+    ///
+    /// The return value is a `Vec` of patches that should *not* be locked.
+    /// This happens when the patch is locked, but the patch has been updated
+    /// so the locked value is no longer correct.
+    pub fn patch(
+        &mut self,
+        url: &Url,
+        deps: &[(&Dependency, Option<(Dependency, PackageId)>)],
+    ) -> CargoResult<Vec<(Dependency, PackageId)>> {
+        // NOTE: None of this code is aware of required features. If a patch
+        // is missing a required feature, you end up with an "unused patch"
+        // warning, which is very hard to understand. Ideally the warning
+        // would be tailored to indicate *why* it is unused.
         let canonical = CanonicalUrl::new(url)?;
 
+        // Return value of patches that shouldn't be locked.
+        let mut unlock_patches = Vec::new();
+
         // First up we need to actually resolve each `deps` specification to
         // precisely one summary. We're not using the `query` method below as it
         // internally uses maps we're building up as part of this method
@@ -243,7 +262,15 @@ impl<'cfg> PackageRegistry<'cfg> {
         // of summaries which should be the same length as `deps` above.
         let unlocked_summaries = deps
             .iter()
-            .map(|dep| {
+            .map(|(orig_patch, locked)| {
+                // Remove double reference in orig_patch. Is there maybe a
+                // magic pattern that could avoid this?
+                let orig_patch = *orig_patch;
+                // Use the locked patch if it exists, otherwise use the original.
+                let dep = match locked {
+                    Some((locked_patch, _locked_id)) => locked_patch,
+                    None => orig_patch,
+                };
                 debug!(
                     "registering a patch for `{}` with `{}`",
                     url,
@@ -261,30 +288,27 @@ impl<'cfg> PackageRegistry<'cfg> {
                         )
                     })?;
 
-                let mut summaries = self
+                let source = self
                     .sources
                     .get_mut(dep.source_id())
-                    .expect("loaded source not present")
-                    .query_vec(dep)?
-                    .into_iter();
-
-                let summary = match summaries.next() {
-                    Some(summary) => summary,
-                    None => anyhow::bail!(
-                        "patch for `{}` in `{}` did not resolve to any crates. If this is \
-                         unexpected, you may wish to consult: \
-                         https://github.com/rust-lang/cargo/issues/4678",
-                        dep.package_name(),
-                        url
-                    ),
-                };
-                if summaries.next().is_some() {
-                    anyhow::bail!(
-                        "patch for `{}` in `{}` resolved to more than one candidate",
-                        dep.package_name(),
-                        url
-                    )
+                    .expect("loaded source not present");
+                let summaries = source.query_vec(dep)?;
+                let (summary, should_unlock) =
+                    summary_for_patch(orig_patch, &locked, summaries, source).chain_err(|| {
+                        format!(
+                            "patch for `{}` in `{}` failed to resolve",
+                            orig_patch.package_name(),
+                            url,
+                        )
+                    })?;
+                debug!(
+                    "patch summary is {:?} should_unlock={:?}",
+                    summary, should_unlock
+                );
+                if let Some(unlock_id) = should_unlock {
+                    unlock_patches.push((orig_patch.clone(), unlock_id));
                 }
+
                 if *summary.package_id().source_id().canonical_url() == canonical {
                     anyhow::bail!(
                         "patch for `{}` in `{}` points to the same source, but \
@@ -321,7 +345,7 @@ impl<'cfg> PackageRegistry<'cfg> {
         self.patches_available.insert(canonical.clone(), ids);
         self.patches.insert(canonical, unlocked_summaries);
 
-        Ok(())
+        Ok(unlock_patches)
     }
 
     /// Lock all patch summaries added via `patch`, making them available to
@@ -335,6 +359,7 @@ impl<'cfg> PackageRegistry<'cfg> {
         assert!(!self.patches_locked);
         for summaries in self.patches.values_mut() {
             for summary in summaries {
+                debug!("locking patch {:?}", summary);
                 *summary = lock(&self.locked, &self.patches_available, summary.clone());
             }
         }
@@ -718,3 +743,97 @@ fn lock(
         dep
     })
 }
+
+/// This is a helper for selecting the summary, or generating a helpful error message.
+fn summary_for_patch(
+    orig_patch: &Dependency,
+    locked: &Option<(Dependency, PackageId)>,
+    mut summaries: Vec<Summary>,
+    source: &mut dyn Source,
+) -> CargoResult<(Summary, Option<PackageId>)> {
+    if summaries.len() == 1 {
+        return Ok((summaries.pop().unwrap(), None));
+    }
+    if summaries.len() > 1 {
+        // TODO: In the future, it might be nice to add all of these
+        // candidates so that version selection would just pick the
+        // appropriate one. However, as this is currently structured, if we
+        // added these all as patches, the unselected versions would end up in
+        // the "unused patch" listing, and trigger a warning. It might take a
+        // fair bit of restructuring to make that work cleanly, and there
+        // isn't any demand at this time to support that.
+        let mut vers: Vec<_> = summaries.iter().map(|summary| summary.version()).collect();
+        vers.sort();
+        let versions: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
+        anyhow::bail!(
+            "patch for `{}` in `{}` resolved to more than one candidate\n\
+            Found versions: {}\n\
+            Update the patch definition to select only one package.\n\
+            For example, add an `=` version requirement to the patch definition, \
+            such as `version = \"={}\"`.",
+            orig_patch.package_name(),
+            orig_patch.source_id(),
+            versions.join(", "),
+            versions.last().unwrap()
+        );
+    }
+    assert!(summaries.is_empty());
+    // No summaries found, try to help the user figure out what is wrong.
+    if let Some((_locked_patch, locked_id)) = locked {
+        // Since the locked patch did not match anything, try the unlocked one.
+        let orig_matches = source.query_vec(orig_patch).unwrap_or_else(|e| {
+            log::warn!(
+                "could not determine unlocked summaries for dep {:?}: {:?}",
+                orig_patch,
+                e
+            );
+            Vec::new()
+        });
+        let (summary, _) = 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 Ok((summary, Some(*locked_id)));
+    }
+    // Try checking if there are *any* packages that match this by name.
+    let name_only_dep = Dependency::new_override(orig_patch.package_name(), orig_patch.source_id());
+    let name_summaries = source.query_vec(&name_only_dep).unwrap_or_else(|e| {
+        log::warn!(
+            "failed to do name-only summary query for {:?}: {:?}",
+            name_only_dep,
+            e
+        );
+        Vec::new()
+    });
+    let mut vers = name_summaries
+        .iter()
+        .map(|summary| summary.version())
+        .collect::<Vec<_>>();
+    let found = match vers.len() {
+        0 => format!(""),
+        1 => format!("version `{}`", vers[0]),
+        _ => {
+            vers.sort();
+            let strs: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
+            format!("versions `{}`", strs.join(", "))
+        }
+    };
+    if found.is_empty() {
+        anyhow::bail!(
+            "The patch location `{}` does not appear to contain any packages \
+            matching the name `{}`.",
+            orig_patch.source_id(),
+            orig_patch.package_name()
+        );
+    } else {
+        anyhow::bail!(
+            "The patch location `{}` contains a `{}` package with {}, but the patch \
+            definition requires `{}`.\n\
+            Check that the version in the patch location is what you expect, \
+            and update the patch definition to match.",
+            orig_patch.source_id(),
+            orig_patch.package_name(),
+            found,
+            orig_patch.version_req()
+        );
+    }
+}

src/cargo/ops/resolve.rs

@@ -20,7 +20,7 @@ use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Worksp
 use crate::ops;
 use crate::sources::PathSource;
 use crate::util::errors::{CargoResult, CargoResultExt};
-use crate::util::profile;
+use crate::util::{profile, CanonicalUrl};
 use log::{debug, trace};
 use std::collections::HashSet;
 
@@ -224,36 +224,26 @@ pub fn resolve_with_previous<'cfg>(
         );
     }
 
-    let keep = |p: &PackageId| {
+    let pre_patch_keep = |p: &PackageId| {
         !to_avoid_sources.contains(&p.source_id())
             && match to_avoid {
                 Some(set) => !set.contains(p),
                 None => true,
             }
     };
 
-    // In the case where a previous instance of resolve is available, we
-    // want to lock as many packages as possible to the previous version
-    // without disturbing the graph structure.
-    let mut try_to_use = HashSet::new();
-    if let Some(r) = previous {
-        trace!("previous: {:?}", r);
-        register_previous_locks(ws, registry, r, &keep);
-
-        // Everything in the previous lock file we want to keep is prioritized
-        // in dependency selection if it comes up, aka we want to have
-        // conservative updates.
-        try_to_use.extend(r.iter().filter(keep).inspect(|id| {
-            debug!("attempting to prefer {}", id);
-        }));
-    }
-
+    // This is a set of PackageIds of `[patch]` entries that should not be
+    // locked.
+    let mut avoid_patch_ids = HashSet::new();
     if register_patches {
         for (url, patches) in ws.root_patch() {
             let previous = match previous {
                 Some(r) => r,
                 None => {
-                    registry.patch(url, patches)?;
+                    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;
                 }
             };
@@ -262,19 +252,52 @@ pub fn resolve_with_previous<'cfg>(
                 .map(|dep| {
                     let unused = previous.unused_patches().iter().cloned();
                     let candidates = previous.iter().chain(unused);
-                    match candidates.filter(keep).find(|&id| dep.matches_id(id)) {
+                    match candidates
+                        .filter(pre_patch_keep)
+                        .find(|&id| dep.matches_id(id))
+                    {
                         Some(id) => {
-                            let mut dep = dep.clone();
-                            dep.lock_to(id);
-                            dep
+                            let mut locked_dep = dep.clone();
+                            locked_dep.lock_to(id);
+                            (dep, Some((locked_dep, id)))
                         }
-                        None => dep.clone(),
+                        None => (dep, None),
                     }
                 })
                 .collect::<Vec<_>>();
-            registry.patch(url, &patches)?;
+            let canonical = CanonicalUrl::new(url)?;
+            for (orig_patch, unlock_id) in registry.patch(url, &patches)? {
+                // 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 keep = |p: &PackageId| pre_patch_keep(p) && !avoid_patch_ids.contains(p);
+
+    // In the case where a previous instance of resolve is available, we
+    // want to lock as many packages as possible to the previous version
+    // without disturbing the graph structure.
+    let mut try_to_use = HashSet::new();
+    if let Some(r) = previous {
+        trace!("previous: {:?}", r);
+        register_previous_locks(ws, registry, r, &keep);
+
+        // Everything in the previous lock file we want to keep is prioritized
+        // in dependency selection if it comes up, aka we want to have
+        // conservative updates.
+        try_to_use.extend(r.iter().filter(keep).inspect(|id| {
+            debug!("attempting to prefer {}", id);
+        }));
+    }
+
+    if register_patches {
         registry.lock_patches();
     }