From 309958deedd1288e669a1b877acdef04a96bf765 Mon Sep 17 00:00:00 2001
From: Filip Tibell <filip.tibell@gmail.com>
Date: Sun, 26 Mar 2023 11:52:38 +0200
Subject: [PATCH] Fix instance reference property crash

---
 CHANGELOG.md                                  |  1 +
 .../lib-roblox/src/datatypes/conversion.rs    |  9 +++++-
 packages/lib-roblox/src/instance/mod.rs       | 30 +++++++++++++++----
 tests/roblox/instance/attributes.luau         | 12 ++++++--
 4 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 66b2fdd..5036641 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 -   Fixed error messages for reading & writing roblox files not containing the full error message
+-   Fixed crash when trying to access an instance reference property that points to a destroyed instance
 
 ## `0.6.3` - March 26th, 2023
 
diff --git a/packages/lib-roblox/src/datatypes/conversion.rs b/packages/lib-roblox/src/datatypes/conversion.rs
index 8d2a55b..692fe4c 100644
--- a/packages/lib-roblox/src/datatypes/conversion.rs
+++ b/packages/lib-roblox/src/datatypes/conversion.rs
@@ -55,6 +55,14 @@ impl<'lua> DomValueToLua<'lua> for LuaValue<'lua> {
                     lua.create_string(AsRef::<str>::as_ref(s))?,
                 )),
 
+                // NOTE: Dom references may point to instances that
+                // no longer exist, so we handle that here instead of
+                // in the userdata conversion to be able to return nils
+                DomValue::Ref(value) => match Instance::new_opt(*value) {
+                    Some(inst) => Ok(inst.to_lua(lua)?),
+                    None => Ok(LuaValue::Nil),
+                },
+
                 // NOTE: Some values are either optional or default and we should handle
                 // that properly here since the userdata conversion above will always fail
                 DomValue::OptionalCFrame(None) => Ok(LuaValue::Nil),
@@ -186,7 +194,6 @@ impl<'lua> DomValueToLua<'lua> for LuaAnyUserData<'lua> {
             DomValue::NumberSequence(value) => dom_to_userdata!(lua, value => NumberSequence),
             DomValue::Ray(value)            => dom_to_userdata!(lua, value => Ray),
             DomValue::Rect(value)           => dom_to_userdata!(lua, value => Rect),
-            DomValue::Ref(value)            => dom_to_userdata!(lua, value => Instance),
             DomValue::Region3(value)        => dom_to_userdata!(lua, value => Region3),
             DomValue::Region3int16(value)   => dom_to_userdata!(lua, value => Region3int16),
             DomValue::UDim(value)           => dom_to_userdata!(lua, value => UDim),
diff --git a/packages/lib-roblox/src/instance/mod.rs b/packages/lib-roblox/src/instance/mod.rs
index a164ff2..b7e480d 100644
--- a/packages/lib-roblox/src/instance/mod.rs
+++ b/packages/lib-roblox/src/instance/mod.rs
@@ -59,6 +59,30 @@ impl Instance {
         }
     }
 
+    /**
+        Creates a new `Instance` from a dom object ref, if the instance exists.
+
+        Panics if the given dom object ref points to the dom root.
+    */
+    pub(crate) fn new_opt(dom_ref: DomRef) -> Option<Self> {
+        let dom = INTERNAL_DOM
+            .try_read()
+            .expect("Failed to get read access to document");
+
+        if let Some(instance) = dom.get_by_ref(dom_ref) {
+            if instance.referent() == dom.root_ref() {
+                panic!("Instances can not be created from dom roots")
+            }
+
+            Some(Self {
+                dom_ref,
+                class_name: instance.class.clone(),
+            })
+        } else {
+            None
+        }
+    }
+
     /**
         Creates a new orphaned `Instance` with a given class name.
 
@@ -1103,9 +1127,3 @@ impl From<Instance> for DomRef {
         value.dom_ref
     }
 }
-
-impl From<DomRef> for Instance {
-    fn from(value: DomRef) -> Self {
-        Instance::new(value)
-    }
-}
diff --git a/tests/roblox/instance/attributes.luau b/tests/roblox/instance/attributes.luau
index f373aa5..5a0a61e 100644
--- a/tests/roblox/instance/attributes.luau
+++ b/tests/roblox/instance/attributes.luau
@@ -1,4 +1,6 @@
+local fs = require("@lune/fs") :: any
 local roblox = require("@lune/roblox") :: any
+
 local BrickColor = roblox.BrickColor
 local Color3 = roblox.Color3
 local ColorSequence = roblox.ColorSequence
@@ -11,12 +13,11 @@ local UDim = roblox.UDim
 local UDim2 = roblox.UDim2
 local Vector2 = roblox.Vector2
 local Vector3 = roblox.Vector3
-local CFrame = roblox.CFrame
+local Instance = roblox.Instance
 
 local model = roblox.readModelFile("tests/roblox/rbx-test-files/models/attributes/binary.rbxm")[1]
 
 model:SetAttribute("Foo", "Bar")
-model:SetAttribute("CFrame", CFrame.identity)
 
 local ATTRS_ACTUAL = model:GetAttributes()
 local ATTRS_EXPECTED: { [string]: any } = {
@@ -46,7 +47,6 @@ local ATTRS_EXPECTED: { [string]: any } = {
 	NaN = 0 / 0,
 	-- Extras we set
 	Foo = "Bar",
-	CFrame = CFrame.identity,
 }
 
 for name, value in ATTRS_EXPECTED do
@@ -65,3 +65,9 @@ for name, value in ATTRS_EXPECTED do
 		)
 	end
 end
+
+local game = Instance.new("DataModel")
+model.Parent = game
+
+fs.writeDir("bin/roblox")
+roblox.writePlaceFile("bin/roblox/attributes.rbxl", game)