Reduce the use of unsafe in corelib and in the rust backend

api/sixtyfps-cpp/include/sixtyfps.h

@@ -43,17 +43,16 @@ using internal::TouchArea;
 // these on the heap
 inline void dummy_destory(ComponentRef) { }
 
-constexpr inline ItemTreeNode make_item_node(std::intptr_t offset,
+constexpr inline ItemTreeNode<uint8_t> make_item_node(std::uintptr_t offset,
                                              const internal::ItemVTable *vtable,
                                              uint32_t child_count, uint32_t child_index)
 {
-    return ItemTreeNode { ItemTreeNode::Tag::Item,
+    return ItemTreeNode<uint8_t> { ItemTreeNode<uint8_t>::Tag::Item,
-                          { ItemTreeNode::Item_Body { offset, vtable, child_count,
+                          { ItemTreeNode<uint8_t>::Item_Body { {vtable, offset}, child_count,
                                                       child_index } } };
 }
 
-// FIXME: this should have a better name
+using internal::sixtyfps_visit_item_tree;
-using internal::visit_item_tree;
 
 // layouts:
 using internal::Slice;

api/sixtyfps-cpp/include/vtable.h

@@ -26,3 +26,10 @@ struct VRef {
     const void *instance;
 };
 */
+
+template<typename Base, typename T>
+struct VOffset
+{
+    const T *vtable;
+    uintptr_t offset;
+};

api/sixtyfps-rs/lib.rs

@@ -75,6 +75,7 @@ pub mod re_exports {
     pub use sixtyfps_corelib::abi::properties::Property;
     pub use sixtyfps_corelib::abi::signals::Signal;
     pub use sixtyfps_corelib::abi::slice::Slice;
+    pub use sixtyfps_corelib::item_tree::visit_item_tree;
     pub use sixtyfps_corelib::layout::{
         solve_grid_layout, Constraint, GridLayoutCellData, GridLayoutData,
     };

helper_crates/vtable/src/lib.rs

@@ -405,7 +405,7 @@ macro_rules! new_vref {
 /// Represent an offset to a field of type mathcing the vtable, within the Base container structure.
 #[repr(C)]
 pub struct VOffset<Base, T: ?Sized + VTableMeta> {
-    vtable: *const T::VTable,
+    vtable: &'static T::VTable,
     /// Safety invariant: the vtable is valid, and the field at the given offset within Base is
     /// matching with the vtable
     offset: usize,
@@ -418,7 +418,7 @@ impl<Base, T: ?Sized + VTableMeta> VOffset<Base, T> {
         let ptr = x as *const Base as *const u8;
         unsafe {
             VRef::from_raw(
-                NonNull::new_unchecked(self.vtable as *mut _),
+                NonNull::from(self.vtable),
                 NonNull::new_unchecked(ptr.add(self.offset) as *mut _),
             )
         }
@@ -429,18 +429,21 @@ impl<Base, T: ?Sized + VTableMeta> VOffset<Base, T> {
         let ptr = x as *mut Base as *mut u8;
         unsafe {
             VRefMut::from_raw(
-                NonNull::new_unchecked(self.vtable as *mut _),
+                NonNull::from(self.vtable),
                 NonNull::new_unchecked(ptr.add(self.offset)),
             )
         }
     }
 
     pub fn new<X: HasStaticVTable<T>>(o: FieldOffset<Base, X>) -> Self {
-        Self {
+        Self { vtable: X::static_vtable(), offset: o.get_byte_offset(), phantom: PhantomData }
-            vtable: X::static_vtable() as *const T::VTable,
-            offset: o.get_byte_offset(),
-            phantom: PhantomData,
     }
+
+    /// Create a new VOffset from raw data
+    ///
+    /// Safety: there must be a field that matches the vtable at offset T in base
+    pub unsafe fn from_raw(vtable: &'static T::VTable, offset: usize) -> Self {
+        Self { vtable, offset, phantom: PhantomData }
     }
 }
 

sixtyfps_compiler/generator/cpp.rs

@@ -328,9 +328,9 @@ pub fn generate(component: &Component, diag: &mut Diagnostics) -> Option<impl st
         name: format!("{}::visit_children", component.id),
         signature: "(sixtyfps::ComponentRef component, intptr_t index, sixtyfps::ItemVisitorRefMut visitor) -> void".into(),
         statements: Some(vec![
-            "static const sixtyfps::ItemTreeNode children[] {".to_owned(),
+            "static const sixtyfps::ItemTreeNode<uint8_t> children[] {".to_owned(),
             format!("    {} }};", tree_array),
-            "return sixtyfps::visit_item_tree(component, { const_cast<sixtyfps::ItemTreeNode*>(children), std::size(children)}, index, visitor);".to_owned(),
+            "return sixtyfps::sixtyfps_visit_item_tree(component, { const_cast<sixtyfps::ItemTreeNode<uint8_t>*>(children), std::size(children)}, index, visitor);".to_owned(),
         ]),
         ..Default::default()
     }));

sixtyfps_compiler/generator/rust.rs

@@ -106,12 +106,10 @@ pub fn generate(component: &Component, diag: &mut Diagnostics) -> Option<TokenSt
     super::build_array_helper(component, |item, children_index| {
         let item = item.borrow();
         let field_name = quote::format_ident!("{}", item.id);
-        let vtable = quote::format_ident!("{}", item.base_type.as_builtin().vtable_symbol);
         let children_count = item.children.len() as u32;
         item_tree_array.push(quote!(
             sixtyfps::re_exports::ItemTreeNode::Item{
-                offset: #component_id::field_offsets().#field_name.get_byte_offset() as isize,
+                item: VOffset::new(#component_id::field_offsets().#field_name),
-                vtable: &#vtable as *const _,
                 chilren_count: #children_count,
                 children_index: #children_index,
              }
@@ -152,8 +150,6 @@ pub fn generate(component: &Component, diag: &mut Diagnostics) -> Option<TokenSt
         item_types.push(quote::format_ident!("{}", item.base_type.as_builtin().class_name));
     });
 
-    let item_tree_array_len = item_tree_array.len();
-
     let resource_symbols: Vec<proc_macro2::TokenStream> = component
         .embedded_file_resources
         .borrow()
@@ -194,8 +190,8 @@ pub fn generate(component: &Component, diag: &mut Diagnostics) -> Option<TokenSt
         impl sixtyfps::re_exports::Component for #component_id {
             fn visit_children_item(&self, index: isize, visitor: sixtyfps::re_exports::ItemVisitorRefMut) {
                 use sixtyfps::re_exports::*;
-                static TREE : [ItemTreeNode; #item_tree_array_len] = [#(#item_tree_array),*];
+                let tree = &[#(#item_tree_array),*];
-                unsafe { visit_item_tree(VRef::new(self), Slice::from_slice(&TREE), index, visitor) };
+                sixtyfps::re_exports::visit_item_tree(self, VRef::new(self), tree, index, visitor);
             }
             fn create() -> Self {
                 Default::default()

sixtyfps_runtime/corelib/abi/datastructures.rs

@@ -1,7 +1,6 @@
 //! This module contains the basic datastructures that are exposed to the C API
 
 use super::slice::Slice;
-use core::ptr::NonNull;
 use std::cell::Cell;
 use vtable::*;
 
@@ -82,13 +81,11 @@ impl CachedRenderingData {
 /// The item tree is an array of ItemTreeNode representing a static tree of items
 /// within a component.
 #[repr(C)]
-pub enum ItemTreeNode {
+pub enum ItemTreeNode<T> {
     /// Static item
     Item {
         /// byte offset where we can find the item (from the *ComponentImpl)
-        offset: isize,
+        item: vtable::VOffset<T, ItemVTable>,
-        /// virtual table of the item
-        vtable: *const ItemVTable,
 
         /// number of children
         chilren_count: u32,
@@ -118,9 +115,6 @@ pub enum ItemTreeNode {
     },
 }
 
-/// It is supposed to be in static array
-unsafe impl Sync for ItemTreeNode {}
-
 /// Items are the nodes in the render tree.
 #[vtable]
 #[repr(C)]
@@ -285,87 +279,6 @@ pub struct MouseEvent {
     pub what: MouseEventType,
 }
 
-/// Visit each items recursively
-///
-/// The state parametter returned by the visitor is passed to each children.
-pub fn visit_items<State>(
-    component: VRef<'_, ComponentVTable>,
-    mut visitor: impl FnMut(ItemRef<'_>, &State) -> State,
-    state: State,
-) {
-    visit_internal(component, &mut visitor, -1, &state)
-}
-
-fn visit_internal<State>(
-    component: VRef<'_, ComponentVTable>,
-    visitor: &mut impl FnMut(ItemRef<'_>, &State) -> State,
-    index: isize,
-    state: &State,
-) {
-    let mut actual_visitor =
-        |component: VRef<ComponentVTable>, index: isize, item: VRef<ItemVTable>| {
-            let s = visitor(item, state);
-            visit_internal(component, visitor, index, &s);
-        };
-    vtable::new_vref!(let mut actual_visitor : VRefMut<ItemVisitorVTable> for ItemVisitor = &mut actual_visitor);
-    component.visit_children_item(index, actual_visitor);
-}
-
-/// FIXME: this is just a layer to keep compatibility with the olde ItemTreeNode array, but there are better way to implement the visitor
-#[no_mangle]
-pub unsafe extern "C" fn visit_item_tree(
-    component: VRef<ComponentVTable>,
-    item_tree: Slice<ItemTreeNode>,
-    index: isize,
-    mut visitor: VRefMut<ItemVisitorVTable>,
-) {
-    let mut visit_at_index = |idx: usize| match &item_tree[idx] {
-        ItemTreeNode::Item { vtable, offset, .. } => {
-            let item = ItemRef::from_raw(
-                NonNull::new_unchecked(*vtable as *mut _),
-                NonNull::new_unchecked(component.as_ptr().offset(*offset) as *mut _),
-            );
-            visitor.visit_item(component, idx as isize, item);
-        }
-        _ => panic!(),
-    };
-    if index == -1 {
-        visit_at_index(0);
-    } else {
-        match &item_tree[index as usize] {
-            ItemTreeNode::Item { children_index, chilren_count, .. } => {
-                for c in *children_index..(*children_index + *chilren_count) {
-                    visit_at_index(c as usize);
-                }
-            }
-            ItemTreeNode::DynamicTree { .. } => todo!(),
-        };
-    };
-}
-
-// This is here because for some reason (rust bug?) the ItemVTable_static is not accessible in the other modules
-
-ItemVTable_static! {
-    /// The VTable for `Image`
-    #[no_mangle]
-    pub static ImageVTable for crate::abi::primitives::Image
-}
-ItemVTable_static! {
-    /// The VTable for `Rectangle`
-    #[no_mangle]
-    pub static RectangleVTable for crate::abi::primitives::Rectangle
-}
-ItemVTable_static! {
-    /// The VTable for `Text`
-    #[no_mangle]
-    pub static TextVTable for crate::abi::primitives::Text
-}
-ItemVTable_static! {
-    /// The VTable for `TouchArea`
-    #[no_mangle]
-    pub static TouchAreaVTable for crate::abi::primitives::TouchArea
-}
-
 #[repr(C)]
 #[vtable]
 /// Object to be passed in visit_item_children method of the Component.
@@ -396,3 +309,45 @@ impl<T: FnMut(VRef<ComponentVTable>, isize, VRef<ItemVTable>)> ItemVisitor for T
         self(component, index, item)
     }
 }
+
+/// Expose `crate::item_tree::visit_item_tree` to C++
+///
+/// Safety: Assume a correct implementation of the item_tree array
+#[no_mangle]
+pub unsafe extern "C" fn sixtyfps_visit_item_tree(
+    component: VRef<ComponentVTable>,
+    item_tree: Slice<ItemTreeNode<u8>>,
+    index: isize,
+    visitor: VRefMut<ItemVisitorVTable>,
+) {
+    crate::item_tree::visit_item_tree(
+        &*(component.as_ptr() as *const u8),
+        component,
+        item_tree.as_slice(),
+        index,
+        visitor,
+    )
+}
+
+// This is here because for some reason (rust bug?) the ItemVTable_static is not accessible in the other modules
+
+ItemVTable_static! {
+    /// The VTable for `Image`
+    #[no_mangle]
+    pub static ImageVTable for crate::abi::primitives::Image
+}
+ItemVTable_static! {
+    /// The VTable for `Rectangle`
+    #[no_mangle]
+    pub static RectangleVTable for crate::abi::primitives::Rectangle
+}
+ItemVTable_static! {
+    /// The VTable for `Text`
+    #[no_mangle]
+    pub static TextVTable for crate::abi::primitives::Text
+}
+ItemVTable_static! {
+    /// The VTable for `TouchArea`
+    #[no_mangle]
+    pub static TouchAreaVTable for crate::abi::primitives::TouchArea
+}

sixtyfps_runtime/corelib/build.rs

@@ -60,7 +60,7 @@ fn main() {
 
     let mut resource_config = config.clone();
     resource_config.export.include = vec!["Resource".into()];
-    resource_config.export.exclude = vec!["visit_item_tree".into()];
+    resource_config.export.exclude = vec!["sixtyfps_visit_item_tree".into()];
     resource_config.enumeration = cbindgen::EnumConfig {
         derive_tagged_enum_copy_assignment: true,
         derive_tagged_enum_copy_constructor: true,

sixtyfps_runtime/corelib/input.rs

@@ -14,7 +14,7 @@ pub fn process_mouse_event(
 ) {
     let offset = Vector2D::new(0., 0.);
 
-    crate::abi::datastructures::visit_items(
+    crate::item_tree::visit_items(
         component,
         |item, offset| {
             let geom = item.geometry(context);

sixtyfps_runtime/corelib/item_rendering.rs

@@ -45,7 +45,7 @@ pub(crate) fn render_component_items<Backend: GraphicsBackend>(
 ) {
     let transform = Matrix4::identity();
 
-    crate::abi::datastructures::visit_items(
+    crate::item_tree::visit_items(
         component,
         |item, transform| {
             let origin = item.geometry(context).origin;

sixtyfps_runtime/corelib/lib.rs

@@ -9,6 +9,7 @@ You should use the `sixtyfps` crate instead
 
 pub mod graphics;
 pub mod input;
+pub mod item_tree;
 pub mod layout;
 
 /// Things that are exposed to the C ABI
@@ -185,7 +186,7 @@ pub fn run_component<GraphicsBackend: graphics::GraphicsBackend + 'static>(
         component,
         move |component, mut rendering_primitives_builder, rendering_cache| {
             // Generate cached rendering data once
-            crate::abi::datastructures::visit_items(
+            crate::item_tree::visit_items(
                 component,
                 |item, _| {
                     let ctx = EvaluationContext { component };

sixtyfps_runtime/interpreter/dynamic_component.rs

@@ -2,6 +2,7 @@ use crate::{dynamic_type, eval};
 
 use core::convert::TryInto;
 use core::ptr::NonNull;
+use dynamic_type::Instance;
 use object_tree::ElementRc;
 use sixtyfps_compilerlib::typeregister::Type;
 use sixtyfps_compilerlib::*;
@@ -57,7 +58,7 @@ pub struct ComponentImpl {
 pub struct ComponentDescription {
     pub(crate) ct: ComponentVTable,
     dynamic_type: Rc<dynamic_type::TypeInfo>,
-    it: Vec<ItemTreeNode>,
+    it: Vec<ItemTreeNode<crate::dynamic_type::Instance>>,
     pub(crate) items: HashMap<String, ItemWithinComponent>,
     pub(crate) custom_properties: HashMap<String, PropertiesWithinComponent>,
     /// the usize is the offset within `mem` to the Signal<()>
@@ -66,12 +67,17 @@ pub struct ComponentDescription {
     pub(crate) original: object_tree::Document,
 }
 
-unsafe extern "C" fn visit_children_item(this: ComponentRef, index: isize, v: ItemVisitorRefMut) {
+unsafe extern "C" fn visit_children_item(
+    component: ComponentRef,
+    index: isize,
+    v: ItemVisitorRefMut,
+) {
     let component_type =
-        &*(this.get_vtable() as *const ComponentVTable as *const ComponentDescription);
+        &*(component.get_vtable() as *const ComponentVTable as *const ComponentDescription);
     let item_tree = &component_type.it;
-    sixtyfps_corelib::abi::datastructures::visit_item_tree(
+    sixtyfps_corelib::item_tree::visit_item_tree(
-        this,
+        &*(component.as_ptr() as *const Instance),
+        component,
         item_tree.as_slice().into(),
         index,
         v,
@@ -149,8 +155,7 @@ pub fn load(
         let rt = &rtti[&*item.base_type.as_builtin().class_name];
         let offset = builder.add_field(rt.type_info);
         tree_array.push(ItemTreeNode::Item {
-            offset: offset as isize,
+            item: unsafe { vtable::VOffset::from_raw(rt.vtable, offset) },
-            vtable: rt.vtable,
             children_index: child_offset,
             chilren_count: item.children.len() as _,
         });