Auto merge of #8274 - Eh2406:8249-repro, r=alexcrichton

reset lockfile information between resolutions

#8249 pointed out that some kind of lockfile data was leaking between calls to the resolver. @ehuss made a reproducing test case. This PR resets the `LockedMap` data structure when calling `register_previous_locks`.

lets see if CI likes it.
fix #8249

Author: bors

crates/cargo-test-support/src/registry.rs

@@ -145,6 +145,7 @@ pub struct Package {
     alternative: bool,
     invalid_json: bool,
     proc_macro: bool,
+    links: Option<String>,
 }
 
 #[derive(Clone)]
@@ -245,6 +246,7 @@ impl Package {
             alternative: false,
             invalid_json: false,
             proc_macro: false,
+            links: None,
         }
     }
 
@@ -368,6 +370,11 @@ impl Package {
         self
     }
 
+    pub fn links(&mut self, links: &str) -> &mut Package {
+        self.links = Some(links.to_string());
+        self
+    }
+
     /// Creates the package and place it in the registry.
     ///
     /// This does not actually use Cargo's publishing system, but instead
@@ -420,6 +427,7 @@ impl Package {
             "cksum": cksum,
             "features": self.features,
             "yanked": self.yanked,
+            "links": self.links,
         })
         .to_string();
 

crates/resolver-tests/tests/resolve.rs

@@ -1458,6 +1458,31 @@ fn conflict_store_more_then_one_match() {
     let _ = resolve_and_validated(vec![dep("nA")], &reg, None);
 }
 
+#[test]
+fn bad_lockfile_from_8249() {
+    let input = vec![
+        pkg!(("a-sys", "0.2.0")),
+        pkg!(("a-sys", "0.1.0")),
+        pkg!(("b", "0.1.0") => [
+            dep_req("a-sys", "0.1"), // should be optional: true, but not deeded for now
+        ]),
+        pkg!(("c", "1.0.0") => [
+            dep_req("b", "=0.1.0"),
+        ]),
+        pkg!("foo" => [
+            dep_req("a-sys", "=0.2.0"),
+            {
+                let mut b = dep_req("b", "=0.1.0");
+                b.set_features(vec!["a-sys"]);
+                b
+            },
+            dep_req("c", "=1.0.0"),
+        ]),
+    ];
+    let reg = registry(input);
+    let _ = resolve_and_validated(vec![dep("foo")], &reg, None);
+}
+
 #[test]
 fn cyclic_good_error_message() {
     let input = vec![

src/cargo/core/registry.rs

@@ -5,8 +5,8 @@ use log::{debug, trace};
 use semver::VersionReq;
 use url::Url;
 
-use crate::core::PackageSet;
 use crate::core::{Dependency, PackageId, Source, SourceId, SourceMap, Summary};
+use crate::core::{InternedString, PackageSet};
 use crate::sources::config::SourceConfigMap;
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::{profile, CanonicalUrl, Config};
@@ -91,16 +91,13 @@ pub struct PackageRegistry<'cfg> {
 type LockedMap = HashMap<
     // The first level of key-ing done in this hash map is the source that
     // dependencies come from, identified by a `SourceId`.
-    SourceId,
-    HashMap<
-        // This next level is keyed by the name of the package...
-        String,
-        // ... and the value here is a list of tuples. The first element of each
-        // tuple is a package which has the source/name used to get to this
-        // point. The second element of each tuple is the list of locked
-        // dependencies that the first element has.
-        Vec<(PackageId, Vec<PackageId>)>,
-    >,
+    // The next level is keyed by the name of the package...
+    (SourceId, InternedString),
+    // ... and the value here is a list of tuples. The first element of each
+    // tuple is a package which has the source/name used to get to this
+    // point. The second element of each tuple is the list of locked
+    // dependencies that the first element has.
+    Vec<(PackageId, Vec<PackageId>)>,
 >;
 
 #[derive(PartialEq, Eq, Clone, Copy)]
@@ -198,17 +195,20 @@ impl<'cfg> PackageRegistry<'cfg> {
         self.yanked_whitelist.extend(pkgs);
     }
 
+    /// remove all residual state from previous lock files.
+    pub fn clear_lock(&mut self) {
+        trace!("clear_lock");
+        self.locked = HashMap::new();
+    }
+
     pub fn register_lock(&mut self, id: PackageId, deps: Vec<PackageId>) {
         trace!("register_lock: {}", id);
         for dep in deps.iter() {
             trace!("\t-> {}", dep);
         }
-        let sub_map = self
+        let sub_vec = self
             .locked
-            .entry(id.source_id())
-            .or_insert_with(HashMap::new);
-        let sub_vec = sub_map
-            .entry(id.name().to_string())
+            .entry((id.source_id(), id.name()))
             .or_insert_with(Vec::new);
         sub_vec.push((id, deps));
     }
@@ -639,8 +639,7 @@ fn lock(
     summary: Summary,
 ) -> Summary {
     let pair = locked
-        .get(&summary.source_id())
-        .and_then(|map| map.get(&*summary.name()))
+        .get(&(summary.source_id(), summary.name()))
         .and_then(|vec| vec.iter().find(|&&(id, _)| id == summary.package_id()));
 
     trace!("locking summary of {}", summary.package_id());
@@ -729,8 +728,7 @@ fn lock(
         // all known locked packages to see if they match this dependency.
         // If anything does then we lock it to that and move on.
         let v = locked
-            .get(&dep.source_id())
-            .and_then(|map| map.get(&*dep.package_name()))
+            .get(&(dep.source_id(), dep.package_name()))
             .and_then(|vec| vec.iter().find(|&&(id, _)| dep.matches_id(id)));
         if let Some(&(id, _)) = v {
             trace!("\tsecond hit on {}", id);

src/cargo/ops/resolve.rs

@@ -584,6 +584,7 @@ fn register_previous_locks(
     // the registry as a locked dependency.
     let keep = |id: &PackageId| keep(id) && !avoid_locking.contains(id);
 
+    registry.clear_lock();
     for node in resolve.iter().filter(keep) {
         let deps = resolve
             .deps_not_replaced(node)

tests/testsuite/build.rs

@@ -4939,3 +4939,44 @@ hello stderr!
         stdout
     );
 }
+
+use cargo_test_support::registry::Dependency;
+
+#[cargo_test]
+fn reduced_reproduction_8249() {
+    // https://github.com/rust-lang/cargo/issues/8249
+    Package::new("a-src", "0.1.0").links("a").publish();
+    Package::new("a-src", "0.2.0").links("a").publish();
+
+    Package::new("b", "0.1.0")
+        .add_dep(Dependency::new("a-src", "0.1").optional(true))
+        .publish();
+    Package::new("b", "0.2.0")
+        .add_dep(Dependency::new("a-src", "0.2").optional(true))
+        .publish();
+
+    Package::new("c", "1.0.0")
+        .add_dep(&Dependency::new("b", "0.1.0"))
+        .publish();
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                b = { version = "*", features = ["a-src"] }
+                a-src = "*"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("generate-lockfile").run();
+    cargo::util::paths::append(&p.root().join("Cargo.toml"), b"c = \"*\"").unwrap();
+    p.cargo("check").run();
+    p.cargo("check").run();
+}