Auto merge of #11450 - willcrichton:example-analyzer, r=weihanglo

Allow Check targets needed for optional doc-scraping to fail without killing the build

### What does this PR try to resolve?

In doing a Crater run of -Zrustdoc-scrape-examples, I found that the only remaining regressions are cases where:
* A library does not type-check
* The library has examples
* Cargo tries to scrape the examples, which requires checking the library
* The Check unit for the library fails, crashing the build

The core issue is that the Check unit should be able to fail without killing the build. This PR fixes this issue by checking for this condition, and then allowing the unit to fail.

Specifically, I added a new method `BuildContext::unit_can_fail_for_docscraping` that determines the conditions for whether a unit is allowed to fail. This method is used both in `JobQueue` to interpret process failure, and in the `rustc`/`rustdoc` functions to emit a warning upon failure. I modified `rustc` to handle the case of failure similar to `rustdoc`, but with a slightly different diagnostic.

### How should we test and review this PR?

The unit test `no_fail_bad_lib` has been extended with example files to test this case.

r? `@weihanglo`

Author: bors

src/cargo/core/compiler/job_queue.rs

@@ -812,10 +812,7 @@ impl<'cfg> DrainState<'cfg> {
                 debug!("end ({:?}): {:?}", unit, result);
                 match result {
                     Ok(()) => self.finish(id, &unit, artifact, cx)?,
-                    Err(_)
-                        if unit.mode.is_doc_scrape()
-                            && unit.target.doc_scrape_examples().is_unset() =>
-                    {
+                    Err(_) if cx.bcx.unit_can_fail_for_docscraping(&unit) => {
                         cx.failed_scrape_units
                             .lock()
                             .unwrap()

src/cargo/core/compiler/mod.rs

@@ -25,6 +25,7 @@ pub mod unit_graph;
 use std::collections::{HashMap, HashSet};
 use std::env;
 use std::ffi::{OsStr, OsString};
+use std::fmt::Display;
 use std::fs::{self, File};
 use std::io::{BufRead, Write};
 use std::path::{Path, PathBuf};
@@ -207,6 +208,27 @@ fn compile<'cfg>(
     Ok(())
 }
 
+/// Generates the warning message used when fallible doc-scrape units fail,
+/// either for rustdoc or rustc.
+fn make_failed_scrape_diagnostic(
+    cx: &Context<'_, '_>,
+    unit: &Unit,
+    top_line: impl Display,
+) -> String {
+    let manifest_path = unit.pkg.manifest_path();
+    let relative_manifest_path = manifest_path
+        .strip_prefix(cx.bcx.ws.root())
+        .unwrap_or(&manifest_path);
+
+    format!(
+        "\
+{top_line}
+    Try running with `--verbose` to see the error message.
+    If an example or library should not be scanned, then consider adding `doc-scrape-examples = false` to its `[[example]]` or `[lib]` definition in {}",
+        relative_manifest_path.display()
+    )
+}
+
 fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc<dyn Executor>) -> CargoResult<Work> {
     let mut rustc = prepare_rustc(cx, &unit.target.rustc_crate_types(), unit)?;
     let build_plan = cx.bcx.build_config.build_plan;
@@ -265,6 +287,26 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc<dyn Executor>) -> Car
     let is_local = unit.is_local();
     let artifact = unit.artifact;
 
+    let hide_diagnostics_for_scrape_unit = cx.bcx.unit_can_fail_for_docscraping(unit)
+        && !matches!(cx.bcx.config.shell().verbosity(), Verbosity::Verbose);
+    let failed_scrape_diagnostic = hide_diagnostics_for_scrape_unit.then(|| {
+        // If this unit is needed for doc-scraping, then we generate a diagnostic that
+        // describes the set of reverse-dependencies that cause the unit to be needed.
+        let target_desc = unit.target.description_named();
+        let mut for_scrape_units = cx
+            .bcx
+            .scrape_units_have_dep_on(unit)
+            .into_iter()
+            .map(|unit| unit.target.description_named())
+            .collect::<Vec<_>>();
+        for_scrape_units.sort();
+        let for_scrape_units = for_scrape_units.join(", ");
+        make_failed_scrape_diagnostic(cx, unit, format_args!("failed to check {target_desc} in package `{name}` as a prerequisite for scraping examples from: {for_scrape_units}"))
+    });
+    if hide_diagnostics_for_scrape_unit {
+        output_options.show_diagnostics = false;
+    }
+
     return Ok(Work::new(move |state| {
         // Artifacts are in a different location than typical units,
         // hence we must assure the crate- and target-dependent
@@ -339,38 +381,48 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc<dyn Executor>) -> Car
         if build_plan {
             state.build_plan(buildkey, rustc.clone(), outputs.clone());
         } else {
-            exec.exec(
-                &rustc,
-                package_id,
-                &target,
-                mode,
-                &mut |line| on_stdout_line(state, line, package_id, &target),
-                &mut |line| {
-                    on_stderr_line(
-                        state,
-                        line,
-                        package_id,
-                        &manifest_path,
-                        &target,
-                        &mut output_options,
-                    )
-                },
-            )
-            .map_err(verbose_if_simple_exit_code)
-            .with_context(|| {
-                // adapted from rustc_errors/src/lib.rs
-                let warnings = match output_options.warnings_seen {
-                    0 => String::new(),
-                    1 => "; 1 warning emitted".to_string(),
-                    count => format!("; {} warnings emitted", count),
-                };
-                let errors = match output_options.errors_seen {
-                    0 => String::new(),
-                    1 => " due to previous error".to_string(),
-                    count => format!(" due to {} previous errors", count),
-                };
-                format!("could not compile `{}`{}{}", name, errors, warnings)
-            })?;
+            let result = exec
+                .exec(
+                    &rustc,
+                    package_id,
+                    &target,
+                    mode,
+                    &mut |line| on_stdout_line(state, line, package_id, &target),
+                    &mut |line| {
+                        on_stderr_line(
+                            state,
+                            line,
+                            package_id,
+                            &manifest_path,
+                            &target,
+                            &mut output_options,
+                        )
+                    },
+                )
+                .map_err(verbose_if_simple_exit_code)
+                .with_context(|| {
+                    // adapted from rustc_errors/src/lib.rs
+                    let warnings = match output_options.warnings_seen {
+                        0 => String::new(),
+                        1 => "; 1 warning emitted".to_string(),
+                        count => format!("; {} warnings emitted", count),
+                    };
+                    let errors = match output_options.errors_seen {
+                        0 => String::new(),
+                        1 => " due to previous error".to_string(),
+                        count => format!(" due to {} previous errors", count),
+                    };
+                    format!("could not compile `{}`{}{}", name, errors, warnings)
+                });
+
+            if let Err(e) = result {
+                if let Some(diagnostic) = failed_scrape_diagnostic {
+                    state.warning(diagnostic)?;
+                }
+
+                return Err(e);
+            }
+
             // Exec should never return with success *and* generate an error.
             debug_assert_eq!(output_options.errors_seen, 0);
         }
@@ -713,20 +765,24 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Work> {
     let build_script_outputs = Arc::clone(&cx.build_script_outputs);
     let package_id = unit.pkg.package_id();
     let manifest_path = PathBuf::from(unit.pkg.manifest_path());
-    let relative_manifest_path = manifest_path
-        .strip_prefix(cx.bcx.ws.root())
-        .unwrap_or(&manifest_path)
-        .to_owned();
     let target = Target::clone(&unit.target);
     let mut output_options = OutputOptions::new(cx, unit);
     let script_metadata = cx.find_build_script_metadata(unit);
+
     let failed_scrape_units = Arc::clone(&cx.failed_scrape_units);
-    let hide_diagnostics_for_scrape_unit = unit.mode.is_doc_scrape()
-        && unit.target.doc_scrape_examples().is_unset()
+    let hide_diagnostics_for_scrape_unit = cx.bcx.unit_can_fail_for_docscraping(unit)
         && !matches!(cx.bcx.config.shell().verbosity(), Verbosity::Verbose);
+    let failed_scrape_diagnostic = hide_diagnostics_for_scrape_unit.then(|| {
+        make_failed_scrape_diagnostic(
+            cx,
+            unit,
+            format_args!("failed to scan {target_desc} in package `{name}` for example code usage"),
+        )
+    });
     if hide_diagnostics_for_scrape_unit {
         output_options.show_diagnostics = false;
     }
+
     Ok(Work::new(move |state| {
         add_custom_flags(
             &mut rustdoc,
@@ -774,15 +830,8 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Work> {
             .with_context(|| format!("could not document `{}`", name));
 
         if let Err(e) = result {
-            if hide_diagnostics_for_scrape_unit {
-                let diag = format!(
-                    "\
-failed to scan {target_desc} in package `{name}` for example code usage
-    Try running with `--verbose` to see the error message.
-    If this example should not be scanned, consider adding `doc-scrape-examples = false` to the `[[example]]` definition in {}",
-                    relative_manifest_path.display()
-                );
-                state.warning(diag)?;
+            if let Some(diagnostic) = failed_scrape_diagnostic {
+                state.warning(diagnostic)?;
             }
 
             return Err(e);

src/cargo/core/compiler/rustdoc.rs

@@ -2,7 +2,7 @@
 
 use crate::core::compiler::context::Context;
 use crate::core::compiler::unit::Unit;
-use crate::core::compiler::CompileKind;
+use crate::core::compiler::{BuildContext, CompileKind};
 use crate::sources::CRATES_IO_REGISTRY;
 use crate::util::errors::{internal, CargoResult};
 use cargo_util::ProcessBuilder;
@@ -209,3 +209,40 @@ impl RustdocScrapeExamples {
         matches!(self, RustdocScrapeExamples::Unset)
     }
 }
+
+impl BuildContext<'_, '_> {
+    /// Returns the set of Docscrape units that have a direct dependency on `unit`
+    pub fn scrape_units_have_dep_on<'a>(&'a self, unit: &'a Unit) -> Vec<&'a Unit> {
+        self.scrape_units
+            .iter()
+            .filter(|scrape_unit| {
+                self.unit_graph[scrape_unit]
+                    .iter()
+                    .any(|dep| &dep.unit == unit)
+            })
+            .collect::<Vec<_>>()
+    }
+
+    /// Returns true if this unit is needed for doing doc-scraping and is also
+    /// allowed to fail without killing the build.
+    pub fn unit_can_fail_for_docscraping(&self, unit: &Unit) -> bool {
+        // If the unit is not a Docscrape unit, e.g. a Lib target that is
+        // checked to scrape an Example target, then we need to get the doc-scrape-examples
+        // configuration for the reverse-dependent Example target.
+        let for_scrape_units = if unit.mode.is_doc_scrape() {
+            vec![unit]
+        } else {
+            self.scrape_units_have_dep_on(unit)
+        };
+
+        if for_scrape_units.is_empty() {
+            false
+        } else {
+            // All Docscrape units must have doc-scrape-examples unset. If any are true,
+            // then the unit is not allowed to fail.
+            for_scrape_units
+                .iter()
+                .all(|unit| unit.target.doc_scrape_examples().is_unset())
+        }
+    }
+}

tests/testsuite/docscrape.rs

@@ -332,17 +332,32 @@ fn no_fail_bad_lib() {
             "#,
         )
         .file("src/lib.rs", "pub fn foo() { CRASH_THE_BUILD() }")
+        .file("examples/ex.rs", "fn main() { foo::foo(); }")
+        .file("examples/ex2.rs", "fn main() { foo::foo(); }")
         .build();
 
     p.cargo("doc -Zunstable-options -Z rustdoc-scrape-examples")
         .masquerade_as_nightly_cargo(&["rustdoc-scrape-examples"])
-        .with_stderr(
+        .with_stderr_unordered(
         "\
+[CHECKING] foo v0.0.1 ([CWD])
 [SCRAPING] foo v0.0.1 ([CWD])
 warning: failed to scan lib in package `foo` for example code usage
     Try running with `--verbose` to see the error message.
-    If this example should not be scanned, consider adding `doc-scrape-examples = false` to the `[[example]]` definition in Cargo.toml
+    If an example or library should not be scanned, then consider adding `doc-scrape-examples = false` to its `[[example]]` or `[lib]` definition in Cargo.toml
 warning: `foo` (lib) generated 1 warning
+warning: failed to check lib in package `foo` as a prerequisite for scraping examples from: example \"ex\", example \"ex2\"
+    Try running with `--verbose` to see the error message.
+    If an example or library should not be scanned, then consider adding `doc-scrape-examples = false` to its `[[example]]` or `[lib]` definition in Cargo.toml
+warning: `foo` (lib) generated 1 warning
+warning: failed to scan example \"ex\" in package `foo` for example code usage
+    Try running with `--verbose` to see the error message.
+    If an example or library should not be scanned, then consider adding `doc-scrape-examples = false` to its `[[example]]` or `[lib]` definition in Cargo.toml
+warning: `foo` (example \"ex\") generated 1 warning
+warning: failed to scan example \"ex2\" in package `foo` for example code usage
+    Try running with `--verbose` to see the error message.
+    If an example or library should not be scanned, then consider adding `doc-scrape-examples = false` to its `[[example]]` or `[lib]` definition in Cargo.toml
+warning: `foo` (example \"ex2\") generated 1 warning
 [DOCUMENTING] foo v0.0.1 ([CWD])
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]",
     )
@@ -374,7 +389,7 @@ fn no_fail_bad_example() {
 [SCRAPING] foo v0.0.1 ([CWD])
 warning: failed to scan example \"ex1\" in package `foo` for example code usage
     Try running with `--verbose` to see the error message.
-    If this example should not be scanned, consider adding `doc-scrape-examples = false` to the `[[example]]` definition in Cargo.toml
+    If an example or library should not be scanned, then consider adding `doc-scrape-examples = false` to its `[[example]]` or `[lib]` definition in Cargo.toml
 warning: `foo` (example \"ex1\") generated 1 warning
 [DOCUMENTING] foo v0.0.1 ([CWD])
 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]",