Merge pull request #58835 from vnen/gdscript-check-override-signature

GDScript: Check if method signature matches the parent

modules/gdscript/gdscript_analyzer.cpp

@@ -1139,7 +1139,7 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
 #endif // TOOLS_ENABLED
 	}
 
-	if (p_function->identifier->name == "_init") {
+	if (p_function->identifier->name == GDScriptLanguage::get_singleton()->strings._init) {
 		// Constructor.
 		GDScriptParser::DataType return_type = parser->current_class->get_datatype();
 		return_type.is_meta_type = false;
@@ -1153,6 +1153,57 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
 	} else {
 		GDScriptParser::DataType return_type = resolve_datatype(p_function->return_type);
 		p_function->set_datatype(return_type);
+
+#ifdef DEBUG_ENABLED
+		// Check if the function signature matches the parent. If not it's an error since it breaks polymorphism.
+		// Not for the constructor which can vary in signature.
+		GDScriptParser::DataType base_type = parser->current_class->base_type;
+		GDScriptParser::DataType parent_return_type;
+		List<GDScriptParser::DataType> parameters_types;
+		int default_par_count = 0;
+		bool is_static = false;
+		bool is_vararg = false;
+		if (get_function_signature(p_function, false, base_type, p_function->identifier->name, parent_return_type, parameters_types, default_par_count, is_static, is_vararg)) {
+			bool valid = p_function->is_static == is_static;
+			valid = valid && parent_return_type == p_function->get_datatype();
+
+			int par_count_diff = p_function->parameters.size() - parameters_types.size();
+			valid = valid && par_count_diff >= 0;
+			valid = valid && p_function->default_arg_values.size() >= default_par_count + par_count_diff;
+
+			int i = 0;
+			for (const GDScriptParser::DataType &par_type : parameters_types) {
+				valid = valid && par_type == p_function->parameters[i++]->get_datatype();
+			}
+
+			if (!valid) {
+				// Compute parent signature as a string to show in the error message.
+				String parent_signature = parent_return_type.is_hard_type() ? parent_return_type.to_string() : "Variant";
+				if (parent_signature == "null") {
+					parent_signature = "void";
+				}
+				parent_signature += " " + p_function->identifier->name.operator String() + "(";
+				int j = 0;
+				for (const GDScriptParser::DataType &par_type : parameters_types) {
+					if (j > 0) {
+						parent_signature += ", ";
+					}
+					String parameter = par_type.to_string();
+					if (parameter == "null") {
+						parameter = "Variant";
+					}
+					parent_signature += parameter;
+					if (j == parameters_types.size() - default_par_count) {
+						parent_signature += " = default";
+					}
+
+					j++;
+				}
+				parent_signature += ")";
+				push_error(vformat(R"(The function signature doesn't match the parent. Parent signature is "%s".)", parent_signature), p_function);
+			}
+		}
+#endif
 	}
 
 	parser->current_function = previous_function;
@@ -2416,7 +2467,9 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
 	GDScriptParser::DataType return_type;
 	List<GDScriptParser::DataType> par_types;
 
-	if (get_function_signature(p_call, base_type, p_call->function_name, return_type, par_types, default_arg_count, is_static, is_vararg)) {
+	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 the function require typed arrays we must make literals be typed.
 		for (const KeyValue<int, GDScriptParser::ArrayNode *> &E : arrays) {
 			int index = E.key;
@@ -3576,7 +3629,7 @@ GDScriptParser::DataType GDScriptAnalyzer::type_from_property(const PropertyInfo
 	return result;
 }
 
-bool GDScriptAnalyzer::get_function_signature(GDScriptParser::CallNode *p_source, 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) {
+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) {
 	r_static = false;
 	r_vararg = false;
 	r_default_arg_count = 0;
@@ -3616,8 +3669,7 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::CallNode *p_source
 		return false;
 	}
 
-	bool is_constructor = (p_base_type.is_meta_type || (p_source->callee && p_source->callee->type == GDScriptParser::Node::IDENTIFIER)) && p_function == StaticCString::create("new");
-	if (is_constructor) {
+	if (p_is_constructor) {
 		function_name = "_init";
 		r_static = true;
 	}
@@ -3638,7 +3690,7 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::CallNode *p_source
 	}
 
 	if (found_function != nullptr) {
-		r_static = is_constructor || found_function->is_static;
+		r_static = p_is_constructor || found_function->is_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]->default_value != nullptr) {
@@ -3664,7 +3716,7 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::CallNode *p_source
 	}
 
 	// If the base is a script, it might be trying to access members of the Script class itself.
-	if (p_base_type.is_meta_type && !is_constructor && (p_base_type.kind == GDScriptParser::DataType::SCRIPT || p_base_type.kind == GDScriptParser::DataType::CLASS)) {
+	if (p_base_type.is_meta_type && !p_is_constructor && (p_base_type.kind == GDScriptParser::DataType::SCRIPT || p_base_type.kind == GDScriptParser::DataType::CLASS)) {
 		MethodInfo info;
 		StringName script_class = p_base_type.kind == GDScriptParser::DataType::SCRIPT ? p_base_type.script_type->get_class_name() : StringName(GDScript::get_class_static());
 
@@ -3684,7 +3736,7 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::CallNode *p_source
 	}
 #endif
 
-	if (is_constructor) {
+	if (p_is_constructor) {
 		// Native types always have a default constructor.
 		r_return_type = p_base_type;
 		r_return_type.type_source = GDScriptParser::DataType::ANNOTATED_EXPLICIT;

modules/gdscript/gdscript_analyzer.h

@@ -105,7 +105,7 @@ class GDScriptAnalyzer {
 	GDScriptParser::DataType type_from_metatype(const GDScriptParser::DataType &p_meta_type) const;
 	GDScriptParser::DataType type_from_property(const PropertyInfo &p_property) const;
 	GDScriptParser::DataType make_global_class_meta_type(const StringName &p_class_name, const GDScriptParser::Node *p_source);
-	bool get_function_signature(GDScriptParser::CallNode *p_source, 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);
+	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);
 	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 validate_call_arg(const List<GDScriptParser::DataType> &p_par_types, int p_default_args_count, bool p_is_vararg, const GDScriptParser::CallNode *p_call);
 	bool validate_call_arg(const MethodInfo &p_method, const GDScriptParser::CallNode *p_call);

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_parameter_count_less.gd

@@ -0,0 +1,10 @@
+func test():
+	print("Shouldn't reach this")
+
+class Parent:
+	func my_function(_par1: int) -> int:
+		return 0
+
+class Child extends Parent:
+	func my_function() -> int:
+		return 0

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_parameter_count_less.out

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+The function signature doesn't match the parent. Parent signature is "int my_function(int)".

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_parameter_count_more.gd

@@ -0,0 +1,10 @@
+func test():
+	print("Shouldn't reach this")
+
+class Parent:
+	func my_function(_par1: int) -> int:
+		return 0
+
+class Child extends Parent:
+	func my_function(_pary1: int, _par2: int) -> int:
+		return 0

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_parameter_count_more.out

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+The function signature doesn't match the parent. Parent signature is "int my_function(int)".

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_parameter_default_values.gd

@@ -0,0 +1,10 @@
+func test():
+	print("Shouldn't reach this")
+
+class Parent:
+	func my_function(_par1: int = 0) -> int:
+		return 0
+
+class Child extends Parent:
+	func my_function(_par1: int) -> int:
+		return 0

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_parameter_default_values.out

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+The function signature doesn't match the parent. Parent signature is "int my_function(int = default)".

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_parameter_type.gd

@@ -0,0 +1,10 @@
+func test():
+	print("Shouldn't reach this")
+
+class Parent:
+	func my_function(_par1: int) -> int:
+		return 0
+
+class Child extends Parent:
+	func my_function(_par1: Vector2) -> int:
+		return 0

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_parameter_type.out

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+The function signature doesn't match the parent. Parent signature is "int my_function(int)".

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_return_type.gd

@@ -0,0 +1,10 @@
+func test():
+	print("Shouldn't reach this")
+
+class Parent:
+	func my_function() -> int:
+		return 0
+
+class Child extends Parent:
+	func my_function() -> Vector2:
+		return Vector2()

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_return_type.out

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+The function signature doesn't match the parent. Parent signature is "int my_function()".

modules/gdscript/tests/scripts/analyzer/features/function_match_parent_signature_with_extra_parameters.gd

@@ -0,0 +1,17 @@
+func test():
+	var instance := Parent.new()
+	var result := instance.my_function(1)
+	print(result)
+	assert(result == 1)
+	instance = Child.new()
+	result = instance.my_function(2)
+	print(result)
+	assert(result == 0)
+
+class Parent:
+	func my_function(par1: int) -> int:
+		return par1
+
+class Child extends Parent:
+	func my_function(_par1: int, par2: int = 0) -> int:
+		return par2

modules/gdscript/tests/scripts/analyzer/features/function_match_parent_signature_with_extra_parameters.out

@@ -0,0 +1,3 @@
+GDTEST_OK
+1
+0