Fix cyclic reference base being loaded but not valid (which is ok) #69259

adamscott · 2022-11-27T15:38:53Z

In some cases, a script from GDScriptCache::get_full_script() can be not null, but invalid. In the context of cyclic references and while populating class members (GDScriptCompiler::_populate_class_members()), the test was too strict and was issuing an error even if the script itself is usable.

This was breaking seemingly valid code that was working in previous betas.

Making the tests more lax seems to be the right thing to do.

Fixes #69213

adamscott · 2022-11-27T15:46:44Z

@rune-scape I'm informally asking you to review my PR.

kleonc · 2022-11-27T17:39:18Z

modules/gdscript/gdscript_compiler.cpp

-				if (base_root.is_valid()) {
+				if (!base_root.is_null()) {


This changes nothing, before/after are equivalent.

godot/core/object/ref_counted.h

Lines 212 to 213 in 860884b

inline bool is_valid() const { return reference != nullptr; }

inline bool is_null() const { return reference == nullptr; }

And I think is_valid() is preferred in here, that's kinda why this method exists (to avoid having negated is_null() checks).

You're right for Ref<T> instances, except for GDScript.

godot/modules/gdscript/gdscript.h

Line 185 in 860884b

virtual bool is_valid() const override { return valid; }

but thats defned in Script, not Ref

godot/core/object/script_language.h

Line 144 in 860884b

virtual bool is_valid() const = 0;

You're right. My bad.

base_root is Ref<GDScript> so:

base_root.is_valid() calls Ref::is_valid,

base_root->is_valid() calls GDScript::is_valid (or its override as it's virtual).

base_root is Ref<GDScript> so:

* `base_root.is_valid()` calls `Ref::is_valid`, * `base_root->is_valid()` calls `GDScript::is_valid` (or its override as it's virtual).

I got mixed up. Thanks.

i don't blame you its very confusing

Wow this is an API asking for bugs...

rune-scape

seems to me like 2 of the changes wouldn't have any effect, im confused

rune-scape · 2022-11-27T20:22:53Z

modules/gdscript/gdscript_compiler.cpp

 					base = Ref<GDScript>(base_root->find_class(base->fully_qualified_name));
 				}
 				if (base.is_null()) {
 					_set_error(vformat(R"(Could not find class "%s" in "%s".)", base->fully_qualified_name, base->path), nullptr);
 					return ERR_COMPILATION_FAILED;
 				}
-				ERR_FAIL_COND_V(!base->is_valid(), ERR_BUG);


this shouldn't be removed, if the base isn't valid here, theres something wrong elsewhere,
either get_full_script() should have given an error, or the inner class is not valid for some reason
the code i replaced didn't allow this

In the case of the MRP of #69213, base is reloading. While reloading, the script valid value is set to false, as shown:

godot/modules/gdscript/gdscript.cpp

Lines 850 to 860 in 860884b

MutexLock lock(GDScriptCache::singleton->mutex);

GDScriptCache::singleton->shallow_gdscript_cache[source_path] = Ref<GDScript>(this);

}

}

valid = false;

GDScriptParser parser;

Error err = parser.parse(source, path, false);

if (err) {

if (EngineDebugger::is_active()) {

GDScriptLanguage::get_singleton()->debug_break_parse(_get_debug_path(), parser.get_errors().front()->get().line, "Parser Error: " + parser.get_errors().front()->get().message);

So, as replied to @kleonc, in gdscript.h, GDScript::is_valid() returns a different value than being the inverse of GDScript::is_null(). An invalid script just means that it's reloading. We could change the behaviour, but checking if the script is valid, at this stage, just creates bug, it doesn't prevent ones.

godot/modules/gdscript/gdscript.h

Line 185 in 860884b

virtual bool is_valid() const override { return valid; }

Maybe we could add a check to is_valid() that it must be GDScript::reloading == true too. Otherwise, it's a bug.

I put back the error with the added && !base->reloading condition.

that sounds good to me 👍

akien-mga · 2022-11-28T10:45:15Z

Thanks!

produno · 2022-11-28T11:39:07Z

I compiled from source to test this and i am receiving new errors. Please let me know if i should create a new issue for this, or if i should wait and test the release of Beta 7?

When i try to run my project it complains that it cannot find a property in my inherited class that resides in a base class and shows the error below.

ERROR: Condition "!nc" is true. Returning: false
at: GDScriptCompiler::_is_class_member_property (modules\gdscript\gdscript_compiler.cpp:63)

If i comment that out it will then crash when trying to run showing this:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.0.beta.custom_build (166066d)
Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues
[0] <couldn't map PC to fn name>
[1] <couldn't map PC to fn name>
[2] embree::TaskScheduler::wait
[3] oidnUnmapBuffer
[4] oidnUnmapBuffer
[5] oidnUnmapBuffer
[6] oidnUnmapBuffer
[7] oidnUnmapBuffer
[8] oidnUnmapBuffer
[9] oidnUnmapBuffer
[10] oidnUnmapBuffer
[11] oidnUnmapBuffer
[12] oidnUnmapBuffer
[13] oidnUnmapBuffer
[14] oidnUnmapBuffer
[15] oidnUnmapBuffer
[16] oidnUnmapBuffer
[17] oidnUnmapBuffer
[18] oidnUnmapBuffer
[19] oidnUnmapBuffer
[20] oidnUnmapBuffer
[21] oidnUnmapBuffer
[22] oidnUnmapBuffer
[23] oidnUnmapBuffer
[24] oidnUnmapBuffer
[25] oidnUnmapBuffer
[26] oidnUnmapBuffer
[27] oidnUnmapBuffer
[28] oidnUnmapBuffer
[29] oidnUnmapBuffer
[30] oidnUnmapBuffer
[31] oidnUnmapBuffer
[32] oidnUnmapBuffer
[33] oidnUnmapBuffer
[34] oidnUnmapBuffer
[35] oidnUnmapBuffer
[36] oidnUnmapBuffer
[37] oidnUnmapBuffer
[38] oidnUnmapBuffer
[39] oidnUnmapBuffer
[40] oidnUnmapBuffer
[41] oidnUnmapBuffer
[42] oidnUnmapBuffer
[43] oidnUnmapBuffer
[44] oidnUnmapBuffer
[45] embree::TaskScheduler::wait
[46] embree::TaskScheduler::wait
[47] embree::TaskScheduler::wait
[48] <couldn't map PC to fn name>
[49] <couldn't map PC to fn name>
[50] <couldn't map PC to fn name>
[51] <couldn't map PC to fn name>
[52] embree::TaskScheduler::wait
[53] BaseThreadInitThunk
-- END OF BACKTRACE --

adamscott · 2022-11-28T15:21:15Z

~~@produno As mentioned by @akien-mga in #69295, it seems that there's some issues with libraries. Please follow this issue about your bug.~~

akien-mga · 2022-11-28T15:36:07Z

@adamscott The bug they're reporting is still valid and related to GDScript. #69295 just clarifies that the stacktrace is useless, it doesn't invalidate the fact that it's crashing :)

produno · 2022-12-05T04:16:34Z

Fyi, i am still getting this issue ##69259 (comment)

When starting the game it will show Parser Error: Identifier not found: encloses_room (This property is on a base class) and will show the below in the Debugger/Errors window.

E 0:00:02:0383 _is_class_member_property: Condition "!nc" is true. Returning: false
<C++ Source> modules/gdscript/gdscript_compiler.cpp:63 @ _is_class_member_property()

Removing the property will cause the game to crash (not load). Beta 5 does not cause any errors and the project runs as expected.

adamscott · 2022-12-05T04:18:15Z

Oh, sorry, I completely forgot that issue. Fell out of the radar.
I'll take a look ASAP.

produno · 2022-12-05T04:21:58Z

Oh, sorry, I completely forgot that issue. Fell out of the radar. I'll take a look ASAP.

No problem. Thanks!

adamscott · 2022-12-06T01:00:24Z

@produno Could you test with one of the build artifacts of #69587? Maybe my PR could help (or not!)

produno · 2022-12-06T09:17:14Z

@produno Could you test with one of the build artifacts of #69587? Maybe my PR could help (or not!)

Unfortunately i have the same issue when compiling from your repository and the current master branch.
Heres the backtrace if it helps.

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.0.beta.custom_build (f3e6750)
Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues
[0] <couldn't map PC to fn name>
[1] <couldn't map PC to fn name>
[2] <couldn't map PC to fn name>
[3] <couldn't map PC to fn name>
[4] <couldn't map PC to fn name>
[5] <couldn't map PC to fn name>
[6] <couldn't map PC to fn name>
[7] <couldn't map PC to fn name>
[8] <couldn't map PC to fn name>
[9] <couldn't map PC to fn name>
[10] <couldn't map PC to fn name>
[11] <couldn't map PC to fn name>
[12] <couldn't map PC to fn name>
[13] <couldn't map PC to fn name>
[14] <couldn't map PC to fn name>
[15] <couldn't map PC to fn name>
[16] <couldn't map PC to fn name>
[17] <couldn't map PC to fn name>
[18] <couldn't map PC to fn name>
[19] <couldn't map PC to fn name>
[20] <couldn't map PC to fn name>
[21] <couldn't map PC to fn name>
[22] <couldn't map PC to fn name>
[23] <couldn't map PC to fn name>
[24] <couldn't map PC to fn name>
[25] <couldn't map PC to fn name>
[26] <couldn't map PC to fn name>
[27] <couldn't map PC to fn name>
[28] <couldn't map PC to fn name>
[29] <couldn't map PC to fn name>
[30] <couldn't map PC to fn name>
[31] <couldn't map PC to fn name>
[32] <couldn't map PC to fn name>
[33] <couldn't map PC to fn name>
[34] <couldn't map PC to fn name>
[35] <couldn't map PC to fn name>
[36] <couldn't map PC to fn name>
[37] <couldn't map PC to fn name>
[38] <couldn't map PC to fn name>
[39] <couldn't map PC to fn name>
[40] <couldn't map PC to fn name>
[41] <couldn't map PC to fn name>
[42] <couldn't map PC to fn name>
[43] <couldn't map PC to fn name>
[44] <couldn't map PC to fn name>
[45] <couldn't map PC to fn name>
[46] <couldn't map PC to fn name>
[47] <couldn't map PC to fn name>
[48] <couldn't map PC to fn name>
[49] <couldn't map PC to fn name>
[50] <couldn't map PC to fn name>
[51] <couldn't map PC to fn name>
[52] <couldn't map PC to fn name>
-- END OF BACKTRACE --

If i leave the offending property commented then try reload Godot, my project will not even load the editor. So i need to either reload in Beta 5 and uncomment the property, or modify the text file for it to load up in the current builds.

If there is any way i can actually try and help debug this i am all ears, i dont feel i am being much help atm. Although unfortunately i am back on 12 hour night shifts for the next few days so i will be pretty busy.

I have tried to reproduce the issue in a fresh project but have not managed to so far.

produno · 2022-12-07T09:20:06Z

@adamscott I should probably also note i had a circular dependency issue on the base class in Beta 5. I tried to re-add the type for this in Beta 6 and it still caused issues. When i removed the type, nothing changed as if it had been cached but did not clear, so it still assumed it was of that type, which was still causing issues.

I have no idea if that's related or another issue altogether but before that it didnt complain about the property in the other class, but about the class itself. Only after i changed the type of a property in the base class to cause a circular dependency bug it complained about the other property.

Hopefully i am making sense.

Edit*

Ok, if i change the type of this it will throw a different error every time. If i clear the errors by removing/commenting the problems, the game will crash on load like before.

It also shows this error in the console

ERROR: Parser bug (please report): Trying to check compatibility of unset value type
at: (modules/gdscript/gdscript_analyzer.cpp:4144)

Edit2*

Ok, playing around with this, i tried to remove the offending property on the base class. But now my project wont load again in Beta 7 no matter what i do and when loading in Beta 5 half my scripts are empty, even though they are showing ok in an external editor. So it seems i have managed to completely break it and have no idea how to get it load correctly now or even show my scripts.

Ok last edit - i hope*

I have managed to get it loading in Beta 5 again all ok. The only way i can get the project to even load in Beta 7 is by deleting everything in the imported folder. Then i am back to where i first started where it shows the following error

Could not resolve super class inheritance from ...

If i follow this back through the chain to the supposedly problem class and there are no errors. If i close the project and try re-open it in Beta 7 again then it will not open unless i delete the imported folder again.

adamscott · 2022-12-08T19:46:27Z

@produno If you could create a minimal reproduction project, it would be ideal. I don't know how to reproduce, unfortunately.
If you don't know how, try to copy your project, and start erasing stuff until there's only the issue at hand.

adamscott requested a review from a team as a code owner November 27, 2022 15:38

Chaosus added bug topic:gdscript regression labels Nov 27, 2022

Chaosus added this to the 4.0 milestone Nov 27, 2022

kleonc reviewed Nov 27, 2022

View reviewed changes

rune-scape reviewed Nov 27, 2022

View reviewed changes

Fix cyclic reference base being loaded but not valid (which is ok)

4e60689

adamscott force-pushed the fix-cyclic-reference-base-issue branch from a6c152e to 4e60689 Compare November 27, 2022 20:50

rune-scape approved these changes Nov 27, 2022

View reviewed changes

akien-mga approved these changes Nov 28, 2022

View reviewed changes

akien-mga merged commit 166066d into godotengine:master Nov 28, 2022

akien-mga mentioned this pull request Nov 28, 2022

MSVC crash stacktrace without debug symbols wrongly points to embree and oidn symbols #69295

Closed

TokisanGames mentioned this pull request Nov 28, 2022

Fix cyclic references in GDScript 2.0 #67714

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cyclic reference base being loaded but not valid (which is ok) #69259

Fix cyclic reference base being loaded but not valid (which is ok) #69259

adamscott commented Nov 27, 2022

adamscott commented Nov 27, 2022

kleonc Nov 27, 2022

adamscott Nov 27, 2022 •

edited

Loading

rune-scape Nov 27, 2022

adamscott Nov 27, 2022

kleonc Nov 27, 2022 •

edited

Loading

adamscott Nov 27, 2022

rune-scape Nov 27, 2022

akien-mga Nov 28, 2022

rune-scape left a comment

rune-scape Nov 27, 2022

adamscott Nov 27, 2022

adamscott Nov 27, 2022

rune-scape Nov 27, 2022

akien-mga commented Nov 28, 2022

produno commented Nov 28, 2022 •

edited

Loading

adamscott commented Nov 28, 2022 •

edited

Loading

akien-mga commented Nov 28, 2022

produno commented Dec 5, 2022

adamscott commented Dec 5, 2022

produno commented Dec 5, 2022

adamscott commented Dec 6, 2022

produno commented Dec 6, 2022 •

edited

Loading

produno commented Dec 7, 2022 •

edited

Loading

adamscott commented Dec 8, 2022

	inline bool is_valid() const { return reference != nullptr; }
	inline bool is_null() const { return reference == nullptr; }

	MutexLock lock(GDScriptCache::singleton->mutex);
	GDScriptCache::singleton->shallow_gdscript_cache[source_path] = Ref<GDScript>(this);
	}
	}

	valid = false;
	GDScriptParser parser;
	Error err = parser.parse(source, path, false);
	if (err) {
	if (EngineDebugger::is_active()) {
	GDScriptLanguage::get_singleton()->debug_break_parse(_get_debug_path(), parser.get_errors().front()->get().line, "Parser Error: " + parser.get_errors().front()->get().message);

Fix cyclic reference base being loaded but not valid (which is ok) #69259

Fix cyclic reference base being loaded but not valid (which is ok) #69259

Conversation

adamscott commented Nov 27, 2022

adamscott commented Nov 27, 2022

Choose a reason for hiding this comment

adamscott Nov 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleonc Nov 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rune-scape left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Nov 28, 2022

produno commented Nov 28, 2022 • edited Loading

adamscott commented Nov 28, 2022 • edited Loading

akien-mga commented Nov 28, 2022

produno commented Dec 5, 2022

adamscott commented Dec 5, 2022

produno commented Dec 5, 2022

adamscott commented Dec 6, 2022

produno commented Dec 6, 2022 • edited Loading

produno commented Dec 7, 2022 • edited Loading

adamscott commented Dec 8, 2022

adamscott Nov 27, 2022 •

edited

Loading

kleonc Nov 27, 2022 •

edited

Loading

produno commented Nov 28, 2022 •

edited

Loading

adamscott commented Nov 28, 2022 •

edited

Loading

produno commented Dec 6, 2022 •

edited

Loading

produno commented Dec 7, 2022 •

edited

Loading