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

GDScript: Fix unnecessary calls to remove_parser #95115

Merged
merged 1 commit into from
Aug 6, 2024
Merged

GDScript: Fix unnecessary calls to remove_parser #95115

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
52 changes: 29 additions & 23 deletions modules/gdscript/gdscript_analyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,12 +299,14 @@ void GDScriptAnalyzer::get_class_node_current_scope_classes(GDScriptParser::Clas

// Prioritize node base type over its outer class
if (p_node->base_type.class_type != nullptr) {
ensure_cached_parser_for_class(p_node->base_type.class_type, p_node, vformat(R"(Trying to fetch classes in the current scope: base class of "%s")", p_node->fqcn), p_source);
// TODO: 'ensure_cached_external_parser_for_class()' is only necessary because 'resolve_class_inheritance()' is not getting called here.
ensure_cached_external_parser_for_class(p_node->base_type.class_type, p_node, "Trying to fetch classes in the current scope", p_source);
get_class_node_current_scope_classes(p_node->base_type.class_type, p_list, p_source);
}

if (p_node->outer != nullptr) {
ensure_cached_parser_for_class(p_node->outer, p_node, vformat(R"(Trying to fetch classes in the current scope: outer class of "%s")", p_node->fqcn), p_source);
// TODO: 'ensure_cached_external_parser_for_class()' is only necessary because 'resolve_class_inheritance()' is not getting called here.
ensure_cached_external_parser_for_class(p_node->outer, p_node, "Trying to fetch classes in the current scope", p_source);
get_class_node_current_scope_classes(p_node->outer, p_list, p_source);
}
}
Expand All @@ -314,10 +316,10 @@ Error GDScriptAnalyzer::resolve_class_inheritance(GDScriptParser::ClassNode *p_c
p_source = p_class;
}

Ref<GDScriptParserRef> parser_ref = ensure_cached_parser_for_class(p_class, nullptr, vformat(R"(Trying to resolve class inheritance of "%s")", p_class->fqcn), p_source);
Ref<GDScriptParserRef> parser_ref = ensure_cached_external_parser_for_class(p_class, nullptr, "Trying to resolve class inheritance", p_source);
Finally finally([&]() {
for (GDScriptParser::ClassNode *look_class = p_class; look_class != nullptr; look_class = look_class->base_type.class_type) {
ensure_cached_parser_for_class(look_class->base_type.class_type, look_class, vformat(R"(Trying to resolve class inheritance of "%s")", p_class->fqcn), p_source);
ensure_cached_external_parser_for_class(look_class->base_type.class_type, look_class, "Trying to resolve class inheritance", p_source);
}
});

Expand Down Expand Up @@ -888,12 +890,12 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
p_source = member.get_source_node();
}

Ref<GDScriptParserRef> parser_ref = ensure_cached_parser_for_class(p_class, nullptr, vformat(R"(Trying to resolve class member "%s" of "%s")", member.get_name(), p_class->fqcn), p_source);
Ref<GDScriptParserRef> parser_ref = ensure_cached_external_parser_for_class(p_class, nullptr, "Trying to resolve class member", p_source);
Finally finally([&]() {
ensure_cached_parser_for_class(member.get_datatype().class_type, p_class, vformat(R"(Trying to resolve datatype of class member "%s" of "%s")", member.get_name(), p_class->fqcn), p_source);
ensure_cached_external_parser_for_class(member.get_datatype().class_type, p_class, "Trying to resolve datatype of class member", p_source);
GDScriptParser::DataType member_type = member.get_datatype();
if (member_type.has_container_element_type(0)) {
ensure_cached_parser_for_class(member_type.get_container_element_type(0).class_type, p_class, vformat(R"(Trying to resolve datatype of class member "%s" of "%s")", member.get_name(), p_class->fqcn), p_source);
ensure_cached_external_parser_for_class(member_type.get_container_element_type(0).class_type, p_class, "Trying to resolve datatype of class member", p_source);
}
});

Expand Down Expand Up @@ -1181,7 +1183,7 @@ void GDScriptAnalyzer::resolve_class_interface(GDScriptParser::ClassNode *p_clas
p_source = p_class;
}

Ref<GDScriptParserRef> parser_ref = ensure_cached_parser_for_class(p_class, nullptr, vformat(R"(Trying to resolve class interface of "%s")", p_class->fqcn), p_source);
Ref<GDScriptParserRef> parser_ref = ensure_cached_external_parser_for_class(p_class, nullptr, "Trying to resolve class interface", p_source);

if (!p_class->resolved_interface) {
#ifdef DEBUG_ENABLED
Expand Down Expand Up @@ -1271,7 +1273,7 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class, co
p_source = p_class;
}

Ref<GDScriptParserRef> parser_ref = ensure_cached_parser_for_class(p_class, nullptr, vformat(R"(Trying to resolve class body of "%s")", p_class->fqcn), p_source);
Ref<GDScriptParserRef> parser_ref = ensure_cached_external_parser_for_class(p_class, nullptr, "Trying to resolve class body", p_source);

if (p_class->resolved_body) {
return;
Expand Down Expand Up @@ -3654,19 +3656,19 @@ GDScriptParser::DataType GDScriptAnalyzer::make_global_class_meta_type(const Str
}
}

Ref<GDScriptParserRef> GDScriptAnalyzer::ensure_cached_parser_for_class(const GDScriptParser::ClassNode *p_class, const GDScriptParser::ClassNode *p_from_class, const String &p_context, const GDScriptParser::Node *p_source) {
Ref<GDScriptParserRef> GDScriptAnalyzer::ensure_cached_external_parser_for_class(const GDScriptParser::ClassNode *p_class, const GDScriptParser::ClassNode *p_from_class, const char *p_context, const GDScriptParser::Node *p_source) {
if (p_class == nullptr) {
return nullptr;
}

if (parser->has_class(p_class)) {
return nullptr;
}

if (HashMap<const GDScriptParser::ClassNode *, Ref<GDScriptParserRef>>::Iterator E = external_class_parser_cache.find(p_class)) {
return E->value;
}

if (parser->has_class(p_class)) {
return nullptr;
}

if (p_from_class == nullptr) {
p_from_class = parser->head;
}
Expand All @@ -3676,39 +3678,39 @@ Ref<GDScriptParserRef> GDScriptAnalyzer::ensure_cached_parser_for_class(const GD
Ref<GDScriptParserRef> parser_ref;
for (const GDScriptParser::ClassNode *look_class = p_from_class; look_class != nullptr; look_class = look_class->base_type.class_type) {
if (parser->has_class(look_class)) {
parser_ref = find_cached_parser_for_class(p_class, parser);
parser_ref = find_cached_external_parser_for_class(p_class, parser);
if (parser_ref.is_valid()) {
break;
}
}

if (HashMap<const GDScriptParser::ClassNode *, Ref<GDScriptParserRef>>::Iterator E = external_class_parser_cache.find(look_class)) {
parser_ref = find_cached_parser_for_class(p_class, E->value);
parser_ref = find_cached_external_parser_for_class(p_class, E->value);
if (parser_ref.is_valid()) {
break;
}
}

String look_class_script_path = look_class->get_datatype().script_path;
if (HashMap<String, Ref<GDScriptParserRef>>::Iterator E = parser->depended_parsers.find(look_class_script_path)) {
parser_ref = find_cached_parser_for_class(p_class, E->value);
parser_ref = find_cached_external_parser_for_class(p_class, E->value);
if (parser_ref.is_valid()) {
break;
}
}
}

if (parser_ref.is_null()) {
push_error(vformat(R"(Parser bug: Could not find external parser. (%s))", p_context), p_source);
// A null parser will be inserted into the cache and this error won't spam for the same class.
push_error(vformat(R"(Parser bug (please report): Could not find external parser for class "%s". (%s))", p_class->fqcn, p_context), p_source);
// A null parser will be inserted into the cache, so this error won't spam for the same class.
// This is ok, the values of external_class_parser_cache are not assumed to be valid references.
}

external_class_parser_cache.insert(p_class, parser_ref);
return parser_ref;
}

Ref<GDScriptParserRef> GDScriptAnalyzer::find_cached_parser_for_class(const GDScriptParser::ClassNode *p_class, const Ref<GDScriptParserRef> &p_dependant_parser) {
Ref<GDScriptParserRef> GDScriptAnalyzer::find_cached_external_parser_for_class(const GDScriptParser::ClassNode *p_class, const Ref<GDScriptParserRef> &p_dependant_parser) {
if (p_dependant_parser.is_null()) {
return nullptr;
}
Expand All @@ -3729,10 +3731,10 @@ Ref<GDScriptParserRef> GDScriptAnalyzer::find_cached_parser_for_class(const GDSc

// Silently ensure it's parsed.
p_dependant_parser->raise_status(GDScriptParserRef::PARSED);
return find_cached_parser_for_class(p_class, p_dependant_parser->get_parser());
return find_cached_external_parser_for_class(p_class, p_dependant_parser->get_parser());
}

Ref<GDScriptParserRef> GDScriptAnalyzer::find_cached_parser_for_class(const GDScriptParser::ClassNode *p_class, GDScriptParser *p_dependant_parser) {
Ref<GDScriptParserRef> GDScriptAnalyzer::find_cached_external_parser_for_class(const GDScriptParser::ClassNode *p_class, GDScriptParser *p_dependant_parser) {
if (p_dependant_parser == nullptr) {
return nullptr;
}
Expand Down Expand Up @@ -4474,7 +4476,11 @@ void GDScriptAnalyzer::reduce_preload(GDScriptParser::PreloadNode *p_preload) {
p_preload->is_constant = true;
p_preload->reduced_value = p_preload->resource;
p_preload->set_datatype(type_from_variant(p_preload->reduced_value, p_preload));
ensure_cached_parser_for_class(p_preload->get_datatype().class_type, nullptr, vformat(R"(Trying to resolve preload of "%s")", p_preload->resolved_path), p_preload);

// TODO: Not sure if this is necessary anymore.
// 'type_from_variant()' should call 'resolve_class_inheritance()' which would call 'ensure_cached_external_parser_for_class()'
// Better safe than sorry.
ensure_cached_external_parser_for_class(p_preload->get_datatype().class_type, nullptr, "Trying to resolve preload", p_preload);
}

void GDScriptAnalyzer::reduce_self(GDScriptParser::SelfNode *p_self) {
Expand Down
6 changes: 3 additions & 3 deletions modules/gdscript/gdscript_analyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,9 @@ class GDScriptAnalyzer {
void resolve_pending_lambda_bodies();
bool class_exists(const StringName &p_class) const;
void reduce_identifier_from_base_set_class(GDScriptParser::IdentifierNode *p_identifier, GDScriptParser::DataType p_identifier_datatype);
Ref<GDScriptParserRef> ensure_cached_parser_for_class(const GDScriptParser::ClassNode *p_class, const GDScriptParser::ClassNode *p_from_class, const String &p_context, const GDScriptParser::Node *p_source);
Ref<GDScriptParserRef> find_cached_parser_for_class(const GDScriptParser::ClassNode *p_class, const Ref<GDScriptParserRef> &p_dependant_parser);
Ref<GDScriptParserRef> find_cached_parser_for_class(const GDScriptParser::ClassNode *p_class, GDScriptParser *p_dependant_parser);
Ref<GDScriptParserRef> ensure_cached_external_parser_for_class(const GDScriptParser::ClassNode *p_class, const GDScriptParser::ClassNode *p_from_class, const char *p_context, const GDScriptParser::Node *p_source);
Ref<GDScriptParserRef> find_cached_external_parser_for_class(const GDScriptParser::ClassNode *p_class, const Ref<GDScriptParserRef> &p_dependant_parser);
Ref<GDScriptParserRef> find_cached_external_parser_for_class(const GDScriptParser::ClassNode *p_class, GDScriptParser *p_dependant_parser);
Ref<GDScript> get_depended_shallow_script(const String &p_path, Error &r_error);
#ifdef DEBUG_ENABLED
void is_shadowing(GDScriptParser::IdentifierNode *p_identifier, const String &p_context, const bool p_in_local_scope);
Expand Down
4 changes: 3 additions & 1 deletion modules/gdscript/gdscript_cache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,10 @@ void GDScriptParserRef::clear() {

GDScriptParserRef::~GDScriptParserRef() {
clear();

if (!abandoned) {
GDScriptCache::remove_parser(path);
MutexLock lock(GDScriptCache::singleton->mutex);
GDScriptCache::singleton->parser_map.erase(path);
}
}

Expand Down
Loading