Auto merge of #9077 - ehuss:fix-doc-resolver2-proc-macro, r=alexcrichton

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

This contains two related commits fixing some issues with `cargo doc` and the new feature resolver.

The first commit fixes a panic. The old code was using `UnitFor::new_normal` when computing doc units, but this was wrong. That essentially circumvents the new feature resolver, and breaks determining the correct features to use. I don't remember exactly what I was thinking when I wrote that, other than "rustdoc shouldn't care", but it does matter.

Unfortunately changing that makes collisions worse because it aggravates #6313, so the second commit adds some logic for removing some collisions automatically. Specifically:

- Prefers dependencies for the target over dependencies for the host (such as proc-macros).
- Prefers the "newest" version if it comes from the same source.

There are still plenty of situations where there can be collisions, but I'm uncertain on the best way to handle those.

Fixes #9064

Author: bors

src/cargo/core/compiler/unit_dependencies.rs

@@ -231,7 +231,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();
@@ -414,9 +414,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));
 
@@ -433,7 +437,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,
@@ -460,11 +464,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};
 
@@ -508,6 +509,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()
@@ -1490,3 +1497,113 @@ 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());
+        }
+    }
+    // Keep track of units to remove so that they can be efficiently removed
+    // from the unit_deps.
+    let mut removed_units: HashSet<Unit> = HashSet::new();
+    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);
+            removed_units.insert(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?
+    }
+    // Also remove units from the unit_deps so there aren't any dangling edges.
+    for unit_deps in unit_graph.values_mut() {
+        unit_deps.retain(|unit_dep| !removed_units.contains(&unit_dep.unit));
+    }
+}