Rollup merge of #139502 - yaahc:still-mutable-ice, r=bjorn3

fix "still mutable" ice while metrics are enabled

Resolves "still mutable" ICE discovered by `@matthiaskrgr` here: [#t-docs-rs > metrics intitiative @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/356853-t-docs-rs/topic/metrics.20intitiative/near/510490790)

This was caused by invoking `crate_hash` before the `definitions` struct was frozen here: e643f59f6d/compiler/rustc_interface/src/passes.rs (L951)

resolved by moving metrics dumping to occur after `analysis` freezes the definitions

I'm guessing we didn't discover this in CI because the problem only occurs when you try to calculate the crash hash with incremental compilation enabled when it tries to freeze the definitions here: e643f59f6d/compiler/rustc_middle/src/hir/map.rs (L1172)

my understanding is that this causes us to freeze the definitions too early in compilation, then we subsequently try to mutate them, likely during `analysis`, and this causes the ICE.

r? `@bjorn3`

compiler/rustc_data_structures/src/sync/freeze.rs

@@ -88,7 +88,7 @@ impl<T> FreezeLock<T> {
     #[inline]
     #[track_caller]
     pub fn write(&self) -> FreezeWriteGuard<'_, T> {
-        self.try_write().expect("still mutable")
+        self.try_write().expect("data should not be frozen if we're still attempting to mutate it")
     }
 
     #[inline]

compiler/rustc_driver_impl/src/lib.rs

@@ -348,10 +348,6 @@ pub fn run_compiler(at_args: &[String], callbacks: &mut (dyn Callbacks + Send))
             // Make sure name resolution and macro expansion is run.
             let _ = tcx.resolver_for_lowering();
 
-            if let Some(metrics_dir) = &sess.opts.unstable_opts.metrics_dir {
-                dump_feature_usage_metrics(tcx, metrics_dir);
-            }
-
             if callbacks.after_expansion(compiler, tcx) == Compilation::Stop {
                 return early_exit();
             }
@@ -370,6 +366,10 @@ pub fn run_compiler(at_args: &[String], callbacks: &mut (dyn Callbacks + Send))
 
             tcx.ensure_ok().analysis(());
 
+            if let Some(metrics_dir) = &sess.opts.unstable_opts.metrics_dir {
+                dump_feature_usage_metrics(tcx, metrics_dir);
+            }
+
             if callbacks.after_analysis(compiler, tcx) == Compilation::Stop {
                 return early_exit();
             }

compiler/rustc_middle/src/query/mod.rs

@@ -1900,6 +1900,11 @@ rustc_queries! {
 
     // The macro which defines `rustc_metadata::provide_extern` depends on this query's name.
     // Changing the name should cause a compiler error, but in case that changes, be aware.
+    //
+    // The hash should not be calculated before the `analysis` pass is complete, specifically
+    // until `tcx.untracked().definitions.freeze()` has been called, otherwise if incremental
+    // compilation is enabled calculating this hash can freeze this structure too early in
+    // compilation and cause subsequent crashes when attempting to write to `definitions`
     query crate_hash(_: CrateNum) -> Svh {
         eval_always
         desc { "looking up the hash a crate" }

tests/run-make/unstable-feature-usage-metrics-incremental/main.rs

@@ -0,0 +1,16 @@
+#![feature(ascii_char)] // random lib feature
+#![feature(box_patterns)] // random lang feature
+
+// picked arbitrary unstable features, just need a random lib and lang feature, ideally ones that
+// won't be stabilized any time soon so we don't have to update this test
+fn main() {
+    for s in quix("foo/bar") {
+        print!("{s}");
+    }
+    println!();
+}
+
+// need a latebound var to trigger the incremental compilation ICE
+fn quix(foo: &str) -> impl Iterator<Item = &'_ str> + '_ {
+    foo.split('/')
+}

tests/run-make/unstable-feature-usage-metrics-incremental/rmake.rs

@@ -0,0 +1,94 @@
+//! This test checks if unstable feature usage metric dump files `unstable-feature-usage*.json` work
+//! as expected.
+//!
+//! - Basic sanity checks on a default ICE dump.
+//!
+//! See <https://github.com/rust-lang/rust/issues/129485>.
+//!
+//! # Test history
+//!
+//! - forked from dump-ice-to-disk test, which has flakeyness issues on i686-mingw, I'm assuming
+//! those will be present in this test as well on the same platform
+
+//@ ignore-windows
+//FIXME(#128911): still flakey on i686-mingw.
+
+use std::path::{Path, PathBuf};
+
+use run_make_support::rfs::create_dir_all;
+use run_make_support::{
+    cwd, filename_contains, has_extension, rfs, run_in_tmpdir, rustc, serde_json,
+    shallow_find_files,
+};
+
+fn find_feature_usage_metrics<P: AsRef<Path>>(dir: P) -> Vec<PathBuf> {
+    shallow_find_files(dir, |path| {
+        if filename_contains(path, "unstable_feature_usage") && has_extension(path, "json") {
+            true
+        } else {
+            dbg!(path);
+            false
+        }
+    })
+}
+
+fn main() {
+    test_metrics_dump();
+    test_metrics_errors();
+}
+
+#[track_caller]
+fn test_metrics_dump() {
+    run_in_tmpdir(|| {
+        let metrics_dir = cwd().join("metrics");
+        create_dir_all(&metrics_dir);
+        rustc()
+            .input("main.rs")
+            .incremental("incremental")
+            .env("RUST_BACKTRACE", "short")
+            .arg(format!("-Zmetrics-dir={}", metrics_dir.display()))
+            .run();
+        let mut metrics = find_feature_usage_metrics(&metrics_dir);
+        let json_path =
+            metrics.pop().expect("there should be one metrics file in the output directory");
+
+        // After the `pop` above, there should be no files left.
+        assert!(
+            metrics.is_empty(),
+            "there should be no more than one metrics file in the output directory"
+        );
+
+        let message = rfs::read_to_string(json_path);
+        let mut parsed: serde_json::Value =
+            serde_json::from_str(&message).expect("metrics should be dumped as json");
+        // remove timestamps
+        assert!(parsed["lib_features"][0]["timestamp"].is_number());
+        assert!(parsed["lang_features"][0]["timestamp"].is_number());
+        parsed["lib_features"][0]["timestamp"] = serde_json::json!(null);
+        parsed["lang_features"][0]["timestamp"] = serde_json::json!(null);
+        let expected = serde_json::json!(
+            {
+                "lib_features":[{"symbol":"ascii_char", "timestamp":null}],
+                "lang_features":[{"symbol":"box_patterns","since":null, "timestamp":null}]
+            }
+        );
+
+        assert_eq!(expected, parsed);
+    });
+}
+
+#[track_caller]
+fn test_metrics_errors() {
+    run_in_tmpdir(|| {
+        rustc()
+            .input("main.rs")
+            .incremental("incremental")
+            .env("RUST_BACKTRACE", "short")
+            .arg("-Zmetrics-dir=invaliddirectorythatdefinitelydoesntexist")
+            .run_fail()
+            .assert_stderr_contains(
+                "error: cannot dump feature usage metrics: No such file or directory",
+            )
+            .assert_stdout_not_contains("internal compiler error");
+    });
+}