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.

Sign up for GitHub

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 reference base being loaded but not valid (which is ok) #69259

Merged
merged 1 commit into from
Nov 28, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/gdscript/gdscript_cache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ Ref<GDScript> GDScriptCache::get_full_script(const String &p_path, Error &r_erro
Ref<GDScript> script;
r_error = OK;
if (singleton->full_gdscript_cache.has(p_path)) {
script = Ref<GDScript>(singleton->full_gdscript_cache[p_path]);
script = singleton->full_gdscript_cache[p_path];
if (!p_update_from_disk) {
return script;
}
Expand Down
2 changes: 1 addition & 1 deletion modules/gdscript/gdscript_compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2284,7 +2284,7 @@ Error GDScriptCompiler::_populate_class_members(GDScript *p_script, const GDScri
_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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds good to me 👍

ERR_FAIL_COND_V(!base->is_valid() && !base->reloading, ERR_BUG);
}

p_script->base = base;
Expand Down