Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print nicer async/await trait errors for generators in any place in the error 'stack' #67116

Open
wants to merge 9 commits into
base: master
from

Conversation

@Aaron1011
Copy link
Contributor

Aaron1011 commented Dec 7, 2019

Fixes #67025

Currently, we only print 'nice' async/await trait errors when the 'bad' type inside the generator occurs at the top of the 'stack' of errors (the nested errors in the ObligationCauseCode.

This PR allows these 'nice' error messages to be printed each time we encounter a generator in the error stack. This has the result of showing the user how each async fn contributed to the overall type not implementing some auto trait.

This PR still has a few issues:

  • The errors have some unnecessary 'occurs within GenFuture/impl Future/[static generator]. It would be nice to try to completely eliminate these when it doesn't reduce clarity, as they often add no actual information.
  • We should be printing an additional 'nice' error in the test issue-64130-non-send-future-diags.rs, for async fn foo. I suspect that some of the involved lifetimes are causing a false negative when we check if a type occurs within the generator.

However, I'd be interested in feedback on the overall idea of displaying multiple spans in the error message.

cc @estebank

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 7, 2019

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/better-async-error branch from 5d60d27 to 54c6927 Dec 9, 2019
@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 9, 2019

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-12-09T01:52:18.7214855Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-09T01:52:18.7231965Z ##[command]git config gc.auto 0
2019-12-09T01:52:18.7242752Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-09T01:52:18.7246945Z ##[command]git config --get-all http.proxy
2019-12-09T01:52:18.7255893Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67116/merge:refs/remotes/pull/67116/merge
---
2019-12-09T01:59:29.6239346Z 
2019-12-09T01:59:29.6239590Z error: variant is never constructed: `Other`
2019-12-09T01:59:29.6239867Z   --> src/librustc/traits/error_reporting.rs:50:5
2019-12-09T01:59:29.6240188Z    |
2019-12-09T01:59:29.6240563Z 50 |     Other(&'a dyn fmt::Display)
2019-12-09T01:59:29.6240882Z 
2019-12-09T01:59:30.5868872Z error: aborting due to 2 previous errors
2019-12-09T01:59:30.5868979Z 
2019-12-09T01:59:30.8270990Z error: could not compile `rustc`.
---
2019-12-09T01:59:30.8387271Z   local time: Mon Dec  9 01:59:30 UTC 2019
2019-12-09T01:59:31.3571903Z   network time: Mon, 09 Dec 2019 01:59:31 GMT
2019-12-09T01:59:31.3572623Z == end clock drift check ==
2019-12-09T01:59:31.9436041Z 
2019-12-09T01:59:31.9537101Z ##[error]Bash exited with code '1'.
2019-12-09T01:59:31.9582920Z ##[section]Starting: Checkout
2019-12-09T01:59:31.9584588Z ==============================================================================
2019-12-09T01:59:31.9584638Z Task         : Get sources
2019-12-09T01:59:31.9584681Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/better-async-error branch from 54c6927 to eff0090 Dec 9, 2019
@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 9, 2019

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-12-09T18:39:44.2950414Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-09T18:39:44.8310556Z ##[command]git config gc.auto 0
2019-12-09T18:39:44.8318836Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-09T18:39:44.8324739Z ##[command]git config --get-all http.proxy
2019-12-09T18:39:44.8329384Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67116/merge:refs/remotes/pull/67116/merge
---
2019-12-09T18:48:03.7256110Z 
2019-12-09T18:48:03.7261299Z error: variant is never constructed: `Other`
2019-12-09T18:48:03.7262094Z   --> src/librustc/traits/error_reporting.rs:50:5
2019-12-09T18:48:03.7262684Z    |
2019-12-09T18:48:03.7263322Z 50 |     Other(&'a dyn fmt::Display)
2019-12-09T18:48:03.7264312Z 
2019-12-09T18:48:04.9299528Z error: aborting due to 2 previous errors
2019-12-09T18:48:04.9300465Z 
2019-12-09T18:48:05.2403839Z error: could not compile `rustc`.
---
2019-12-09T18:48:05.2515129Z   local time: Mon Dec  9 18:48:05 UTC 2019
2019-12-09T18:48:05.5700061Z   network time: Mon, 09 Dec 2019 18:48:05 GMT
2019-12-09T18:48:05.5700643Z == end clock drift check ==
2019-12-09T18:48:06.1261410Z 
2019-12-09T18:48:06.1373900Z ##[error]Bash exited with code '1'.
2019-12-09T18:48:06.1404348Z ##[section]Starting: Checkout
2019-12-09T18:48:06.1406633Z ==============================================================================
2019-12-09T18:48:06.1406716Z Task         : Get sources
2019-12-09T18:48:06.1406770Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@estebank

This comment was marked as off-topic.

Copy link
Contributor

estebank commented Dec 9, 2019

I love the direction this is going.

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 10, 2019

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-12-10T00:19:39.0015508Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-10T00:19:39.0212734Z ##[command]git config gc.auto 0
2019-12-10T00:19:39.0282994Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-10T00:19:39.0339117Z ##[command]git config --get-all http.proxy
2019-12-10T00:19:39.0515868Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67116/merge:refs/remotes/pull/67116/merge
---
2019-12-10T01:19:48.8313777Z .................................................................................................... 1600/9339
2019-12-10T01:19:53.7535233Z .................................................................................................... 1700/9339
2019-12-10T01:20:06.6797496Z ................................................i................................................... 1800/9339
2019-12-10T01:20:15.3164212Z .................................................................................................... 1900/9339
2019-12-10T01:20:30.1237931Z .................................iiiii.............................................................. 2000/9339
2019-12-10T01:20:40.7535009Z .................................................................................................... 2200/9339
2019-12-10T01:20:43.4146300Z .................................................................................................... 2300/9339
2019-12-10T01:20:47.9952801Z .................................................................................................... 2400/9339
2019-12-10T01:21:11.4156444Z .................................................................................................... 2500/9339
---
2019-12-10T01:23:59.9716632Z ....................................i...............i............................................... 4800/9339
2019-12-10T01:24:09.7199422Z .................................................................................................... 4900/9339
2019-12-10T01:24:16.4849543Z ................................................................................i................... 5000/9339
2019-12-10T01:24:22.3798592Z .................................................................................................... 5100/9339
2019-12-10T01:24:32.1144854Z .............................................ii.ii...........i...................................... 5200/9339
2019-12-10T01:24:41.5672018Z .................................................................................................... 5400/9339
2019-12-10T01:24:51.1634270Z .................................................................................................... 5500/9339
2019-12-10T01:24:58.3269985Z ...........................i........................................................................ 5600/9339
2019-12-10T01:25:04.7596955Z .................................................................................................... 5700/9339
2019-12-10T01:25:04.7596955Z .................................................................................................... 5700/9339
2019-12-10T01:25:16.5699532Z .................................................................................................... 5800/9339
2019-12-10T01:25:27.7490723Z ..............ii...i..ii............i............................................................... 5900/9339
2019-12-10T01:25:45.8911108Z .................................................................................................... 6100/9339
2019-12-10T01:25:53.7033966Z .................................................................................................... 6200/9339
2019-12-10T01:25:53.7033966Z .................................................................................................... 6200/9339
2019-12-10T01:26:07.1714530Z ......................................i..ii......................................................... 6300/9339
2019-12-10T01:26:28.4672496Z .................................................................................................... 6500/9339
2019-12-10T01:26:30.5917308Z ..........i......................................................................................... 6600/9339
2019-12-10T01:26:32.9208195Z .................................................................................................... 6700/9339
2019-12-10T01:26:35.4941094Z .i.................................................................................................. 6800/9339
---
2019-12-10T01:28:13.3920647Z .................................................................................................... 7400/9339
2019-12-10T01:28:18.5787042Z .................................................................................................... 7500/9339
2019-12-10T01:28:25.6646887Z .................................................................................................... 7600/9339
2019-12-10T01:28:36.1878327Z .................................................................................................... 7700/9339
2019-12-10T01:28:42.4245604Z .................iiii............................................................................... 7800/9339
2019-12-10T01:28:56.4253166Z .................................................................................................... 8000/9339
2019-12-10T01:29:07.6395156Z .................................................................................................... 8100/9339
2019-12-10T01:29:20.4021626Z .................................................................................................... 8200/9339
2019-12-10T01:29:27.3126985Z .................................................................................................... 8300/9339
---
2019-12-10T01:31:45.9032618Z  finished in 5.979
2019-12-10T01:31:45.9213271Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-10T01:31:46.1027403Z 
2019-12-10T01:31:46.1028191Z running 165 tests
2019-12-10T01:31:49.1610773Z iiii......i.......ii..iiii...i.............................i..i..................i....i............i 100/165
2019-12-10T01:31:51.2472749Z .i.i...iii..iiiiiii.......................iii............ii......
2019-12-10T01:31:51.2473950Z 
2019-12-10T01:31:51.2480758Z  finished in 5.326
2019-12-10T01:31:51.2659690Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-10T01:31:52.2629161Z 
---
2019-12-10T01:31:53.4275421Z  finished in 2.161
2019-12-10T01:31:53.4465102Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-10T01:31:53.6068391Z 
2019-12-10T01:31:53.6069311Z running 9 tests
2019-12-10T01:31:53.6070634Z iiiiiiiii
2019-12-10T01:31:53.6071348Z 
2019-12-10T01:31:53.6071753Z  finished in 0.160
2019-12-10T01:31:53.6264155Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-10T01:31:53.8242064Z 
---
2019-12-10T01:32:13.0541905Z  finished in 19.427
2019-12-10T01:32:13.0761397Z Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-10T01:32:13.2577360Z 
2019-12-10T01:32:13.2578003Z running 124 tests
2019-12-10T01:32:37.8094879Z .iiiii..ii.....i..i...i..i.i.i..i..i..iii....ii.ii....ii..........iiii..........i.....i..ii.......ii 100/124
2019-12-10T01:32:42.0022844Z .i.iii.....iiiiii.....ii
2019-12-10T01:32:42.0023359Z 
2019-12-10T01:32:42.0028947Z  finished in 28.925
2019-12-10T01:32:42.0029432Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-10T01:32:42.0029762Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2019-12-10T01:43:59.9490796Z 
2019-12-10T01:43:59.9492002Z    Doc-tests core
2019-12-10T01:44:04.5555103Z 
2019-12-10T01:44:04.5555975Z running 2435 tests
2019-12-10T01:44:13.6908941Z ......iiiii......................................................................................... 100/2435
2019-12-10T01:44:32.9751136Z .................................................................................................... 300/2435
2019-12-10T01:44:43.4268927Z ..............i..................................................................................... 400/2435
2019-12-10T01:44:43.4268927Z ..............i..................................................................................... 400/2435
2019-12-10T01:44:52.5829649Z ..............................................................i..i..................iiii............ 500/2435
2019-12-10T01:45:08.8840250Z .................................................................................................... 700/2435
2019-12-10T01:45:17.2699057Z .................................................................................................... 800/2435
2019-12-10T01:45:25.7874308Z .................................................................................................... 900/2435
2019-12-10T01:45:34.3303118Z .................................................................................................... 1000/2435
---
2019-12-10T01:47:45.4788944Z 
2019-12-10T01:47:45.4858601Z  finished in 3.503
2019-12-10T01:47:45.4883969Z Testing std stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-12-10T01:47:45.7316754Z    Compiling std v0.0.0 (/checkout/src/libstd)
2019-12-10T01:48:06.8055198Z error: duplicate diagnostic item found: `from_generator`.
2019-12-10T01:48:06.8056390Z   --> src/libstd/future.rs:22:1
2019-12-10T01:48:06.8057429Z    |
2019-12-10T01:48:06.8057990Z 22 | / pub fn from_generator<T: Generator<Yield = ()>>(x: T) -> impl Future<Output = T::Return> {
2019-12-10T01:48:06.8058506Z 23 | |     GenFuture(x)
2019-12-10T01:48:06.8059823Z    | |_^
2019-12-10T01:48:06.8060286Z    |
2019-12-10T01:48:06.8060764Z    = note: first defined in crate `std`.
2019-12-10T01:48:06.8060986Z 
---
2019-12-10T01:48:07.5712284Z   local time: Tue Dec 10 01:48:07 UTC 2019
2019-12-10T01:48:07.8585401Z   network time: Tue, 10 Dec 2019 01:48:07 GMT
2019-12-10T01:48:07.8590805Z == end clock drift check ==
2019-12-10T01:48:08.3809705Z 
2019-12-10T01:48:08.3910822Z ##[error]Bash exited with code '1'.
2019-12-10T01:48:08.3946567Z ##[section]Starting: Checkout
2019-12-10T01:48:08.3948834Z ==============================================================================
2019-12-10T01:48:08.3948878Z Task         : Get sources
2019-12-10T01:48:08.3948934Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 10, 2019

How does this compare against #65345 ?

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Dec 10, 2019

@nikomatsakis: #65345 doesn't appear to handle multiple generator in the same error stack, but it has more diagnostic improvements for when it does detect a generator.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 10, 2019

@Aaron1011

OK. How would you feel about landing #65345 first, and trying to rebase this atop that?

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Dec 10, 2019

@nikomatsakis: Sounds good to me

@bors

This comment was marked as outdated.

Copy link
Contributor

bors commented Dec 11, 2019

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

Aaron1011 added 3 commits Dec 7, 2019
@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/better-async-error branch from 1e450a6 to 17d6d11 Dec 12, 2019
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 12, 2019

@Aaron1011

This comment was marked as outdated.

Copy link
Contributor Author

Aaron1011 commented Dec 12, 2019

My initial attempt at rebasing appears to have broken this PR. I'm investigating what changes are needed to make it work again.

Aaron1011 added 4 commits Dec 12, 2019
@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 12, 2019

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-12-12T22:54:07.0534274Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-12T22:54:07.9223143Z ##[command]git config gc.auto 0
2019-12-12T22:54:07.9228938Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-12T22:54:07.9232972Z ##[command]git config --get-all http.proxy
2019-12-12T22:54:07.9237206Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67116/merge:refs/remotes/pull/67116/merge
---
2019-12-12T23:01:32.4423668Z     Checking syntax_ext v0.0.0 (/checkout/src/libsyntax_ext)
2019-12-12T23:02:01.5327205Z error: variable does not need to be mutable
2019-12-12T23:02:01.5327787Z     --> src/librustc/traits/error_reporting.rs:2191:14
2019-12-12T23:02:01.5328018Z      |
2019-12-12T23:02:01.5328514Z 2191 |         let (mut trait_ref, mut target_ty) = (trait_predicate.trait_ref, trait_predicate.self_ty());
2019-12-12T23:02:01.5329096Z      |              |
2019-12-12T23:02:01.5329584Z      |              help: remove this `mut`
2019-12-12T23:02:01.5330094Z      |
2019-12-12T23:02:01.5330358Z      = note: `-D unused-mut` implied by `-D warnings`
2019-12-12T23:02:01.5330358Z      = note: `-D unused-mut` implied by `-D warnings`
2019-12-12T23:02:01.5330396Z 
2019-12-12T23:02:01.5330647Z error: variable does not need to be mutable
2019-12-12T23:02:01.5330917Z     --> src/librustc/traits/error_reporting.rs:2191:29
2019-12-12T23:02:01.5331120Z      |
2019-12-12T23:02:01.5331541Z 2191 |         let (mut trait_ref, mut target_ty) = (trait_predicate.trait_ref, trait_predicate.self_ty());
2019-12-12T23:02:01.5332469Z      |                             |
2019-12-12T23:02:01.5332780Z      |                             help: remove this `mut`
2019-12-12T23:02:01.5332820Z 
2019-12-12T23:02:01.5339166Z error: variable does not need to be mutable
---
2019-12-12T23:02:10.3247711Z   local time: Thu Dec 12 23:02:10 UTC 2019
2019-12-12T23:02:10.6042743Z   network time: Thu, 12 Dec 2019 23:02:10 GMT
2019-12-12T23:02:10.6046516Z == end clock drift check ==
2019-12-12T23:02:11.2381188Z 
2019-12-12T23:02:11.2499050Z ##[error]Bash exited with code '1'.
2019-12-12T23:02:11.2528338Z ##[section]Starting: Checkout
2019-12-12T23:02:11.2529930Z ==============================================================================
2019-12-12T23:02:11.2529982Z Task         : Get sources
2019-12-12T23:02:11.2530043Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Aaron1011 added 2 commits Dec 12, 2019
@Aaron1011 Aaron1011 marked this pull request as ready for review Dec 12, 2019
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 17, 2019

@Aaron1011

I think the behavior introduced by #65345 is incorrect - it skips over error codes that are completely unrelated to generators:

I can easily believe it's overzealous -- can you give an example where it seems to be doing something incorrect though? I saw cases where it was suppressing information, but I didn't see any case where the message changed back from the new, more specialized format to the older style, more generic message, which is what I thought you were saying would happen. Or maybe I missed something.

Copy link
Member

davidtwco left a comment

I've left some thoughts below.

I'm happy with the implementation changes (pending a clarification in one of the comments below) if we decide we do want these diagnostic changes.

I'm not entirely convinced that the diagnostic changes are all improvements though.

@@ -2342,9 +2412,8 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
("`Sync`", "shared")
};

err.clear_code();

This comment has been minimized.

Copy link
@davidtwco

davidtwco Dec 17, 2019

Member

I don't have any strong feelings about undoing this change from #65345. My intent in clearing the error code was avoiding having E0277 have multiple different primary messages:

#65345 introduced two special-cased error messages:

  • "future cannot be sent between threads safely" (special case for Send)
  • "future cannot be shared between threads safely" (special case for Sync)

E0277 otherwise has the following message:

the trait bound Foo: Qux is not satisfied in impl std::future::Future

If we don't mind having E0277 be the error code for all of these cases, then we should remove this line.

cc @rust-lang/wg-diagnostics

This comment has been minimized.

Copy link
@estebank

estebank Dec 17, 2019

Contributor

I do find E0277 to be a too generic error that happens all over the place, and we have precedence of changing error codes for more specific cases (type errors are not just E0308). We should add new error codes for these cases so that we can add explanations in the index.

@@ -0,0 +1,42 @@
error[E0277]: future cannot be sent between threads safely

This comment has been minimized.

Copy link
@davidtwco

davidtwco Dec 17, 2019

Member

I also think that this is quite a lot of information - I think there is some value in tracing the path that the compiler has taken to users, I could imagine a scenario where the Send bound was introduced quite far away from where the non-Send type was captured. Perhaps for intermediate steps, we could do something like:

note: future is not `Send` as this value is used across an await
  --> $DIR/nested-async-calls.rs:17:5
LL | async fn third() {
   |          ----- in function `third`
LL |     let _a: Outer;
   |         -- has type `third::{{closure}}#0::Outer`
LL |     dummy().await;
   |     ^^^^^^^^^^^^^ await occurs here, with `_a` maybe used later
LL | }
   | - `_a` is later dropped here
note: `third` was invoked by `second`
note: `second` was invoked by `first` which was passed to `require_send`
--> $DIR/issue-64130-4-async-move.rs:15:17
|
LL | pub fn foo() -> impl Future + Send {
| ^^^^^^^^^^^^^^^^^^ future returned by `foo` is not `Send`
|
= help: the trait `std::marker::Sync` is not implemented for `(dyn std::any::Any + std::marker::Send + 'static)`
= note: required because of the requirements on the impl of `std::marker::Sync` for `std::ptr::Unique<(dyn std::any::Any + std::marker::Send + 'static)>`
= note: required because it appears within the type `std::boxed::Box<(dyn std::any::Any + std::marker::Send + 'static)>`
= note: required because it appears within the type `Client`

This comment has been minimized.

Copy link
@davidtwco

davidtwco Dec 17, 2019

Member

I personally find these obligation notes quite dense and hard to extract useful, actionable help out of - I think it might be better to continue omitting them when we provide the async-specific note.


if handle_async_await(*parent_trait_ref.skip_binder(), &data.parent_code) {
return;
}

This comment has been minimized.

Copy link
@davidtwco

davidtwco Dec 17, 2019

Member

Could you elaborate on which cases are supported by adding handle_async_await here? I can't work out from reading the diff which cases this would catch but the existing call in note_obligation_cause wouldn't?

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Dec 17, 2019

We were intentionally trying to avoid overwhelming the user with random types and to help construct for them a mental model of what's going on -- though it seems clear that they might also benefit from "connecting the dots" a bit more than we currently do.

@nikomatsakis: I understand your concern that this PR displays a large amount of information to the user. However, I think this is an unavoidable consequence of the fact that async fns are complicated, especially when it comes to auto traits.

I definitely think there's room to improve the way in which we display the 'stack trace' to users. However, I think we should display some kind of message each time our 'cause' crosses a function boundary.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 20, 2019

@Aaron1011

I think a more compact message is probably the best. It's also possible that it might be good to move the "stack trace" towards the end of the message. What did you think of @davidtwco's suggestion to add the stack track as notes in the end, sort of like this?

note: future is not `Send` as this value is used across an await
  --> $DIR/nested-async-calls.rs:17:5
LL | async fn third() {
   |          ----- in function `third`
LL |     let _a: Outer;
   |         -- has type `third::{{closure}}#0::Outer`
LL |     dummy().await;
   |     ^^^^^^^^^^^^^ await occurs here, with `_a` maybe used later
LL | }
   | - `_a` is later dropped here
note: `third` was invoked by `second`
note: `second` was invoked by `first` which was passed to `require_send`

I would also add I think it might be ok to elide random structs and things that lie in between. It seems like most of the time those will be future adapters like join that don't really add a lot of information for end users?

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Dec 20, 2019

note: third was invoked by second
note: second was invoked by first which was passed to require_send

@nikomatsakis: I'm worried that this kind of message won't be very useful in practice. The call to first most likely corresponds to the code the user is writing (or most recently modified) -e.g. a user is trying to spawn a non-Send future on an executor. However, we're displaying the least infomration about that call, leaving the user to hunt through their code to determine where the call is,

I think it's best to err on the side of providing more information in async-related messages, rather than less. While there is a risk of overwhelming users with too much information, I think that each part of the new message conveys useful information. This is in contrast to the current messages, which pointlessly displays the entire generator witness type.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 21, 2019

@Aaron1011

The extra information that is being suppressed is where, within second, the call to third occurs? (And so on...?) Or is there something else?

I can definitely relate to having been frustrating trying to track an "auto trait" backtrace and wondering "where is type X contained within type Y???". I can imagine users feeling similarly confused, although I'm not sure it's the same. It seems like it doesn't matter much where the call occurs -- unless it can potentialy be removed altogether, I guess.

I think you are underestimating the danger of "too much information" -- but I also can appreciate that not having access to information that the compiler knows is pretty frustrating. But I guess I'd like to hear what others think, I'm not sure of my take here. Or maybe we can mock up a version of the error that gives the backtrace like above, but also includes the point of call to compare?

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 23, 2019

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 31, 2019

I was discussing this with @sfackler the other day. He seemed to feel that showing the "leaf call" was most important, and the call stack distinctly secondary. He pointed out that almost always the code you want to change is the leaf function -- not the intermediate functions -- so it doesn't matter so much "how you got there". This is contrast to other auto trait errors, where you get the error typically because some leaf type like *mut Foo is not Send, but it's not clear where the right place to 'intervene' is.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Jan 1, 2020

He pointed out that almost always the code you want to change is the leaf function -- not the intermediate functions -- so it doesn't matter so much "how you got there".

That makes some amount of sense to me (as long as we're still showing a span and explanation for the 'root' call, e.g. where we first required that a generator be Send/Sync).

I'm still concerned that hiding information is making the wrong trade-off here. I think there are two main cases to worry about:

  1. The user causes a !Send/!Sync error due to a problem at the 'leaf' call. We display the entire 'call stack', which is more information than they really need. If there are multiple such errors, it could become difficult to read and understand all of them.
  2. The user cases a !Send/!Sync error, but the 'leaf' call is not clearly at fault. For example, a 'legitimately' !Send generator gets .awaite'd deep in the call stack. Here, the user is lacking important information (why the 'leaf' call actually ended up in the call stack), which we could have displayed but chose not to.

I think case 2 is much more concerning than case 1. I think it's important to keep in mind that this kind of error combines several concepts which don't really exist (AFAIK) in other mainstream languages:

  • Thread safety enforced via the type system (Send/Sync)
  • Trait implementations depending on 'constituent' types (auto traits)
  • Type errors due to the call stack (a !Send generator wrapped in several intermediate generators).

This means that many people will never have seen this kind of error before. I think we should be much more concerned about producing 'unactionable' error messages (the user has no idea where to begin to solve the error) than about producing 'overwhelming' error messages (we display a lot of information that could have been omitted).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 2, 2020

@Aaron1011 I think that's a helpful analysis, and indeed that is the case I am worried about as well.

This is the sort of case where I really wish we had a more "interactive" way of presenting errors, so that people could expand out to get more information. (Or perhaps supply a command-line option for more details.)

In any case, I was hoping that a minimal trace ("called from here...") would indeed help in that second scenario, by identifying the call chain. I guess what is missing in such a trace is showing precisely where the call occurred? That's the only info that's omitted, right?

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Jan 5, 2020

I guess what is missing in such a trace is showing precisely where the call occurred? That's the only info that's omitted, right?

That's right.

What's the next step for this PR?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 8, 2020

@Aaron1011 good question! I guess I don't really know how to resolve this. I continue to feel best about having a kind of "limited" stack trace and worried that adding more information would overwhelm people, but I think what we most need is more input. I'd be happy to add the info if more people were indicating they would find it useful.

In the extreme, we could some sort of survey...

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 9, 2020

Here's a random idea to throw out: emit the shorter error, but add a -Z flag that says "please emit the longer error" (maybe this is just -Zteach, or we could create a custom one, doesn't matter).

Also, in the shorter error, link an issue and notify the user about -Zteach (if on nightly) asking for feedback :)

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Jan 9, 2020

Here's a random idea to throw out: emit the shorter error, but add a -Z flag that says "please emit the longer error" (maybe this is just -Zteach, or we could create a custom one, doesn't matter).

The issue with this is that enabling it will require a full rebuilds if the entire crate graph (and then another rebuild when you disable it again).

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 9, 2020

Hm, maybe we can work around that? (Environment variable or something) It also seems plausibly not bad. Maybe even just giving an issue link without the switching and asking for feedback is good (it's sort of like the survey suggested by Niko, but maybe a bit easier).

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Jan 11, 2020

Maybe even just giving an issue link without the switching and asking for feedback is good (it's sort of like the survey suggested by Niko, but maybe a bit easier).

Without actually showing the intermediate spans, I'm not sure how useful that would be. If showing that information would end up being distracting and not very useful, this we wouldn't find out by just giving an issue link. Conversely, users might not realize that showing extra spans would be helpful, since they don't know what they would be seeing for their particular error.

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Jan 18, 2020

triage: merge conflicts

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Jan 19, 2020

@JohnCSimon: This PR is currently blocked on reaching some kind of consensus as to what these kinds of errors should look like:

  • "Intermediate" frames are always hidden
  • "Intermediate" frames are hidden by default, can be displayed via a compiler flag
  • Something else
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 24, 2020

@Aaron1011 do you think you could prepare a hackmd or gist that summarizes the question at hand? It would be great to give some example programs along with two sample outputs, for example. I would like to focus the discussion on very specific examples and proposals. I'd also like to get this resolved. =)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 6, 2020

OK, I prepared a summary of the question at hand -- does this sound like it captures the question

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 6, 2020

I was thinking of starting an internals thread to drive this to a conclusion

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 6, 2020

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 13, 2020

We discussed this in our compiler team triage meeting. The internals thread seems to have died down somewhat but the polling at least suggests a preference for "minimal stack trace" (72%).

There is definitely some interest in a "verbose mode" as well. But we felt that would best be preferred as a separate consideration.

@Aaron1011 do you think you could explore altering the PR to give a more "minimal stack trace"? We'll have to experiment a bit with the best way to format that, I imagine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

9 participants
You can’t perform that action at this time.