Auto merge of #135057 - compiler-errors:project-unconstrained, r=<try>

Project to `TyKind::Error` when there are unconstrained non-lifetime (ty/const) impl params

I think this is a bit less invasive of an approach compared to #127973.

It splits the `enforce_impl_params_are_constrained` function into lifetime/non-lifetime, and queryfies the latter. We can then use the result of the latter query (`Result<(), ErrorGuaranteed>`) to intercept projection and constrain the projected type to `TyKind::Error`, which ensures that we leak no ty or const vars to places that don't expect them, like `normalize_erasing_regions`.

The reason we split `enforce_impl_params_are_constrained` into two parts is because we only error for *lifetimes* if the lifetime ends up showing up in any of the associated types of the impl (e.g. we allow `impl<'a> Foo { type Assoc = (); }`). However, in order to compute the `type_of` query for the anonymous associated type of an RPITIT, we need to do trait solving (in `query collect_return_position_impl_trait_in_trait_tys`). That would induce cycles. Luckily, it turns out for lifetimes we don't even care about if they're unconstrained, since they're erased in all contexts that we are trying to fix ICEs. So it's sufficient to keep this check separated out of the query.

Fixes #123141
Fixes #125874
Fixes #126942
Fixes #127804
Fixes #130967

r? oli-obk

Author: bors

compiler/rustc_hir_analysis/src/impl_wf_check.rs

@@ -57,22 +57,24 @@ pub(crate) fn check_impl_wf(
     tcx: TyCtxt<'_>,
     impl_def_id: LocalDefId,
 ) -> Result<(), ErrorGuaranteed> {
-    let min_specialization = tcx.features().min_specialization();
-    let mut res = Ok(());
     debug_assert_matches!(tcx.def_kind(impl_def_id), DefKind::Impl { .. });
-    res = res.and(enforce_impl_params_are_constrained(tcx, impl_def_id));
-    if min_specialization {
+
+    // Check that the args are constrained. We queryfied the check for ty/const params
+    // since unconstrained type/const params cause ICEs in projection, so we want to
+    // detect those specifically and project those to `TyKind::Error`.
+    let mut res = tcx.ensure().enforce_impl_non_lifetime_params_are_constrained(impl_def_id);
+    res = res.and(enforce_impl_lifetime_params_are_constrained(tcx, impl_def_id));
+
+    if tcx.features().min_specialization() {
         res = res.and(check_min_specialization(tcx, impl_def_id));
     }
-
     res
 }
 
-fn enforce_impl_params_are_constrained(
+pub(crate) fn enforce_impl_lifetime_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() {
         // Don't complain about unconstrained type params when self ty isn't known due to errors.
@@ -88,6 +90,7 @@ fn enforce_impl_params_are_constrained(
         // 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);
     let impl_trait_ref = tcx.impl_trait_ref(impl_def_id).map(ty::EarlyBinder::instantiate_identity);
@@ -121,6 +124,84 @@ fn enforce_impl_params_are_constrained(
         })
         .collect();
 
+    let mut res = Ok(());
+    for param in &impl_generics.own_params {
+        match param.kind {
+            ty::GenericParamDefKind::Lifetime => {
+                let param_lt = cgp::Parameter::from(param.to_early_bound_region_data());
+                if lifetimes_in_associated_types.contains(&param_lt) // (*)
+                    && !input_parameters.contains(&param_lt)
+                {
+                    let mut diag = tcx.dcx().create_err(UnconstrainedGenericParameter {
+                        span: tcx.def_span(param.def_id),
+                        param_name: param.name,
+                        param_def_kind: tcx.def_descr(param.def_id),
+                        const_param_note: false,
+                        const_param_note2: false,
+                    });
+                    diag.code(E0207);
+                    res = Err(diag.emit());
+                }
+                // (*) This is a horrible concession to reality. I think it'd be
+                // better to just ban unconstrained lifetimes outright, but in
+                // practice people do non-hygienic macros like:
+                //
+                // ```
+                // macro_rules! __impl_slice_eq1 {
+                //   ($Lhs: ty, $Rhs: ty, $Bound: ident) => {
+                //     impl<'a, 'b, A: $Bound, B> PartialEq<$Rhs> for $Lhs where A: PartialEq<B> {
+                //        ....
+                //     }
+                //   }
+                // }
+                // ```
+                //
+                // In a concession to backwards compatibility, we continue to
+                // permit those, so long as the lifetimes aren't used in
+                // associated types. I believe this is sound, because lifetimes
+                // used elsewhere are not projected back out.
+            }
+            ty::GenericParamDefKind::Type { .. } | ty::GenericParamDefKind::Const { .. } => {
+                // Enforced in `enforce_impl_non_lifetime_params_are_constrained`.
+            }
+        }
+    }
+    res
+}
+
+pub(crate) fn enforce_impl_non_lifetime_params_are_constrained(
+    tcx: TyCtxt<'_>,
+    impl_def_id: LocalDefId,
+) -> Result<(), ErrorGuaranteed> {
+    let impl_self_ty = tcx.type_of(impl_def_id).instantiate_identity();
+    if impl_self_ty.references_error() {
+        // Don't complain about unconstrained type params when self ty isn't known due to errors.
+        // (#36836)
+        tcx.dcx().span_delayed_bug(
+            tcx.def_span(impl_def_id),
+            format!(
+                "potentially unconstrained type parameters weren't evaluated: {impl_self_ty:?}",
+            ),
+        );
+        // 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);
+    let impl_trait_ref = tcx.impl_trait_ref(impl_def_id).map(ty::EarlyBinder::instantiate_identity);
+
+    impl_trait_ref.error_reported()?;
+
+    let mut input_parameters = cgp::parameters_for_impl(tcx, impl_self_ty, impl_trait_ref);
+    cgp::identify_constrained_generic_params(
+        tcx,
+        impl_predicates,
+        impl_trait_ref,
+        &mut input_parameters,
+    );
+
     let mut res = Ok(());
     for param in &impl_generics.own_params {
         let err = match param.kind {
@@ -129,15 +210,14 @@ fn enforce_impl_params_are_constrained(
                 let param_ty = ty::ParamTy::for_def(param);
                 !input_parameters.contains(&cgp::Parameter::from(param_ty))
             }
-            ty::GenericParamDefKind::Lifetime => {
-                let param_lt = cgp::Parameter::from(param.to_early_bound_region_data());
-                lifetimes_in_associated_types.contains(&param_lt) && // (*)
-                    !input_parameters.contains(&param_lt)
-            }
             ty::GenericParamDefKind::Const { .. } => {
                 let param_ct = ty::ParamConst::for_def(param);
                 !input_parameters.contains(&cgp::Parameter::from(param_ct))
             }
+            ty::GenericParamDefKind::Lifetime => {
+                // Enforced in `enforce_impl_type_params_are_constrained`.
+                false
+            }
         };
         if err {
             let const_param_note = matches!(param.kind, ty::GenericParamDefKind::Const { .. });
@@ -153,23 +233,4 @@ fn enforce_impl_params_are_constrained(
         }
     }
     res
-
-    // (*) This is a horrible concession to reality. I think it'd be
-    // better to just ban unconstrained lifetimes outright, but in
-    // practice people do non-hygienic macros like:
-    //
-    // ```
-    // macro_rules! __impl_slice_eq1 {
-    //     ($Lhs: ty, $Rhs: ty, $Bound: ident) => {
-    //         impl<'a, 'b, A: $Bound, B> PartialEq<$Rhs> for $Lhs where A: PartialEq<B> {
-    //            ....
-    //         }
-    //     }
-    // }
-    // ```
-    //
-    // In a concession to backwards compatibility, we continue to
-    // permit those, so long as the lifetimes aren't used in
-    // associated types. I believe this is sound, because lifetimes
-    // used elsewhere are not projected back out.
 }

compiler/rustc_hir_analysis/src/lib.rs

@@ -128,6 +128,8 @@ pub fn provide(providers: &mut Providers) {
     hir_wf_check::provide(providers);
     *providers = Providers {
         inherit_sig_for_delegation_item: delegation::inherit_sig_for_delegation_item,
+        enforce_impl_non_lifetime_params_are_constrained:
+            impl_wf_check::enforce_impl_non_lifetime_params_are_constrained,
         ..*providers
     };
 }

compiler/rustc_middle/src/query/mod.rs

@@ -1719,6 +1719,11 @@ rustc_queries! {
         ensure_forwards_result_if_red
     }
 
+    query enforce_impl_non_lifetime_params_are_constrained(key: LocalDefId) -> Result<(), ErrorGuaranteed> {
+        desc { |tcx| "checking that `{}`'s generics are constrained by the impl header", tcx.def_path_str(key) }
+        ensure_forwards_result_if_red
+    }
+
     // The `DefId`s of all non-generic functions and statics in the given crate
     // that can be reached from outside the crate.
     //

compiler/rustc_next_trait_solver/src/delegate.rs

@@ -95,7 +95,10 @@ pub trait SolverDelegate: Deref<Target = <Self as SolverDelegate>::Infcx> + Size
         goal_trait_ref: ty::TraitRef<Self::Interner>,
         trait_assoc_def_id: <Self::Interner as Interner>::DefId,
         impl_def_id: <Self::Interner as Interner>::DefId,
-    ) -> Result<Option<<Self::Interner as Interner>::DefId>, NoSolution>;
+    ) -> Result<
+        Option<<Self::Interner as Interner>::DefId>,
+        <Self::Interner as Interner>::ErrorGuaranteed,
+    >;
 
     fn is_transmutable(
         &self,

compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs

@@ -950,7 +950,7 @@ where
         goal_trait_ref: ty::TraitRef<I>,
         trait_assoc_def_id: I::DefId,
         impl_def_id: I::DefId,
-    ) -> Result<Option<I::DefId>, NoSolution> {
+    ) -> Result<Option<I::DefId>, I::ErrorGuaranteed> {
         self.delegate.fetch_eligible_assoc_item(goal_trait_ref, trait_assoc_def_id, impl_def_id)
     }
 

compiler/rustc_next_trait_solver/src/solve/normalizes_to/mod.rs

@@ -244,20 +244,7 @@ where
                     .map(|pred| goal.with(cx, pred)),
             );
 
-            // In case the associated item is hidden due to specialization, we have to
-            // return ambiguity this would otherwise be incomplete, resulting in
-            // unsoundness during coherence (#105782).
-            let Some(target_item_def_id) = ecx.fetch_eligible_assoc_item(
-                goal_trait_ref,
-                goal.predicate.def_id(),
-                impl_def_id,
-            )?
-            else {
-                return ecx.evaluate_added_goals_and_make_canonical_response(Certainty::AMBIGUOUS);
-            };
-
-            let error_response = |ecx: &mut EvalCtxt<'_, D>, msg: &str| {
-                let guar = cx.delay_bug(msg);
+            let error_response = |ecx: &mut EvalCtxt<'_, D>, guar| {
                 let error_term = match goal.predicate.alias.kind(cx) {
                     ty::AliasTermKind::ProjectionTy => Ty::new_error(cx, guar).into(),
                     ty::AliasTermKind::ProjectionConst => Const::new_error(cx, guar).into(),
@@ -267,8 +254,24 @@ where
                 ecx.evaluate_added_goals_and_make_canonical_response(Certainty::Yes)
             };
 
+            // In case the associated item is hidden due to specialization, we have to
+            // return ambiguity this would otherwise be incomplete, resulting in
+            // unsoundness during coherence (#105782).
+            let target_item_def_id = match ecx.fetch_eligible_assoc_item(
+                goal_trait_ref,
+                goal.predicate.def_id(),
+                impl_def_id,
+            ) {
+                Ok(Some(target_item_def_id)) => target_item_def_id,
+                Ok(None) => {
+                    return ecx
+                        .evaluate_added_goals_and_make_canonical_response(Certainty::AMBIGUOUS);
+                }
+                Err(guar) => return error_response(ecx, guar),
+            };
+
             if !cx.has_item_definition(target_item_def_id) {
-                return error_response(ecx, "missing item");
+                return error_response(ecx, cx.delay_bug("missing item"));
             }
 
             let target_container_def_id = cx.parent(target_item_def_id);
@@ -292,7 +295,10 @@ where
             )?;
 
             if !cx.check_args_compatible(target_item_def_id, target_args) {
-                return error_response(ecx, "associated item has mismatched arguments");
+                return error_response(
+                    ecx,
+                    cx.delay_bug("associated item has mismatched arguments"),
+                );
             }
 
             // Finally we construct the actual value of the associated type.

compiler/rustc_trait_selection/src/solve/delegate.rs

@@ -190,9 +190,8 @@ impl<'tcx> rustc_next_trait_solver::delegate::SolverDelegate for SolverDelegate<
         goal_trait_ref: ty::TraitRef<'tcx>,
         trait_assoc_def_id: DefId,
         impl_def_id: DefId,
-    ) -> Result<Option<DefId>, NoSolution> {
-        let node_item = specialization_graph::assoc_def(self.tcx, impl_def_id, trait_assoc_def_id)
-            .map_err(|ErrorGuaranteed { .. }| NoSolution)?;
+    ) -> Result<Option<DefId>, ErrorGuaranteed> {
+        let node_item = specialization_graph::assoc_def(self.tcx, impl_def_id, trait_assoc_def_id)?;
 
         let eligible = if node_item.is_final() {
             // Non-specializable items are always projectable.

compiler/rustc_trait_selection/src/traits/project.rs

@@ -950,39 +950,45 @@ fn assemble_candidates_from_impls<'cx, 'tcx>(
                 //
                 // NOTE: This should be kept in sync with the similar code in
                 // `rustc_ty_utils::instance::resolve_associated_item()`.
-                let node_item = specialization_graph::assoc_def(
+                match specialization_graph::assoc_def(
                     selcx.tcx(),
                     impl_data.impl_def_id,
                     obligation.predicate.def_id,
-                )
-                .map_err(|ErrorGuaranteed { .. }| ())?;
-
-                if node_item.is_final() {
-                    // Non-specializable items are always projectable.
-                    true
-                } else {
-                    // Only reveal a specializable default if we're past type-checking
-                    // and the obligation is monomorphic, otherwise passes such as
-                    // transmute checking and polymorphic MIR optimizations could
-                    // get a result which isn't correct for all monomorphizations.
-                    match selcx.infcx.typing_mode() {
-                        TypingMode::Coherence
-                        | TypingMode::Analysis { .. }
-                        | TypingMode::PostBorrowckAnalysis { .. } => {
-                            debug!(
-                                assoc_ty = ?selcx.tcx().def_path_str(node_item.item.def_id),
-                                ?obligation.predicate,
-                                "assemble_candidates_from_impls: not eligible due to default",
-                            );
-                            false
-                        }
-                        TypingMode::PostAnalysis => {
-                            // NOTE(eddyb) inference variables can resolve to parameters, so
-                            // assume `poly_trait_ref` isn't monomorphic, if it contains any.
-                            let poly_trait_ref = selcx.infcx.resolve_vars_if_possible(trait_ref);
-                            !poly_trait_ref.still_further_specializable()
+                ) {
+                    Ok(node_item) => {
+                        if node_item.is_final() {
+                            // Non-specializable items are always projectable.
+                            true
+                        } else {
+                            // Only reveal a specializable default if we're past type-checking
+                            // and the obligation is monomorphic, otherwise passes such as
+                            // transmute checking and polymorphic MIR optimizations could
+                            // get a result which isn't correct for all monomorphizations.
+                            match selcx.infcx.typing_mode() {
+                                TypingMode::Coherence
+                                | TypingMode::Analysis { .. }
+                                | TypingMode::PostBorrowckAnalysis { .. } => {
+                                    debug!(
+                                        assoc_ty = ?selcx.tcx().def_path_str(node_item.item.def_id),
+                                        ?obligation.predicate,
+                                        "not eligible due to default",
+                                    );
+                                    false
+                                }
+                                TypingMode::PostAnalysis => {
+                                    // NOTE(eddyb) inference variables can resolve to parameters, so
+                                    // assume `poly_trait_ref` isn't monomorphic, if it contains any.
+                                    let poly_trait_ref =
+                                        selcx.infcx.resolve_vars_if_possible(trait_ref);
+                                    !poly_trait_ref.still_further_specializable()
+                                }
+                            }
                         }
                     }
+                    // Always project `ErrorGuaranteed`, since this will just help
+                    // us propagate `TyKind::Error` around which suppresses ICEs
+                    // and spurious, unrelated inference errors.
+                    Err(ErrorGuaranteed { .. }) => true,
                 }
             }
             ImplSource::Builtin(BuiltinImplSource::Misc, _) => {
@@ -2014,7 +2020,6 @@ fn confirm_impl_candidate<'cx, 'tcx>(
         Ok(assoc_ty) => assoc_ty,
         Err(guar) => return Progress::error(tcx, guar),
     };
-
     if !assoc_ty.item.defaultness(tcx).has_value() {
         // This means that the impl is missing a definition for the
         // associated type. This error will be reported by the type

compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs

@@ -376,6 +376,12 @@ pub(crate) fn assoc_def(
     // If there is no such item in that impl, this function will fail with a
     // cycle error if the specialization graph is currently being built.
     if let Some(&impl_item_id) = tcx.impl_item_implementor_ids(impl_def_id).get(&assoc_def_id) {
+        // Ensure that the impl is constrained, otherwise projection may give us
+        // bad unconstrained infer vars.
+        if let Some(impl_def_id) = impl_def_id.as_local() {
+            tcx.ensure().enforce_impl_non_lifetime_params_are_constrained(impl_def_id)?;
+        }
+
         let item = tcx.associated_item(impl_item_id);
         let impl_node = Node::Impl(impl_def_id);
         return Ok(LeafDef {
@@ -391,6 +397,14 @@ pub(crate) fn assoc_def(
 
     let ancestors = trait_def.ancestors(tcx, impl_def_id)?;
     if let Some(assoc_item) = ancestors.leaf_def(tcx, assoc_def_id) {
+        // Ensure that the impl is constrained, otherwise projection may give us
+        // bad unconstrained infer vars.
+        if assoc_item.item.container == ty::AssocItemContainer::Impl
+            && let Some(impl_def_id) = assoc_item.item.container_id(tcx).as_local()
+        {
+            tcx.ensure().enforce_impl_non_lifetime_params_are_constrained(impl_def_id)?;
+        }
+
         Ok(assoc_item)
     } else {
         // This is saying that neither the trait nor