Fix cyclic references in GDScript 2.0

[adamscott]

TL;DR

This PR makes possible to use cyclic references in GDScript 2.0, i.e. preload class (or use by class name) A in B and B in A, preload cyclic scenes, use typing in addons.

Summary

This PR tweaks the engine to accept and support cyclic references.

What it tries to solve

The current GDScript engine blocks cyclic references as this would normally lead to memory leaks. Why? Because cyclic references artificially increase the refcount of gdscripts.

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 its reference count will hit 0. A and B will also be deleted because A lost his reference from Main and B will lose its 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 its 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 its 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

Cyclic dependency detection

There is no way to use the reference count as a mean to know if a cyclic Ref<RefCounted> needs to be disposed of. If A and B refers to themselves a number of times, the reference count will skyrocket and it would not be easy to track down the exact number of times the count has been incremented.

If, someday or someone, comes up with a reliable way to detect these, it would be the ideal solution.

Meanwhile, to fix this issue, I propose to use the existing SelfList<GDScript> script_list found in the GDScript class. This list holds each GDScript instance.

Take this table as basis for my following example:

Script list	Script constants
Main	A, B, X
A
B	C
C	D, E
D
E	F
F	E
X	Y
Y	F, Z
Z

On the left column, each script contained in script_list and on the right one, each script referenced, contained in its constants (script constants, function constants, ...).

In the example, if B is disposed, C and D will be too, but not E and F. How to make sure they are disposed correctly? And how to make sure to not dispose any script that can be legitimately used by another one?

When we dispose of B, we can check the dependencies of its dependencies. After all, there's a good chance that those will be disposed of with B. So, B will check its get_must_clear_dependencies().

"Must clear" dependencies

GDScript::get_must_clear_dependencies() is called in GDScript::clear() to know which dependencies must be forcibly cleared.

The process may seem complicated, but it's quite simple. Let's say Main runs get_must_clear_dependencies().

1. Get a dependencies set. These dependencies includes all the dependencies of the script dependencies;
2. Get the inverted dependencies of each dependency of set 1;
3. For each dependency of set 1, if one their inverted dependencies (step 2) is outside of the dependencies of step 1, add the dependency (and it's dependencies) to a "can't clear" set;
4. Filter out each dependency that appears in the "can't clear" set of step 3;
5. Return the set of step 4.

With this, we know for a fact that each dependency in the "must_clear" set can be deleted without impacting the rest of the scripts, even if their reference count is high. That means that it's high only because of cyclic references.

By clearing each of those scripts (GDScript::clear() clears all the constants holding scripts), their reference count will necessarily drop to zero, so they will be disposed without any need of hacks.

Example

Must clear dependencies of Main

Must clear dependencies of Main is quite simple. It's every GDScript because every script comes from Main.

Step 1

Get a dependencies set. These dependencies includes all the dependencies of the script dependencies;

The direct dependencies of Main are A, B and X, but as it includes all the dependencies of the dependencies, it includes every class in the diagram.

Step 2

Get the inverted dependencies of each dependency of set 1;

Class	Inverted dependencies
A	Main
B	Main
C	B
D	C
E	C, F
F	E, Y
X	Main
Y	X
Z	Y

Step 3

For each dependency of set 1, if one their inverted dependencies (step 2) is outside of the dependencies of step 1, add the dependency (and it's dependencies) to a "can't clear" set;

As every class is a dependency of Main, "can't clear" set is empty.

Step 4

Filter out each dependency that appears in the "can't clear" set of step 3;

As "can't clear" is empty, the dependencies stay the same.

Step 5

5. Return the set of step 4.

It returns every classes.

Must clear dependencies of B

This is where must_clear_dependencies shine, because it doesn't clear everything this time. Based on the algorithm, only C and D can be cleared without any consequences, as E and F are used by Y.

Step 1

Get a dependencies set. These dependencies includes all the dependencies of the script dependencies;

Class	Dependencies
B	C, D, E, F

The direct dependency of B is C, but as it includes all the dependencies of the dependencies, it includes D, E and F.

Step 2

Get the inverted dependencies of each dependency of set 1;

Class	Inverted dependencies
C	B
D	C
E	C, F
F	E, Y

Step 3

For each dependency of set 1, if one their inverted dependencies (step 2) is outside of the dependencies of step 1, add the dependency (and it's dependencies) to a "can't clear" set;

Y is outside of B (itself), C, D, E and F. So, we add F, E and Y to the "can't clear" set.

Step 4

Filter out each dependency that appears in the "can't clear" set of step 3;

We remove F and E from the dependencies.

Step 5

5. Return the set of step 4.

It returns C and D.

Backup

If, for some reason, a script was able to slip out of the process, it still has a reference to script_list. Then, we can call clear() in the loop found in GDScriptLanguage::~GDScriptLanguage() for script instances still left. This adds the ability to force the clear of internal variables of a script, which can trigger the reference count decrement of other scripts.

Known issues

• ✔️ Code completion isn't working for cyclic references
  • Bug present in 4.0beta2 for in-game scripts. The method list does not update at all until a refresh. Code completion for this PR does not show issues other than current ones;
• ✔️ The code works, but the cleaning isn't optimal. I found a reliable way to calculate cyclic references (see GDScript::get_cyclic_references_count()), but my efforts to clear dynamically the script failed. It seems the unreference() function is somewhat unreliable.
  • Implemented with success, clearing GDScript with GDScript::clear() and GDScript::get_must_clear_dependencies();
• ❌ For some reason, GDScript instances can still hold Shape2D references, even if PhysicsServer2D singleton was destructed, which leads to memory leaks.
• ❌ Same thing for Shape3D

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
Fixes #65263 - GDScript reload generates error when created dynamically (no backing script)
Fixes #68211 - Can't preload a scene in autoload if that scene uses the autoload

Does not fix (yet)

#61386 - [4.x regression] AutoLoad scenes (not scripts) do not support implicit types

Notes

This PR follows up #65752.

[dalexeev]

Please note: cyclic references (RefCounted-related issue), cyclic dependencies (a bug in GDScript) and cyclic definitions/calls (a user error, not a GDScript bug) are different things.

[adamscott]

Please note: cyclic references (RefCounted-related issue), cyclic dependencies (a bug in GDScript) and cyclic definitions/calls (a user error, not a GDScript bug) are different things.

I renamed the PR to "Add support for cyclic references in GDScript 2.0".

[adamscott]

I added the .notest.gd exception here, as it was defined elsewhere, but not used. Practical for .gd files that are loaded by tests, but not tests themselves.

[adamscott]

Renamed the PR to "Fix cyclic references in GDScript 2.0", as this fixes a bug (missing cyclic references support). It is not a "new feature" per se.

[adamscott]

I fixed #61386 in this PR because I used code that I added in this one, but I could separate the fix into another PR, if needed.

[adamscott]

Here's the fix for #61386 (the whole else if).

[TokisanGames]

I just tried beta 6. This isn't quite working.

GameInit.gd # Autoloaded as 'class_name Game'
     var player: Player   # CRC errors before. In beta 6 unlocatable, non-CRC error. Works if no type.

Player and Enemy both inherit Unit. Many classes including Enemy reference Game.*, especially Game.player.

In previous versions we had CRC errors in Enemy when Player was typed as shown above, and we referenced Game.player. I received an editor notification highlighting the exact line in either GameInit.gd or Enemy.gd (I don't recall).

Now in Beta 6, I get the following error in the console upon loading the editor or the game, and there's no reference at all to the GDScript location. The editor does load scenes, but the game does not run.

ERROR: Condition "!base->is_valid()" is true. Returning: ERR_BUG
   at: _populate_class_members (modules/gdscript/gdscript_compiler.cpp:2287)

Upon running a scene, it breaks, and in the editor I'm presented with no info:

It does not break on any gdscript. The Errors(2) shows the same error listed on the console, but none of the lines takes me to gdscript when clicking on them. So if I didn't already know exactly where the CRC error is, I'd never find it.

Someone else has a similar issue, perhaps also caused by a CRC error they don't know about. #69213. If I can make an MRP, I'll make an issue or contribute to that one. However hopefully the cpp line and error will highlight the bug.

[adamscott]

@tinmanjuggernaut Please create an issue and a MRP if possible. The .cpp path helps, but by itself, I cannot find the root of the issue. The issue you're experiencing can be related to this PR, but it's not necessarily the case.

@rune-scape Does it ring a bell to you?

[TokisanGames]

#69259 fixed my errors above using cyclic references. Thanks @adamscott .