Update GDScript 2.0 engine to use weak references for scripts and packed scenes to enable cyclic references

[adamscott]

Superseded by #67714

TL;DR

This PR makes possible to use cyclic references in GDScript 2.0

Summary

This PR makes the GDScript 2.0 engine compatible with weak references of Scripts and GDScripts. Instead of passing the scripts around, which makes the counter up, this PR stores the scripts inside their own weak references. So, the scripts can be used as much as needed without making the counter up. This makes cyclic references possible.

What it tries to solve

The current GDScript engine uses strong references to store the scripts and scenes that it loads. This usually work well in the standard situations, as those scripts and scenes are RefCounted objects.

Standard case

Let's say Main loads A that loads B.

Here, Main reference count is 1 (referenced by the engine), A is 1 (referenced by Main) and B is 1 (referenced by A).

When the engine unloads, Main reference count will be decremented by 1 and will be deleted, because it's reference count will hit 0. A and B will also be deleted because A lost his reference from Main and B will lose it's reference from A later.

Current cyclic reference issue

Currently, the engine makes it so that using cyclic references is impossible. Why, you say?

Let's bring back our example, but with a cyclic reference. Let's say Main loads A that loads B that loads back A.

Here, Main reference count is 1 (referenced by the engine), A is 2 (referenced by Main and B) and B is 1 (referenced by A).

Do you see the issue? When Main will be deleted, there will be no way to delete A and B. A will lose it's reference to Main and will decrement, but it will not be deleted as A reference count is now 1. B is not deleted because it didn't lose it's reference to A.

So, the answer is: the engine makes cyclic references impossible due to memory leaks concerns. That explains errors thrown by the parser or the analyzer like Constant value uses script from "res://b.gd" which is loaded but not compiled.

Proposed solution

Let's bring back our example.

Main loads A that loads B that loads back A.

What if loading increments a wrapper instead of the script reference? And what if we use a dependency chart to detect when a script needs to be deleted?

Weak references

Instead of using strong references for scripts and scenes, this PR uses weak references. Weak references are referenced values that holds another value. When accessing that later value, it doesn't increase the reference counter, making it so that it does not matter how cyclic references are.

Instead of having a strong reference to A in B (which would increase A to 2), let's use altogether weak references for scripts and scenes. B will still be able to use A, but A reference count will stay at 1.

The engine had to be changed to support values that may be a weak reference or not. Hence the rather heavy PR.

Dependency chart

If we don't use references as the way to dispose of the loaded objects, we need another way to delete them. Here comes the dependency chart.

Take the dependency chart of our example

• Main
  • loads A
• A
  • loads B
• B
  • loads A

With this, we know that A and B cannot be unloaded. Because A and B are found in other dependencies. But when Main will be disposed of, it's only a matter of disposing of every linked dependency.

List of changes:

• Add weak references to support preload cyclic references
• Add new algorithm for removing scripts based on dependencies
• Expose GDScriptCache::remove_script()
• Use .notest.gd system to bypass tests
• Add ScriptRef
• Add weakref for GDScriptParser
• Add global classes support (replaces Fix cyclic references issues with global classes #65664)
• Add packed scenes support

Known issues

• heap-use-after-free or leaks issues

Code example

So, this code works with this PR, which returns this error on master: Constant value uses script from "res://b.gd" which is loaded but not compiled.:

# main.gd
const A = preload("res://a.gd")

func _ready() -> void:
	A.test()  # The console will print: "A.HELLO_WORLD: hello, world!"

# a.gd
extends Node

const B = preload("res://b.gd")

const HELLO_WORLD: = "hello, world!"

static func test() -> void:
	B.test()

	# Test cyclic `is` keyword
	var b_instance: = B.new()
	if b_instance is B:
		prints("b_instance is B")  # The console will print: "b_instance is B"
	b_instance.queue_free()

# b.gd
extends Node

const A = preload("res://a.gd")

static func test() -> void:
	prints("A.HELLO_WORLD:", A.HELLO_WORLD)

	# Test cyclic `is` keyword
	var a_instance: = A.new()
	if a_instance is A:
		prints("a_instance is A")  # The console will print: "a_instance is A"
	a_instance.queue_free()

Minimal reproduction project:

cyclic3.zip

Fixes

Fixes #21461 - Some usages of class_name can produce cyclic errors unexpectedly
Fixes #32165 - Cyclic Errors After Autoload of Imported Asset
Fixes #41027 - Cyclic reference error in GDScript 2.0
Fixes #58551 - Can't preload a scene in AutoLoad script when the scene's script references the AutoLoad
Fixes #58181 - GDScript 2.0: Cannot preload cyclic const script references
Fixes #58871 - Cyclic Autoload Singleton Plugin Bug
Fixes #61043 - New cyclic reference error with autoload and preloaded script

Notes

This PR was the follow-up to #65672.
This PR replaces #65664 as it implements global classes too.

[KoBeWi]

I tested it with my project and got this crash:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.0.beta.custom_build (f74491fdee9bc2d68668137fbacd8f3a7e7e8df7)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] ObjectID::is_null (C:\godot_source\core\object\object_id.h:47)
[1] ObjectID::is_null (C:\godot_source\core\object\object_id.h:47)
[2] WeakRef::get_ref (C:\godot_source\core\object\ref_counted.cpp:103)
[3] ScriptRef::get_ref (C:\godot_source\core\object\script_language.h:258)
[4] GDScriptRef::get_ref (C:\godot_source\modules\gdscript\gdscript.h:540)
[5] ResourceFormatLoaderGDScript::load (C:\godot_source\modules\gdscript\gdscript.cpp:2402)
[6] ResourceLoader::_load (C:\godot_source\core\io\resource_loader.cpp:213)
[7] ResourceLoader::_thread_load_function (C:\godot_source\core\io\resource_loader.cpp:240)
[8] ResourceLoader::load (C:\godot_source\core\io\resource_loader.cpp:570)
[9] EditorAutoloadSettings::_create_autoload (C:\godot_source\editor\editor_autoload_settings.cpp:403)
[10] EditorAutoloadSettings::EditorAutoloadSettings (C:\godot_source\editor\editor_autoload_settings.cpp:860)
[11] ProjectSettingsEditor::ProjectSettingsEditor (C:\godot_source\editor\project_settings_editor.cpp:678)
[12] EditorNode::EditorNode (C:\godot_source\editor\editor_node.cpp:6664)
[13] Main::start (C:\godot_source\main\main.cpp:2754)
[14] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:176)
[15] _main (C:\godot_source\platform\windows\godot_windows.cpp:201)
[16] main (C:\godot_source\platform\windows\godot_windows.cpp:215)
[17] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:229)
[18] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[19] BaseThreadInitThunk
-- END OF BACKTRACE --

[adamscott]

@KoBeWi Thanks! This should be fixed by now.

[KoBeWi]

Another crash, this time I didn't get any stack trace, had to use debugger to find it:

CowData<char32_t>::size() Line 133 (c:\godot_source\core\templates\cowdata.h:133)
CowData<char32_t>::get(int p_index) Line 157 (c:\godot_source\core\templates\cowdata.h:157)
String::operator[](int p_index) Line 224 (c:\godot_source\core\string\ustring.h:224)
String::get_data() Line 970 (c:\godot_source\core\string\ustring.cpp:970)
String::hash() Line 2688 (c:\godot_source\core\string\ustring.cpp:2688)
HashMapHasherDefault::hash(const String & p_string) Line 306 (c:\godot_source\core\templates\hashfuncs.h:306)
HashMap<String,Ref<GDScript>,HashMapHasherDefault,HashMapComparatorDefault<String>,DefaultTypedAllocator<HashMapElement<String,Ref<GDScript>>>>::_hash(const String & p_key) Line 85 (c:\godot_source\core\templates\hash_map.h:85)
HashMap<String,Ref<GDScript>,HashMapHasherDefault,HashMapComparatorDefault<String>,DefaultTypedAllocator<HashMapElement<String,Ref<GDScript>>>>::_lookup_pos(const String & p_key, unsigned int & r_pos) Line 106 (c:\godot_source\core\templates\hash_map.h:106)
HashMap<String,Ref<GDScript>,HashMapHasherDefault,HashMapComparatorDefault<String>,DefaultTypedAllocator<HashMapElement<String,Ref<GDScript>>>>::erase(const String & p_key) Line 315 (c:\godot_source\core\templates\hash_map.h:315)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 136 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:136)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)
GDScriptCache::remove_dependencies(const String & p_source, const String & p_path, const bool & repeat) Line 141 (c:\godot_source\modules\gdscript\gdscript_cache.cpp:141)

EDIT:
Wait, it's truncated. It's actually stack overflow. Unfortunately I can't get the full stack.
The maximum number of stack frames supported by Visual Studio has been exceeded.

[bitbrain]

I opened this in my Godot 4 ported RPG (around 2 years of development on it already) and it gave me no errors either! (apart from regression errors that are already covered by other issues)

[h0lley]

for my project any of the errors / crashes I posted earlier in this thread have been resolved as well!
however there are still a couple of errors being logged, although I cannot link them to anything malfunctioning.

I get this one when loading the project in the editor or when starting test play:

E 0:00:01:0015   _ready: Attempt to open script '' resulted in error 'File not found'.
  <C++ Error>    Condition "err" is true. Returning: err
  <C++ Source>   modules/gdscript/gdscript.cpp:1065 @ load_source_code()
  <Stack Trace>  boot.tscn::1:40 @ _ready()

the file in question appears to be a built-in script attached to the main scene.
line 40 in said script contains get_tree().change_scene_to_file("...").
unfortunately I haven't been able to reconstruct it in a minimal project so far.

then, when dragging GD script files that register classes between folders in the FileSystem panel, I get these errros:

  Attempt to open script 'res://cyclic/class_a.gd' resulted in error 'File not found'.
  res://cyclic/class_a.gd:2 - Parse Error: Class "TestA" hides a global script class.

this can be easily reconstructed.
for instance in this project: tmp.zip

1. drag the class_a.gd from cyclic into res://
2. notice error: res://class_a.gd:2 - Parse Error: Class "TestA" hides a global script class.
3. drag it back into cyclic
4. notice error: Attempt to open script 'res://class_a.gd' resulted in error 'File not found'.

[bitbrain]

Hopefully this makes it into Beta 3 keeps fingers crossed

[bitbrain]

would it make more sense to introduce a method for this that something like clear_cache that unreferences all references and then clears it afterwards? You then can call it on both the shallow_gdscript_cache and full_gdscript_cache

[adamscott]

This code was removed, as this was quite a ugly hack. Now, as references are not leaking, clearing shallow_gdscript_cache and full_gdscript_cache suffice.

[adamscott]

Just fixed it.

[adamscott]

Superseded by #67714