Merge 6f52537e62 into af4eb5262d

packages/charm/src/init.luau

@@ -52,6 +52,8 @@ local RECURSED = 0b001000 -- Has been visited in a recursion check
 local DIRTY = 0b010000 -- Value is outdated and needs recomputation
 local PENDING = 0b100000 -- Might be dirty, pending verification
 
+local HAS_CHILD = 0b1000000 -- Dependencies include child effects or scopes
+
 -- Global version counter for dependency link deduplication. Incremented at the
 -- start of each effect/computed run to ensure links created in the same cycle
 -- are deduplicated.
@@ -171,6 +173,33 @@ local function purgeDeps(sub: ReactiveNode)
 	end
 end
 
+--[[
+	Unlinks child effects and effect scopes before a parent effect reruns.
+]]
+local function unlinkChildEffects(sub: ReactiveNode)
+	local current = sub.depsTail
+	while current do
+		local previous = current.prevDep
+		local dep = current.dep
+		if not bit32.btest(dep.flags, MUTABLE) then
+			unlink(current, sub)
+		end
+		current = previous
+	end
+end
+
+--[[
+	Unlinks all dependencies in LIFO order (newest to oldest).
+]]
+local function disposeAllDepsInReverse(sub: ReactiveNode)
+	local current = sub.depsTail
+	while current do
+		local previous = current.prevDep
+		unlink(current, sub)
+		current = previous
+	end
+end
+
 --[[
 	Runs all cleanup functions registered with the effect node. Any errors
 	thrown during cleanup are caught and logged in a single error after all
@@ -209,6 +238,10 @@ local function run(effect: EffectNode)
 		bit32.btest(effectFlags, DIRTY)
 		or (bit32.btest(effectFlags, PENDING) and checkDirty(effect.deps :: Link, effect))
 	then
+		if bit32.btest(effectFlags, HAS_CHILD) then
+			unlinkChildEffects(effect)
+		end
+
 		runCleanups(effect.cleanups)
 
 		-- Cancel the run if the effect was disposed during cleanup
@@ -238,7 +271,7 @@ local function run(effect: EffectNode)
 			runCleanups(effect.cleanups)
 		end
 	else
-		effect.flags = WATCHING
+		effect.flags = bit32.bor(WATCHING, bit32.band(effectFlags, HAS_CHILD))
 	end
 end
 
@@ -341,15 +374,14 @@ end
 	inactive, with all cleanup functions run and resources released.
 ]]
 local function stopEffect(effect: EffectScopeNode)
-	effect.depsTail = nil
 	effect.flags = NONE
 
-	purgeDeps(effect)
+	disposeAllDepsInReverse(effect)
 
 	-- Unlink from the parent effect, if it exists
 	local sub = effect.subs
 	if sub then
-		unlink(sub, effect)
+		unlink(sub)
 	end
 
 	runCleanups(effect.cleanups)
@@ -413,9 +445,8 @@ local function unwatched(node: ReactiveNode)
 	if bit32.band(node.flags, MUTABLE) == 0 then
 		stopEffect(node :: EffectScopeNode)
 	elseif node.depsTail then
-		node.depsTail = nil
 		node.flags = bit32.bor(MUTABLE, DIRTY)
-		purgeDeps(node)
+		disposeAllDepsInReverse(node)
 	end
 end
 
@@ -614,8 +645,10 @@ local function effect(fn: () -> ...Cleanup?): Cleanup
 	-- Inner effects are tracked as a dependency of the parent effect, so that
 	-- they can be automatically cleaned up when the parent effect re-runs, and
 	-- to ensure the correct execution order when nested effects are notified
-	if activeSub and (flags.trackInnerEffects or activeSub.flags == NONE) then
-		link(node, activeSub, 0)
+	local parent = activeSub
+	if parent and (flags.trackInnerEffects or bit32.band(parent.flags, bit32.bnot(HAS_CHILD)) == NONE) then
+		link(node, parent, 0)
+		parent.flags = bit32.bor(parent.flags, HAS_CHILD)
 	end
 
 	-- Start a batch so that the effect can register a cleanup function before
@@ -656,8 +689,10 @@ local function effectScope(fn: () -> ...Cleanup?, detached: boolean?): Cleanup
 	}
 
 	-- Inner effect scopes are tracked as a dependency of the parent effect
-	if activeSub and not detached then
-		link(node, activeSub, 0)
+	local parent = activeSub
+	if parent and not detached then
+		link(node, parent, 0)
+		parent.flags = bit32.bor(parent.flags, HAS_CHILD)
 	end
 
 	local prevSub = setActiveSub(node)

packages/charm/test/effect.test.luau

@@ -521,15 +521,59 @@ test("should effects dispose in order", function()
 
 	assert(#order == 0, "should not cleanup immediately")
 	setSrc(true)
-	assert(table.concat(order) == "123", `bad rerun order: {table.concat(order)}`)
+	assert(table.concat(order) == "321", `bad rerun order: {table.concat(order)}`)
 	table.clear(order)
 	stopEffect()
-	assert(table.concat(order) == "123", `bad dispose order: {table.concat(order)}`)
+	assert(table.concat(order) == "321", `bad dispose order: {table.concat(order)}`)
 	table.clear(order)
 	setSrc(false)
 	assert(#order == 0, "should not cleanup after dispose")
 end)
 
+test("should cleanup nested effects deepest first", function()
+	local order: { string } = {}
+
+	local stopEffect = effect(function()
+		effect(function()
+			effect(function()
+				return function()
+					table.insert(order, "grandchild")
+				end
+			end)
+			return function()
+				table.insert(order, "child")
+			end
+		end)
+		return function()
+			table.insert(order, "outer")
+		end
+	end)
+
+	stopEffect()
+	assert(table.concat(order, ", ") == "grandchild, child, outer", `bad dispose order: {table.concat(order, ", ")}`)
+end)
+
+test("should cleanup computed child effects in reverse when unwatched", function()
+	local order: { number } = {}
+	local c = computed(function()
+		for i = 1, 3 do
+			effect(function()
+				return function()
+					table.insert(order, i)
+				end
+			end)
+		end
+		return 0
+	end)
+
+	local stopEffect = effect(function()
+		c()
+	end)
+
+	stopEffect()
+	assert(table.concat(order) == "321", `bad dispose order: {table.concat(order)}`)
+end)
+
 test("should handle inner effect clearing outer effect's deps on rerun", function()
 	local src, setSrc = signal(false)
 	local stopEffect: () -> ()
@@ -545,7 +589,7 @@ test("should handle inner effect clearing outer effect's deps on rerun", functio
 	setSrc(true)
 end)
 
-test.skip("should handle inner effect cleanup disposing outer effect", function()
+test("should handle inner effect cleanup disposing outer effect", function()
 	local order: { number } = {}
 
 	local src, setSrc = signal(false)
@@ -566,28 +610,65 @@ test.skip("should handle inner effect cleanup disposing outer effect", function(
 
 	table.clear(order)
 	setSrc(true)
-	assert(table.concat(order) == "1234", `bad rerun order: {table.concat(order)}`)
+	assert(table.concat(order) == "4321", `bad rerun order: {table.concat(order)}`)
 
 	table.clear(order)
 	setSrc(false)
 	assert(#order == 0, "should not cleanup after dispose")
 end)
 
-test.skip("should old inner effect dispose before outer effect re-runs", function()
+test("should cleanup child before parent on rerun", function()
 	local order: { string } = {}
 	local src, setSrc = signal(false)
 	effect(function()
 		src()
-		table.insert(order, "outer")
+		table.insert(order, "outer run")
 		effect(function()
+			table.insert(order, "inner run")
 			return function()
-				table.insert(order, "inner")
+				table.insert(order, "inner cleanup")
 			end
 		end)
+		return function()
+			table.insert(order, "outer cleanup")
+		end
 	end)
+
 	table.clear(order)
 	setSrc(true)
-	assert(table.concat(order) == "innerouter", `bad rerun order: {table.concat(order)}`)
+	assert(
+		table.concat(order, ", ") == "inner cleanup, outer cleanup, outer run, inner run",
+		`bad rerun order: {table.concat(order, ", ")}`
+	)
+end)
+
+test("should preserve child tracking after an inner-only rerun", function()
+	local order: { string } = {}
+	local outerSrc, setOuterSrc = signal(false)
+	local innerSrc, setInnerSrc = signal(false)
+
+	effect(function()
+		outerSrc()
+		table.insert(order, "outer run")
+		effect(function()
+			innerSrc()
+			table.insert(order, "inner run")
+			return function()
+				table.insert(order, "inner cleanup")
+			end
+		end)
+		return function()
+			table.insert(order, "outer cleanup")
+		end
+	end)
+
+	setInnerSrc(true)
+	table.clear(order)
+	setOuterSrc(true)
+	assert(
+		table.concat(order, ", ") == "inner cleanup, outer cleanup, outer run, inner run",
+		`bad rerun order: {table.concat(order, ", ")}`
+	)
 end)
 
 return {}

packages/charm/test/effectScope.test.luau

@@ -92,4 +92,67 @@ test("should dispose inner effects if created in an effect", function()
 	assert(triggers == 2, "ran after dispose")
 end)
 
+test("should dispose child effects in reverse order", function()
+	local order: { number } = {}
+	local stopScope = effectScope(function()
+		for i = 1, 3 do
+			effect(function()
+				return function()
+					table.insert(order, i)
+				end
+			end)
+		end
+	end)
+
+	stopScope()
+	assert(table.concat(order) == "321", `bad dispose order: {table.concat(order)}`)
+end)
+
+test("should dispose nested effects deepest first", function()
+	local order: { string } = {}
+	local stopScope = effectScope(function()
+		effect(function()
+			effect(function()
+				return function()
+					table.insert(order, "grandchild")
+				end
+			end)
+			return function()
+				table.insert(order, "child")
+			end
+		end)
+	end)
+
+	stopScope()
+	assert(table.concat(order, ", ") == "grandchild, child", `bad dispose order: {table.concat(order, ", ")}`)
+end)
+
+test("should cleanup an intermediate scope before its parent effect", function()
+	local order: { string } = {}
+	local src, setSrc = signal(false)
+
+	effect(function()
+		src()
+		table.insert(order, "outer run")
+		effectScope(function()
+			effect(function()
+				table.insert(order, "inner run")
+				return function()
+					table.insert(order, "inner cleanup")
+				end
+			end)
+		end)
+		return function()
+			table.insert(order, "outer cleanup")
+		end
+	end)
+
+	table.clear(order)
+	setSrc(true)
+	assert(
+		table.concat(order, ", ") == "inner cleanup, outer cleanup, outer run, inner run",
+		`bad rerun order: {table.concat(order, ", ")}`
+	)
+end)
+
 return {}