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.
use LocalDefId instead of HirId in trait resolution to simplify
the obligation clause resolution
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Unify `Opaque`/`Projection` handling in region outlives code
They share basically identical paths in most places which are even easier to unify now that they're both `ty::Alias`
r? types
Change reported_violations to use IndexSet
It is being used to iterate and to insert, without a lot of lookups
so hopefully it won't be a perf hit
Change MiniGraph.nodes to use IndexSet
It is being used to iterate and to insert, without performing lookups
so hopefully it won't be a perf hit
Change RegionConstraintData.givens to a FxIndexSet
This might result in a perf hit. Remove was being used in `givens`,
and `FxIndexSet` doesn't allow calling remove without losing the
fixed iteration order. So it was necessary to change remove to
`shift_remove`, but this method is slower.
Change OpaqueTypesVisitor to use stable sets and maps
This could also be a perf hit.
Make TraitObject visitor use a stable set
Neither require nor imply lifetime bounds on opaque type for well formedness
The actual hidden type can live arbitrarily longer than any individual lifetime and arbitrarily shorter than all but one of the lifetimes.
fixes#86218fixes#84305
This is a **breaking change** but it is a necessary soundness fix
Make cycle errors recoverable
In particular, this allows rustdoc to recover from cycle errors when normalizing associated types for documentation.
In the past, ```@jackh726``` has said we need to be careful about overflow errors: https://github.com/rust-lang/rust/pull/91430#issuecomment-983997013
> Off the top of my head, we definitely should be careful about treating overflow errors the same as
"not implemented for some reason" errors. Otherwise, you could end up with behavior that is
different depending on recursion depth. But, that might be context-dependent.
But cycle errors should be safe to unconditionally report; they don't depend on the recursion depth, they will always be an error whenever they're encountered.
Helps with https://github.com/rust-lang/rust/issues/81091.
r? ```@lcnr``` cc ```@matthewjasper```
In particular, this allows rustdoc to recover from cycle errors when normalizing associated types for documentation.
In the past, `@jackh726` has said we need to be careful about overflow errors:
> Off the top of my head, we definitely should be careful about treating overflow errors the same as
"not implemented for some reason" errors. Otherwise, you could end up with behavior that is
different depending on recursion depth. But, that might be context-dependent.
But cycle errors should be safe to unconditionally report; they don't depend on the recursion depth, they will always be an error whenever they're encountered.
don't succeed `evaluate_obligation` query if new opaque types were registered
fixes#98608fixes#98604
The root cause of all this is that in type flag computation we entirely ignore nongeneric things like struct fields and the signature of function items. So if a flag had to be set for a struct if it is set for a field, that will only happen if the field is generic, as only the generic parameters are checked.
I now believe we cannot use type flags to handle opaque types. They seem like the wrong tool for this.
Instead, this PR replaces the previous logic by adding a new variant of `EvaluatedToOk`: `EvaluatedToOkModuloOpaqueTypes`, which says that there were some opaque types that got hidden types bound, but that binding may not have been legal (because we don't know if the opaque type was in its defining scope or not).
