Rollup merge of #157282 - xmakro:fix/parallel-pme-note, r=petrochenkov

Fix post-monomorphization error note race in the parallel frontend

Fixes #154260. Part of #154314.

`collect_items_rec` decides whether to attach the note "the above error was encountered while instantiating `fn ...`" by snapshotting the error count before processing a mono item and comparing it afterwards. The mono item graph is walked in parallel (the `par_for_each_in` in `collect_crate_mono_items`), so the global error count can be bumped by an error emitted while collecting a different item on another thread between the two reads. That makes an unrelated item get blamed, which showed up as a spurious note when using parallel frontend:

```
note: the above error was encountered while instantiating `fn std::rt::lang_start::<()>`
```

This PR records the thread that emits each error alongside its `ErrorGuaranteed` in `err_guars`, `lint_err_guars`, and the stashed-diagnostics map, and add `err_count_on_current_thread`, which counts only the errors whose recorded thread is the calling thread. The collector uses it for the snapshot and comparison, so the delta is not affected by errors emitted on other threads. In serial compilation it is equivalent to a delta of `err_count`.

Author: jhpratt

compiler/rustc_errors/src/lib.rs

@@ -24,6 +24,7 @@ use std::io::Write;
 use std::num::NonZero;
 use std::ops::DerefMut;
 use std::path::{Path, PathBuf};
+use std::thread::ThreadId;
 use std::{assert_matches, fmt, panic};
 
 use Level::*;
@@ -297,11 +298,12 @@ impl<'a> std::ops::Deref for DiagCtxtHandle<'a> {
 struct DiagCtxtInner {
     flags: DiagCtxtFlags,
 
-    /// The error guarantees from all emitted errors. The length gives the error count.
-    err_guars: Vec<ErrorGuaranteed>,
-    /// The error guarantee from all emitted lint errors. The length gives the
-    /// lint error count.
-    lint_err_guars: Vec<ErrorGuaranteed>,
+    /// The error guarantees from all emitted errors, each paired with the
+    /// thread that emitted it. The length gives the error count.
+    err_guars: Vec<(ErrorGuaranteed, ThreadId)>,
+    /// The error guarantee from all emitted lint errors, each paired with the
+    /// thread that emitted it. The length gives the lint error count.
+    lint_err_guars: Vec<(ErrorGuaranteed, ThreadId)>,
     /// The delayed bugs and their error guarantees.
     delayed_bugs: Vec<(DelayedDiagInner, ErrorGuaranteed)>,
 
@@ -343,7 +345,7 @@ struct DiagCtxtInner {
     /// `emit_stashed_diagnostics` by the time the `DiagCtxtInner` is dropped,
     /// otherwise an assertion failure will occur.
     stashed_diagnostics:
-        FxIndexMap<StashKey, FxIndexMap<Span, (DiagInner, Option<ErrorGuaranteed>)>>,
+        FxIndexMap<StashKey, FxIndexMap<Span, (DiagInner, Option<ErrorGuaranteed>, ThreadId)>>,
 
     future_breakage_diagnostics: Vec<DiagInner>,
 
@@ -613,7 +615,7 @@ impl<'a> DiagCtxtHandle<'a> {
             .stashed_diagnostics
             .entry(key)
             .or_default()
-            .insert(span.with_parent(None), (diag, guar));
+            .insert(span.with_parent(None), (diag, guar, std::thread::current().id()));
 
         guar
     }
@@ -623,7 +625,7 @@ impl<'a> DiagCtxtHandle<'a> {
     /// error.
     pub fn steal_non_err(self, span: Span, key: StashKey) -> Option<Diag<'a, ()>> {
         // FIXME(#120456) - is `swap_remove` correct?
-        let (diag, guar) = self.inner.borrow_mut().stashed_diagnostics.get_mut(&key).and_then(
+        let (diag, guar, _) = self.inner.borrow_mut().stashed_diagnostics.get_mut(&key).and_then(
             |stashed_diagnostics| stashed_diagnostics.swap_remove(&span.with_parent(None)),
         )?;
         assert!(!diag.is_error());
@@ -648,7 +650,7 @@ impl<'a> DiagCtxtHandle<'a> {
         let err = self.inner.borrow_mut().stashed_diagnostics.get_mut(&key).and_then(
             |stashed_diagnostics| stashed_diagnostics.swap_remove(&span.with_parent(None)),
         );
-        err.map(|(err, guar)| {
+        err.map(|(err, guar, _)| {
             // The use of `::<ErrorGuaranteed>` is safe because level is `Level::Error`.
             assert_eq!(err.level, Error);
             assert!(guar.is_some());
@@ -673,7 +675,7 @@ impl<'a> DiagCtxtHandle<'a> {
             |stashed_diagnostics| stashed_diagnostics.swap_remove(&span.with_parent(None)),
         );
         match old_err {
-            Some((old_err, guar)) => {
+            Some((old_err, guar, _)) => {
                 assert_eq!(old_err.level, Error);
                 assert!(guar.is_some());
                 // Because `old_err` has already been counted, it can only be
@@ -710,7 +712,27 @@ impl<'a> DiagCtxtHandle<'a> {
             + inner
                 .stashed_diagnostics
                 .values()
-                .map(|a| a.values().filter(|(_, guar)| guar.is_some()).count())
+                .map(|a| a.values().filter(|(_, guar, _)| guar.is_some()).count())
+                .sum::<usize>()
+    }
+
+    /// The number of errors that have been emitted on the *current thread*.
+    ///
+    /// Like [`DiagCtxtHandle::err_count`], but only counts errors whose recorded
+    /// emitting thread is the calling thread.
+    pub fn err_count_on_current_thread(&self) -> usize {
+        let inner = self.inner.borrow();
+        let current = std::thread::current().id();
+        inner.err_guars.iter().filter(|(_, thread)| *thread == current).count()
+            + inner.lint_err_guars.iter().filter(|(_, thread)| *thread == current).count()
+            + inner
+                .stashed_diagnostics
+                .values()
+                .map(|a| {
+                    a.values()
+                        .filter(|(_, guar, thread)| guar.is_some() && *thread == current)
+                        .count()
+                })
                 .sum::<usize>()
     }
 
@@ -879,7 +901,8 @@ impl<'a> DiagCtxtHandle<'a> {
             // This `unchecked_error_guaranteed` is valid. It is where the
             // `ErrorGuaranteed` for unused_extern errors originates.
             #[allow(deprecated)]
-            inner.lint_err_guars.push(ErrorGuaranteed::unchecked_error_guaranteed());
+            let guar = ErrorGuaranteed::unchecked_error_guaranteed();
+            inner.lint_err_guars.push((guar, std::thread::current().id()));
             inner.panic_if_treat_err_as_bug();
         }
 
@@ -1178,7 +1201,7 @@ impl DiagCtxtInner {
         let mut guar = None;
         let has_errors = !self.err_guars.is_empty();
         for (_, stashed_diagnostics) in std::mem::take(&mut self.stashed_diagnostics).into_iter() {
-            for (_, (diag, _guar)) in stashed_diagnostics {
+            for (_, (diag, _guar, _thread)) in stashed_diagnostics {
                 if !diag.is_error() {
                     // Unless they're forced, don't flush stashed warnings when
                     // there are errors, to avoid causing warning overload. The
@@ -1347,13 +1370,14 @@ impl DiagCtxtInner {
                 // `ErrorGuaranteed` for errors and lint errors originates.
                 #[allow(deprecated)]
                 let guar = ErrorGuaranteed::unchecked_error_guaranteed();
+                let thread = std::thread::current().id();
                 if is_lint {
-                    self.lint_err_guars.push(guar);
+                    self.lint_err_guars.push((guar, thread));
                 } else {
                     if let Some(taint) = taint {
                         taint.set(Some(guar));
                     }
-                    self.err_guars.push(guar);
+                    self.err_guars.push((guar, thread));
                 }
                 self.panic_if_treat_err_as_bug();
                 Some(guar)
@@ -1377,12 +1401,12 @@ impl DiagCtxtInner {
     }
 
     fn has_errors_excluding_lint_errors(&self) -> Option<ErrorGuaranteed> {
-        self.err_guars.get(0).copied().or_else(|| {
-            if let Some((_diag, guar)) = self
+        self.err_guars.get(0).map(|(guar, _)| *guar).or_else(|| {
+            if let Some((_diag, guar, _)) = self
                 .stashed_diagnostics
                 .values()
                 .flat_map(|stashed_diagnostics| stashed_diagnostics.values())
-                .find(|(diag, guar)| guar.is_some() && diag.is_lint.is_none())
+                .find(|(diag, guar, _)| guar.is_some() && diag.is_lint.is_none())
             {
                 *guar
             } else {
@@ -1392,13 +1416,15 @@ impl DiagCtxtInner {
     }
 
     fn has_errors(&self) -> Option<ErrorGuaranteed> {
-        self.err_guars.get(0).copied().or_else(|| self.lint_err_guars.get(0).copied()).or_else(
-            || {
+        self.err_guars
+            .get(0)
+            .map(|(guar, _)| *guar)
+            .or_else(|| self.lint_err_guars.get(0).map(|(guar, _)| *guar))
+            .or_else(|| {
                 self.stashed_diagnostics.values().find_map(|stashed_diagnostics| {
-                    stashed_diagnostics.values().find_map(|(_, guar)| *guar)
+                    stashed_diagnostics.values().find_map(|(_, guar, _)| *guar)
                 })
-            },
-        )
+            })
     }
 
     fn has_errors_or_delayed_bugs(&self) -> Option<ErrorGuaranteed> {

compiler/rustc_monomorphize/src/collector.rs

@@ -413,7 +413,7 @@ fn collect_items_rec<'tcx>(
     // current step of mono items collection.
     //
     // FIXME: don't rely on global state, instead bubble up errors. Note: this is very hard to do.
-    let error_count = tcx.dcx().err_count();
+    let error_count = tcx.dcx().err_count_on_current_thread();
 
     // In `mentioned_items` we collect items that were mentioned in this MIR but possibly do not
     // need to be monomorphized. This is done to ensure that optimizing away function calls does not
@@ -538,7 +538,7 @@ fn collect_items_rec<'tcx>(
 
     // Check for PMEs and emit a diagnostic if one happened. To try to show relevant edges of the
     // mono item graph.
-    if tcx.dcx().err_count() > error_count
+    if tcx.dcx().err_count_on_current_thread() > error_count
         && starting_item.node.is_generic_fn()
         && starting_item.node.is_user_defined()
     {

tests/ui/abi/simd-abi-checks-avx.rs

@@ -1,7 +1,7 @@
 //@ only-x86_64
 //@ build-fail
 //@ compile-flags: -C target-feature=-avx
-//@ ignore-parallel-frontend post-monomorphization errors
+
 #![feature(portable_simd)]
 #![feature(simd_ffi)]
 #![allow(improper_ctypes_definitions)]

tests/ui/consts/const-eval/index-out-of-bounds-never-type.rs

@@ -1,5 +1,5 @@
 //@ build-fail
-//@ ignore-parallel-frontend post-monomorphization errors
+
 // Regression test for #66975
 #![warn(unconditional_panic)]
 #![feature(never_type)]

tests/ui/consts/const-eval/issue-50814-2.rs

@@ -2,7 +2,7 @@
 //@ revisions: normal mir-opt
 //@ [mir-opt]compile-flags: -Zmir-opt-level=4
 //@ dont-require-annotations: NOTE
-//@ ignore-parallel-frontend post-monomorphization errors
+
 trait C {
     const BOO: usize;
 }

tests/ui/consts/const-eval/issue-50814.rs

@@ -1,6 +1,6 @@
 //@ build-fail
 //@ dont-require-annotations: NOTE
-//@ ignore-parallel-frontend post-monomorphization errors
+
 trait Unsigned {
     const MAX: u8;
 }

tests/ui/consts/mono-reachable-invalid-const.rs

@@ -1,5 +1,5 @@
 //@ build-fail
-//@ ignore-parallel-frontend post-monomorphization errors
+
 struct Bar<const BITS: usize>;
 
 impl<const BITS: usize> Bar<BITS> {

tests/ui/consts/required-consts/collect-in-called-fn.rs

@@ -4,7 +4,7 @@
 //@[opt] compile-flags: -O
 //! Make sure we detect erroneous constants post-monomorphization even when they are unused. This is
 //! crucial, people rely on it for soundness. (https://github.com/rust-lang/rust/issues/112090)
-//@ ignore-parallel-frontend post-monomorphization errors
+
 struct Fail<T>(T);
 impl<T> Fail<T> {
     const C: () = panic!(); //~ERROR evaluation panicked: explicit panic

tests/ui/consts/required-consts/collect-in-dead-closure.rs

@@ -3,7 +3,7 @@
 //@[noopt] compile-flags: -Copt-level=0
 //@[opt] compile-flags: -O
 //! This fails without optimizations, so it should also fail with optimizations.
-//@ ignore-parallel-frontend post-monomorphization errors
+
 struct Fail<T>(T);
 impl<T> Fail<T> {
     const C: () = panic!(); //~ERROR evaluation panicked: explicit panic

tests/ui/consts/required-consts/collect-in-dead-drop.rs

@@ -3,7 +3,7 @@
 //@[noopt] compile-flags: -Copt-level=0
 //@[opt] compile-flags: -O
 //! This fails without optimizations, so it should also fail with optimizations.
-//@ ignore-parallel-frontend post-monomorphization errors
+
 struct Fail<T>(T);
 impl<T> Fail<T> {
     const C: () = panic!(); //~ERROR evaluation panicked: explicit panic