avm2: Better trait verification

core/src/avm2/activation.rs

@@ -392,6 +392,7 @@ impl<'a, 'gc> Activation<'a, 'gc> {
                     abc_method,
                     body,
                 )?;
+
                 ClassObject::from_class(&mut dummy_activation, activation_class, None)
             })?;
 

core/src/avm2/class.rs

@@ -1,7 +1,7 @@
 //! AVM2 classes
 
 use crate::avm2::activation::Activation;
-use crate::avm2::error::{make_error_1014, Error1014Type};
+use crate::avm2::error::{make_error_1014, make_error_1053, verify_error, Error1014Type};
 use crate::avm2::method::{Method, NativeMethodImpl};
 use crate::avm2::object::{scriptobject_allocator, ClassObject, Object};
 use crate::avm2::script::TranslationUnit;
@@ -648,20 +648,48 @@ impl<'gc> Class<'gc> {
         Ok(())
     }
 
-    /// Completely validate a class against it's resolved superclass.
+    /// Completely validate a class against its resolved superclass.
     ///
     /// This should be called at class creation time once the superclass name
     /// has been resolved. It will return Ok for a valid class, and a
     /// VerifyError for any invalid class.
-    pub fn validate_class(self, superclass: Option<ClassObject<'gc>>) -> Result<(), Error<'gc>> {
+    pub fn validate_class(self, activation: &mut Activation<'_, 'gc>) -> Result<(), Error<'gc>> {
         let read = self.0.read();
 
         // System classes do not throw verify errors.
         if read.is_system {
             return Ok(());
         }
 
+        let superclass = read.super_class;
+
         if let Some(superclass) = superclass {
+            // We have to make an exception for `c_class`es
+            if superclass.is_final() && !self.is_c_class() {
+                return Err(Error::AvmError(verify_error(
+                    activation,
+                    &format!(
+                        "Error #1103: Class {} cannot extend final base class.",
+                        read.name.to_qualified_name(activation.gc())
+                    ),
+                    1103,
+                )?));
+            }
+
+            if superclass.is_interface() {
+                return Err(Error::AvmError(verify_error(
+                    activation,
+                    &format!(
+                        "Error #1110: Class {} cannot extend {}.",
+                        read.name.to_qualified_name(activation.gc()),
+                        superclass
+                            .name()
+                            .to_qualified_name_err_message(activation.gc())
+                    ),
+                    1110,
+                )?));
+            }
+
             for instance_trait in read.traits.iter() {
                 let is_protected = read.protected_namespace.is_some_and(|prot| {
                     prot.exact_version_match(instance_trait.name().namespace())
@@ -671,32 +699,72 @@ impl<'gc> Class<'gc> {
                 let mut did_override = false;
 
                 while let Some(superclass) = current_superclass {
-                    let superclass_def = superclass.inner_class_definition();
-
-                    for supertrait in &*superclass_def.traits() {
+                    for supertrait in &*superclass.traits() {
                         let super_name = supertrait.name();
                         let my_name = instance_trait.name();
 
                         let names_match = super_name.local_name() == my_name.local_name()
                             && (super_name.namespace().matches_ns(my_name.namespace())
                                 || (is_protected
-                                    && superclass_def.protected_namespace().is_some_and(|prot| {
+                                    && superclass.protected_namespace().is_some_and(|prot| {
                                         prot.exact_version_match(super_name.namespace())
                                     })));
                         if names_match {
                             match (supertrait.kind(), instance_trait.kind()) {
                                 //Getter/setter pairs do NOT override one another
                                 (TraitKind::Getter { .. }, TraitKind::Setter { .. }) => continue,
                                 (TraitKind::Setter { .. }, TraitKind::Getter { .. }) => continue,
-                                (_, _) => did_override = true,
-                            }
-
-                            if supertrait.is_final() {
-                                return Err(format!("VerifyError: Trait {} in class {} overrides final trait {} in class {}", instance_trait.name().local_name(), superclass_def.name().local_name(), supertrait.name().local_name(), superclass_def.name().local_name()).into());
-                            }
 
-                            if !instance_trait.is_override() {
-                                return Err(format!("VerifyError: Trait {} in class {} has same name as trait {} in class {}, but does not override it", instance_trait.name().local_name(), self.name().local_name(), supertrait.name().local_name(), superclass_def.name().local_name()).into());
+                                (_, TraitKind::Const { .. }) | (_, TraitKind::Slot { .. }) => {
+                                    did_override = true;
+
+                                    // Const/Var traits override anything in avmplus
+                                    // even if the base trait is marked as final or the
+                                    // overriding trait isn't marked as override.
+                                }
+                                (_, TraitKind::Class { .. }) => {
+                                    // Class traits aren't allowed in a class (except `global` classes)
+                                    return Err(Error::AvmError(verify_error(
+                                        activation,
+                                        "Error #1059: ClassInfo is referenced before definition.",
+                                        1059,
+                                    )?));
+                                }
+                                (TraitKind::Getter { .. }, TraitKind::Getter { .. })
+                                | (TraitKind::Setter { .. }, TraitKind::Setter { .. })
+                                | (TraitKind::Method { .. }, TraitKind::Method { .. }) => {
+                                    did_override = true;
+
+                                    if supertrait.is_final() {
+                                        return Err(make_error_1053(
+                                            activation,
+                                            instance_trait.name().local_name(),
+                                            read.name
+                                                .to_qualified_name_err_message(activation.gc()),
+                                        ));
+                                    }
+
+                                    if !instance_trait.is_override() {
+                                        return Err(make_error_1053(
+                                            activation,
+                                            instance_trait.name().local_name(),
+                                            read.name
+                                                .to_qualified_name_err_message(activation.gc()),
+                                        ));
+                                    }
+                                }
+                                (_, TraitKind::Getter { .. })
+                                | (_, TraitKind::Setter { .. })
+                                | (_, TraitKind::Method { .. }) => {
+                                    // FIXME: Getters, setters, and methods cannot override
+                                    // any other traits in FP. However, currently our
+                                    // playerglobals don't match FP closely enough;
+                                    // without explicitly allowing this, many SWFs VerifyError
+                                    // attempting to declare an overridden getter on what
+                                    // should be a getter in the subclass but which we
+                                    // declare as a field (such as MouseEvent.delta).
+                                    did_override = true;
+                                }
                             }
 
                             break;
@@ -709,11 +777,16 @@ impl<'gc> Class<'gc> {
                         break;
                     }
 
-                    current_superclass = superclass.superclass_object();
+                    current_superclass = superclass.super_class();
                 }
 
                 if instance_trait.is_override() && !did_override {
-                    return Err(format!("VerifyError: Trait {} in class {:?} marked as override, does not override any other trait", instance_trait.name().local_name(), read.name).into());
+                    return Err(make_error_1053(
+                        activation,
+                        instance_trait.name().local_name(),
+                        read.name
+                            .to_qualified_name_err_message(activation.context.gc_context),
+                    ));
                 }
             }
         }

core/src/avm2/error.rs

@@ -251,6 +251,25 @@ pub fn make_error_1033<'gc>(activation: &mut Activation<'_, 'gc>) -> Error<'gc>
     }
 }
 
+pub fn make_error_1053<'gc>(
+    activation: &mut Activation<'_, 'gc>,
+    trait_name: AvmString<'gc>,
+    class_name: AvmString<'gc>,
+) -> Error<'gc> {
+    let err = verify_error(
+        activation,
+        &format!(
+            "Error #1053: Illegal override of {} in {}.",
+            trait_name, class_name
+        ),
+        1053,
+    );
+    match err {
+        Ok(err) => Error::AvmError(err),
+        Err(err) => err,
+    }
+}
+
 #[inline(never)]
 #[cold]
 pub fn make_error_1054<'gc>(activation: &mut Activation<'_, 'gc>) -> Error<'gc> {

core/src/avm2/globals.rs

@@ -555,20 +555,20 @@ pub fn load_player_globals<'gc>(
     let gs = scope.chain(mc, &[Scope::new(globals.into())]);
     activation.set_outer(gs);
 
-    let object_class = ClassObject::from_class_partial(activation, object_i_class, None)?;
+    let object_class = ClassObject::from_class_partial(activation, object_i_class, None);
     let object_proto =
         ScriptObject::custom_object(mc, object_i_class, None, object_class.instance_vtable());
 
     let class_class =
-        ClassObject::from_class_partial(activation, class_i_class, Some(object_class))?;
+        ClassObject::from_class_partial(activation, class_i_class, Some(object_class));
     let class_proto = ScriptObject::custom_object(
         mc,
         object_i_class,
         Some(object_proto),
         object_class.instance_vtable(),
     );
 
-    let fn_class = ClassObject::from_class_partial(activation, fn_classdef, Some(object_class))?;
+    let fn_class = ClassObject::from_class_partial(activation, fn_classdef, Some(object_class));
     let fn_proto = ScriptObject::custom_object(
         mc,
         fn_classdef,
@@ -577,13 +577,13 @@ pub fn load_player_globals<'gc>(
     );
 
     // Now to weave the Gordian knot...
-    object_class.link_prototype(activation, object_proto)?;
+    object_class.link_prototype(activation, object_proto);
     object_class.link_type(mc, class_proto);
 
-    fn_class.link_prototype(activation, fn_proto)?;
+    fn_class.link_prototype(activation, fn_proto);
     fn_class.link_type(mc, class_proto);
 
-    class_class.link_prototype(activation, class_proto)?;
+    class_class.link_prototype(activation, class_proto);
     class_class.link_type(mc, class_proto);
 
     // At this point, we need at least a partial set of system classes in
@@ -607,7 +607,7 @@ pub fn load_player_globals<'gc>(
 
     // Function's prototype is an instance of itself
     let fn_proto = fn_class.construct(activation, &[])?.as_object().unwrap();
-    fn_class.link_prototype(activation, fn_proto)?;
+    fn_class.link_prototype(activation, fn_proto);
 
     // Object prototype is enough
     globals.set_proto(mc, object_class.prototype());

core/src/avm2/object/class_object.rs

@@ -90,14 +90,14 @@ impl<'gc> ClassObject<'gc> {
         self,
         activation: &mut Activation<'_, 'gc>,
         superclass_object: Option<ClassObject<'gc>>,
-    ) -> Result<Object<'gc>, Error<'gc>> {
+    ) -> Object<'gc> {
         let proto = ScriptObject::new_object(activation);
 
         if let Some(superclass_object) = superclass_object {
             let base_proto = superclass_object.prototype();
             proto.set_proto(activation.gc(), base_proto);
         }
-        Ok(proto)
+        proto
     }
 
     /// Construct a class.
@@ -114,10 +114,10 @@ impl<'gc> ClassObject<'gc> {
         class: Class<'gc>,
         superclass_object: Option<ClassObject<'gc>>,
     ) -> Result<Self, Error<'gc>> {
-        let class_object = Self::from_class_partial(activation, class, superclass_object)?;
-        let class_proto = class_object.allocate_prototype(activation, superclass_object)?;
+        let class_object = Self::from_class_partial(activation, class, superclass_object);
+        let class_proto = class_object.allocate_prototype(activation, superclass_object);
 
-        class_object.link_prototype(activation, class_proto)?;
+        class_object.link_prototype(activation, class_proto);
 
         let class_class_proto = activation.avm2().classes().class.prototype();
         class_object.link_type(activation.gc(), class_class_proto);
@@ -141,29 +141,12 @@ impl<'gc> ClassObject<'gc> {
         activation: &mut Activation<'_, 'gc>,
         class: Class<'gc>,
         superclass_object: Option<ClassObject<'gc>>,
-    ) -> Result<Self, Error<'gc>> {
+    ) -> Self {
         let c_class = class
             .c_class()
             .expect("Can only call ClassObject::from_class on i_classes");
 
         let scope = activation.create_scopechain();
-        if let Some(base_class) = superclass_object.map(|b| b.inner_class_definition()) {
-            if base_class.is_final() {
-                return Err(format!(
-                    "Base class {:?} is final and cannot be extended",
-                    base_class.name().local_name()
-                )
-                .into());
-            }
-
-            if base_class.is_interface() {
-                return Err(format!(
-                    "Base class {:?} is an interface and cannot be extended",
-                    base_class.name().local_name()
-                )
-                .into());
-            }
-        }
 
         let mc = activation.gc();
 
@@ -193,18 +176,16 @@ impl<'gc> ClassObject<'gc> {
             instance_scope
         )
         .set(instance_scope);
-        class_object.init_instance_vtable(activation)?;
+        class_object.init_instance_vtable(activation);
 
         class.add_class_object(activation.gc(), class_object);
 
-        Ok(class_object)
+        class_object
     }
 
-    fn init_instance_vtable(self, activation: &mut Activation<'_, 'gc>) -> Result<(), Error<'gc>> {
+    fn init_instance_vtable(self, activation: &mut Activation<'_, 'gc>) {
         let class = self.inner_class_definition();
 
-        class.validate_class(self.superclass_object())?;
-
         self.instance_vtable().init_vtable(
             class,
             self.superclass_object(),
@@ -213,9 +194,7 @@ impl<'gc> ClassObject<'gc> {
             activation.gc(),
         );
 
-        self.link_interfaces(activation)?;
-
-        Ok(())
+        self.link_interfaces(activation);
     }
 
     /// Finish initialization of the class.
@@ -263,26 +242,22 @@ impl<'gc> ClassObject<'gc> {
     }
 
     /// Link this class to a prototype.
-    pub fn link_prototype(
-        self,
-        activation: &mut Activation<'_, 'gc>,
-        class_proto: Object<'gc>,
-    ) -> Result<(), Error<'gc>> {
+    pub fn link_prototype(self, activation: &mut Activation<'_, 'gc>, class_proto: Object<'gc>) {
         let mc = activation.gc();
 
         unlock!(Gc::write(mc, self.0), ClassObjectData, prototype).set(Some(class_proto));
-        class_proto.set_string_property_local(istr!("constructor"), self.into(), activation)?;
+        class_proto
+            .set_string_property_local(istr!("constructor"), self.into(), activation)
+            .expect("Prototype is a dynamic object");
         class_proto.set_local_property_is_enumerable(mc, istr!("constructor"), false);
-
-        Ok(())
     }
 
     /// Link this class to it's interfaces.
     ///
     /// This should be done after all instance traits has been resolved, as
     /// instance traits will be resolved to their corresponding methods at this
     /// time.
-    pub fn link_interfaces(self, activation: &mut Activation<'_, 'gc>) -> Result<(), Error<'gc>> {
+    fn link_interfaces(self, activation: &mut Activation<'_, 'gc>) {
         let class = self.inner_class_definition();
 
         // FIXME - we should only be copying properties for newly-implemented
@@ -302,8 +277,6 @@ impl<'gc> ClassObject<'gc> {
                 }
             }
         }
-
-        Ok(())
     }
 
     /// Manually set the type of this `Class`.