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 references in GDScript 2.0 #67714

Merged
merged 1 commit into from
Nov 18, 2022
Merged

Fix cyclic references in GDScript 2.0 #67714

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
285 changes: 254 additions & 31 deletions modules/gdscript/gdscript.cpp
View file
Open in desktop

Large diffs are not rendered by default.

16 changes: 16 additions & 0 deletions modules/gdscript/gdscript.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ class GDScript : public Script {
GDCLASS(GDScript, Script);
bool tool = false;
bool valid = false;
bool reloading = false;
bool skip_dependencies = false;

struct MemberInfo {
int index = 0;
Expand Down Expand Up @@ -124,6 +126,8 @@ class GDScript : public Script {

int subclass_count = 0;
RBSet<Object *> instances;
bool destructing = false;
bool clearing = false;
//exported members
String source;
String path;
Expand Down Expand Up @@ -163,6 +167,9 @@ class GDScript : public Script {
// This method will map the class name from "RefCounted" to "MyClass.InnerClass".
static String _get_gdscript_reference_class_name(const GDScript *p_gdscript);

GDScript *_get_gdscript_from_variant(const Variant &p_variant);
void _get_dependencies(RBSet<GDScript *> &p_dependencies, const GDScript *p_except);

protected:
bool _get(const StringName &p_name, Variant &r_ret) const;
bool _set(const StringName &p_name, const Variant &p_value);
Expand All @@ -173,6 +180,8 @@ class GDScript : public Script {
static void _bind_methods();

public:
void clear();

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

bool inherits_script(const Ref<Script> &p_script) const override;
Expand All @@ -193,6 +202,10 @@ class GDScript : public Script {
const Ref<GDScriptNativeClass> &get_native() const { return native; }
const String &get_script_class_name() const { return name; }

RBSet<GDScript *> get_dependencies();
RBSet<GDScript *> get_inverted_dependencies();
RBSet<GDScript *> get_must_clear_dependencies();

virtual bool has_script_signal(const StringName &p_signal) const override;
virtual void get_script_signal_list(List<MethodInfo> *r_signals) const override;

Expand Down Expand Up @@ -270,6 +283,7 @@ class GDScriptInstance : public ScriptInstance {
friend class GDScriptLambdaCallable;
friend class GDScriptLambdaSelfCallable;
friend class GDScriptCompiler;
friend class GDScriptCache;
friend struct GDScriptUtilityFunctionsDefinitions;

ObjectID owner_id;
Expand Down Expand Up @@ -518,6 +532,8 @@ class GDScriptLanguage : public ScriptLanguage {
void add_orphan_subclass(const String &p_qualified_name, const ObjectID &p_subclass);
Ref<GDScript> get_orphan_subclass(const String &p_qualified_name);

Ref<GDScript> get_script_by_fully_qualified_name(const String &p_name);

GDScriptLanguage();
~GDScriptLanguage();
};
Expand Down
136 changes: 94 additions & 42 deletions modules/gdscript/gdscript_analyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "core/templates/hash_map.h"
#include "gdscript.h"
#include "gdscript_utility_functions.h"
#include "scene/resources/packed_scene.h"

static MethodInfo info_from_utility_func(const StringName &p_function) {
ERR_FAIL_COND_V(!Variant::has_utility_function(p_function), MethodInfo());
Expand Down Expand Up @@ -3103,14 +3104,42 @@ void GDScriptAnalyzer::reduce_identifier(GDScriptParser::IdentifierNode *p_ident
GDScriptParser::DataType result;
result.kind = GDScriptParser::DataType::NATIVE;
result.type_source = GDScriptParser::DataType::ANNOTATED_EXPLICIT;
if (autoload.path.to_lower().ends_with(GDScriptLanguage::get_singleton()->get_extension())) {
if (ResourceLoader::get_resource_type(autoload.path) == "GDScript") {
Ref<GDScriptParserRef> singl_parser = get_parser_for(autoload.path);
if (singl_parser.is_valid()) {
Error err = singl_parser->raise_status(GDScriptParserRef::INTERFACE_SOLVED);
if (err == OK) {
result = type_from_metatype(singl_parser->get_parser()->head->get_datatype());
}
}
} else if (ResourceLoader::get_resource_type(autoload.path) == "PackedScene") {
Copy link
Member Author

@adamscott adamscott Oct 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here's the fix for #61386 (the whole else if).

Error err = OK;
Ref<PackedScene> scene = GDScriptCache::get_packed_scene(autoload.path, err);
if (err == OK && scene->get_state().is_valid()) {
Ref<SceneState> state = scene->get_state();
if (state->get_node_count() > 0) {
const int ROOT_NODE = 0;
for (int i = 0; i < state->get_node_property_count(ROOT_NODE); i++) {
if (state->get_node_property_name(ROOT_NODE, i) != SNAME("script")) {
continue;
}

Ref<GDScript> scr = state->get_node_property_value(ROOT_NODE, i);
if (scr.is_null()) {
continue;
}

Ref<GDScriptParserRef> singl_parser = get_parser_for(scr->get_path());
if (singl_parser.is_valid()) {
err = singl_parser->raise_status(GDScriptParserRef::INTERFACE_SOLVED);
if (err == OK) {
result = type_from_metatype(singl_parser->get_parser()->head->get_datatype());
}
}
break;
}
}
}
}
result.is_constant = true;
p_identifier->set_datatype(result);
Expand Down Expand Up @@ -3236,9 +3265,28 @@ void GDScriptAnalyzer::reduce_preload(GDScriptParser::PreloadNode *p_preload) {
}
} else {
// TODO: Don't load if validating: use completion cache.
p_preload->resource = ResourceLoader::load(p_preload->resolved_path);
if (p_preload->resource.is_null()) {
push_error(vformat(R"(Could not preload resource file "%s".)", p_preload->resolved_path), p_preload->path);

// Must load GDScript and PackedScenes separately to permit cyclic references
// as ResourceLoader::load() detect and reject those.
if (ResourceLoader::get_resource_type(p_preload->resolved_path) == "GDScript") {
Error err = OK;
Ref<GDScript> res = GDScriptCache::get_shallow_script(p_preload->resolved_path, err, parser->script_path);
p_preload->resource = res;
if (err != OK) {
push_error(vformat(R"(Could not preload resource script "%s".)", p_preload->resolved_path), p_preload->path);
}
} else if (ResourceLoader::get_resource_type(p_preload->resolved_path) == "PackedScene") {
Copy link
Member

Choose a reason for hiding this comment

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

What would happen with other types of resources (maybe custom ones) that may have a reference to a script?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're in reduce_preload(), so it parses the preload() value. If a GDScript is loaded otherwise, through other resources, the standard loader should work.

Error err = OK;
Ref<PackedScene> res = GDScriptCache::get_packed_scene(p_preload->resolved_path, err, parser->script_path);
p_preload->resource = res;
if (err != OK) {
push_error(vformat(R"(Could not preload resource scene "%s".)", p_preload->resolved_path), p_preload->path);
}
} else {
p_preload->resource = ResourceLoader::load(p_preload->resolved_path);
if (p_preload->resource.is_null()) {
push_error(vformat(R"(Could not preload resource file "%s".)", p_preload->resolved_path), p_preload->path);
}
}
}
}
Expand Down Expand Up @@ -3280,6 +3328,17 @@ void GDScriptAnalyzer::reduce_subscript(GDScriptParser::SubscriptNode *p_subscri
// Just try to get it.
bool valid = false;
Variant value = p_subscript->base->reduced_value.get_named(p_subscript->attribute->name, valid);

// If it's a GDScript instance, try to get the full script. Maybe it's not still completely loaded.
Ref<GDScript> gdscr = Ref<GDScript>(p_subscript->base->reduced_value);
if (!valid && gdscr.is_valid()) {
Error err = OK;
GDScriptCache::get_full_script(gdscr->get_path(), err);
if (err == OK) {
value = p_subscript->base->reduced_value.get_named(p_subscript->attribute->name, valid);
}
}

if (!valid) {
push_error(vformat(R"(Cannot get member "%s" from "%s".)", p_subscript->attribute->name, p_subscript->base->reduced_value), p_subscript->index);
result_type.kind = GDScriptParser::DataType::VARIANT;
Expand Down Expand Up @@ -3662,50 +3721,43 @@ GDScriptParser::DataType GDScriptAnalyzer::type_from_variant(const Variant &p_va
scr = obj->get_script();
}
if (scr.is_valid()) {
if (scr->is_valid()) {
result.script_type = scr;
result.script_path = scr->get_path();
Ref<GDScript> gds = scr;
if (gds.is_valid()) {
result.kind = GDScriptParser::DataType::CLASS;
// This might be an inner class, so we want to get the parser for the root.
// But still get the inner class from that tree.
GDScript *current = gds.ptr();
List<StringName> class_chain;
while (current->_owner) {
// Push to front so it's in reverse.
class_chain.push_front(current->name);
current = current->_owner;
}

Ref<GDScriptParserRef> ref = get_parser_for(current->get_path());
if (ref.is_null()) {
push_error("Could not find script in path.", p_source);
GDScriptParser::DataType error_type;
error_type.kind = GDScriptParser::DataType::VARIANT;
return error_type;
}
ref->raise_status(GDScriptParserRef::INTERFACE_SOLVED);
result.script_type = scr;
result.script_path = scr->get_path();
Ref<GDScript> gds = scr;
if (gds.is_valid()) {
result.kind = GDScriptParser::DataType::CLASS;
// This might be an inner class, so we want to get the parser for the root.
// But still get the inner class from that tree.
GDScript *current = gds.ptr();
List<StringName> class_chain;
while (current->_owner) {
// Push to front so it's in reverse.
class_chain.push_front(current->name);
current = current->_owner;
}

GDScriptParser::ClassNode *found = ref->get_parser()->head;
Ref<GDScriptParserRef> ref = get_parser_for(current->get_path());
if (ref.is_null()) {
push_error("Could not find script in path.", p_source);
GDScriptParser::DataType error_type;
error_type.kind = GDScriptParser::DataType::VARIANT;
return error_type;
}
ref->raise_status(GDScriptParserRef::INTERFACE_SOLVED);

// It should be okay to assume this exists, since we have a complete script already.
for (const StringName &E : class_chain) {
found = found->get_member(E).m_class;
}
GDScriptParser::ClassNode *found = ref->get_parser()->head;

result.class_type = found;
result.script_path = ref->get_parser()->script_path;
} else {
result.kind = GDScriptParser::DataType::SCRIPT;
// It should be okay to assume this exists, since we have a complete script already.
for (const StringName &E : class_chain) {
found = found->get_member(E).m_class;
}
result.native_type = scr->get_instance_base_type();

result.class_type = found;
result.script_path = ref->get_parser()->script_path;
} else {
push_error(vformat(R"(Constant value uses script from "%s" which is loaded but not compiled.)", scr->get_path()), p_source);
result.kind = GDScriptParser::DataType::VARIANT;
result.type_source = GDScriptParser::DataType::UNDETECTED;
result.is_meta_type = false;
result.kind = GDScriptParser::DataType::SCRIPT;
}
result.native_type = scr->get_instance_base_type();
} else {
result.kind = GDScriptParser::DataType::NATIVE;
if (result.native_type == GDScriptNativeClass::get_class_static()) {
Expand Down
Loading