Auto merge of #100654 - compiler-errors:rework-point-at-arg, r=estebank

Rework "point at arg" suggestions to be more accurate

Fixes #100560

Introduce a new set of `ObligationCauseCode`s which have additional bookeeping for what expression caused the obligation, and which predicate caused the obligation. This allows us to look at the _unsubstituted_ signature to find out which parameter or generic type argument caused an obligaton to fail.

This means that (in most cases) we significantly improve the likelihood of pointing out the right argument that causes a fulfillment error. Also, since this logic isn't happening in just the `select_where_possible_and_mutate_fulfillment()` calls in the argument checking code, but instead during all trait selection in `FnCtxt`, we are also able to point out the correct argument even if inference means that we don't know whether an obligation has failed until well after a call expression has been checked.

r? `@ghost`

6 files changed, 156 insertions, 148 deletions

diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
index 1a39a168038..70fac83325a 100644
--- a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
+++ b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
@@ -860,8 +860,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                             }
                         }
 
-                        err.emit();
-                        return;
+                        err
                     }
 
                     ty::PredicateKind::WellFormed(ty) => {
@@ -1564,6 +1563,8 @@ impl<'a, 'tcx> InferCtxtPrivExt<'a, 'tcx> for InferCtxt<'a, 'tcx> {
                     obligation.cause.code().peel_derives(),
                     ObligationCauseCode::ItemObligation(_)
                         | ObligationCauseCode::BindingObligation(_, _)
+                        | ObligationCauseCode::ExprItemObligation(..)
+                        | ObligationCauseCode::ExprBindingObligation(..)
                         | ObligationCauseCode::ObjectCastObligation(..)
                         | ObligationCauseCode::OpaqueType
                 );
@@ -2091,13 +2092,11 @@ impl<'a, 'tcx> InferCtxtPrivExt<'a, 'tcx> for InferCtxt<'a, 'tcx> {
                     }
                 }
 
-                if let ObligationCauseCode::ItemObligation(def_id) = *obligation.cause.code() {
+                if let ObligationCauseCode::ItemObligation(def_id) | ObligationCauseCode::ExprItemObligation(def_id, ..) = *obligation.cause.code() {
                     self.suggest_fully_qualified_path(&mut err, def_id, span, trait_ref.def_id());
-                } else if let (
-                    Ok(ref snippet),
-                    &ObligationCauseCode::BindingObligation(def_id, _),
-                ) =
-                    (self.tcx.sess.source_map().span_to_snippet(span), obligation.cause.code())
+                } else if let Ok(snippet) = &self.tcx.sess.source_map().span_to_snippet(span)
+                    && let ObligationCauseCode::BindingObligation(def_id, _) | ObligationCauseCode::ExprBindingObligation(def_id, ..)
+                        = *obligation.cause.code()
                 {
                     let generics = self.tcx.generics_of(def_id);
                     if generics.params.iter().any(|p| p.name != kw::SelfUpper)
@@ -2520,15 +2519,10 @@ impl<'a, 'tcx> InferCtxtPrivExt<'a, 'tcx> for InferCtxt<'a, 'tcx> {
         err: &mut Diagnostic,
         obligation: &PredicateObligation<'tcx>,
     ) {
-        let (
-            ty::PredicateKind::Trait(pred),
-            &ObligationCauseCode::BindingObligation(item_def_id, span),
-        ) = (
-            obligation.predicate.kind().skip_binder(),
-            obligation.cause.code().peel_derives(),
-        )  else {
-            return;
-        };
+        let ty::PredicateKind::Trait(pred) = obligation.predicate.kind().skip_binder() else { return; };
+        let (ObligationCauseCode::BindingObligation(item_def_id, span)
+        | ObligationCauseCode::ExprBindingObligation(item_def_id, span, ..))
+            = *obligation.cause.code().peel_derives() else { return; };
         debug!(?pred, ?item_def_id, ?span);
 
         let (Some(node), true) = (
diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs
index e6907637c57..6b03555bc69 100644
--- a/compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs
+++ b/compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs
@@ -143,7 +143,9 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
         }
 
         if let ObligationCauseCode::ItemObligation(item)
-        | ObligationCauseCode::BindingObligation(item, _) = *obligation.cause.code()
+        | ObligationCauseCode::BindingObligation(item, _)
+        | ObligationCauseCode::ExprItemObligation(item, ..)
+        | ObligationCauseCode::ExprBindingObligation(item, ..) = *obligation.cause.code()
         {
             // FIXME: maybe also have some way of handling methods
             // from other traits? That would require name resolution,
diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
index 0d279069694..a93f9ec0397 100644
--- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
+++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
@@ -671,11 +671,16 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
         trait_pred: ty::PolyTraitPredicate<'tcx>,
     ) -> bool {
         // It only make sense when suggesting dereferences for arguments
-        let ObligationCauseCode::FunctionArgumentObligation { arg_hir_id, .. } = obligation.cause.code() else {
-            return false;
-        };
-        let param_env = obligation.param_env;
-        let body_id = obligation.cause.body_id;
+        let ObligationCauseCode::FunctionArgumentObligation { arg_hir_id, .. } = obligation.cause.code()
+            else { return false; };
+        let Some(typeck_results) = self.in_progress_typeck_results
+            else { return false; };
+        let typeck_results = typeck_results.borrow();
+        let hir::Node::Expr(expr) = self.tcx.hir().get(*arg_hir_id)
+            else { return false; };
+        let Some(arg_ty) = typeck_results.expr_ty_adjusted_opt(expr)
+            else { return false; };
+
         let span = obligation.cause.span;
         let mut real_trait_pred = trait_pred;
         let mut code = obligation.cause.code();
@@ -687,9 +692,19 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
 
             // Skipping binder here, remapping below
             let real_ty = real_trait_pred.self_ty().skip_binder();
+            if self.can_eq(obligation.param_env, real_ty, arg_ty).is_err() {
+                continue;
+            }
 
             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);
+                let mut autoderef = Autoderef::new(
+                    self,
+                    obligation.param_env,
+                    obligation.cause.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 });
@@ -697,8 +712,10 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                     // 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);
+                    let obligation = self.mk_trait_obligation_with_new_self_ty(
+                        obligation.param_env,
+                        real_trait_pred_and_ty,
+                    );
                     Some(steps).filter(|_| self.predicate_may_hold(&obligation))
                 }) {
                     if steps > 0 {
@@ -727,7 +744,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                     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,
+                        obligation.param_env,
                         real_trait_pred_and_base_ty,
                     );
                     if self.predicate_may_hold(&obligation) {
@@ -855,6 +872,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             _ => return false,
         };
         if matches!(obligation.cause.code(), ObligationCauseCode::FunctionArgumentObligation { .. })
+            && obligation.cause.span.can_be_used_for_suggestions()
         {
             // When the obligation error has been ensured to have been caused by
             // an argument, the `obligation.cause.span` points at the expression
@@ -885,7 +903,9 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             obligation.cause.code()
         {
             &parent_code
-        } else if let ObligationCauseCode::ItemObligation(_) = obligation.cause.code() {
+        } else if let ObligationCauseCode::ItemObligation(_)
+        | ObligationCauseCode::ExprItemObligation(..) = obligation.cause.code()
+        {
             obligation.cause.code()
         } else if let ExpnKind::Desugaring(DesugaringKind::ForLoop) =
             span.ctxt().outer_expn_data().kind
@@ -911,35 +931,36 @@ 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;
-                }
-                // 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()),
-                    )
-                });
+        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()),
+                )
+            });
 
-                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_ref_self_ty_satisfies_pred = mk_result(trait_pred_and_imm_ref);
-                let mut_ref_self_ty_satisfies_pred = mk_result(trait_pred_and_mut_ref);
+            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_ref_self_ty_satisfies_pred = mk_result(trait_pred_and_imm_ref);
+            let mut_ref_self_ty_satisfies_pred = mk_result(trait_pred_and_mut_ref);
 
-                let (ref_inner_ty_satisfies_pred, ref_inner_ty_mut) =
-                if let ObligationCauseCode::ItemObligation(_) = obligation.cause.code()
+            let (ref_inner_ty_satisfies_pred, ref_inner_ty_mut) =
+                if let ObligationCauseCode::ItemObligation(_) | ObligationCauseCode::ExprItemObligation(..) = obligation.cause.code()
                     && let ty::Ref(_, ty, mutability) = old_pred.self_ty().skip_binder().kind()
                 {
                     (
@@ -950,79 +971,81 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                     (false, false)
                 };
 
-                if imm_ref_self_ty_satisfies_pred
-                    || mut_ref_self_ty_satisfies_pred
-                    || ref_inner_ty_satisfies_pred
-                {
-                    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.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,
+            if imm_ref_self_ty_satisfies_pred
+                || mut_ref_self_ty_satisfies_pred
+                || ref_inner_ty_satisfies_pred
+            {
+                if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
+                    // We don't want a borrowing suggestion on the fields in structs,
+                    // ```
+                    // struct Foo {
+                    //  the_foos: Vec<Foo>
+                    // }
+                    // ```
+                    if !matches!(
+                        span.ctxt().outer_expn_data().kind,
+                        ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop)
+                    ) {
+                        return false;
+                    }
+                    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;
+                    }
+                    // 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.message =
+                            vec![(rustc_errors::DiagnosticMessage::Str(msg), Style::NoStyle)];
+                    }
+                    err.span_label(
+                        span,
+                        format!(
+                            "the trait `{}` is not implemented for `{}`",
+                            old_pred.print_modifiers_and_trait_path(),
+                            old_pred.self_ty().skip_binder(),
+                        ),
+                    );
+
+                    if imm_ref_self_ty_satisfies_pred && mut_ref_self_ty_satisfies_pred {
+                        err.span_suggestions(
+                            span.shrink_to_lo(),
+                            "consider borrowing here",
+                            ["&".to_string(), "&mut ".to_string()].into_iter(),
+                            Applicability::MaybeIncorrect,
+                        );
+                    } else {
+                        let is_mut = mut_ref_self_ty_satisfies_pred || ref_inner_ty_mut;
+                        err.span_suggestion_verbose(
+                            span.shrink_to_lo(),
                             &format!(
-                                "expected an implementor of trait `{}`",
-                                old_pred.print_modifiers_and_trait_path(),
+                                "consider{} borrowing here",
+                                if is_mut { " mutably" } else { "" }
                             ),
+                            format!("&{}", if is_mut { "mut " } else { "" }),
+                            Applicability::MaybeIncorrect,
                         );
-
-                        // 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_ref_self_ty_satisfies_pred && mut_ref_self_ty_satisfies_pred {
-                                err.span_suggestions(
-                                    span.shrink_to_lo(),
-                                    "consider borrowing here",
-                                    ["&".to_string(), "&mut ".to_string()].into_iter(),
-                                    Applicability::MaybeIncorrect,
-                                );
-                            } else {
-                                let is_mut = mut_ref_self_ty_satisfies_pred || ref_inner_ty_mut;
-                                err.span_suggestion_verbose(
-                                    span.shrink_to_lo(),
-                                    &format!(
-                                        "consider{} borrowing here",
-                                        if is_mut { " mutably" } else { "" }
-                                    ),
-                                    format!("&{}", if is_mut { "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, &[])
         } else if let ObligationCauseCode::BindingObligation(_, _)
-        | ObligationCauseCode::ItemObligation(..) = code
+        | ObligationCauseCode::ItemObligation(_)
+        | ObligationCauseCode::ExprItemObligation(..)
+        | ObligationCauseCode::ExprBindingObligation(..) = code
         {
             try_borrowing(poly_trait_pred, &never_suggest_borrow)
         } else {
@@ -2244,11 +2267,13 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                     region, object_ty,
                 ));
             }
-            ObligationCauseCode::ItemObligation(_item_def_id) => {
+            ObligationCauseCode::ItemObligation(_)
+            | ObligationCauseCode::ExprItemObligation(..) => {
                 // We hold the `DefId` of the item introducing the obligation, but displaying it
                 // doesn't add user usable information. It always point at an associated item.
             }
-            ObligationCauseCode::BindingObligation(item_def_id, span) => {
+            ObligationCauseCode::BindingObligation(item_def_id, span)
+            | ObligationCauseCode::ExprBindingObligation(item_def_id, span, ..) => {
                 let item_name = tcx.def_path_str(item_def_id);
                 let mut multispan = MultiSpan::from(span);
                 if let Some(ident) = tcx.opt_item_ident(item_def_id) {
diff --git a/compiler/rustc_trait_selection/src/traits/mod.rs b/compiler/rustc_trait_selection/src/traits/mod.rs
index b6d6df1eec6..85ff6e23711 100644
--- a/compiler/rustc_trait_selection/src/traits/mod.rs
+++ b/compiler/rustc_trait_selection/src/traits/mod.rs
@@ -117,11 +117,21 @@ pub enum TraitQueryMode {
 
 /// Creates predicate obligations from the generic bounds.
 pub fn predicates_for_generics<'tcx>(
-    cause: ObligationCause<'tcx>,
+    cause: impl Fn(usize, Span) -> ObligationCause<'tcx>,
     param_env: ty::ParamEnv<'tcx>,
     generic_bounds: ty::InstantiatedPredicates<'tcx>,
 ) -> impl Iterator<Item = PredicateObligation<'tcx>> {
-    util::predicates_for_generics(cause, 0, param_env, generic_bounds)
+    let generic_bounds = generic_bounds;
+    debug!("predicates_for_generics(generic_bounds={:?})", generic_bounds);
+
+    std::iter::zip(generic_bounds.predicates, generic_bounds.spans).enumerate().map(
+        move |(idx, (predicate, span))| Obligation {
+            cause: cause(idx, span),
+            recursion_depth: 0,
+            param_env: param_env,
+            predicate,
+        },
+    )
 }
 
 /// Determines whether the type `ty` is known to meet `bound` and
diff --git a/compiler/rustc_trait_selection/src/traits/util.rs b/compiler/rustc_trait_selection/src/traits/util.rs
index d2500601662..0f5dff01c66 100644
--- a/compiler/rustc_trait_selection/src/traits/util.rs
+++ b/compiler/rustc_trait_selection/src/traits/util.rs
@@ -11,8 +11,6 @@ use rustc_middle::ty::{self, ImplSubject, ToPredicate, Ty, TyCtxt, TypeVisitable
 use super::{Normalized, Obligation, ObligationCause, PredicateObligation, SelectionContext};
 pub use rustc_infer::traits::{self, util::*};
 
-use std::iter;
-
 ///////////////////////////////////////////////////////////////////////////
 // `TraitAliasExpander` iterator
 ///////////////////////////////////////////////////////////////////////////
@@ -210,7 +208,7 @@ pub fn impl_subject_and_oblig<'a, 'tcx>(
     let Normalized { value: predicates, obligations: normalization_obligations2 } =
         super::normalize(selcx, param_env, ObligationCause::dummy(), predicates);
     let impl_obligations =
-        predicates_for_generics(ObligationCause::dummy(), 0, param_env, predicates);
+        super::predicates_for_generics(|_, _| ObligationCause::dummy(), param_env, predicates);
 
     let impl_obligations = impl_obligations
         .chain(normalization_obligations1.into_iter())
@@ -219,27 +217,6 @@ pub fn impl_subject_and_oblig<'a, 'tcx>(
     (subject, impl_obligations)
 }
 
-pub fn predicates_for_generics<'tcx>(
-    cause: ObligationCause<'tcx>,
-    recursion_depth: usize,
-    param_env: ty::ParamEnv<'tcx>,
-    generic_bounds: ty::InstantiatedPredicates<'tcx>,
-) -> impl Iterator<Item = PredicateObligation<'tcx>> {
-    debug!("predicates_for_generics(generic_bounds={:?})", generic_bounds);
-
-    iter::zip(generic_bounds.predicates, generic_bounds.spans).map(move |(predicate, span)| {
-        let cause = match *cause.code() {
-            traits::ItemObligation(def_id) if !span.is_dummy() => traits::ObligationCause::new(
-                cause.span,
-                cause.body_id,
-                traits::BindingObligation(def_id, span),
-            ),
-            _ => cause.clone(),
-        };
-        Obligation { cause, recursion_depth, param_env, predicate }
-    })
-}
-
 pub fn predicate_for_trait_ref<'tcx>(
     tcx: TyCtxt<'tcx>,
     cause: ObligationCause<'tcx>,
diff --git a/compiler/rustc_trait_selection/src/traits/wf.rs b/compiler/rustc_trait_selection/src/traits/wf.rs
index 7a5e67ff74b..4bd179d2391 100644
--- a/compiler/rustc_trait_selection/src/traits/wf.rs
+++ b/compiler/rustc_trait_selection/src/traits/wf.rs
@@ -711,7 +711,7 @@ impl<'tcx> WfPredicates<'tcx> {
         iter::zip(iter::zip(predicates.predicates, predicates.spans), origins.into_iter().rev())
             .map(|((mut pred, span), origin_def_id)| {
                 let code = if span.is_dummy() {
-                    traits::MiscObligation
+                    traits::ItemObligation(origin_def_id)
                 } else {
                     traits::BindingObligation(origin_def_id, span)
                 };