Rollup merge of #97123 - ricked-twice:issue-96223-clean-fix, r=jackh726

Clean fix for #96223

Okay, so here we are (hopefully) 👍

Closes #96223

Thanks a lot to `@jackh726` for your help and explanation 🙏

- Modified `InferCtxt::mk_trait_obligation_with_new_self_ty` to take as argument a `Binder<(TraitPredicate, Ty)>` instead of a `Binder<TraitPredicate>` and a separate `Ty` with no bound vars.

- Modified all call places to avoid calling `Binder::no_bounds_var` or `Binder::skip_binder` when it is not safe.

r? `@jackh726`

Author: Dylan-DPC

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

@@ -1384,8 +1384,7 @@ trait InferCtxtPrivExt<'hir, 'tcx> {
     fn mk_trait_obligation_with_new_self_ty(
         &self,
         param_env: ty::ParamEnv<'tcx>,
-        trait_ref: ty::PolyTraitPredicate<'tcx>,
-        new_self_ty: Ty<'tcx>,
+        trait_ref_and_ty: ty::Binder<'tcx, (ty::TraitPredicate<'tcx>, Ty<'tcx>)>,
     ) -> PredicateObligation<'tcx>;
 
     fn maybe_report_ambiguity(
@@ -1923,14 +1922,11 @@ impl<'a, 'tcx> InferCtxtPrivExt<'a, 'tcx> for InferCtxt<'a, 'tcx> {
     fn mk_trait_obligation_with_new_self_ty(
         &self,
         param_env: ty::ParamEnv<'tcx>,
-        trait_ref: ty::PolyTraitPredicate<'tcx>,
-        new_self_ty: Ty<'tcx>,
+        trait_ref_and_ty: ty::Binder<'tcx, (ty::TraitPredicate<'tcx>, Ty<'tcx>)>,
     ) -> PredicateObligation<'tcx> {
-        assert!(!new_self_ty.has_escaping_bound_vars());
-
-        let trait_pred = trait_ref.map_bound_ref(|tr| ty::TraitPredicate {
+        let trait_pred = trait_ref_and_ty.map_bound_ref(|(tr, new_self_ty)| ty::TraitPredicate {
             trait_ref: ty::TraitRef {
-                substs: self.tcx.mk_substs_trait(new_self_ty, &tr.trait_ref.substs[1..]),
+                substs: self.tcx.mk_substs_trait(*new_self_ty, &tr.trait_ref.substs[1..]),
                 ..tr.trait_ref
             },
             ..*tr

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

@@ -628,17 +628,21 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             if let Some(parent_trait_pred) = parent_trait_pred {
                 real_trait_pred = parent_trait_pred;
             }
-            let Some(real_ty) = real_trait_pred.self_ty().no_bound_vars() else {
-                continue;
-            };
+
+            // Skipping binder here, remapping below
+            let real_ty = real_trait_pred.self_ty().skip_binder();
 
             if let ty::Ref(region, base_ty, mutbl) = *real_ty.kind() {
                 let mut autoderef = Autoderef::new(self, param_env, body_id, span, base_ty, span);
                 if let Some(steps) = autoderef.find_map(|(ty, steps)| {
                     // Re-add the `&`
                     let ty = self.tcx.mk_ref(region, TypeAndMut { ty, mutbl });
-                    let obligation =
-                        self.mk_trait_obligation_with_new_self_ty(param_env, real_trait_pred, ty);
+
+                    // Remapping bound vars here
+                    let real_trait_pred_and_ty =
+                        real_trait_pred.map_bound(|inner_trait_pred| (inner_trait_pred, ty));
+                    let obligation = self
+                        .mk_trait_obligation_with_new_self_ty(param_env, real_trait_pred_and_ty);
                     Some(steps).filter(|_| self.predicate_may_hold(&obligation))
                 }) {
                     if steps > 0 {
@@ -659,10 +663,13 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                     }
                 } else if real_trait_pred != trait_pred {
                     // This branch addresses #87437.
+
+                    // Remapping bound vars here
+                    let real_trait_pred_and_base_ty =
+                        real_trait_pred.map_bound(|inner_trait_pred| (inner_trait_pred, base_ty));
                     let obligation = self.mk_trait_obligation_with_new_self_ty(
                         param_env,
-                        real_trait_pred,
-                        base_ty,
+                        real_trait_pred_and_base_ty,
                     );
                     if self.predicate_may_hold(&obligation) {
                         err.span_suggestion_verbose(
@@ -720,9 +727,8 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
         err: &mut Diagnostic,
         trait_pred: ty::PolyTraitPredicate<'tcx>,
     ) -> bool {
-        let Some(self_ty) = trait_pred.self_ty().no_bound_vars() else {
-            return false;
-        };
+        // Skipping binder here, remapping below
+        let self_ty = trait_pred.self_ty().skip_binder();
 
         let (def_id, output_ty, callable) = match *self_ty.kind() {
             ty::Closure(def_id, substs) => (def_id, substs.as_closure().sig().output(), "closure"),
@@ -731,14 +737,15 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
         };
         let msg = format!("use parentheses to call the {}", callable);
 
-        // `mk_trait_obligation_with_new_self_ty` only works for types with no escaping bound
-        // variables, so bail out if we have any.
-        let Some(output_ty) = output_ty.no_bound_vars() else {
-            return false;
-        };
+        // "We should really create a single list of bound vars from the combined vars
+        // from the predicate and function, but instead we just liberate the function bound vars"
+        let output_ty = self.tcx.liberate_late_bound_regions(def_id, output_ty);
+
+        // Remapping bound vars here
+        let trait_pred_and_self = trait_pred.map_bound(|trait_pred| (trait_pred, output_ty));
 
         let new_obligation =
-            self.mk_trait_obligation_with_new_self_ty(obligation.param_env, trait_pred, output_ty);
+            self.mk_trait_obligation_with_new_self_ty(obligation.param_env, trait_pred_and_self);
 
         match self.evaluate_obligation(&new_obligation) {
             Ok(
@@ -842,96 +849,97 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
         let param_env = obligation.param_env;
 
         // Try to apply the original trait binding obligation by borrowing.
-        let mut try_borrowing = |old_pred: ty::PolyTraitPredicate<'tcx>,
-                                 blacklist: &[DefId]|
-         -> bool {
-            if blacklist.contains(&old_pred.def_id()) {
-                return false;
-            }
-
-            // This is a quick fix to resolve an ICE (#96223).
-            // This change should probably be deeper.
-            // As suggested by @jackh726, `mk_trait_obligation_with_new_self_ty` could take a `Binder<(TraitRef, Ty)>
-            // instead of `Binder<Ty>` leading to some changes to its call places.
-            let Some(orig_ty) = old_pred.self_ty().no_bound_vars() else {
-                return false;
-            };
-            let mk_result = |new_ty| {
-                let obligation =
-                    self.mk_trait_obligation_with_new_self_ty(param_env, old_pred, new_ty);
-                self.predicate_must_hold_modulo_regions(&obligation)
-            };
-            let imm_result = mk_result(self.tcx.mk_imm_ref(self.tcx.lifetimes.re_static, orig_ty));
-            let mut_result = mk_result(self.tcx.mk_mut_ref(self.tcx.lifetimes.re_static, orig_ty));
-
-            if imm_result || mut_result {
-                if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
-                    // We have a very specific type of error, where just borrowing this argument
-                    // might solve the problem. In cases like this, the important part is the
-                    // original type obligation, not the last one that failed, which is arbitrary.
-                    // Because of this, we modify the error to refer to the original obligation and
-                    // return early in the caller.
-
-                    let msg = format!(
-                        "the trait bound `{}: {}` is not satisfied",
-                        orig_ty,
-                        old_pred.print_modifiers_and_trait_path(),
-                    );
-                    if has_custom_message {
-                        err.note(&msg);
-                    } else {
-                        err.message =
-                            vec![(rustc_errors::DiagnosticMessage::Str(msg), Style::NoStyle)];
-                    }
-                    if snippet.starts_with('&') {
-                        // This is already a literal borrow and the obligation is failing
-                        // somewhere else in the obligation chain. Do not suggest non-sense.
-                        return false;
-                    }
-                    err.span_label(
-                        span,
-                        &format!(
-                            "expected an implementor of trait `{}`",
-                            old_pred.print_modifiers_and_trait_path(),
-                        ),
-                    );
+        let mut try_borrowing =
+            |old_pred: ty::PolyTraitPredicate<'tcx>, blacklist: &[DefId]| -> bool {
+                if blacklist.contains(&old_pred.def_id()) {
+                    return false;
+                }
+                // We map bounds to `&T` and `&mut T`
+                let trait_pred_and_imm_ref = old_pred.map_bound(|trait_pred| {
+                    (
+                        trait_pred,
+                        self.tcx.mk_imm_ref(self.tcx.lifetimes.re_static, trait_pred.self_ty()),
+                    )
+                });
+                let trait_pred_and_mut_ref = old_pred.map_bound(|trait_pred| {
+                    (
+                        trait_pred,
+                        self.tcx.mk_mut_ref(self.tcx.lifetimes.re_static, trait_pred.self_ty()),
+                    )
+                });
 
-                    // This if is to prevent a special edge-case
-                    if matches!(
-                        span.ctxt().outer_expn_data().kind,
-                        ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop)
-                    ) {
-                        // We don't want a borrowing suggestion on the fields in structs,
-                        // ```
-                        // struct Foo {
-                        //  the_foos: Vec<Foo>
-                        // }
-                        // ```
-
-                        if imm_result && mut_result {
-                            err.span_suggestions(
-                                span.shrink_to_lo(),
-                                "consider borrowing here",
-                                ["&".to_string(), "&mut ".to_string()].into_iter(),
-                                Applicability::MaybeIncorrect,
-                            );
+                let mk_result = |trait_pred_and_new_ty| {
+                    let obligation =
+                        self.mk_trait_obligation_with_new_self_ty(param_env, trait_pred_and_new_ty);
+                    self.predicate_must_hold_modulo_regions(&obligation)
+                };
+                let imm_result = mk_result(trait_pred_and_imm_ref);
+                let mut_result = mk_result(trait_pred_and_mut_ref);
+
+                if imm_result || mut_result {
+                    if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
+                        // We have a very specific type of error, where just borrowing this argument
+                        // might solve the problem. In cases like this, the important part is the
+                        // original type obligation, not the last one that failed, which is arbitrary.
+                        // Because of this, we modify the error to refer to the original obligation and
+                        // return early in the caller.
+
+                        let msg = format!("the trait bound `{}` is not satisfied", old_pred);
+                        if has_custom_message {
+                            err.note(&msg);
                         } else {
-                            err.span_suggestion_verbose(
-                                span.shrink_to_lo(),
-                                &format!(
-                                    "consider{} borrowing here",
-                                    if mut_result { " mutably" } else { "" }
-                                ),
-                                format!("&{}", if mut_result { "mut " } else { "" }),
-                                Applicability::MaybeIncorrect,
-                            );
+                            err.message =
+                                vec![(rustc_errors::DiagnosticMessage::Str(msg), Style::NoStyle)];
                         }
+                        if snippet.starts_with('&') {
+                            // This is already a literal borrow and the obligation is failing
+                            // somewhere else in the obligation chain. Do not suggest non-sense.
+                            return false;
+                        }
+                        err.span_label(
+                            span,
+                            &format!(
+                                "expected an implementor of trait `{}`",
+                                old_pred.print_modifiers_and_trait_path(),
+                            ),
+                        );
+
+                        // This if is to prevent a special edge-case
+                        if matches!(
+                            span.ctxt().outer_expn_data().kind,
+                            ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop)
+                        ) {
+                            // We don't want a borrowing suggestion on the fields in structs,
+                            // ```
+                            // struct Foo {
+                            //  the_foos: Vec<Foo>
+                            // }
+                            // ```
+
+                            if imm_result && mut_result {
+                                err.span_suggestions(
+                                    span.shrink_to_lo(),
+                                    "consider borrowing here",
+                                    ["&".to_string(), "&mut ".to_string()].into_iter(),
+                                    Applicability::MaybeIncorrect,
+                                );
+                            } else {
+                                err.span_suggestion_verbose(
+                                    span.shrink_to_lo(),
+                                    &format!(
+                                        "consider{} borrowing here",
+                                        if mut_result { " mutably" } else { "" }
+                                    ),
+                                    format!("&{}", if mut_result { "mut " } else { "" }),
+                                    Applicability::MaybeIncorrect,
+                                );
+                            }
+                        }
+                        return true;
                     }
-                    return true;
                 }
-            }
-            return false;
-        };
+                return false;
+            };
 
         if let ObligationCauseCode::ImplDerivedObligation(cause) = &*code {
             try_borrowing(cause.derived.parent_trait_pred, &[])
@@ -992,20 +1000,22 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                 return false;
             }
 
-            let Some(mut suggested_ty) = trait_pred.self_ty().no_bound_vars() else {
-                return false;
-            };
+            // Skipping binder here, remapping below
+            let mut suggested_ty = trait_pred.self_ty().skip_binder();
 
             for refs_remaining in 0..refs_number {
                 let ty::Ref(_, inner_ty, _) = suggested_ty.kind() else {
                     break;
                 };
                 suggested_ty = *inner_ty;
 
+                // Remapping bound vars here
+                let trait_pred_and_suggested_ty =
+                    trait_pred.map_bound(|trait_pred| (trait_pred, suggested_ty));
+
                 let new_obligation = self.mk_trait_obligation_with_new_self_ty(
                     obligation.param_env,
-                    trait_pred,
-                    suggested_ty,
+                    trait_pred_and_suggested_ty,
                 );
 
                 if self.predicate_may_hold(&new_obligation) {
@@ -1125,26 +1135,21 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                 return;
             }
 
+            // Skipping binder here, remapping below
             if let ty::Ref(region, t_type, mutability) = *trait_pred.skip_binder().self_ty().kind()
             {
-                if region.is_late_bound() || t_type.has_escaping_bound_vars() {
-                    // Avoid debug assertion in `mk_obligation_for_def_id`.
-                    //
-                    // If the self type has escaping bound vars then it's not
-                    // going to be the type of an expression, so the suggestion
-                    // probably won't apply anyway.
-                    return;
-                }
-
                 let suggested_ty = match mutability {
                     hir::Mutability::Mut => self.tcx.mk_imm_ref(region, t_type),
                     hir::Mutability::Not => self.tcx.mk_mut_ref(region, t_type),
                 };
 
+                // Remapping bound vars here
+                let trait_pred_and_suggested_ty =
+                    trait_pred.map_bound(|trait_pred| (trait_pred, suggested_ty));
+
                 let new_obligation = self.mk_trait_obligation_with_new_self_ty(
                     obligation.param_env,
-                    trait_pred,
-                    suggested_ty,
+                    trait_pred_and_suggested_ty,
                 );
                 let suggested_ty_would_satisfy_obligation = self
                     .evaluate_obligation_no_overflow(&new_obligation)
@@ -1195,7 +1200,9 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             // Only suggest this if the expression behind the semicolon implements the predicate
             && let Some(typeck_results) = self.in_progress_typeck_results
             && let Some(ty) = typeck_results.borrow().expr_ty_opt(expr)
-            && self.predicate_may_hold(&self.mk_trait_obligation_with_new_self_ty(obligation.param_env, trait_pred, ty))
+            && self.predicate_may_hold(&self.mk_trait_obligation_with_new_self_ty(
+                obligation.param_env, trait_pred.map_bound(|trait_pred| (trait_pred, ty))
+            ))
         {
             err.span_label(
                 expr.span,
@@ -2727,8 +2734,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                 );
                 let try_obligation = self.mk_trait_obligation_with_new_self_ty(
                     obligation.param_env,
-                    trait_pred,
-                    normalized_ty.ty().unwrap(),
+                    trait_pred.map_bound(|trait_pred| (trait_pred, normalized_ty.ty().unwrap())),
                 );
                 debug!("suggest_await_before_try: try_trait_obligation {:?}", try_obligation);
                 if self.predicate_may_hold(&try_obligation)

src/test/ui/binop/issue-77910-1.stderr

@@ -12,20 +12,14 @@ LL |     assert_eq!(foo, y);
 error[E0277]: `for<'r> fn(&'r i32) -> &'r i32 {foo}` doesn't implement `Debug`
   --> $DIR/issue-77910-1.rs:8:5
    |
+LL | fn foo(s: &i32) -> &i32 {
+   |    --- consider calling this function
+...
 LL |     assert_eq!(foo, y);
    |     ^^^^^^^^^^^^^^^^^^ `for<'r> fn(&'r i32) -> &'r i32 {foo}` cannot be formatted using `{:?}` because it doesn't implement `Debug`
    |
    = help: the trait `Debug` is not implemented for `for<'r> fn(&'r i32) -> &'r i32 {foo}`
-   = help: the following other types implement trait `Debug`:
-             extern "C" fn() -> Ret
-             extern "C" fn(A) -> Ret
-             extern "C" fn(A, ...) -> Ret
-             extern "C" fn(A, B) -> Ret
-             extern "C" fn(A, B, ...) -> Ret
-             extern "C" fn(A, B, C) -> Ret
-             extern "C" fn(A, B, C, ...) -> Ret
-             extern "C" fn(A, B, C, D) -> Ret
-           and 68 others
+   = help: use parentheses to call the function: `foo(s)`
    = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: aborting due to 2 previous errors