Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid a track_errors by bubbling up most errors from check_well_formed #116849

Merged
merged 3 commits into from
Oct 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_data_structures/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
29 changes: 29 additions & 0 deletions compiler/rustc_data_structures/src/sync/parallel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -167,6 +176,26 @@ mod enabled {
});
}

pub fn try_par_for_each_in<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does look at bit expensive. It's probably a good idea to measure the performance impact of it. It might be a better idea to write the error to a mutex / atomic instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof. I'll build some benchmarks. Seems bad if rayon's fold is more expensive than doing this manually.

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>,
Expand Down
230 changes: 124 additions & 106 deletions compiler/rustc_hir_analysis/src/check/wfcheck.rs

Large diffs are not rendered by default.

6 changes: 2 additions & 4 deletions compiler/rustc_hir_analysis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_macros/src/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand All @@ -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()?;
Expand Down Expand Up @@ -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"));
}
Expand All @@ -206,6 +214,7 @@ fn parse_query_modifiers(input: ParseStream<'_>) -> Result<QueryModifiers> {
depth_limit,
separate_provide_extern,
feedable,
ensure_forwards_result_if_red,
})
}

Expand Down Expand Up @@ -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() {
Expand Down
15 changes: 13 additions & 2 deletions compiler/rustc_middle/src/hir/map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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]
Expand Down Expand Up @@ -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]
Expand Down
32 changes: 22 additions & 10 deletions compiler/rustc_middle/src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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))
}
}

Expand Down
10 changes: 7 additions & 3 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) -> () {
Expand Down Expand Up @@ -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
Expand Down
58 changes: 55 additions & 3 deletions compiler/rustc_middle/src/query/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment explaining why we can fall back to 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> };
Expand Down Expand Up @@ -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]) => {
()
Expand Down Expand Up @@ -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,
);
)
})*
}

Expand Down
19 changes: 18 additions & 1 deletion compiler/rustc_span/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2244,7 +2244,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(());

Expand All @@ -2256,3 +2256,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"
)
}
}
14 changes: 2 additions & 12 deletions src/tools/clippy/tests/ui/crashes/ice-6252.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
9 changes: 3 additions & 6 deletions tests/ui/associated-consts/issue-105330.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
}
Loading
Loading