Greatly improve error reporting for futures and generators in `note_obligation_cause_code` #98259

jyn514 · 2022-06-19T19:37:00Z

Most futures don't go through this code path, because they're caught by
maybe_note_obligation_cause_for_async_await. But all generators do,
and maybe_note is imperfect and doesn't catch all futures. Improve the error message for those it misses.

At some point, we may want to consider unifying this with the code for maybe_note_async_await,
so that async_await notes all parent constraints, and note_obligation can point to yield points.
But both functions are quite complicated, and it's not clear to me how to combine them;
this seems like a good incremental improvement.

Helps with #97332.

r? @estebank cc @eholk @compiler-errors

jyn514 · 2022-06-19T19:46:56Z

Some more discussion about why I chose to improve this function rather than make maybe_note_async_await smarter: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/josh.20rambles.20about.20drop.20tracking.20.2397332/near/286660618

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

…bligation_cause_code` Most futures don't go through this code path, because they're caught by `maybe_note_obligation_cause_for_async_await`. But all generators do, and `maybe_note` is imperfect and doesn't catch all futures. Improve the error message for those it misses. At some point, we may want to consider unifying this with the code for `maybe_note_async_await`, so that `async_await` notes all parent constraints, and `note_obligation` can point to yield points. But both functions are quite complicated, and it's not clear to me how to combine them; this seems like a good incremental improvement.

eholk

This seems like a significant improvement! It seems like quite a few error messages are clearer now. I left a couple comments, but they are just potential cleanups and clarification questions.

The output for the complex spans with drop tracking option looks okay to me. It's not exactly the same as without, but I think it's still clear enough what's wrong.

Anyway, I think the change looks good.

eholk · 2022-06-20T22:59:21Z

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

                    } else {
                        false
                    }
                };

+                let future_trait = self.tcx.lang_items().future_trait().unwrap();
+                let opaque_ty_is_future = |def_id| {


I wonder if it's worth lifting this out into a method on tcx? It seems like code that might be useful elsewhere too.

sure, done :)

eholk · 2022-06-20T23:13:23Z

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

+                            ty::Generator(def_id, _, movability) => {
+                                let sp = self.tcx.def_span(def_id);
+
+                                // Special-case this to say "async block" instead of `[static generator]`.


Is this always true that a static generator is an async block? I guess it probably is for current stable Rust.

oh good point - I'll change this to use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.generator_kind instead :)

estebank · 2022-06-22T00:23:05Z

@bors r+

bors · 2022-06-22T00:23:07Z

📌 Commit 1deca04 has been approved by estebank

jyn514

I was going to address the comments and then I saw @estebank already approved the PR 😁 I'll make a new PR

jyn514 · 2022-06-22T00:27:13Z

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

+                            ty::Generator(def_id, _, movability) => {
+                                let sp = self.tcx.def_span(def_id);
+
+                                // Special-case this to say "async block" instead of `[static generator]`.


oh good point - I'll change this to use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.generator_kind instead :)

jyn514 · 2022-06-22T00:28:36Z

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

                    } else {
                        false
                    }
                };

+                let future_trait = self.tcx.lang_items().future_trait().unwrap();
+                let opaque_ty_is_future = |def_id| {


sure, done :)

It got merged so fast I didn't have time to make changes xD

…=estebank Greatly improve error reporting for futures and generators in `note_obligation_cause_code` Most futures don't go through this code path, because they're caught by `maybe_note_obligation_cause_for_async_await`. But all generators do, and `maybe_note` is imperfect and doesn't catch all futures. Improve the error message for those it misses. At some point, we may want to consider unifying this with the code for `maybe_note_async_await`, so that `async_await` notes all parent constraints, and `note_obligation` can point to yield points. But both functions are quite complicated, and it's not clear to me how to combine them; this seems like a good incremental improvement. Helps with rust-lang#97332. r? `@estebank` cc `@eholk` `@compiler-errors`

…view-comments, r=eholk Address review comments from rust-lang#98259 It got approved so fast I didn't have time to make changes xD r? `@eholk`

…view-comments, r=eholk Address review comments from rust-lang#98259 It got approved so fast I didn't have time to make changes xD r? ``@eholk``

…piler-errors Rollup of 16 pull requests Successful merges: - rust-lang#96173 (Fix documentation for `with_capacity` and `reserve` families of methods) - rust-lang#98184 (Give name if anonymous region appears in impl signature) - rust-lang#98259 (Greatly improve error reporting for futures and generators in `note_obligation_cause_code`) - rust-lang#98269 (Provide a `PathSegment.res` in more cases) - rust-lang#98283 (Point at private fields in struct literal) - rust-lang#98305 (prohibit_generics: don't alloc error string if no error emitted) - rust-lang#98310 (rustdoc: optimize loading of source sidebar) - rust-lang#98353 (Migrate two diagnostics from the `rustc_builtin_macros` crate) - rust-lang#98355 (Update no_default_libraries handling for emscripten target) - rust-lang#98364 (clarify Arc::clone overflow check comment) - rust-lang#98365 (Address review comments from rust-lang#98259) - rust-lang#98388 (implement `iter_projections` function on `PlaceRef`) - rust-lang#98390 (Fixes handling of keywords in rustdoc json output) - rust-lang#98409 (triagebot.toml: Allow applying nominated labels) - rust-lang#98410 (Update books) - rust-lang#98422 (Update browser-ui-test version to 0.9.6) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints A-traits Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-coroutines Area: Coroutines A-async-await Area: Async & Await labels Jun 19, 2022

rust-highfive assigned estebank Jun 19, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2022

jyn514 force-pushed the improve-obligation-errors branch from c35a756 to fdd9252 Compare June 19, 2022 19:39

compiler-errors reviewed Jun 19, 2022

View reviewed changes

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved

jyn514 force-pushed the improve-obligation-errors branch from fdd9252 to 3057dc6 Compare June 19, 2022 19:55

jyn514 mentioned this pull request Jun 19, 2022

Drop tracking error message regression in issue-70935-complex-spans.rs #97332

Closed

This comment has been minimized.

Sign in to view

jyn514 force-pushed the improve-obligation-errors branch from 3057dc6 to 1deca04 Compare June 19, 2022 20:31

eholk approved these changes Jun 20, 2022

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 Jun 22, 2022

jyn514 commented Jun 22, 2022

View reviewed changes

jyn514 added a commit to jyn514/rust that referenced this pull request Jun 22, 2022

Address review comments from rust-lang#98259

b052d76

It got merged so fast I didn't have time to make changes xD

jyn514 mentioned this pull request Jun 22, 2022

Address review comments from #98259 #98365

Merged

compiler-errors mentioned this pull request Jun 23, 2022

Rollup of 19 pull requests #98433

Closed

compiler-errors mentioned this pull request Jun 23, 2022

Rollup of 16 pull requests #98438

Merged

bors merged commit 413e350 into rust-lang:master Jun 24, 2022

rustbot added this to the 1.63.0 milestone Jun 24, 2022

jyn514 deleted the improve-obligation-errors branch February 25, 2023 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Greatly improve error reporting for futures and generators in `note_obligation_cause_code` #98259

Greatly improve error reporting for futures and generators in `note_obligation_cause_code` #98259

jyn514 commented Jun 19, 2022

jyn514 commented Jun 19, 2022

This comment has been minimized.

eholk left a comment

eholk Jun 20, 2022

jyn514 Jun 22, 2022

eholk Jun 20, 2022

jyn514 Jun 22, 2022

estebank commented Jun 22, 2022

bors commented Jun 22, 2022

jyn514 left a comment

jyn514 Jun 22, 2022

jyn514 Jun 22, 2022

Greatly improve error reporting for futures and generators in note_obligation_cause_code #98259

Greatly improve error reporting for futures and generators in note_obligation_cause_code #98259

Conversation

jyn514 commented Jun 19, 2022

jyn514 commented Jun 19, 2022

This comment has been minimized.

eholk left a comment

Choose a reason for hiding this comment

eholk Jun 20, 2022

Choose a reason for hiding this comment

jyn514 Jun 22, 2022

Choose a reason for hiding this comment

eholk Jun 20, 2022

Choose a reason for hiding this comment

jyn514 Jun 22, 2022

Choose a reason for hiding this comment

estebank commented Jun 22, 2022

bors commented Jun 22, 2022

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Jun 22, 2022

Choose a reason for hiding this comment

jyn514 Jun 22, 2022

Choose a reason for hiding this comment

Greatly improve error reporting for futures and generators in `note_obligation_cause_code` #98259

Greatly improve error reporting for futures and generators in `note_obligation_cause_code` #98259