Auto merge of #6998 - ehuss:doc-duplicate-tracking, r=alexcrichton

Catch filename output collisions in rustdoc.

This does some general cleanup so that rustdoc output is computed correctly.  This allows the collision detection code to work.  There are some known issues with rustdoc creating collisions (rust-lang/rust#61378, rust-lang/rust#56169).  This is related to issue #6313.

This includes a change that `CompileMode::is_doc` no longer returns `true` for doc tests. This has bothered me for a while, as various points in the code was not handling it correctly. Separating it into `is_doc` and `is_doc_test` I think is better and more explicit.

Note for reviewing: There is a big diff in `calc_outputs`, but this is just rearranging code. Everything in `calc_outputs_rustc` is unchanged from the original.

The only functional changes should be:
- A warning is emitted for colliding rustdoc output.
- Don't create an empty fingerprint directory for doc tests.

Author: bors

src/cargo/core/compiler/build_config.rs

@@ -183,16 +183,19 @@ impl CompileMode {
         }
     }
 
-    /// Returns `true` if this is a doc or doc test. Be careful using this.
-    /// Although both run rustdoc, the dependencies for those two modes are
-    /// very different.
+    /// Returns `true` if this is generating documentation.
     pub fn is_doc(self) -> bool {
         match self {
-            CompileMode::Doc { .. } | CompileMode::Doctest => true,
+            CompileMode::Doc { .. } => true,
             _ => false,
         }
     }
 
+    /// Returns `true` if this a doc test.
+    pub fn is_doc_test(self) -> bool {
+        self == CompileMode::Doctest
+    }
+
     /// Returns `true` if this is any type of test (test, benchmark, doc test, or
     /// check test).
     pub fn is_any_test(self) -> bool {

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

@@ -9,7 +9,7 @@ use lazycell::LazyCell;
 use log::info;
 
 use super::{BuildContext, Context, FileFlavor, Kind, Layout};
-use crate::core::compiler::Unit;
+use crate::core::compiler::{CompileMode, Unit};
 use crate::core::{TargetKind, Workspace};
 use crate::util::{self, CargoResult};
 
@@ -146,6 +146,8 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
     pub fn out_dir(&self, unit: &Unit<'a>) -> PathBuf {
         if unit.mode.is_doc() {
             self.layout(unit.kind).root().parent().unwrap().join("doc")
+        } else if unit.mode.is_doc_test() {
+            panic!("doc tests do not have an out dir");
         } else if unit.target.is_custom_build() {
             self.build_script_dir(unit)
         } else if unit.target.is_example() {
@@ -293,107 +295,139 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
         unit: &Unit<'a>,
         bcx: &BuildContext<'a, 'cfg>,
     ) -> CargoResult<Arc<Vec<OutputFile>>> {
+        let ret = match unit.mode {
+            CompileMode::Check { .. } => {
+                // This may be confusing. rustc outputs a file named `lib*.rmeta`
+                // for both libraries and binaries.
+                let file_stem = self.file_stem(unit);
+                let path = self.out_dir(unit).join(format!("lib{}.rmeta", file_stem));
+                vec![OutputFile {
+                    path,
+                    hardlink: None,
+                    export_path: None,
+                    flavor: FileFlavor::Linkable { rmeta: false },
+                }]
+            }
+            CompileMode::Doc { .. } => {
+                let path = self
+                    .out_dir(unit)
+                    .join(unit.target.crate_name())
+                    .join("index.html");
+                vec![OutputFile {
+                    path,
+                    hardlink: None,
+                    export_path: None,
+                    flavor: FileFlavor::Normal,
+                }]
+            }
+            CompileMode::RunCustomBuild => {
+                // At this time, this code path does not handle build script
+                // outputs.
+                vec![]
+            }
+            CompileMode::Doctest => {
+                // Doctests are built in a temporary directory and then
+                // deleted. There is the `--persist-doctests` unstable flag,
+                // but Cargo does not know about that.
+                vec![]
+            }
+            CompileMode::Test | CompileMode::Build | CompileMode::Bench => {
+                self.calc_outputs_rustc(unit, bcx)?
+            }
+        };
+        info!("Target filenames: {:?}", ret);
+
+        Ok(Arc::new(ret))
+    }
+
+    fn calc_outputs_rustc(
+        &self,
+        unit: &Unit<'a>,
+        bcx: &BuildContext<'a, 'cfg>,
+    ) -> CargoResult<Vec<OutputFile>> {
+        let mut ret = Vec::new();
+        let mut unsupported = Vec::new();
+
         let out_dir = self.out_dir(unit);
-        let file_stem = self.file_stem(unit);
         let link_stem = self.link_stem(unit);
         let info = if unit.kind == Kind::Host {
             &bcx.host_info
         } else {
             &bcx.target_info
         };
+        let file_stem = self.file_stem(unit);
 
-        let mut ret = Vec::new();
-        let mut unsupported = Vec::new();
-        {
-            if unit.mode.is_check() {
-                // This may be confusing. rustc outputs a file named `lib*.rmeta`
-                // for both libraries and binaries.
-                let path = out_dir.join(format!("lib{}.rmeta", file_stem));
-                ret.push(OutputFile {
-                    path,
-                    hardlink: None,
-                    export_path: None,
-                    flavor: FileFlavor::Linkable { rmeta: false },
-                });
+        let mut add = |crate_type: &str, flavor: FileFlavor| -> CargoResult<()> {
+            let crate_type = if crate_type == "lib" {
+                "rlib"
             } else {
-                let mut add = |crate_type: &str, flavor: FileFlavor| -> CargoResult<()> {
-                    let crate_type = if crate_type == "lib" {
-                        "rlib"
-                    } else {
-                        crate_type
-                    };
-                    let file_types = info.file_types(
-                        crate_type,
-                        flavor,
-                        unit.target.kind(),
-                        bcx.target_triple(),
-                    )?;
-
-                    match file_types {
-                        Some(types) => {
-                            for file_type in types {
-                                let path = out_dir.join(file_type.filename(&file_stem));
-                                let hardlink = link_stem
-                                    .as_ref()
-                                    .map(|&(ref ld, ref ls)| ld.join(file_type.filename(ls)));
-                                let export_path = if unit.target.is_custom_build() {
-                                    None
-                                } else {
-                                    self.export_dir.as_ref().and_then(|export_dir| {
-                                        hardlink.as_ref().and_then(|hardlink| {
-                                            Some(export_dir.join(hardlink.file_name().unwrap()))
-                                        })
-                                    })
-                                };
-                                ret.push(OutputFile {
-                                    path,
-                                    hardlink,
-                                    export_path,
-                                    flavor: file_type.flavor,
-                                });
-                            }
-                        }
-                        // Not supported; don't worry about it.
-                        None => {
-                            unsupported.push(crate_type.to_string());
-                        }
-                    }
-                    Ok(())
-                };
-                // info!("{:?}", unit);
-                match *unit.target.kind() {
-                    TargetKind::Bin
-                    | TargetKind::CustomBuild
-                    | TargetKind::ExampleBin
-                    | TargetKind::Bench
-                    | TargetKind::Test => {
-                        add("bin", FileFlavor::Normal)?;
-                    }
-                    TargetKind::Lib(..) | TargetKind::ExampleLib(..) if unit.mode.is_any_test() => {
-                        add("bin", FileFlavor::Normal)?;
-                    }
-                    TargetKind::ExampleLib(ref kinds) | TargetKind::Lib(ref kinds) => {
-                        for kind in kinds {
-                            add(
-                                kind.crate_type(),
-                                if kind.linkable() {
-                                    FileFlavor::Linkable { rmeta: false }
-                                } else {
-                                    FileFlavor::Normal
-                                },
-                            )?;
-                        }
-                        let path = out_dir.join(format!("lib{}.rmeta", file_stem));
-                        if !unit.target.requires_upstream_objects() {
-                            ret.push(OutputFile {
-                                path,
-                                hardlink: None,
-                                export_path: None,
-                                flavor: FileFlavor::Linkable { rmeta: true },
-                            });
-                        }
+                crate_type
+            };
+            let file_types =
+                info.file_types(crate_type, flavor, unit.target.kind(), bcx.target_triple())?;
+
+            match file_types {
+                Some(types) => {
+                    for file_type in types {
+                        let path = out_dir.join(file_type.filename(&file_stem));
+                        let hardlink = link_stem
+                            .as_ref()
+                            .map(|&(ref ld, ref ls)| ld.join(file_type.filename(ls)));
+                        let export_path = if unit.target.is_custom_build() {
+                            None
+                        } else {
+                            self.export_dir.as_ref().and_then(|export_dir| {
+                                hardlink.as_ref().and_then(|hardlink| {
+                                    Some(export_dir.join(hardlink.file_name().unwrap()))
+                                })
+                            })
+                        };
+                        ret.push(OutputFile {
+                            path,
+                            hardlink,
+                            export_path,
+                            flavor: file_type.flavor,
+                        });
                     }
                 }
+                // Not supported; don't worry about it.
+                None => {
+                    unsupported.push(crate_type.to_string());
+                }
+            }
+            Ok(())
+        };
+        match *unit.target.kind() {
+            TargetKind::Bin
+            | TargetKind::CustomBuild
+            | TargetKind::ExampleBin
+            | TargetKind::Bench
+            | TargetKind::Test => {
+                add("bin", FileFlavor::Normal)?;
+            }
+            TargetKind::Lib(..) | TargetKind::ExampleLib(..) if unit.mode.is_any_test() => {
+                add("bin", FileFlavor::Normal)?;
+            }
+            TargetKind::ExampleLib(ref kinds) | TargetKind::Lib(ref kinds) => {
+                for kind in kinds {
+                    add(
+                        kind.crate_type(),
+                        if kind.linkable() {
+                            FileFlavor::Linkable { rmeta: false }
+                        } else {
+                            FileFlavor::Normal
+                        },
+                    )?;
+                }
+                let path = out_dir.join(format!("lib{}.rmeta", file_stem));
+                if !unit.target.requires_upstream_objects() {
+                    ret.push(OutputFile {
+                        path,
+                        hardlink: None,
+                        export_path: None,
+                        flavor: FileFlavor::Linkable { rmeta: true },
+                    });
+                }
             }
         }
         if ret.is_empty() {
@@ -413,9 +447,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
                 bcx.target_triple()
             );
         }
-        info!("Target filenames: {:?}", ret);
-
-        Ok(Arc::new(ret))
+        Ok(ret)
     }
 }
 
@@ -439,6 +471,10 @@ fn compute_metadata<'a, 'cfg>(
     cx: &Context<'a, 'cfg>,
     metas: &mut HashMap<Unit<'a>, Option<Metadata>>,
 ) -> Option<Metadata> {
+    if unit.mode.is_doc_test() {
+        // Doc tests do not have metadata.
+        return None;
+    }
     // No metadata for dylibs because of a couple issues:
     // - macOS encodes the dylib name in the executable,
     // - Windows rustc multiple files of which we can't easily link all of them.

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

@@ -166,7 +166,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                 }
             }
 
-            if unit.mode == CompileMode::Doctest {
+            if unit.mode.is_doc_test() {
                 // Note that we can *only* doc-test rlib outputs here. A
                 // staticlib output cannot be linked by the compiler (it just
                 // doesn't do that). A dylib output, however, can be linked by

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

@@ -129,7 +129,7 @@ fn compute_deps<'a, 'cfg, 'tmp>(
 ) -> CargoResult<Vec<(Unit<'a>, UnitFor)>> {
     if unit.mode.is_run_custom_build() {
         return compute_deps_custom_build(unit, state.cx.bcx);
-    } else if unit.mode.is_doc() && !unit.mode.is_any_test() {
+    } else if unit.mode.is_doc() {
         // Note: this does not include doc test.
         return compute_deps_doc(unit, state);
     }

src/cargo/core/compiler/fingerprint.rs

@@ -1015,6 +1015,8 @@ fn calculate<'a, 'cfg>(
     }
     let mut fingerprint = if unit.mode.is_run_custom_build() {
         calculate_run_custom_build(cx, unit)?
+    } else if unit.mode.is_doc_test() {
+        panic!("doc tests do not fingerprint");
     } else {
         calculate_normal(cx, unit)?
     };
@@ -1066,19 +1068,12 @@ fn calculate_normal<'a, 'cfg>(
 
     // Figure out what the outputs of our unit is, and we'll be storing them
     // into the fingerprint as well.
-    let outputs = if unit.mode.is_doc() {
-        vec![cx
-            .files()
-            .out_dir(unit)
-            .join(unit.target.crate_name())
-            .join("index.html")]
-    } else {
-        cx.outputs(unit)?
-            .iter()
-            .filter(|output| output.flavor != FileFlavor::DebugInfo)
-            .map(|output| output.path.clone())
-            .collect()
-    };
+    let outputs = cx
+        .outputs(unit)?
+        .iter()
+        .filter(|output| output.flavor != FileFlavor::DebugInfo)
+        .map(|output| output.path.clone())
+        .collect();
 
     // Fill out a bunch more information that we'll be tracking typically
     // hashed to take up less space on disk as we just need to know when things
@@ -1339,7 +1334,8 @@ fn write_fingerprint(loc: &Path, fingerprint: &Fingerprint) -> CargoResult<()> {
 pub fn prepare_init<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoResult<()> {
     let new1 = cx.files().fingerprint_dir(unit);
 
-    if fs::metadata(&new1).is_err() {
+    // Doc tests have no output, thus no fingerprint.
+    if !new1.exists() && !unit.mode.is_doc_test() {
         fs::create_dir(&new1)?;
     }
 

src/cargo/core/compiler/job_queue.rs

@@ -596,11 +596,10 @@ impl<'a, 'cfg> JobQueue<'a, 'cfg> {
             // being a compiled package.
             Dirty => {
                 if unit.mode.is_doc() {
+                    self.documented.insert(unit.pkg.package_id());
+                    config.shell().status("Documenting", unit.pkg)?;
+                } else if unit.mode.is_doc_test() {
                     // Skip doc test.
-                    if !unit.mode.is_any_test() {
-                        self.documented.insert(unit.pkg.package_id());
-                        config.shell().status("Documenting", unit.pkg)?;
-                    }
                 } else {
                     self.compiled.insert(unit.pkg.package_id());
                     if unit.mode.is_check() {
@@ -613,8 +612,7 @@ impl<'a, 'cfg> JobQueue<'a, 'cfg> {
             Fresh => {
                 // If doc test are last, only print "Fresh" if nothing has been printed.
                 if self.counts[&unit.pkg.package_id()] == 0
-                    && !(unit.mode == CompileMode::Doctest
-                        && self.compiled.contains(&unit.pkg.package_id()))
+                    && !(unit.mode.is_doc_test() && self.compiled.contains(&unit.pkg.package_id()))
                 {
                     self.compiled.insert(unit.pkg.package_id());
                     config.shell().verbose(|c| c.status("Fresh", unit.pkg))?;

src/cargo/core/compiler/mod.rs

@@ -125,7 +125,7 @@ fn compile<'a, 'cfg: 'a>(
 
     let job = if unit.mode.is_run_custom_build() {
         custom_build::prepare(cx, unit)?
-    } else if unit.mode == CompileMode::Doctest {
+    } else if unit.mode.is_doc_test() {
         // We run these targets later, so this is just a no-op for now.
         Job::new(Work::noop(), Freshness::Fresh)
     } else if build_plan {