Auto merge of #89580 - estebank:trait-bounds-are-tricky, r=nagisa

Point at source of trait bound obligations in more places

Be more thorough in using `ItemObligation` and `BindingObligation` when
evaluating obligations so that we can point at trait bounds that
introduced unfulfilled obligations. We no longer incorrectly point at
unrelated trait bounds (`substs-ppaux.verbose.stderr`).

In particular, we now point at trait bounds on method calls.

We no longer point at "obvious" obligation sources (we no longer have a
note pointing at `Trait` saying "required by a bound in `Trait`", like
in `associated-types-no-suitable-supertrait*`).

We no longer point at associated items (`ImplObligation`), as they didn't
add any user actionable information, they just added noise.

Address part of #89418.

Author: bors

Diff for: compiler/rustc_errors/src/emitter.rs

@@ -1266,22 +1266,37 @@ impl EmitterWriter {
             }
             self.msg_to_buffer(&mut buffer, msg, max_line_num_len, "note", None);
         } else {
+            let mut label_width = 0;
             // The failure note level itself does not provide any useful diagnostic information
             if *level != Level::FailureNote {
                 buffer.append(0, level.to_str(), Style::Level(*level));
+                label_width += level.to_str().len();
             }
             // only render error codes, not lint codes
             if let Some(DiagnosticId::Error(ref code)) = *code {
                 buffer.append(0, "[", Style::Level(*level));
                 buffer.append(0, &code, Style::Level(*level));
                 buffer.append(0, "]", Style::Level(*level));
+                label_width += 2 + code.len();
             }
             let header_style = if is_secondary { Style::HeaderMsg } else { Style::MainHeaderMsg };
             if *level != Level::FailureNote {
                 buffer.append(0, ": ", header_style);
+                label_width += 2;
             }
             for &(ref text, _) in msg.iter() {
-                buffer.append(0, &replace_tabs(text), header_style);
+                // Account for newlines to align output to its label.
+                for (line, text) in replace_tabs(text).lines().enumerate() {
+                    buffer.append(
+                        0 + line,
+                        &format!(
+                            "{}{}",
+                            if line == 0 { String::new() } else { " ".repeat(label_width) },
+                            text
+                        ),
+                        header_style,
+                    );
+                }
             }
         }
 

Diff for: compiler/rustc_infer/src/infer/error_reporting/mod.rs

@@ -2141,10 +2141,19 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                         None
                     },
                     self.tcx.generics_of(owner.to_def_id()),
+                    hir.span(hir_id),
                 )
             });
+
+        let span = match generics {
+            // This is to get around the trait identity obligation, that has a `DUMMY_SP` as signal
+            // for other diagnostics, so we need to recover it here.
+            Some((_, _, node)) if span.is_dummy() => node,
+            _ => span,
+        };
+
         let type_param_span = match (generics, bound_kind) {
-            (Some((_, ref generics)), GenericKind::Param(ref param)) => {
+            (Some((_, ref generics, _)), GenericKind::Param(ref param)) => {
                 // Account for the case where `param` corresponds to `Self`,
                 // which doesn't have the expected type argument.
                 if !(generics.has_self && param.index == 0) {
@@ -2181,7 +2190,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
         };
         let new_lt = generics
             .as_ref()
-            .and_then(|(parent_g, g)| {
+            .and_then(|(parent_g, g, _)| {
                 let mut possible = (b'a'..=b'z').map(|c| format!("'{}", c as char));
                 let mut lts_names = g
                     .params
@@ -2203,7 +2212,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
             .unwrap_or("'lt".to_string());
         let add_lt_sugg = generics
             .as_ref()
-            .and_then(|(_, g)| g.params.first())
+            .and_then(|(_, g, _)| g.params.first())
             .and_then(|param| param.def_id.as_local())
             .map(|def_id| {
                 (

Diff for: compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

@@ -192,14 +192,16 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
                 ObligationCauseCode::MatchImpl(parent, ..) => &parent.code,
                 _ => &cause.code,
             };
-            if let ObligationCauseCode::ItemObligation(item_def_id) = *code {
+            if let (ObligationCauseCode::ItemObligation(item_def_id), None) =
+                (code, override_error_code)
+            {
                 // Same case of `impl Foo for dyn Bar { fn qux(&self) {} }` introducing a `'static`
                 // lifetime as above, but called using a fully-qualified path to the method:
                 // `Foo::qux(bar)`.
                 let mut v = TraitObjectVisitor(FxHashSet::default());
                 v.visit_ty(param.param_ty);
                 if let Some((ident, self_ty)) =
-                    self.get_impl_ident_and_self_ty_from_trait(item_def_id, &v.0)
+                    self.get_impl_ident_and_self_ty_from_trait(*item_def_id, &v.0)
                 {
                     if self.suggest_constrain_dyn_trait_in_impl(&mut err, &v.0, ident, self_ty) {
                         override_error_code = Some(ident);

Diff for: compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

@@ -1958,15 +1958,9 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                     region, object_ty,
                 ));
             }
-            ObligationCauseCode::ItemObligation(item_def_id) => {
-                let item_name = tcx.def_path_str(item_def_id);
-                let msg = format!("required by `{}`", item_name);
-                let sp = tcx
-                    .hir()
-                    .span_if_local(item_def_id)
-                    .unwrap_or_else(|| tcx.def_span(item_def_id));
-                let sp = tcx.sess.source_map().guess_head_span(sp);
-                err.span_note(sp, &msg);
+            ObligationCauseCode::ItemObligation(_item_def_id) => {
+                // 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) => {
                 let item_name = tcx.def_path_str(item_def_id);

Diff for: compiler/rustc_trait_selection/src/traits/util.rs

@@ -9,7 +9,9 @@ use rustc_middle::ty::subst::{GenericArg, Subst, SubstsRef};
 use rustc_middle::ty::{self, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness};
 
 use super::{Normalized, Obligation, ObligationCause, PredicateObligation, SelectionContext};
-pub use rustc_infer::traits::util::*;
+pub use rustc_infer::traits::{self, util::*};
+
+use std::iter;
 
 ///////////////////////////////////////////////////////////////////////////
 // `TraitAliasExpander` iterator
@@ -229,11 +231,16 @@ pub fn predicates_for_generics<'tcx>(
 ) -> impl Iterator<Item = PredicateObligation<'tcx>> {
     debug!("predicates_for_generics(generic_bounds={:?})", generic_bounds);
 
-    generic_bounds.predicates.into_iter().map(move |predicate| Obligation {
-        cause: cause.clone(),
-        recursion_depth,
-        param_env,
-        predicate,
+    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 }
     })
 }
 

Diff for: compiler/rustc_trait_selection/src/traits/wf.rs

@@ -709,7 +709,12 @@ impl<'a, 'tcx> WfPredicates<'a, 'tcx> {
 
         iter::zip(iter::zip(predicates.predicates, predicates.spans), origins.into_iter().rev())
             .map(|((pred, span), origin_def_id)| {
-                let cause = self.cause(traits::BindingObligation(origin_def_id, span));
+                let code = if span.is_dummy() {
+                    traits::MiscObligation
+                } else {
+                    traits::BindingObligation(origin_def_id, span)
+                };
+                let cause = self.cause(code);
                 traits::Obligation::with_depth(cause, self.recursion_depth, self.param_env, pred)
             })
             .filter(|pred| !pred.has_escaping_bound_vars())

Diff for: compiler/rustc_typeck/src/check/compare_method.rs

@@ -210,12 +210,8 @@ fn compare_predicate_entailment<'tcx>(
     let normalize_cause = traits::ObligationCause::misc(impl_m_span, impl_m_hir_id);
     let param_env =
         ty::ParamEnv::new(tcx.intern_predicates(&hybrid_preds.predicates), Reveal::UserFacing);
-    let param_env = traits::normalize_param_env_or_error(
-        tcx,
-        impl_m.def_id,
-        param_env,
-        normalize_cause.clone(),
-    );
+    let param_env =
+        traits::normalize_param_env_or_error(tcx, impl_m.def_id, param_env, normalize_cause);
 
     tcx.infer_ctxt().enter(|infcx| {
         let inh = Inherited::new(infcx, impl_m.def_id.expect_local());
@@ -226,12 +222,15 @@ fn compare_predicate_entailment<'tcx>(
         let mut selcx = traits::SelectionContext::new(&infcx);
 
         let impl_m_own_bounds = impl_m_predicates.instantiate_own(tcx, impl_to_placeholder_substs);
-        for predicate in impl_m_own_bounds.predicates {
+        for (predicate, span) in iter::zip(impl_m_own_bounds.predicates, impl_m_own_bounds.spans) {
+            let normalize_cause = traits::ObligationCause::misc(span, impl_m_hir_id);
             let traits::Normalized { value: predicate, obligations } =
-                traits::normalize(&mut selcx, param_env, normalize_cause.clone(), predicate);
+                traits::normalize(&mut selcx, param_env, normalize_cause, predicate);
 
             inh.register_predicates(obligations);
-            inh.register_predicate(traits::Obligation::new(cause.clone(), param_env, predicate));
+            let mut cause = cause.clone();
+            cause.make_mut().span = span;
+            inh.register_predicate(traits::Obligation::new(cause, param_env, predicate));
         }
 
         // We now need to check that the signature of the impl method is
@@ -280,6 +279,12 @@ fn compare_predicate_entailment<'tcx>(
 
         let sub_result = infcx.at(&cause, param_env).sup(trait_fty, impl_fty).map(
             |InferOk { obligations, .. }| {
+                // FIXME: We'd want to keep more accurate spans than "the method signature" when
+                // processing the comparison between the trait and impl fn, but we sadly lose them
+                // and point at the whole signature when a trait bound or specific input or output
+                // type would be more appropriate. In other places we have a `Vec<Span>`
+                // corresponding to their `Vec<Predicate>`, but we don't have that here.
+                // Fixing this would improve the output of test `issue-83765.rs`.
                 inh.register_predicates(obligations);
             },
         );
@@ -1385,12 +1390,13 @@ pub fn check_type_bounds<'tcx>(
 
         let impl_ty_hir_id = tcx.hir().local_def_id_to_hir_id(impl_ty.def_id.expect_local());
         let normalize_cause = traits::ObligationCause::misc(impl_ty_span, impl_ty_hir_id);
-        let mk_cause = |span| {
-            ObligationCause::new(
-                impl_ty_span,
-                impl_ty_hir_id,
-                ObligationCauseCode::BindingObligation(trait_ty.def_id, span),
-            )
+        let mk_cause = |span: Span| {
+            let code = if span.is_dummy() {
+                traits::MiscObligation
+            } else {
+                traits::BindingObligation(trait_ty.def_id, span)
+            };
+            ObligationCause::new(impl_ty_span, impl_ty_hir_id, code)
         };
 
         let obligations = tcx

Diff for: compiler/rustc_typeck/src/check/fn_ctxt/_impl.rs

@@ -586,38 +586,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         }
     }
 
-    /// Given a fully substituted set of bounds (`generic_bounds`), and the values with which each
-    /// type/region parameter was instantiated (`substs`), creates and registers suitable
-    /// trait/region obligations.
-    ///
-    /// For example, if there is a function:
-    ///
-    /// ```
-    /// fn foo<'a,T:'a>(...)
-    /// ```
-    ///
-    /// and a reference:
-    ///
-    /// ```
-    /// let f = foo;
-    /// ```
-    ///
-    /// Then we will create a fresh region variable `'$0` and a fresh type variable `$1` for `'a`
-    /// and `T`. This routine will add a region obligation `$1:'$0` and register it locally.
-    pub fn add_obligations_for_parameters(
-        &self,
-        cause: traits::ObligationCause<'tcx>,
-        predicates: ty::InstantiatedPredicates<'tcx>,
-    ) {
-        assert!(!predicates.has_escaping_bound_vars());
-
-        debug!("add_obligations_for_parameters(predicates={:?})", predicates);
-
-        for obligation in traits::predicates_for_generics(cause, self.param_env, predicates) {
-            self.register_predicate(obligation);
-        }
-    }
-
     // FIXME(arielb1): use this instead of field.ty everywhere
     // Only for fields! Returns <none> for methods>
     // Indifferent to privacy flags
@@ -1522,20 +1490,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
     /// Add all the obligations that are required, substituting and normalized appropriately.
     #[tracing::instrument(level = "debug", skip(self, span, def_id, substs))]
-    fn add_required_obligations(&self, span: Span, def_id: DefId, substs: &SubstsRef<'tcx>) {
-        let (bounds, spans) = self.instantiate_bounds(span, def_id, &substs);
+    crate fn add_required_obligations(&self, span: Span, def_id: DefId, substs: &SubstsRef<'tcx>) {
+        let (bounds, _) = self.instantiate_bounds(span, def_id, &substs);
 
-        for (i, mut obligation) in traits::predicates_for_generics(
+        for obligation in traits::predicates_for_generics(
             traits::ObligationCause::new(span, self.body_id, traits::ItemObligation(def_id)),
             self.param_env,
             bounds,
-        )
-        .enumerate()
-        {
-            // This makes the error point at the bound, but we want to point at the argument
-            if let Some(span) = spans.get(i) {
-                obligation.cause.make_mut().code = traits::BindingObligation(def_id, *span);
-            }
+        ) {
             self.register_predicate(obligation);
         }
     }

Diff for: compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

@@ -509,10 +509,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             self.write_user_type_annotation_from_substs(hir_id, did, substs, None);
 
             // Check bounds on type arguments used in the path.
-            let (bounds, _) = self.instantiate_bounds(path_span, did, substs);
-            let cause =
-                traits::ObligationCause::new(path_span, self.body_id, traits::ItemObligation(did));
-            self.add_obligations_for_parameters(cause, bounds);
+            self.add_required_obligations(path_span, did, substs);
 
             Some((variant, ty))
         } else {

Diff for: compiler/rustc_typeck/src/check/method/confirm.rs

@@ -120,7 +120,12 @@ impl<'a, 'tcx> ConfirmContext<'a, 'tcx> {
         // We won't add these if we encountered an illegal sized bound, so that we can use
         // a custom error in that case.
         if illegal_sized_bound.is_none() {
-            self.add_obligations(self.tcx.mk_fn_ptr(method_sig), all_substs, method_predicates);
+            self.add_obligations(
+                self.tcx.mk_fn_ptr(method_sig),
+                all_substs,
+                method_predicates,
+                pick.item.def_id,
+            );
         }
 
         // Create the final `MethodCallee`.
@@ -471,16 +476,23 @@ impl<'a, 'tcx> ConfirmContext<'a, 'tcx> {
         fty: Ty<'tcx>,
         all_substs: SubstsRef<'tcx>,
         method_predicates: ty::InstantiatedPredicates<'tcx>,
+        def_id: DefId,
     ) {
         debug!(
-            "add_obligations: fty={:?} all_substs={:?} method_predicates={:?}",
-            fty, all_substs, method_predicates
+            "add_obligations: fty={:?} all_substs={:?} method_predicates={:?} def_id={:?}",
+            fty, all_substs, method_predicates, def_id
         );
 
-        self.add_obligations_for_parameters(
-            traits::ObligationCause::misc(self.span, self.body_id),
+        // FIXME: could replace with the following, but we already calculated `method_predicates`,
+        // so we just call `predicates_for_generics` directly to avoid redoing work.
+        // `self.add_required_obligations(self.span, def_id, &all_substs);`
+        for obligation in traits::predicates_for_generics(
+            traits::ObligationCause::new(self.span, self.body_id, traits::ItemObligation(def_id)),
+            self.param_env,
             method_predicates,
-        );
+        ) {
+            self.register_predicate(obligation);
+        }
 
         // this is a projection from a trait reference, so we have to
         // make sure that the trait reference inputs are well-formed.

Diff for: compiler/rustc_typeck/src/check/method/mod.rs

@@ -12,6 +12,7 @@ pub use self::CandidateSource::*;
 pub use self::MethodError::*;
 
 use crate::check::FnCtxt;
+use crate::ObligationCause;
 use rustc_data_structures::sync::Lrc;
 use rustc_errors::{Applicability, DiagnosticBuilder};
 use rustc_hir as hir;
@@ -71,7 +72,8 @@ pub enum MethodError<'tcx> {
 #[derive(Debug)]
 pub struct NoMatchData<'tcx> {
     pub static_candidates: Vec<CandidateSource>,
-    pub unsatisfied_predicates: Vec<(ty::Predicate<'tcx>, Option<ty::Predicate<'tcx>>)>,
+    pub unsatisfied_predicates:
+        Vec<(ty::Predicate<'tcx>, Option<ty::Predicate<'tcx>>, Option<ObligationCause<'tcx>>)>,
     pub out_of_scope_traits: Vec<DefId>,
     pub lev_candidate: Option<ty::AssocItem>,
     pub mode: probe::Mode,
@@ -80,7 +82,11 @@ pub struct NoMatchData<'tcx> {
 impl<'tcx> NoMatchData<'tcx> {
     pub fn new(
         static_candidates: Vec<CandidateSource>,
-        unsatisfied_predicates: Vec<(ty::Predicate<'tcx>, Option<ty::Predicate<'tcx>>)>,
+        unsatisfied_predicates: Vec<(
+            ty::Predicate<'tcx>,
+            Option<ty::Predicate<'tcx>>,
+            Option<ObligationCause<'tcx>>,
+        )>,
         out_of_scope_traits: Vec<DefId>,
         lev_candidate: Option<ty::AssocItem>,
         mode: probe::Mode,