Ref Tracking: Revengance (#57728)

* Ref Tracking: Revengance

Fixes reference tracking ignoring self references due to a poorly thought out tick checking system.
Fixes reference tracking ignoring the contents of assoc lists
Makes the reference tracking printouts actually describe what list the ref is in, rather then just saying "list"

Adds REFERENCE_TRACKING_DEBUG, a define which toggles tracking info for the ref tracking procs, which allows for
oversight on how the proc is working

Allows for direct calls of qdel_and_find_ref_if_fail(), makes it use ref rather then REF(), fixing it breaking
for mobs. (Ditto for the qdel hint which does the same thing)

Moves REAGENTS_TESTING out of the reftracking define block
Makes unit tests define REFERENCE_TRACKING, REFERENCE_TRACKING_DEBUG, and FIND_REF_NO_CHECK_TICK

Adds a unit test that sanity checks the reference finder proc

code/_compile_options.dm

@@ -15,12 +15,8 @@
 //#define REFERENCE_TRACKING
 #ifdef REFERENCE_TRACKING
 
-/*
+///Used for doing dry runs of the reference finder, to test for feature completeness
-* Enables debug messages for every single reaction step. This is 1 message per 0.5s for a SINGLE reaction. Useful for tracking down bugs/asking me for help in the main reaction handiler (equilibrium.dm).
+//#define REFERENCE_TRACKING_DEBUG
-*
-* * Requires TESTING to be defined to work.
-*/
-//#define REAGENTS_TESTING
 
 ///Run a lookup on things hard deleting by default.
 //#define GC_FAILURE_HARD_LOOKUP
@@ -30,6 +26,13 @@
 
 #endif //ifdef REFERENCE_TRACKING
 
+/*
+* Enables debug messages for every single reaction step. This is 1 message per 0.5s for a SINGLE reaction. Useful for tracking down bugs/asking me for help in the main reaction handiler (equilibrium.dm).
+*
+* * Requires TESTING to be defined to work.
+*/
+//#define REAGENTS_TESTING
+
 #define VISUALIZE_ACTIVE_TURFS //Highlights atmos active turfs in green
 #define TRACK_MAX_SHARE //Allows max share tracking, for use in the atmos debugging ui
 #endif //ifdef TESTING
@@ -72,6 +75,13 @@
 #define TESTING
 #endif
 
+#if defined(UNIT_TESTS)
+//Hard del testing defines
+#define REFERENCE_TRACKING
+#define REFERENCE_TRACKING_DEBUG
+#define FIND_REF_NO_CHECK_TICK
+#endif
+
 #ifdef TGS
 // TGS performs its own build of dm.exe, but includes a prepended TGS define.
 #define CBT

code/controllers/subsystem/garbage.dm

@@ -332,9 +332,9 @@ SUBSYSTEM_DEF(garbage)
 			if (QDEL_HINT_FINDREFERENCE) //qdel will, if REFERENCE_TRACKING is enabled, display all references to this object, then queue the object for deletion.
 				SSgarbage.Queue(D)
 				D.find_references()
-			if (QDEL_HINT_IFFAIL_FINDREFERENCE)
+			if (QDEL_HINT_IFFAIL_FINDREFERENCE) //qdel will, if REFERENCE_TRACKING is enabled and the object fails to collect, display all references to this object.
 				SSgarbage.Queue(D)
-				SSgarbage.reference_find_on_fail[REF(D)] = TRUE
+				SSgarbage.reference_find_on_fail["\ref[D]"] = TRUE
 			#endif
 			else
 				#ifdef TESTING

code/datums/datum.dm

@@ -51,9 +51,13 @@
 	*/
 	var/list/cooldowns
 
-#ifdef TESTING
+#ifdef REFERENCE_TRACKING
 	var/running_find_references
 	var/last_find_references = 0
+	#ifdef REFERENCE_TRACKING_DEBUG
+	///Stores info about where refs are found, used for sanity checks and testing
+	var/list/found_refs
+	#endif
 #endif
 
 #ifdef DATUMVAR_DEBUGGING_MODE

code/modules/admin/view_variables/reference_tracking.dm

@@ -31,17 +31,18 @@
 		usr.client.running_find_references = type
 
 	testing("Beginning search for references to a [type].")
-	last_find_references = world.time
 
-	DoSearchVar(GLOB) //globals
+	var/starting_time = world.time
+
+	DoSearchVar(GLOB, "GLOB") //globals
 	for(var/datum/thing in world) //atoms (don't beleive its lies)
-		DoSearchVar(thing, "World -> [thing]")
+		DoSearchVar(thing, "World -> [thing.type]", search_time = starting_time)
 
 	for(var/datum/thing) //datums
-		DoSearchVar(thing, "World -> [thing]")
+		DoSearchVar(thing, "Datums -> [thing.type]", search_time = starting_time)
 
 	for(var/client/thing) //clients
-		DoSearchVar(thing, "World -> [thing]")
+		DoSearchVar(thing, "Clients -> [thing.type]", search_time = starting_time)
 
 	testing("Completed search for references to a [type].")
 	if(usr?.client)
@@ -71,43 +72,65 @@
 	qdel_and_find_ref_if_fail(src, TRUE)
 
 
-/datum/proc/DoSearchVar(potential_container, container_name, recursive_limit = 64)
+/datum/proc/DoSearchVar(potential_container, container_name, recursive_limit = 64, search_time = world.time)
+	#ifdef REFERENCE_TRACKING_DEBUG
+	if(!found_refs)
+		found_refs = list()
+	#endif
+
 	if(usr?.client && !usr.client.running_find_references)
 		return
 
 	if(!recursive_limit)
+		testing("Recursion limit reached. [container_name]")
 		return
 
 	if(istype(potential_container, /datum))
 		var/datum/datum_container = potential_container
-		if(datum_container.last_find_references == last_find_references)
+		if(datum_container.last_find_references == search_time)
 			return
 
-		datum_container.last_find_references = last_find_references
+		datum_container.last_find_references = search_time
 		var/list/vars_list = datum_container.vars
 
 		for(var/varname in vars_list)
-			if (varname == "vars")
+			if (varname == "vars" || varname == "vis_locs") //Fun fact, vis_locs don't count for references
 				continue
 			var/variable = vars_list[varname]
 
 			if(variable == src)
-				testing("Found [type] \ref[src] in [datum_container.type]'s [varname] var. [container_name]")
+				#ifdef REFERENCE_TRACKING_DEBUG
+				found_refs[varname] = TRUE
+				#endif
+				testing("Found [type] \ref[src] in [datum_container.type]'s \ref[datum_container] [varname] var. [container_name]")
 
 			else if(islist(variable))
-				DoSearchVar(variable, "[container_name] -> list", recursive_limit - 1)
+				DoSearchVar(variable, "[container_name] \ref[datum_container] -> [varname] (list)", recursive_limit - 1, search_time)
 
 	else if(islist(potential_container))
 		var/normal = IS_NORMAL_LIST(potential_container)
 		for(var/element_in_list in potential_container)
+			//Check normal entrys
 			if(element_in_list == src)
+				#ifdef REFERENCE_TRACKING_DEBUG
+				found_refs[potential_container] = TRUE
+				#endif
 				testing("Found [type] \ref[src] in list [container_name].")
 
+			//Check assoc entrys
 			else if(element_in_list && !isnum(element_in_list) && normal && potential_container[element_in_list] == src)
+				#ifdef REFERENCE_TRACKING_DEBUG
+				found_refs[potential_container] = TRUE
+				#endif
 				testing("Found [type] \ref[src] in list [container_name]\[[element_in_list]\]")
 
+			//Check normal sublists
 			else if(islist(element_in_list))
-				DoSearchVar(element_in_list, "[container_name] -> list", recursive_limit - 1)
+				DoSearchVar(element_in_list, "[container_name] -> [element_in_list] (list)", recursive_limit - 1, search_time)
+
+			//Check assoc sublists
+			else if(element_in_list && !isnum(element_in_list) && normal && islist(potential_container[element_in_list]))
+				DoSearchVar(potential_container[element_in_list], "[container_name]\[[element_in_list]\] -> [potential_container[element_in_list]] (list)", recursive_limit - 1, search_time)
 
 	#ifndef FIND_REF_NO_CHECK_TICK
 	CHECK_TICK
@@ -115,7 +138,10 @@
 
 
 /proc/qdel_and_find_ref_if_fail(datum/thing_to_del, force = FALSE)
-	SSgarbage.reference_find_on_fail[REF(thing_to_del)] = TRUE
+	thing_to_del.qdel_and_find_ref_if_fail(force)
-	qdel(thing_to_del, force)
+
+/datum/proc/qdel_and_find_ref_if_fail(force = FALSE)
+	SSgarbage.reference_find_on_fail["\ref[src]"] = TRUE
+	qdel(src, force)
 
 #endif

code/modules/unit_tests/_unit_tests.dm

@@ -85,6 +85,10 @@
 #include "timer_sanity.dm"
 #include "unit_test.dm"
 
+#ifdef REFERENCE_TRACKING //Don't try and parse this file if ref tracking isn't turned on. IE: don't parse ref tracking please mr linter
+#include "find_reference_sanity.dm"
+#endif
+
 #undef TEST_ASSERT
 #undef TEST_ASSERT_EQUAL
 #undef TEST_ASSERT_NOTEQUAL

code/modules/unit_tests/find_reference_sanity.dm

@@ -0,0 +1,69 @@
+///Used to test the completeness of the reference finder proc.
+/datum/unit_test/find_reference_sanity
+
+/atom/movable/ref_holder
+	var/atom/movable/ref_test/test
+	var/list/test_list = list()
+	var/list/test_assoc_list = list()
+
+/atom/movable/ref_test
+	var/atom/movable/ref_test/self_ref
+
+/datum/unit_test/find_reference_sanity/Run()
+	var/atom/movable/ref_test/victim = allocate(/atom/movable/ref_test)
+	var/atom/movable/ref_holder/testbed = allocate(/atom/movable/ref_holder)
+
+	//Sanity check
+	victim.DoSearchVar(testbed, "Sanity Check", search_time = 1) //We increment search time to get around an optimization
+	TEST_ASSERT(!victim.found_refs.len, "The ref-tracking tool found a ref where none existed")
+	victim.found_refs.Cut()
+
+	//Set up for the first round of tests
+	testbed.test = victim
+	testbed.test_list += victim
+	testbed.test_assoc_list["baseline"] = victim
+
+	victim.DoSearchVar(testbed, "First Run", search_time = 2)
+
+	TEST_ASSERT(victim.found_refs["test"], "The ref-tracking tool failed to find a regular value")
+	TEST_ASSERT(victim.found_refs[testbed.test_list], "The ref-tracking tool failed to find a list entry")
+	TEST_ASSERT(victim.found_refs[testbed.test_assoc_list], "The ref-tracking tool failed to find an assoc list value")
+	victim.found_refs.Cut()
+	testbed.test = null
+	testbed.test_list.Cut()
+	testbed.test_assoc_list.Cut()
+
+	//Second round, bit harder this time
+	testbed.overlays += victim
+	testbed.vis_contents += victim
+	testbed.test_assoc_list[victim] = TRUE
+
+	victim.DoSearchVar(testbed, "Second Run", search_time = 3)
+
+	//This is another sanity check
+	TEST_ASSERT(!victim.found_refs[testbed.overlays], "The ref-tracking tool found an overlays entry? That shouldn't be possible")
+	TEST_ASSERT(victim.found_refs[testbed.vis_contents], "The ref-tracking tool failed to find a vis_contents entry")
+	TEST_ASSERT(victim.found_refs[testbed.test_assoc_list], "The ref-tracking tool failed to find an assoc list key")
+	victim.found_refs.Cut()
+	testbed.overlays.Cut()
+	testbed.vis_contents.Cut()
+	testbed.test_assoc_list.Cut()
+
+	//Let's get a bit esoteric
+	victim.self_ref = victim
+	var/list/to_find = list(victim)
+	testbed.test_list += list(to_find)
+	var/list/to_find_assoc = list(victim)
+	testbed.test_assoc_list["Nesting"] = to_find_assoc
+
+	victim.DoSearchVar(victim, "Third Run Self", search_time = 4)
+	victim.DoSearchVar(testbed, "Third Run Testbed", search_time = 4)
+	TEST_ASSERT(victim.found_refs["self_ref"], "The ref-tracking tool failed to find a self reference")
+	TEST_ASSERT(victim.found_refs[to_find], "The ref-tracking tool failed to find a nested list entry")
+	TEST_ASSERT(victim.found_refs[to_find_assoc], "The ref-tracking tool failed to find a nested assoc list entry")
+	victim.found_refs.Cut()
+	victim.self_ref = null
+	to_find.Cut()
+	testbed.test_list.Cut()
+	to_find_assoc.Cut()
+	testbed.test_assoc_list.Cut()