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

[3/n][vm-rewrite][Move] Update native context extensions to support Cloning #21067

Open
wants to merge 3 commits into
base: tzakian/vm-rewrite-adapter-2
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,34 @@
// SPDX-License-Identifier: Apache-2.0

use better_any::{Tid, TidAble, TidExt};
use std::{any::TypeId, collections::HashMap};
use std::{any::TypeId, cell::RefCell, collections::HashMap, ops::Deref, rc::Rc};

/// A helper wrapper around a `Tid`able type that encapsulates interior mutability in a single-threaded
/// manner.
///
/// Note that this is _not_ threadsafe. If you need threadsafe access to the `T` you will need to
/// handle that within `T'`s type (just like in the previous implementation of the
/// `NativeContextExtensions`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little confused. It appears we are making it so that all mutability needs to be explicit when building up the native context extension, whereas before everything was arbitrarily mutable. What is the motivation for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the discussions we had about how we wanted to change this, the consensus that I gathered is that we wanted to move to this model where the native context extensions do not make any guarantees about mutability, or directly support interior mutability itself, but instead the decision around how to (or not to) support any type of mutability would be a per-extension decisions, and the extension would need to be explicit about how, and if, it will support mutability.

I think this is generally a better place to be as it makes it much clearer from the perspective of a user of the native extensions about how/if you want to support mutability for the extension (do you want a Mutex for some reason? Maybe no mutability, etc).

#[derive(Tid)]
pub struct NativeContextMut<'a, T: Tid<'a>>(pub RefCell<T>, std::marker::PhantomData<&'a ()>);

impl<'a, T: Tid<'a>> NativeContextMut<'a, T> {
/// Create a new `NativeContextMut` value with the given value.
pub fn new(t: T) -> Self {
NativeContextMut(RefCell::new(t), std::marker::PhantomData)
}

pub fn into_inner(self) -> T {
self.0.into_inner()
}
}

impl<'a, T: Tid<'a>> Deref for NativeContextMut<'a, T> {
type Target = RefCell<T>;
fn deref(&self) -> &Self::Target {
&self.0
}
}

/// A data type to represent a heterogeneous collection of extensions which are available to
/// native functions. A value to this is passed into the session function execution.
Expand All @@ -13,15 +40,15 @@ use std::{any::TypeId, collections::HashMap};
/// avoids that extensions need to have `'static` lifetime, which `Any` requires. In order to make a
/// struct suitable to be a 'Tid', use `#[derive(Tid)]` in the struct declaration. (See also
/// tests at the end of this module.)
#[derive(Default)]
#[derive(Default, Clone)]
pub struct NativeContextExtensions<'a> {
map: HashMap<TypeId, Box<dyn Tid<'a>>>,
map: HashMap<TypeId, Rc<dyn Tid<'a>>>,
}

impl<'a> NativeContextExtensions<'a> {
pub fn add<T: TidAble<'a>>(&mut self, ext: T) {
assert!(
self.map.insert(T::id(), Box::new(ext)).is_none(),
self.map.insert(T::id(), Rc::new(ext)).is_none(),
"multiple extensions of the same type not allowed"
)
}
Expand All @@ -35,32 +62,23 @@ impl<'a> NativeContextExtensions<'a> {
.unwrap()
}

pub fn get_mut<T: TidAble<'a>>(&mut self) -> &mut T {
self.map
.get_mut(&T::id())
.expect("extension unknown")
.as_mut()
.downcast_mut::<T>()
.unwrap()
}

pub fn remove<T: TidAble<'a>>(&mut self) -> T {
pub fn remove<T: TidAble<'a>>(&mut self) -> Rc<T> {
// can't use expect below because it requires `T: Debug`.
match self
.map
.remove(&T::id())
.expect("extension unknown")
.downcast_box::<T>()
.downcast_rc::<T>()
{
Ok(val) => *val,
Ok(val) => val,
Err(_) => panic!("downcast error"),
}
}
}

#[cfg(test)]
mod tests {
use crate::natives::extensions::NativeContextExtensions;
use super::*;
use better_any::{Tid, TidAble};

#[derive(Tid)]
Expand All @@ -73,11 +91,11 @@ mod tests {
let mut v: u64 = 23;
let e = Ext { a: &mut v };
let mut exts = NativeContextExtensions::default();
exts.add(e);
*exts.get_mut::<Ext>().a += 1;
assert_eq!(*exts.get_mut::<Ext>().a, 24);
*exts.get_mut::<Ext>().a += 1;
let e1 = exts.remove::<Ext>();
assert_eq!(*e1.a, 25)
exts.add(NativeContextMut::new(e));
*exts.get::<NativeContextMut<Ext>>().borrow_mut().a += 1;
assert_eq!(*exts.get::<NativeContextMut<Ext>>().borrow_mut().a, 24);
*exts.get::<NativeContextMut<Ext>>().borrow_mut().a += 1;
let e1 = exts.get::<NativeContextMut<Ext>>();
assert_eq!(*e1.borrow().a, 25);
}
}
Loading