Auto merge of #70679 - tmandry:issue-68112, r=nikomatsakis

bors · bors · commit a3ef360368ac · 2020-04-14T06:47:38.000Z
Improve async-await/generator obligation errors in some cases Fixes #68112. This change is best read one commit at a time (I add a test at the beginning and update it in each change after). The `test2` function is a case I found while writing the test that we don't handle with this code yet. I don't attempt to fix it in this PR, but it's a good candidate for future work. r? @davidtwco, @nikomatsakis
diff --git a/src/librustc_ast_lowering/expr.rs b/src/librustc_ast_lowering/expr.rs
@@ -590,6 +590,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
             await_span,
             self.allow_gen_future.clone(),
         );
+        let expr = self.lower_expr(expr);
 
         let pinned_ident = Ident::with_dummy_span(sym::pinned);
         let (pinned_pat, pinned_pat_hid) =
@@ -671,7 +672,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
             let unit = self.expr_unit(span);
             let yield_expr = self.expr(
                 span,
-                hir::ExprKind::Yield(unit, hir::YieldSource::Await),
+                hir::ExprKind::Yield(unit, hir::YieldSource::Await { expr: Some(expr.hir_id) }),
                 ThinVec::new(),
             );
             let yield_expr = self.arena.alloc(yield_expr);
@@ -704,7 +705,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
         // match <expr> {
         //     mut pinned => loop { .. }
         // }
-        let expr = self.lower_expr(expr);
         hir::ExprKind::Match(expr, arena_vec![self; pinned_arm], hir::MatchSource::AwaitDesugar)
     }
 
diff --git a/src/librustc_hir/hir.rs b/src/librustc_hir/hir.rs
@@ -1736,15 +1736,24 @@ pub struct Destination {
 #[derive(Copy, Clone, PartialEq, Eq, Debug, RustcEncodable, RustcDecodable, HashStable_Generic)]
 pub enum YieldSource {
     /// An `<expr>.await`.
-    Await,
+    Await { expr: Option<HirId> },
     /// A plain `yield`.
     Yield,
 }
 
+impl YieldSource {
+    pub fn is_await(&self) -> bool {
+        match self {
+            YieldSource::Await { .. } => true,
+            YieldSource::Yield => false,
+        }
+    }
+}
+
 impl fmt::Display for YieldSource {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.write_str(match self {
-            YieldSource::Await => "`await`",
+            YieldSource::Await { .. } => "`await`",
             YieldSource::Yield => "`yield`",
         })
     }
@@ -1755,7 +1764,7 @@ impl From<GeneratorKind> for YieldSource {
         match kind {
             // Guess based on the kind of the current generator.
             GeneratorKind::Gen => Self::Yield,
-            GeneratorKind::Async(_) => Self::Await,
+            GeneratorKind::Async(_) => Self::Await { expr: None },
         }
     }
 }
diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs
diff --git a/src/librustc_typeck/check/expr.rs b/src/librustc_typeck/check/expr.rs
@@ -1797,7 +1797,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             // we know that the yield type must be `()`; however, the context won't contain this
             // information. Hence, we check the source of the yield expression here and check its
             // value's type against `()` (this check should always hold).
-            None if src == &hir::YieldSource::Await => {
+            None if src.is_await() => {
                 self.check_expr_coercable_to_type(&value, self.tcx.mk_unit());
                 self.tcx.mk_unit()
             }
diff --git a/src/test/ui/async-await/async-fn-nonsend.stderr b/src/test/ui/async-await/async-fn-nonsend.stderr
@@ -12,7 +12,7 @@ note: future is not `Send` as this value is used across an await
   --> $DIR/async-fn-nonsend.rs:24:5
    |
 LL |     let x = non_send();
-   |         - has type `impl std::fmt::Debug`
+   |         - has type `impl std::fmt::Debug` which is not `Send`
 LL |     drop(x);
 LL |     fut().await;
    |     ^^^^^^^^^^^ await occurs here, with `x` maybe used later
@@ -33,7 +33,7 @@ note: future is not `Send` as this value is used across an await
   --> $DIR/async-fn-nonsend.rs:33:20
    |
 LL |     match Some(non_send()) {
-   |                ---------- has type `impl std::fmt::Debug`
+   |                ---------- has type `impl std::fmt::Debug` which is not `Send`
 LL |         Some(_) => fut().await,
    |                    ^^^^^^^^^^^ await occurs here, with `non_send()` maybe used later
 ...
@@ -54,7 +54,7 @@ note: future is not `Send` as this value is used across an await
   --> $DIR/async-fn-nonsend.rs:42:9
    |
 LL |     let f: &mut std::fmt::Formatter = panic!();
-   |         - has type `&mut std::fmt::Formatter<'_>`
+   |         - has type `&mut std::fmt::Formatter<'_>` which is not `Send`
 LL |     if non_sync().fmt(f).unwrap() == () {
 LL |         fut().await;
    |         ^^^^^^^^^^^ await occurs here, with `f` maybe used later
diff --git a/src/test/ui/async-await/issue-64130-1-sync.stderr b/src/test/ui/async-await/issue-64130-1-sync.stderr
@@ -12,7 +12,7 @@ note: future is not `Sync` as this value is used across an await
   --> $DIR/issue-64130-1-sync.rs:15:5
    |
 LL |     let x = Foo;
-   |         - has type `Foo`
+   |         - has type `Foo` which is not `Sync`
 LL |     baz().await;
    |     ^^^^^^^^^^^ await occurs here, with `x` maybe used later
 LL | }
diff --git a/src/test/ui/async-await/issue-64130-2-send.stderr b/src/test/ui/async-await/issue-64130-2-send.stderr
@@ -12,7 +12,7 @@ note: future is not `Send` as this value is used across an await
   --> $DIR/issue-64130-2-send.rs:15:5
    |
 LL |     let x = Foo;
-   |         - has type `Foo`
+   |         - has type `Foo` which is not `Send`
 LL |     baz().await;
    |     ^^^^^^^^^^^ await occurs here, with `x` maybe used later
 LL | }
diff --git a/src/test/ui/async-await/issue-64130-3-other.stderr b/src/test/ui/async-await/issue-64130-3-other.stderr
@@ -16,7 +16,7 @@ note: future does not implement `Qux` as this value is used across an await
   --> $DIR/issue-64130-3-other.rs:18:5
    |
 LL |     let x = Foo;
-   |         - has type `Foo`
+   |         - has type `Foo` which does not implement `Qux`
 LL |     baz().await;
    |     ^^^^^^^^^^^ await occurs here, with `x` maybe used later
 LL | }
diff --git a/src/test/ui/async-await/issue-64130-4-async-move.stderr b/src/test/ui/async-await/issue-64130-4-async-move.stderr
@@ -2,7 +2,7 @@ error: future cannot be sent between threads safely
   --> $DIR/issue-64130-4-async-move.rs:15:17
    |
 LL |   pub fn foo() -> impl Future + Send {
-   |                   ^^^^^^^^^^^^^^^^^^ future returned by `foo` is not `Send`
+   |                   ^^^^^^^^^^^^^^^^^^ future created by async block is not `Send`
 ...
 LL | /     async move {
 LL | |         match client.status() {
@@ -18,7 +18,7 @@ note: future is not `Send` as this value is used across an await
   --> $DIR/issue-64130-4-async-move.rs:21:26
    |
 LL |         match client.status() {
-   |               ------ has type `&Client`
+   |               ------ has type `&Client` which is not `Send`
 LL |             200 => {
 LL |                 let _x = get().await;
    |                          ^^^^^^^^^^^ await occurs here, with `client` maybe used later
diff --git a/src/test/ui/async-await/issue-64130-non-send-future-diags.stderr b/src/test/ui/async-await/issue-64130-non-send-future-diags.stderr
@@ -12,7 +12,7 @@ note: future is not `Send` as this value is used across an await
   --> $DIR/issue-64130-non-send-future-diags.rs:15:5
    |
 LL |     let g = x.lock().unwrap();
-   |         - has type `std::sync::MutexGuard<'_, u32>`
+   |         - has type `std::sync::MutexGuard<'_, u32>` which is not `Send`
 LL |     baz().await;
    |     ^^^^^^^^^^^ await occurs here, with `g` maybe used later
 LL | }
diff --git a/src/test/ui/async-await/issue-67252-unnamed-future.stderr b/src/test/ui/async-await/issue-67252-unnamed-future.stderr
@@ -5,14 +5,14 @@ LL | fn spawn<T: Send>(_: T) {}
    |             ---- required by this bound in `spawn`
 ...
 LL |     spawn(async {
-   |     ^^^^^ future is not `Send`
+   |     ^^^^^ future created by async block is not `Send`
    |
    = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `*mut ()`
 note: future is not `Send` as this value is used across an await
   --> $DIR/issue-67252-unnamed-future.rs:20:9
    |
 LL |         let _a = std::ptr::null_mut::<()>(); // `*mut ()` is not `Send`
-   |             -- has type `*mut ()`
+   |             -- has type `*mut ()` which is not `Send`
 LL |         AFuture.await;
    |         ^^^^^^^^^^^^^ await occurs here, with `_a` maybe used later
 LL |     });
diff --git a/src/test/ui/async-await/issue-68112.rs b/src/test/ui/async-await/issue-68112.rs
@@ -0,0 +1,64 @@
+// edition:2018
+
+use std::{
+    future::Future,
+    cell::RefCell,
+    sync::Arc,
+    pin::Pin,
+    task::{Context, Poll},
+};
+
+fn require_send(_: impl Send) {}
+
+struct Ready<T>(Option<T>);
+impl<T> Future for Ready<T> {
+    type Output = T;
+    fn poll(mut self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<T> {
+        Poll::Ready(self.0.take().unwrap())
+    }
+}
+fn ready<T>(t: T) -> Ready<T> {
+    Ready(Some(t))
+}
+
+fn make_non_send_future1() -> impl Future<Output = Arc<RefCell<i32>>> {
+    ready(Arc::new(RefCell::new(0)))
+}
+
+fn test1() {
+    let send_fut = async {
+        let non_send_fut = make_non_send_future1();
+        let _ = non_send_fut.await;
+        ready(0).await;
+    };
+    require_send(send_fut);
+    //~^ ERROR future cannot be sent between threads
+}
+
+fn test1_no_let() {
+    let send_fut = async {
+        let _ = make_non_send_future1().await;
+        ready(0).await;
+    };
+    require_send(send_fut);
+    //~^ ERROR future cannot be sent between threads
+}
+
+async fn ready2<T>(t: T) -> T { t }
+fn make_non_send_future2() -> impl Future<Output = Arc<RefCell<i32>>> {
+    ready2(Arc::new(RefCell::new(0)))
+}
+
+// Ideally this test would have diagnostics similar to the test above, but right
+// now it doesn't.
+fn test2() {
+    let send_fut = async {
+        let non_send_fut = make_non_send_future2();
+        let _ = non_send_fut.await;
+        ready(0).await;
+    };
+    require_send(send_fut);
+    //~^ ERROR `std::cell::RefCell<i32>` cannot be shared between threads safely
+}
+
+fn main() {}
diff --git a/src/test/ui/async-await/issue-68112.stderr b/src/test/ui/async-await/issue-68112.stderr
@@ -0,0 +1,56 @@
+error: future cannot be sent between threads safely
+  --> $DIR/issue-68112.rs:34:5
+   |
+LL | fn require_send(_: impl Send) {}
+   |                         ---- required by this bound in `require_send`
+...
+LL |     require_send(send_fut);
+   |     ^^^^^^^^^^^^ future created by async block is not `Send`
+   |
+   = help: the trait `std::marker::Sync` is not implemented for `std::cell::RefCell<i32>`
+note: future is not `Send` as it awaits another future which is not `Send`
+  --> $DIR/issue-68112.rs:31:17
+   |
+LL |         let _ = non_send_fut.await;
+   |                 ^^^^^^^^^^^^ await occurs here on type `impl std::future::Future`, which is not `Send`
+
+error: future cannot be sent between threads safely
+  --> $DIR/issue-68112.rs:43:5
+   |
+LL | fn require_send(_: impl Send) {}
+   |                         ---- required by this bound in `require_send`
+...
+LL |     require_send(send_fut);
+   |     ^^^^^^^^^^^^ future created by async block is not `Send`
+   |
+   = help: the trait `std::marker::Sync` is not implemented for `std::cell::RefCell<i32>`
+note: future is not `Send` as it awaits another future which is not `Send`
+  --> $DIR/issue-68112.rs:40:17
+   |
+LL |         let _ = make_non_send_future1().await;
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^ await occurs here on type `impl std::future::Future`, which is not `Send`
+
+error[E0277]: `std::cell::RefCell<i32>` cannot be shared between threads safely
+  --> $DIR/issue-68112.rs:60:5
+   |
+LL | fn require_send(_: impl Send) {}
+   |                         ---- required by this bound in `require_send`
+...
+LL |     require_send(send_fut);
+   |     ^^^^^^^^^^^^ `std::cell::RefCell<i32>` cannot be shared between threads safely
+   |
+   = help: the trait `std::marker::Sync` is not implemented for `std::cell::RefCell<i32>`
+   = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<std::cell::RefCell<i32>>`
+   = note: required because it appears within the type `[static generator@$DIR/issue-68112.rs:47:31: 47:36 t:std::sync::Arc<std::cell::RefCell<i32>> {}]`
+   = note: required because it appears within the type `std::future::from_generator::GenFuture<[static generator@$DIR/issue-68112.rs:47:31: 47:36 t:std::sync::Arc<std::cell::RefCell<i32>> {}]>`
+   = 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 `impl std::future::Future`
+   = note: required because it appears within the type `{std::future::ResumeTy, impl std::future::Future, (), i32, Ready<i32>}`
+   = note: required because it appears within the type `[static generator@$DIR/issue-68112.rs:55:26: 59:6 {std::future::ResumeTy, impl std::future::Future, (), i32, Ready<i32>}]`
+   = note: required because it appears within the type `std::future::from_generator::GenFuture<[static generator@$DIR/issue-68112.rs:55:26: 59:6 {std::future::ResumeTy, impl std::future::Future, (), i32, Ready<i32>}]>`
+   = note: required because it appears within the type `impl std::future::Future`
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0277`.
diff --git a/src/test/ui/async-await/issues/issue-65436-raw-ptr-not-send.stderr b/src/test/ui/async-await/issues/issue-65436-raw-ptr-not-send.stderr
@@ -5,7 +5,7 @@ LL | fn assert_send<T: Send>(_: T) {}
    |                   ---- required by this bound in `assert_send`
 ...
 LL |     assert_send(async {
-   |     ^^^^^^^^^^^ future returned by `main` is not `Send`
+   |     ^^^^^^^^^^^ future created by async block is not `Send`
    |
    = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `*const u8`
 note: future is not `Send` as this value is used across an await
@@ -14,7 +14,7 @@ note: future is not `Send` as this value is used across an await
 LL |         bar(Foo(std::ptr::null())).await;
    |         ^^^^^^^^----------------^^^^^^^^- `std::ptr::null()` is later dropped here
    |         |       |
-   |         |       has type `*const u8`
+   |         |       has type `*const u8` which is not `Send`
    |         await occurs here, with `std::ptr::null()` maybe used later
 help: consider moving this into a `let` binding to create a shorter lived borrow
   --> $DIR/issue-65436-raw-ptr-not-send.rs:14:13
diff --git a/src/test/ui/generator/issue-68112.rs b/src/test/ui/generator/issue-68112.rs
@@ -0,0 +1,56 @@
+#![feature(generators, generator_trait)]
+
+use std::{
+    cell::RefCell,
+    sync::Arc,
+    pin::Pin,
+    ops::{Generator, GeneratorState},
+};
+
+pub struct Ready<T>(Option<T>);
+impl<T> Generator<()> for Ready<T> {
+    type Return = T;
+    type Yield = ();
+    fn resume(mut self: Pin<&mut Self>, _args: ()) -> GeneratorState<(), T> {
+        GeneratorState::Complete(self.0.take().unwrap())
+    }
+}
+pub fn make_gen1<T>(t: T) -> Ready<T> {
+    Ready(Some(t))
+}
+
+fn require_send(_: impl Send) {}
+
+fn make_non_send_generator() -> impl Generator<Return = Arc<RefCell<i32>>> {
+    make_gen1(Arc::new(RefCell::new(0)))
+}
+
+fn test1() {
+    let send_gen = || {
+        let _non_send_gen = make_non_send_generator();
+        yield;
+    };
+    require_send(send_gen);
+    //~^ ERROR generator cannot be sent between threads
+}
+
+pub fn make_gen2<T>(t: T) -> impl Generator<Return = T> {
+    || {
+        yield;
+        t
+    }
+}
+fn make_non_send_generator2() -> impl Generator<Return = Arc<RefCell<i32>>> {
+    make_gen2(Arc::new(RefCell::new(0)))
+}
+
+fn test2() {
+    let send_gen = || {
+        let _non_send_gen = make_non_send_generator2();
+        yield;
+    };
+    require_send(send_gen);
+    //~^ ERROR `std::cell::RefCell<i32>` cannot be shared between threads safely
+}
+
+fn main() {}
diff --git a/src/test/ui/generator/issue-68112.stderr b/src/test/ui/generator/issue-68112.stderr
diff --git a/src/test/ui/generator/not-send-sync.rs b/src/test/ui/generator/not-send-sync.rs
diff --git a/src/test/ui/generator/not-send-sync.stderr b/src/test/ui/generator/not-send-sync.stderr

Original file line number	Diff line number	Diff line change
`@@ -1797,7 +1797,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {`
`1797`	`1797`	// we know that the yield type must be `()`; however, the context won't contain this
`1798`	`1798`	`// information. Hence, we check the source of the yield expression here and check its`
`1799`	`1799`	// value's type against `()` (this check should always hold).
`1800`		`- None if src == &hir::YieldSource::Await => {`
	`1800`	`+ None if src.is_await() => {`
`1801`	`1801`	`self.check_expr_coercable_to_type(&value, self.tcx.mk_unit());`
`1802`	`1802`	`self.tcx.mk_unit()`
`1803`	`1803`	`}`
Original file line number	Diff line number	Diff line change
@@ -12,7 +12,7 @@ note: future is not `Sync` as this value is used across an await
`12`	`12`	`--> $DIR/issue-64130-1-sync.rs:15:5`
`13`	`13`	`\|`
`14`	`14`	`LL \| let x = Foo;`
`15`		- \| - has type `Foo`
	`15`	+ \| - has type `Foo` which is not `Sync`
`16`	`16`	`LL \| baz().await;`
`17`	`17`	\| ^^^^^^^^^^^ await occurs here, with `x` maybe used later
`18`	`18`	`LL \| }`
Original file line number	Diff line number	Diff line change
@@ -16,7 +16,7 @@ note: future does not implement `Qux` as this value is used across an await
`16`	`16`	`--> $DIR/issue-64130-3-other.rs:18:5`
`17`	`17`	`\|`
`18`	`18`	`LL \| let x = Foo;`
`19`		- \| - has type `Foo`
	`19`	+ \| - has type `Foo` which does not implement `Qux`
`20`	`20`	`LL \| baz().await;`
`21`	`21`	\| ^^^^^^^^^^^ await occurs here, with `x` maybe used later
`22`	`22`	`LL \| }`