Skip to content

Commit

Permalink
GDScript: add errors when calling unimplemented virtual functions
Browse files Browse the repository at this point in the history
This PR does a small refactor of how method flags are handled in the GDScript analyzer.
This way, it adds support for the analyzer to use any of MethodInfo's flags, where previously
it could only use METHOD_FLAG_STATIC and METHOD_FLAG_VARARG.

As a side-effect, this also normalizes behavior between editor and release templates, which fixes #76938.

The tests added also brought a different issue to light, where using `super()` appears to generate a
return variable discarded on calling super's _init(), which doesn't have a return value. This should be
tackled in a different PR, which will have to change the output of this PR's tests.
  • Loading branch information
anvilfolk committed Jun 15, 2023
1 parent 7734146 commit 861743c
Show file tree
Hide file tree
Showing 8 changed files with 61 additions and 32 deletions.
66 changes: 36 additions & 30 deletions modules/gdscript/gdscript_analyzer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1610,11 +1610,10 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
GDScriptParser::DataType parent_return_type;
List<GDScriptParser::DataType> parameters_types;
int default_par_count = 0;
bool is_static = false;
bool is_vararg = false;
BitField<MethodFlags> method_flags;
StringName native_base;
if (!p_is_lambda && get_function_signature(p_function, false, base_type, function_name, parent_return_type, parameters_types, default_par_count, is_static, is_vararg, &native_base)) {
bool valid = p_function->is_static == is_static;
if (!p_is_lambda && get_function_signature(p_function, false, base_type, function_name, parent_return_type, parameters_types, default_par_count, method_flags, &native_base)) {
bool valid = p_function->is_static == method_flags.has_flag(METHOD_FLAG_STATIC);
valid = valid && parent_return_type == p_function->get_datatype();

int par_count_diff = p_function->parameters.size() - parameters_types.size();
Expand Down Expand Up @@ -2029,9 +2028,8 @@ void GDScriptAnalyzer::resolve_for(GDScriptParser::ForNode *p_for) {
GDScriptParser::DataType return_type;
List<GDScriptParser::DataType> par_types;
int default_arg_count = 0;
bool is_static = false;
bool is_vararg = false;
if (get_function_signature(p_for->list, false, list_type, CoreStringNames::get_singleton()->_iter_get, return_type, par_types, default_arg_count, is_static, is_vararg)) {
BitField<MethodFlags> method_flags;
if (get_function_signature(p_for->list, false, list_type, CoreStringNames::get_singleton()->_iter_get, return_type, par_types, default_arg_count, method_flags)) {
variable_type = return_type;
variable_type.type_source = list_type.type_source;
} else if (!list_type.is_hard_type()) {
Expand Down Expand Up @@ -3084,30 +3082,39 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
return;
}

bool is_static = false;
bool is_vararg = false;
int default_arg_count = 0;
BitField<MethodFlags> method_flags;
GDScriptParser::DataType return_type;
List<GDScriptParser::DataType> par_types;

bool is_constructor = (base_type.is_meta_type || (p_call->callee && p_call->callee->type == GDScriptParser::Node::IDENTIFIER)) && p_call->function_name == SNAME("new");

if (get_function_signature(p_call, is_constructor, base_type, p_call->function_name, return_type, par_types, default_arg_count, is_static, is_vararg)) {
if (get_function_signature(p_call, is_constructor, base_type, p_call->function_name, return_type, par_types, default_arg_count, method_flags)) {
// If the method is implemented in the class hierarchy, the virtual flag will not be set for that MethodInfo and the search stops there.
// MethodInfo's above the class that defines the method might still have the virtual flag set.
if (method_flags.has_flag(METHOD_FLAG_VIRTUAL)) {
if (p_call->is_super) {
push_error(vformat(R"*(Cannot call the parent class' virtual function "%s()" because it hasn't been defined.)*", p_call->function_name), p_call);
} else {
push_error(vformat(R"*(Cannot call virtual function "%s()" because it hasn't been defined.)*", p_call->function_name), p_call);
}
}

// If the function require typed arrays we must make literals be typed.
for (const KeyValue<int, GDScriptParser::ArrayNode *> &E : arrays) {
int index = E.key;
if (index < par_types.size() && par_types[index].has_container_element_type()) {
update_array_literal_element_type(E.value, par_types[index].get_container_element_type());
}
}
validate_call_arg(par_types, default_arg_count, is_vararg, p_call);
validate_call_arg(par_types, default_arg_count, method_flags.has_flag(METHOD_FLAG_VARARG), p_call);

if (base_type.kind == GDScriptParser::DataType::ENUM && base_type.is_meta_type) {
// Enum type is treated as a dictionary value for function calls.
base_type.is_meta_type = false;
}

if (is_self && static_context && !is_static) {
if (is_self && static_context && !method_flags.has_flag(METHOD_FLAG_STATIC)) {
if (parser->current_function) {
// Get the parent function above any lambda.
GDScriptParser::FunctionNode *parent_function = parser->current_function;
Expand All @@ -3118,10 +3125,10 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
} else {
push_error(vformat(R"*(Cannot call non-static function "%s()" for static variable initializer.)*", p_call->function_name), p_call);
}
} else if (!is_self && base_type.is_meta_type && !is_static) {
} else if (!is_self && base_type.is_meta_type && !method_flags.has_flag(METHOD_FLAG_STATIC)) {
base_type.is_meta_type = false; // For `to_string()`.
push_error(vformat(R"*(Cannot call non-static function "%s()" on the class "%s" directly. Make an instance instead.)*", p_call->function_name, base_type.to_string()), p_call);
} else if (is_self && !is_static) {
} else if (is_self && !method_flags.has_flag(METHOD_FLAG_STATIC)) {
mark_lambda_use_self();
}

Expand All @@ -3135,7 +3142,7 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
parser->push_warning(p_call, GDScriptWarning::RETURN_VALUE_DISCARDED, p_call->function_name);
}

if (is_static && !is_constructor && !base_type.is_meta_type && !(is_self && static_context)) {
if (method_flags.has_flag(METHOD_FLAG_STATIC) && !is_constructor && !base_type.is_meta_type && !(is_self && static_context)) {
String caller_type = String(base_type.native_type);

if (caller_type.is_empty()) {
Expand Down Expand Up @@ -4651,9 +4658,8 @@ GDScriptParser::DataType GDScriptAnalyzer::type_from_property(const PropertyInfo
return result;
}

bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bool p_is_constructor, GDScriptParser::DataType p_base_type, const StringName &p_function, GDScriptParser::DataType &r_return_type, List<GDScriptParser::DataType> &r_par_types, int &r_default_arg_count, bool &r_static, bool &r_vararg, StringName *r_native_class) {
r_static = false;
r_vararg = false;
bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bool p_is_constructor, GDScriptParser::DataType p_base_type, const StringName &p_function, GDScriptParser::DataType &r_return_type, List<GDScriptParser::DataType> &r_par_types, int &r_default_arg_count, BitField<MethodFlags> &r_method_flags, StringName *r_native_class) {
r_method_flags = METHOD_FLAGS_DEFAULT;
r_default_arg_count = 0;
if (r_native_class) {
*r_native_class = StringName();
Expand Down Expand Up @@ -4686,10 +4692,9 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bo

for (const MethodInfo &E : methods) {
if (E.name == p_function) {
function_signature_from_info(E, r_return_type, r_par_types, r_default_arg_count, r_static, r_vararg);
r_static = Variant::is_builtin_method_static(p_base_type.builtin_type, function_name);
function_signature_from_info(E, r_return_type, r_par_types, r_default_arg_count, r_method_flags);
// Cannot use non-const methods on enums.
if (!r_static && was_enum && !(E.flags & METHOD_FLAG_CONST)) {
if (!r_method_flags.has_flag(METHOD_FLAG_STATIC) && was_enum && !(E.flags & METHOD_FLAG_CONST)) {
push_error(vformat(R"*(Cannot call non-const Dictionary function "%s()" on enum "%s".)*", p_function, p_base_type.enum_type), p_source);
}
return true;
Expand Down Expand Up @@ -4720,7 +4725,7 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bo

if (p_is_constructor) {
function_name = GDScriptLanguage::get_singleton()->strings._init;
r_static = true;
r_method_flags.set_flag(METHOD_FLAG_STATIC);
}

GDScriptParser::ClassNode *base_class = p_base_type.class_type;
Expand All @@ -4743,7 +4748,9 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bo
}

if (found_function != nullptr) {
r_static = p_is_constructor || found_function->is_static;
if (p_is_constructor || found_function->is_static) {
r_method_flags.set_flag(METHOD_FLAG_STATIC);
}
for (int i = 0; i < found_function->parameters.size(); i++) {
r_par_types.push_back(found_function->parameters[i]->get_datatype());
if (found_function->parameters[i]->initializer != nullptr) {
Expand All @@ -4763,7 +4770,7 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bo
MethodInfo info = base_script->get_method_info(function_name);

if (!(info == MethodInfo())) {
return function_signature_from_info(info, r_return_type, r_par_types, r_default_arg_count, r_static, r_vararg);
return function_signature_from_info(info, r_return_type, r_par_types, r_default_arg_count, r_method_flags);
}
base_script = base_script->get_base_script();
}
Expand All @@ -4774,7 +4781,7 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bo
StringName script_class = p_base_type.kind == GDScriptParser::DataType::SCRIPT ? p_base_type.script_type->get_class_name() : StringName(GDScript::get_class_static());

if (ClassDB::get_method_info(script_class, function_name, &info)) {
return function_signature_from_info(info, r_return_type, r_par_types, r_default_arg_count, r_static, r_vararg);
return function_signature_from_info(info, r_return_type, r_par_types, r_default_arg_count, r_method_flags);
}
}

Expand All @@ -4788,9 +4795,9 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bo

MethodInfo info;
if (ClassDB::get_method_info(base_native, function_name, &info)) {
bool valid = function_signature_from_info(info, r_return_type, r_par_types, r_default_arg_count, r_static, r_vararg);
bool valid = function_signature_from_info(info, r_return_type, r_par_types, r_default_arg_count, r_method_flags);
if (valid && Engine::get_singleton()->has_singleton(base_native)) {
r_static = true;
r_method_flags.set_flag(METHOD_FLAG_STATIC);
}
#ifdef DEBUG_ENABLED
MethodBind *native_method = ClassDB::get_method(base_native, function_name);
Expand All @@ -4804,11 +4811,10 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bo
return false;
}

bool GDScriptAnalyzer::function_signature_from_info(const MethodInfo &p_info, GDScriptParser::DataType &r_return_type, List<GDScriptParser::DataType> &r_par_types, int &r_default_arg_count, bool &r_static, bool &r_vararg) {
bool GDScriptAnalyzer::function_signature_from_info(const MethodInfo &p_info, GDScriptParser::DataType &r_return_type, List<GDScriptParser::DataType> &r_par_types, int &r_default_arg_count, BitField<MethodFlags> &r_method_flags) {
r_return_type = type_from_property(p_info.return_val);
r_default_arg_count = p_info.default_arguments.size();
r_vararg = (p_info.flags & METHOD_FLAG_VARARG) != 0;
r_static = (p_info.flags & METHOD_FLAG_STATIC) != 0;
r_method_flags = p_info.flags;

for (const PropertyInfo &E : p_info.arguments) {
r_par_types.push_back(type_from_property(E, true));
Expand Down
4 changes: 2 additions & 2 deletions modules/gdscript/gdscript_analyzer.h
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,8 @@ class GDScriptAnalyzer {
static GDScriptParser::DataType type_from_metatype(const GDScriptParser::DataType &p_meta_type);
GDScriptParser::DataType type_from_property(const PropertyInfo &p_property, bool p_is_arg = false, bool p_is_readonly = false) const;
GDScriptParser::DataType make_global_class_meta_type(const StringName &p_class_name, const GDScriptParser::Node *p_source);
bool get_function_signature(GDScriptParser::Node *p_source, bool p_is_constructor, GDScriptParser::DataType base_type, const StringName &p_function, GDScriptParser::DataType &r_return_type, List<GDScriptParser::DataType> &r_par_types, int &r_default_arg_count, bool &r_static, bool &r_vararg, StringName *r_native_class = nullptr);
bool function_signature_from_info(const MethodInfo &p_info, GDScriptParser::DataType &r_return_type, List<GDScriptParser::DataType> &r_par_types, int &r_default_arg_count, bool &r_static, bool &r_vararg);
bool get_function_signature(GDScriptParser::Node *p_source, bool p_is_constructor, GDScriptParser::DataType base_type, const StringName &p_function, GDScriptParser::DataType &r_return_type, List<GDScriptParser::DataType> &r_par_types, int &r_default_arg_count, BitField<MethodFlags> &r_method_flags, StringName *r_native_class = nullptr);
bool function_signature_from_info(const MethodInfo &p_info, GDScriptParser::DataType &r_return_type, List<GDScriptParser::DataType> &r_par_types, int &r_default_arg_count, BitField<MethodFlags> &r_method_flags);
void validate_call_arg(const List<GDScriptParser::DataType> &p_par_types, int p_default_args_count, bool p_is_vararg, const GDScriptParser::CallNode *p_call);
void validate_call_arg(const MethodInfo &p_method, const GDScriptParser::CallNode *p_call);
GDScriptParser::DataType get_operation_type(Variant::Operator p_operation, const GDScriptParser::DataType &p_a, const GDScriptParser::DataType &p_b, bool &r_valid, const GDScriptParser::Node *p_source);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
func test():
_get_property_list()
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
GDTEST_ANALYZER_ERROR
Cannot call virtual function "_get_property_list()" because it hasn't been defined.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
func _init():
super()

func test():
pass
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
GDTEST_ANALYZER_ERROR
Cannot call the parent class' virtual function "_init()" because it hasn't been defined.
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class TestOne:
func _get_property_list():
return {}

class TestTwo extends TestOne:
func _init():
var _x = _get_property_list()

func test():
var x = TestTwo.new()
var _x = x._get_property_list()
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
GDTEST_OK

0 comments on commit 861743c

Please sign in to comment.