Do not project when there are unconstrained impl params

Author: compiler-errors

Diff for: 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.
 }

Diff for: 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
     };
 }

Diff for: 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.
     //

Diff for: 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

Diff for: 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