Rollup merge of rust-lang#106477 - Nathan-Fenner:nathanf/refined-error-span-trait-impl, r=compiler-errors

Refine error spans for "The trait bound `T: Trait` is not satisfied" when passing literal structs/tuples

This PR adds a new heuristic which refines the error span reported for "`T: Trait` is not satisfied" errors, by "drilling down" into individual fields of structs/enums/tuples to point to the "problematic" value.

Here's a self-contained example of the difference in error span:

```rs
struct Burrito<Filling> {
    filling: Filling,
}
impl <Filling: Delicious> Delicious for Burrito<Filling> {}
fn eat_delicious_food<Food: Delicious>(food: Food) {}
fn will_type_error() {
    eat_delicious_food(Burrito { filling: Kale });
    //                 ^~~~~~~~~~~~~~~~~~~~~~~~~ (before) The trait bound `Kale: Delicious` is not satisfied
    //                                    ^~~~   (after)  The trait bound `Kale: Delicious` is not satisfied
}
```
(kale is fine, this is just a silly food-based example)

Before this PR, the error span is identified as the entire argument to the generic function `eat_delicious_food`. However, since only `Kale` is the "problematic" part, we can point at it specifically. In particular, the primary error message itself mentions the missing `Kale: Delicious` trait bound, so it's much clearer if this part is called out explicitly.

---

The _existing_ heuristic tries to label the right function argument in `point_at_arg_if_possible`. It goes something like this:
- Look at the broken base trait `Food: Delicious` and find which generics it mentions (in this case, only `Food`)
- Look at the parameter type definitions and find which of them mention `Filling` (in this case, only `food`)
- If there is exactly one relevant parameter, label the corresponding argument with the error span, instead of the entire call

This PR extends this heuristic by further refining the resulting expression span in the new `point_at_specific_expr_if_possible` function. For each `impl` in the (broken) chain, we apply the following strategy:

The strategy to determine this span involves connecting information about our generic `impl`
with information about our (struct) type and the (struct) literal expression:
- Find the `impl` (`impl <Filling: Delicious> Delicious for Burrito<Filling>`)
  that links our obligation (`Kale: Delicious`) with the parent obligation (`Burrito<Kale>: Delicious`)
- Find the "original" predicate constraint in the impl (`Filling: Delicious`) which produced our obligation.
- Find all of the generics that are mentioned in the predicate (`Filling`).
- Examine the `Self` type in the `impl`, and see which of its type argument(s) mention any of those generics.
- Examing the definition for the `Self` type, and identify (for each of its variants) if there's a unique field
  which uses those generic arguments.
- If there is a unique field mentioning the "blameable" arguments, use that field for the error span.

Before we do any of this logic, we recursively call `point_at_specific_expr_if_possible` on the parent
obligation. Hence we refine the `expr` "outwards-in" and bail at the first kind of expression/impl we don't recognize.

This function returns a `Result<&Expr, &Expr>` - either way, it returns the `Expr` whose span should be
reported as an error. If it is `Ok`, then it means it refined successfull. If it is `Err`, then it may be
only a partial success - but it cannot be refined even further.

---

I added a new test file which exercises this new behavior. A few existing tests were affected, since their error spans are now different. In one case, this leads to a different code suggestion for the autofix - although the new suggestion isn't _wrong_, it is different from what used to be.

This change doesn't create any new errors or remove any existing ones, it just adjusts the spans where they're presented.

---

Some considerations: right now, this check occurs in addition to some similar logic in `adjust_fulfillment_error_for_expr_obligation` function, which tidies up various kinds of error spans (not just trait-fulfillment error). It's possible that this new code would be better integrated into that function (or another one) - but I haven't looked into this yet.

Although this code only occurs when there's a type error, it's definitely not as efficient as possible. In particular, there are definitely some cases where it degrades to quadratic performance (e.g. for a trait `impl` with 100+ generic parameters or 100 levels deep nesting of generic types). I'm not sure if these are realistic enough to worry about optimizing yet.

There's also still a lot of repetition in some of the logic, where the behavior for different types (namely, `struct` vs `enum` variant) is _similar_ but not the same.

---

I think the biggest win here is better targeting for tuples; in particular, if you're using tuples + traits to express variadic-like functions, the compiler can't tell you which part of a tuple has the wrong type, since the span will cover the entire argument. This change allows the individual field in the tuple to be highlighted, as in this example:

```
// NEW
LL |     want(Wrapper { value: (3, q) });
   |     ----                      ^ the trait `T3` is not implemented for `Q`

// OLD
LL |     want(Wrapper { value: (3, q) });
   |     ---- ^~~~~~~~~~~~~~~~~~~~~~~~~ the trait `T3` is not implemented for `Q`
```
Especially with large tuples, the existing error spans are not very effective at quickly narrowing down the source of the problem.

Author: matthiaskrgr

compiler/rustc_hir_typeck/src/fn_ctxt/adjust_fulfillment_errors.rs

@@ -34,9 +34,10 @@ use rustc_trait_selection::traits::{self, ObligationCauseCode, SelectionContext}
 
 use std::iter;
 use std::mem;
-use std::ops::ControlFlow;
 use std::slice;
 
+use std::ops::ControlFlow;
+
 impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     pub(in super::super) fn check_casts(&mut self) {
         // don't hold the borrow to deferred_cast_checks while checking to avoid borrow checker errors
@@ -1843,7 +1844,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         .into_iter()
                         .flatten()
                     {
-                        if self.point_at_arg_if_possible(
+                        if self.blame_specific_arg_if_possible(
                                 error,
                                 def_id,
                                 param,
@@ -1873,7 +1874,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                     .into_iter()
                     .flatten()
                 {
-                    if self.point_at_arg_if_possible(
+                    if self.blame_specific_arg_if_possible(
                         error,
                         def_id,
                         param,
@@ -1898,16 +1899,24 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                     for param in
                         [param_to_point_at, fallback_param_to_point_at, self_param_to_point_at]
                     {
-                        if let Some(param) = param
-                            && self.point_at_field_if_possible(
-                                error,
+                        if let Some(param) = param {
+                            let refined_expr = self.point_at_field_if_possible(
                                 def_id,
                                 param,
                                 variant_def_id,
                                 fields,
-                            )
-                        {
-                            return true;
+                            );
+
+                            match refined_expr {
+                                None => {}
+                                Some((refined_expr, _)) => {
+                                    error.obligation.cause.span = refined_expr
+                                        .span
+                                        .find_ancestor_in_same_ctxt(error.obligation.cause.span)
+                                        .unwrap_or(refined_expr.span);
+                                    return true;
+                                }
+                            }
                         }
                     }
                 }
@@ -1940,7 +1949,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         }
     }
 
-    fn point_at_arg_if_possible(
+    /// - `blame_specific_*` means that the function will recursively traverse the expression,
+    /// looking for the most-specific-possible span to blame.
+    ///
+    /// - `point_at_*` means that the function will only go "one level", pointing at the specific
+    /// expression mentioned.
+    ///
+    /// `blame_specific_arg_if_possible` will find the most-specific expression anywhere inside
+    /// the provided function call expression, and mark it as responsible for the fullfillment
+    /// error.
+    fn blame_specific_arg_if_possible(
         &self,
         error: &mut traits::FulfillmentError<'tcx>,
         def_id: DefId,
@@ -1959,13 +1977,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             .inputs()
             .iter()
             .enumerate()
-            .filter(|(_, ty)| find_param_in_ty(**ty, param_to_point_at))
+            .filter(|(_, ty)| Self::find_param_in_ty((**ty).into(), param_to_point_at))
             .collect();
         // If there's one field that references the given generic, great!
         if let [(idx, _)] = args_referencing_param.as_slice()
             && let Some(arg) = receiver
                 .map_or(args.get(*idx), |rcvr| if *idx == 0 { Some(rcvr) } else { args.get(*idx - 1) }) {
+
             error.obligation.cause.span = arg.span.find_ancestor_in_same_ctxt(error.obligation.cause.span).unwrap_or(arg.span);
+
+            if let hir::Node::Expr(arg_expr) = self.tcx.hir().get(arg.hir_id) {
+                // This is more specific than pointing at the entire argument.
+                self.blame_specific_expr_if_possible(error, arg_expr)
+            }
+
             error.obligation.cause.map_code(|parent_code| {
                 ObligationCauseCode::FunctionArgumentObligation {
                     arg_hir_id: arg.hir_id,
@@ -1983,14 +2008,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         false
     }
 
-    fn point_at_field_if_possible(
+    // FIXME: Make this private and move to mod adjust_fulfillment_errors
+    pub fn point_at_field_if_possible(
         &self,
-        error: &mut traits::FulfillmentError<'tcx>,
         def_id: DefId,
         param_to_point_at: ty::GenericArg<'tcx>,
         variant_def_id: DefId,
         expr_fields: &[hir::ExprField<'tcx>],
-    ) -> bool {
+    ) -> Option<(&'tcx hir::Expr<'tcx>, Ty<'tcx>)> {
         let def = self.tcx.adt_def(def_id);
 
         let identity_substs = ty::InternalSubsts::identity_for_item(self.tcx, def_id);
@@ -2000,7 +2025,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             .iter()
             .filter(|field| {
                 let field_ty = field.ty(self.tcx, identity_substs);
-                find_param_in_ty(field_ty, param_to_point_at)
+                Self::find_param_in_ty(field_ty.into(), param_to_point_at)
             })
             .collect();
 
@@ -2010,17 +2035,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 // same rules that check_expr_struct uses for macro hygiene.
                 if self.tcx.adjust_ident(expr_field.ident, variant_def_id) == field.ident(self.tcx)
                 {
-                    error.obligation.cause.span = expr_field
-                        .expr
-                        .span
-                        .find_ancestor_in_same_ctxt(error.obligation.cause.span)
-                        .unwrap_or(expr_field.span);
-                    return true;
+                    return Some((expr_field.expr, self.tcx.type_of(field.did)));
                 }
             }
         }
 
-        false
+        None
     }
 
     fn point_at_path_if_possible(
@@ -2240,23 +2260,3 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         }
     }
 }
-
-fn find_param_in_ty<'tcx>(ty: Ty<'tcx>, param_to_point_at: ty::GenericArg<'tcx>) -> bool {
-    let mut walk = ty.walk();
-    while let Some(arg) = walk.next() {
-        if arg == param_to_point_at {
-            return true;
-        } else if let ty::GenericArgKind::Type(ty) = arg.unpack()
-            && let ty::Alias(ty::Projection, ..) = ty.kind()
-        {
-            // This logic may seem a bit strange, but typically when
-            // we have a projection type in a function signature, the
-            // argument that's being passed into that signature is
-            // not actually constraining that projection's substs in
-            // a meaningful way. So we skip it, and see improvements
-            // in some UI tests.
-            walk.skip_current_subtree();
-        }
-    }
-    false
-}

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

@@ -1,4 +1,5 @@
 mod _impl;
+mod adjust_fulfillment_errors;
 mod arg_matrix;
 mod checks;
 mod suggestions;

compiler/rustc_hir_typeck/src/fn_ctxt/mod.rs

@@ -1563,6 +1563,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
                                     traits::ImplDerivedObligationCause {
                                         derived,
                                         impl_def_id,
+                                        impl_def_predicate_index: None,
                                         span,
                                     },
                                 ))

compiler/rustc_hir_typeck/src/method/probe.rs

@@ -145,30 +145,32 @@ impl<'tcx> Elaborator<'tcx> {
                 // Get predicates declared on the trait.
                 let predicates = tcx.super_predicates_of(data.def_id());
 
-                let obligations = predicates.predicates.iter().map(|&(mut pred, span)| {
-                    // when parent predicate is non-const, elaborate it to non-const predicates.
-                    if data.constness == ty::BoundConstness::NotConst {
-                        pred = pred.without_const(tcx);
-                    }
-
-                    let cause = obligation.cause.clone().derived_cause(
-                        bound_predicate.rebind(data),
-                        |derived| {
-                            traits::ImplDerivedObligation(Box::new(
-                                traits::ImplDerivedObligationCause {
-                                    derived,
-                                    impl_def_id: data.def_id(),
-                                    span,
-                                },
-                            ))
-                        },
-                    );
-                    predicate_obligation(
-                        pred.subst_supertrait(tcx, &bound_predicate.rebind(data.trait_ref)),
-                        obligation.param_env,
-                        cause,
-                    )
-                });
+                let obligations =
+                    predicates.predicates.iter().enumerate().map(|(index, &(mut pred, span))| {
+                        // when parent predicate is non-const, elaborate it to non-const predicates.
+                        if data.constness == ty::BoundConstness::NotConst {
+                            pred = pred.without_const(tcx);
+                        }
+
+                        let cause = obligation.cause.clone().derived_cause(
+                            bound_predicate.rebind(data),
+                            |derived| {
+                                traits::ImplDerivedObligation(Box::new(
+                                    traits::ImplDerivedObligationCause {
+                                        derived,
+                                        impl_def_id: data.def_id(),
+                                        impl_def_predicate_index: Some(index),
+                                        span,
+                                    },
+                                ))
+                            },
+                        );
+                        predicate_obligation(
+                            pred.subst_supertrait(tcx, &bound_predicate.rebind(data.trait_ref)),
+                            obligation.param_env,
+                            cause,
+                        )
+                    });
                 debug!(?data, ?obligations, "super_predicates");
 
                 // Only keep those bounds that we haven't already seen.

compiler/rustc_infer/src/traits/util.rs

@@ -475,6 +475,8 @@ pub enum WellFormedLoc {
 pub struct ImplDerivedObligationCause<'tcx> {
     pub derived: DerivedObligationCause<'tcx>,
     pub impl_def_id: DefId,
+    /// The index of the derived predicate in the parent impl's predicates.
+    pub impl_def_predicate_index: Option<usize>,
     pub span: Span,
 }
 

compiler/rustc_middle/src/traits/mod.rs

@@ -1190,6 +1190,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                 ImplDerivedObligation(Box::new(ImplDerivedObligationCause {
                     derived,
                     impl_def_id,
+                    impl_def_predicate_index: None,
                     span: obligation.cause.span,
                 }))
             });

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

@@ -2608,11 +2608,12 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         assert_eq!(predicates.parent, None);
         let predicates = predicates.instantiate_own(tcx, substs);
         let mut obligations = Vec::with_capacity(predicates.len());
-        for (predicate, span) in predicates {
+        for (index, (predicate, span)) in predicates.into_iter().enumerate() {
             let cause = cause.clone().derived_cause(parent_trait_pred, |derived| {
                 ImplDerivedObligation(Box::new(ImplDerivedObligationCause {
                     derived,
                     impl_def_id: def_id,
+                    impl_def_predicate_index: Some(index),
                     span,
                 }))
             });

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

@@ -1,8 +1,8 @@
 error[E0277]: the trait bound `B<C>: Copy` is not satisfied
-  --> $DIR/deriving-copyclone.rs:31:13
+  --> $DIR/deriving-copyclone.rs:31:26
    |
 LL |     is_copy(B { a: 1, b: C });
-   |     ------- ^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `B<C>`
+   |     -------              ^ the trait `Copy` is not implemented for `B<C>`
    |     |
    |     required by a bound introduced by this call
    |
@@ -19,14 +19,14 @@ LL | fn is_copy<T: Copy>(_: T) {}
    = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)
 help: consider borrowing here
    |
-LL |     is_copy(&B { a: 1, b: C });
-   |             +
+LL |     is_copy(B { a: 1, b: &C });
+   |                          +
 
 error[E0277]: the trait bound `B<C>: Clone` is not satisfied
-  --> $DIR/deriving-copyclone.rs:32:14
+  --> $DIR/deriving-copyclone.rs:32:27
    |
 LL |     is_clone(B { a: 1, b: C });
-   |     -------- ^^^^^^^^^^^^^^^^ the trait `Clone` is not implemented for `B<C>`
+   |     --------              ^ the trait `Clone` is not implemented for `B<C>`
    |     |
    |     required by a bound introduced by this call
    |
@@ -43,14 +43,14 @@ LL | fn is_clone<T: Clone>(_: T) {}
    = note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info)
 help: consider borrowing here
    |
-LL |     is_clone(&B { a: 1, b: C });
-   |              +
+LL |     is_clone(B { a: 1, b: &C });
+   |                           +
 
 error[E0277]: the trait bound `B<D>: Copy` is not satisfied
-  --> $DIR/deriving-copyclone.rs:35:13
+  --> $DIR/deriving-copyclone.rs:35:26
    |
 LL |     is_copy(B { a: 1, b: D });
-   |     ------- ^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `B<D>`
+   |     -------              ^ the trait `Copy` is not implemented for `B<D>`
    |     |
    |     required by a bound introduced by this call
    |
@@ -67,8 +67,8 @@ LL | fn is_copy<T: Copy>(_: T) {}
    = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)
 help: consider borrowing here
    |
-LL |     is_copy(&B { a: 1, b: D });
-   |             +
+LL |     is_copy(B { a: 1, b: &D });
+   |                          +
 
 error: aborting due to 3 previous errors
 

tests/ui/derives/deriving-copyclone.stderr

@@ -0,0 +1,28 @@
+trait T1 {}
+trait T2 {}
+trait T3 {}
+trait T4 {}
+
+impl<B: T2> T1 for Wrapper<B> {}
+
+impl T2 for i32 {}
+impl T3 for i32 {}
+
+impl<A: T3> T2 for Burrito<A> {}
+
+struct Wrapper<W> {
+    value: W,
+}
+
+struct Burrito<F> {
+    filling: F,
+}
+
+fn want<V: T1>(_x: V) {}
+
+fn example<Q>(q: Q) {
+    want(Wrapper { value: Burrito { filling: q } });
+    //~^ ERROR the trait bound `Q: T3` is not satisfied [E0277]
+}
+
+fn main() {}