Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cyclic references issues with global classes #65664

Conversation

adamscott
Copy link
Member

@adamscott adamscott commented Sep 11, 2022

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
@nathanfranke
Copy link
Contributor

nathanfranke commented Sep 15, 2022

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
Member Author

@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
Member Author

adamscott commented Sep 16, 2022

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
Member Author

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
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
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