Skip to content

Commit

Permalink
Rollup merge of rust-lang#94559 - m-ou-se:thread-scope-spawn-closure-…
Browse files Browse the repository at this point in the history
…without-arg, r=Mark-Simulacrum

Remove argument from closure in thread::Scope::spawn.

This implements ```@danielhenrymantilla's``` [suggestion](rust-lang#93203 (comment)) for improving the scoped threads interface.

Summary:

The `Scope` type gets an extra lifetime argument, which represents basically its own lifetime that will be used in `&'scope Scope<'scope, 'env>`:

```diff
- pub struct Scope<'env> { .. };
+ pub struct Scope<'scope, 'env: 'scope> { .. }

  pub fn scope<'env, F, T>(f: F) -> T
  where
-     F: FnOnce(&Scope<'env>) -> T;
+     F: for<'scope> FnOnce(&'scope Scope<'scope, 'env>) -> T;
```

This simplifies the `spawn` function, which now no longer passes an argument to the closure you give it, and now uses the `'scope` lifetime for everything:

```diff
-     pub fn spawn<'scope, F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T>
+     pub fn spawn<F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T>
      where
-         F: FnOnce(&Scope<'env>) -> T + Send + 'env,
+         F: FnOnce() -> T + Send + 'scope,
-         T: Send + 'env;
+         T: Send + 'scope;
```

The only difference the user will notice, is that their closure now takes no arguments anymore, even when spawning threads from spawned threads:

```diff
  thread::scope(|s| {
-     s.spawn(|_| {
+     s.spawn(|| {
          ...
      });
-     s.spawn(|s| {
+     s.spawn(|| {
          ...
-         s.spawn(|_| ...);
+         s.spawn(|| ...);
      });
  });
```

<details><summary>And, as a bonus, errors get <em>slightly</em> better because now any lifetime issues point to the outermost <code>s</code> (since there is only one <code>s</code>), rather than the innermost <code>s</code>, making it clear that the lifetime lasts for the entire <code>thread::scope</code>.

</summary>

```diff
  error[E0373]: closure may outlive the current function, but it borrows `a`, which is owned by the current function
   --> src/main.rs:9:21
    |
- 7 |         s.spawn(|s| {
-   |                  - has type `&Scope<'1>`
+ 6 |     thread::scope(|s| {
+   |                    - lifetime `'1` appears in the type of `s`
  9 |             s.spawn(|| println!("{:?}", a)); // might run after `a` is dropped
    |                     ^^                  - `a` is borrowed here
    |                     |
    |                     may outlive borrowed value `a`
    |
  note: function requires argument type to outlive `'1`
   --> src/main.rs:9:13
    |
  9 |             s.spawn(|| println!("{:?}", a)); // might run after `a` is dropped
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  help: to force the closure to take ownership of `a` (and any other referenced variables), use the `move` keyword
    |
  9 |             s.spawn(move || println!("{:?}", a)); // might run after `a` is dropped
    |                     ++++
"
```
</details>

The downside is that the signature of `scope` and `Scope` gets slightly more complex, but in most cases the user wouldn't need to write those, as they just use the argument provided by `thread::scope` without having to name its type.

Another downside is that this does not work nicely in Rust 2015 and Rust 2018, since in those editions, `s` would be captured by reference and not by copy. In those editions, the user would need to use `move ||` to capture `s` by copy. (Which is what the compiler suggests in the error.)
  • Loading branch information
matthiaskrgr authored Mar 8, 2022
2 parents e22331c + a3d269e commit aec535f
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 24 deletions.
6 changes: 3 additions & 3 deletions library/core/src/sync/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ impl AtomicBool {
/// let a = &*AtomicBool::from_mut_slice(&mut some_bools);
/// std::thread::scope(|s| {
/// for i in 0..a.len() {
/// s.spawn(move |_| a[i].store(true, Ordering::Relaxed));
/// s.spawn(move || a[i].store(true, Ordering::Relaxed));
/// }
/// });
/// assert_eq!(some_bools, [true; 10]);
Expand Down Expand Up @@ -984,7 +984,7 @@ impl<T> AtomicPtr<T> {
/// let a = &*AtomicPtr::from_mut_slice(&mut some_ptrs);
/// std::thread::scope(|s| {
/// for i in 0..a.len() {
/// s.spawn(move |_| {
/// s.spawn(move || {
/// let name = Box::new(format!("thread{i}"));
/// a[i].store(Box::into_raw(name), Ordering::Relaxed);
/// });
Expand Down Expand Up @@ -1533,7 +1533,7 @@ macro_rules! atomic_int {
#[doc = concat!("let a = &*", stringify!($atomic_type), "::from_mut_slice(&mut some_ints);")]
/// std::thread::scope(|s| {
/// for i in 0..a.len() {
/// s.spawn(move |_| a[i].store(i as _, Ordering::Relaxed));
/// s.spawn(move || a[i].store(i as _, Ordering::Relaxed));
/// }
/// });
/// for (i, n) in some_ints.into_iter().enumerate() {
Expand Down
44 changes: 23 additions & 21 deletions library/std/src/thread/scoped.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,24 @@ use crate::sync::Arc;
/// A scope to spawn scoped threads in.
///
/// See [`scope`] for details.
pub struct Scope<'env> {
pub struct Scope<'scope, 'env: 'scope> {
data: ScopeData,
/// Invariance over 'env, to make sure 'env cannot shrink,
/// Invariance over 'scope, to make sure 'scope cannot shrink,
/// which is necessary for soundness.
///
/// Without invariance, this would compile fine but be unsound:
///
/// ```compile_fail
/// ```compile_fail,E0373
/// #![feature(scoped_threads)]
///
/// std::thread::scope(|s| {
/// s.spawn(|s| {
/// s.spawn(|| {
/// let a = String::from("abcd");
/// s.spawn(|_| println!("{:?}", a)); // might run after `a` is dropped
/// s.spawn(|| println!("{:?}", a)); // might run after `a` is dropped
/// });
/// });
/// ```
scope: PhantomData<&'scope mut &'scope ()>,
env: PhantomData<&'env mut &'env ()>,
}

Expand Down Expand Up @@ -88,12 +89,12 @@ impl ScopeData {
/// let mut x = 0;
///
/// thread::scope(|s| {
/// s.spawn(|_| {
/// s.spawn(|| {
/// println!("hello from the first scoped thread");
/// // We can borrow `a` here.
/// dbg!(&a);
/// });
/// s.spawn(|_| {
/// s.spawn(|| {
/// println!("hello from the second scoped thread");
/// // We can even mutably borrow `x` here,
/// // because no other threads are using it.
Expand All @@ -109,7 +110,7 @@ impl ScopeData {
#[track_caller]
pub fn scope<'env, F, T>(f: F) -> T
where
F: FnOnce(&Scope<'env>) -> T,
F: for<'scope> FnOnce(&'scope Scope<'scope, 'env>) -> T,
{
let scope = Scope {
data: ScopeData {
Expand All @@ -118,6 +119,7 @@ where
a_thread_panicked: AtomicBool::new(false),
},
env: PhantomData,
scope: PhantomData,
};

// Run `f`, but catch panics so we can make sure to wait for all the threads to join.
Expand All @@ -138,7 +140,7 @@ where
}
}

impl<'env> Scope<'env> {
impl<'scope, 'env> Scope<'scope, 'env> {
/// Spawns a new thread within a scope, returning a [`ScopedJoinHandle`] for it.
///
/// Unlike non-scoped threads, threads spawned with this function may
Expand All @@ -163,10 +165,10 @@ impl<'env> Scope<'env> {
/// to recover from such errors.
///
/// [`join`]: ScopedJoinHandle::join
pub fn spawn<'scope, F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T>
pub fn spawn<F, T>(&'scope self, f: F) -> ScopedJoinHandle<'scope, T>
where
F: FnOnce(&Scope<'env>) -> T + Send + 'env,
T: Send + 'env,
F: FnOnce() -> T + Send + 'scope,
T: Send + 'scope,
{
Builder::new().spawn_scoped(self, f).expect("failed to spawn thread")
}
Expand Down Expand Up @@ -196,7 +198,7 @@ impl Builder {
/// thread::scope(|s| {
/// thread::Builder::new()
/// .name("first".to_string())
/// .spawn_scoped(s, |_|
/// .spawn_scoped(s, ||
/// {
/// println!("hello from the {:?} scoped thread", thread::current().name());
/// // We can borrow `a` here.
Expand All @@ -205,7 +207,7 @@ impl Builder {
/// .unwrap();
/// thread::Builder::new()
/// .name("second".to_string())
/// .spawn_scoped(s, |_|
/// .spawn_scoped(s, ||
/// {
/// println!("hello from the {:?} scoped thread", thread::current().name());
/// // We can even mutably borrow `x` here,
Expand All @@ -222,14 +224,14 @@ impl Builder {
/// ```
pub fn spawn_scoped<'scope, 'env, F, T>(
self,
scope: &'scope Scope<'env>,
scope: &'scope Scope<'scope, 'env>,
f: F,
) -> io::Result<ScopedJoinHandle<'scope, T>>
where
F: FnOnce(&Scope<'env>) -> T + Send + 'env,
T: Send + 'env,
F: FnOnce() -> T + Send + 'scope,
T: Send + 'scope,
{
Ok(ScopedJoinHandle(unsafe { self.spawn_unchecked_(|| f(scope), Some(&scope.data)) }?))
Ok(ScopedJoinHandle(unsafe { self.spawn_unchecked_(f, Some(&scope.data)) }?))
}
}

Expand All @@ -244,7 +246,7 @@ impl<'scope, T> ScopedJoinHandle<'scope, T> {
/// use std::thread;
///
/// thread::scope(|s| {
/// let t = s.spawn(|_| {
/// let t = s.spawn(|| {
/// println!("hello");
/// });
/// println!("thread id: {:?}", t.thread().id());
Expand Down Expand Up @@ -277,7 +279,7 @@ impl<'scope, T> ScopedJoinHandle<'scope, T> {
/// use std::thread;
///
/// thread::scope(|s| {
/// let t = s.spawn(|_| {
/// let t = s.spawn(|| {
/// panic!("oh no");
/// });
/// assert!(t.join().is_err());
Expand All @@ -302,7 +304,7 @@ impl<'scope, T> ScopedJoinHandle<'scope, T> {
}
}

impl<'env> fmt::Debug for Scope<'env> {
impl fmt::Debug for Scope<'_, '_> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Scope")
.field("num_running_threads", &self.data.num_running_threads.load(Ordering::Relaxed))
Expand Down

0 comments on commit aec535f

Please sign in to comment.