Auto merge of #9142 - ehuss:fix-doc-orphan, r=Eh2406

Fix panic with doc collision orphan.

As I feared, the collision removal added in #9077 caused problems due to orphans left in the unit graph. Ironically, it was the collision warning detection code which failed, as it iterates over all the keys of the graph.

The solution is to remove all orphans from the graph after collisions have been removed.

Fixes #9136

Author: bors

src/cargo/ops/cargo_compile.rs

@@ -507,7 +507,7 @@ pub fn create_bcx<'a, 'cfg>(
     // TODO: In theory, Cargo should also dedupe the roots, but I'm uncertain
     // what heuristics to use in that case.
     if build_config.mode == (CompileMode::Doc { deps: true }) {
-        remove_duplicate_doc(build_config, &mut unit_graph);
+        remove_duplicate_doc(build_config, &units, &mut unit_graph);
     }
 
     if build_config
@@ -1508,14 +1508,11 @@ fn opt_patterns_and_names(
 /// - Different sources. See `collision_doc_sources` test.
 ///
 /// Ideally this would not be necessary.
-fn remove_duplicate_doc(build_config: &BuildConfig, unit_graph: &mut UnitGraph) {
-    // NOTE: There is some risk that this can introduce problems because it
-    // may create orphans in the unit graph (parts of the tree get detached
-    // from the roots). I currently can't think of any ways this will cause a
-    // problem because all other parts of Cargo traverse the graph starting
-    // from the roots. Perhaps this should scan for detached units and remove
-    // them too?
-    //
+fn remove_duplicate_doc(
+    build_config: &BuildConfig,
+    root_units: &[Unit],
+    unit_graph: &mut UnitGraph,
+) {
     // First, create a mapping of crate_name -> Unit so we can see where the
     // duplicates are.
     let mut all_docs: HashMap<String, Vec<Unit>> = HashMap::new();
@@ -1601,4 +1598,18 @@ fn remove_duplicate_doc(build_config: &BuildConfig, unit_graph: &mut UnitGraph)
     for unit_deps in unit_graph.values_mut() {
         unit_deps.retain(|unit_dep| !removed_units.contains(&unit_dep.unit));
     }
+    // Remove any orphan units that were detached from the graph.
+    let mut visited = HashSet::new();
+    fn visit(unit: &Unit, graph: &UnitGraph, visited: &mut HashSet<Unit>) {
+        if !visited.insert(unit.clone()) {
+            return;
+        }
+        for dep in &graph[unit] {
+            visit(&dep.unit, graph, visited);
+        }
+    }
+    for unit in root_units {
+        visit(unit, unit_graph, &mut visited);
+    }
+    unit_graph.retain(|unit, _| visited.contains(unit));
 }

tests/testsuite/collisions.rs

@@ -3,9 +3,8 @@
 //! Ideally these should never happen, but I don't think we'll ever be able to
 //! prevent all collisions.
 
-use cargo_test_support::basic_manifest;
-use cargo_test_support::project;
 use cargo_test_support::registry::Package;
+use cargo_test_support::{basic_manifest, cross_compile, project};
 use std::env;
 
 #[cargo_test]
@@ -431,3 +430,51 @@ the same path; see <https://github.com/rust-lang/cargo/issues/6313>.
         )
         .run();
 }
+
+#[cargo_test]
+fn collision_doc_target() {
+    // collision in doc with --target, doesn't fail due to orphans
+    if cross_compile::disabled() {
+        return;
+    }
+
+    Package::new("orphaned", "1.0.0").publish();
+    Package::new("bar", "1.0.0")
+        .dep("orphaned", "1.0")
+        .publish();
+    Package::new("bar", "2.0.0").publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+
+                [dependencies]
+                bar2 = { version = "2.0", package="bar" }
+                bar = "1.0"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("doc --target")
+        .arg(cross_compile::alternate())
+        .with_stderr_unordered(
+            "\
+[UPDATING] [..]
+[DOWNLOADING] crates ...
+[DOWNLOADED] orphaned v1.0.0 [..]
+[DOWNLOADED] bar v2.0.0 [..]
+[DOWNLOADED] bar v1.0.0 [..]
+[CHECKING] orphaned v1.0.0
+[DOCUMENTING] bar v2.0.0
+[CHECKING] bar v2.0.0
+[CHECKING] bar v1.0.0
+[DOCUMENTING] foo v0.1.0 [..]
+[FINISHED] [..]
+",
+        )
+        .run();
+}