Merge pull request #109345 from HolonProduction/resource-cache-shall-shun-shallow

Prevent shallow scripts from leaking into the `ResourceCache`

modules/SCsub

@@ -52,6 +52,7 @@ for name, path in env.module_list.items():
 
 # Generate header to be included in `tests/test_main.cpp` to run module-specific tests.
 if env["tests"]:
+    env.Append(CPPDEFINES=["TESTS_ENABLED"])
     env.CommandNoCache("modules_tests.gen.h", test_headers, env.Run(modules_builders.modules_tests_builder))
 
 # libmodules.a with only register_module_types.

modules/gdscript/gdscript.cpp

@@ -1074,6 +1074,12 @@ void GDScript::_bind_methods() {
 	ClassDB::bind_vararg_method(METHOD_FLAGS_DEFAULT, "new", &GDScript::_new, MethodInfo("new"));
 }
 
+void GDScript::set_path_cache(const String &p_path) {
+	if (is_root_script()) {
+		Script::set_path_cache(p_path);
+	}
+}
+
 void GDScript::set_path(const String &p_path, bool p_take_over) {
 	if (is_root_script()) {
 		Script::set_path(p_path, p_take_over);
@@ -3041,7 +3047,11 @@ Ref<GDScript> GDScriptLanguage::get_script_by_fully_qualified_name(const String
 Ref<Resource> ResourceFormatLoaderGDScript::load(const String &p_path, const String &p_original_path, Error *r_error, bool p_use_sub_threads, float *r_progress, CacheMode p_cache_mode) {
 	Error err;
 	bool ignoring = p_cache_mode == CACHE_MODE_IGNORE || p_cache_mode == CACHE_MODE_IGNORE_DEEP;
-	Ref<GDScript> scr = GDScriptCache::get_full_script(p_original_path, err, "", ignoring);
+	Ref<GDScript> scr = GDScriptCache::get_full_script_no_resource_cache(p_original_path, err, "", ignoring);
+	// Reset `path_cache` so that when resource loader uses `set_path()` later, the script gets added to the cache.
+	if (scr.is_valid()) {
+		scr->set_path_cache(String());
+	}
 
 	if (err && scr.is_valid()) {
 		// If !scr.is_valid(), the error was likely from scr->load_source_code(), which already generates an error.

modules/gdscript/gdscript.h

@@ -310,6 +310,7 @@ public:
 
 	virtual Error reload(bool p_keep_state = false) override;
 
+	virtual void set_path_cache(const String &p_path) override;
 	virtual void set_path(const String &p_path, bool p_take_over = false) override;
 	String get_script_path() const;
 	Error load_source_code(const String &p_path);

modules/gdscript/gdscript_cache.cpp

@@ -212,7 +212,7 @@ void GDScriptCache::remove_script(const String &p_path) {
 Ref<GDScriptParserRef> GDScriptCache::get_parser(const String &p_path, GDScriptParserRef::Status p_status, Error &r_error, const String &p_owner) {
 	MutexLock lock(singleton->mutex);
 	Ref<GDScriptParserRef> ref;
-	if (!p_owner.is_empty()) {
+	if (!p_owner.is_empty() && p_path != p_owner) {
 		singleton->dependencies[p_owner].insert(p_path);
 		singleton->parser_inverse_dependencies[p_path].insert(p_owner);
 	}
@@ -298,7 +298,7 @@ Vector<uint8_t> GDScriptCache::get_binary_tokens(const String &p_path) {
 Ref<GDScript> GDScriptCache::get_shallow_script(const String &p_path, Error &r_error, const String &p_owner) {
 	MutexLock lock(singleton->mutex);
 
-	if (!p_owner.is_empty()) {
+	if (!p_owner.is_empty() && p_path != p_owner) {
 		singleton->dependencies[p_owner].insert(p_path);
 	}
 	if (singleton->full_gdscript_cache.has(p_path)) {
@@ -312,7 +312,8 @@ Ref<GDScript> GDScriptCache::get_shallow_script(const String &p_path, Error &r_e
 
 	Ref<GDScript> script;
 	script.instantiate();
-	script->set_path(p_path, true);
+
+	script->set_path_cache(p_path);
 	if (remapped_path.has_extension("gdc")) {
 		Vector<uint8_t> buffer = get_binary_tokens(remapped_path);
 		if (buffer.is_empty()) {
@@ -338,9 +339,20 @@ Ref<GDScript> GDScriptCache::get_shallow_script(const String &p_path, Error &r_e
 }
 
 Ref<GDScript> GDScriptCache::get_full_script(const String &p_path, Error &r_error, const String &p_owner, bool p_update_from_disk) {
+	Ref<GDScript> uncached_script = get_full_script_no_resource_cache(p_path, r_error, p_owner, p_update_from_disk);
+
+	// Resources don't know whether they are cached, so using `set_path()` after `set_path_cache()` does not add the resource to the cache if the path is the same.
+	// We reset the cached path from `get_shallow_script()` so that the subsequent call to `set_path()` caches everything correctly.
+	uncached_script->set_path_cache(String());
+	uncached_script->set_path(p_path, true);
+
+	return uncached_script;
+}
+
+Ref<GDScript> GDScriptCache::get_full_script_no_resource_cache(const String &p_path, Error &r_error, const String &p_owner, bool p_update_from_disk) {
 	MutexLock lock(singleton->mutex);
 
-	if (!p_owner.is_empty()) {
+	if (!p_owner.is_empty() && p_path != p_owner) {
 		singleton->dependencies[p_owner].insert(p_path);
 	}
 

modules/gdscript/gdscript_cache.h

@@ -78,6 +78,12 @@ public:
 	~GDScriptParserRef();
 };
 
+#ifdef TESTS_ENABLED
+namespace GDScriptTests {
+class TestGDScriptCacheAccessor;
+}
+#endif // TESTS_ENABLED
+
 class GDScriptCache {
 	// String key is full path.
 	HashMap<String, GDScriptParserRef *> parser_map;
@@ -91,6 +97,9 @@ class GDScriptCache {
 	friend class GDScript;
 	friend class GDScriptParserRef;
 	friend class GDScriptInstance;
+#ifdef TESTS_ENABLED
+	friend class GDScriptTests::TestGDScriptCacheAccessor;
+#endif // TESTS_ENABLED
 
 	static GDScriptCache *singleton;
 
@@ -112,6 +121,19 @@ public:
 	static String get_source_code(const String &p_path);
 	static Vector<uint8_t> get_binary_tokens(const String &p_path);
 	static Ref<GDScript> get_shallow_script(const String &p_path, Error &r_error, const String &p_owner = String());
+	/**
+	 * Returns a fully loaded GDScript using an already cached script if one exists.
+	 *
+	 * If a new script is created, the resource will only have its patch_cache set, so it won't be present in the ResourceCache.
+	 * Mismatches between GDScriptCache and ResourceCache might trigger complex issues so when using this method ensure
+	 * that the script is added to the resource cache or removed from the GDScript cache.
+	 */
+	static Ref<GDScript> get_full_script_no_resource_cache(const String &p_path, Error &r_error, const String &p_owner = String(), bool p_update_from_disk = false);
+	/**
+	 * Returns a fully loaded GDScript using an already cached script if one exists.
+	 *
+	 * The returned instance is present in GDScriptCache and ResourceCache.
+	 */
 	static Ref<GDScript> get_full_script(const String &p_path, Error &r_error, const String &p_owner = String(), bool p_update_from_disk = false);
 	static Ref<GDScript> get_cached_script(const String &p_path);
 	static Error finish_compiling(const String &p_owner);

modules/gdscript/tests/gdscript_test_runner_suite.h

@@ -32,10 +32,23 @@
 
 #include "gdscript_test_runner.h"
 
+#include "modules/gdscript/gdscript_cache.h"
 #include "tests/test_macros.h"
+#include "tests/test_utils.h"
 
 namespace GDScriptTests {
 
+class TestGDScriptCacheAccessor {
+public:
+	static bool has_shallow(String p_path) {
+		return GDScriptCache::singleton->shallow_gdscript_cache.has(p_path);
+	}
+
+	static bool has_full(String p_path) {
+		return GDScriptCache::singleton->full_gdscript_cache.has(p_path);
+	}
+};
+
 // TODO: Handle some cases failing on release builds. See: https://github.com/godotengine/godot/pull/88452
 #ifdef TOOLS_ENABLED
 TEST_SUITE("[Modules][GDScript]") {
@@ -72,6 +85,26 @@ func _init():
 	CHECK_MESSAGE(int(ref_counted->get_meta("result")) == 42, "The script should assign object metadata successfully.");
 }
 
+TEST_CASE("[Modules][GDScript] Loading keeps ResourceCache and GDScriptCache in sync") {
+	const String path = TestUtils::get_temp_path("gdscript_load_test.gd");
+
+	{
+		Ref<FileAccess> fa = FileAccess::open(path, FileAccess::ModeFlags::WRITE);
+		fa->store_string("extends Node\n");
+		fa->close();
+	}
+
+	CHECK(!ResourceCache::has(path));
+	CHECK(!TestGDScriptCacheAccessor::has_shallow(path));
+	CHECK(!TestGDScriptCacheAccessor::has_full(path));
+
+	Ref<GDScript> loaded = ResourceLoader::load(path);
+
+	CHECK(ResourceCache::has(path));
+	CHECK(!TestGDScriptCacheAccessor::has_shallow(path));
+	CHECK(TestGDScriptCacheAccessor::has_full(path));
+}
+
 TEST_CASE("[Modules][GDScript] Validate built-in API") {
 	GDScriptLanguage *lang = GDScriptLanguage::get_singleton();