-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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
GDScript: Fix common mismatched external parser errors (reverted) #94617
GDScript: Fix common mismatched external parser errors (reverted) #94617
Conversation
There's a failing unit test:
|
85d5dbf
to
1ecfdde
Compare
1ecfdde
to
a5437d7
Compare
the failing test helped me generalize it more, its much simpler and more robust now |
It is a bit complex but it looks okay to me. I just really don't understand the need of |
it searches the depended parsers of other parsers, so it has to be delayed until they actually resolve and call and the finally pattern is of course to make the code much cleaner |
a5437d7
to
c75225f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as I tested it fixes the bug, and I need it for 4.3 RC 1.
Discussion can continue post-merge on implementation details.
Thanks! |
I'm getting a nullptr exception at the bellow line when I try opening a scene after this PR. godot/modules/gdscript/gdscript_analyzer.cpp Line 3705 in 91eb688
Coming from here: godot/modules/gdscript/gdscript_analyzer.cpp Line 891 in 91eb688
I don't know how the analyzer system works but the second argument (which is p_from_class ) is nullptr so this code path always throws an exception.
Call stack:
|
I don't know if that preserves the intention of this code, but this might fix the crash: diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp
index 990384bcaa..27e1a97b8e 100644
--- a/modules/gdscript/gdscript_analyzer.cpp
+++ b/modules/gdscript/gdscript_analyzer.cpp
@@ -3669,7 +3669,7 @@ Ref<GDScriptParserRef> GDScriptAnalyzer::ensure_cached_parser_for_class(const GD
Ref<GDScriptParserRef> parser_ref;
Ref<GDScriptParserRef> dependant_parser_ref;
GDScriptParser *dependant_parser = parser;
- do {
+ while (p_from_class != nullptr) {
if (HashMap<const GDScriptParser::ClassNode *, Ref<GDScriptParserRef>>::Iterator E = external_class_parser_cache.find(p_from_class)) {
dependant_parser_ref = E->value;
dependant_parser = E->value->get_parser();
@@ -3703,7 +3703,7 @@ Ref<GDScriptParserRef> GDScriptAnalyzer::ensure_cached_parser_for_class(const GD
dependant_parser_ref = Ref<GDScriptParserRef>();
dependant_parser = nullptr;
p_from_class = p_from_class->base_type.class_type;
- } while (p_from_class != nullptr);
+ }
if (parser_ref.is_null()) {
push_error(vformat(R"(Parser bug: Could not find external parser. (%s))", p_context), p_source); (or move it somewhere else) Could you try and confirm? I couldn't reproduce the crash in a couple projects I tested. Edit: Well that might fix the crash but then it will raise the parser bug below: push_error(vformat(R"(Parser bug: Could not find external parser. (%s))", p_context), p_source); |
I think we can just add a null check before dereferencing it: diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp
index 990384bcaa..3299aa44a2 100644
--- a/modules/gdscript/gdscript_analyzer.cpp
+++ b/modules/gdscript/gdscript_analyzer.cpp
@@ -3702,7 +3702,9 @@ Ref<GDScriptParserRef> GDScriptAnalyzer::ensure_cached_parser_for_class(const GD
dependant_parser_ref = Ref<GDScriptParserRef>();
dependant_parser = nullptr;
- p_from_class = p_from_class->base_type.class_type;
+ if (p_from_class != nullptr) {
+ p_from_class = p_from_class->base_type.class_type;
+ }
} while (p_from_class != nullptr);
if (parser_ref.is_null()) { EDIT: Or simply break early: diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp
index 990384bcaa..2e29c141bd 100644
--- a/modules/gdscript/gdscript_analyzer.cpp
+++ b/modules/gdscript/gdscript_analyzer.cpp
@@ -3702,6 +3702,9 @@ Ref<GDScriptParserRef> GDScriptAnalyzer::ensure_cached_parser_for_class(const GD
dependant_parser_ref = Ref<GDScriptParserRef>();
dependant_parser = nullptr;
+ if (p_from_class == nullptr) {
+ break;
+ }
p_from_class = p_from_class->base_type.class_type;
} while (p_from_class != nullptr); |
Moving the while statement causes an infinite loop for scripts that didn't trigger the exception before. |
I can confirm that both suggestions by @vnen resolves the crash but instead results in the errors mentioned by @ydeltastar. |
This maybe isn't the most minimal of MRPs, but it does reproduce the crash/regression mentioned above: cached-parser-crash.zip It's essentially just a clean project with the Cheatsheet add-on added to it, but with one tiny modification done to it, which is that func register(name: String, callback: Callable) -> Command:
var command := db.register(name, callback)
+ if not command.args.is_empty():
+ print(command.args[0].name)
commands_changed.emit()
return command That modification causes the crash, and without that modification the project loads just fine. EDIT: There's a more minimal MRP here as well: #94697 (comment) |
This reverts commit c75225f. This caused a crashing regression for multiple users: godotengine#94617 (comment)
Reverted for now with #94723 to fix the new regression, which is more critical than the one fixed by this PR. |
This reverts commit c75225f. This caused a crashing regression for multiple users: godotengine#94617 (comment)
This reverts commit c75225f. This caused a crashing regression for multiple users: godotengine/godot#94617 (comment)
This reverts commit c75225f. This caused a crashing regression for multiple users: godotengine#94617 (comment)
This reverts commit c75225f. This caused a crashing regression for multiple users: godotengine#94617 (comment)
This reverts commit c75225f. This caused a crashing regression for multiple users: godotengine#94617 (comment)
This reverts commit c75225f. This caused a crashing regression for multiple users: godotengine#94617 (comment)
This reverts commit c75225f. This caused a crashing regression for multiple users: godotengine#94617 (comment)
This reverts commit c75225f. This caused a crashing regression for multiple users: godotengine#94617 (comment)
tried to make this simple and thorough without fundamentally changing how GDScriptCache or GDScriptAnalyzer works
adds a cache of external class nodes that have been requested to be resolved, that map to the
GDScriptParserRef
that owns the class nodethen the external resolve code can use it to find the parser that owns the node it wants to resolve
this is all needed to accommodates multiple different parser trees of the same script in the dependency tree
https://github.com/Zylann/godot_heightmap_plugin was excellent in demonstrating weird edge cases, i feel confident this PR solves the issue generally
also added much more descriptive errors
fixes #94244