refactor: simplify charm sync validation

packages/charm-sync/src/__tests__/patch.spec.luau

@@ -81,16 +81,18 @@ test("preserves array density after serdes", function()
 	local patches = mockArraySerdes(patch.diff(prevState, nextState))
 	local applied = patch.apply(prevState, patches)
 
-	for index in next, prevState do
+	for index in prevState do
 		assert(applied[index] == nextState[index], `expected {applied[index]} to be {nextState[index]}`)
 	end
 
-	for index in next, applied do
+	for index in applied do
 		assert(applied[index] == nextState[index], `expected {applied[index]} to be {nextState[index]}`)
 	end
 end)
 
 test("cannot preserve sparse arrays", function()
+	config.validatePatches = false
+
 	local prevState = {}
 	local nextState = { [2] = "foo" }
 
@@ -106,13 +108,13 @@ end)
 test("optionally fixes sparse arrays", function()
 	config.fixArrays = true
 
-	for index in next, patch.diff({ 1, 1, 1, 1, 1 }, { 2, 1, 1, 2 }) do
+	for index in patch.diff({ 1, 1, 1, 1, 1 }, { 2, 1, 1, 2 }) do
 		assert(type(index) == "string", `expected {type(index)} {index} to be string`)
 	end
 
 	config.fixArrays = false
 
-	for index in next, patch.diff({ 1, 1, 1, 1, 1 }, { 2, 1, 1, 2 }) do
+	for index in patch.diff({ 1, 1, 1, 1, 1 }, { 2, 1, 1, 2 }) do
 		assert(type(index) == "number", `expected {type(index)} {index} to be number`)
 	end
 end)
@@ -120,14 +122,14 @@ end)
 test("optionally validates state", function()
 	local success = pcall(function()
 		config.validatePatches = true
-		patch.diff({}, { [0.1] = true })
+		patch.diff({}, { [1] = 1, a = true })
 	end)
 
-	assert(not success, "should not allow non-integer keys")
+	assert(not success, "should not allow mixed tables")
 
 	success = pcall(function()
 		config.validatePatches = false
-		patch.diff({}, { [0.1] = true })
+		patch.diff({}, { [1] = 1, a = true })
 	end)
 
 	assert(success, "should skip validation if not requested")

packages/charm-sync/src/__tests__/validate.spec.luau

@@ -1,28 +1,18 @@
-local Charm = require("../../Charm")
 local suite = require("../../../../test/suite")
 local validate = require("../validate")
 
-local atom = Charm.atom
 local test = suite.test
 
 test("validates inputs", function()
 	local cases: { [string]: { value: any, expected: any } } = {
 		array = { value = { 1, 2, 3 }, expected = true },
-		array_sparse = { value = { 1, nil :: any, 3 }, expected = true },
-		array_sparse_invalid = { value = { [1] = 0, [3] = 0 }, expected = false },
-
-		map_decimal_key = { value = { [1.5] = 0 }, expected = false },
-		map_infinity_key = { value = { [math.huge] = true }, expected = false },
-		map_mixed_key = { value = { [1] = 0, a = 0 }, expected = false },
-
-		nested_atom = { value = atom(0), expected = false },
+		arraySparse = { value = { 1, nil :: any, 3 }, expected = true },
+		arraySparseInvalid = { value = { [1] = 0, [3] = 0 }, expected = false },
+		mixedTable = { value = { [1] = "foo", a = true }, expected = false },
 	}
 
-	for key, case in next, cases do
-		local success, result = pcall(function()
-			validate(case.value, key)
-		end)
-
+	for key, case in cases do
+		local success, result: any = pcall(validate, case.value)
 		if case.expected and not success then
 			error(`Expected {key} to be valid, threw {result}`)
 		elseif not case.expected and success then

packages/charm-sync/src/config.luau

@@ -13,8 +13,8 @@ return {
 	--- This option should be disabled if your network library uses a custom
 	--- serialization method (i.e. Zap, ByteNet) to prevent interference.
 	fixArrays = true,
-	--- When `true`, patches will be validated before being sent to clients to
-	--- enforce remote event limitations. Only checked in strict mode and if
+	--- When `true`, synced state containing unsafe sparse arrays or mixed
+	--- tables will throw an error. Only checked in strict mode and if
 	--- `fixArrays` is enabled. Defaults to `true`.
 	validatePatches = true,
 }

packages/charm-sync/src/index.d.ts

@@ -90,8 +90,8 @@ export const config: {
 	 */
 	fixArrays: boolean;
 	/**
-	 * When `true`, patches will be validated before being sent to clients to
-	 * enforce remote event limitations. Only checked in strict mode and if
+	 * When `true`, synced state containing unsafe sparse arrays or mixed
+	 * tables will throw an error. Only checked in strict mode and if
 	 * `fixArrays` is enabled. Defaults to `true`.
 	 *
 	 * @server

packages/charm-sync/src/patch.luau

@@ -74,15 +74,13 @@ local function diff(currentState: any, nextState: any): any
 	end
 
 	if config.fixArrays then
-		-- Coerce sparse array patches into dictionaries
+		-- Convert sparse array patches that could be misserialized
 		if currentState[1] ~= nil or nextState[1] ~= nil then
 			patches = stringifySparseArray(patches)
 		end
 
 		if globals.strict and config.validatePatches then
-			for key, value in patches do
-				validate(value, key)
-			end
+			validate(patches)
 		end
 	end
 

packages/charm-sync/src/validate.luau

@@ -1,66 +1,50 @@
-local SAFE_KEYS = { string = true, number = true }
-local UNSAFE_VALUES = { ["function"] = true, thread = true }
-
 local function isTableUnsafe(object: { [any]: any })
 	local key = next(object)
 
 	if key == nil then
-		-- An empty table is always safe
 		return false
 	end
 
 	local keyType = type(key)
-	local keyCount = 0
+	local maxKey = 0
 
-	-- Keys must either be all strings or all numbers
+	-- Keys must all be the same type
 	for key in object do
 		if type(key) ~= keyType then
 			return true
+		elseif keyType == "number" and key > maxKey then
+			maxKey = key
 		end
-		keyCount += 1
 	end
 
 	-- Array keys must be sequential
-	if keyType == "number" and keyCount > #object then
+	if keyType == "number" and maxKey > #object then
 		return true
 	end
 
 	return false
 end
 
---- Validates a value to ensure it can be synced over a remote event.
+--- Throws an error if the value is a table that cannot be synced via
+--- RemoteEvents or RemoteFunctions. This only aims to catch common mistakes
+--- with remote argument limitations, such as sparse arrays or mixed tables.
 ---
---- @param value The value to validate.
---- @param key The key of the value in the table.
+--- @param value The table value.
+--- @param key The key of the value.
 --- @error Throws an error if the value cannot be synced.
-local function validate(value: any, key: any)
-	local typeKey = type(key)
-	local typeValue = type(value)
-
-	if not SAFE_KEYS[typeKey] then
-		error(`Invalid key type '{typeKey}' at key '{key}'`)
-	elseif UNSAFE_VALUES[typeValue] then
-		error(`Invalid value type '{typeValue}' at key '{key}'`)
-	elseif typeValue == "table" then
-		if getmetatable(value) ~= nil then
-			error(`Cannot sync tables with metatables! Got {value} at key '{key}'`)
-		elseif isTableUnsafe(value) then
-			error(
-				`Cannot sync tables unsupported by remote events! The table was found at the key '{key}'.\n\n`
-					.. "This can be for the following reasons:\n"
-					.. "1. The object is an array with non-sequential keys\n"
-					.. "2. The object is a dictionary with mixed key types (e.g. string and number)\n\n"
-					.. "Read more: https://create.roblox.com/docs/scripting/events/remote#argument-limitations"
-			)
-		end
-	end
-
-	if typeKey == "number" then
-		if key == math.huge or key == -math.huge then
-			error("Cannot sync infinity as key")
-		elseif key ~= math.floor(key) then
-			error("Cannot sync non-integer number as key")
+local function validate(value: { [any]: any })
+	if isTableUnsafe(value) then
+		-- Silence warnings during testing
+		if game then
+			warn("Cannot sync table:", value)
 		end
+		error(
+			`Cannot sync tables unsupported by remote events! See the warning above for more info.\n\n`
+				.. "This can be for the following reasons:\n"
+				.. "1. The table is a sparse array with non-sequential indices\n"
+				.. "2. The table is a mixed table (not all keys have the same type)\n\n"
+				.. "Read more: https://create.roblox.com/docs/scripting/events/remote#argument-limitations"
+		)
 	end
 end