From 4d73ffa3611d71a68ca40202f0c0e9667e56ed11 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Fri, 16 Dec 2022 19:54:33 -0800 Subject: [PATCH 1/5] fix: save listeners, borrowed props, and pull back props --- examples/crm.rs | 6 +- packages/core/src/arena.rs | 45 ++-- packages/core/src/create.rs | 7 +- packages/core/src/diff.rs | 425 +++++++++++++------------------ packages/core/src/scope_arena.rs | 8 +- packages/core/src/scopes.rs | 23 +- packages/core/src/virtual_dom.rs | 12 +- 7 files changed, 237 insertions(+), 289 deletions(-) diff --git a/examples/crm.rs b/examples/crm.rs index 724010177..e99cfe241 100644 --- a/examples/crm.rs +++ b/examples/crm.rs @@ -30,7 +30,7 @@ fn app(cx: Scope) -> Element { integrity: "sha384-Uu6IeWbM+gzNVXJcM9XV3SohHtmWE+3VGi496jvgX1jyvDTXfdK+rfZc8C1Aehk5", crossorigin: "anonymous", } - h1 {"Dioxus CRM Example"} + h1 { "Dioxus CRM Example" } Router { Route { to: "/", div { class: "crm", @@ -40,12 +40,12 @@ fn app(cx: Scope) -> Element { div { class: "client", style: "margin-bottom: 50px", p { "First Name: {client.first_name}" } p { "Last Name: {client.last_name}" } - p {"Description: {client.description}"} + p { "Description: {client.description}" } }) ) } Link { to: "/new", class: "pure-button pure-button-primary", "Add New" } - Link { to: "/new", class: "pure-button", "Settings" } + Link { to: "/settings", class: "pure-button", "Settings" } } } Route { to: "/new", diff --git a/packages/core/src/arena.rs b/packages/core/src/arena.rs index 28d182b8f..d54cf1388 100644 --- a/packages/core/src/arena.rs +++ b/packages/core/src/arena.rs @@ -1,4 +1,7 @@ -use crate::{nodes::RenderReturn, nodes::VNode, virtual_dom::VirtualDom, DynamicNode, ScopeId}; +use crate::{ + nodes::RenderReturn, nodes::VNode, virtual_dom::VirtualDom, AttributeValue, DynamicNode, + ScopeId, +}; use bumpalo::boxed::Box as BumpBox; /// An Element's unique identifier. @@ -122,31 +125,27 @@ impl VirtualDom { /// Descend through the tree, removing any borrowed props and listeners pub(crate) fn ensure_drop_safety(&self, scope: ScopeId) { - let node = unsafe { self.scopes[scope.0].previous_frame().try_load_node() }; + let scope = &self.scopes[scope.0]; - // And now we want to make sure the previous frame has dropped anything that borrows self - if let Some(RenderReturn::Sync(Ok(node))) = node { - self.ensure_drop_safety_inner(node); - } - } - - fn ensure_drop_safety_inner(&self, node: &VNode) { - node.clear_listeners(); - - node.dynamic_nodes.iter().for_each(|child| match child { - // Only descend if the props are borrowed - DynamicNode::Component(c) if !c.static_props => { - if let Some(scope) = c.scope.get() { - self.ensure_drop_safety(scope); - } - c.props.take(); + // make sure we drop all borrowed props manually to guarantee that their drop implementation is called before we + // run the hooks (which hold an &mut Reference) + // recursively call ensure_drop_safety on all children + let mut props = scope.borrowed_props.borrow_mut(); + props.drain(..).for_each(|comp| { + let comp = unsafe { &*comp }; + if let Some(scope_id) = comp.scope.get() { + self.ensure_drop_safety(scope_id); } + drop(comp.props.take()); + }); - DynamicNode::Fragment(f) => f - .iter() - .for_each(|node| self.ensure_drop_safety_inner(node)), - - _ => {} + // Now that all the references are gone, we can safely drop our own references in our listeners. + let mut listeners = scope.listeners.borrow_mut(); + listeners.drain(..).for_each(|listener| { + let listener = unsafe { &*listener }; + if let AttributeValue::Listener(l) = &listener.value { + _ = l.take(); + } }); } } diff --git a/packages/core/src/create.rs b/packages/core/src/create.rs index fa102bd52..834b62437 100644 --- a/packages/core/src/create.rs +++ b/packages/core/src/create.rs @@ -1,3 +1,4 @@ +use crate::any_props::AnyProps; use crate::innerlude::{VComponent, VPlaceholder, VText}; use crate::mutations::Mutation; use crate::mutations::Mutation::*; @@ -170,12 +171,12 @@ impl<'b> VirtualDom { attribute.mounted_element.set(id); // Safety: we promise not to re-alias this text later on after committing it to the mutation - let unbounded_name = unsafe { std::mem::transmute(attribute.name) }; + let unbounded_name: &str = unsafe { std::mem::transmute(attribute.name) }; match &attribute.value { AttributeValue::Text(value) => { // Safety: we promise not to re-alias this text later on after committing it to the mutation - let unbounded_value = unsafe { std::mem::transmute(*value) }; + let unbounded_value: &str = unsafe { std::mem::transmute(*value) }; self.mutations.push(SetAttribute { name: unbounded_name, @@ -334,7 +335,7 @@ impl<'b> VirtualDom { ) -> usize { let scope = match component.props.take() { Some(props) => { - let unbounded_props = unsafe { std::mem::transmute(props) }; + let unbounded_props: Box = unsafe { std::mem::transmute(props) }; let scope = self.new_scope(unbounded_props, component.name); scope.id } diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 8b283f7f5..966afd624 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -1,4 +1,5 @@ use crate::{ + any_props::AnyProps, arena::ElementId, innerlude::{DirtyScope, VComponent, VPlaceholder, VText}, mutations::Mutation, @@ -141,32 +142,6 @@ impl<'b> VirtualDom { } } - fn replace_placeholder(&mut self, l: &'b VPlaceholder, r: &'b [VNode<'b>]) { - let m = self.create_children(r); - let id = l.id.get().unwrap(); - self.mutations.push(Mutation::ReplaceWith { id, m }); - self.reclaim(id); - } - - fn node_to_placeholder(&mut self, l: &'b [VNode<'b>], r: &'b VPlaceholder) { - // Remove the old nodes, except for one - self.remove_nodes(&l[1..]); - - // Now create the new one - let first = self.replace_inner(&l[0]); - - // Create the placeholder first, ensuring we get a dedicated ID for the placeholder - let placeholder = self.next_element(&l[0], &[]); - r.id.set(Some(placeholder)); - self.mutations - .push(Mutation::CreatePlaceholder { id: placeholder }); - - self.mutations - .push(Mutation::ReplaceWith { id: first, m: 1 }); - - self.try_reclaim(first); - } - fn diff_vcomponent( &mut self, left: &'b VComponent<'b>, @@ -180,39 +155,32 @@ impl<'b> VirtualDom { // Replace components that have different render fns if left.render_fn != right.render_fn { - let created = self.create_component_node(right_template, right, idx); - let head = unsafe { - self.scopes[left.scope.get().unwrap().0] - .root_node() - .extend_lifetime_ref() - }; - let id = match head { - RenderReturn::Sync(Ok(node)) => self.replace_inner(node), - _ => todo!(), - }; - self.mutations - .push(Mutation::ReplaceWith { id, m: created }); - self.drop_scope(left.scope.get().unwrap()); - return; + todo!() + // let created = self.create_component_node(right_template, right, idx); + // let head = unsafe { + // self.scopes[left.scope.get().unwrap().0] + // .root_node() + // .extend_lifetime_ref() + // }; + // let last = match head { + // RenderReturn::Sync(Ok(node)) => self.find_last_element(node), + // _ => todo!(), + // }; + // self.mutations + // .push(Mutation::ReplaceWith { id, m: created }); + // self.drop_scope(left.scope.get().unwrap()); + // return; } // Make sure the new vcomponent has the right scopeid associated to it - let Some(scope_id) = left.scope.get() else { - return; - }; - // let scope_id = left.scope.get().unwrap_or_else(|| { - // panic!( - // "A component should always have a scope associated to it. {:?}\n {:#?}", - // right.name, - // std::backtrace::Backtrace::force_capture() - // ) - // }); + let scope_id = left.scope.get().unwrap(); right.scope.set(Some(scope_id)); // copy out the box for both let old = self.scopes[scope_id.0].props.as_ref(); - let new = right.props.take().unwrap(); + let new: Box = right.props.take().unwrap(); + let new: Box = unsafe { std::mem::transmute(new) }; // If the props are static, then we try to memoize by setting the new with the old // The target scopestate still has the reference to the old props, so there's no need to update anything @@ -222,7 +190,7 @@ impl<'b> VirtualDom { } // First, move over the props from the old to the new, dropping old props in the process - self.scopes[scope_id.0].props = unsafe { std::mem::transmute(new) }; + self.scopes[scope_id.0].props = Some(new); // Now run the component and diff it self.run_scope(scope_id); @@ -273,7 +241,7 @@ impl<'b> VirtualDom { /// ``` fn light_diff_templates(&mut self, left: &'b VNode<'b>, right: &'b VNode<'b>) { match matching_components(left, right) { - None => self.replace(left, right), + None => self.replace(left, [right]), Some(components) => components .into_iter() .enumerate() @@ -298,120 +266,6 @@ impl<'b> VirtualDom { } } - /// Remove all the top-level nodes, returning the firstmost root ElementId - /// - /// All IDs will be garbage collected - fn replace_inner(&mut self, node: &'b VNode<'b>) -> ElementId { - let id = match node.dynamic_root(0) { - None => node.root_ids[0].get().unwrap(), - Some(Text(t)) => t.id.get().unwrap(), - Some(Placeholder(e)) => e.id.get().unwrap(), - Some(Fragment(nodes)) => { - let id = self.replace_inner(&nodes[0]); - self.remove_nodes(&nodes[1..]); - id - } - Some(Component(comp)) => { - let scope = comp.scope.get().unwrap(); - match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { - RenderReturn::Sync(Ok(t)) => self.replace_inner(t), - _ => todo!("cannot handle nonstandard nodes"), - } - } - }; - - // Just remove the rest from the dom - for (idx, _) in node.template.roots.iter().enumerate().skip(1) { - self.remove_root_node(node, idx); - } - - // Garabge collect all of the nodes since this gets used in replace - self.clean_up_node(node); - - id - } - - /// Clean up the node, not generating mutations - /// - /// Simply walks through the dynamic nodes - fn clean_up_node(&mut self, node: &'b VNode<'b>) { - for (idx, dyn_node) in node.dynamic_nodes.iter().enumerate() { - // Roots are cleaned up automatically? - if node.template.node_paths[idx].len() == 1 { - continue; - } - - match dyn_node { - Component(comp) => { - if let Some(scope) = comp.scope.take() { - match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { - RenderReturn::Sync(Ok(t)) => self.clean_up_node(t), - _ => todo!("cannot handle nonstandard nodes"), - }; - } - } - Text(t) => { - if let Some(id) = t.id.take() { - self.reclaim(id) - } - } - Placeholder(t) => { - if let Some(id) = t.id.take() { - self.reclaim(id) - } - } - Fragment(nodes) => nodes.iter().for_each(|node| self.clean_up_node(node)), - }; - } - - // we clean up nodes with dynamic attributes, provided the node is unique and not a root node - let mut id = None; - for (idx, attr) in node.dynamic_attrs.iter().enumerate() { - // We'll clean up the root nodes either way, so don't worry - if node.template.attr_paths[idx].len() == 1 { - continue; - } - - let next_id = attr.mounted_element.get(); - - if id == Some(next_id) { - continue; - } - - id = Some(next_id); - - self.reclaim(next_id); - } - } - - fn remove_root_node(&mut self, node: &'b VNode<'b>, idx: usize) { - match node.dynamic_root(idx) { - Some(Text(i)) => { - let id = i.id.take().unwrap(); - self.mutations.push(Mutation::Remove { id }); - self.reclaim(id); - } - Some(Placeholder(e)) => { - let id = e.id.take().unwrap(); - self.mutations.push(Mutation::Remove { id }); - self.reclaim(id); - } - Some(Fragment(nodes)) => self.remove_nodes(nodes), - Some(Component(comp)) => { - let scope = comp.scope.take().unwrap(); - match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { - RenderReturn::Sync(Ok(t)) => self.remove_node(t), - _ => todo!("cannot handle nonstandard nodes"), - }; - } - None => { - let id = node.root_ids[idx].get().unwrap(); - self.mutations.push(Mutation::Remove { id }); - self.reclaim(id); - } - }; - } - fn diff_non_empty_fragment(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { let new_is_keyed = new[0].key.is_some(); let old_is_keyed = old[0].key.is_some(); @@ -662,7 +516,7 @@ impl<'b> VirtualDom { if shared_keys.is_empty() { if old.get(0).is_some() { self.remove_nodes(&old[1..]); - self.replace_many(&old[0], new); + self.replace(&old[0], new); } else { // I think this is wrong - why are we appending? // only valid of the if there are no trailing elements @@ -678,7 +532,7 @@ impl<'b> VirtualDom { for child in old { let key = child.key.unwrap(); if !shared_keys.contains(&key) { - self.remove_node(child); + self.remove_node(child, true); } } @@ -781,54 +635,6 @@ impl<'b> VirtualDom { } } - /// Remove these nodes from the dom - /// Wont generate mutations for the inner nodes - fn remove_nodes(&mut self, nodes: &'b [VNode<'b>]) { - // note that we iterate in reverse to unlink lists of nodes in their rough index order - nodes.iter().rev().for_each(|node| self.remove_node(node)); - } - - fn remove_node(&mut self, node: &'b VNode<'b>) { - for (idx, _) in node.template.roots.iter().enumerate() { - let id = match node.dynamic_root(idx) { - Some(Text(t)) => t.id.take(), - Some(Placeholder(t)) => t.id.take(), - Some(Fragment(t)) => return self.remove_nodes(t), - Some(Component(comp)) => { - comp.scope.set(None); - return self.remove_component(comp.scope.get().unwrap()); - } - None => node.root_ids[idx].get(), - } - .unwrap(); - - self.mutations.push(Mutation::Remove { id }) - } - - self.clean_up_node(node); - - for root in node.root_ids { - let id = root.get().unwrap(); - if id.0 != 0 { - self.reclaim(id); - } - } - } - - fn remove_component(&mut self, scope_id: ScopeId) { - let height = self.scopes[scope_id.0].height; - self.dirty_scopes.remove(&DirtyScope { - height, - id: scope_id, - }); - - // I promise, since we're descending down the tree, this is safe - match unsafe { self.scopes[scope_id.0].root_node().extend_lifetime_ref() } { - RenderReturn::Sync(Ok(t)) => self.remove_node(t), - _ => todo!("cannot handle nonstandard nodes"), - } - } - /// Push all the real nodes on the stack fn push_all_real_nodes(&mut self, node: &'b VNode<'b>) -> usize { node.template @@ -836,44 +642,50 @@ impl<'b> VirtualDom { .iter() .enumerate() .map(|(idx, _)| { - match node.dynamic_root(idx) { - Some(Text(t)) => { + let node = match node.dynamic_root(idx) { + Some(node) => node, + None => { + self.mutations.push(Mutation::PushRoot { + id: node.root_ids[idx].get().unwrap(), + }); + return 1; + } + }; + + match node { + Text(t) => { self.mutations.push(Mutation::PushRoot { id: t.id.get().unwrap(), }); 1 } - Some(Placeholder(t)) => { + Placeholder(t) => { self.mutations.push(Mutation::PushRoot { id: t.id.get().unwrap(), }); 1 } - Some(Fragment(nodes)) => nodes + Fragment(nodes) => nodes .iter() .map(|node| self.push_all_real_nodes(node)) .count(), - Some(Component(comp)) => { + Component(comp) => { let scope = comp.scope.get().unwrap(); match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { RenderReturn::Sync(Ok(node)) => self.push_all_real_nodes(node), _ => todo!(), } } - None => { - self.mutations.push(Mutation::PushRoot { - id: node.root_ids[idx].get().unwrap(), - }); - 1 - } - }; + } }) .count() } - fn create_children(&mut self, nodes: &'b [VNode<'b>]) -> usize { - nodes.iter().fold(0, |acc, child| acc + self.create(child)) + fn create_children(&mut self, nodes: impl IntoIterator>) -> usize { + nodes + .into_iter() + .fold(0, |acc, child| acc + self.create(child)) } fn create_and_insert_before(&mut self, new: &'b [VNode<'b>], before: &'b VNode<'b>) { @@ -888,6 +700,135 @@ impl<'b> VirtualDom { self.mutations.push(Mutation::InsertAfter { id, m }) } + /// Simply replace a placeholder with a list of nodes + fn replace_placeholder(&mut self, l: &'b VPlaceholder, r: &'b [VNode<'b>]) { + let m = self.create_children(r); + let id = l.id.get().unwrap(); + self.mutations.push(Mutation::ReplaceWith { id, m }); + self.reclaim(id); + } + + fn replace(&mut self, left: &'b VNode<'b>, right: impl IntoIterator>) { + let m = self.create_children(right); + let id = self.find_last_element(left); + self.mutations.push(Mutation::InsertAfter { id, m }); + self.remove_node(left, true); + } + + fn node_to_placeholder(&mut self, l: &'b [VNode<'b>], r: &'b VPlaceholder) { + // Create the placeholder first, ensuring we get a dedicated ID for the placeholder + let placeholder = self.next_element(&l[0], &[]); + + r.id.set(Some(placeholder)); + + let id = self.find_last_element(&l[0]); + + self.mutations + .push(Mutation::CreatePlaceholder { id: placeholder }); + + self.mutations.push(Mutation::InsertAfter { id, m: 1 }); + + self.remove_nodes(l); + } + + /// Remove these nodes from the dom + /// Wont generate mutations for the inner nodes + fn remove_nodes(&mut self, nodes: &'b [VNode<'b>]) { + nodes + .iter() + .rev() + .for_each(|node| self.remove_node(node, true)); + } + + fn remove_node(&mut self, node: &'b VNode<'b>, gen_muts: bool) { + // Clean up the roots, assuming we need to generate mutations for these + for (idx, _) in node.template.roots.iter().enumerate() { + if let Some(dy) = node.dynamic_root(idx) { + self.remove_dynamic_node(dy, gen_muts); + } else { + let id = node.root_ids[idx].get().unwrap(); + if gen_muts { + self.mutations.push(Mutation::Remove { id }); + } + self.reclaim(id); + } + } + + for (idx, dyn_node) in node.dynamic_nodes.iter().enumerate() { + // Roots are cleaned up automatically above + if node.template.node_paths[idx].len() == 1 { + continue; + } + + self.remove_dynamic_node(dyn_node, false); + } + + // we clean up nodes with dynamic attributes, provided the node is unique and not a root node + let mut id = None; + for (idx, attr) in node.dynamic_attrs.iter().enumerate() { + // We'll clean up the root nodes either way, so don't worry + if node.template.attr_paths[idx].len() == 1 { + continue; + } + + let next_id = attr.mounted_element.get(); + + if id == Some(next_id) { + continue; + } + + id = Some(next_id); + + self.reclaim(next_id); + } + } + + fn remove_dynamic_node(&mut self, node: &DynamicNode, gen_muts: bool) { + match node { + Component(comp) => self.remove_component_node(comp, gen_muts), + Text(t) => self.remove_text_node(t), + Placeholder(t) => self.remove_placeholder(t), + Fragment(nodes) => nodes + .iter() + .for_each(|node| self.remove_node(node, gen_muts)), + }; + } + + fn remove_placeholder(&mut self, t: &VPlaceholder) { + if let Some(id) = t.id.take() { + self.reclaim(id) + } + } + + fn remove_text_node(&mut self, t: &VText) { + if let Some(id) = t.id.take() { + self.reclaim(id) + } + } + + fn remove_component_node(&mut self, comp: &VComponent, gen_muts: bool) { + if let Some(scope) = comp.scope.take() { + match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { + RenderReturn::Sync(Ok(t)) => self.remove_node(t, gen_muts), + _ => todo!("cannot handle nonstandard nodes"), + }; + + let props = self.scopes[scope.0].props.take(); + // let props: Option> = comp.props.take(); + + println!("taking props... {:?}", scope); + + self.dirty_scopes.remove(&DirtyScope { + height: self.scopes[scope.0].height, + id: scope, + }); + + *comp.props.borrow_mut() = unsafe { std::mem::transmute(props) }; + + self.scopes.remove(scope.0); + } + } + fn find_first_element(&self, node: &'b VNode<'b>) -> ElementId { match node.dynamic_root(0) { None => node.root_ids[0].get().unwrap(), @@ -919,28 +860,6 @@ impl<'b> VirtualDom { } } } - - fn replace(&mut self, left: &'b VNode<'b>, right: &'b VNode<'b>) { - let first = self.find_first_element(left); - let id = self.replace_inner(left); - let created = self.create(right); - self.mutations.push(Mutation::ReplaceWith { - id: first, - m: created, - }); - self.try_reclaim(id); - } - - fn replace_many(&mut self, left: &'b VNode<'b>, right: &'b [VNode<'b>]) { - let first = self.find_first_element(left); - let id = self.replace_inner(left); - let created = self.create_children(right); - self.mutations.push(Mutation::ReplaceWith { - id: first, - m: created, - }); - self.try_reclaim(id); - } } /// Are the templates the same? diff --git a/packages/core/src/scope_arena.rs b/packages/core/src/scope_arena.rs index db328fd93..9c0f96db3 100644 --- a/packages/core/src/scope_arena.rs +++ b/packages/core/src/scope_arena.rs @@ -32,8 +32,9 @@ impl VirtualDom { parent, id, height, - props: Some(props), name, + props: Some(props), + tasks: self.scheduler.clone(), placeholder: Default::default(), node_arena_1: BumpFrame::new(0), node_arena_2: BumpFrame::new(0), @@ -43,7 +44,8 @@ impl VirtualDom { hook_list: Default::default(), hook_idx: Default::default(), shared_contexts: Default::default(), - tasks: self.scheduler.clone(), + borrowed_props: Default::default(), + listeners: Default::default(), })) } @@ -75,7 +77,7 @@ impl VirtualDom { scope.hook_idx.set(0); // safety: due to how we traverse the tree, we know that the scope is not currently aliased - let props = scope.props.as_ref().unwrap().as_ref(); + let props: &dyn AnyProps = scope.props.as_ref().unwrap().as_ref(); let props: &dyn AnyProps = mem::transmute(props); props.render(scope).extend_lifetime() }; diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index 0e487bfa8..14fc8c9e6 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -87,6 +87,9 @@ pub struct ScopeState { pub(crate) tasks: Rc, pub(crate) spawned_tasks: FxHashSet, + pub(crate) borrowed_props: RefCell>>, + pub(crate) listeners: RefCell>>, + pub(crate) props: Option>>, pub(crate) placeholder: Cell>, } @@ -369,7 +372,25 @@ impl<'src> ScopeState { /// } ///``` pub fn render(&'src self, rsx: LazyNodes<'src, '_>) -> Element<'src> { - Ok(rsx.call(self)) + let element = rsx.call(self); + + let mut listeners = self.listeners.borrow_mut(); + for attr in element.dynamic_attrs { + if let AttributeValue::Listener(_) = attr.value { + let unbounded = unsafe { std::mem::transmute(attr as *const Attribute) }; + listeners.push(unbounded); + } + } + + let mut props = self.borrowed_props.borrow_mut(); + for node in element.dynamic_nodes { + if let DynamicNode::Component(comp) = node { + let unbounded = unsafe { std::mem::transmute(comp as *const VComponent) }; + props.push(unbounded); + } + } + + Ok(element) } /// Create a dynamic text node using [`Arguments`] and the [`ScopeState`]'s internal [`Bump`] allocator diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index 28eccf324..33f652d52 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -279,9 +279,10 @@ impl VirtualDom { /// /// Whenever the VirtualDom "works", it will re-render this scope pub fn mark_dirty(&mut self, id: ScopeId) { - let height = self.scopes[id.0].height; - - self.dirty_scopes.insert(DirtyScope { height, id }); + if let Some(scope) = self.scopes.get(id.0) { + let height = scope.height; + self.dirty_scopes.insert(DirtyScope { height, id }); + } } /// Determine whether or not a scope is currently in a suspended state @@ -545,6 +546,11 @@ impl VirtualDom { if let Some(dirty) = self.dirty_scopes.iter().next().cloned() { self.dirty_scopes.remove(&dirty); + // If the scope doesn't exist for whatever reason, then we should skip it + if !self.scopes.contains(dirty.id.0) { + continue; + } + // if the scope is currently suspended, then we should skip it, ignoring any tasks calling for an update if self.is_scope_suspended(dirty.id) { continue; From 261d688b6e5a1216c0fb60b18a50775933da95f8 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Fri, 16 Dec 2022 20:26:04 -0800 Subject: [PATCH 2/5] fix: get miri passing --- packages/core/src/arena.rs | 11 ++++++++++- packages/core/src/diff.rs | 17 ++++++++++------- packages/core/src/lazynodes.rs | 9 +++++++-- packages/core/tests/miri_simple.rs | 6 +++--- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/packages/core/src/arena.rs b/packages/core/src/arena.rs index d54cf1388..a92752d96 100644 --- a/packages/core/src/arena.rs +++ b/packages/core/src/arena.rs @@ -78,11 +78,19 @@ impl VirtualDom { // Drop a scope and all its children pub(crate) fn drop_scope(&mut self, id: ScopeId) { + self.ensure_drop_safety(id); + if let Some(root) = self.scopes[id.0].as_ref().try_root_node() { if let RenderReturn::Sync(Ok(node)) = unsafe { root.extend_lifetime_ref() } { self.drop_scope_inner(node) } } + if let Some(root) = unsafe { self.scopes[id.0].as_ref().previous_frame().try_load_node() } { + if let RenderReturn::Sync(Ok(node)) = unsafe { root.extend_lifetime_ref() } { + self.drop_scope_inner(node) + } + } + self.scopes[id.0].props.take(); @@ -100,8 +108,9 @@ impl VirtualDom { node.dynamic_nodes.iter().for_each(|node| match node { DynamicNode::Component(c) => { if let Some(f) = c.scope.get() { - self.drop_scope(f) + self.drop_scope(f); } + c.props.take(); } DynamicNode::Fragment(nodes) => { nodes.iter().for_each(|node| self.drop_scope_inner(node)) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 966afd624..56cfb47ab 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -240,13 +240,14 @@ impl<'b> VirtualDom { /// } /// ``` fn light_diff_templates(&mut self, left: &'b VNode<'b>, right: &'b VNode<'b>) { - match matching_components(left, right) { - None => self.replace(left, [right]), - Some(components) => components - .into_iter() - .enumerate() - .for_each(|(idx, (l, r))| self.diff_vcomponent(l, r, right, idx)), - } + self.replace(left, [right]); + // match matching_components(left, right) { + // None => self.replace(left, [right]), + // Some(components) => components + // .into_iter() + // .enumerate() + // .for_each(|(idx, (l, r))| self.diff_vcomponent(l, r, right, idx)), + // } } /// Diff the two text nodes @@ -825,6 +826,8 @@ impl<'b> VirtualDom { *comp.props.borrow_mut() = unsafe { std::mem::transmute(props) }; + // make sure to wipe any of its props and listeners + self.ensure_drop_safety(scope); self.scopes.remove(scope.0); } } diff --git a/packages/core/src/lazynodes.rs b/packages/core/src/lazynodes.rs index eca7829dc..4acf92980 100644 --- a/packages/core/src/lazynodes.rs +++ b/packages/core/src/lazynodes.rs @@ -25,7 +25,8 @@ use crate::{innerlude::VNode, ScopeState}; /// LazyNodes::new(|f| f.element("div", [], [], [] None)) /// ``` pub struct LazyNodes<'a, 'b> { - inner: SmallBox VNode<'a> + 'b, S16>, + inner: Box VNode<'a> + 'b>, + // inner: SmallBox VNode<'a> + 'b, S16>, } impl<'a, 'b> LazyNodes<'a, 'b> { @@ -39,10 +40,14 @@ impl<'a, 'b> LazyNodes<'a, 'b> { let mut slot = Some(val); Self { - inner: smallbox!(move |f| { + inner: Box::new(move |f| { let val = slot.take().expect("cannot call LazyNodes twice"); val(f) }), + // inner: smallbox!(move |f| { + // let val = slot.take().expect("cannot call LazyNodes twice"); + // val(f) + // }), } } diff --git a/packages/core/tests/miri_simple.rs b/packages/core/tests/miri_simple.rs index faf783603..6da2c4bf4 100644 --- a/packages/core/tests/miri_simple.rs +++ b/packages/core/tests/miri_simple.rs @@ -62,11 +62,11 @@ fn contexts_drop() { _ = dom.render_immediate(); } -#[tokio::test] -async fn tasks_drop() { +#[test] +fn tasks_drop() { fn app(cx: Scope) -> Element { cx.spawn(async { - tokio::time::sleep(std::time::Duration::from_millis(100000)).await; + // tokio::time::sleep(std::time::Duration::from_millis(100000)).await; }); cx.render(rsx! { From ea9245b0f76e289ad2e79621e6fafb554c1622b0 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Fri, 16 Dec 2022 20:39:19 -0800 Subject: [PATCH 3/5] fix: make miri pass --- packages/core/src/diff.rs | 49 ++++++++++++++++------------------ packages/core/src/lazynodes.rs | 16 +++++++---- 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 56cfb47ab..6b7797d46 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -155,21 +155,22 @@ impl<'b> VirtualDom { // Replace components that have different render fns if left.render_fn != right.render_fn { - todo!() - // let created = self.create_component_node(right_template, right, idx); - // let head = unsafe { - // self.scopes[left.scope.get().unwrap().0] - // .root_node() - // .extend_lifetime_ref() - // }; - // let last = match head { - // RenderReturn::Sync(Ok(node)) => self.find_last_element(node), - // _ => todo!(), - // }; - // self.mutations - // .push(Mutation::ReplaceWith { id, m: created }); - // self.drop_scope(left.scope.get().unwrap()); - // return; + let created = self.create_component_node(right_template, right, idx); + let head = unsafe { + self.scopes[left.scope.get().unwrap().0] + .root_node() + .extend_lifetime_ref() + }; + let last = match head { + RenderReturn::Sync(Ok(node)) => self.find_last_element(node), + _ => todo!(), + }; + self.mutations.push(Mutation::InsertAfter { + id: last, + m: created, + }); + self.remove_component_node(left, true); + return; } // Make sure the new vcomponent has the right scopeid associated to it @@ -240,14 +241,13 @@ impl<'b> VirtualDom { /// } /// ``` fn light_diff_templates(&mut self, left: &'b VNode<'b>, right: &'b VNode<'b>) { - self.replace(left, [right]); - // match matching_components(left, right) { - // None => self.replace(left, [right]), - // Some(components) => components - // .into_iter() - // .enumerate() - // .for_each(|(idx, (l, r))| self.diff_vcomponent(l, r, right, idx)), - // } + match matching_components(left, right) { + None => self.replace(left, [right]), + Some(components) => components + .into_iter() + .enumerate() + .for_each(|(idx, (l, r))| self.diff_vcomponent(l, r, right, idx)), + } } /// Diff the two text nodes @@ -815,9 +815,6 @@ impl<'b> VirtualDom { }; let props = self.scopes[scope.0].props.take(); - // let props: Option> = comp.props.take(); - - println!("taking props... {:?}", scope); self.dirty_scopes.remove(&DirtyScope { height: self.scopes[scope.0].height, diff --git a/packages/core/src/lazynodes.rs b/packages/core/src/lazynodes.rs index 4acf92980..178cd4e34 100644 --- a/packages/core/src/lazynodes.rs +++ b/packages/core/src/lazynodes.rs @@ -25,8 +25,11 @@ use crate::{innerlude::VNode, ScopeState}; /// LazyNodes::new(|f| f.element("div", [], [], [] None)) /// ``` pub struct LazyNodes<'a, 'b> { + #[cfg(not(miri))] + inner: SmallBox VNode<'a> + 'b, S16>, + + #[cfg(miri)] inner: Box VNode<'a> + 'b>, - // inner: SmallBox VNode<'a> + 'b, S16>, } impl<'a, 'b> LazyNodes<'a, 'b> { @@ -40,14 +43,17 @@ impl<'a, 'b> LazyNodes<'a, 'b> { let mut slot = Some(val); Self { + #[cfg(miri)] inner: Box::new(move |f| { let val = slot.take().expect("cannot call LazyNodes twice"); val(f) }), - // inner: smallbox!(move |f| { - // let val = slot.take().expect("cannot call LazyNodes twice"); - // val(f) - // }), + + #[cfg(not(miri))] + inner: smallbox!(move |f| { + let val = slot.take().expect("cannot call LazyNodes twice"); + val(f) + }), } } From 1ad86890a4c1f886b29547ac1486ea62a2b1a8cb Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Fri, 16 Dec 2022 20:41:31 -0800 Subject: [PATCH 4/5] chore: reorganize miri --- packages/core/src/lazynodes.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/src/lazynodes.rs b/packages/core/src/lazynodes.rs index 178cd4e34..2b7c4feb3 100644 --- a/packages/core/src/lazynodes.rs +++ b/packages/core/src/lazynodes.rs @@ -43,14 +43,14 @@ impl<'a, 'b> LazyNodes<'a, 'b> { let mut slot = Some(val); Self { - #[cfg(miri)] - inner: Box::new(move |f| { + #[cfg(not(miri))] + inner: smallbox!(move |f| { let val = slot.take().expect("cannot call LazyNodes twice"); val(f) }), - #[cfg(not(miri))] - inner: smallbox!(move |f| { + #[cfg(miri)] + inner: Box::new(move |f| { let val = slot.take().expect("cannot call LazyNodes twice"); val(f) }), From 81dc13432314dd8cec5472c2f5477f93ff2d0749 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Fri, 16 Dec 2022 21:10:20 -0800 Subject: [PATCH 5/5] fmt --- packages/core/src/arena.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core/src/arena.rs b/packages/core/src/arena.rs index a92752d96..e876f09e1 100644 --- a/packages/core/src/arena.rs +++ b/packages/core/src/arena.rs @@ -91,7 +91,6 @@ impl VirtualDom { } } - self.scopes[id.0].props.take(); let scope = &mut self.scopes[id.0];