From 1911c5658b3740fea254db2c9d49b602fa3cbeba Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Sat, 3 Jun 2023 23:41:39 +0000 Subject: [PATCH] Revert "Merge #233" This reverts commit e3b226069dbcea52b94a1ed84e9361018e9cdbbd, reversing changes made to 8f39b775effd387b175993b0091b082c4d60f921. --- src/lib.rs | 133 +++++++++------------------------------------------- tests/it.rs | 64 ------------------------- 2 files changed, 23 insertions(+), 174 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 22016c0..4c2d0e4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -716,52 +716,15 @@ pub mod unsync { /// // 92 /// ``` pub struct Lazy T> { - state: Cell, - data: Data, - } - - /// Discriminant of the union. - #[derive(Debug, Clone, Copy)] - enum State { - /// `.init` has not been taken yet. - Uninit, - /// `.init` has been *taken*, but the call has not completed (panic?), so there is no `.value` either. - Poisoned, - /// `.value` has been set. - Value, - } - - // One of the rare cases `#[repr(C)]` is not needed for an union ("externally tagged `enum`" pattern) - union Data { - /// Idea: we don't need *mutation* to dispose of `F`, we can just let it be there, inert, behind a MD. - /// - /// This makes it so we remain covariant in `F`. - init: mem::ManuallyDrop, - value: mem::ManuallyDrop>, - } - - impl Drop for Lazy { - fn drop(&mut self) { - match self.state.get_mut() { - State::Value => unsafe { mem::ManuallyDrop::drop(&mut self.data.value) }, - State::Uninit => unsafe { mem::ManuallyDrop::drop(&mut self.data.init) }, - State::Poisoned => {} - } - } + cell: OnceCell, + init: Cell>, } impl RefUnwindSafe for Lazy where OnceCell: RefUnwindSafe {} impl fmt::Debug for Lazy { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self.state.get() { - // `Lazy("some value")` - State::Value => { - f.debug_tuple("Lazy").field(unsafe { &*self.data.value.get() }).finish() - } - // `Uninit`, or `Poisoned` - state => state.fmt(f), - } + f.debug_struct("Lazy").field("cell", &self.cell).field("init", &"..").finish() } } @@ -781,28 +744,18 @@ pub mod unsync { /// # } /// ``` pub const fn new(init: F) -> Lazy { - Lazy { - state: Cell::new(State::Uninit), - data: Data { init: mem::ManuallyDrop::new(init) }, - } + Lazy { cell: OnceCell::new(), init: Cell::new(Some(init)) } } /// Consumes this `Lazy` returning the stored value. /// /// Returns `Ok(value)` if `Lazy` is initialized and `Err(f)` otherwise. pub fn into_value(this: Lazy) -> Result { - // Safety: beyond the typical "check discriminant before accessing corresponding union variant", - // we have to be careful not to run `this`' `impl Drop` glue as we extract its fields, - // hence the initial extra layer of `ManuallyDrop`. - // Once that's done nothing is owned anymore, so we are free to `ManuallyDrop::take` what we want. - let mut this = mem::ManuallyDrop::new(this); - match this.state.get_mut() { - State::Value => { - Ok(unsafe { mem::ManuallyDrop::take(&mut this.data.value) }.into_inner()) - } - State::Uninit => Err(unsafe { mem::ManuallyDrop::take(&mut this.data.init) }), - State::Poisoned => panic!("Lazy instance has previously been poisoned"), - } + let cell = this.cell; + let init = this.init; + cell.into_inner().ok_or_else(|| { + init.take().unwrap_or_else(|| panic!("Lazy instance has previously been poisoned")) + }) } } @@ -822,37 +775,10 @@ pub mod unsync { /// assert_eq!(&*lazy, &92); /// ``` pub fn force(this: &Lazy) -> &T { - match this.state.get() { - State::Value => unsafe { &*this.data.value.get() }, - State::Uninit => { - // Safety: `<*const _>::read(&*...)` is the by-ref counterpart of `ManuallyDrop::take`. - // We are allowed to `take` the `F` once we have taken its `State::Uninit` discriminant. - // We cannot put `State::Value` in its stead yet in case `init()` panics. - // Hence the `Poisoned` / "empty `Lazy`" state. - let init = unsafe { - this.state.set(State::Poisoned); - <*const F>::read(&*this.data.init) - }; - let value = init(); - unsafe { - // Safety: here we need to tread with caution, since we need to "pick a union variant" - // without accidentally asserting that the `ManuallyDrop<... T ...>` is init since it still isn't. - // Hence `ptr::addr_of!`. - let ptr: *const mem::ManuallyDrop> = - core::ptr::addr_of!(this.data.value); - // There is no raw accessor on `ManuallyDrop`, but it's a `transparent` wrapper so we can - // just `.cast()` that layer away. - let ptr: *const UnsafeCell = ptr.cast(); - // To handle the `UnsafeCell` layer, no need for casts, there is a dedicated API. - let ptr: *mut T = UnsafeCell::raw_get(ptr); - // Once we've gotten the pointer, the rest is easy: set the value and discriminants accordingly. - ptr.write(value); - this.state.set(State::Value); - &*ptr - } - } - State::Poisoned => panic!("Lazy instance has previously been poisoned"), - } + this.cell.get_or_init(|| match this.init.take() { + Some(f) => f(), + None => panic!("Lazy instance has previously been poisoned"), + }) } /// Forces the evaluation of this lazy value and returns a mutable reference to @@ -870,25 +796,14 @@ pub mod unsync { /// assert_eq!(*lazy, 92); /// ``` pub fn force_mut(this: &mut Lazy) -> &mut T { - let state = this.state.get_mut(); - match state { - State::Value => unsafe { (*this.data.value).get_mut() }, - State::Uninit => { - // Safety: same reasoning as with `force`, except simpler since we can use `&mut` accesses. - // (so we do not need to worry about any `UnsafeCell` shenanigans) - let init = unsafe { - *state = State::Poisoned; - mem::ManuallyDrop::take(&mut this.data.init) - }; - let value = mem::ManuallyDrop::new(init().into()); - unsafe { - core::ptr::addr_of_mut!(this.data.value).write(value); - *state = State::Value; - (*this.data.value).get_mut() - } - } - State::Poisoned => panic!("Lazy instance has previously been poisoned"), + if this.cell.get_mut().is_none() { + let value = match this.init.get_mut().take() { + Some(f) => f(), + None => panic!("Lazy instance has previously been poisoned"), + }; + this.cell = OnceCell::with_value(value); } + this.cell.get_mut().unwrap_or_else(|| unreachable!()) } /// Gets the reference to the result of this lazy value if @@ -905,7 +820,7 @@ pub mod unsync { /// assert_eq!(Lazy::get(&lazy), Some(&92)); /// ``` pub fn get(this: &Lazy) -> Option<&T> { - matches!(this.state.get(), State::Value).then(|| unsafe { &*this.data.value.get() }) + this.cell.get() } /// Gets the mutable reference to the result of this lazy value if @@ -922,8 +837,7 @@ pub mod unsync { /// assert_eq!(Lazy::get_mut(&mut lazy), Some(&mut 92)); /// ``` pub fn get_mut(this: &mut Lazy) -> Option<&mut T> { - matches!(this.state.get_mut(), State::Value) - .then(|| unsafe { (*this.data.value).get_mut() }) + this.cell.get_mut() } } @@ -1376,8 +1290,7 @@ pub mod sync { let cell = this.cell; let init = this.init; cell.into_inner().ok_or_else(|| { - init.into_inner() - .unwrap_or_else(|| panic!("Lazy instance has previously been poisoned")) + init.take().unwrap_or_else(|| panic!("Lazy instance has previously been poisoned")) }) } } diff --git a/tests/it.rs b/tests/it.rs index 37cbd0a..d18f0a1 100644 --- a/tests/it.rs +++ b/tests/it.rs @@ -1,53 +1,3 @@ -/// Put here any code relying on duck-typed `Lazy` and `OnceCell`, oblivious to -/// their exact `sync` or `unsync` nature. -macro_rules! tests_for_both { - () => { - #[test] - fn lazy_does_drop() { - type Counter = std::rc::Rc<()>; - - let (counter, [c1, c2, c3]) = { - let c = Counter::new(()); - (Counter::downgrade(&c), [(); 3].map(|()| c.clone())) - }; - assert_eq!(counter.strong_count(), 3); - - let lazy_1 = Lazy::::new(|| c1); - assert_eq!(counter.strong_count(), 3); - drop(lazy_1); - assert_eq!( - counter.strong_count(), - 2, - "dropping a `Lazy::Uninit` drops the `init` closure" - ); - - let lazy_2 = Lazy::::new(|| c2); - Lazy::force(&lazy_2); - assert_eq!( - counter.strong_count(), - 2, - "from `Lazy::Uninit` to `Lazy::Value` drops `init` but owns `value`" - ); - drop(lazy_2); - assert_eq!(counter.strong_count(), 1, "dropping a `Lazy::Value` drops its `value`"); - - let lazy_3 = Lazy::::new(|| { - None::<()>.unwrap(); - c3 - }); - assert_eq!(counter.strong_count(), 1); - let _ = std::panic::catch_unwind(|| Lazy::force(&lazy_3)).expect_err("it panicked"); - assert_eq!( - counter.strong_count(), - 0, - "`init` closure is properly dropped despite panicking" - ); - drop(lazy_3); - assert_eq!(counter.strong_count(), 0, "what is dead may never die 🧟"); - } - }; -} - mod unsync { use core::{ cell::Cell, @@ -56,8 +6,6 @@ mod unsync { use once_cell::unsync::{Lazy, OnceCell}; - tests_for_both!(); - #[test] fn once_cell() { let c = OnceCell::new(); @@ -299,16 +247,6 @@ mod unsync { cell.set(&s).unwrap(); } } - - #[test] - fn assert_lazy_is_covariant_in_the_ctor() { - #[allow(dead_code)] - type AnyLazy<'f, T> = Lazy T>>; - - fn _for<'short, T>(it: *const (AnyLazy<'static, T>,)) -> *const (AnyLazy<'short, T>,) { - it - } - } } #[cfg(any(feature = "std", feature = "critical-section"))] @@ -325,8 +263,6 @@ mod sync { use once_cell::sync::{Lazy, OnceCell}; - tests_for_both!(); - #[test] fn once_cell() { let c = OnceCell::new();