From 3410144bb53dbdd7df5ce1903f5ebee5d4e44edf Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 8 Nov 2022 17:20:30 -0800 Subject: [PATCH 1/4] Add IntoStackFutureError to provide more detail about conversion failures --- src/lib.rs | 139 +++++++++++++++++++++++++++++++++++++++------------ src/tests.rs | 15 ++++-- 2 files changed, 120 insertions(+), 34 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index e3d77d0..f9c1663 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,7 +11,10 @@ #![cfg_attr(not(test), no_std)] #![warn(missing_docs)] +use core::fmt::Debug; +use core::fmt::Display; use core::future::Future; +use core::future::IntoFuture; use core::marker::PhantomData; use core::mem; use core::mem::MaybeUninit; @@ -147,34 +150,7 @@ impl<'a, T, const STACK_SIZE: usize> StackFuture<'a, T, { STACK_SIZE }> { #[allow(clippy::let_unit_value)] let _ = AssertFits::::ASSERT; - // Since we have the static assert above, we know this will not fail. This means we could - // use `unwrap_unchecked` here. The extra `unsafe` code probably isn't worth it since very - // likely the compiler will be able to make that optimization for us, this comment is here - // as a reminder that we can change it if for some reason this ends up being a performance - // issue in the future. - Self::try_from(future).unwrap_or_else(|f| { - match (Self::has_alignment_for_val(&f), Self::has_space_for_val(&f)) { - (false, false) => panic!( - "cannot create StackFuture, required size is {}, available space is {}; required alignment is {} but maximum alignment is {}", - mem::size_of_val(&f), - STACK_SIZE, - mem::align_of::(), - mem::align_of::() - ), - (false, true) => panic!( - "cannot create StackFuture, required alignment is {} but maximum alignment is {}", - mem::align_of::(), - mem::align_of::() - ), - (true, false) => panic!( - "cannot create StackFuture, required size is {}, available space is {}", - mem::size_of_val(&f), - STACK_SIZE - ), - // If we have space and alignment, then `try_from` would have succeeded - (true, true) => unreachable!(), - } - }) + Self::try_from(future).unwrap() } /// Attempts to create a `StackFuture` from an existing future @@ -185,7 +161,7 @@ impl<'a, T, const STACK_SIZE: usize> StackFuture<'a, T, { STACK_SIZE }> { /// Panics /// /// If we cannot satisfy the alignment requirements for `F`, this function will panic. - pub fn try_from(future: F) -> Result + pub fn try_from(future: F) -> Result> where F: Future + Send + 'a, // the bounds here should match those in the _phantom field { @@ -212,7 +188,7 @@ impl<'a, T, const STACK_SIZE: usize> StackFuture<'a, T, { STACK_SIZE }> { Ok(result) } else { - Err(future) + Err(IntoStackFutureError::new::(future)) } } @@ -231,7 +207,7 @@ impl<'a, T, const STACK_SIZE: usize> StackFuture<'a, T, { STACK_SIZE }> { where F: Future + Send + 'a, // the bounds here should match those in the _phantom field { - Self::try_from(future).unwrap_or_else(|future| Self::from(Box::pin(future))) + Self::try_from(future).unwrap_or_else(|future| Self::from(Box::pin(future.into_future()))) } /// A wrapper around the inner future's poll function, which we store in the poll_fn field @@ -338,5 +314,106 @@ impl AssertFits { }; } +/// Captures information about why a future could not be converted into a [`StackFuture`] +/// +/// It also contains the original future so that callers can still run the future in error +/// recovery paths, such as by boxing the future instead of wrapping it in [`StackFuture`]. +pub struct IntoStackFutureError { + /// The size of the StackFuture we tried to convert the future into + maximum_size: usize, + /// The StackFuture's alignment + maximum_alignment: usize, + /// The future that was attempted to be wrapped + future: F, +} + +impl IntoStackFutureError { + fn new(future: F) -> Self { + Self { + maximum_size: mem::size_of::(), + maximum_alignment: mem::align_of::(), + future, + } + } + + /// Returns true if the target [`StackFuture`] was too small to hold the given future. + pub fn insufficient_space(&self) -> bool { + self.maximum_size < mem::size_of_val(&self.future) + } + + /// Returns true if the target [`StackFuture`]'s alignment was too small to accommodate the given future. + pub fn alignment_too_small(&self) -> bool { + self.maximum_alignment < mem::align_of_val(&self.future) + } + + /// Returns the alignment of the wrapped future. + pub fn required_alignment(&self) -> usize { + mem::align_of_val(&self.future) + } + + /// Returns the size of the wrapped future. + pub fn required_space(&self) -> usize { + mem::size_of_val(&self.future) + } + + /// Returns the alignment of the target [`StackFuture`], which is also the maximum alignment + /// that can be wrapped. + pub const fn available_alignment(&self) -> usize { + self.maximum_alignment + } + + /// Returns the amount of space that was available in the target [`StackFuture`]. + pub const fn available_space(&self) -> usize { + self.maximum_size + } +} + +impl IntoFuture for IntoStackFutureError { + type Output = F::Output; + + type IntoFuture = F; + + /// Returns the underlying future that caused this error + /// + /// Can be used to try again, either by directly awaiting the future, wrapping it in a `Box`, + /// or some other method. + fn into_future(self) -> Self::IntoFuture { + self.future + } +} + +impl Display for IntoStackFutureError { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match (self.alignment_too_small(), self.insufficient_space()) { + (true, true) => write!(f, + "cannot create StackFuture, required size is {}, available space is {}; required alignment is {} but maximum alignment is {}", + self.required_space(), + self.available_space(), + self.required_alignment(), + self.available_alignment() + ), + (true, false) => write!(f, + "cannot create StackFuture, required alignment is {} but maximum alignment is {}", + self.required_alignment(), + self.available_alignment() + ), + (false, true) => write!(f, + "cannot create StackFuture, required size is {}, available space is {}", + self.required_space(), + self.available_space() + ), + // If we have space and alignment, then `try_from` would have succeeded + (false, false) => unreachable!(), + } + } +} + +impl Debug for IntoStackFutureError { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + // Forward to the Display implementation + write!(f, "{self}") + } +} + #[cfg(test)] mod tests; diff --git a/src/tests.rs b/src/tests.rs index af5bfd0..4622382 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -7,6 +7,7 @@ use futures::Future; use futures::SinkExt; use futures::Stream; use futures::StreamExt; +use std::future::IntoFuture; use std::sync::Arc; use std::task::Context; use std::task::Wake; @@ -92,7 +93,10 @@ fn test_size_failure() { buf[0] }; - assert!(StackFuture::<_, 4>::try_from(f).is_err()); + match StackFuture::<_, 4>::try_from(f) { + Ok(_) => panic!("conversion to StackFuture should not have succeeded"), + Err(e) => assert!(e.insufficient_space()), + } } #[test] @@ -127,7 +131,10 @@ fn test_alignment_failure() { Poll::Pending } } - assert!(StackFuture::<'_, _, 1016>::try_from(BigAlignment(42)).is_err()); + match StackFuture::<'_, _, 1016>::try_from(BigAlignment(42)) { + Ok(_) => panic!("conversion to StackFuture should not have succeeded"), + Err(e) => assert!(e.alignment_too_small()), + } } #[cfg(feature = "alloc")] @@ -189,7 +196,9 @@ fn try_from() { match StackFuture::<_, 10>::try_from(big_future) { Ok(_) => panic!("try_from should not have succeeded"), - Err(big_future) => assert!(StackFuture::<_, 1500>::try_from(big_future).is_ok()), + Err(big_future) => { + assert!(StackFuture::<_, 1500>::try_from(big_future.into_future()).is_ok()) + } }; } From c3e5a929f97ff3089b08c7b67915b49b40ce9e48 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 16 Nov 2022 15:08:17 -0800 Subject: [PATCH 2/4] Update src/lib.rs Co-authored-by: Daniel Prilik <71350465+daprilik@users.noreply.github.com> --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index f9c1663..40df3d8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -207,7 +207,7 @@ impl<'a, T, const STACK_SIZE: usize> StackFuture<'a, T, { STACK_SIZE }> { where F: Future + Send + 'a, // the bounds here should match those in the _phantom field { - Self::try_from(future).unwrap_or_else(|future| Self::from(Box::pin(future.into_future()))) + Self::try_from(future).unwrap_or_else(|err| Self::from(Box::pin(err.into_future()))) } /// A wrapper around the inner future's poll function, which we store in the poll_fn field From decdb1f2355813540b1ddc47ce6e24de693beaa1 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 16 Nov 2022 15:31:50 -0800 Subject: [PATCH 3/4] into_future -> into_inner --- src/lib.rs | 11 ++--------- src/tests.rs | 3 +-- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 40df3d8..d8aa348 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,7 +14,6 @@ use core::fmt::Debug; use core::fmt::Display; use core::future::Future; -use core::future::IntoFuture; use core::marker::PhantomData; use core::mem; use core::mem::MaybeUninit; @@ -207,7 +206,7 @@ impl<'a, T, const STACK_SIZE: usize> StackFuture<'a, T, { STACK_SIZE }> { where F: Future + Send + 'a, // the bounds here should match those in the _phantom field { - Self::try_from(future).unwrap_or_else(|err| Self::from(Box::pin(err.into_future()))) + Self::try_from(future).unwrap_or_else(|err| Self::from(Box::pin(err.into_inner()))) } /// A wrapper around the inner future's poll function, which we store in the poll_fn field @@ -366,18 +365,12 @@ impl IntoStackFutureError { pub const fn available_space(&self) -> usize { self.maximum_size } -} - -impl IntoFuture for IntoStackFutureError { - type Output = F::Output; - - type IntoFuture = F; /// Returns the underlying future that caused this error /// /// Can be used to try again, either by directly awaiting the future, wrapping it in a `Box`, /// or some other method. - fn into_future(self) -> Self::IntoFuture { + fn into_inner(self) -> F { self.future } } diff --git a/src/tests.rs b/src/tests.rs index 4622382..56b01e2 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -7,7 +7,6 @@ use futures::Future; use futures::SinkExt; use futures::Stream; use futures::StreamExt; -use std::future::IntoFuture; use std::sync::Arc; use std::task::Context; use std::task::Wake; @@ -197,7 +196,7 @@ fn try_from() { match StackFuture::<_, 10>::try_from(big_future) { Ok(_) => panic!("try_from should not have succeeded"), Err(big_future) => { - assert!(StackFuture::<_, 1500>::try_from(big_future.into_future()).is_ok()) + assert!(StackFuture::<_, 1500>::try_from(big_future.into_inner()).is_ok()) } }; } From d9ae39d4918a990591352fcb20b14f06ea85abec Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 16 Nov 2022 15:34:38 -0800 Subject: [PATCH 4/4] Use a more traditional Debug impl for IntoStackFutureError --- src/lib.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index d8aa348..12d8173 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -403,8 +403,11 @@ impl Display for IntoStackFutureError { impl Debug for IntoStackFutureError { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - // Forward to the Display implementation - write!(f, "{self}") + f.debug_struct("IntoStackFutureError") + .field("maximum_size", &self.maximum_size) + .field("maximum_alignment", &self.maximum_alignment) + .field("future", &core::any::type_name::()) + .finish() } }