Auto merge of #9827 - weihanglo:issue-6199, r=Eh2406

Improve resolver message to include dependency requirements

Resolves #6199.

Thanks for previous efforts: #5452, #6374, #6665, which are great but somehow outdated, so I tweak them and create this PR. This will also be obsolete if we ship pubgrub-rs with cargo in the future 😃 But before that happens, IMO these changes are still helpful.

---

This PR changes the resolver error message from

https://github.com/rust-lang/cargo/blob/216f915c46b8ada2323423d049314ba18247ef95/tests/testsuite/build.rs#L1104-L1106

to

https://github.com/rust-lang/cargo/blob/0afd40b4de17a5c45145a0762beb4ef001720fe1/tests/testsuite/build.rs#L1104-L1106

Also provide different message for different source kinds, such like:

https://github.com/rust-lang/cargo/blob/0afd40b4de17a5c45145a0762beb4ef001720fe1/tests/testsuite/build.rs#L2810-L2812

## TODO?

From #5452 (comment), there shall be at least one task left behind:

> 3. Special case pind by a lock file and not a `"=1.1.2"` in a dependency. Also add a "note: try cargo update" to the end.

In this PR, `validate_links` also faces this issue that a dependency requirement is locked into a precise version `=0.1.0`.

https://github.com/rust-lang/cargo/blob/a5f8bc94f5d38539dd127f735ea4d3a515c230fd/tests/testsuite/build_script.rs#L1002-L1004

I am uncertain about how to resolve this. Besides  the function`validate_links`, is this problem really a thing that may happen? If not, since `validate_links` only handles old validation logic, it may be ok to drop the commit a5f8bc9 and leave it as is.

Author: bors

crates/resolver-tests/tests/resolve.rs

@@ -1498,7 +1498,7 @@ fn cyclic_good_error_message() {
     assert_eq!("\
 cyclic package dependency: package `A v0.0.0 (registry `https://example.com/`)` depends on itself. Cycle:
 package `A v0.0.0 (registry `https://example.com/`)`
-    ... which is depended on by `C v0.0.0 (registry `https://example.com/`)`
-    ... which is depended on by `A v0.0.0 (registry `https://example.com/`)`\
+    ... which satisfies dependency `A = \"*\"` of package `C v0.0.0 (registry `https://example.com/`)`
+    ... which satisfies dependency `C = \"*\"` of package `A v0.0.0 (registry `https://example.com/`)`\
 ", error.to_string());
 }

src/cargo/core/compiler/links.rs

@@ -1,8 +1,8 @@
 use super::unit_graph::UnitGraph;
+use crate::core::resolver::errors::describe_path;
 use crate::core::{PackageId, Resolve};
 use crate::util::errors::CargoResult;
 use std::collections::{HashMap, HashSet};
-use std::fmt::Write;
 
 /// Validate `links` field does not conflict between packages.
 pub fn validate_links(resolve: &Resolve, unit_graph: &UnitGraph) -> CargoResult<()> {
@@ -28,17 +28,15 @@ pub fn validate_links(resolve: &Resolve, unit_graph: &UnitGraph) -> CargoResult<
             None => continue,
         };
         if let Some(&prev) = links.get(lib) {
+            let prev_path = resolve
+                .path_to_top(&prev)
+                .into_iter()
+                .map(|(p, d)| (p, d.and_then(|d| d.iter().next())));
             let pkg = unit.pkg.package_id();
-
-            let describe_path = |pkgid: PackageId| -> String {
-                let dep_path = resolve.path_to_top(&pkgid);
-                let mut dep_path_desc = format!("package `{}`", dep_path[0]);
-                for dep in dep_path.iter().skip(1) {
-                    write!(dep_path_desc, "\n    ... which is depended on by `{}`", dep).unwrap();
-                }
-                dep_path_desc
-            };
-
+            let path = resolve
+                .path_to_top(&pkg)
+                .into_iter()
+                .map(|(p, d)| (p, d.and_then(|d| d.iter().next())));
             anyhow::bail!(
                 "multiple packages link to native library `{}`, \
                  but a native library can be linked only once\n\
@@ -47,9 +45,9 @@ pub fn validate_links(resolve: &Resolve, unit_graph: &UnitGraph) -> CargoResult<
                  \n\
                  {}\nalso links to native library `{}`",
                 lib,
-                describe_path(prev),
+                describe_path(prev_path),
                 lib,
-                describe_path(pkg),
+                describe_path(path),
                 lib
             )
         }

src/cargo/core/resolver/dep_cache.rs

@@ -10,7 +10,7 @@
 //! This module impl that cache in all the gory details
 
 use crate::core::resolver::context::Context;
-use crate::core::resolver::errors::describe_path;
+use crate::core::resolver::errors::describe_path_in_context;
 use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet};
 use crate::core::resolver::{
     ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts, VersionOrdering,
@@ -216,7 +216,7 @@ impl<'a> RegistryQueryer<'a> {
                     format!(
                         "failed to get `{}` as a dependency of {}",
                         dep.package_name(),
-                        describe_path(&cx.parents.path_to_bottom(&candidate.package_id())),
+                        describe_path_in_context(cx, &candidate.package_id()),
                     )
                 })?;
                 Ok((dep, candidates, features))

src/cargo/core/resolver/errors.rs

@@ -82,6 +82,7 @@ pub(super) fn activation_error(
             cx.parents
                 .path_to_bottom(&parent.package_id())
                 .into_iter()
+                .map(|(node, _)| node)
                 .cloned()
                 .collect(),
         )
@@ -90,9 +91,7 @@ pub(super) fn activation_error(
     if !candidates.is_empty() {
         let mut msg = format!("failed to select a version for `{}`.", dep.package_name());
         msg.push_str("\n    ... required by ");
-        msg.push_str(&describe_path(
-            &cx.parents.path_to_bottom(&parent.package_id()),
-        ));
+        msg.push_str(&describe_path_in_context(cx, &parent.package_id()));
 
         msg.push_str("\nversions that meet the requirements `");
         msg.push_str(&dep.version_req().to_string());
@@ -128,7 +127,7 @@ pub(super) fn activation_error(
                     msg.push_str("`, but it conflicts with a previous package which links to `");
                     msg.push_str(link);
                     msg.push_str("` as well:\n");
-                    msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
+                    msg.push_str(&describe_path_in_context(cx, p));
                     msg.push_str("\nOnly one package in the dependency graph may specify the same links value. This helps ensure that only one copy of a native library is linked in the final binary. ");
                     msg.push_str("Try to adjust your dependencies so that only one package uses the links ='");
                     msg.push_str(&*dep.package_name());
@@ -197,7 +196,7 @@ pub(super) fn activation_error(
             for (p, r) in &conflicting_activations {
                 if let ConflictReason::Semver = r {
                     msg.push_str("\n\n  previously selected ");
-                    msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
+                    msg.push_str(&describe_path_in_context(cx, p));
                 }
             }
         }
@@ -250,9 +249,7 @@ pub(super) fn activation_error(
                 registry.describe_source(dep.source_id()),
             );
             msg.push_str("required by ");
-            msg.push_str(&describe_path(
-                &cx.parents.path_to_bottom(&parent.package_id()),
-            ));
+            msg.push_str(&describe_path_in_context(cx, &parent.package_id()));
 
             // If we have a path dependency with a locked version, then this may
             // indicate that we updated a sub-package and forgot to run `cargo
@@ -330,9 +327,7 @@ pub(super) fn activation_error(
             }
             msg.push_str(&format!("location searched: {}\n", dep.source_id()));
             msg.push_str("required by ");
-            msg.push_str(&describe_path(
-                &cx.parents.path_to_bottom(&parent.package_id()),
-            ));
+            msg.push_str(&describe_path_in_context(cx, &parent.package_id()));
 
             msg
         };
@@ -351,12 +346,57 @@ pub(super) fn activation_error(
     to_resolve_err(anyhow::format_err!("{}", msg))
 }
 
+/// Returns String representation of dependency chain for a particular `pkgid`
+/// within given context.
+pub(super) fn describe_path_in_context(cx: &Context, id: &PackageId) -> String {
+    let iter = cx
+        .parents
+        .path_to_bottom(id)
+        .into_iter()
+        .map(|(p, d)| (p, d.and_then(|d| d.iter().next())));
+    describe_path(iter)
+}
+
 /// Returns String representation of dependency chain for a particular `pkgid`.
-pub(super) fn describe_path(path: &[&PackageId]) -> String {
+///
+/// Note that all elements of `path` iterator should have `Some` dependency
+/// except the first one. It would look like:
+///
+/// (pkg0, None)
+/// -> (pkg1, dep from pkg1 satisfied by pkg0)
+/// -> (pkg2, dep from pkg2 satisfied by pkg1)
+/// -> ...
+pub(crate) fn describe_path<'a>(
+    mut path: impl Iterator<Item = (&'a PackageId, Option<&'a Dependency>)>,
+) -> String {
     use std::fmt::Write;
-    let mut dep_path_desc = format!("package `{}`", path[0]);
-    for dep in path[1..].iter() {
-        write!(dep_path_desc, "\n    ... which is depended on by `{}`", dep).unwrap();
+
+    if let Some(p) = path.next() {
+        let mut dep_path_desc = format!("package `{}`", p.0);
+        for (pkg, dep) in path {
+            let dep = dep.unwrap();
+            let source_kind = if dep.source_id().is_path() {
+                "path "
+            } else if dep.source_id().is_git() {
+                "git "
+            } else {
+                ""
+            };
+            let requirement = if source_kind.is_empty() {
+                format!("{} = \"{}\"", dep.name_in_toml(), dep.version_req())
+            } else {
+                dep.name_in_toml().to_string()
+            };
+            write!(
+                dep_path_desc,
+                "\n    ... which satisfies {}dependency `{}` of package `{}`",
+                source_kind, requirement, pkg
+            )
+            .unwrap();
+        }
+
+        return dep_path_desc;
     }
-    dep_path_desc
+
+    String::new()
 }

src/cargo/core/resolver/mod.rs

@@ -47,7 +47,7 @@
 //! that we're implementing something that probably shouldn't be allocating all
 //! over the place.
 
-use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::mem;
 use std::rc::Rc;
 use std::time::{Duration, Instant};
@@ -78,7 +78,7 @@ mod conflict_cache;
 mod context;
 mod dep_cache;
 mod encode;
-mod errors;
+pub(crate) mod errors;
 pub mod features;
 mod resolve;
 mod types;
@@ -1007,13 +1007,15 @@ fn check_cycles(resolve: &Resolve) -> CargoResult<()> {
     // dev-dependency since that doesn't count for cycles.
     let mut graph = BTreeMap::new();
     for id in resolve.iter() {
-        let set = graph.entry(id).or_insert_with(BTreeSet::new);
-        for (dep, listings) in resolve.deps_not_replaced(id) {
-            let is_transitive = listings.iter().any(|d| d.is_transitive());
-
-            if is_transitive {
-                set.insert(dep);
-                set.extend(resolve.replacement(dep));
+        let map = graph.entry(id).or_insert_with(BTreeMap::new);
+        for (dep_id, listings) in resolve.deps_not_replaced(id) {
+            let transitive_dep = listings.iter().find(|d| d.is_transitive());
+
+            if let Some(transitive_dep) = transitive_dep.cloned() {
+                map.insert(dep_id, transitive_dep.clone());
+                resolve
+                    .replacement(dep_id)
+                    .map(|p| map.insert(p, transitive_dep));
             }
         }
     }
@@ -1033,23 +1035,29 @@ fn check_cycles(resolve: &Resolve) -> CargoResult<()> {
     return Ok(());
 
     fn visit(
-        graph: &BTreeMap<PackageId, BTreeSet<PackageId>>,
+        graph: &BTreeMap<PackageId, BTreeMap<PackageId, Dependency>>,
         id: PackageId,
         visited: &mut HashSet<PackageId>,
         path: &mut Vec<PackageId>,
         checked: &mut HashSet<PackageId>,
     ) -> CargoResult<()> {
         path.push(id);
         if !visited.insert(id) {
+            let iter = path.iter().rev().skip(1).scan(id, |child, parent| {
+                let dep = graph.get(parent).and_then(|adjacent| adjacent.get(child));
+                *child = *parent;
+                Some((parent, dep))
+            });
+            let iter = std::iter::once((&id, None)).chain(iter);
             anyhow::bail!(
                 "cyclic package dependency: package `{}` depends on itself. Cycle:\n{}",
                 id,
-                errors::describe_path(&path.iter().rev().collect::<Vec<_>>()),
+                errors::describe_path(iter),
             );
         }
 
         if checked.insert(id) {
-            for dep in graph[&id].iter() {
+            for dep in graph[&id].keys() {
                 visit(graph, *dep, visited, path, checked)?;
             }
         }

src/cargo/core/resolver/resolve.rs

@@ -117,7 +117,10 @@ impl Resolve {
 
     /// Resolves one of the paths from the given dependent package up to
     /// the root.
-    pub fn path_to_top<'a>(&'a self, pkg: &'a PackageId) -> Vec<&'a PackageId> {
+    pub fn path_to_top<'a>(
+        &'a self,
+        pkg: &'a PackageId,
+    ) -> Vec<(&'a PackageId, Option<&'a HashSet<Dependency>>)> {
         self.graph.path_to_top(pkg)
     }
 

src/cargo/util/graph.rs

@@ -89,40 +89,50 @@ impl<N: Eq + Ord + Clone, E: Default + Clone> Graph<N, E> {
 
     /// Resolves one of the paths from the given dependent package down to
     /// a leaf.
-    pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> {
-        let mut result = vec![pkg];
+    ///
+    /// Each element contains a node along with an edge except the first one.
+    /// The representation would look like:
+    ///
+    /// (Node0,) -> (Node1, Edge01) -> (Node2, Edge12)...
+    pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> {
+        let mut result = vec![(pkg, None)];
         while let Some(p) = self.nodes.get(pkg).and_then(|p| {
             p.iter()
                 // Note that we can have "cycles" introduced through dev-dependency
                 // edges, so make sure we don't loop infinitely.
-                .find(|(node, _)| !result.contains(node))
-                .map(|(p, _)| p)
+                .find(|&(node, _)| result.iter().all(|p| p.0 != node))
+                .map(|(node, edge)| (node, Some(edge)))
         }) {
             result.push(p);
-            pkg = p;
+            pkg = p.0;
         }
         result
     }
 
     /// Resolves one of the paths from the given dependent package up to
     /// the root.
-    pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> {
+    ///
+    /// Each element contains a node along with an edge except the first one.
+    /// The representation would look like:
+    ///
+    /// (Node0,) -> (Node1, Edge01) -> (Node2, Edge12)...
+    pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> {
         // Note that this implementation isn't the most robust per se, we'll
         // likely have to tweak this over time. For now though it works for what
         // it's used for!
-        let mut result = vec![pkg];
-        let first_pkg_depending_on = |pkg: &N, res: &[&N]| {
+        let mut result = vec![(pkg, None)];
+        let first_pkg_depending_on = |pkg, res: &[(&N, Option<&E>)]| {
             self.nodes
                 .iter()
                 .filter(|(_, adjacent)| adjacent.contains_key(pkg))
                 // Note that we can have "cycles" introduced through dev-dependency
                 // edges, so make sure we don't loop infinitely.
-                .find(|(node, _)| !res.contains(node))
-                .map(|(p, _)| p)
+                .find(|&(node, _)| !res.iter().any(|p| p.0 == node))
+                .map(|(p, adjacent)| (p, adjacent.get(pkg)))
         };
         while let Some(p) = first_pkg_depending_on(pkg, &result) {
             result.push(p);
-            pkg = p;
+            pkg = p.0;
         }
         result
     }