add a harder test.

Author: Eh2406

src/cargo/core/resolver/mod.rs

@@ -601,30 +601,36 @@ fn activate(
                 .link(candidate.summary.package_id(), parent.package_id()),
         )
         .push(dep.clone());
-        let mut stack = vec![(parent.package_id(), dep.is_public())];
-        while let Some((p, public)) = stack.pop() {
-            match cx
-                .public_dependency
-                .entry(p)
-                .or_default()
-                .entry(candidate.summary.name())
-            {
-                im_rc::hashmap::Entry::Occupied(mut o) => {
-                    assert_eq!(o.get().0, candidate.summary.package_id());
-                    if o.get().1 {
-                        continue;
+        let cs: Vec<PackageId> = cx
+            .public_dependency
+            .get(&candidate.summary.package_id())
+            .iter()
+            .flat_map(|x| x.values())
+            .filter_map(|x| if x.1 { Some(&x.0) } else { None })
+            .chain(Some(candidate.summary.package_id()).iter())
+            .cloned()
+            .collect();
+        for c in cs {
+            let mut stack = vec![(parent.package_id(), dep.is_public())];
+            while let Some((p, public)) = stack.pop() {
+                match cx.public_dependency.entry(p).or_default().entry(c.name()) {
+                    im_rc::hashmap::Entry::Occupied(mut o) => {
+                        assert_eq!(o.get().0, c);
+                        if o.get().1 {
+                            continue;
+                        }
+                        if public {
+                            o.insert((c, public));
+                        }
                     }
-                    if public {
-                        o.insert((candidate.summary.package_id(), public));
+                    im_rc::hashmap::Entry::Vacant(v) => {
+                        v.insert((c, public));
                     }
                 }
-                im_rc::hashmap::Entry::Vacant(v) => {
-                    v.insert((candidate.summary.package_id(), public));
-                }
-            }
-            if public {
-                for &(grand, ref d) in cx.parents.edges(&p) {
-                    stack.push((grand, d.iter().any(|x| x.is_public())));
+                if public {
+                    for &(grand, ref d) in cx.parents.edges(&p) {
+                        stack.push((grand, d.iter().any(|x| x.is_public())));
+                    }
                 }
             }
         }
@@ -763,23 +769,27 @@ impl RemainingCandidates {
                     continue;
                 }
             }
-
-            let mut stack = vec![(parent, dep.is_public())];
-            while let Some((p, public)) = stack.pop() {
-                // TODO: dont look at the same thing more then once
-                if let Some(o) = cx
-                    .public_dependency
-                    .get(&p)
-                    .and_then(|x| x.get(&b.summary.name()))
-                {
-                    if o.0 != b.summary.package_id() {
-                        // TODO: conflicting_prev_active
-                        continue 'main;
+            for &t in cx
+                .public_dependency
+                .get(&b.summary.package_id())
+                .iter()
+                .flat_map(|x| x.values())
+                .filter_map(|x| if x.1 { Some(&x.0) } else { None })
+                .chain(Some(b.summary.package_id()).iter())
+            {
+                let mut stack = vec![(parent, dep.is_public())];
+                while let Some((p, public)) = stack.pop() {
+                    // TODO: dont look at the same thing more then once
+                    if let Some(o) = cx.public_dependency.get(&p).and_then(|x| x.get(&t.name())) {
+                        if o.0 != t {
+                            // TODO: conflicting_prev_active
+                            continue 'main;
+                        }
                     }
-                }
-                if public {
-                    for &(grand, ref d) in cx.parents.edges(&p) {
-                        stack.push((grand, d.iter().any(|x| x.is_public())));
+                    if public {
+                        for &(grand, ref d) in cx.parents.edges(&p) {
+                            stack.push((grand, d.iter().any(|x| x.is_public())));
+                        }
                     }
                 }
             }

tests/testsuite/resolve.rs

@@ -236,12 +236,12 @@ proptest! {
 }
 
 #[test]
-fn public_dependency() {
+fn basic_public_dependency() {
     let reg = registry(vec![
         pkg!(("A", "0.1.0")),
         pkg!(("A", "0.2.0")),
         pkg!("B" => [dep_req_kind("A", "0.1", Kind::Normal, true)]),
-        pkg!("C" => [dep_req("A", "*"), dep_req("B", "*")]),
+        pkg!("C" => [dep("A"), dep("B")]),
     ]);
 
     let res = resolve_and_validated(&pkg_id("root"), vec![dep("C")], &reg).unwrap();
@@ -256,6 +256,78 @@ fn public_dependency() {
     );
 }
 
+#[test]
+fn public_dependency_filling_in() {
+    // The resolver has an optimization where if a candidate to resolve a dependency
+    // has already bean activated then we skip looking at the candidates dependencies.
+    // However, we have to be careful as the new path may make pub dependencies invalid.
+
+    // Triggering this case requires dependencies to be resolved in a specific order.
+    // Fuzzing found this unintuitive case, that triggers this unfortunate order of operations:
+    // 1. `d`'s dep on `c` is resolved
+    // 2. `d`'s dep on `a` is resolved with `0.1.1`
+    // 3. `c`'s dep on `b` is resolved with `0.0.2`
+    // 4. `b`'s dep on `a` is resolved with `0.0.6` no pub dev conflict as `b` is private to `c`
+    // 5. `d`'s dep on `b` is resolved with `0.0.2` triggering the optimization.
+    // Do we notice that `d` has a pub dep conflict on `a`? Lets try it and see.
+    let reg = registry(vec![
+        pkg!(("a", "0.0.6")),
+        pkg!(("a", "0.1.1")),
+        pkg!(("b", "0.0.0") => [dep("bad")]),
+        pkg!(("b", "0.0.1") => [dep("bad")]),
+        pkg!(("b", "0.0.2") => [dep_req_kind("a", "=0.0.6", Kind::Normal, true)]),
+        pkg!("c" => [dep_req("b", ">=0.0.1")]),
+        pkg!("d" => [dep("c"), dep("a"), dep("b")]),
+    ]);
+
+    let res = resolve_and_validated(&pkg_id("root"), vec![dep("d")], &reg).unwrap();
+    assert_same(
+        &res,
+        &names(&[
+            ("root", "1.0.0"),
+            ("d", "1.0.0"),
+            ("c", "1.0.0"),
+            ("b", "0.0.2"),
+            ("a", "0.0.6"),
+        ]),
+    );
+}
+
+#[test]
+fn public_dependency_filling_in_and_update() {
+    // The resolver has an optimization where if a candidate to resolve a dependency
+    // has already bean activated then we skip looking at the candidates dependencies.
+    // However, we have to be careful as the new path may make pub dependencies invalid.
+
+    // Triggering this case requires dependencies to be resolved in a specific order.
+    // Fuzzing found this unintuitive case, that triggers this unfortunate order of operations:
+    // 1. `D`'s dep on `B` is resolved
+    // 2. `D`'s dep on `C` is resolved
+    // 3. `B`'s dep on `A` is resolved with `0.0.0`
+    // 4. `C`'s dep on `B` triggering the optimization.
+    // So did we add `A 0.0.0` to the deps `C` can see?
+    // Or are we going to  resolve `C`'s dep on `A` with `0.0.2`?
+    // Lets try it and see.
+    let reg = registry(vec![
+        pkg!(("A", "0.0.0")),
+        pkg!(("A", "0.0.2")),
+        pkg!("B" => [dep_req_kind("A", "=0.0.0", Kind::Normal, true),]),
+        pkg!("C" => [dep("A"),dep("B")]),
+        pkg!("D" => [dep("B"),dep("C")]),
+    ]);
+    let res = resolve_and_validated(&pkg_id("root"), vec![dep("D")], &reg).unwrap();
+    assert_same(
+        &res,
+        &names(&[
+            ("root", "1.0.0"),
+            ("D", "1.0.0"),
+            ("C", "1.0.0"),
+            ("B", "1.0.0"),
+            ("A", "0.0.0"),
+        ]),
+    );
+}
+
 #[test]
 #[should_panic(expected = "assertion failed: !name.is_empty()")]
 fn test_dependency_with_empty_name() {