Defining the Archivable property on an instance causes binary serializer to write false for all other instances' Archivable properties

[kennethloeffler]

Minimal repro:

use rbx_dom_weak::{InstanceBuilder, WeakDom};

fn main() {
    let dom = WeakDom::new(InstanceBuilder::new("Folder").with_children([
        InstanceBuilder::new("Folder").with_property("Archivable", true),
        InstanceBuilder::new("Folder"),
    ]));

    let file = std::fs::File::create("weird-archivable.rbxm").unwrap();
    rbx_binary::to_writer(file, &dom, &[dom.root_ref()]).unwrap()
}

Output of rbx-util view-binary weird-archivable.rbxm:

---
num_types: 1
num_instances: 3
chunks:
  - Inst:
      type_id: 0
      type_name: Folder
      object_format: 0
      referents:
        - 0
        - 1
        - 2
  - Prop:
      type_id: 0
      prop_name: archivable
      prop_type: Bool
      values:
        - false
        - true
        - false
  - Prop:
      type_id: 0
      prop_name: Name
      prop_type: String
      values:
        - Folder
        - Folder
        - Folder
  - Prnt:
      version: 0
      links:
        - - 2
          - 0
        - - 1
          - 0
        - - 0
          - -1
  - End

There are several things aligning that cause this and make it problematic:

• The reflection database is missing default values for Instance.Archivable. This is because Roblox Studio never writes this property, meaning rbx-reflector cannot discover its default value.
• Because the reflection database is missing default values for Instance.Archivable, the binary serializer writes the boolean fallback default (which is false) for instances that are missing the Archivable property.
• rbx-dom serializes Instance.Archivable asInstance.archivable.
• Roblox Studio does not load Instance.Archivable, but does load Instance.archivable. This means all the instances with Instance.archivable == false will in fact be loaded that way in Roblox Studio, and thus will not be saved when publishing to Roblox or when playtesting.

I thought of a few different ways we could solve this:

1. Don't serialize Instance.Archivable as Instance.archivable. This would still exhibit similar behavior, but since Roblox Studio does not load Instance.Archivable it wouldn't cause problems. The drawbacks are that it would be impossible to serialize an instance that has Archivable == false and have Roblox Studio load the property, and it would still be possible to define Instance.archivable and run into the same problematic behavior. Not a great option.
2. Allow rbx-reflector patches to define default values for properties, and patch in a default value for Instance.Archivable. No drawbacks that I can see, besides the burdens of implementation and maintenance.
3. Never serialize archivable properties. This would bring us mostly in line with Roblox's behavior, but it would be impossible to serialize an instance that has Archivable == false and have Roblox Studio load the property, which may be useful when users do not want to save test scripts or other instances that are unnecessary outside of development (here's an example in the wild: https://github.com/Aloroid/gorp/blob/main/src/stories/init.meta.json).

It seems like the only way that totally avoids breakage is 2.

[Dekkonot]

We probably don't want to serialize Archivable to begin with, but I don't want to mark properties unnecessarily as non-serializable, so 2 is the best option IMO.

I think the idea of Roblox deserializing Instance.archivable is really funny though. Under what circumstances would that even come up, ha ha?

[Dekkonot]

After giving this a shot, I've ran into a minor hiccup: the way we handle default properties is per-class right now. This means that in order to override a default for an abstract class like Instance, we'd need to override it for every class that inherits from it. That's a much bigger change than I was hoping for due to the nature of Instance.

I don't really have any good ideas to fix this. I have no problem cascading patches down to every inherited class, but it would mean we'd need to be careful with new patches to Instance's defaults since every one would result in a rather large addition to the database.

[kennethloeffler]

It is a little weird how we that information duplicated... but maybe instead of adding the default value patches to all subclasses, we could change the way rbx_binary's serializer finds default properties? e.g., replace this:

https://github.com/rojo-rbx/rbx-dom/blob/f6007d561bcde2d120922cec818c0765a00ec460/rbx_binary/src/serializer/state.rs#L401-L403

with a call to a function like:

    fn find_default_value(
        database: &'db ReflectionDatabase,
        mut class: &'db ClassDescriptor<'db>,
        canonical_property_name: &str,
    ) -> Option<&'db Variant> {
        loop {
            match class.default_properties.get(canonical_property_name) {
                None => {
                    class = database
                        .classes
                        .get(class.superclass.as_deref()?)
                        .expect("superclass that is Some should exist in reflection database")
                }
                default_value => return default_value,
            }
        }
    }

We wouldn't have to move any existing defaults if we did this (although it's probably a good idea to deduplicate all the defaults in the future), but it would work with the obvious implementation of property default value patches

[kennethloeffler]

Downside: more overhead for inherited properties (especially if we eventually do move default property values into their respective classes), since this does multiple hashmap lookups for each inherited property