Skip to content

Commit

Permalink
Implement the RFC3498 Captures Trick for lifetimes.
Browse files Browse the repository at this point in the history
I finally took the time to squint at this RFC, understand the problem,
and then apply the solution here. What I had before was wrong in subtle
ways that had caused me to overcomplicate some things in the past -- it
would tend to put weird constraints on generic types or lifetimes that
would lead to weird compile errors.

Fortunately, it was "wrong" in the sense that valid programs wouldn't
compile, not in the sense that it allowed bugs.

This commit adds a Captures trait to util and uses it to do lifetime and
parameter capture in the best way available in the 2021 edition.
  • Loading branch information
cbiffle committed Mar 6, 2024
1 parent 8516f43 commit 48028c7
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 20 deletions.
14 changes: 6 additions & 8 deletions os/src/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ use core::task::{Context, Poll, RawWaker, RawWakerVTable, Waker};
use pin_project_lite::pin_project;

use crate::atomic::{AtomicExt, AtomicArithExt};
use crate::util::Captures;

// Despite the untangling of exec and time that happened in the 1.0 release, we
// still have some intimate dependencies between the modules. You'll see a few
Expand Down Expand Up @@ -744,13 +745,12 @@ impl Notify {
/// improve cancel safety by avoiding the move, if possible.
/// 2. Passing a closure you received as `cond` instead of making a new one.
/// In this case, consider passing the closure by reference.
pub fn until<'a, 'b, F, T: TestResult>(
pub fn until<'a, F, T: TestResult>(
&'a self,
cond: F,
) -> Until<'a, F>
where
'b: 'a,
F: FnMut() -> T + 'b,
F: FnMut() -> T,
{
Until {
cond,
Expand Down Expand Up @@ -789,13 +789,12 @@ impl Notify {
/// improve cancel safety by avoiding the move, if possible.
/// 2. Passing a closure you received as `cond` instead of making a new one.
/// In this case, consider passing the closure by reference.
pub fn until_racy<'a, 'b, F, T: TestResult>(
pub fn until_racy<'a, F, T: TestResult>(
&'a self,
cond: F,
) -> UntilRacy<'a, F>
where
'b: 'a,
F: FnMut() -> T + 'b,
F: FnMut() -> T,
{
UntilRacy {
cond,
Expand All @@ -815,8 +814,7 @@ impl Notify {
/// (meaning one potential spurious wakeup in the future is possible).
pub fn until_next(
&self,
) -> impl Future<Output = ()> + '_
{
) -> impl Future<Output = ()> + Captures<&'_ Self> {
let mut setup = false;
self.until(move || core::mem::replace(&mut setup, true))
}
Expand Down
20 changes: 8 additions & 12 deletions os/src/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ use core::mem::ManuallyDrop;
use core::pin::Pin;
use core::ptr::NonNull;
use core::task::{Poll, RawWaker, RawWakerVTable, Waker};
use crate::util::NotSendMarker;
use crate::util::{NotSendMarker, Captures};

/// A cell specialized for holding `Waker`s. This is functionally equivalent to
/// `Cell` except that it will allow one operation to be performed on its
Expand Down Expand Up @@ -426,9 +426,7 @@ impl<T: PartialOrd> List<T> {
pub fn insert_and_wait<'list, 'node>(
self: Pin<&'list Self>,
node: Pin<&'node mut Node<T>>,
) -> impl Future<Output = ()> + 'node
where 'list: 'node
{
) -> impl Future<Output = ()> + Captures<(&'list Self, &'node mut Node<T>)> {
self.insert_and_wait_with_cleanup(
node,
|| (),
Expand Down Expand Up @@ -478,9 +476,7 @@ impl<T: PartialOrd> List<T> {
self: Pin<&'list Self>,
node: Pin<&'node mut Node<T>>,
cleanup: F,
) -> impl Future<Output = ()> + 'node
where 'list: 'node
{
) -> impl Future<Output = ()> + Captures<(&'list Self, &'node mut Node<T>)> {
// We required `node` to be `mut` to prove exclusive ownership, but we
// don't actually need to mutate it -- and we're going to alias it. So,
// downgrade.
Expand Down Expand Up @@ -579,9 +575,9 @@ impl<T: core::fmt::Debug> core::fmt::Debug for List<T> {
}
/// Internal future type used for `insert_and_wait`. Gotta express this as a
/// named type because it needs a custom `Drop` impl.
struct WaitForDetach<'a, T, F: FnOnce()> {
node: Pin<&'a Node<T>>,
list: Pin<&'a List<T>>,
struct WaitForDetach<'node, 'list, T, F: FnOnce()> {
node: Pin<&'node Node<T>>,
list: Pin<&'list List<T>>,
state: Cell<WaitState>,
cleanup: Option<F>,
}
Expand All @@ -593,7 +589,7 @@ enum WaitState {
DetachedAndPolled,
}

impl<T: PartialOrd, F: FnOnce()> Future for WaitForDetach<'_, T, F> {
impl<T: PartialOrd, F: FnOnce()> Future for WaitForDetach<'_, '_, T, F> {
type Output = ();

fn poll(self: Pin<&mut Self>, cx: &mut core::task::Context<'_>)
Expand Down Expand Up @@ -667,7 +663,7 @@ impl<T: PartialOrd, F: FnOnce()> Future for WaitForDetach<'_, T, F> {
}
}

impl<T, F: FnOnce()> Drop for WaitForDetach<'_, T, F> {
impl<T, F: FnOnce()> Drop for WaitForDetach<'_, '_, T, F> {
fn drop(&mut self) {
match self.state.get() {
WaitState::NotYetAttached => {
Expand Down
9 changes: 9 additions & 0 deletions os/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ pub struct NotSyncMarker(PhantomData<core::cell::Cell<()>>);
#[derive(Default, Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd)]
pub struct NotSendMarker(PhantomData<*const ()>);

/// Marker trait implementing the "Captures Trick" from Rust RFC 3498, ensuring
/// that we do lifetime capturing right in the 2021 edition.
///
/// TODO: revisit this when we can switch to the 2024 edition, where the default
/// behavior makes this less necessary.
pub trait Captures<T> {}

impl<U: ?Sized, T> Captures<T> for U {}

/// Extension trait for `Future` that adds common utility operations.
///
/// This is intended to complement the `futures` crate and reduce the number of
Expand Down

0 comments on commit 48028c7

Please sign in to comment.