Support setting referent properties via attributes

[Dekkonot]

Ref properties have been neglected by Rojo for a long time, in no small part because of the potential complexity of their implementation. We've gotten by since they've been handled in rbxm and rbxmx files just fine but we should do better. This PR does the groundwork for us to do exactly that, by adding a system for manually specifying Ref properties with IDs and attributes.

I will be the first to say this: the UX of this approach is bad. However, I don't think there's actually a better way to support this universally. If an rbxm file needs to refer to a model.json Instance, that 'pointer' has to be stored inside the file somehow, which gives us relatively few options. Attributes are the only ones that are arbitrary key-value pairs, so it makes the most sense to use them. As a bonus, they persist through saving and loading into Studio so they'll be suitable for syncback and eventually two way sync.

Long-term, I want to support using IDs directly as properties in JSON files because it's a much better UX. I didn't want to do that in this PR though, because it will involve altering property resolution in some capacity and that's a potentially invasive change that I want to make stand-alone.

Usage

An ID for an Instance can be specified by putting an id/$id field in any of the JSON file formats, or by setting an attribute named Rojo_Id to a string.

This ID can be referred to by other Instances to set a Ref property via an attribute named Rojo_Target_PROP_NAME, where PROP_NAME is the name of a property. The attribute should be a string that references an ID defined elsewhere in the file. You can see an example of that in the tests files I included. Again, I don't love this UX but if anyone has better suggestions I'm happy to hear them.

Design Notes

RojoRef has been added and is a newtype surrounding Arc<String>. We may want to cache these eventually (at which point we could probably swap to using SharedString under the hood, though it's a Vec<u8>) but right now it feels like overkill to do so. However, they are cloned a fair bit so using Arc feels like the right call.

They're currently loaded during patch application because it's straightforward and attributes need to be traversed during that process anyway. I realize this might not be the best option though, so I do have an idea of how to do it 'properly' during the snapshotting process. It'd be a lot more invasive than this change though, so I defaulted to this one just to be safe.

---

Closes #427.

[Dekkonot]

Hm. The aftman timeout is concerning. Going to manually rerun the jobs just to confirm it isn't a bigger problem.

[Dekkonot]

Ok whatever it was, wasn't me. 👍🏼

[kennethloeffler]

Awesome, this has been a long-needed feature. It's looking mostly sound - a few comments:

[kennethloeffler]

I'm a little concerned that there are two different ways to specify IDs... it could be tricky to continue to ensure that everything stays consistent. This is mostly caused by the requirement to use attributes to represent refs (we absolutely need this for cross-model refs, right?), so I'm not sure there's much we can do to mitigate this :(

[Dekkonot]

I'm a little concerned that there are two different ways to specify IDs... it could be tricky to continue to ensure that everything stays consistent. This is mostly caused by the requirement to use attributes to represent refs (we absolutely need this for cross-model refs, right?), so I'm not sure there's much we can do to mitigate this :(

We need something like it. Since we want to be able to support pointing into and out of rbxms, there are very few options. The most obvious one is UniqueId, which we could use for Instance IDs, but that doesn't give us an easy way to specify properties still. Since we have to use attributes either way, we may as well use it for both.

I think it's probably fine to support both ways though. We have two ways to specify attributes in most if not all places, but that's not a real concern since people don't generally use both. It doesn't cause problems unless a user makes it a problem. If you're concerned we can make it an error if you do both though!

[kennethloeffler]

Let's also make sure to close the relevant issue for this feature: #427

[kennethloeffler]

I think we're pretty much done here - just need to fix angry rustfmt, and I'd like to try to break this one more time. I also noticed a few Ref sync bugs during the course of my testing, so I'll spin off to document/fix them shortly

[kennethloeffler]

Okay, so I can't find any more issues directly related to this PR, but there remain some problems one can run into when using the feature.

The first one I found is related to the order of addition operations within the Rojo plugin's reconciler:

rojo/plugin/src/Reconciler/applyPatch.lua

Lines 40 to 79 in 5c4260f

	for id, virtualInstance in pairs(patch.added) do
	if instanceMap.fromIds[id] ~= nil then
	-- This instance already exists. We might've already added it in a
	-- previous iteration of this loop, or maybe this patch was not
	-- supposed to list this instance.
	--
	-- It's probably fine, right?
	continue
	end
	-- Find the first ancestor of this instance that is marked for an
	-- addition.
	--
	-- This helps us make sure we only reify each instance once, and we
	-- start from the top.
	while patch.added[virtualInstance.Parent] ~= nil do
	id = virtualInstance.Parent
	virtualInstance = patch.added[id]
	end
	local parentInstance = instanceMap.fromIds[virtualInstance.Parent]
	if parentInstance == nil then
	-- This would be peculiar. If you create an instance with no
	-- parent, were you supposed to create it at all?
	invariant(
	"Cannot add an instance from a patch that has no parent.\nInstance {} with parent {}.\nState: {:#?}",
	id,
	virtualInstance.Parent,
	instanceMap
	)
	end
	local failedToReify = reify(instanceMap, patch.added, id, parentInstance)
	if not PatchSet.isEmpty(failedToReify) then
	Log.debug("Failed to reify as part of applying a patch: {:#?}", failedToReify)
	PatchSet.assign(unappliedPatch, failedToReify)
	end
	end

There's a problem at line 73. In this loop, we reify each incoming added instance one after the other, thereby creating each one with Instance.new and applying any properties (which includes Refs). It's possible to break this by having two sibling instances, and creating a Ref property on one that points to the other:

{
  "name": "big-bad-refs",
  "tree": {
    "$className": "DataModel",
    "ReplicatedStorage": {
      "MyCoolPart": {
        "$className": "Part",
        "$id": "MyCoolPart"
      },
      "ObjectValue": {
        "$className": "ObjectValue",
        "$attributes": {
          "Rojo_Target_Value": "MyCoolPart"
        }
      }
    }
  }
}

When I sync this project into Roblox Studio, it fails to apply ObjectValue.Value. If I disconnect and reconnect, then the property applies correctly, suggesting that the cause is related to the order of operations (i.e. MyCoolPart doesn't yet exist at the time at the time of the ObjectValue's reification). For correctness, we should be creating all added instances before attempting to apply Ref properties. We'll probably need to decouple instance creation and property application in this case.

We can either merge this PR, document the issue separately, and fix it sometime later, or else we can fix it right here in this PR. I'd go with the former (we don't want to add too much bloat), but I'll leave the final decision up to you.

[Dekkonot]

I'm inclined to agree with you about not touching the plugin code with this PR. I'm inclined to say we need to change that code anyway to support async in reify (for MeshParts/Unions) so it'll be fine to wait on it.

Where do you think we should document that? Intuition says an issue is the right place.

[kennethloeffler]

Yes, an issue will be fine

[Dekkonot]

Looking to get this merged soon if possible since it's a blocker on getting syncback closer to merging upstream @kennethloeffler

[kennethloeffler]

LGTM, thanks for the heads up!