Fix some issues with cargo doc and the new feature resolver.

src/cargo/core/compiler/unit_dependencies.rs

@@ -225,7 +225,7 @@ fn compute_deps(
         return compute_deps_custom_build(unit, unit_for, state);
     } else if unit.mode.is_doc() {
         // Note: this does not include doc test.
-        return compute_deps_doc(unit, state);
+        return compute_deps_doc(unit, state, unit_for);
     }
 
     let id = unit.pkg.package_id();
@@ -395,9 +395,13 @@ fn compute_deps_custom_build(
 }
 
 /// Returns the dependencies necessary to document a package.
-fn compute_deps_doc(unit: &Unit, state: &mut State<'_, '_>) -> CargoResult<Vec<UnitDep>> {
+fn compute_deps_doc(
+    unit: &Unit,
+    state: &mut State<'_, '_>,
+    unit_for: UnitFor,
+) -> CargoResult<Vec<UnitDep>> {
     let deps = state
-        .deps(unit, UnitFor::new_normal())
+        .deps(unit, unit_for)
         .into_iter()
         .filter(|&(_id, deps)| deps.iter().any(|dep| dep.kind() == DepKind::Normal));
 
@@ -414,7 +418,7 @@ fn compute_deps_doc(unit: &Unit, state: &mut State<'_, '_>) -> CargoResult<Vec<U
         // Rustdoc only needs rmeta files for regular dependencies.
         // However, for plugins/proc macros, deps should be built like normal.
         let mode = check_or_build_mode(unit.mode, lib);
-        let dep_unit_for = UnitFor::new_normal().with_dependency(unit, lib);
+        let dep_unit_for = unit_for.with_dependency(unit, lib);
         let lib_unit_dep = new_unit_dep(
             state,
             unit,
@@ -441,11 +445,11 @@ fn compute_deps_doc(unit: &Unit, state: &mut State<'_, '_>) -> CargoResult<Vec<U
     }
 
     // Be sure to build/run the build script for documented libraries.
-    ret.extend(dep_build_script(unit, UnitFor::new_normal(), state)?);
+    ret.extend(dep_build_script(unit, unit_for, state)?);
 
     // If we document a binary/example, we need the library available.
     if unit.target.is_bin() || unit.target.is_example() {
-        ret.extend(maybe_lib(unit, state, UnitFor::new_normal())?);
+        ret.extend(maybe_lib(unit, state, unit_for)?);
     }
     Ok(ret)
 }

src/cargo/ops/cargo_compile.rs

@@ -36,10 +36,11 @@ use crate::core::profiles::{Profiles, UnitFor};
 use crate::core::resolver::features::{self, FeaturesFor, RequestedFeatures};
 use crate::core::resolver::{HasDevUnits, Resolve, ResolveOpts};
 use crate::core::{FeatureValue, Package, PackageSet, Shell, Summary, Target};
-use crate::core::{PackageId, PackageIdSpec, TargetKind, Workspace};
+use crate::core::{PackageId, PackageIdSpec, SourceId, TargetKind, Workspace};
 use crate::ops;
 use crate::ops::resolve::WorkspaceResolve;
 use crate::util::config::Config;
+use crate::util::interning::InternedString;
 use crate::util::restricted_names::is_glob_pattern;
 use crate::util::{closest_msg, profile, CargoResult, StableHasher};
 
@@ -503,6 +504,12 @@ pub fn create_bcx<'a, 'cfg>(
         interner,
     )?;
 
+    // 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);
+    }
+
     if build_config
         .requested_kinds
         .iter()
@@ -1455,3 +1462,108 @@ fn opt_patterns_and_names(
     }
     Ok((opt_patterns, opt_names))
 }
+
+/// Removes duplicate CompileMode::Doc units that would cause problems with
+/// filename collisions.
+///
+/// Rustdoc only separates units by crate name in the file directory
+/// structure. If any two units with the same crate name exist, this would
+/// cause a filename collision, causing different rustdoc invocations to stomp
+/// on one another's files.
+///
+/// Unfortunately this does not remove all duplicates, as some of them are
+/// either user error, or difficult to remove. Cases that I can think of:
+///
+/// - Same target name in different packages. See the `collision_doc` test.
+/// - 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?
+    //
+    // 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();
+    for unit in unit_graph.keys() {
+        if unit.mode.is_doc() {
+            all_docs
+                .entry(unit.target.crate_name())
+                .or_default()
+                .push(unit.clone());
+        }
+    }
+    let mut remove = |units: Vec<Unit>, reason: &str| {
+        for unit in &units {
+            log::debug!(
+                "removing duplicate doc due to {} for package {} target `{}`",
+                reason,
+                unit.pkg,
+                unit.target.name()
+            );
+            unit_graph.remove(unit);
+        }
+        for unit_deps in unit_graph.values_mut() {
+            unit_deps.retain(|unit_dep| !units.iter().any(|unit| *unit == unit_dep.unit));
+        }
+    };
+    // Iterate over the duplicates and try to remove them from unit_graph.
+    for (_crate_name, mut units) in all_docs {
+        if units.len() == 1 {
+            continue;
+        }
+        // Prefer target over host if --target was not specified.
+        if build_config
+            .requested_kinds
+            .iter()
+            .all(CompileKind::is_host)
+        {
+            let (to_remove, remaining_units): (Vec<Unit>, Vec<Unit>) =
+                units.into_iter().partition(|unit| unit.kind.is_host());
+            // Note these duplicates may not be real duplicates, since they
+            // might get merged in rebuild_unit_graph_shared. Either way, it
+            // shouldn't hurt to remove them early (although the report in the
+            // log might be confusing).
+            remove(to_remove, "host/target merger");
+            units = remaining_units;
+            if units.len() == 1 {
+                continue;
+            }
+        }
+        // Prefer newer versions over older.
+        let mut source_map: HashMap<(InternedString, SourceId, CompileKind), Vec<Unit>> =
+            HashMap::new();
+        for unit in units {
+            let pkg_id = unit.pkg.package_id();
+            // Note, this does not detect duplicates from different sources.
+            source_map
+                .entry((pkg_id.name(), pkg_id.source_id(), unit.kind))
+                .or_default()
+                .push(unit);
+        }
+        let mut remaining_units = Vec::new();
+        for (_key, mut units) in source_map {
+            if units.len() > 1 {
+                units.sort_by(|a, b| a.pkg.version().partial_cmp(b.pkg.version()).unwrap());
+                // Remove any entries with version < newest.
+                let newest_version = units.last().unwrap().pkg.version().clone();
+                let (to_remove, keep_units): (Vec<Unit>, Vec<Unit>) = units
+                    .into_iter()
+                    .partition(|unit| unit.pkg.version() < &newest_version);
+                remove(to_remove, "older version");
+                remaining_units.extend(keep_units);
+            } else {
+                remaining_units.extend(units);
+            }
+        }
+        if remaining_units.len() == 1 {
+            continue;
+        }
+        // Are there other heuristics to remove duplicates that would make
+        // sense? Maybe prefer path sources over all others?
+    }
+}