Auto merge of #9526 - ehuss:doc-collision-resolve, r=alexcrichton

Consolidate doc collision detection.

This removes the separate collision detection pass (in cargo_doc.rs) and uses the global collision detection instead. This removes the separate pass for running the resolver (which resulted in running the resolver four times every time `cargo doc` was run).

The `--open` option needs to know which path to open, so this is passed in a roundabout way via `Compilation`. Since this method uses the root units, I added a sort on the roots so that the crate that is opened is consistent between runs. This results in some slight behavioral changes.

This also makes the collision check slightly more stringent. The test `doc_multiple_targets_same_name` now generates an error instead of a warning because the old code did not check for collisions between libs and bins across packages (which should be very rare).

src/cargo/core/compiler/compilation.rs

@@ -51,6 +51,9 @@ pub struct Compilation<'cfg> {
     /// An array of all cdylibs created.
     pub cdylibs: Vec<UnitOutput>,
 
+    /// The crate names of the root units specified on the command-line.
+    pub root_crate_names: Vec<String>,
+
     /// All directories for the output of native build commands.
     ///
     /// This is currently used to drive some entries which are added to the
@@ -136,6 +139,7 @@ impl<'cfg> Compilation<'cfg> {
             tests: Vec::new(),
             binaries: Vec::new(),
             cdylibs: Vec::new(),
+            root_crate_names: Vec::new(),
             extra_env: HashMap::new(),
             to_doc_test: Vec::new(),
             config: bcx.config,

src/cargo/core/compiler/context/mod.rs

@@ -2,7 +2,7 @@ use std::collections::{BTreeSet, HashMap, HashSet};
 use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
 
-use anyhow::Context as _;
+use anyhow::{bail, Context as _};
 use filetime::FileTime;
 use jobserver::Client;
 
@@ -309,6 +309,9 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         }
         self.primary_packages
             .extend(self.bcx.roots.iter().map(|u| u.pkg.package_id()));
+        self.compilation
+            .root_crate_names
+            .extend(self.bcx.roots.iter().map(|u| u.target.crate_name()));
 
         self.record_units_requiring_metadata();
 
@@ -480,6 +483,22 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             }
         };
 
+        fn doc_collision_error(unit: &Unit, other_unit: &Unit) -> CargoResult<()> {
+            bail!(
+                "document output filename collision\n\
+                 The {} `{}` in package `{}` has the same name as the {} `{}` in package `{}`.\n\
+                 Only one may be documented at once since they output to the same path.\n\
+                 Consider documenting only one, renaming one, \
+                 or marking one with `doc = false` in Cargo.toml.",
+                unit.target.kind().description(),
+                unit.target.name(),
+                unit.pkg,
+                other_unit.target.kind().description(),
+                other_unit.target.name(),
+                other_unit.pkg,
+            );
+        }
+
         let mut keys = self
             .bcx
             .unit_graph
@@ -494,6 +513,11 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                     if unit.mode.is_doc() {
                         // See https://github.com/rust-lang/rust/issues/56169
                         // and https://github.com/rust-lang/rust/issues/61378
+                        if self.is_primary_package(unit) {
+                            // This has been an error since before 1.0, so it
+                            // is not a warning like the other situations.
+                            doc_collision_error(unit, other_unit)?;
+                        }
                         report_collision(unit, other_unit, &output.path, rustdoc_suggestion)?;
                     } else {
                         report_collision(unit, other_unit, &output.path, suggestion)?;

src/cargo/ops/cargo_compile.rs

@@ -1092,6 +1092,9 @@ fn generate_targets(
     // It is computed by the set of enabled features for the package plus
     // every enabled feature of every enabled dependency.
     let mut features_map = HashMap::new();
+    // This needs to be a set to de-duplicate units. Due to the way the
+    // targets are filtered, it is possible to have duplicate proposals for
+    // the same thing.
     let mut units = HashSet::new();
     for Proposal {
         pkg,
@@ -1136,7 +1139,10 @@ fn generate_targets(
         }
         // else, silently skip target.
     }
-    Ok(units.into_iter().collect())
+    let mut units: Vec<_> = units.into_iter().collect();
+    // Keep the roots in a consistent order, which helps with checking test output.
+    units.sort_unstable();
+    Ok(units)
 }
 
 /// Warns if a target's required-features references a feature that doesn't exist.

src/cargo/ops/cargo_doc.rs

@@ -1,12 +1,11 @@
-use crate::core::resolver::HasDevUnits;
 use crate::core::{Shell, Workspace};
 use crate::ops;
+use crate::util::config::PathAndArgs;
 use crate::util::CargoResult;
-use crate::{core::compiler::RustcTargetData, util::config::PathAndArgs};
 use serde::Deserialize;
 use std::path::Path;
+use std::path::PathBuf;
 use std::process::Command;
-use std::{collections::HashMap, path::PathBuf};
 
 /// Strongly typed options for the `cargo doc` command.
 #[derive(Debug)]
@@ -26,64 +25,11 @@ struct CargoDocConfig {
 
 /// Main method for `cargo doc`.
 pub fn doc(ws: &Workspace<'_>, options: &DocOptions) -> CargoResult<()> {
-    let specs = options.compile_opts.spec.to_package_id_specs(ws)?;
-    let target_data = RustcTargetData::new(ws, &options.compile_opts.build_config.requested_kinds)?;
-    let ws_resolve = ops::resolve_ws_with_opts(
-        ws,
-        &target_data,
-        &options.compile_opts.build_config.requested_kinds,
-        &options.compile_opts.cli_features,
-        &specs,
-        HasDevUnits::No,
-        crate::core::resolver::features::ForceAllTargets::No,
-    )?;
-
-    let ids = ws_resolve.targeted_resolve.specs_to_ids(&specs)?;
-    let pkgs = ws_resolve.pkg_set.get_many(ids)?;
-
-    let mut lib_names = HashMap::new();
-    let mut bin_names = HashMap::new();
-    let mut names = Vec::new();
-    for package in &pkgs {
-        for target in package.targets().iter().filter(|t| t.documented()) {
-            if target.is_lib() {
-                if let Some(prev) = lib_names.insert(target.crate_name(), package) {
-                    anyhow::bail!(
-                        "The library `{}` is specified by packages `{}` and \
-                         `{}` but can only be documented once. Consider renaming \
-                         or marking one of the targets as `doc = false`.",
-                        target.crate_name(),
-                        prev,
-                        package
-                    );
-                }
-            } else if let Some(prev) = bin_names.insert(target.crate_name(), package) {
-                anyhow::bail!(
-                    "The binary `{}` is specified by packages `{}` and \
-                     `{}` but can be documented only once. Consider renaming \
-                     or marking one of the targets as `doc = false`.",
-                    target.crate_name(),
-                    prev,
-                    package
-                );
-            }
-            names.push(target.crate_name());
-        }
-    }
-
-    let open_kind = if options.open_result {
-        Some(options.compile_opts.build_config.single_requested_kind()?)
-    } else {
-        None
-    };
-
     let compilation = ops::compile(ws, &options.compile_opts)?;
 
-    if let Some(kind) = open_kind {
-        let name = match names.first() {
-            Some(s) => s.to_string(),
-            None => return Ok(()),
-        };
+    if options.open_result {
+        let name = &compilation.root_crate_names[0];
+        let kind = options.compile_opts.build_config.single_requested_kind()?;
         let path = compilation.root_output[&kind]
             .with_file_name("doc")
             .join(&name)

tests/testsuite/doc.rs

@@ -223,9 +223,15 @@ fn doc_multiple_targets_same_name_lib() {
 
     p.cargo("doc --workspace")
         .with_status(101)
-        .with_stderr_contains("[..] library `foo_lib` is specified [..]")
-        .with_stderr_contains("[..] `foo v0.1.0[..]` [..]")
-        .with_stderr_contains("[..] `bar v0.1.0[..]` [..]")
+        .with_stderr(
+            "\
+error: document output filename collision
+The lib `foo_lib` in package `foo v0.1.0 ([ROOT]/foo/foo)` has the same name as \
+the lib `foo_lib` in package `bar v0.1.0 ([ROOT]/foo/bar)`.
+Only one may be documented at once since they output to the same path.
+Consider documenting only one, renaming one, or marking one with `doc = false` in Cargo.toml.
+",
+        )
         .run();
 }
 
@@ -265,13 +271,17 @@ fn doc_multiple_targets_same_name() {
         .build();
 
     p.cargo("doc --workspace")
-        .with_stderr_contains("[DOCUMENTING] foo v0.1.0 ([CWD]/foo)")
-        .with_stderr_contains("[DOCUMENTING] bar v0.1.0 ([CWD]/bar)")
-        .with_stderr_contains("[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]")
+        .with_status(101)
+        .with_stderr(
+            "\
+error: document output filename collision
+The bin `foo_lib` in package `foo v0.1.0 ([ROOT]/foo/foo)` has the same name as \
+the lib `foo_lib` in package `bar v0.1.0 ([ROOT]/foo/bar)`.
+Only one may be documented at once since they output to the same path.
+Consider documenting only one, renaming one, or marking one with `doc = false` in Cargo.toml.
+",
+        )
         .run();
-    assert!(p.root().join("target/doc").is_dir());
-    let doc_file = p.root().join("target/doc/foo_lib/index.html");
-    assert!(doc_file.is_file());
 }
 
 #[cargo_test]
@@ -290,29 +300,31 @@ fn doc_multiple_targets_same_name_bin() {
                 [package]
                 name = "foo"
                 version = "0.1.0"
-                [[bin]]
-                name = "foo-cli"
             "#,
         )
-        .file("foo/src/foo-cli.rs", "")
+        .file("foo/src/bin/foo-cli.rs", "")
         .file(
             "bar/Cargo.toml",
             r#"
                 [package]
                 name = "bar"
                 version = "0.1.0"
-                [[bin]]
-                name = "foo-cli"
             "#,
         )
-        .file("bar/src/foo-cli.rs", "")
+        .file("bar/src/bin/foo-cli.rs", "")
         .build();
 
     p.cargo("doc --workspace")
         .with_status(101)
-        .with_stderr_contains("[..] binary `foo_cli` is specified [..]")
-        .with_stderr_contains("[..] `foo v0.1.0[..]` [..]")
-        .with_stderr_contains("[..] `bar v0.1.0[..]` [..]")
+        .with_stderr(
+            "\
+error: document output filename collision
+The bin `foo-cli` in package `foo v0.1.0 ([ROOT]/foo/foo)` has the same name as \
+the bin `foo-cli` in package `bar v0.1.0 ([ROOT]/foo/bar)`.
+Only one may be documented at once since they output to the same path.
+Consider documenting only one, renaming one, or marking one with `doc = false` in Cargo.toml.
+",
+        )
         .run();
 }
 
@@ -1152,7 +1164,7 @@ fn doc_workspace_open_help_message() {
         .env("BROWSER", "echo")
         .with_stderr_contains("[..] Documenting bar v0.1.0 ([..])")
         .with_stderr_contains("[..] Documenting foo v0.1.0 ([..])")
-        .with_stderr_contains("[..] Opening [..]/foo/index.html")
+        .with_stderr_contains("[..] Opening [..]/bar/index.html")
         .run();
 }
 
@@ -1378,7 +1390,7 @@ fn doc_private_ws() {
         .file("a/src/lib.rs", "fn p() {}")
         .file("b/Cargo.toml", &basic_manifest("b", "0.0.1"))
         .file("b/src/lib.rs", "fn p2() {}")
-        .file("b/src/main.rs", "fn main() {}")
+        .file("b/src/bin/b-cli.rs", "fn main() {}")
         .build();
     p.cargo("doc --workspace --bins --lib --document-private-items -v")
         .with_stderr_contains(
@@ -1388,7 +1400,7 @@ fn doc_private_ws() {
             "[RUNNING] `rustdoc [..] b/src/lib.rs [..]--document-private-items[..]",
         )
         .with_stderr_contains(
-            "[RUNNING] `rustdoc [..] b/src/main.rs [..]--document-private-items[..]",
+            "[RUNNING] `rustdoc [..] b/src/bin/b-cli.rs [..]--document-private-items[..]",
         )
         .run();
 }