Consolidate doc collision detection.

Author: ehuss

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();
 }