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

non-send futures error at point of use, but could cite point of definition in some cases #64130

Open
nikomatsakis opened this issue Sep 3, 2019 · 5 comments

Comments

@nikomatsakis
Copy link
Contributor

commented Sep 3, 2019

Consider this example (playground):

#![feature(async_await)]

use std::sync::Mutex;

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

async fn foo() {
  bar(&Mutex::new(22)).await;
}

async fn bar(x: &Mutex<u32>) {
  let g = x.lock().unwrap();
  baz().await;
}

async fn baz() {
    
}

fn main() {
    is_send(foo());
} 

This program is in error because bar() is holding a mutex guard (which is not Send) live across the baz().await call. The error however is rather opaque:

error[E0277]: `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
  --> src/main.rs:23:5
   |
23 |     is_send(foo());
   |     ^^^^^^^ `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, u32>`
   = note: required because it appears within the type `for<'r, 's> {&'r std::sync::Mutex<u32>, std::sync::MutexGuard<'s, u32>, impl std::future::Future, ()}`
   = note: required because it appears within the type `[static generator@src/main.rs:13:30: 16:2 x:&std::sync::Mutex<u32> for<'r, 's> {&'r std::sync::Mutex<u32>, std::sync::MutexGuard<'s, u32>, impl std::future::Future, ()}]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@src/main.rs:13:30: 16:2 x:&std::sync::Mutex<u32> for<'r, 's> {&'r std::sync::Mutex<u32>, std::sync::MutexGuard<'s, u32>, impl std::future::Future, ()}]>`
   = note: required because it appears within the 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 `for<'r> {impl std::future::Future, ()}`
   = note: required because it appears within the type `[static generator@src/main.rs:9:16: 11:2 for<'r> {impl std::future::Future, ()}]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@src/main.rs:9:16: 11:2 for<'r> {impl std::future::Future, ()}]>`
   = note: required because it appears within the type `impl std::future::Future`
   = note: required because it appears within the type `impl std::future::Future`
note: required by `is_send`
  --> src/main.rs:5:1
   |
5  | fn is_send<T: Send>(t: T) {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^

When discussing this error, @cramertj suggested that we might look for patterns like this and try to offer a customized error. They sketched out

error[E0277]: `foo()` cannot be sent between threads
note: the `is_send` function requires `T: Send`:
  --> src/main.rs:5:1
   |
5  | fn is_send<T: Send>(t: T) {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
but in this invocation, the `foo()` is not `Send`
  --> src/main.rs:23:5
   |
23 |     is_send(foo());
   |             ^^^^^
because it is of type `impl Future<Output = ()>`
which contains an `std::sync::MutexGuard<'_, u32>`
note: pass `--full-error-type-info` for more information

Something along these lines could be a big improvement!

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

I'm abusing the blocking tag here to try and keep this on the shortlist of "things we might try to do before beta". Assigning to myself to leave some mentoring notes.

@estebank

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

CC #42855

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

I laid out something thinking in this Zulip thread

bors added a commit that referenced this issue Sep 11, 2019
Auto merge of #64151 - estebank:binding-error, r=varkor
On obligation errors point at the unfulfilled binding when possible

CC #42855, #64130, #64135.
Centril added a commit to Centril/rust that referenced this issue Sep 16, 2019
Rollup merge of rust-lang#64151 - estebank:binding-error, r=varkor
On obligation errors point at the unfulfilled binding when possible

CC rust-lang#42855, rust-lang#64130, rust-lang#64135.
bors added a commit that referenced this issue Sep 17, 2019
Auto merge of #64151 - estebank:binding-error, r=varkor
On obligation errors point at the unfulfilled binding when possible

CC #42855, #64130, #64135.
@davidtwco

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

@rustbot claim

@rustbot rustbot assigned davidtwco and unassigned nikomatsakis Sep 17, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

OK let me try to leave a few notes here.

First, we need to decide how this error should look. I created a gist that shows roughly what I'm going after, but it's not ideal:

https://gist.github.com/nikomatsakis/ac95488236dec61976c92e523df622f9

First off, the messages like this:

 = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, u32>`
   = note: required because it appears within the type `for<'r, 's> {&'r std::sync::Mutex<u32>, std::sync::MutexGuard<'s, u32>, impl std::future::Future, ()}`
   = note: required because it appears within the type `[static generator@src/main.rs:13:30: 16:2 x:&std::sync::Mutex<u32> for<'r, 's> {&'r std::sync::Mutex<u32>, std::sync::MutexGuard<'s, u32>, impl std::future::Future, ()}]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@src/main.rs:13:30: 16:2 x:&std::sync::Mutex<u32> for<'r, 's> {&'r std::sync::Mutex<u32>, std::sync::MutexGuard<'s, u32>, impl std::future::Future, ()}]>`
   = note: required because it appears within the 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 `for<'r> {impl std::future::Future, ()}`
   = note: required because it appears within the type `[static generator@src/main.rs:9:16: 11:2 for<'r> {impl std::future::Future, ()}]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@src/main.rs:9:16: 11:2 for<'r> {impl std::future::Future, ()}]>`
   = note: required because it appears within the type `impl std::future::Future`
   = note: required because it appears within the type `impl std::future::Future`

are generated by the note_obligation_cause_code function:

fn note_obligation_cause<T>(&self,
err: &mut DiagnosticBuilder<'_>,
obligation: &Obligation<'tcx, T>)
where T: fmt::Display
{
self.note_obligation_cause_code(err,
&obligation.predicate,
&obligation.cause.code,
&mut vec![]);
}

What happens here is that we track some information about why we are forced to prove any particular trait relationship. For auto trait chains, we wind up with a chain of "obligation causes", specifically these two variants:

ObligationCauseCode::BuiltinDerivedObligation(ref data) => {

ObligationCauseCode::ImplDerivedObligation(ref data) => {

So, to start, I think what we want to do is to look for the case where we have a chain of obligation causes that leads to generator types (we might want to include closures here, too, but I'm going to ignore them for now) and invoke a special path for such cases. To start, we could limit this to cases that involve only generators, generator interiors, and opaque type (apart from the top type), which seems to be the case in our example. This won't handle examples where futures are combined, e.g.

async fn foo() {...}
async fn bar() {...}
fn is_send<T: Send>(_: T) { }
fn main() {
    is_send((foo(), bar())); // introduces a tuple type into the chain
}

but that seems ok to start. We can always think how to generalize it.

I'm imagining we'll basically have a function like obligation_cause_code_to_stack_trace that takes a chain of causes and, if it consists solely of generators/opaque types, converts it to a list of "stack frames". These would contain the def-id of each generator and maybe we'll also try to identify
which "subtype" of the generator interior we passed through.

So then the idea would be that we can dump this "stack trace" as a series of notes. If the generator is local to the crate, we'll try to grab its span, but if it's foreign, we'll just print a path.

Now, the nice thing is that each generator type is tagged with its def-id:

rust/src/librustc/ty/sty.rs

Lines 163 to 165 in dece573

/// The anonymous type of a generator. Used to represent the type of
/// `|a| yield a`.
Generator(DefId, GeneratorSubsts<'tcx>, hir::GeneratorMovability),

If this is a local def-id, we can go from that to get the HIR for the function and a span. If it's not local, we can't get the HIR, but we can print a path to it.

One challenge is that the "generator interior" contains types that originate from expressions and variables, but it doesn't currently track where those types came from. It's just a list of types:

rust/src/librustc/ty/sty.rs

Lines 167 to 169 in dece573

/// A type representin the types stored inside a generator.
/// This should only appear in GeneratorInteriors.
GeneratorWitness(Binder<&'tcx List<Ty<'tcx>>>),

This value is computed by generator_interior module here:

https://github.com/rust-lang/rust/blob/dece57302a8e6775b34dd6447eb98552e83bdc9d/src/librustc_typeck/check/generator_interior.rs

What we do is to visit patterns and expressions, looking for those that may overlook a type. For each one, we invoke the record function:

fn record(&mut self,
ty: Ty<'tcx>,
scope: Option<region::Scope>,
expr: Option<&'tcx Expr>,
source_span: Span) {

This function adds the type into the list of types if (a) it's not already present and (b) it may be live over a yield. The actual add happens here:

self.types.entry(&ty).or_insert(entries);

I think what I would like to do is to also remember the hir_id that caused this type to be added. This would probably be stored in the typeck-tables for the generator. Then, when we are printing the stack trace above, and we see an error was reported for some type in the interior, we can find its index and use that to grab the hir_id that caused the type to be added. This maps to a span (or maybe we want to store the span separately) that we can report to the user, but it also can be used to find the yield by calling yield_in_scope_for_expr.

bors added a commit that referenced this issue Sep 19, 2019
Auto merge of #64151 - estebank:binding-error, r=varkor
On obligation errors point at the unfulfilled binding when possible

CC #42855, #64130, #64135.
Centril added a commit to Centril/rust that referenced this issue Sep 19, 2019
Rollup merge of rust-lang#64151 - estebank:binding-error, r=varkor
On obligation errors point at the unfulfilled binding when possible

CC rust-lang#42855, rust-lang#64130, rust-lang#64135.
bors added a commit that referenced this issue Sep 19, 2019
Auto merge of #64151 - estebank:binding-error, r=varkor
On obligation errors point at the unfulfilled binding when possible

CC #42855, #64130, #64135.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.