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

Nathan-Fenner · 2023-01-05T02:12:51Z

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:

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.

rustbot · 2023-01-05T02:12:59Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) soon.

Please see the contribution instructions for more information.

compiler-errors

Left mostly a bunch of style nits. I've gotta take a few more passes over this code to reason about its correctness :)

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

compiler-errors · 2023-01-05T02:28:53Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

+        };
+
+        // update the error span.
+        error.obligation.cause.span = expr


Hm, so the span diverging from the actual hir id of the new obligation cause is very suspicious. It'll probably cause incorrect suggestions, since those can look at the type of the argument in the obligation cause via TypeckResults.

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

compiler-errors · 2023-01-05T02:31:12Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

+            hir::ExprKind::Struct(
+                hir::QPath::Resolved(
+                    _,
+                    hir::Path { res: hir::def::Res::Def(struct_def_kind, struct_def_id), .. },


There should be a way of extracting the def here rather than destructuring the path's res -- try type_dependent_def

This will help us in cases where the path is resolved during type-check rather than during the earlier name resolve stage.

I used .qpath_res here instead; type_dependent_def doesn't seem to work, I think because this isn't a call?

There's a second use of pretty much the same logic below, where type_dependent_def might work. I haven't tried that yet.

My bad -- qpath_res is the one I was thinking of, yeah. That'll store the properly resolved res for a qpath, regardless of it's TypeRelative or Resolved

compiler-errors · 2023-01-05T02:39:17Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

+                let struct_def_generics: Vec<&ty::GenericParamDef> = relevant_ty_args_indices
+                    .into_iter()
+                    .filter_map(|index| struct_def_generics.params.get(index))
+                    .collect();


nit: this, and similar below, could be a retain :laugh:

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

compiler-errors · 2023-01-05T02:47:59Z

Some considerations: right now, this check occurs in parallel to the independent adjust_fulfillment_error_for_expr_obligation function.

I'm not sure if I understand this comment, point_at_specific_expr_if_possible is being called in point_at_arg_if_possible, which happens within adjust_fulfillment_error_for_expr_obligation. Is that comment out of date, or am I misunderstanding what it's saying?

compiler-errors

Some more mostly nit-picks.

Needless to say, I'm quite happy with the state of this PR as-is, so please don't take my comments in a negative light 😃

compiler/rustc_middle/src/traits/mod.rs

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

src/test/ui/traits/negative-impls/negated-auto-traits-error.stderr

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

Nathan-Fenner · 2023-01-05T04:17:45Z

I've fixed up most of the small nits, and marked those comments with (👍) - I'm not sure for rustc if the policy is to prefer the reviewer resolve their own comments, or the PR author?

I'll try to get to the rest of the nits tomorrow. They all seem reasonable, it will just take me a bit to fix some of them.

compiler-errors · 2023-01-05T04:19:37Z

Feel free to click resolve as you fix things. I'll resolve the ones I see are outdated when I do another review of this tomorrow. And sure, take your time addressing feedback, and as always, speak up if you disagree with my suggestions.

Nathan-Fenner · 2023-01-05T04:20:12Z

I'm not sure if I understand this comment, point_at_specific_expr_if_possible is being called in point_at_arg_if_possible, which happens within adjust_fulfillment_error_for_expr_obligation. Is that comment out of date, or am I misunderstanding what it's saying?

I meant that adjust_fulfillment_error_for_expr_obligation is doing a bunch of similar span-reduction logic, of which point_at_arg_if_possible is just one small part. It might make more sense to move this logic into adjust_fulfillment_error_for_expr_obligation directly, or as a helper that it directly calls, so that it can "work more closely" with its existing span reductions. For example, it's not clear to me if it would ever be helpful to do adjust_fulfillment_error_for_expr_obligation, go through the new point_at_specific_expr_if_possible, and then adjust again using the existing heuristics in adjust_fulfillment_error_for_expr_obligation- or if it's always best to do them "outside-in" like I picked here.

src/test/ui/traits/bad-trait-error-span-in-call.stderr

Nathan-Fenner · 2023-01-06T08:26:02Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

                            return true;
+                             */


The old way of doing things and the new way of doing things introduced in this PR are in conflict. The old way mutate the span in place, and returns true if it does this.

The new way uses Result<Expr, Expr> to indicate whether it successfully refined or unsuccessfully refined. But it can still make a partial refinement in the unsuccessful case. The downside is that it needs to know the "root" expr to return it in failure.

I think a hybrid approach is actually going to be nicer. I will introduce a new ErrorSpanRefiner which can be called with:

struct FullyRefined; struct NotFullyRefined; fn refine_span_to_expr(&mut self, expr: &hir::Expr<'tcx>) -> Result<(), NotFullyRefined>

and the new/existing point_to_*_ functions will be updated to return Result<FullyRefined, NotFullyRefined> instead of bool or Result<&Expr, &Expr>.

The ErrorSpanRefiner can also deal with updating the cause, as needed, so that the cause and the span stay in sync.

src/test/ui/traits/bad-trait-error-span-in-call.stderr

compiler-errors · 2023-01-06T22:27:04Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

@@ -1863,23 +1863,26 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                    return true;
                }
            }
-            hir::ExprKind::Struct(qpath, fields, ..) => {
-                if let Res::Def(DefKind::Struct | DefKind::Variant, variant_def_id) =
+            hir::ExprKind::Struct(qpath, _fields, ..) => {


Another thing you can do after the rest of the review is done -- all of these methods under adjust_fulfillment_errors_for_expr_obligation can be moved into a distinct module. Maybe adjust_fulfillment_errors.rs or something.

I ran into some merge conflicts trying to move the existing functions - the new ones are currently placed into a separate module. I'd like to move the remaining ones in a separate PR, just to avoid rebase-churn on this one, since it adds a lot of net-new code.

bors · 2023-01-09T22:11:16Z

☔ The latest upstream changes (presumably #106637) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors · 2023-01-17T02:12:23Z

Howdy -- sorry for not reviewing this in the mean time. This PR needs to be rebased and tests need to be fixed/blessed. Can you do that @Nathan-Fenner? (no rush obviously)

compiler-errors · 2023-01-18T01:03:15Z

(use @rustbot ready to mark this ready for review once it's rebased)

@rustbot author

rustbot · 2023-01-23T19:33:18Z

Some changes occurred in src/tools/cargo

cc @ehuss

compiler-errors · 2023-02-03T04:10:27Z

Subtle reminder for the future to use

@rustbot ready

to mark this as ready for review. Sorry, hadn't seen that this was updated because it was marked as S-waiting-on-author. I'll take a look soon.

compiler-errors

Thanks, I'm pretty happy with the changes to the logic.

compiler-errors · 2023-02-06T06:11:04Z

compiler/rustc_hir_typeck/src/fn_ctxt/adjust_fulfillment_errors.rs

+        Err(expr)
+    }
+
+    // FIXME: This can be made into a private, non-impl function later.


We should follow-up with these FIXMEs. If you can't in the next few weeks, then I will.

compiler-errors · 2023-02-06T06:11:25Z

@bors r+

bors · 2023-02-06T06:11:27Z

📌 Commit 99638a6 has been approved by compiler-errors

It is now in the queue for this repository.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#106477 (Refine error spans for "The trait bound `T: Trait` is not satisfied" when passing literal structs/tuples) - rust-lang#107596 (Add nicer output to PGO build timer) - rust-lang#107692 (Sort Generator `print-type-sizes` according to their yield points) - rust-lang#107714 (Clarify wording on f64::round() and f32::round()) - rust-lang#107720 (end entry paragraph with a period (.)) - rust-lang#107724 (remove unused rustc_* imports) - rust-lang#107725 (Turn MarkdownWithToc into a struct with named fields) - rust-lang#107731 (interpret: move discriminant reading and writing to separate file) - rust-lang#107735 (Add mailmap for commits made by xes@meta.com) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…ust_fulfillment_errors, r=compiler-errors Split fn_ctxt/adjust_fulfillment_errors from fn_ctxt/checks This is a follow-up from rust-lang#106477, addressing a small number of the `FIXME`s that were added, by moving some functions into the new(er) `adjust_fulfillment_errors` module. More cleanup is possible for this file (and I'll hopefully get around to doing some of that soon) but the very first thing is to just move these functions out. There should be no "real" changes in this PR, besides minor adjustments to imports and the functions being transferred.

…ef-trait-refine, r=WaffleLapkin Refine error span for trait error into borrowed expression Extends the error span refinement in rust-lang#106477 to drill into borrowed expressions just like tuples/struct/enum literals. For example, ```rs trait Fancy {} trait Good {} impl <'a, T> Fancy for &'a T where T: Good {} impl <S> Good for Option<S> where S: Iterator {} fn want_fancy<F>(f: F) where F: Fancy {} fn example() { want_fancy(&Some(5)); // (BEFORE) ^^^^^^^^ `{integer}` is not an iterator // (AFTER) ^ `{integer}` is not an iterator } ``` Existing heuristics try to find the right part of the expression to "point at"; current heuristics look at e.g. struct constructors and tuples. This PR adds a new check for borrowed expressions when looking into a borrowed type.

…-span-fix-Some, r=WaffleLapkin Point error span at Some constructor argument when trait resolution fails This is a follow up to rust-lang#108254 and rust-lang#106477 which extends error span refinement to handle a case which I mistakenly believed was handled in rust-lang#106477. The goal is to refine the error span depicted below: ```rs trait Fancy {} impl <T> Fancy for Option<T> where T: Iterator {} fn want_fancy<F>(f: F) where F: Fancy {} fn example() { want_fancy(Some(5)); // (BEFORE) ^^^^^^^ `{integer}` is not an iterator // (AFTER) ^ `{integer}` is not an iterator } ``` I had used a (slightly more complex) example as an illustrative example in rust-lang#108254 , but hadn't actually turned it into a test, because I had (incorrectly) believed at the time it was covered by existing behavior. It turns out that `Some` is slightly "special" in that it resolves differently from the other `enum` constructors I had tried, and therefore this test was actually broken. I've now updated the tests to include this example, and fixed the code to correctly resolve the `Some` constructor so that the span of the error is reduced.

rustbot assigned compiler-errors Jan 5, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 5, 2023

compiler-errors reviewed Jan 5, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

compiler-errors reviewed Jan 5, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Nathan-Fenner commented Jan 6, 2023

View reviewed changes

src/test/ui/traits/bad-trait-error-span-in-call.stderr Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Nathan-Fenner commented Jan 6, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

Nathan-Fenner commented Jan 6, 2023

View reviewed changes

src/test/ui/traits/bad-trait-error-span-in-call.stderr Outdated Show resolved Hide resolved

compiler-errors reviewed Jan 6, 2023

View reviewed changes

Nathan-Fenner force-pushed the nathanf/refined-error-span-trait-impl branch from a2b69cf to 56e2593 Compare January 9, 2023 06:45

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2023

Nathan-Fenner force-pushed the nathanf/refined-error-span-trait-impl branch from 56e2593 to aa30dd7 Compare January 23, 2023 19:33

Nathan-Fenner force-pushed the nathanf/refined-error-span-trait-impl branch from aa30dd7 to bd8812b Compare January 23, 2023 19:55

This comment has been minimized.

Sign in to view

Point at specific field in struct literal when trait fulfillment fails

2a67e99

Nathan-Fenner force-pushed the nathanf/refined-error-span-trait-impl branch from bd8812b to 2a67e99 Compare January 23, 2023 21:38

This comment has been minimized.

Sign in to view

fix up doc submodules

3b562db

This comment has been minimized.

Sign in to view

Nathan-Fenner added 2 commits January 23, 2023 21:43

fix cargo submodule

eb13b7b

revert change to test source

99638a6

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 3, 2023

compiler-errors approved these changes Feb 6, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2023

matthiaskrgr mentioned this pull request Feb 6, 2023

Rollup of 9 pull requests #107738

Merged

bors merged commit 800221b into rust-lang:master Feb 6, 2023

rustbot added this to the 1.69.0 milestone Feb 6, 2023

Nathan-Fenner mentioned this pull request Feb 7, 2023

Split fn_ctxt/adjust_fulfillment_errors from fn_ctxt/checks #107746

Merged

eggyal mentioned this pull request Feb 15, 2023

ICE when generic constraint involving trait alias is not met #108072

Closed

Nathan-Fenner mentioned this pull request Feb 20, 2023

Refine error span for trait error into borrowed expression #108254

Merged

Nathan-Fenner mentioned this pull request Feb 28, 2023

Point error span at Some constructor argument when trait resolution fails #108557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Nathan-Fenner commented Jan 5, 2023 •

edited

Loading

rustbot commented Jan 5, 2023

compiler-errors left a comment

compiler-errors Jan 5, 2023 •

edited

Loading

compiler-errors Jan 5, 2023 •

edited

Loading

Nathan-Fenner Jan 6, 2023

compiler-errors Jan 6, 2023 •

edited

Loading

compiler-errors Jan 5, 2023

This comment has been minimized.

compiler-errors commented Jan 5, 2023

compiler-errors left a comment

Nathan-Fenner commented Jan 5, 2023

compiler-errors commented Jan 5, 2023

Nathan-Fenner commented Jan 5, 2023

This comment has been minimized.

This comment has been minimized.

Nathan-Fenner Jan 6, 2023

This comment has been minimized.

compiler-errors Jan 6, 2023

Nathan-Fenner Jan 9, 2023

This comment has been minimized.

bors commented Jan 9, 2023

compiler-errors commented Jan 17, 2023 •

edited

Loading

compiler-errors commented Jan 18, 2023

rustbot commented Jan 23, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

compiler-errors commented Feb 3, 2023

compiler-errors left a comment

compiler-errors Feb 6, 2023

compiler-errors commented Feb 6, 2023

bors commented Feb 6, 2023

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

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

Conversation

Nathan-Fenner commented Jan 5, 2023 • edited Loading

rustbot commented Jan 5, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

compiler-errors Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

compiler-errors Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Nathan-Fenner Jan 6, 2023

Choose a reason for hiding this comment

compiler-errors Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

compiler-errors Jan 5, 2023

Choose a reason for hiding this comment

This comment has been minimized.

compiler-errors commented Jan 5, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

Nathan-Fenner commented Jan 5, 2023

compiler-errors commented Jan 5, 2023

Nathan-Fenner commented Jan 5, 2023

This comment has been minimized.

This comment has been minimized.

Nathan-Fenner Jan 6, 2023

Choose a reason for hiding this comment

This comment has been minimized.

compiler-errors Jan 6, 2023

Choose a reason for hiding this comment

Nathan-Fenner Jan 9, 2023

Choose a reason for hiding this comment

This comment has been minimized.

bors commented Jan 9, 2023

compiler-errors commented Jan 17, 2023 • edited Loading

compiler-errors commented Jan 18, 2023

rustbot commented Jan 23, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

compiler-errors commented Feb 3, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

compiler-errors Feb 6, 2023

Choose a reason for hiding this comment

compiler-errors commented Feb 6, 2023

bors commented Feb 6, 2023

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

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

Nathan-Fenner commented Jan 5, 2023 •

edited

Loading

compiler-errors Jan 5, 2023 •

edited

Loading

compiler-errors Jan 5, 2023 •

edited

Loading

compiler-errors Jan 6, 2023 •

edited

Loading

compiler-errors commented Jan 17, 2023 •

edited

Loading