avm2: Store PropertyClass in VtableData

Calling `get_trait` copies the returned `Property`, so the caching
we performed in `PropertyClass` was never actually getting used.

Instead, we now store our `PropertyClass` values in a `Vec`
indexed by slot id. `set_property` and `init_property` now perform
coercions by going through the `VTable,` which writes the updated
`PropertyClass` back into the array.

Author: Aaron1011

core/src/avm2/object.rs

@@ -142,8 +142,9 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into<Object<'gc>> + Clone + Copy
         activation: &mut Activation<'_, 'gc, '_>,
     ) -> Result<Value<'gc>, Error> {
         match self.vtable().and_then(|vtable| vtable.get_trait(multiname)) {
-            Some(Property::Slot { slot_id, class: _ })
-            | Some(Property::ConstSlot { slot_id, class: _ }) => self.base().get_slot(slot_id),
+            Some(Property::Slot { slot_id }) | Some(Property::ConstSlot { slot_id }) => {
+                self.base().get_slot(slot_id)
+            }
             Some(Property::Method { disp_id }) => {
                 if let Some(bound_method) = self.get_bound_method(disp_id) {
                     return Ok(bound_method.into());
@@ -197,8 +198,11 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into<Object<'gc>> + Clone + Copy
         activation: &mut Activation<'_, 'gc, '_>,
     ) -> Result<(), Error> {
         match self.vtable().and_then(|vtable| vtable.get_trait(multiname)) {
-            Some(Property::Slot { slot_id, mut class }) => {
-                let value = class.coerce(activation, value)?;
+            Some(Property::Slot { slot_id }) => {
+                let value = self
+                    .vtable()
+                    .unwrap()
+                    .coerce_trait_value(slot_id, value, activation)?;
                 self.base_mut(activation.context.gc_context).set_slot(
                     slot_id,
                     value,
@@ -247,9 +251,11 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into<Object<'gc>> + Clone + Copy
         activation: &mut Activation<'_, 'gc, '_>,
     ) -> Result<(), Error> {
         match self.vtable().and_then(|vtable| vtable.get_trait(multiname)) {
-            Some(Property::Slot { slot_id, mut class })
-            | Some(Property::ConstSlot { slot_id, mut class }) => {
-                let value = class.coerce(activation, value)?;
+            Some(Property::Slot { slot_id }) | Some(Property::ConstSlot { slot_id }) => {
+                let value = self
+                    .vtable()
+                    .unwrap()
+                    .coerce_trait_value(slot_id, value, activation)?;
                 self.base_mut(activation.context.gc_context).set_slot(
                     slot_id,
                     value,
@@ -303,8 +309,7 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into<Object<'gc>> + Clone + Copy
         activation: &mut Activation<'_, 'gc, '_>,
     ) -> Result<Value<'gc>, Error> {
         match self.vtable().and_then(|vtable| vtable.get_trait(multiname)) {
-            Some(Property::Slot { slot_id, class: _ })
-            | Some(Property::ConstSlot { slot_id, class: _ }) => {
+            Some(Property::Slot { slot_id }) | Some(Property::ConstSlot { slot_id }) => {
                 let obj = self.base().get_slot(slot_id)?.as_callable(
                     activation,
                     Some(multiname),

core/src/avm2/property.rs

@@ -11,22 +11,11 @@ use gc_arena::{Collect, Gc};
 
 #[derive(Debug, Collect, Clone, Copy)]
 #[collect(no_drop)]
-pub enum Property<'gc> {
-    Virtual {
-        get: Option<u32>,
-        set: Option<u32>,
-    },
-    Method {
-        disp_id: u32,
-    },
-    Slot {
-        slot_id: u32,
-        class: PropertyClass<'gc>,
-    },
-    ConstSlot {
-        slot_id: u32,
-        class: PropertyClass<'gc>,
-    },
+pub enum Property {
+    Virtual { get: Option<u32>, set: Option<u32> },
+    Method { disp_id: u32 },
+    Slot { slot_id: u32 },
+    ConstSlot { slot_id: u32 },
 }
 
 /// The type of a `Slot`/`ConstSlot` property, represented
@@ -42,7 +31,7 @@ pub enum Property<'gc> {
 /// Additionally, property class resolution uses special
 /// logic, different from normal "runtime" class resolution,
 /// that allows private types to be referenced.
-#[derive(Debug, Collect, Clone, Copy)]
+#[derive(Debug, Collect, Clone)]
 #[collect(no_drop)]
 pub enum PropertyClass<'gc> {
     /// The type `*` (Multiname::is_any()). This allows
@@ -61,24 +50,28 @@ impl<'gc> PropertyClass<'gc> {
     ) -> Self {
         PropertyClass::Name(Gc::allocate(activation.context.gc_context, (name, unit)))
     }
+
+    /// Returns `value` coerced to the type of this `PropertyClass`.
+    /// The bool is `true` if this `PropertyClass` was just modified
+    /// to cache a class resolution, and `false` if it was not modified.
     pub fn coerce(
         &mut self,
         activation: &mut Activation<'_, 'gc, '_>,
         value: Value<'gc>,
-    ) -> Result<Value<'gc>, Error> {
-        let class = match self {
-            PropertyClass::Class(class) => Some(*class),
+    ) -> Result<(Value<'gc>, bool), Error> {
+        let (class, changed) = match self {
+            PropertyClass::Class(class) => (Some(*class), false),
             PropertyClass::Name(gc) => {
                 let (name, unit) = &**gc;
                 let outcome = resolve_class_private(name, *unit, activation)?;
                 let class = match outcome {
                     ResolveOutcome::Class(class) => {
                         *self = PropertyClass::Class(class);
-                        Some(class)
+                        (Some(class), true)
                     }
                     ResolveOutcome::Any => {
                         *self = PropertyClass::Any;
-                        None
+                        (None, true)
                     }
                     ResolveOutcome::NotFound => {
                         // FP allows a class to reference its own type in a static initializer:
@@ -103,7 +96,9 @@ impl<'gc> PropertyClass<'gc> {
                                     && unit.map(|u| u.domain())
                                         == Some(class.class_scope().domain())
                                 {
-                                    return Ok(value);
+                                    // Even though resolution succeeded, we haven't modified
+                                    // this `PropertyClass`, so return `false`
+                                    return Ok((value, false));
                                 }
                             }
                         }
@@ -115,15 +110,15 @@ impl<'gc> PropertyClass<'gc> {
                 };
                 class
             }
-            PropertyClass::Any => None,
+            PropertyClass::Any => (None, false),
         };
 
         if let Some(class) = class {
-            value.coerce_to_type(activation, class)
+            Ok((value.coerce_to_type(activation, class)?, changed))
         } else {
             // We have a type of '*' ("any"), so don't
             // perform any coercions
-            Ok(value)
+            Ok((value, changed))
         }
     }
 }
@@ -191,7 +186,7 @@ fn resolve_class_private<'gc>(
     }
 }
 
-impl<'gc> Property<'gc> {
+impl Property {
     pub fn new_method(disp_id: u32) -> Self {
         Property::Method { disp_id }
     }
@@ -210,11 +205,11 @@ impl<'gc> Property<'gc> {
         }
     }
 
-    pub fn new_slot(slot_id: u32, class: PropertyClass<'gc>) -> Self {
-        Property::Slot { slot_id, class }
+    pub fn new_slot(slot_id: u32) -> Self {
+        Property::Slot { slot_id }
     }
 
-    pub fn new_const_slot(slot_id: u32, class: PropertyClass<'gc>) -> Self {
-        Property::ConstSlot { slot_id, class }
+    pub fn new_const_slot(slot_id: u32) -> Self {
+        Property::ConstSlot { slot_id }
     }
 }

core/src/avm2/vtable.rs

@@ -27,7 +27,11 @@ pub struct VTableData<'gc> {
 
     protected_namespace: Option<Namespace<'gc>>,
 
-    resolved_traits: PropertyMap<'gc, Property<'gc>>,
+    resolved_traits: PropertyMap<'gc, Property>,
+
+    /// Stores the `PropertyClass` for each slot,
+    /// indexed by `slot_id`
+    slot_classes: Vec<PropertyClass<'gc>>,
 
     method_table: Vec<ClassBoundMethod<'gc>>,
 
@@ -54,6 +58,7 @@ impl<'gc> VTable<'gc> {
                 scope: None,
                 protected_namespace: None,
                 resolved_traits: PropertyMap::new(),
+                slot_classes: vec![],
                 method_table: vec![],
                 default_slots: vec![],
             },
@@ -64,14 +69,34 @@ impl<'gc> VTable<'gc> {
         VTable(GcCell::allocate(mc, self.0.read().clone()))
     }
 
-    pub fn get_trait(self, name: &Multiname<'gc>) -> Option<Property<'gc>> {
+    pub fn get_trait(self, name: &Multiname<'gc>) -> Option<Property> {
         self.0
             .read()
             .resolved_traits
             .get_for_multiname(name)
             .cloned()
     }
 
+    /// Coerces `value` to the type of the slot with id `slot_id`
+    pub fn coerce_trait_value(
+        &self,
+        slot_id: u32,
+        value: Value<'gc>,
+        activation: &mut Activation<'_, 'gc, '_>,
+    ) -> Result<Value<'gc>, Error> {
+        // Drop the `write()` guard, as 'slot_class.coerce' may need to access this vtable.
+        let mut slot_class = { self.0.read().slot_classes[slot_id as usize].clone() };
+
+        let (value, changed) = slot_class.coerce(activation, value)?;
+
+        // Calling coerce modified `PropertyClass` to cache the class lookup,
+        // so store the new value back in the vtable.
+        if changed {
+            self.0.write(activation.context.gc_context).slot_classes[slot_id as usize] = slot_class;
+        }
+        Ok(value)
+    }
+
     pub fn has_trait(self, name: &Multiname<'gc>) -> bool {
         self.0
             .read()
@@ -175,6 +200,7 @@ impl<'gc> VTable<'gc> {
 
         if let Some(superclass_vtable) = superclass_vtable {
             write.resolved_traits = superclass_vtable.0.read().resolved_traits.clone();
+            write.slot_classes = superclass_vtable.0.read().slot_classes.clone();
             write.method_table = superclass_vtable.0.read().method_table.clone();
             write.default_slots = superclass_vtable.0.read().default_slots.clone();
 
@@ -195,10 +221,11 @@ impl<'gc> VTable<'gc> {
             }
         }
 
-        let (resolved_traits, method_table, default_slots) = (
+        let (resolved_traits, method_table, default_slots, slot_classes) = (
             &mut write.resolved_traits,
             &mut write.method_table,
             &mut write.default_slots,
+            &mut write.slot_classes,
         );
 
         for trait_data in traits {
@@ -299,31 +326,38 @@ impl<'gc> VTable<'gc> {
                         slot_id
                     };
 
-                    let new_prop = match trait_data.kind() {
+                    if new_slot_id as usize >= slot_classes.len() {
+                        // We will overwrite `PropertyClass::Any` when we process the slots
+                        // with the ids that we just skipped over.
+                        slot_classes.resize(new_slot_id as usize + 1, PropertyClass::Any);
+                    }
+
+                    let (new_prop, new_class) = match trait_data.kind() {
                         TraitKind::Slot {
                             type_name, unit, ..
-                        } => Property::new_slot(
-                            new_slot_id,
+                        } => (
+                            Property::new_slot(new_slot_id),
                             PropertyClass::name(activation, type_name.clone(), *unit),
                         ),
-                        TraitKind::Function { .. } => Property::new_slot(
-                            new_slot_id,
+                        TraitKind::Function { .. } => (
+                            Property::new_slot(new_slot_id),
                             PropertyClass::Class(activation.avm2().classes().function),
                         ),
                         TraitKind::Const {
                             type_name, unit, ..
-                        } => Property::new_const_slot(
-                            new_slot_id,
+                        } => (
+                            Property::new_const_slot(new_slot_id),
                             PropertyClass::name(activation, type_name.clone(), *unit),
                         ),
-                        TraitKind::Class { .. } => Property::new_const_slot(
-                            new_slot_id,
+                        TraitKind::Class { .. } => (
+                            Property::new_const_slot(new_slot_id),
                             PropertyClass::Class(activation.avm2().classes().class),
                         ),
                         _ => unreachable!(),
                     };
 
                     resolved_traits.insert(trait_data.name(), new_prop);
+                    slot_classes[new_slot_id as usize] = new_class;
                 }
             }
         }
@@ -379,10 +413,10 @@ impl<'gc> VTable<'gc> {
 
         write.default_slots.push(Some(value));
         let new_slot_id = write.default_slots.len() as u32 - 1;
-        write.resolved_traits.insert(
-            name,
-            Property::new_slot(new_slot_id, PropertyClass::Class(class)),
-        );
+        write
+            .resolved_traits
+            .insert(name, Property::new_slot(new_slot_id));
+        write.slot_classes.push(PropertyClass::Class(class));
 
         new_slot_id
     }