Auto merge of rust-lang#116849 - oli-obk:error_shenanigans, r=cjgillot

Avoid a `track_errors` by bubbling up most errors from `check_well_formed`

I believe `track_errors` is mostly papering over issues that a sufficiently convoluted query graph can hit. I made this change, while the actual change I want to do is to stop bailing out early on errors, and instead use this new `ErrorGuaranteed` to invoke `check_well_formed` for individual items before doing all the `typeck` logic on them.

This works towards resolving rust-lang#97477 and various other ICEs, as well as allowing us to use parallel rustc more (which is currently rather limited/bottlenecked due to the very sequential nature in which we do `rustc_hir_analysis::check_crate`)

cc `@SparrowLii` `@Zoxc` for the new `try_par_for_each_in` function

Author: bors

compiler/rustc_data_structures/src/sync.rs

@@ -54,7 +54,7 @@ pub use worker_local::{Registry, WorkerLocal};
 mod parallel;
 #[cfg(parallel_compiler)]
 pub use parallel::scope;
-pub use parallel::{join, par_for_each_in, par_map, parallel_guard};
+pub use parallel::{join, par_for_each_in, par_map, parallel_guard, try_par_for_each_in};
 
 pub use std::sync::atomic::Ordering;
 pub use std::sync::atomic::Ordering::SeqCst;

compiler/rustc_data_structures/src/sync/parallel.rs

@@ -77,6 +77,15 @@ mod disabled {
         })
     }
 
+    pub fn try_par_for_each_in<T: IntoIterator, E: Copy>(
+        t: T,
+        mut for_each: impl FnMut(T::Item) -> Result<(), E>,
+    ) -> Result<(), E> {
+        parallel_guard(|guard| {
+            t.into_iter().fold(Ok(()), |ret, i| guard.run(|| for_each(i)).unwrap_or(ret).and(ret))
+        })
+    }
+
     pub fn par_map<T: IntoIterator, R, C: FromIterator<R>>(
         t: T,
         mut map: impl FnMut(<<T as IntoIterator>::IntoIter as Iterator>::Item) -> R,
@@ -167,6 +176,26 @@ mod enabled {
         });
     }
 
+    pub fn try_par_for_each_in<
+        T: IntoIterator + IntoParallelIterator<Item = <T as IntoIterator>::Item>,
+        E: Copy + Send,
+    >(
+        t: T,
+        for_each: impl Fn(<T as IntoIterator>::Item) -> Result<(), E> + DynSync + DynSend,
+    ) -> Result<(), E> {
+        parallel_guard(|guard| {
+            if mode::is_dyn_thread_safe() {
+                let for_each = FromDyn::from(for_each);
+                t.into_par_iter()
+                    .fold_with(Ok(()), |ret, i| guard.run(|| for_each(i)).unwrap_or(ret).and(ret))
+                    .reduce(|| Ok(()), |a, b| a.and(b))
+            } else {
+                t.into_iter()
+                    .fold(Ok(()), |ret, i| guard.run(|| for_each(i)).unwrap_or(ret).and(ret))
+            }
+        })
+    }
+
     pub fn par_map<
         I,
         T: IntoIterator<Item = I> + IntoParallelIterator<Item = I>,

compiler/rustc_hir_analysis/src/check/wfcheck.rs

@@ -205,10 +205,8 @@ pub fn check_crate(tcx: TyCtxt<'_>) -> Result<(), ErrorGuaranteed> {
         })?;
     }
 
-    tcx.sess.track_errors(|| {
-        tcx.sess.time("wf_checking", || {
-            tcx.hir().par_for_each_module(|module| tcx.ensure().check_mod_type_wf(module))
-        });
+    tcx.sess.time("wf_checking", || {
+        tcx.hir().try_par_for_each_module(|module| tcx.ensure().check_mod_type_wf(module))
     })?;
 
     // NOTE: This is copy/pasted in librustdoc/core.rs and should be kept in sync.

compiler/rustc_hir_analysis/src/lib.rs

@@ -114,6 +114,11 @@ struct QueryModifiers {
 
     /// Generate a `feed` method to set the query's value from another query.
     feedable: Option<Ident>,
+
+    /// Forward the result on ensure if the query gets recomputed, and
+    /// return `Ok(())` otherwise. Only applicable to queries returning
+    /// `Result<(), ErrorGuaranteed>`
+    ensure_forwards_result_if_red: Option<Ident>,
 }
 
 fn parse_query_modifiers(input: ParseStream<'_>) -> Result<QueryModifiers> {
@@ -128,6 +133,7 @@ fn parse_query_modifiers(input: ParseStream<'_>) -> Result<QueryModifiers> {
     let mut depth_limit = None;
     let mut separate_provide_extern = None;
     let mut feedable = None;
+    let mut ensure_forwards_result_if_red = None;
 
     while !input.is_empty() {
         let modifier: Ident = input.parse()?;
@@ -187,6 +193,8 @@ fn parse_query_modifiers(input: ParseStream<'_>) -> Result<QueryModifiers> {
             try_insert!(separate_provide_extern = modifier);
         } else if modifier == "feedable" {
             try_insert!(feedable = modifier);
+        } else if modifier == "ensure_forwards_result_if_red" {
+            try_insert!(ensure_forwards_result_if_red = modifier);
         } else {
             return Err(Error::new(modifier.span(), "unknown query modifier"));
         }
@@ -206,6 +214,7 @@ fn parse_query_modifiers(input: ParseStream<'_>) -> Result<QueryModifiers> {
         depth_limit,
         separate_provide_extern,
         feedable,
+        ensure_forwards_result_if_red,
     })
 }
 
@@ -325,6 +334,7 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream {
             eval_always,
             depth_limit,
             separate_provide_extern,
+            ensure_forwards_result_if_red,
         );
 
         if modifiers.cache.is_some() {

compiler/rustc_macros/src/query.rs

@@ -6,7 +6,7 @@ use rustc_ast as ast;
 use rustc_data_structures::fingerprint::Fingerprint;
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_data_structures::svh::Svh;
-use rustc_data_structures::sync::{par_for_each_in, DynSend, DynSync};
+use rustc_data_structures::sync::{par_for_each_in, try_par_for_each_in, DynSend, DynSync};
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{DefId, LocalDefId, LocalModDefId, LOCAL_CRATE};
 use rustc_hir::definitions::{DefKey, DefPath, DefPathData, DefPathHash};
@@ -16,7 +16,7 @@ use rustc_index::Idx;
 use rustc_middle::hir::nested_filter;
 use rustc_span::def_id::StableCrateId;
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
-use rustc_span::Span;
+use rustc_span::{ErrorGuaranteed, Span};
 use rustc_target::spec::abi::Abi;
 
 #[inline]
@@ -632,6 +632,17 @@ impl<'hir> Map<'hir> {
         })
     }
 
+    #[inline]
+    pub fn try_par_for_each_module(
+        self,
+        f: impl Fn(LocalModDefId) -> Result<(), ErrorGuaranteed> + DynSend + DynSync,
+    ) -> Result<(), ErrorGuaranteed> {
+        let crate_items = self.tcx.hir_crate_items(());
+        try_par_for_each_in(&crate_items.submodules[..], |module| {
+            f(LocalModDefId::new_unchecked(module.def_id))
+        })
+    }
+
     /// Returns an iterator for the nodes in the ancestor tree of the `current_id`
     /// until the crate root is reached. Prefer this over your own loop using `parent_id`.
     #[inline]

compiler/rustc_middle/src/hir/map/mod.rs

@@ -9,12 +9,12 @@ pub mod place;
 use crate::query::Providers;
 use crate::ty::{EarlyBinder, ImplSubject, TyCtxt};
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
-use rustc_data_structures::sync::{par_for_each_in, DynSend, DynSync};
+use rustc_data_structures::sync::{try_par_for_each_in, DynSend, DynSync};
 use rustc_hir::def::DefKind;
 use rustc_hir::def_id::{DefId, LocalDefId, LocalModDefId};
 use rustc_hir::*;
 use rustc_query_system::ich::StableHashingContext;
-use rustc_span::{ExpnId, DUMMY_SP};
+use rustc_span::{ErrorGuaranteed, ExpnId, DUMMY_SP};
 
 /// Top-level HIR node for current owner. This only contains the node for which
 /// `HirId::local_id == 0`, and excludes bodies.
@@ -78,20 +78,32 @@ impl ModuleItems {
         self.owners().map(|id| id.def_id)
     }
 
-    pub fn par_items(&self, f: impl Fn(ItemId) + DynSend + DynSync) {
-        par_for_each_in(&self.items[..], |&id| f(id))
+    pub fn par_items(
+        &self,
+        f: impl Fn(ItemId) -> Result<(), ErrorGuaranteed> + DynSend + DynSync,
+    ) -> Result<(), ErrorGuaranteed> {
+        try_par_for_each_in(&self.items[..], |&id| f(id))
     }
 
-    pub fn par_trait_items(&self, f: impl Fn(TraitItemId) + DynSend + DynSync) {
-        par_for_each_in(&self.trait_items[..], |&id| f(id))
+    pub fn par_trait_items(
+        &self,
+        f: impl Fn(TraitItemId) -> Result<(), ErrorGuaranteed> + DynSend + DynSync,
+    ) -> Result<(), ErrorGuaranteed> {
+        try_par_for_each_in(&self.trait_items[..], |&id| f(id))
     }
 
-    pub fn par_impl_items(&self, f: impl Fn(ImplItemId) + DynSend + DynSync) {
-        par_for_each_in(&self.impl_items[..], |&id| f(id))
+    pub fn par_impl_items(
+        &self,
+        f: impl Fn(ImplItemId) -> Result<(), ErrorGuaranteed> + DynSend + DynSync,
+    ) -> Result<(), ErrorGuaranteed> {
+        try_par_for_each_in(&self.impl_items[..], |&id| f(id))
     }
 
-    pub fn par_foreign_items(&self, f: impl Fn(ForeignItemId) + DynSend + DynSync) {
-        par_for_each_in(&self.foreign_items[..], |&id| f(id))
+    pub fn par_foreign_items(
+        &self,
+        f: impl Fn(ForeignItemId) -> Result<(), ErrorGuaranteed> + DynSend + DynSync,
+    ) -> Result<(), ErrorGuaranteed> {
+        try_par_for_each_in(&self.foreign_items[..], |&id| f(id))
     }
 }
 

compiler/rustc_middle/src/hir/mod.rs

@@ -25,7 +25,9 @@ use crate::mir::interpret::{
 use crate::mir::interpret::{LitToConstError, LitToConstInput};
 use crate::mir::mono::CodegenUnit;
 use crate::query::erase::{erase, restore, Erase};
-use crate::query::plumbing::{query_ensure, query_get_at, CyclePlaceholder, DynamicQuery};
+use crate::query::plumbing::{
+    query_ensure, query_ensure_error_guaranteed, query_get_at, CyclePlaceholder, DynamicQuery,
+};
 use crate::thir;
 use crate::traits::query::{
     CanonicalPredicateGoal, CanonicalProjectionGoal, CanonicalTyGoal,
@@ -965,8 +967,9 @@ rustc_queries! {
         desc { |tcx| "checking that impls are well-formed in {}", describe_as_module(key, tcx) }
     }
 
-    query check_mod_type_wf(key: LocalModDefId) -> () {
+    query check_mod_type_wf(key: LocalModDefId) -> Result<(), ErrorGuaranteed> {
         desc { |tcx| "checking that types are well-formed in {}", describe_as_module(key, tcx) }
+        ensure_forwards_result_if_red
     }
 
     query collect_mod_item_types(key: LocalModDefId) -> () {
@@ -1499,8 +1502,9 @@ rustc_queries! {
         feedable
     }
 
-    query check_well_formed(key: hir::OwnerId) -> () {
+    query check_well_formed(key: hir::OwnerId) -> Result<(), ErrorGuaranteed> {
         desc { |tcx| "checking that `{}` is well-formed", tcx.def_path_str(key) }
+        ensure_forwards_result_if_red
     }
 
     // The `DefId`s of all non-generic functions and statics in the given crate

compiler/rustc_middle/src/query/mod.rs

@@ -173,6 +173,45 @@ pub fn query_ensure<'tcx, Cache>(
     }
 }
 
+#[inline]
+pub fn query_ensure_error_guaranteed<'tcx, Cache>(
+    tcx: TyCtxt<'tcx>,
+    execute_query: fn(TyCtxt<'tcx>, Span, Cache::Key, QueryMode) -> Option<Cache::Value>,
+    query_cache: &Cache,
+    key: Cache::Key,
+    check_cache: bool,
+) -> Result<(), ErrorGuaranteed>
+where
+    Cache: QueryCache<Value = super::erase::Erase<Result<(), ErrorGuaranteed>>>,
+{
+    let key = key.into_query_param();
+    if let Some(res) = try_get_cached(tcx, query_cache, &key) {
+        super::erase::restore(res)
+    } else {
+        execute_query(tcx, DUMMY_SP, key, QueryMode::Ensure { check_cache })
+            .map(super::erase::restore)
+            // Either we actually executed the query, which means we got a full `Result`,
+            // or we can just assume the query succeeded, because it was green in the
+            // incremental cache. If it is green, that means that the previous compilation
+            // that wrote to the incremental cache compiles successfully. That is only
+            // possible if the cache entry was `Ok(())`, so we emit that here, without
+            // actually encoding the `Result` in the cache or loading it from there.
+            .unwrap_or(Ok(()))
+    }
+}
+
+macro_rules! query_ensure {
+    ([]$($args:tt)*) => {
+        query_ensure($($args)*)
+    };
+    ([(ensure_forwards_result_if_red) $($rest:tt)*]$($args:tt)*) => {
+        query_ensure_error_guaranteed($($args)*)
+    };
+    ([$other:tt $($modifiers:tt)*]$($args:tt)*) => {
+        query_ensure!([$($modifiers)*]$($args)*)
+    };
+}
+
 macro_rules! query_helper_param_ty {
     (DefId) => { impl IntoQueryParam<DefId> };
     (LocalDefId) => { impl IntoQueryParam<LocalDefId> };
@@ -220,6 +259,18 @@ macro_rules! separate_provide_extern_decl {
     };
 }
 
+macro_rules! ensure_result {
+    ([][$ty:ty]) => {
+        ()
+    };
+    ([(ensure_forwards_result_if_red) $($rest:tt)*][$ty:ty]) => {
+        Result<(), ErrorGuaranteed>
+    };
+    ([$other:tt $($modifiers:tt)*][$($args:tt)*]) => {
+        ensure_result!([$($modifiers)*][$($args)*])
+    };
+}
+
 macro_rules! separate_provide_extern_default {
     ([][$name:ident]) => {
         ()
@@ -343,14 +394,15 @@ macro_rules! define_callbacks {
         impl<'tcx> TyCtxtEnsure<'tcx> {
             $($(#[$attr])*
             #[inline(always)]
-            pub fn $name(self, key: query_helper_param_ty!($($K)*)) {
-                query_ensure(
+            pub fn $name(self, key: query_helper_param_ty!($($K)*)) -> ensure_result!([$($modifiers)*][$V]) {
+                query_ensure!(
+                    [$($modifiers)*]
                     self.tcx,
                     self.tcx.query_system.fns.engine.$name,
                     &self.tcx.query_system.caches.$name,
                     key.into_query_param(),
                     false,
-                );
+                )
             })*
         }
 

compiler/rustc_middle/src/query/plumbing.rs

@@ -2247,7 +2247,7 @@ where
 
 /// Useful type to use with `Result<>` indicate that an error has already
 /// been reported to the user, so no need to continue checking.
-#[derive(Clone, Copy, Debug, Encodable, Decodable, Hash, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)]
 #[derive(HashStable_Generic)]
 pub struct ErrorGuaranteed(());
 
@@ -2259,3 +2259,20 @@ impl ErrorGuaranteed {
         ErrorGuaranteed(())
     }
 }
+
+impl<E: rustc_serialize::Encoder> Encodable<E> for ErrorGuaranteed {
+    #[inline]
+    fn encode(&self, _e: &mut E) {
+        panic!(
+            "should never serialize an `ErrorGuaranteed`, as we do not write metadata or incremental caches in case errors occurred"
+        )
+    }
+}
+impl<D: rustc_serialize::Decoder> Decodable<D> for ErrorGuaranteed {
+    #[inline]
+    fn decode(_d: &mut D) -> ErrorGuaranteed {
+        panic!(
+            "`ErrorGuaranteed` should never have been serialized to metadata or incremental caches"
+        )
+    }
+}

compiler/rustc_span/src/lib.rs

@@ -24,16 +24,6 @@ help: you might be missing a type parameter
 LL | impl<N, M, VAL> TypeVal<usize> for Multiply<N, M> where N: TypeVal<VAL> {}
    |          +++++
 
-error[E0046]: not all trait items implemented, missing: `VAL`
-  --> $DIR/ice-6252.rs:11:1
-   |
-LL |     const VAL: T;
-   |     ------------ `VAL` from trait
-...
-LL | impl<N, M> TypeVal<usize> for Multiply<N, M> where N: TypeVal<VAL> {}
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `VAL` in implementation
-
-error: aborting due to 3 previous errors
+error: aborting due to 2 previous errors
 
-Some errors have detailed explanations: E0046, E0412.
-For more information about an error, try `rustc --explain E0046`.
+For more information about this error, try `rustc --explain E0412`.

src/tools/clippy/tests/ui/crashes/ice-6252.stderr

@@ -9,13 +9,10 @@ impl TraitWAssocConst for impl Demo { //~ ERROR E0404
 }
 
 fn foo<A: TraitWAssocConst<A=32>>() { //~ ERROR E0658
-    foo::<Demo>()(); //~ ERROR E0271
-    //~^ ERROR E0618
-    //~| ERROR E0277
+    foo::<Demo>()();
 }
 
-fn main<A: TraitWAssocConst<A=32>>() { //~ ERROR E0131
+fn main<A: TraitWAssocConst<A=32>>() {
     //~^ ERROR E0658
-    foo::<Demo>(); //~ ERROR E0277
-    //~^ ERROR E0271
+    foo::<Demo>();
 }