GDScript: Solve _populate_class_members() cyclic dependency problem

[anvilfolk]

Fixes #78146 and fixes #79153 (which is pretty much a duplicate). The issue occurs because _populate_class_members(descendant_class) is getting called during a call for _populate_class_members(ancestor_class). Thus class members of ancestors are not available for the descendant class when it is populating its own, resulting the identifier not found error of the linked issue when the descendant class gets compiled.

Issue 🔥

GDScriptCompiler::_populate_class_members() sets up the datastructures so that the compiler knows the addresses of member variables of the class being compiled. For this, it needs to know members its base class, so that those are also addressable. In order to obtain those members, it asks for the fully compiled script of the base class using GDScriptCache::get_full_script().

However in certain configurations of dependencies, the base class of the current class may depend on a descendant class. Let's say Daughter(D) extends Mother(M) extends Grandmother(GM), as in the linked issue. If you start by compiling (not just analyzing) M, that requires GM's class members, so you get_full_script(GM), but if GM somehow references D in a way that asks for its get_full_script(D), then D will run its _populate_class_members() without M having hers populated from GM.

Oof, that's a mouthful.

I don't believe it is possible to replicate the issue within the test framework, as it relies on global class names that exist in different files. Instead, I am attaching 🐛 MRP.zip 🐞, which causes the issue when running the scene, before this PR, but works properly after this PR.

The dependency is as follows: Daughter extends Mother extends Grandmother, with Grandmother referring to (but not extending Daughter), and Mother being the first file loaded, in this case by being attached to the main scene node.

Solution ⚕️

The solution here was to make it so that _populate_class_members() did not require get_full_script(), but instead only requires the shallow script (which does resolve any necessary inheritance dependencies), followed by calling its _populate_class_members(). This no longer triggers get_full_script(), and so descendant classes don't get compiled before their base classes are done populating their members.

Concerns 😨

I'm not super well versed in the meta-organization of the GDScript infrastructure, so I wonder whether this is the best solution. This doesn't so much solve cyclic dependencies as it sidesteps them insofar as _populate_class_members is concerned. Maybe that's not so bad, since it's such a specific case?

Either way, I'd love to see a refactor of the way scripts are loaded, maybe doing away with get_shallow/full_script and using GDScriptCache/raise_status() to give us more fine-grained control over what is needed at each point in time, and making GDScript::reload(), which get_full_script() relies on, a little less monolithic.

[anvilfolk]

Right now we don't get compilation-related warnings in the editor. This line enables them. This does not mean analyzer errors, which presumably there are none of.

Compilation errors, if the analyzer passes, are our problem, not a game developer's, so I understand the desire to hide them. But if the error only occurs when game devs run the game, that just puts a barrier towards bug reporting/fixing.

[anvilfolk]

Currently running into failure of the out_of_order_external.gd test, but only when running the tests. It works both in-editor and when directly running the scene 🤔 Will keep investigating.

[anvilfolk]

Currently running into failure of the out_of_order_external.gd test, but only when running the tests. It works both in-editor and when directly running the scene 🤔 Will keep investigating.

This appears to have been caused by the fact that the test runner doesn't use ResourceLoader::load() to load a GDScript, it creates it itself. This meant that GDScript::path was being set but Resource::path was empty, which meant that GDScriptCache::finish_compiling(main_script->get_path()), which grabs the Resource's path, was passing it the empty string, and thus finish_compiling was doing a no-op.

[anvilfolk]

This was the issue with testing, which doesn't use ResourceLoader::load() to load scripts, meaning that Resource::path, which you get using main_script->get_path() was empty, so that GDScriptCache::finish_compiling() was getting an empty string and being a no-op :)

[adamscott]

Great work, ocean!

[anvilfolk]

This is what I'm worried about! I haven't noticed the absence of this causing problems in the small tests I have done, but I'd love to see this merged so we can fix any regressions this might lead to.

[anvilfolk]

I believe that the checks above should cause the function to return early in the case of errors so that this isn't necessary. But you never know when things are this subtle :)

[adamscott]

@YuriSizov If you could merge this PR, I think it could fix some issues.

[YuriSizov]

Sure, let's go ahead with this. Thanks!

[anvilfolk]

Nice, thank you! Let's keep an eye out for potential regressions :)

[YuriSizov]

Given the regressions I'd err on the side of caution and skip this from cherry-picking. If there is still desire to bring this to 4.1.x, I'd appreciate a summary of the PRs that need to be picked and the stability of this change. But given that some of the fixes are pretty recent, I'd at least give it more time.

[anvilfolk]

The bug this fixes can be pretty project-stopping with unclear or impossible workarounds, but it only affects with cyclic dependencies and specific load orders. Unless we see a lot of folks complaining about class members that aren't recognized in 4.1, I think you're right in letting this + the recent fixes stew a little longer in 4.2 :)

The current other PRs that continue this effort to fix cyclic dependencies in compilation are #81577 and #81201. If you do consider cherry-picking at some point, let me know and I can check whether other PRs are needed as a follow-up!