Feat: notes on safety, and inline listeners

notes/SAFETY.md

@@ -0,0 +1,26 @@
+# Safety
+
+We don't claim to be a "safe" library. We want to be 100% safe, and will create tests and validate that we are, but our priorities for this library are:
+
+- productivity
+- performance
+- safety
+  
+We are willing to use sharp tools (ie transmuting self-referential pointer types) in order to achieve higher ergonomics (ie returning self-referential listeners). 
+
+However, we can only use these sharp tools if we verify that it's not possible to write user-facing code that breaks safety guarantees. For internal code... well, whoever contributes needs to understand the architecture and read the comments related to safety.
+
+We are doing one of the more annoying things to do with Rust: self-referential graph structures. VNodes reference a bump arena which is contained by scope. Conveniently, the bump arenas also belong to scope, and now we have a self-referential struct. 
+
+We */would/* use a solution like ourborous or self_referential, but these libraries generate "imaginary code" that doesn't integrate with RA. It's simpler and easier to review if we set some rules on what is/isn't allowed.
+
+Here's the two main sources of unsafety:
+- 1) vnodes, bump arenas, and scope
+- 2) context_api and use_context
+  
+For 1), we can fairly confidently guarantee safety by being careful about lifetime casts. 
+
+For 2), use_context authors (mostly state management) can implement either the Unsafe API or the Safe API. The Safe API is less performant, but will likely do everything you need. The Unsafe API is more performant,
+
+Because of 2), we provide two state management solutions (D-Reducer and D-Dataflow) that use the Unsafe API, but will still be 100% safe.
+

packages/core/src/context.rs

@@ -39,16 +39,14 @@ pub struct Context<'src> {
 
     pub(crate) final_nodes: Rc<RefCell<Option<VNode<'static>>>>,
 
+    pub listeners: &'src RefCell<Vec<*const dyn Fn(crate::events::VirtualEvent)>>,
+
     // holder for the src lifetime
     // todo @jon remove this
     pub _p: std::marker::PhantomData<&'src ()>,
 }
 
 impl<'a> Context<'a> {
-    // pub fn props<P>() -> &'a P {
-    //     todo!()
-    // }
-
     // impl<'a, PropType> Context<'a, PropType> {
     /// Access the children elements passed into the component
     pub fn children(&self) -> Vec<VNode> {
@@ -73,7 +71,19 @@ impl<'a> Context<'a> {
     ) -> VNode<'a> {
         todo!()
     }
+}
 
+// NodeCtx is used to build VNodes in the component's memory space.
+// This struct adds metadata to the final DomTree about listeners, attributes, and children
+#[derive(Debug, Clone)]
+pub struct NodeCtx<'a> {
+    pub bump: &'a Bump,
+    pub idx: RefCell<usize>,
+    pub scope: ScopeIdx,
+    pub listeners: &'a RefCell<Vec<*const dyn Fn(crate::events::VirtualEvent)>>,
+}
+
+impl<'a> Context<'a> {
     /// Take a lazy VNode structure and actually build it with the context of the VDom's efficient VNode allocator.
     ///
     /// This function consumes the context and absorb the lifetime, so these VNodes *must* be returned.
@@ -92,8 +102,9 @@ impl<'a> Context<'a> {
     pub fn render(self, lazy_nodes: impl FnOnce(&NodeCtx<'a>) -> VNode<'a> + 'a) -> DomTree {
         let ctx = NodeCtx {
             bump: self.bump,
-            idx: 0.into(),
             scope: self.scope,
+            idx: 0.into(),
+            listeners: self.listeners,
         };
 
         let safe_nodes = lazy_nodes(&ctx);
@@ -103,26 +114,8 @@ impl<'a> Context<'a> {
     }
 }
 
-// NodeCtx is used to build VNodes in the component's memory space.
-// This struct adds metadata to the final DomTree about listeners, attributes, and children
-#[derive(Debug, Clone)]
-pub struct NodeCtx<'a> {
-    pub bump: &'a Bump,
-    pub idx: RefCell<usize>,
-    pub scope: ScopeIdx,
-}
-
-impl NodeCtx<'_> {
-    #[inline]
-    pub fn bump(&self) -> &Bump {
-        self.bump
-    }
-}
-
+/// This module provides internal state management functionality for Dioxus components
 pub mod hooks {
-    //! This module provides internal state management functionality for Dioxus components
-    //!
-
     use super::*;
 
     #[derive(Debug)]
@@ -196,23 +189,21 @@ pub mod hooks {
     }
 }
 
-mod context_api {
-    //! Context API
-    //!
-    //! The context API provides a mechanism for components to borrow state from other components higher in the tree.
-    //! By combining the Context API and the Subscription API, we can craft ergonomic global state management systems.
-    //!
-    //! This API is inherently dangerous because we could easily cause UB by allowing &T and &mut T to exist at the same time.
-    //! To prevent this, we expose the RemoteState<T> and RemoteLock<T> types which act as a form of reverse borrowing.
-    //! This is very similar to RwLock, except that RemoteState is copy-able. Unlike RwLock, derefing RemoteState can
-    //! cause panics if the pointer is null. In essence, we sacrifice the panic protection for ergonomics, but arrive at
-    //! a similar end result.
-    //!
-    //! Instead of placing the onus on the receiver of the data to use it properly, we wrap the source object in a
-    //! "shield" where gaining &mut access can only be done if no active StateGuards are open. This would fail and indicate
-    //! a failure of implementation.
-    //!
-    //!
+/// Context API
+///
+/// The context API provides a mechanism for components to borrow state from other components higher in the tree.
+/// By combining the Context API and the Subscription API, we can craft ergonomic global state management systems.
+///
+/// This API is inherently dangerous because we could easily cause UB by allowing &T and &mut T to exist at the same time.
+/// To prevent this, we expose the RemoteState<T> and RemoteLock<T> types which act as a form of reverse borrowing.
+/// This is very similar to RwLock, except that RemoteState is copy-able. Unlike RwLock, derefing RemoteState can
+/// cause panics if the pointer is null. In essence, we sacrifice the panic protection for ergonomics, but arrive at
+/// a similar end result.
+///
+/// Instead of placing the onus on the receiver of the data to use it properly, we wrap the source object in a
+/// "shield" where gaining &mut access can only be done if no active StateGuards are open. This would fail and indicate
+/// a failure of implementation.
+pub mod context_api {
 
     use std::ops::Deref;
 

packages/core/src/nodebuilder.rs

@@ -1,6 +1,6 @@
 //! Helpers for building virtual DOM VNodes.
 
-use std::ops::Deref;
+use std::{borrow::BorrowMut, ops::Deref};
 
 use crate::{
     context::NodeCtx,
@@ -344,6 +344,7 @@ where
         // todo:
         // increment listner id from nodectx ref
         // add listener attrs here instead of later?
+
         self.listeners.push(Listener {
             event,
             callback: self.ctx.bump.alloc(callback),
@@ -353,6 +354,15 @@ where
 
         // bump the context id forward
         *self.ctx.idx.borrow_mut() += 1;
+
+        // Add this listener to the context list
+        // This casts the listener to a self-referential pointer
+        // This is okay because the bump arena is stable
+        self.listeners.last().map(|g| {
+            let r = unsafe { std::mem::transmute::<&Listener<'a>, &Listener<'static>>(g) };
+            self.ctx.listeners.borrow_mut().push(r.callback as *const _);
+        });
+
         self
     }
 }

packages/core/src/scope.rs

@@ -3,18 +3,12 @@ use crate::context::hooks::Hook;
 use crate::innerlude::*;
 use crate::nodes::VNode;
 use bumpalo::Bump;
-// use generational_arena::ScopeIdx;
 
 use std::{
-    any::TypeId,
-    borrow::{Borrow, BorrowMut},
-    cell::{RefCell, UnsafeCell},
-    future::Future,
+    any::{Any, TypeId},
+    cell::RefCell,
     marker::PhantomData,
-    ops::{Deref, DerefMut},
-    sync::atomic::AtomicUsize,
-    sync::atomic::Ordering,
-    todo,
+    ops::Deref,
 };
 
 /// Every component in Dioxus is represented by a `Scope`.
@@ -24,31 +18,35 @@ use std::{
 /// Scopes are allocated in a generational arena. As components are mounted/unmounted, they will replace slots of dead components.
 /// The actual contents of the hooks, though, will be allocated with the standard allocator. These should not allocate as frequently.
 pub struct Scope {
-    // pub(crate) struct Scope {
-    // TODO @Jon
-    // These hooks are actually references into the hook arena
-    // These two could be combined with "OwningRef" to remove unsafe usage
-    // could also use ourborous
-    pub hooks: RefCell<Vec<*mut Hook>>,
-    pub hook_arena: typed_arena::Arena<Hook>,
-
     // Map to the parent
     pub parent: Option<ScopeIdx>,
 
-    pub frames: ActiveFrame,
-
-    // List of listeners generated when CTX is evaluated
-    pub listeners: Vec<*const dyn Fn(crate::events::VirtualEvent)>,
-    // pub listeners: Vec<*const dyn Fn(crate::events::VirtualEvent)>,
-    // pub listeners: Vec<Box<dyn Fn(crate::events::VirtualEvent)>>,
-
     // lying, cheating reference >:(
     pub props: Box<dyn std::any::Any>,
 
     // our own index
     pub myidx: ScopeIdx,
 
-    // pub props_type: TypeId,
+    // ==========================
+    // slightly unsafe stuff
+    // ==========================
+    // an internal, highly efficient storage of vnodes
+    pub frames: ActiveFrame,
+
+    // These hooks are actually references into the hook arena
+    // These two could be combined with "OwningRef" to remove unsafe usage
+    // could also use ourborous
+    pub hooks: RefCell<Vec<*mut Hook>>,
+    pub hook_arena: typed_arena::Arena<Hook>,
+
+    // Unsafety:
+    // - is self-refenrential and therefore needs to point into the bump
+    //
+    // Stores references into the listeners attached to the vnodes
+    pub listeners: RefCell<Vec<*const dyn Fn(crate::events::VirtualEvent)>>,
+
+    // Unsafety
+    // - is a raw ptr because we need to compare
     pub caller: *const (),
 }
 
@@ -66,8 +64,7 @@ impl Scope {
         // Capture the caller
         let caller = f as *const ();
 
-        let listeners = vec![];
-        // let listeners: Vec<Box<dyn Fn(crate::events::VirtualEvent)>> = vec![];
+        let listeners = Default::default();
 
         let old_frame = BumpFrame {
             bump: Bump::new(),
@@ -97,7 +94,21 @@ impl Scope {
             props,
         }
     }
+}
 
+pub struct RawComponent {
+    // used as a memoization strategy
+    comparator: *const Box<dyn Fn(&Box<dyn Any>) -> bool>,
+
+    // used to actually run the component
+    // encapsulates props
+    runner: *const Box<dyn Fn(Context) -> DomTree>,
+
+    // the actual FC<T>
+    raw: *const (),
+}
+
+impl Scope {
     /// Create a new context and run the component with references from the Virtual Dom
     /// This function downcasts the function pointer based on the stored props_type
     ///
@@ -106,11 +117,11 @@ impl Scope {
         let frame = {
             let frame = self.frames.next();
             frame.bump.reset();
-            log::debug!("Rednering into frame {:?}", frame as *const _);
             frame
         };
 
         let node_slot = std::rc::Rc::new(RefCell::new(None));
+
         let ctx: Context<'bump> = Context {
             arena: &self.hook_arena,
             hooks: &self.hooks,
@@ -119,6 +130,7 @@ impl Scope {
             _p: PhantomData {},
             final_nodes: node_slot.clone(),
             scope: self.myidx,
+            listeners: &self.listeners,
         };
 
         unsafe {
@@ -139,7 +151,7 @@ impl Scope {
             let props = self.props.downcast_ref::<PLocked>().unwrap();
 
             // Note that the actual modification of the vnode head element occurs during this call
-            let _nodes: DomTree = caller(ctx, props);
+            let _: DomTree = caller(ctx, props);
 
             /*
             SAFETY ALERT
@@ -158,32 +170,8 @@ impl Scope {
                 .borrow_mut()
                 .take()
                 .expect("Viewing did not happen");
-
-            // todo:
-            // make this so we dont have to iterate through the vnodes to get its listener
-            let mut listeners = vec![];
-            retrieve_listeners(&frame.head_node, &mut listeners);
-            self.listeners = listeners
-                .into_iter()
-                .map(|f| {
-                    let g = f.callback;
-                    g as *const _
-                })
-                .collect();
-
-            // consume the listeners from the head_node into a list of boxed ref listeners
         }
     }
-
-    /// Accessor to get the root node and its children (safely)\
-    /// Scope is self-referntial, so we are forced to use the 'static lifetime to cheat
-    pub fn new_frame<'bump>(&'bump self) -> &'bump VNode<'bump> {
-        self.frames.current_head_node()
-    }
-
-    pub fn old_frame<'bump>(&'bump self) -> &'bump VNode<'bump> {
-        self.frames.prev_head_node()
-    }
 }
 
 fn retrieve_listeners(node: &VNode<'static>, listeners: &mut Vec<&Listener>) {
@@ -198,6 +186,21 @@ fn retrieve_listeners(node: &VNode<'static>, listeners: &mut Vec<&Listener>) {
     }
 }
 
+// ==========================
+// Active-frame related code
+// ==========================
+impl Scope {
+    /// Accessor to get the root node and its children (safely)\
+    /// Scope is self-referntial, so we are forced to use the 'static lifetime to cheat
+    pub fn new_frame<'bump>(&'bump self) -> &'bump VNode<'bump> {
+        self.frames.current_head_node()
+    }
+
+    pub fn old_frame<'bump>(&'bump self) -> &'bump VNode<'bump> {
+        self.frames.prev_head_node()
+    }
+}
+
 // todo, do better with the active frame stuff
 // somehow build this vnode with a lifetime tied to self
 // This root node has  "static" lifetime, but it's really not static.
@@ -205,7 +208,7 @@ fn retrieve_listeners(node: &VNode<'static>, listeners: &mut Vec<&Listener>) {
 // Use this node as if it were static is unsafe, and needs to be fixed with ourborous or owning ref
 // ! do not copy this reference are things WILL break !
 pub struct ActiveFrame {
-    pub idx: AtomicUsize,
+    pub idx: RefCell<usize>,
     pub frames: [BumpFrame; 2],
 }
 
@@ -223,7 +226,7 @@ impl ActiveFrame {
     }
 
     fn current_head_node<'b>(&'b self) -> &'b VNode<'b> {
-        let raw_node = match self.idx.borrow().load(Ordering::Relaxed) & 1 == 0 {
+        let raw_node = match *self.idx.borrow() & 1 == 0 {
             true => &self.frames[0],
             false => &self.frames[1],
         };
@@ -237,7 +240,7 @@ impl ActiveFrame {
     }
 
     fn prev_head_node<'b>(&'b self) -> &'b VNode<'b> {
-        let raw_node = match self.idx.borrow().load(Ordering::Relaxed) & 1 != 0 {
+        let raw_node = match *self.idx.borrow() & 1 != 0 {
             true => &self.frames[0],
             false => &self.frames[1],
         };
@@ -251,10 +254,9 @@ impl ActiveFrame {
     }
 
     fn next(&mut self) -> &mut BumpFrame {
-        self.idx.fetch_add(1, Ordering::Relaxed);
-        let cur = self.idx.borrow().load(Ordering::Relaxed);
+        *self.idx.borrow_mut() += 1;
 
-        if cur % 2 == 0 {
+        if *self.idx.borrow() % 2 == 0 {
             &mut self.frames[0]
         } else {
             &mut self.frames[1]
@@ -262,7 +264,7 @@ impl ActiveFrame {
     }
 }
 
-// #[cfg(test)]
+#[cfg(test)]
 mod tests {
     use super::*;
     use crate::prelude::*;

packages/core/src/virtual_dom.rs

@@ -136,11 +136,12 @@ impl VirtualDom {
             .get_mut(component_id)
             .expect("Component should exist if an event was triggered");
 
-        log::debug!("list: {}", component.listeners.len());
+        log::debug!("list: {}", component.listeners.borrow().len());
 
         let listener = unsafe {
             component
                 .listeners
+                .borrow()
                 .get(listener_id as usize)
                 .expect("Listener should exist if it was triggered")
                 .as_ref()