diff --git a/core/src/environment.rs b/core/src/environment.rs index e7aebec1ef..d19fba41e0 100644 --- a/core/src/environment.rs +++ b/core/src/environment.rs @@ -100,7 +100,7 @@ impl Environment { /// Creates an iterator that visits all layers from the most recent one to the oldest. /// The element iterator type is `Rc>`. - pub fn iter_layers(&self) -> EnvLayerIter<'_, K, V> { + pub fn iter_layers<'slf>(&'slf self) -> EnvLayerIter<'slf, K, V> { EnvLayerIter { env: if !self.was_cloned() { Some(NonNull::from(self)) diff --git a/core/src/identifier.rs b/core/src/identifier.rs index d3d4caae3b..695f5fdb24 100644 --- a/core/src/identifier.rs +++ b/core/src/identifier.rs @@ -240,9 +240,11 @@ mod interner { /// The interner, which serves a double purpose: it pre-allocates space /// so that [Ident](super::Ident) labels are created faster /// and it makes it so that labels are stored only once, saving space. - pub(crate) struct Interner<'a>(RwLock>); + // NOTE: We set the lifetime parameter of InnerInterner to 'static since + // it's arbitrary, and there's no reason to expose it to users of Interner + pub(crate) struct Interner(RwLock>); - impl<'a> Interner<'a> { + impl Interner { /// Creates an empty [Interner]. pub(crate) fn new() -> Self { Self(RwLock::new(InnerInterner::new())) @@ -258,28 +260,32 @@ mod interner { /// /// This operation cannot fails since the only way to have a [Symbol] is to have /// [interned](Interner::intern) the corresponding string first. - pub(crate) fn lookup(&self, sym: Symbol) -> &str { + pub(crate) fn lookup<'slf>(&'slf self, sym: Symbol) -> &'slf str { // SAFETY: We are making the returned &str lifetime the same as our struct, // which is okay here since the InnerInterner uses a typed_arena which prevents // deallocations, so the reference will be valid while the InnerInterner exists, // hence while the struct exists. - unsafe { std::mem::transmute(self.0.read().unwrap().lookup(sym)) } + unsafe { std::mem::transmute::<&'_ str, &'slf str>(self.0.read().unwrap().lookup(sym)) } } } /// The main part of the Interner. - struct InnerInterner<'a> { + /// 'internal should never be exposed to the outside, as it is an + /// implementation detail and could be set to anything. It should be treated + /// within the implementation of InnerInterner as the lifetime of the object + /// itself, since that's what it is turned into in the `lookup()` method. + struct InnerInterner<'internal> { /// Preallocates space where strings are stored. arena: Mutex>, /// Prevents the arena from creating different [Symbols](Symbol) for the same string. - map: HashMap<&'a str, Symbol>, + map: HashMap<&'internal str, Symbol>, /// Allows retrieving a string from a [Symbol]. - vec: Vec<&'a str>, + vec: Vec<&'internal str>, } - impl<'a> InnerInterner<'a> { + impl<'internal> InnerInterner<'internal> { /// Creates an empty [InnerInterner]. fn new() -> Self { Self { @@ -299,9 +305,12 @@ mod interner { // This is okay since the lifetime of the arena is identical to the one of the struct. // It is also okay to use it from inside the mutex, since typed_arena does not allow // deallocation, so references are valid until the arena drop, which is tied to the - // struct drop. + // struct drop. Since there is no 'self lifetime, we use 'internal instead, which + // at least has 'internal: 'self. let in_string = unsafe { - std::mem::transmute(self.arena.lock().unwrap().alloc_str(string.as_ref())) + std::mem::transmute::<&'_ str, &'internal str>( + self.arena.lock().unwrap().alloc_str(string.as_ref()), + ) }; let sym = Symbol(self.vec.len() as u32); self.vec.push(in_string); @@ -313,6 +322,8 @@ mod interner { /// This operation cannot fails since the only way to have a [Symbol] /// is to have [interned](InnerInterner::intern) the corresponding string first. fn lookup(&self, sym: Symbol) -> &str { + // The lifetime implicitly shrinks from 'internal to 'self, which + // prevents 'internal from leaking out. self.vec[sym.0 as usize] } } diff --git a/core/src/term/array.rs b/core/src/term/array.rs index 88bcdc0d71..24b203a675 100644 --- a/core/src/term/array.rs +++ b/core/src/term/array.rs @@ -157,29 +157,50 @@ impl IntoIterator for Array { // to the inner slice, then we can: // - Drop the elements outside our inner view, // - Move out the elements inside out inner view. + // - Drop the rest of the elements when we're dropped // Otherwise, we clone everything. - - unsafe { - let mut inner: Rc<[ManuallyDrop]> = transmute(self.inner); - let idx = self.start; - let end = self.end; - - if let Some(slice) = Rc::get_mut(&mut inner) { - for term in &mut slice[..idx] { + // + // If we start as a shared reference, we could become the only reference + // later, but we clone everything anyways, so we don't end up in an + // invalid in-between state where some terms have been freed manually + // and others haven't. + + let inner = if Rc::strong_count(&self.inner) != 1 || Rc::weak_count(&self.inner) != 0 { + IntoIterInner::Shared(self.inner) + } else { + unsafe { + let mut inner = + transmute::, Rc<[ManuallyDrop]>>(self.inner); + let slice = + Rc::get_mut(&mut inner).expect("non-unique Rc after checking for uniqueness"); + for term in &mut slice[..self.start] { ManuallyDrop::drop(term) } - for term in &mut slice[end..] { + for term in &mut slice[self.end..] { ManuallyDrop::drop(term) } - } - IntoIter { inner, idx, end } + IntoIterInner::Owned(inner) + } + }; + IntoIter { + inner, + idx: self.start, + end: self.end, } } } +enum IntoIterInner { + Shared(Rc<[RichTerm]>), + // This shouldn't really be an Rc. It should only ever have one reference. + // There may be a way to rewrite this once unsized locals are stabilized. + // But for now, there is no good way to repackage the inner array, so we + // keep it in an Rc. + Owned(Rc<[ManuallyDrop]>), +} pub struct IntoIter { - inner: Rc<[ManuallyDrop]>, + inner: IntoIterInner, idx: usize, end: usize, } @@ -191,14 +212,14 @@ impl Iterator for IntoIter { if self.idx == self.end { None } else { - let term = match Rc::get_mut(&mut self.inner) { - None => self.inner[..self.end] - .get(self.idx) - .cloned() - .map(ManuallyDrop::into_inner), - Some(slice) => slice[..self.end] - .get_mut(self.idx) - .map(|t| unsafe { ManuallyDrop::take(t) }), + let term = match &mut self.inner { + IntoIterInner::Shared(inner) => inner.get(self.idx).cloned(), + IntoIterInner::Owned(inner) => unsafe { + Rc::get_mut(inner) + .expect("non-unique Rc after checking for uniqueness") + .get_mut(self.idx) + .map(|t| ManuallyDrop::take(t)) + }, }; self.idx += 1; @@ -207,6 +228,18 @@ impl Iterator for IntoIter { } } +impl Drop for IntoIter { + fn drop(&mut self) { + // drop the rest of the items we haven't iterated through + if let IntoIterInner::Owned(inner) = &mut self.inner { + let inner = Rc::get_mut(inner).expect("non-unique Rc after checking for uniqueness"); + for term in &mut inner[self.idx..self.end] { + unsafe { ManuallyDrop::drop(term) } + } + } + } +} + impl ExactSizeIterator for IntoIter { fn len(&self) -> usize { self.end - self.idx