Move `check_mod_impl_wf` query call out of track_errors and bubble errors up instead.

7 files changed, 163 insertions, 75 deletions

diff --git a/compiler/rustc_hir_analysis/src/impl_wf_check.rs b/compiler/rustc_hir_analysis/src/impl_wf_check.rs
index 3f9b1f384d7..2fe08ead72b 100644
--- a/compiler/rustc_hir_analysis/src/impl_wf_check.rs
+++ b/compiler/rustc_hir_analysis/src/impl_wf_check.rs
@@ -17,7 +17,7 @@ use rustc_hir::def::DefKind;
 use rustc_hir::def_id::{LocalDefId, LocalModDefId};
 use rustc_middle::query::Providers;
 use rustc_middle::ty::{self, TyCtxt, TypeVisitableExt};
-use rustc_span::{Span, Symbol};
+use rustc_span::{ErrorGuaranteed, Span, Symbol};
 
 mod min_specialization;
 
@@ -51,24 +51,29 @@ mod min_specialization;
 /// impl<'a> Trait<Foo> for Bar { type X = &'a i32; }
 /// //   ^ 'a is unused and appears in assoc type, error
 /// ```
-fn check_mod_impl_wf(tcx: TyCtxt<'_>, module_def_id: LocalModDefId) {
+fn check_mod_impl_wf(tcx: TyCtxt<'_>, module_def_id: LocalModDefId) -> Result<(), ErrorGuaranteed> {
     let min_specialization = tcx.features().min_specialization;
     let module = tcx.hir_module_items(module_def_id);
+    let mut res = Ok(());
     for id in module.items() {
         if matches!(tcx.def_kind(id.owner_id), DefKind::Impl { .. }) {
-            enforce_impl_params_are_constrained(tcx, id.owner_id.def_id);
+            res = res.and(enforce_impl_params_are_constrained(tcx, id.owner_id.def_id));
             if min_specialization {
-                check_min_specialization(tcx, id.owner_id.def_id);
+                res = res.and(check_min_specialization(tcx, id.owner_id.def_id));
             }
         }
     }
+    res
 }
 
 pub fn provide(providers: &mut Providers) {
     *providers = Providers { check_mod_impl_wf, ..*providers };
 }
 
-fn enforce_impl_params_are_constrained(tcx: TyCtxt<'_>, impl_def_id: LocalDefId) {
+fn enforce_impl_params_are_constrained(
+    tcx: TyCtxt<'_>,
+    impl_def_id: LocalDefId,
+) -> Result<(), ErrorGuaranteed> {
     // Every lifetime used in an associated type must be constrained.
     let impl_self_ty = tcx.type_of(impl_def_id).instantiate_identity();
     if impl_self_ty.references_error() {
@@ -80,7 +85,10 @@ fn enforce_impl_params_are_constrained(tcx: TyCtxt<'_>, impl_def_id: LocalDefId)
                 "potentially unconstrained type parameters weren't evaluated: {impl_self_ty:?}",
             ),
         );
-        return;
+        // This is super fishy, but our current `rustc_hir_analysis::check_crate` pipeline depends on
+        // `type_of` having been called much earlier, and thus this value being read from cache.
+        // Compilation must continue in order for other important diagnostics to keep showing up.
+        return Ok(());
     }
     let impl_generics = tcx.generics_of(impl_def_id);
     let impl_predicates = tcx.predicates_of(impl_def_id);
@@ -113,13 +121,19 @@ fn enforce_impl_params_are_constrained(tcx: TyCtxt<'_>, impl_def_id: LocalDefId)
         })
         .collect();
 
+    let mut res = Ok(());
     for param in &impl_generics.params {
         match param.kind {
             // Disallow ANY unconstrained type parameters.
             ty::GenericParamDefKind::Type { .. } => {
                 let param_ty = ty::ParamTy::for_def(param);
                 if !input_parameters.contains(&cgp::Parameter::from(param_ty)) {
-                    report_unused_parameter(tcx, tcx.def_span(param.def_id), "type", param_ty.name);
+                    res = Err(report_unused_parameter(
+                        tcx,
+                        tcx.def_span(param.def_id),
+                        "type",
+                        param_ty.name,
+                    ));
                 }
             }
             ty::GenericParamDefKind::Lifetime => {
@@ -127,27 +141,28 @@ fn enforce_impl_params_are_constrained(tcx: TyCtxt<'_>, impl_def_id: LocalDefId)
                 if lifetimes_in_associated_types.contains(&param_lt) && // (*)
                     !input_parameters.contains(&param_lt)
                 {
-                    report_unused_parameter(
+                    res = Err(report_unused_parameter(
                         tcx,
                         tcx.def_span(param.def_id),
                         "lifetime",
                         param.name,
-                    );
+                    ));
                 }
             }
             ty::GenericParamDefKind::Const { .. } => {
                 let param_ct = ty::ParamConst::for_def(param);
                 if !input_parameters.contains(&cgp::Parameter::from(param_ct)) {
-                    report_unused_parameter(
+                    res = Err(report_unused_parameter(
                         tcx,
                         tcx.def_span(param.def_id),
                         "const",
                         param_ct.name,
-                    );
+                    ));
                 }
             }
         }
     }
+    res
 
     // (*) This is a horrible concession to reality. I think it'd be
     // better to just ban unconstrained lifetimes outright, but in
@@ -169,7 +184,12 @@ fn enforce_impl_params_are_constrained(tcx: TyCtxt<'_>, impl_def_id: LocalDefId)
     // used elsewhere are not projected back out.
 }
 
-fn report_unused_parameter(tcx: TyCtxt<'_>, span: Span, kind: &str, name: Symbol) {
+fn report_unused_parameter(
+    tcx: TyCtxt<'_>,
+    span: Span,
+    kind: &str,
+    name: Symbol,
+) -> ErrorGuaranteed {
     let mut err = struct_span_code_err!(
         tcx.dcx(),
         span,
@@ -188,5 +208,5 @@ fn report_unused_parameter(tcx: TyCtxt<'_>, span: Span, kind: &str, name: Symbol
             "proving the result of expressions other than the parameter are unique is not supported",
         );
     }
-    err.emit();
+    err.emit()
 }
diff --git a/compiler/rustc_hir_analysis/src/impl_wf_check/min_specialization.rs b/compiler/rustc_hir_analysis/src/impl_wf_check/min_specialization.rs
index 93dd2342a4d..6d685aec663 100644
--- a/compiler/rustc_hir_analysis/src/impl_wf_check/min_specialization.rs
+++ b/compiler/rustc_hir_analysis/src/impl_wf_check/min_specialization.rs
@@ -82,10 +82,14 @@ use rustc_trait_selection::traits::error_reporting::TypeErrCtxtExt;
 use rustc_trait_selection::traits::outlives_bounds::InferCtxtExt as _;
 use rustc_trait_selection::traits::{self, translate_args_with_cause, wf, ObligationCtxt};
 
-pub(super) fn check_min_specialization(tcx: TyCtxt<'_>, impl_def_id: LocalDefId) {
+pub(super) fn check_min_specialization(
+    tcx: TyCtxt<'_>,
+    impl_def_id: LocalDefId,
+) -> Result<(), ErrorGuaranteed> {
     if let Some(node) = parent_specialization_node(tcx, impl_def_id) {
-        check_always_applicable(tcx, impl_def_id, node);
+        check_always_applicable(tcx, impl_def_id, node)?;
     }
+    Ok(())
 }
 
 fn parent_specialization_node(tcx: TyCtxt<'_>, impl1_def_id: LocalDefId) -> Option<Node> {
@@ -109,42 +113,58 @@ fn parent_specialization_node(tcx: TyCtxt<'_>, impl1_def_id: LocalDefId) -> Opti
 
 /// Check that `impl1` is a sound specialization
 #[instrument(level = "debug", skip(tcx))]
-fn check_always_applicable(tcx: TyCtxt<'_>, impl1_def_id: LocalDefId, impl2_node: Node) {
+fn check_always_applicable(
+    tcx: TyCtxt<'_>,
+    impl1_def_id: LocalDefId,
+    impl2_node: Node,
+) -> Result<(), ErrorGuaranteed> {
     let span = tcx.def_span(impl1_def_id);
-    check_has_items(tcx, impl1_def_id, impl2_node, span);
-
-    if let Ok((impl1_args, impl2_args)) = get_impl_args(tcx, impl1_def_id, impl2_node) {
-        let impl2_def_id = impl2_node.def_id();
-        debug!(?impl2_def_id, ?impl2_args);
-
-        let parent_args = if impl2_node.is_from_trait() {
-            impl2_args.to_vec()
-        } else {
-            unconstrained_parent_impl_args(tcx, impl2_def_id, impl2_args)
-        };
-
-        check_constness(tcx, impl1_def_id, impl2_node, span);
-        check_static_lifetimes(tcx, &parent_args, span);
-        check_duplicate_params(tcx, impl1_args, &parent_args, span);
-        check_predicates(tcx, impl1_def_id, impl1_args, impl2_node, impl2_args, span);
-    }
+    let mut res = check_has_items(tcx, impl1_def_id, impl2_node, span);
+
+    let (impl1_args, impl2_args) = get_impl_args(tcx, impl1_def_id, impl2_node)?;
+    let impl2_def_id = impl2_node.def_id();
+    debug!(?impl2_def_id, ?impl2_args);
+
+    let parent_args = if impl2_node.is_from_trait() {
+        impl2_args.to_vec()
+    } else {
+        unconstrained_parent_impl_args(tcx, impl2_def_id, impl2_args)
+    };
+
+    res = res.and(check_constness(tcx, impl1_def_id, impl2_node, span));
+    res = res.and(check_static_lifetimes(tcx, &parent_args, span));
+    res = res.and(check_duplicate_params(tcx, impl1_args, &parent_args, span));
+    res = res.and(check_predicates(tcx, impl1_def_id, impl1_args, impl2_node, impl2_args, span));
+
+    res
 }
 
-fn check_has_items(tcx: TyCtxt<'_>, impl1_def_id: LocalDefId, impl2_node: Node, span: Span) {
+fn check_has_items(
+    tcx: TyCtxt<'_>,
+    impl1_def_id: LocalDefId,
+    impl2_node: Node,
+    span: Span,
+) -> Result<(), ErrorGuaranteed> {
     if let Node::Impl(impl2_id) = impl2_node
         && tcx.associated_item_def_ids(impl1_def_id).is_empty()
     {
         let base_impl_span = tcx.def_span(impl2_id);
-        tcx.dcx().emit_err(errors::EmptySpecialization { span, base_impl_span });
+        return Err(tcx.dcx().emit_err(errors::EmptySpecialization { span, base_impl_span }));
     }
+    Ok(())
 }
 
 /// Check that the specializing impl `impl1` is at least as const as the base
 /// impl `impl2`
-fn check_constness(tcx: TyCtxt<'_>, impl1_def_id: LocalDefId, impl2_node: Node, span: Span) {
+fn check_constness(
+    tcx: TyCtxt<'_>,
+    impl1_def_id: LocalDefId,
+    impl2_node: Node,
+    span: Span,
+) -> Result<(), ErrorGuaranteed> {
     if impl2_node.is_from_trait() {
         // This isn't a specialization
-        return;
+        return Ok(());
     }
 
     let impl1_constness = tcx.constness(impl1_def_id.to_def_id());
@@ -152,9 +172,10 @@ fn check_constness(tcx: TyCtxt<'_>, impl1_def_id: LocalDefId, impl2_node: Node,
 
     if let hir::Constness::Const = impl2_constness {
         if let hir::Constness::NotConst = impl1_constness {
-            tcx.dcx().emit_err(errors::ConstSpecialize { span });
+            return Err(tcx.dcx().emit_err(errors::ConstSpecialize { span }));
         }
     }
+    Ok(())
 }
 
 /// Given a specializing impl `impl1`, and the base impl `impl2`, returns two
@@ -290,15 +311,17 @@ fn check_duplicate_params<'tcx>(
     impl1_args: GenericArgsRef<'tcx>,
     parent_args: &Vec<GenericArg<'tcx>>,
     span: Span,
-) {
+) -> Result<(), ErrorGuaranteed> {
     let mut base_params = cgp::parameters_for(parent_args, true);
     base_params.sort_by_key(|param| param.0);
     if let (_, [duplicate, ..]) = base_params.partition_dedup() {
         let param = impl1_args[duplicate.0 as usize];
-        tcx.dcx()
+        return Err(tcx
+            .dcx()
             .struct_span_err(span, format!("specializing impl repeats parameter `{param}`"))
-            .emit();
+            .emit());
     }
+    Ok(())
 }
 
 /// Check that `'static` lifetimes are not introduced by the specializing impl.
@@ -313,10 +336,11 @@ fn check_static_lifetimes<'tcx>(
     tcx: TyCtxt<'tcx>,
     parent_args: &Vec<GenericArg<'tcx>>,
     span: Span,
-) {
+) -> Result<(), ErrorGuaranteed> {
     if tcx.any_free_region_meets(parent_args, |r| r.is_static()) {
-        tcx.dcx().emit_err(errors::StaticSpecialize { span });
+        return Err(tcx.dcx().emit_err(errors::StaticSpecialize { span }));
     }
+    Ok(())
 }
 
 /// Check whether predicates on the specializing impl (`impl1`) are allowed.
@@ -337,7 +361,7 @@ fn check_predicates<'tcx>(
     impl2_node: Node,
     impl2_args: GenericArgsRef<'tcx>,
     span: Span,
-) {
+) -> Result<(), ErrorGuaranteed> {
     let impl1_predicates: Vec<_> = traits::elaborate(
         tcx,
         tcx.predicates_of(impl1_def_id).instantiate(tcx, impl1_args).into_iter(),
@@ -399,14 +423,16 @@ fn check_predicates<'tcx>(
     }
     impl2_predicates.extend(traits::elaborate(tcx, always_applicable_traits));
 
+    let mut res = Ok(());
     for (clause, span) in impl1_predicates {
         if !impl2_predicates
             .iter()
             .any(|pred2| trait_predicates_eq(tcx, clause.as_predicate(), *pred2, span))
         {
-            check_specialization_on(tcx, clause, span)
+            res = res.and(check_specialization_on(tcx, clause, span))
         }
     }
+    res
 }
 
 /// Checks if some predicate on the specializing impl (`predicate1`) is the same
@@ -443,19 +469,26 @@ fn trait_predicates_eq<'tcx>(
 }
 
 #[instrument(level = "debug", skip(tcx))]
-fn check_specialization_on<'tcx>(tcx: TyCtxt<'tcx>, clause: ty::Clause<'tcx>, span: Span) {
+fn check_specialization_on<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    clause: ty::Clause<'tcx>,
+    span: Span,
+) -> Result<(), ErrorGuaranteed> {
     match clause.kind().skip_binder() {
         // Global predicates are either always true or always false, so we
         // are fine to specialize on.
-        _ if clause.is_global() => (),
+        _ if clause.is_global() => Ok(()),
         // We allow specializing on explicitly marked traits with no associated
         // items.
         ty::ClauseKind::Trait(ty::TraitPredicate { trait_ref, polarity: _ }) => {
-            if !matches!(
+            if matches!(
                 trait_specialization_kind(tcx, clause),
                 Some(TraitSpecializationKind::Marker)
             ) {
-                tcx.dcx()
+                Ok(())
+            } else {
+                Err(tcx
+                    .dcx()
                     .struct_span_err(
                         span,
                         format!(
@@ -463,17 +496,16 @@ fn check_specialization_on<'tcx>(tcx: TyCtxt<'tcx>, clause: ty::Clause<'tcx>, sp
                             tcx.def_path_str(trait_ref.def_id),
                         ),
                     )
-                    .emit();
+                    .emit())
             }
         }
-        ty::ClauseKind::Projection(ty::ProjectionPredicate { projection_ty, term }) => {
-            tcx.dcx()
-                .struct_span_err(
-                    span,
-                    format!("cannot specialize on associated type `{projection_ty} == {term}`",),
-                )
-                .emit();
-        }
+        ty::ClauseKind::Projection(ty::ProjectionPredicate { projection_ty, term }) => Err(tcx
+            .dcx()
+            .struct_span_err(
+                span,
+                format!("cannot specialize on associated type `{projection_ty} == {term}`",),
+            )
+            .emit()),
         ty::ClauseKind::ConstArgHasType(..) => {
             // FIXME(min_specialization), FIXME(const_generics):
             // It probably isn't right to allow _every_ `ConstArgHasType` but I am somewhat unsure
@@ -483,12 +515,12 @@ fn check_specialization_on<'tcx>(tcx: TyCtxt<'tcx>, clause: ty::Clause<'tcx>, sp
             // While we do not support constructs like `<T, const N: T>` there is probably no risk of
             // soundness bugs, but when we support generic const parameter types this will need to be
             // revisited.
+            Ok(())
         }
-        _ => {
-            tcx.dcx()
-                .struct_span_err(span, format!("cannot specialize on predicate `{clause}`"))
-                .emit();
-        }
+        _ => Err(tcx
+            .dcx()
+            .struct_span_err(span, format!("cannot specialize on predicate `{clause}`"))
+            .emit()),
     }
 }
 
diff --git a/compiler/rustc_hir_analysis/src/lib.rs b/compiler/rustc_hir_analysis/src/lib.rs
index 16e530ca946..1a370ad10cf 100644
--- a/compiler/rustc_hir_analysis/src/lib.rs
+++ b/compiler/rustc_hir_analysis/src/lib.rs
@@ -166,17 +166,18 @@ pub fn check_crate(tcx: TyCtxt<'_>) -> Result<(), ErrorGuaranteed> {
         tcx.hir().for_each_module(|module| tcx.ensure().collect_mod_item_types(module))
     });
 
-    // FIXME(matthewjasper) We shouldn't need to use `track_errors` anywhere in this function
-    // or the compiler in general.
     if tcx.features().rustc_attrs {
         tcx.sess.time("outlives_testing", || outlives::test::test_inferred_outlives(tcx))?;
     }
 
-    tcx.sess.track_errors(|| {
-        tcx.sess.time("coherence_checking", || {
-            // Check impls constrain their parameters
-            tcx.hir().for_each_module(|module| tcx.ensure().check_mod_impl_wf(module));
+    tcx.sess.time("coherence_checking", || {
+        // Check impls constrain their parameters
+        let res =
+            tcx.hir().try_par_for_each_module(|module| tcx.ensure().check_mod_impl_wf(module));
 
+        // FIXME(matthewjasper) We shouldn't need to use `track_errors` anywhere in this function
+        // or the compiler in general.
+        res.and(tcx.sess.track_errors(|| {
             for &trait_def_id in tcx.all_local_trait_impls(()).keys() {
                 tcx.ensure().coherent_trait(trait_def_id);
             }
@@ -184,7 +185,7 @@ pub fn check_crate(tcx: TyCtxt<'_>) -> Result<(), ErrorGuaranteed> {
             // these queries are executed for side-effects (error reporting):
             tcx.ensure().crate_inherent_impls(());
             tcx.ensure().crate_inherent_impls_overlap_check(());
-        });
+        }))
     })?;
 
     if tcx.features().rustc_attrs {
diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs
index 1dc77220881..6e5870df7c1 100644
--- a/compiler/rustc_middle/src/query/mod.rs
+++ b/compiler/rustc_middle/src/query/mod.rs
@@ -961,8 +961,9 @@ rustc_queries! {
         desc { |tcx| "checking deathness of variables in {}", describe_as_module(key, tcx) }
     }
 
-    query check_mod_impl_wf(key: LocalModDefId) -> () {
+    query check_mod_impl_wf(key: LocalModDefId) -> Result<(), ErrorGuaranteed> {
         desc { |tcx| "checking that impls are well-formed in {}", describe_as_module(key, tcx) }
+        ensure_forwards_result_if_red
     }
 
     query check_mod_type_wf(key: LocalModDefId) -> Result<(), ErrorGuaranteed> {
diff --git a/tests/ui/impl-trait/in-trait/return-dont-satisfy-bounds.rs b/tests/ui/impl-trait/in-trait/return-dont-satisfy-bounds.rs
index ad73b12feb5..ff265e576b9 100644
--- a/tests/ui/impl-trait/in-trait/return-dont-satisfy-bounds.rs
+++ b/tests/ui/impl-trait/in-trait/return-dont-satisfy-bounds.rs
@@ -7,6 +7,8 @@ struct Bar;
 impl Foo<char> for Bar {
     fn foo<F2: Foo<u8>>(self) -> impl Foo<u8> {
         //~^ ERROR: the trait bound `impl Foo<u8>: Foo<char>` is not satisfied [E0277]
+        //~| ERROR: the trait bound `Bar: Foo<u8>` is not satisfied [E0277]
+        //~| ERROR: impl has stricter requirements than trait
         self
     }
 }
diff --git a/tests/ui/impl-trait/in-trait/return-dont-satisfy-bounds.stderr b/tests/ui/impl-trait/in-trait/return-dont-satisfy-bounds.stderr
index db587d069f8..638de01f913 100644
--- a/tests/ui/impl-trait/in-trait/return-dont-satisfy-bounds.stderr
+++ b/tests/ui/impl-trait/in-trait/return-dont-satisfy-bounds.stderr
@@ -11,6 +11,28 @@ note: required by a bound in `Foo::{opaque#0}`
 LL |     fn foo<F2>(self) -> impl Foo<T>;
    |                              ^^^^^^ required by this bound in `Foo::{opaque#0}`
 
-error: aborting due to 1 previous error
+error[E0276]: impl has stricter requirements than trait
+  --> $DIR/return-dont-satisfy-bounds.rs:8:16
+   |
+LL |     fn foo<F2>(self) -> impl Foo<T>;
+   |     -------------------------------- definition of `foo` from trait
+...
+LL |     fn foo<F2: Foo<u8>>(self) -> impl Foo<u8> {
+   |                ^^^^^^^ impl has extra requirement `F2: Foo<u8>`
+
+error[E0277]: the trait bound `Bar: Foo<u8>` is not satisfied
+  --> $DIR/return-dont-satisfy-bounds.rs:8:34
+   |
+LL |     fn foo<F2: Foo<u8>>(self) -> impl Foo<u8> {
+   |                                  ^^^^^^^^^^^^ the trait `Foo<u8>` is not implemented for `Bar`
+...
+LL |         self
+   |         ---- return type was inferred to be `Bar` here
+   |
+   = help: the trait `Foo<char>` is implemented for `Bar`
+   = help: for that trait implementation, expected `char`, found `u8`
+
+error: aborting due to 3 previous errors
 
-For more information about this error, try `rustc --explain E0277`.
+Some errors have detailed explanations: E0276, E0277.
+For more information about an error, try `rustc --explain E0276`.
diff --git a/tests/ui/rfcs/rfc-2632-const-trait-impl/specializing-constness-2.stderr b/tests/ui/rfcs/rfc-2632-const-trait-impl/specializing-constness-2.stderr
index 5210a694201..e5b9493b3ce 100644
--- a/tests/ui/rfcs/rfc-2632-const-trait-impl/specializing-constness-2.stderr
+++ b/tests/ui/rfcs/rfc-2632-const-trait-impl/specializing-constness-2.stderr
@@ -7,6 +7,16 @@ LL | impl<T: Default> A for T {
 LL | impl<T: Default + ~const Sup> const A for T {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation
 
-error: aborting due to 1 previous error
+error[E0308]: mismatched types
+  --> $DIR/specializing-constness-2.rs:27:5
+   |
+LL |     <T as A>::a();
+   |     ^^^^^^^^^^^^^ expected `host`, found `true`
+   |
+   = note: expected constant `host`
+              found constant `true`
+
+error: aborting due to 2 previous errors
 
-For more information about this error, try `rustc --explain E0119`.
+Some errors have detailed explanations: E0119, E0308.
+For more information about an error, try `rustc --explain E0119`.