Updates for Ref properties specified with attributes are always null in /api/subscribe responses

[kennethloeffler]

To reproduce:

1. Run rojo plugin install
2. Run rojo serve on the ref_properties test project
3. Open a fresh baseplate in Roblox Studio, enable trace logging in the Rojo plugin, and click Connect
4. Make any change to ref_properties' default.project.json (I added a newline)
5. Observe the output, and notice that the subscribe response contains null for all the Ref properties specified with attributes in ref_properties' default.project.json:

{
  "sessionId": "d264957a-76ce-486b-8df6-7151de8da0c1",
  "messageCursor": 2,
  "messages": [
    {
      "removed": [],
      "added": {},
      "updated": [
        {
          "id": "516ae8375fd349b4a61ac47529c969cf",
          "changedName": null,
          "changedClassName": null,
          "changedProperties": {
            "Value": null
          },
          "changedMetadata": null
        },
        {
          "id": "1aee8e1cc88e674f9917cc95e9d56bf6",
          "changedName": null,
          "changedClassName": null,
          "changedProperties": {
            "Value": null
          },
          "changedMetadata": null
        },
        {
          "id": "5e3e89750a0826e8d5d83b1ee86cad1a",
          "changedName": null,
          "changedClassName": null,
          "changedProperties": {
            "PrimaryPart": null
          },
          "changedMetadata": null
        },
        {
          "id": "04b43e42134d58c42408ff2b9ba1c9a4",
          "changedName": null,
          "changedClassName": null,
          "changedProperties": {
            "Value": null
          },
          "changedMetadata": null
        }
      ]
    }
  ]
}

I noticed this behavior in some test failures after #843 was merged: https://github.com/rojo-rbx/rojo/actions/runs/9605402645/job/26492956529#step:7:762. This makes it impossible for the Rojo plugin to live update a Ref property, could cause problems once rbx_dom_lua gains better ability to represent optional values, and can currently cause test failures on macOS due to seemingly inherent noisiness in FSEvents.

One reason this could happen is because deferred Ref property application appears to not influence the returned result of patch application (i.e. PatchApplyContext.applied_patch_set), but rather only influences the tree:

rojo/src/snapshot/patch_apply.rs

Lines 246 to 299 in 7e2bab9

	fn defer_ref_properties(tree: &mut RojoTree, id: Ref, context: &mut PatchApplyContext) {
	let instance = tree
	.get_instance(id)
	.expect("Instances should exist when calculating deferred refs");
	let attributes = match instance.properties().get("Attributes") {
	Some(Variant::Attributes(attrs)) => attrs,
	_ => return,
	};
	let mut attr_id = None;
	for (attr_name, attr_value) in attributes.iter() {
	if attr_name == REF_ID_ATTRIBUTE_NAME {
	if let Variant::String(specified_id) = attr_value {
	attr_id = Some(RojoRef::new(specified_id.clone()));
	} else if let Variant::BinaryString(specified_id) = attr_value {
	if let Ok(str) = std::str::from_utf8(specified_id.as_ref()) {
	attr_id = Some(RojoRef::new(str.to_string()))
	} else {
	log::error!("Specified IDs must be valid UTF-8 strings.")
	}
	} else {
	log::warn!(
	"Attribute {attr_name} is of type {:?} when it was \
	expected to be a String",
	attr_value.ty()
	)
	}
	}
	if let Some(prop_name) = attr_name.strip_prefix(REF_POINTER_ATTRIBUTE_PREFIX) {
	if let Variant::String(prop_value) = attr_value {
	context
	.attribute_refs_to_rewrite
	.insert(id, (prop_name.to_owned(), prop_value.clone()));
	} else if let Variant::BinaryString(prop_value) = attr_value {
	if let Ok(str) = std::str::from_utf8(prop_value.as_ref()) {
	context
	.attribute_refs_to_rewrite
	.insert(id, (prop_name.to_owned(), str.to_string()));
	} else {
	log::error!("IDs specified by referent property attributes must be valid UTF-8 strings.")
	}
	} else {
	log::warn!(
	"Attribute {attr_name} is of type {:?} when it was \
	expected to be a String",
	attr_value.ty()
	)
	}
	}
	}
	if let Some(specified_id) = attr_id {
	tree.set_specified_id(id, specified_id);
	}
	}

It will also be worth investigating patch computation, to see if Ref properties specified via attributes are erroneously detected as removed there.

[Dekkonot]

I'll be completely honest: I'm not sure what part of the code I need to touch for this. I can do some backtracing to work out what part, but it might be easier to just ask if you know. So, what part of the code would I need to look in for this? Outside the patch application code, I mean.

[kennethloeffler]

The root cause seems to be that ref properties specified via attributes are not included in snapshots of the file system, i.e. when we get here

rojo/src/snapshot/patch_compute.rs

Lines 120 to 141 in 3ca975d

	for (name, snapshot_value) in take(&mut snapshot.properties) {
	visited_properties.insert(name.clone());
	match instance.properties().get(&name) {
	Some(instance_value) => {
	if &snapshot_value != instance_value {
	changed_properties.insert(name, Some(snapshot_value));
	}
	}
	None => {
	changed_properties.insert(name, Some(snapshot_value));
	}
	}
	}
	for name in instance.properties().keys() {
	if visited_properties.contains(name.as_str()) {
	continue;
	}
	changed_properties.insert(name.clone(), None);
	}

visited_properties is only populated with properties directly from the snapshot. Ref properties specified via attributes will not exist there, and will be detected as having changed to None in the second loop

why this happens makes sense - it's just that resolving ref properties here will require a slight design change