let a host configure diagnostics

modules/signals/README.md

@@ -159,7 +159,8 @@ Creates a read-only reactive derived value.
 ```luau
 function createComputed<T>(
     computed: (scope) -> T,
-    equals: equals<T>?
+    equals: equals<T>?,
+    debugName: string?
 ): getter<T>
 ```
 
@@ -256,3 +257,15 @@ end)
 ```
 
 Use `false` or `nil` when you want to "peek" at a value without subscribing to changes, or when reading values outside of reactive contexts.
+
+## Host diagnostics
+
+`Signals.configure({ showInternals = true })` records state for sources created
+after configuration. `Signals.getDebugState(getter)` returns that state, or `nil`
+for a source created while the option was off. Debug names use the third argument
+of both `createSignal(initial, equals, debugName)` and
+`createComputed(fn, equals, debugName)`.
+
+`Signals.configure({ warnScopelessReads = true, report = sink })` reports a
+`nil`-scope read from inside a computed or effect body once per source line. A
+read outside a reactive body, or an intentional `get(false)` read, is silent.

modules/signals/src/Signals.lua

@@ -28,6 +28,45 @@ type source = (observer?, true?) -> number
 -- The "observer" function is used by a source to notify the observer that one of its sources may be stale
 type observer = () -> ()
 
+--[[
+	Diagnostics are configured by the host rather than compiled in, because what
+	counts as a problem depends on the runtime driving these signals, and none of
+	it should cost anything when it has not been asked for.
+]]
+export type Config = {
+	-- Records state for signals and computeds created while enabled. Inspect it
+	-- with `getDebugState`. Off by default: it is an allocation per source.
+	showInternals: boolean?,
+	-- Reports a read that named no scope.
+	warnScopelessReads: boolean?,
+	-- Where a report goes. Defaults to `warn`.
+	report: ((message: string) -> ())?,
+}
+
+local showInternals = false
+local warnScopelessReads = false
+local activeEvaluationDepth = 0
+local debugStates: { [any]: any } = setmetatable({}, { __mode = "k" }) :: any
+
+local function getDebugState(getter: any): any
+	return debugStates[getter]
+end
+local report: (message: string) -> () = function(message: string)
+	warn(message)
+end
+
+local function configure(options: Config)
+	if options.showInternals ~= nil then
+		showInternals = options.showInternals
+	end
+	if options.warnScopelessReads ~= nil then
+		warnScopelessReads = options.warnScopelessReads
+	end
+	if options.report ~= nil then
+		report = options.report
+	end
+end
+
 type set<T> = { [T]: true? }
 local WeakSetMetatable = table.freeze({ __mode = "k" })
 local function createWeakSet<T>(set: set<T>)
@@ -38,6 +77,21 @@ local function defaultEquals<T>(current: T, incoming: T)
 	return current == incoming
 end
 
+--[[
+	Reports a read that named no scope, so nothing re-runs when the value changes.
+
+	Deduplicated by source line, because the reads worth finding are the ones
+	inside an evaluation that runs repeatedly.
+]]
+local reportedScopelessReads: { [string]: true } = {}
+
+local function reportScopelessRead()
+	local source, line = debug.info(4, "sl")
+	local where = `{tostring(source)}:{tostring(line)}`
+	if reportedScopelessReads[where] == nil then
+		reportedScopelessReads[where] = true
+		report(`Signals: a source was read without naming a scope at {where}, so nothing re-runs when it changes`)
+	end
 -- A table carrying `__call` is callable when its metatable is visible.
 -- Protected metatables hide `__call`, so they return false here.
 local function isCallable(value: any): boolean
@@ -84,6 +138,21 @@ end
 local validationEnabled = _G.__SIGNALS_VALIDATION_ENABLED__ or _G.__DEV__
 local callUserSpaceWithScope = if validationEnabled then callUserSpaceWithScopeValidation else callUserSpace :: never
 
+local function evaluateWithScope(fn: (scope) -> any, scopeValue: scope): any
+	if not warnScopelessReads then
+		return callUserSpaceWithScope(fn, scopeValue)
+	end
+	activeEvaluationDepth += 1
+	local ok, result = xpcall(function()
+		return callUserSpaceWithScope(fn, scopeValue)
+	end, debug.traceback)
+	activeEvaluationDepth -= 1
+	if not ok then
+		error(result, 0)
+	end
+	return result
+end
+
 -- Keep the third position for the optional debug name added by the diagnostics
 -- branch, so the no-flush option has the same position when both land.
 local function createSignal<T>(
@@ -99,13 +168,19 @@ local function createSignal<T>(
 	local observers: set<observer>
 
 	local isEqual: equals<T> = if equals ~= nil then equals else defaultEquals
+	local debugState: any = if showInternals then { name = debugName, version = 0, value = initial } else nil
 
 	local function ensureInitialized()
 		if not isInitialized then
 			isInitialized = true
 			value = if typeof(initial) == "function" then callUserSpace(initial) else initial
 			version = os.clock()
 			observers = createWeakSet({})
+			if debugState then
+				debugState.version = version
+				debugState.value = value
+				debugState.observers = observers
+			end
 		end
 	end
 
@@ -130,6 +205,10 @@ local function createSignal<T>(
 			if childObserver ~= nil then
 				observers[childObserver] = true
 			end
+		elseif requestor == nil and warnScopelessReads and activeEvaluationDepth > 0 then
+			-- `false` is a deliberate untracked read. Omitting the argument entirely
+			-- is the mistake worth reporting.
+			reportScopelessRead()
 		end
 	end
 
@@ -157,17 +236,24 @@ local function createSignal<T>(
 		if not callUserSpace(isEqual, value, newValue) then
 			value = newValue
 			version = os.clock()
+			if debugState then
+				debugState.version = version
+				debugState.value = value
+			end
 			notifyObservers()
 			if options == nil or options.flushOnSet ~= false then
 				flush()
 			end
 		end
 	end
 
+	if debugState then
+		debugStates[getter] = debugState
+	end
 	return getter, setter
 end
 
-local function createComputed<T>(computed: (scope) -> T, equals: equals<T>?): getter<T>
+local function createComputed<T>(computed: (scope) -> T, equals: equals<T>?, debugName: string?): getter<T>
 	local isInitialized = false
 	local isStale = false
 	local cachedVersion = 0
@@ -178,6 +264,7 @@ local function createComputed<T>(computed: (scope) -> T, equals: equals<T>?): ge
 	local observers: set<observer>
 
 	local isEqual: equals<T> = if equals ~= nil then equals else defaultEquals
+	local debugState: any = if showInternals then { name = debugName, version = 0 } else nil
 
 	local function notifyObservers()
 		for childObserver in observers do
@@ -203,9 +290,13 @@ local function createComputed<T>(computed: (scope) -> T, equals: equals<T>?): ge
 			isInitialized = true
 			observers = createWeakSet({})
 			sources = {}
-			value = callUserSpaceWithScope(computed, scope)
+			value = evaluateWithScope(computed, scope)
 			absoluteVersion = os.clock()
 			cachedVersion = absoluteVersion
+			if debugState then
+				debugState.version = absoluteVersion
+				debugState.value = value
+			end
 		end
 	end
 
@@ -223,11 +314,15 @@ local function createComputed<T>(computed: (scope) -> T, equals: equals<T>?): ge
 				local newVersion = parentSource()
 				if newVersion > absoluteVersion then
 					disconnectSources()
-					local newValue = callUserSpaceWithScope(computed, scope)
+					local newValue = evaluateWithScope(computed, scope)
 					absoluteVersion = os.clock()
 					if not callUserSpace(isEqual, value, newValue) then
 						value = newValue
 						cachedVersion = absoluteVersion
+						if debugState then
+							debugState.version = absoluteVersion
+							debugState.value = value
+						end
 					end
 					return
 				end
@@ -260,6 +355,10 @@ local function createComputed<T>(computed: (scope) -> T, equals: equals<T>?): ge
 			if childObserver ~= nil then
 				observers[childObserver] = true
 			end
+		elseif requestor == nil and warnScopelessReads and activeEvaluationDepth > 0 then
+			-- `false` is a deliberate untracked read. Omitting the argument entirely
+			-- is the mistake worth reporting.
+			reportScopelessRead()
 		end
 	end
 
@@ -270,6 +369,9 @@ local function createComputed<T>(computed: (scope) -> T, equals: equals<T>?): ge
 		return value
 	end
 
+	if debugState then
+		debugStates[getter] = debugState
+	end
 	return getter
 end
 
@@ -306,7 +408,7 @@ local function createEffect(effect: (scope) -> ()): dispose
 				local newVersion = parentSource()
 				if newVersion > version then
 					disconnectSources()
-					callUserSpaceWithScope(effect, scope)
+					evaluateWithScope(effect, scope)
 					version = os.clock()
 					return
 				end
@@ -326,7 +428,7 @@ local function createEffect(effect: (scope) -> ()): dispose
 		end
 	end
 
-	callUserSpaceWithScope(effect, scope)
+	evaluateWithScope(effect, scope)
 	version = os.clock()
 
 	return dispose
@@ -337,5 +439,7 @@ return {
 	createComputed = createComputed,
 	createEffect = createEffect,
 
+	configure = configure,
+	getDebugState = getDebugState,
 	isCallable = isCallable,
 }

modules/signals/src/__tests__/Signals.test.lua

@@ -967,3 +967,54 @@ it("should handle nested batch calls", function()
 
 	dispose()
 end)
+
+it("reports a scopeless read in an effect once per call site", function()
+	local reported = {}
+
+	Signals.configure({
+		warnScopelessReads = true,
+		report = function(message)
+			table.insert(reported, message)
+		end,
+	})
+
+	local get = createSignal(1)
+	local getTick, setTick = createSignal(0)
+
+	get(false) -- deliberate, so nothing is reported
+	expect(#reported).toEqual(0)
+
+	get(nil) -- outside a reactive body, so no report
+	expect(#reported).toEqual(0)
+
+	local dispose = createEffect(function(scope)
+		getTick(scope)
+		get(nil)
+	end)
+	expect(#reported).toEqual(1)
+	expect(string.find(reported[1], "without naming a scope", 1, true) ~= nil).toEqual(true)
+	setTick(1)
+	expect(#reported).toEqual(1)
+	dispose()
+
+	Signals.configure({ warnScopelessReads = false, report = warn })
+end)
+
+it("exposes requested signal and computed debug state", function()
+	Signals.configure({ showInternals = true })
+	local get, set = createSignal(1, nil, "source")
+	local computed = createComputed(function(scope)
+		return get(scope) * 2
+	end, nil, "derived")
+	expect(Signals.getDebugState(get).name).toEqual("source")
+	expect(Signals.getDebugState(computed).name).toEqual("derived")
+	expect(get(false)).toEqual(1)
+	expect(computed(false)).toEqual(2)
+	set(2)
+	expect(computed(false)).toEqual(4)
+	expect(Signals.getDebugState(get).value).toEqual(2)
+	expect(Signals.getDebugState(computed).value).toEqual(4)
+	Signals.configure({ showInternals = false })
+	local plain = createSignal(0)
+	expect(Signals.getDebugState(plain)).toEqual(nil)
+end)

modules/signals/src/init.lua

@@ -9,4 +9,6 @@ export type SignalOptions = Signals.SignalOptions
 
 export type scope = Signals.scope
 
+export type Config = Signals.Config
+
 return Signals