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.
Remove confusing 'while checking' note from opaque future type mismatches
Maybe I'm just misinterpreting the wording of the note. The only value I can see in this note is that it points out where the async's opaque future is coming from, but the way it's doing it is misleading IMO.
For example:
```rust
note: while checking the return type of the `async fn`
--> $DIR/dont-suggest-missing-await.rs:7:24
|
LL | async fn make_u32() -> u32 {
| ^^^ checked the `Output` of this `async fn`, found opaque type
```
We point at the type `u32` in the HIR, but then say "found opaque type". We also say "while checking"... but we're typechecking a totally different function when we get this type mismatch!
r? ``@estebank`` but feel free to reassign and/or take your time reviewing this. I'd be inclined to also discuss reworking the presentation of this type mismatch to restore some of these labels in a way that makes it more clear what it's trying to point out.
Track bound types like bound regions
When we instantiate bound types into placeholder types, we throw away the names for some reason. These names are particularly useful for error reporting once we have `for<T>` binders.
r? types
Use `FallibleTypeFolder` for `ConstInferUnifier` not `TypeRelation`
I am not sure why this was using a `TypeRelation`, maybe it predates the ability to have fallible type folders
internally change regions to be covariant
Surprisingly, we consider the reference type `&'a T` to be contravaraint in its lifetime parameter. This is confusing and conflicts with the documentation we have in the reference, rustnomicon, and rustc-dev-guide. This also arguably not the correct use of terminology since we can use `&'static u8` in a place where `&' a u8` is expected, this implies that `&'static u8 <: &' a u8` and consequently `'static <: ' a`, hence covariance.
Because of this, when relating two types, we used to switch the argument positions in a confusing way:
`Subtype(&'a u8 <: &'b u8) => Subtype('b <: 'a) => Outlives('a: 'b) => RegionSubRegion('b <= 'a)`
The reason for the current behavior is probably that we wanted `Subtype('b <: 'a)` and `RegionSubRegion('b <= 'a)` to be equivalent, but I don' t think this is a good reason since these relations are sufficiently different in that the first is a relation in the subtyping lattice and is intrinsic to the type-systems, while the the second relation is an implementation detail of regionck.
This PR changes this behavior to use covariance, so..
`Subtype(&'a u8 <: &'b u8) => Subtype('a <: 'b) => Outlives('a: 'b) => RegionSubRegion('b <= 'a) `
Resolves#103676
r? `@lcnr`
Use `can_eq` to compare types for default assoc type error
This correctly handles inference variables like `{integer}`. I had to move all of this `note_and_explain` code to `rustc_infer`, it made no sense for it to be in `rustc_middle` anyways.
The commits are reviewed separately.
Fixes#106968
use `LocalDefId` instead of `HirId` in trait resolution to simplify the obligation clause resolution
This commit introduces a refactoring suggested by `@lcnr` to simplify the obligation clause resolution.
This is just the first PR that introduces a type of refactoring, but others PRs will follow this to introduce name changing to change from the variable name from `body_id` to something else.
Fixes https://github.com/rust-lang/rust/issues/104827
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
`@rustbot` r? `@lcnr`
- On compiler-error's suggestion of moving this lower down the stack,
along the path of `report_mismatched_types()`, which is used
by `rustc_hir_analysis` and `rustc_hir_typeck`.
- update ui tests, add test
- add suggestions for references to fn pointers
- modify `TypeErrCtxt::same_type_modulo_infer` to take `T: relate::Relate` instead of `Ty`
use LocalDefId instead of HirId in trait resolution to simplify
the obligation clause resolution
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Switching them to `Break(())` and `Continue(())` instead.
libs-api would like to remove these constants, so stop using them in compiler to make the removal PR later smaller.
Switch to `EarlyBinder` for `item_bounds` query
Part of the work to finish #105779 (also see https://github.com/rust-lang/types-team/issues/78).
Several queries `X` have a `bound_X` variant that wraps the output in `EarlyBinder`. This adds `EarlyBinder` to the return type of the `item_bounds` query and removes `bound_item_bounds`.
r? `@lcnr`
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
Suggestion for type mismatch when we need a u8 but the programmer wrote a char literal
Today Rust just points out that we have a char and we need a u8, but if I wrote 'A' then I could fix this by just writing b'A' instead. This code should detect the case where we're about to report a type mismatch of this kind, and the programmer wrote a char literal, and the char they wrote is ASCII, so therefore just prefixing b to make a byte literal will do what they meant.
I have definitely written this mistake more than once, it's not difficult to figure out what to do, but the compiler might as well tell us anyway.
I provided a test with two simple examples where the suggestion is appropriate, and one where it is not because the char literal is not ASCII, showing that the suggestion is only triggered in the former cases.
I have contributed only a small typo doc fix before, so this is my first substantive rustc change.
Switch to `EarlyBinder` for `const_param_default` and `impl_trait_ref` queries
Part of the work to close#105779 and implement https://github.com/rust-lang/types-team/issues/78.
Several queries `X` have a `bound_X` variant that wraps the output in `EarlyBinder`. This PR adds `EarlyBinder` to the return type of `const_param_default` and `impl_trait_ref`, and removes their `bound_X` variants.
r? `@lcnr`
remove unreachable error code `E0490`
AFAIK, the untested and undocumented error code `E0490` is now unreachable, it was from the days of the original borrow checker.
cc ``@GuillaumeGomez`` #61137
Simplify some canonical type alias names
* delete the `Canonicalized<'tcx>` type alias in favor for `Canonical<'tcx>`
* `CanonicalizedQueryResponse` -> `CanonicalQueryResponse`
I don't particularly care about the latter, but it should be consistent. We could alternatively delete the first alias and rename the struct to `Canonicalized`, and then keep the name of `CanonicalizedQueryResponse` untouched.
Move `check_region_obligations_and_report_errors` to `TypeErrCtxt`
Makes sense for this function to live with its sibling `resolve_regions_and_report_errors`, around which it's basically just a wrapper.
Migrating rustc_infer to session diagnostics (part 3)
``@rustbot`` label +A-translation
r? rust-lang/diagnostics
cc https://github.com/rust-lang/rust/issues/100717
Seems like a part of static_impl_trait.rs emits suggestions in a loop, and note.rs needs to have two instances of the same subdiagnostic, so these will need to wait until we have eager translation/list support.
Other than that, there is only error_reporting/mod.rs left to migrate.
Suggest adding named lifetime when the return contains value borrowed from more than one lifetimes of function inputs
fix for #105227.
The problem: The suggestion of adding an explicit `'_` lifetime bound is **incorrect** when the function's return type contains a value which could be borrowed from more than one lifetimes of the function's inputs. Instead, a named lifetime parameter can be introduced in such a case.
The solution: Checking the number of elided lifetimes in the function signature. If more than one lifetimes found in the function inputs when the suggestion of adding explicit `'_` lifetime, change it to using named lifetime parameter `'a` instead.
Rename `hir::Map::{get_,find_}parent_node` to `hir::Map::{,opt_}parent_id`, and add `hir::Map::{get,find}_parent`
The `hir::Map::get_parent_node` function doesn't return a `Node`, and I think that's quite confusing. Let's rename it to something that sounds more like something that gets the parent hir id => `hir::Map::parent_id`. Same with `find_parent_node` => `opt_parent_id`.
Also, combine `hir.get(hir.parent_id(hir_id))` and similar `hir.find(hir.parent_id(hir_id))` function into new functions that actually retrieve the parent node in one call. This last commit is the only one that might need to be looked at closely.
Some `compare_method` tweaks
1. Make some of the comparison functions' names more regular
2. Reduce pub scope of some of the things in `compare_method`
~3. Remove some unnecessary opaque type handling code -- `InferCtxt` already is in a mode that doesn't define opaque types~
* moved to a different PR
4. Bubble up `ErrorGuaranteed` for region constraint errors in `compare_method` - Improves a redundant error message in one unit test.
5. Move the `compare_method` module to have a more general name, since it's more like `compare_impl_item` :)
6. Rename `collect_trait_impl_trait_tys`
