Revert "Deduplicate use of ptr_eq for Models"

This was not intended to get uploaded without review!

This reverts commit d678f4b1da.

CHANGELOG.md

@@ -9,8 +9,6 @@ All notable changes to this project will be documented in this file.
    has changed. This was undocumented, but if one was handling this in the
    `FocusScope` event, these keys will now be ignored. Use the `Keys.LeftArrow`
    and other code exposed in the `Keys` namespace instead
-- **Breaking:** Changed the base class of `ModelHandle` to be a `SharedModel` which wraps the
-   `Rc<dyn Model>` that was used before.
 
 ### Added
 

api/sixtyfps-node/native/lib.rs

@@ -188,7 +188,7 @@ fn to_eval_value<'cx>(
                 obj.get(cx, "rowCount")?.downcast_or_throw::<JsFunction, _>(cx)?;
                 obj.get(cx, "rowData")?.downcast_or_throw::<JsFunction, _>(cx)?;
                 let m = js_model::JsModel::new(obj, *a, cx, persistent_context)?;
-                Ok(Value::Model(sixtyfps_corelib::model::SharedModel(m)))
+                Ok(Value::Model(m))
             }
         },
         Type::Image => {

sixtyfps_runtime/corelib/model.rs

@@ -291,7 +291,7 @@ impl<T: 'static> VecModel<T> {
     where
         T: Clone,
     {
-        ModelHandle::new(Rc::<Self>::new(slice.to_vec().into()))
+        ModelHandle(Some(Rc::<Self>::new(slice.to_vec().into())))
     }
 
     /// Add a row at the end of the model
@@ -372,49 +372,14 @@ impl Model for bool {
     }
 }
 
-/// A shared model
-#[repr(transparent)]
-#[derive(derive_more::Deref, derive_more::DerefMut)]
-pub struct SharedModel<T>(pub Rc<dyn Model<Data = T>>);
-
-impl<T> std::fmt::Debug for SharedModel<T> {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "SharedModel(dyn Model)")
-    }
-}
-
-impl<T> Clone for SharedModel<T> {
-    fn clone(&self) -> Self {
-        Self(self.0.clone())
-    }
-}
-
-impl<T> core::cmp::PartialEq for SharedModel<T> {
-    #[allow(clippy::vtable_address_comparisons)]
-    // We think vtable_address_comparisons are safe here!
-    // Rc::ptr_eq will compare a fat pointer consisting of a `vtable` and a `self` pointer.
-    //
-    // We can get:
-    // * false positives: Two different `Model`s get considered the same, even though they are not.
-    //   For this the `self` pointer need to point to the same place. That can not happen here
-    //   since `Rc` will allocate for the refcounts, even if `T` is `()`.
-    // * false negatives: The same `Model` gets considered to be two different `Model`s.
-    //   In this case the `vtable` pointer must differ, e.g. because two crates created separate
-    //   instances of the vtable for the `Model` trait.
-    //   This will trigger some unnecessary rendering, but is benign otherwise.
-    fn eq(&self, other: &Self) -> bool {
-        Rc::ptr_eq(&self.0, &other.0)
-    }
-}
-
 /// Properties of type array in the .60 language are represented as
-/// an [`Option`] of an [`SharedModel`]
+/// an [`Option`] of an [`Rc`] of something implemented the [`Model`] trait
 #[derive(derive_more::Deref, derive_more::DerefMut, derive_more::From, derive_more::Into)]
-pub struct ModelHandle<T>(pub Option<SharedModel<T>>);
+pub struct ModelHandle<T>(pub Option<Rc<dyn Model<Data = T>>>);
 
 impl<T> core::fmt::Debug for ModelHandle<T> {
     fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
-        write!(f, "ModelHandle({:?})", self.0.as_ref())
+        write!(f, "ModelHandle({:?})", self.0.as_ref().map(|_| "dyn Model"))
     }
 }
 
@@ -433,22 +398,26 @@ impl<T> core::cmp::PartialEq for ModelHandle<T> {
     fn eq(&self, other: &Self) -> bool {
         match (&self.0, &other.0) {
             (None, None) => true,
-            (Some(a), Some(b)) => a == b,
-            _ => false,
+            (None, Some(_)) => false,
+            (Some(_), None) => false,
+            (Some(a), Some(b)) => core::ptr::eq(
+                (&**a) as *const dyn Model<Data = T> as *const u8,
+                (&**b) as *const dyn Model<Data = T> as *const u8,
+            ),
         }
     }
 }
 
 impl<T> From<Rc<dyn Model<Data = T>>> for ModelHandle<T> {
     fn from(x: Rc<dyn Model<Data = T>>) -> Self {
-        Self(Some(SharedModel(x)))
+        Self(Some(x))
     }
 }
 
 impl<T> ModelHandle<T> {
     /// Create a new handle wrapping the given model
     pub fn new(model: Rc<dyn Model<Data = T>>) -> Self {
-        Self(Some(SharedModel(model)))
+        Self(Some(model))
     }
 }
 

sixtyfps_runtime/interpreter/api.rs

@@ -97,7 +97,7 @@ pub enum Value {
     /// An Array in the .60 language.
     Array(SharedVector<Value>),
     /// A more complex model which is not created by the interpreter itself (Value::Array can also be used for models)
-    Model(sixtyfps_corelib::model::SharedModel<Value>),
+    Model(Rc<dyn sixtyfps_corelib::model::Model<Data = Value>>),
     /// An object
     Struct(Struct),
     /// Correspond to `brush` or `color` type in .60.  For color, this is then a [`Brush::SolidColor`]
@@ -149,7 +149,7 @@ impl PartialEq for Value {
             Value::Bool(lhs) => matches!(other, Value::Bool(rhs) if lhs == rhs),
             Value::Image(lhs) => matches!(other, Value::Image(rhs) if lhs == rhs),
             Value::Array(lhs) => matches!(other, Value::Array(rhs) if lhs == rhs),
-            Value::Model(lhs) => matches!(other, Value::Model(rhs) if lhs == rhs),
+            Value::Model(lhs) => matches!(other, Value::Model(rhs) if Rc::ptr_eq(lhs, rhs)),
             Value::Struct(lhs) => matches!(other, Value::Struct(rhs) if lhs == rhs),
             Value::Brush(lhs) => matches!(other, Value::Brush(rhs) if lhs == rhs),
             Value::PathElements(lhs) => matches!(other, Value::PathElements(rhs) if lhs == rhs),

sixtyfps_runtime/interpreter/dynamic_component.rs

@@ -1365,8 +1365,8 @@ pub fn instantiate(
                     InstanceRef::from_pin_ref(c, guard)
                 }),
             );
-            sixtyfps_corelib::model::ModelHandle::new(Rc::new(crate::value_model::ValueModel::new(
-                m,
+            sixtyfps_corelib::model::ModelHandle(Some(Rc::new(
+                crate::value_model::ValueModel::new(m),
             )))
         });
     }

sixtyfps_runtime/interpreter/ffi.rs

@@ -123,10 +123,7 @@ pub unsafe extern "C" fn sixtyfps_interpreter_value_new_model(
     model: vtable::VBox<ModelAdaptorVTable>,
     val: *mut ValueOpaque,
 ) {
-    std::ptr::write(
-        val as *mut Value,
-        Value::Model(sixtyfps_corelib::model::SharedModel(Rc::new(ModelAdaptorWrapper(model)))),
-    )
+    std::ptr::write(val as *mut Value, Value::Model(Rc::new(ModelAdaptorWrapper(model))))
 }
 
 #[no_mangle]