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

Rollup of 4 pull requests #94717

Closed
wants to merge 11 commits into from

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

clarfonthey and others added 11 commits January 29, 2022 18:37
Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>
Co-authored-by: Mark Rousskov <mark.simulacrum@gmail.com>
Add Result::{ok, err, and, or, unwrap_or} as const

Already opened tracking issue rust-lang#92384.

I don't think that this should actually cause any issues as long as the constness is unstable, but we may want to double-check that this doesn't get interpreted as a weird `Drop` bound even for non-const usages.
…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.)
…sumption, r=Mark-Simulacrum

promot debug_assert to assert

Fixes rust-lang#94705
@rustbot rustbot added the rollup A PR which is a rollup label Mar 8, 2022
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=4

@bors
Copy link
Contributor

bors commented Mar 8, 2022

📌 Commit de31dcf has been approved by matthiaskrgr

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 8, 2022
@bors
Copy link
Contributor

bors commented Mar 8, 2022

⌛ Testing commit de31dcf with merge ca7974eb890beb0933293c9b57be6cc26782a646...

@RalfJung
Copy link
Member

RalfJung commented Mar 8, 2022

@bors r- retry

Miri is broken by some very recently landing changes. Sorry for that.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2022
@Dylan-DPC Dylan-DPC closed this Mar 8, 2022
@RalfJung RalfJung reopened this Mar 8, 2022
@RalfJung
Copy link
Member

RalfJung commented Mar 8, 2022

Lol we clicked that button the same time.^^

@RalfJung RalfJung closed this Mar 8, 2022
@Dylan-DPC
Copy link
Member

creating a new rollup already :P don't race me there xD

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Verifying status of rls...
Verifying status of miri...
This PR updated 'src/tools/miri', verifying if status is 'test-pass'...

We detected that this PR updated 'miri', but its tests failed.

If you do intend to update 'miri', please check the error messages above and
commit another update.

If you do NOT intend to update 'miri', please ensure you did not accidentally
change the submodule at 'src/tools/miri'. You may ask your reviewer for the
Build completed unsuccessfully in 0:00:00

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Verifying status of rls...
Verifying status of miri...
This PR updated 'src/tools/miri', verifying if status is 'test-pass'...

We detected that this PR updated 'miri', but its tests failed.
Build completed unsuccessfully in 0:00:00
Build completed unsuccessfully in 0:00:00
If you do intend to update 'miri', please check the error messages above and
commit another update.

If you do NOT intend to update 'miri', please ensure you did not accidentally
change the submodule at 'src/tools/miri'. You may ask your reviewer for the

@matthiaskrgr matthiaskrgr deleted the rollup-bh1qk3c branch March 11, 2022 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants