Auto merge of #7450 - ehuss:stabilize-cache-messages, r=alexcrichton

Stabilize cache-messages

This stabilizes the -Zcache-messages feature, making it always enabled.

## What is stabilized?

This feature is intended to redisplay previous warnings on a "fresh" build instead of displaying no output.

Users have occasionally indicated frustration when they know there are warnings, but no output is displayed when the build is fresh. This also improves the interaction between `cargo check` and `cargo clippy-preview`. This also simplifies the code, and opens more possibilities for `rustc` to send side-channel messages to Cargo.

Cargo will now use JSON output from `rustc` and `rustdoc` 100% of the time (`rustdoc --test` does not use JSON). Previously Cargo would only use JSON for pipelined crates.

Cargo will save the JSON output into a file named `output` in the `.fingerprint` directory. This file is only created when the compiler outputs a diagnostic message.

If a crate is being recompiled, and Cargo considers it to be "fresh", it will replay the output file to the console.

## Notable changes in this PR

- Fixed a bug where replays were erroneously including pipeline rmeta artifact json messages.
- clippy-preview is now included in the metadata hash, to force its artifacts to be separate from `cargo check`.
- clippy-preview is no longer force-enabled, under the assumption that caching and fingerprinting is accurate, and the cached messages will be replayed.
- clippy-preview's arguments are included in the fingerprint now that it is not force-enabled.
- Rustdoc colors and short messages were fixed when pipelining was stabilized, so updated tests.

Closes #6986
Closes #6848
Closes #6664
Closes #2458

## Concerns

The only notable issue with this is that switching between short and long human messages only replays the format from the first invocation.  That is, if you do `cargo build` and it generates warnings, then running again with `--message-format=short` will still show the full length human messages. I'm personally fine with that behavior, even though it is not ideal. I think this feature overall improves the situation (where before *no* output was displayed). Being able to re-render between short/long is a very difficult problem, and unlikely to be fixable in the foreseeable future.

There was some concern expressed about being able to disable this. I think that would only be necessary if a severe bug is discovered. I do not feel that this change is risky enough to warrant a configurable option. If it does cause a problem, it can be quickly reverted with a one-line change to set `OutputOptions::cache_cell` to `None`. Since pipelining has been using JSON output for a while now without complaints, I feel pretty confident in it.

src/bin/cargo/cli.rs

@@ -35,7 +35,6 @@ Available unstable (nightly-only) flags:
     -Z unstable-options -- Allow the usage of unstable options such as --registry
     -Z config-profile   -- Read profiles from .cargo/config files
     -Z install-upgrade  -- `cargo install` will upgrade instead of failing
-    -Z cache-messages   -- Cache compiler messages
     -Z timings          -- Display concurrency information
     -Z doctest-xcompile -- Compile and run doctests for non-host target using runner config
 

src/bin/cargo/commands/clippy.rs

@@ -79,7 +79,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
     }
 
     compile_opts.build_config.primary_unit_rustc = Some(wrapper);
-    compile_opts.build_config.force_rebuild = true;
 
     ops::compile(&ws, &compile_opts)?;
     Ok(())

src/cargo/core/compiler/build_config.rs

@@ -43,8 +43,6 @@ pub struct BuildConfig {
     /// An optional override of the rustc path for primary units only
     pub primary_unit_rustc: Option<ProcessBuilder>,
     pub rustfix_diagnostic_server: RefCell<Option<RustfixDiagnosticServer>>,
-    /// Whether or not Cargo should cache compiler output on disk.
-    cache_messages: bool,
 }
 
 impl BuildConfig {
@@ -101,15 +99,9 @@ impl BuildConfig {
             build_plan: false,
             primary_unit_rustc: None,
             rustfix_diagnostic_server: RefCell::new(None),
-            cache_messages: config.cli_unstable().cache_messages,
         })
     }
 
-    /// Whether or not Cargo should cache compiler messages on disk.
-    pub fn cache_messages(&self) -> bool {
-        self.cache_messages
-    }
-
     /// Whether or not the *user* wants JSON output. Whether or not rustc
     /// actually uses JSON is decided in `add_error_format`.
     pub fn emit_json(&self) -> bool {

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

@@ -556,6 +556,14 @@ fn compute_metadata<'a, 'cfg>(
 
     bcx.rustc.verbose_version.hash(&mut hasher);
 
+    if cx.is_primary_package(unit) {
+        // This is primarily here for clippy. This ensures that the clippy
+        // artifacts are separate from the `check` ones.
+        if let Some(proc) = &cx.bcx.build_config.primary_unit_rustc {
+            proc.get_program().hash(&mut hasher);
+        }
+    }
+
     // Seed the contents of `__CARGO_DEFAULT_LIB_METADATA` to the hasher if present.
     // This should be the release channel, to get a different hash for each channel.
     if let Ok(ref channel) = __cargo_default_lib_metadata {

src/cargo/core/compiler/fingerprint.rs

@@ -1083,11 +1083,23 @@ fn calculate_normal<'a, 'cfg>(
     // 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
     // change.
-    let extra_flags = if unit.mode.is_doc() {
+    let mut extra_flags = if unit.mode.is_doc() {
         cx.bcx.rustdocflags_args(unit)
     } else {
         cx.bcx.rustflags_args(unit)
-    };
+    }
+    .to_vec();
+    if cx.is_primary_package(unit) {
+        // This is primarily here for clippy arguments.
+        if let Some(proc) = &cx.bcx.build_config.primary_unit_rustc {
+            let args = proc
+                .get_args()
+                .iter()
+                .map(|s| s.to_string_lossy().to_string());
+            extra_flags.extend(args);
+        }
+    }
+
     let profile_hash = util::hash_u64((&unit.profile, unit.mode, cx.bcx.extra_args_for(unit)));
     // Include metadata since it is exposed as environment variables.
     let m = unit.pkg.manifest().metadata();
@@ -1104,7 +1116,7 @@ fn calculate_normal<'a, 'cfg>(
         local: Mutex::new(local),
         memoized_hash: Mutex::new(None),
         metadata,
-        rustflags: extra_flags.to_vec(),
+        rustflags: extra_flags,
         fs_status: FsStatus::Stale,
         outputs,
     })

src/cargo/core/compiler/layout.rs

@@ -36,6 +36,9 @@
 //!                 # Detailed information used for logging the reason why
 //!                 # something is being recompiled.
 //!                 lib-$pkgname-$META.json
+//!                 # The console output from the compiler. This is cached
+//!                 # so that warnings can be redisplayed for "fresh" units.
+//!                 output
 //!
 //!         # This is the root directory for all rustc artifacts except build
 //!         # scripts, examples, and test and bench executables. Almost every

src/cargo/core/compiler/mod.rs

@@ -136,9 +136,7 @@ fn compile<'a, 'cfg: 'a>(
             };
             work.then(link_targets(cx, unit, false)?)
         } else {
-            let work = if cx.bcx.build_config.cache_messages()
-                && cx.bcx.show_warnings(unit.pkg.package_id())
-            {
+            let work = if cx.bcx.show_warnings(unit.pkg.package_id()) {
                 replay_output_cache(
                     unit.pkg.package_id(),
                     unit.target,
@@ -663,79 +661,31 @@ fn add_cap_lints(bcx: &BuildContext<'_, '_>, unit: &Unit<'_>, cmd: &mut ProcessB
 
 /// Add error-format flags to the command.
 ///
-/// This is somewhat odd right now, but the general overview is that if
-/// `-Zcache-messages` or `pipelined` is enabled then Cargo always uses JSON
-/// output. This has several benefits, such as being easier to parse, handles
-/// changing formats (for replaying cached messages), ensures atomic output (so
-/// messages aren't interleaved), etc.
-///
-/// It is intended in the future that Cargo *always* uses the JSON output (by
-/// turning on cache-messages by default), and this function can be simplified.
+/// Cargo always uses JSON output. This has several benefits, such as being
+/// easier to parse, handles changing formats (for replaying cached messages),
+/// ensures atomic output (so messages aren't interleaved), allows for
+/// intercepting messages like rmeta artifacts, etc. rustc includes a
+/// "rendered" field in the JSON message with the message properly formatted,
+/// which Cargo will extract and display to the user.
 fn add_error_format_and_color(
     cx: &Context<'_, '_>,
     cmd: &mut ProcessBuilder,
     pipelined: bool,
 ) -> CargoResult<()> {
-    // If this unit is producing a required rmeta file then we need to know
-    // when the rmeta file is ready so we can signal to the rest of Cargo that
-    // it can continue dependent compilations. To do this we are currently
-    // required to switch the compiler into JSON message mode, but we still
-    // want to present human readable errors as well. (this rabbit hole just
-    // goes and goes)
-    //
-    // All that means is that if we're not already in JSON mode we need to
-    // switch to JSON mode, ensure that rustc error messages can be rendered
-    // prettily, and then when parsing JSON messages from rustc we need to
-    // internally understand that we should extract the `rendered` field and
-    // present it if we can.
-    if cx.bcx.build_config.cache_messages() || pipelined {
-        cmd.arg("--error-format=json");
-        let mut json = String::from("--json=diagnostic-rendered-ansi");
-        if pipelined {
-            json.push_str(",artifacts");
-        }
-        match cx.bcx.build_config.message_format {
-            MessageFormat::Short | MessageFormat::Json { short: true, .. } => {
-                json.push_str(",diagnostic-short");
-            }
-            _ => {}
-        }
-        cmd.arg(json);
-    } else {
-        let mut color = true;
-        match cx.bcx.build_config.message_format {
-            MessageFormat::Human => (),
-            MessageFormat::Json {
-                ansi,
-                short,
-                render_diagnostics,
-            } => {
-                cmd.arg("--error-format").arg("json");
-                // If ansi is explicitly requested, enable it. If we're
-                // rendering diagnostics ourselves then also enable it because
-                // we'll figure out what to do with the colors later.
-                if ansi || render_diagnostics {
-                    cmd.arg("--json=diagnostic-rendered-ansi");
-                }
-                if short {
-                    cmd.arg("--json=diagnostic-short");
-                }
-                color = false;
-            }
-            MessageFormat::Short => {
-                cmd.arg("--error-format").arg("short");
-            }
-        }
-
-        if color {
-            let color = if cx.bcx.config.shell().supports_color() {
-                "always"
-            } else {
-                "never"
-            };
-            cmd.args(&["--color", color]);
+    cmd.arg("--error-format=json");
+    let mut json = String::from("--json=diagnostic-rendered-ansi");
+    if pipelined {
+        // Pipelining needs to know when rmeta files are finished. Tell rustc
+        // to emit a message that cargo will intercept.
+        json.push_str(",artifacts");
+    }
+    match cx.bcx.build_config.message_format {
+        MessageFormat::Short | MessageFormat::Json { short: true, .. } => {
+            json.push_str(",diagnostic-short");
         }
+        _ => {}
     }
+    cmd.arg(json);
     Ok(())
 }
 
@@ -1058,22 +1008,19 @@ struct OutputOptions {
     color: bool,
     /// Where to write the JSON messages to support playback later if the unit
     /// is fresh. The file is created lazily so that in the normal case, lots
-    /// of empty files are not created. This is None if caching is disabled.
+    /// of empty files are not created. If this is None, the output will not
+    /// be cached (such as when replaying cached messages).
     cache_cell: Option<(PathBuf, LazyCell<File>)>,
 }
 
 impl OutputOptions {
     fn new<'a>(cx: &Context<'a, '_>, unit: &Unit<'a>) -> OutputOptions {
         let look_for_metadata_directive = cx.rmeta_required(unit);
         let color = cx.bcx.config.shell().supports_color();
-        let cache_cell = if cx.bcx.build_config.cache_messages() {
-            let path = cx.files().message_cache_path(unit);
-            // Remove old cache, ignore ENOENT, which is the common case.
-            drop(fs::remove_file(&path));
-            Some((path, LazyCell::new()))
-        } else {
-            None
-        };
+        let path = cx.files().message_cache_path(unit);
+        // Remove old cache, ignore ENOENT, which is the common case.
+        drop(fs::remove_file(&path));
+        let cache_cell = Some((path, LazyCell::new()));
         OutputOptions {
             format: cx.bcx.build_config.message_format,
             look_for_metadata_directive,
@@ -1100,23 +1047,35 @@ fn on_stderr_line(
     target: &Target,
     options: &mut OutputOptions,
 ) -> CargoResult<()> {
-    // Check if caching is enabled.
-    if let Some((path, cell)) = &mut options.cache_cell {
-        // Cache the output, which will be replayed later when Fresh.
-        let f = cell.try_borrow_mut_with(|| File::create(path))?;
-        debug_assert!(!line.contains('\n'));
-        f.write_all(line.as_bytes())?;
-        f.write_all(&[b'\n'])?;
+    if on_stderr_line_inner(state, line, package_id, target, options)? {
+        // Check if caching is enabled.
+        if let Some((path, cell)) = &mut options.cache_cell {
+            // Cache the output, which will be replayed later when Fresh.
+            let f = cell.try_borrow_mut_with(|| File::create(path))?;
+            debug_assert!(!line.contains('\n'));
+            f.write_all(line.as_bytes())?;
+            f.write_all(&[b'\n'])?;
+        }
     }
+    Ok(())
+}
 
+/// Returns true if the line should be cached.
+fn on_stderr_line_inner(
+    state: &JobState<'_>,
+    line: &str,
+    package_id: PackageId,
+    target: &Target,
+    options: &mut OutputOptions,
+) -> CargoResult<bool> {
     // We primarily want to use this function to process JSON messages from
     // rustc. The compiler should always print one JSON message per line, and
     // otherwise it may have other output intermingled (think RUST_LOG or
     // something like that), so skip over everything that doesn't look like a
     // JSON message.
     if !line.starts_with('{') {
         state.stderr(line.to_string());
-        return Ok(());
+        return Ok(true);
     }
 
     let mut compiler_message: Box<serde_json::value::RawValue> = match serde_json::from_str(line) {
@@ -1128,7 +1087,7 @@ fn on_stderr_line(
         Err(e) => {
             debug!("failed to parse json: {:?}", e);
             state.stderr(line.to_string());
-            return Ok(());
+            return Ok(true);
         }
     };
 
@@ -1164,14 +1123,13 @@ fn on_stderr_line(
                         .expect("strip should never fail")
                 };
                 state.stderr(rendered);
-                return Ok(());
+                return Ok(true);
             }
         }
 
-        // Remove color information from the rendered string. When pipelining is
-        // enabled and/or when cached messages are enabled we're always asking
-        // for ANSI colors from rustc, so unconditionally postprocess here and
-        // remove ansi color codes.
+        // Remove color information from the rendered string if color is not
+        // enabled. Cargo always asks for ANSI colors from rustc. This allows
+        // cached replay to enable/disable colors without re-invoking rustc.
         MessageFormat::Json { ansi: false, .. } => {
             #[derive(serde::Deserialize, serde::Serialize)]
             struct CompilerMessage {
@@ -1213,7 +1171,7 @@ fn on_stderr_line(
                 log::debug!("looks like metadata finished early!");
                 state.rmeta_produced();
             }
-            return Ok(());
+            return Ok(false);
         }
     }
 
@@ -1231,7 +1189,7 @@ fn on_stderr_line(
     // instead. We want the stdout of Cargo to always be machine parseable as
     // stderr has our colorized human-readable messages.
     state.stdout(msg);
-    Ok(())
+    Ok(true)
 }
 
 fn replay_output_cache(
@@ -1244,7 +1202,7 @@ fn replay_output_cache(
     let target = target.clone();
     let mut options = OutputOptions {
         format,
-        look_for_metadata_directive: false,
+        look_for_metadata_directive: true,
         color,
         cache_cell: None,
     };

src/cargo/core/features.rs

@@ -335,7 +335,6 @@ pub struct CliUnstable {
     pub dual_proc_macros: bool,
     pub mtime_on_use: bool,
     pub install_upgrade: bool,
-    pub cache_messages: bool,
     pub named_profiles: bool,
     pub binary_dep_depinfo: bool,
     pub build_std: Option<Vec<String>>,
@@ -393,7 +392,6 @@ impl CliUnstable {
             // can also be set in .cargo/config or with and ENV
             "mtime-on-use" => self.mtime_on_use = true,
             "install-upgrade" => self.install_upgrade = true,
-            "cache-messages" => self.cache_messages = true,
             "named-profiles" => self.named_profiles = true,
             "binary-dep-depinfo" => self.binary_dep_depinfo = true,
             "build-std" => {

src/doc/src/reference/unstable.md

@@ -23,11 +23,11 @@ index each time.
 * Original Issue: [#6477](https://github.com/rust-lang/cargo/pull/6477)
 * Cache usage meta tracking issue: [#7150](https://github.com/rust-lang/cargo/issues/7150)
 
-The `-Z mtime-on-use` flag is an experiment to have Cargo update the mtime of 
-used files to make it easier for tools like cargo-sweep to detect which files 
+The `-Z mtime-on-use` flag is an experiment to have Cargo update the mtime of
+used files to make it easier for tools like cargo-sweep to detect which files
 are stale. For many workflows this needs to be set on *all* invocations of cargo.
 To make this more practical setting the `unstable.mtime_on_use` flag in `.cargo/config`
-or the corresponding ENV variable will apply the `-Z mtime-on-use` to all 
+or the corresponding ENV variable will apply the `-Z mtime-on-use` to all
 invocations of nightly cargo. (the config flag is ignored by stable)
 
 ### avoid-dev-deps
@@ -323,25 +323,6 @@ my_dep = { version = "1.2.3", public = true }
 private_dep = "2.0.0" # Will be 'private' by default
 ```
 
-### cache-messages
-* Tracking Issue: [#6986](https://github.com/rust-lang/cargo/issues/6986)
-
-The `cache-messages` feature causes Cargo to cache the messages generated by
-the compiler. This is primarily useful if a crate compiles successfully with
-warnings. Previously, re-running Cargo would not display any output. With the
-`cache-messages` feature, it will quickly redisplay the previous warnings.
-
-```
-cargo +nightly check -Z cache-messages
-```
-
-This works with any command that runs the compiler (`build`, `check`, `test`,
-etc.).
-
-This also changes the way Cargo interacts with the compiler, helping to
-prevent interleaved messages when multiple crates attempt to display a message
-at the same time.
-
 ### build-std
 * Tracking Repository: https://github.com/rust-lang/wg-cargo-std-aware