Poll: how much context to give for async error messages?

One of the challenges of the current async await design is that a requirement that a future must be Send often comes at some top-level function, where the actual problem that prevents the future from being Send occurs in some other function. We're struggling a bit at how much information to present and how in order to best explain what's going on. This thread explores a particular example and some possible errors. There is a poll at the end!

Simple example: one step

Consider this example (playground):

fn is_send<T: Send>(t: T) { }

fn main() {
    is_send(async_fn1(generate()));
}

async fn async_fn1(future: impl Future + Send) {
    let x = Mutex::new(22);
    let data = x.lock().unwrap();
    future.await;
}

async fn generate() {
    
}

Currently, it gives the following error:

error: future cannot be sent between threads safely
  --> src/main.rs:9:5
   |
6  | fn is_send<T: Send>(t: T) { }
   |    -------    ---- required by this bound in `is_send`
...
9  |     is_send(async_fn1(generate()));
   |     ^^^^^^^ future returned by `async_fn1` is not `Send`
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, i32>`
note: future is not `Send` as this value is used across an await
  --> src/main.rs:15:5
   |
14 |     let data = x.lock().unwrap();
   |         ---- has type `std::sync::MutexGuard<'_, i32>`
15 |     future.await;
   |     ^^^^^^^^^^^^ await occurs here, with `data` maybe used later
16 | }
   | - `data` is later dropped here

As the error explains, the problem is reported in the main function, but it's caused by the code in async_fn1. I think this error is pretty decent, though I'm definitely open to suggestions on how to improve this case.

Example with multiple steps

Now consider this case (playground). The only difference is that main calls async_fn3, which in turn calls async_fn2, and ultimately async_fn1, which has the problem:

fn is_send<T: Send>(t: T) { }

fn main() {
    is_send(async_fn3(generate()));
}

async fn async_fn3(future: impl Future + Send) {
    async_fn2(future).await;
}

async fn async_fn2(future: impl Future + Send) {
    async_fn1(future).await;
}

async fn async_fn1(future: impl Future + Send) {
    let x = Mutex::new(22);
    let data = x.lock().unwrap();
    future.await;
}

async fn generate() {
    
}

The error we report in this case is largely unchanged:

error: future cannot be sent between threads safely
  --> src/main.rs:9:5
   |
6  | fn is_send<T: Send>(t: T) { }
   |    -------    ---- required by this bound in `is_send`
...
9  |     is_send(async_fn3(generate()));
   |     ^^^^^^^ future returned by `async_fn3` is not `Send`
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, i32>`
note: future is not `Send` as this value is used across an await
  --> src/main.rs:23:5
   |
22 |     let data = x.lock().unwrap();
   |         ---- has type `std::sync::MutexGuard<'_, i32>`
23 |     future.await;
   |     ^^^^^^^^^^^^ await occurs here, with `data` maybe used later
24 | }
   | - `data` is later dropped here

The concern

There are two concerns with the error messages here:

  • They are complex and trying to pack in and explain a lot of stuff.
  • Also, with our current reporting, we show only the point of the error (main) and the leaf function (async_fn1) that caused the error. Users have to intuit the path between them.

There is a bit of a trade-off here that we are trying to decide how to resolve. Adding more information makes the message more complex and foreboding, but leaving it out means that users have to figure out.

How to resolve this?

There are a few thoughts on how to resolve this.

Do not show stack trace details (i.e., status quo)

We could keep the status quo. After all, if you want to fix the bug, almost always you will do so by altering the code in the leaf function, so it's not that interesting to find the path from the cause of error to the leaf function, and it's usually not that hard to find. @sfackler expressed this opinion in the past (not to put words in their mouth).

Give the full stack trace

At the other extreme, PR #67116 proposed to alter our reporting in these "multi-step" to include the full details for each step along the way. So, for a very similar example, we get output like this:

error[E0277]: future cannot be sent between threads safely
  --> $DIR/nested-async-calls.rs:26:5
   |
LL | fn require_send<T: Send>(_val: T) {}
   |    ------------    ---- required by this bound in `require_send`
...
LL |     require_send(wrapped);
   |     ^^^^^^^^^^^^ future returned by `first` is not `Send`
   |
   = help: within `main::Wrapper<impl std::future::Future>`, the trait `std::marker::Send` is not implemented for `*const ()`
   = note: required because it appears within the type `third::{{closure}}#0::NotSend`
note: future is not `Send` as this value is used across an await
  --> $DIR/nested-async-calls.rs:17:5
   |
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: future is not `Send` as this value is used across an await
  --> $DIR/nested-async-calls.rs:8:5
   |
LL |     third().await;
   |     -------^^^^^^- `third()` is later dropped here
   |     |
   |     await occurs here, with `third()` maybe used later
   |     has type `impl std::future::Future`
note: future is not `Send` as this value is used across an await
  --> $DIR/nested-async-calls.rs:4:5
   |
LL |     second().await;
   |     --------^^^^^^- `second()` is later dropped here
   |     |
   |     await occurs here, with `second()` maybe used later
   |     has type `impl std::future::Future`
   = note: required because it appears within the type `impl std::future::Future`
   = note: required because it appears within the type `main::Wrapper<impl std::future::Future>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.

Minimal notes

A middle ground might be to note the functions in the stack trace, without giving full details (see the final "note" entries at the end), although we might want to show line numbers or some bit of more information as well:

error[E0277]: future cannot be sent between threads safely
  --> $DIR/nested-async-calls.rs:26:5
   |
LL | fn require_send<T: Send>(_val: T) {}
   |    ------------    ---- required by this bound in `require_send`
...
LL |     require_send(wrapped);
   |     ^^^^^^^^^^^^ future returned by `first` is not `Send`
   |
   = help: within `main::Wrapper<impl std::future::Future>`, the trait `std::marker::Send` is not implemented for `*const ()`
   = note: required because it appears within the type `third::{{closure}}#0::NotSend`
note: future is not `Send` as this value is used across an await
  --> $DIR/nested-async-calls.rs:17:5
   |
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()` is called from `second()`
   = note: `second()` is called from `first()`
   = note: `first()` is wrapped within the type within the type `main::Wrapper<impl std::future::Future>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.

Give user control

We don't presently have an option to request verbose errors. We could add this. I am somewhat skeptical -- I think most folks won't know it exists, and I buy into the idea that we should try to tune the defaults to be as useful as we can (without being overwhelming) and avoid adding a lot of knobs. Knobs in particular feel like they will allow us to expend less effort on the defaults and -- since most folks won't use them -- the overall quality of our errors goes down.

What I would like from you

Well, first and foremost, there is a poll in the next section to indicate which of the options you prefer. But you may also have ideas we've not considered! I'm particularly interested in feedback from:

  • people who are using async-await frequently
  • specific examples of code where having the full backtrace would have been useful
    • or cases where you encountered the current (minimal) error and felt confused because it was hard to connect the two points of error
  • any suggestions on how we might improve the "core error" as well

Poll

To help simplify things, let's have a poll! Here are the choices that I described above. Vote for as many as you would like to indicate what you prefer. For the purpose of this poll, I've left off the "verbose flag" option. I'd like to know what people think we should do if we don't add any flags. (OK fine, if you really want to vote for a verbose flag, I added an option for that :wink: But please also select one of the other choices too)

  • Do not show stack trace details
  • Show full stack trace details
  • Show minimal stack trace details
  • Other (add a comment below)
  • I can't help it, I really really want a verbose flag. But I've also selected one of the other options.

0 voters

6 Likes

Maybe "rather than printing the span with the offending await, add a note pointing out that some function produces non-Send futures". E.g:

   = note: the `Future` returned by `third` awaits across a non-`Send` type

I think this is mostly an even more minimal form of your "minimal trace details" option. It balances the desire of knowing who's fault this is, with not producing gigantic errors involving functions deep in the stack trace, reminiscent of clang template instantiation errors (std::make_unique with private ctor is a common one). If you own the function responsible, you want to know which it is. If you don't own it, you're going to need the full trace anyway, which is an O(N) line error (which I would prefer to avoid...)

A minimal stack trace by would be inline with how missing auto traits impls are handled gor normal data types.

I voted 'minimal', but for completeness: while I do agree that all the arguments presented make a naive "verbose flag" option a very bad idea, I do think a verbose flag that we explicitly mention in the default output is fine in principle. IIUC --explain is pretty close to being that already. But in this context, I think 'minimal' by default covers 99.99% of these cases just fine so it's not worth adding a flag anyway.

5 Likes

I think it's like external macro backtrace: most of the time you don't need it, but when you need it you need it.

4 Likes

That opens the conversation to whether the regular trait errors should be more verbose. :slightly_smiling_face:

Regular traits directly by the user, so I don't see how they could be more verbose. With auto traits, you could have a Rc deeply nested in your type, so when you try to send it to another thread, a sort of "stack trace" is neccessary to see why you can't send it to another thread.

I guess with normal generic trait impls we could do better. For example, if I implement a trait for all Vec<T>, where T: Copy, then check if Vec<Box_>> implements the trait, I should get an error noting Box is not Copy. In order to be consistent.

But given that this discussion is about auto traits for async functions, this isn't relevant. But we should be consistent with auto trait handling.

I was thinking about this some more and I had a few thoughts:

  • First of all, I think it would make sense to spend more time iterating on the original message. e.g., I think it might be better if we inverted the place that we "report" the error so that we report the leaf function as the primary place, and then give the "main function" as an additional "note" with context. This would require some experimentation.
  • More than a verbose flag, I'd be really excited if we had ways to "go beyond the command line" in terms of giving rich errors that can be expanded to get more context. I don't yet know how that would work. I loved the idea of webm that @nrc was pursuing at some point, but who knows if it would take off; better would presumably be to integrate with VSCode, but I don't know that they have any features that would permit richer error messages.
  • If we were going to add a "verbose" flag, I was thinking that one of my main concerns would be people not knowing it's there, meaning that we do a lot of work to add it with little value. But maybe if there were a "note" that reminds you that you can use this flag to get the full details, I'd feel somewhat better about it. Still, I think I remain of the feeling that we should push really hard to see what we can get without such a flag, because I'm not convinced we've exhausted the space here.
3 Likes

FYI, your second playground link is just an <a> tag with no href field. It doesn't go anyway

This would be extremely useful, potentially for other types of errors as well. With 'plain' command-line errors, there's always a trade-off between displaying more context and overwhelming the user/displaying useless information.

I think it's very important that any such 'verbose' flag not require recompilation of the entire crate graph. For example, it would be extremely painful to try to use this flag when working on librustc_driver if it meant rebuilding (even with incr comp) the entire compiler.

Could rustc always generate these more in-depth error messages and have cargo display them or demand?

That seems clear. If we added a --verbose flag, I would expect one to do cargo build --verbose or something like that to get the result. But really I think this is just a distinct feature. (In the interim, of course, you could do cargo rustc -- --verbose.)

But already this gets to part of why I'd prefer not to go that route: this "verbose output" feature then has to be routed through various bits of infrastructure to be nice. (I guess it could be an env var, but that has its obstacles too.)

1 Like

A useful alternative to a verbose flag would be to default to verbose output and use some variation of a quiet flag to compress warnings and errors. I personally get a bit annoyed with rustc's output when I'm doing large rewrites and I'm fully expecting to get a lot of "mut needed", "deref needed", "unused import", "unused variable", "name not in scope," etc, which all take up multiple lines today. It would be great if I could pass in a flag to restrict these kinds of messages to a single line when I know I don't need any additional info to resolve them.

I do wonder if we could make --explain better... I like to imagine passing --explain <u32 hash> to cargo check would supress all errors except for one with that hash, and provide relevant expansion of it. This may be just a symptom of rustc not having something like the go compiler's "too many errors" error, though...

(Errors, of course, would have to include a note with the --explain hash...)

1 Like

That's an interesting idea. Cargo already does this sort of 'output caching' to display warnings when a crate is not rebuilt, so we might be able to build on that.

I wonder if it matters whether code is mine (in my crate/my workspace) vs code in 3rd party library.

The stack doesn't seem necessary if the fix only belongs to the leaf function, but I may need the stack is if I did not expect the leaf function to be called in this context. In that case the fix isn't in the leaf function, but requires restructuring the program to call different things.


Idea of --explain combining pre-written explanation with actual code that is affected sounds awesome.

1 Like

For new users, I think it would make the most sense if --verbose was enabled by default or instead a --quiet=2 level was used, defaulting to the most verbose output. That way experienced users can set their level up in ~/.cargo/config if they don't want the long errors anymore.

1 Like

More than a verbose flag, I'd be really excited if we had ways to "go beyond the command line" in terms of giving rich errors that can be expanded to get more context.

Django (the Python web-framework) used to have very nice error messages on exceptions when running in debug mode (runtime errors, but probably many of them for things that would be compile-time errors in Rust). They were simple html pages, with some basic context, but an expandable stack-trace, expandable captured variables for each function call, expandable source-code snippets, etc.

Of course, this was in a context of a web framework, so you were (in many cases) already viewing the page in a browser, making html an easy choice; for a commandline tool this might be a harder sell. Still, emitting a detailed html somewhere (target dir?) and adding a line to the output saying where you can find it and open with your browser might be good enough - and would probably make IDE integration relatively easy (they just need to display this html, perhaps even skipping the console output entirely).

2 Likes