Skip to content

Fix cyclic references issues with global classes#65664

Closed
adamscott wants to merge 1 commit intogodotengine:masterfrom
adamscott:fix-global-class-cyclic-references
Closed

Fix cyclic references issues with global classes#65664
adamscott wants to merge 1 commit intogodotengine:masterfrom
adamscott:fix-global-class-cyclic-references

Conversation

@adamscott
Copy link
Copy Markdown
Member

@adamscott adamscott commented Sep 11, 2022 •
edited
Loading

This PR works in tandem with #65752 for fixing cyclic references issues.
This PR is not needed if #65752 is merged, as it deals too with global classes.

This makes possible to reference class A (class_name A) in class B (class_name B) and B in A. This is not possible actually.

# a.gd
class_name A
extends Node

enum Test {
	VALUE_A,
	VALUE_B
}

static func hello() -> void:
	prints("Hello from A")
	B.hello()

# b.gd
class_name B
extends Node

static func hello() -> void:
	prints("Hello from B")
	prints("from A:", A.Test.VALUE_A)

In the master branch, this code fails on runtime with this error: Parser Error: Can't load global class B, cyclic reference?

The compiler, in this PR, checks if the script of the global class can be loaded. When the script exists but cannot be loaded, it means usually that the script is loaded but not compiled. So, the compiler now tags this entity as temporary to be read dynamically instead of being considered as a constant.

Minimal reproduction project:
cyclic2.zip

Fixes #21461, fixes #41027.

@adamscott adamscott requested a review from a team as a code owner September 11, 2022 18:55
@kleonc kleonc requested a review from vnen September 11, 2022 19:28
@kleonc kleonc added this to the 4.0 milestone Sep 11, 2022
@adamscott adamscott force-pushed the fix-global-class-cyclic-references branch from 43dfee5 to 3d0f36a Compare September 13, 2022 14:18
@adamscott adamscott mentioned this pull request Sep 13, 2022
Closed
@nathanfranke
Copy link
Copy Markdown

nathanfranke commented Sep 15, 2022 •
edited
Loading

Here's a quick script to merge and test both #65664 and #65752

pr() {
    : ${1?"Usage: pr 12345"}
    git fetch origin "pull/$1/head:pr/$1" || return 1
    git switch "pr/$1" || return 1
}

pr 65664
pr 65752
git checkout master
git merge --no-edit 'pr/65664'
git merge --no-edit 'pr/65752'

Edit: Response to below: I don't plan to review the code much, I am mostly enjoying this for my "fork" for prototyping projects, so memory leaks aren't a huge deal for that.

@adamscott
Copy link
Copy Markdown
Member Author

adamscott commented Sep 15, 2022

@nathanfranke I think they should be separated. #65752 has some problems related to memory leaks that I must solve. This PR does not seem to share this problem.

@adamscott
Copy link
Copy Markdown
Member Author

adamscott commented Sep 16, 2022 •
edited
Loading

Edit: Response to below: I don't plan to review the code much, I am mostly enjoying this for my "fork" for prototyping projects, so memory leaks aren't a huge deal for that.

@nathanfranke Don't hesitate to report bugs here or on the other PR! I would love some feedback, even if it's not related to code review purposes. 😊

@adamscott
Copy link
Copy Markdown
Member Author

adamscott commented Sep 23, 2022

I updated the PR description to underline that it is not needed if #65752 is merged, as it deals with global classes too and is much more in depth.

#65752 updates the GDScript engine to use weak references for scripts, which permits cyclic references with preload() too.

@adamscott
Copy link
Copy Markdown
Member Author

adamscott commented Oct 7, 2022

#65752 is the way to go. I'm closing this PR.

@adamscott adamscott closed this Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vnen vnen Awaiting requested review from vnen

Assignees

No one assigned

Labels

archived bug topic:gdscript

Projects

None yet

Milestone

4.0

Development

Successfully merging this pull request may close these issues.

Cyclic reference error in GDScript 2.0 Some usages of class_name can produce cyclic errors unexpectedly

4 participants

@adamscott @nathanfranke @akien-mga @kleonc