Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ref property application can fail when pointed-to instances don't yet exist #928

Open
kennethloeffler opened this issue Jun 20, 2024 · 0 comments
Labels
impact: medium Moderate issue for Rojo users or a large issue with a reasonable workaround. type: bug Something happens that shouldn't happen

Comments

@kennethloeffler
Copy link
Member

kennethloeffler commented Jun 20, 2024

This issue is related to the order of addition operations within the Rojo plugin's reconciler:

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.

Originally posted by @kennethloeffler in #843 (comment)

@kennethloeffler kennethloeffler added type: bug Something happens that shouldn't happen impact: medium Moderate issue for Rojo users or a large issue with a reasonable workaround. labels Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: medium Moderate issue for Rojo users or a large issue with a reasonable workaround. type: bug Something happens that shouldn't happen
Projects
None yet
Development

No branches or pull requests

1 participant