Merge pull request #53103 from ZuBsPaCe/gdscript-analyze-properties-fix

GDScript: Report property type errors

modules/gdscript/gdscript_analyzer.cpp

@@ -875,18 +875,32 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class) {
 	GDScriptParser::ClassNode *previous_class = parser->current_class;
 	parser->current_class = p_class;
 
-	// Do functions now.
+	// Do functions and properties now.
 	for (int i = 0; i < p_class->members.size(); i++) {
 		GDScriptParser::ClassNode::Member member = p_class->members[i];
-		if (member.type != GDScriptParser::ClassNode::Member::FUNCTION) {
-			continue;
-		}
+		if (member.type == GDScriptParser::ClassNode::Member::FUNCTION) {
+			resolve_function_body(member.function);
 
-		resolve_function_body(member.function);
+			// Apply annotations.
+			for (GDScriptParser::AnnotationNode *&E : member.function->annotations) {
+				E->apply(parser, member.function);
+			}
+		} else if (member.type == GDScriptParser::ClassNode::Member::VARIABLE && member.variable->property != GDScriptParser::VariableNode::PROP_NONE) {
+			if (member.variable->property == GDScriptParser::VariableNode::PROP_INLINE) {
+				if (member.variable->getter != nullptr) {
+					member.variable->getter->set_datatype(member.variable->datatype);
 
-		// Apply annotations.
-		for (GDScriptParser::AnnotationNode *&E : member.function->annotations) {
-			E->apply(parser, member.function);
+					resolve_function_body(member.variable->getter);
+				}
+				if (member.variable->setter != nullptr) {
+					if (member.variable->setter->parameters.size() > 0) {
+						member.variable->setter->parameters[0]->datatype_specifier = member.variable->datatype_specifier;
+					}
+
+					resolve_function_signature(member.variable->setter);
+					resolve_function_body(member.variable->setter);
+				}
+			}
 		}
 	}
 
@@ -902,17 +916,80 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class) {
 		resolve_class_body(member.m_class);
 	}
 
-	// Check unused variables.
+	// Check unused variables and datatypes of property getters and setters.
 	for (int i = 0; i < p_class->members.size(); i++) {
 		GDScriptParser::ClassNode::Member member = p_class->members[i];
-		if (member.type != GDScriptParser::ClassNode::Member::VARIABLE) {
-			continue;
-		}
+		if (member.type == GDScriptParser::ClassNode::Member::VARIABLE) {
 #ifdef DEBUG_ENABLED
-		if (member.variable->usages == 0 && String(member.variable->identifier->name).begins_with("_")) {
-			parser->push_warning(member.variable->identifier, GDScriptWarning::UNUSED_PRIVATE_CLASS_VARIABLE, member.variable->identifier->name);
-		}
+			if (member.variable->usages == 0 && String(member.variable->identifier->name).begins_with("_")) {
+				parser->push_warning(member.variable->identifier, GDScriptWarning::UNUSED_PRIVATE_CLASS_VARIABLE, member.variable->identifier->name);
+			}
+#endif
+
+			if (member.variable->property == GDScriptParser::VariableNode::PROP_SETGET) {
+				GDScriptParser::FunctionNode *getter_function = nullptr;
+				GDScriptParser::FunctionNode *setter_function = nullptr;
+
+				bool has_valid_getter = false;
+				bool has_valid_setter = false;
+
+				if (member.variable->getter_pointer != nullptr) {
+					if (p_class->has_function(member.variable->getter_pointer->name)) {
+						getter_function = p_class->get_member(member.variable->getter_pointer->name).function;
+					}
+
+					if (getter_function == nullptr) {
+						push_error(vformat(R"(Getter "%s" not found.)", member.variable->getter_pointer->name), member.variable);
+
+					} else if (getter_function->parameters.size() != 0 || getter_function->datatype.has_no_type()) {
+						push_error(vformat(R"(Function "%s" cannot be used as getter because of its signature.)", getter_function->identifier->name), member.variable);
+
+					} else if (!is_type_compatible(member.variable->datatype, getter_function->datatype, true)) {
+						push_error(vformat(R"(Function with return type "%s" cannot be used as getter for a property of type "%s".)", getter_function->datatype.to_string(), member.variable->datatype.to_string()), member.variable);
+
+					} else {
+						has_valid_getter = true;
+
+#ifdef DEBUG_ENABLED
+						if (member.variable->datatype.builtin_type == Variant::INT && getter_function->datatype.builtin_type == Variant::FLOAT) {
+							parser->push_warning(member.variable, GDScriptWarning::NARROWING_CONVERSION);
+						}
+#endif
+					}
+				}
+
+				if (member.variable->setter_pointer != nullptr) {
+					if (p_class->has_function(member.variable->setter_pointer->name)) {
+						setter_function = p_class->get_member(member.variable->setter_pointer->name).function;
+					}
+
+					if (setter_function == nullptr) {
+						push_error(vformat(R"(Setter "%s" not found.)", member.variable->setter_pointer->name), member.variable);
+
+					} else if (setter_function->parameters.size() != 1) {
+						push_error(vformat(R"(Function "%s" cannot be used as setter because of its signature.)", setter_function->identifier->name), member.variable);
+
+					} else if (!is_type_compatible(member.variable->datatype, setter_function->parameters[0]->datatype, true)) {
+						push_error(vformat(R"(Function with argument type "%s" cannot be used as setter for a property of type "%s".)", setter_function->parameters[0]->datatype.to_string(), member.variable->datatype.to_string()), member.variable);
+
+					} else {
+						has_valid_setter = true;
+
+#ifdef DEBUG_ENABLED
+						if (member.variable->datatype.builtin_type == Variant::INT && setter_function->return_type->datatype.builtin_type == Variant::FLOAT) {
+							parser->push_warning(member.variable, GDScriptWarning::NARROWING_CONVERSION);
+						}
 #endif
+					}
+				}
+
+				if (member.variable->datatype.is_variant() && has_valid_getter && has_valid_setter) {
+					if (!is_type_compatible(getter_function->datatype, setter_function->parameters[0]->datatype, true)) {
+						push_error(vformat(R"(Getter with type "%s" cannot be used along with setter of type "%s".)", getter_function->datatype.to_string(), setter_function->parameters[0]->datatype.to_string()), member.variable);
+					}
+				}
+			}
+		}
 	}
 }
 
@@ -1529,12 +1606,20 @@ void GDScriptAnalyzer::resolve_parameter(GDScriptParser::ParameterNode *p_parame
 void GDScriptAnalyzer::resolve_return(GDScriptParser::ReturnNode *p_return) {
 	GDScriptParser::DataType result;
 
+	GDScriptParser::DataType expected_type;
+	bool has_expected_type = false;
+
+	if (parser->current_function != nullptr) {
+		expected_type = parser->current_function->get_datatype();
+		has_expected_type = true;
+	}
+
 	if (p_return->return_value != nullptr) {
 		reduce_expression(p_return->return_value);
 		if (p_return->return_value->type == GDScriptParser::Node::ARRAY) {
 			// Check if assigned value is an array literal, so we can make it a typed array too if appropriate.
-			if (parser->current_function->get_datatype().has_container_element_type() && p_return->return_value->type == GDScriptParser::Node::ARRAY) {
-				update_array_literal_element_type(parser->current_function->get_datatype(), static_cast<GDScriptParser::ArrayNode *>(p_return->return_value));
+			if (has_expected_type && expected_type.has_container_element_type() && p_return->return_value->type == GDScriptParser::Node::ARRAY) {
+				update_array_literal_element_type(expected_type, static_cast<GDScriptParser::ArrayNode *>(p_return->return_value));
 			}
 		}
 		result = p_return->return_value->get_datatype();
@@ -1546,23 +1631,24 @@ void GDScriptAnalyzer::resolve_return(GDScriptParser::ReturnNode *p_return) {
 		result.is_constant = true;
 	}
 
-	GDScriptParser::DataType function_type = parser->current_function->get_datatype();
-	function_type.is_meta_type = false;
-	if (function_type.is_hard_type()) {
-		if (!is_type_compatible(function_type, result)) {
-			// Try other way. Okay but not safe.
-			if (!is_type_compatible(result, function_type)) {
-				push_error(vformat(R"(Cannot return value of type "%s" because the function return type is "%s".)", result.to_string(), function_type.to_string()), p_return);
-			} else {
-				// TODO: Add warning.
-				mark_node_unsafe(p_return);
-			}
+	if (has_expected_type) {
+		expected_type.is_meta_type = false;
+		if (expected_type.is_hard_type()) {
+			if (!is_type_compatible(expected_type, result)) {
+				// Try other way. Okay but not safe.
+				if (!is_type_compatible(result, expected_type)) {
+					push_error(vformat(R"(Cannot return value of type "%s" because the function return type is "%s".)", result.to_string(), expected_type.to_string()), p_return);
+				} else {
+					// TODO: Add warning.
+					mark_node_unsafe(p_return);
+				}
 #ifdef DEBUG_ENABLED
-		} else if (function_type.builtin_type == Variant::INT && result.builtin_type == Variant::FLOAT) {
-			parser->push_warning(p_return, GDScriptWarning::NARROWING_CONVERSION);
-		} else if (result.is_variant()) {
-			mark_node_unsafe(p_return);
+			} else if (expected_type.builtin_type == Variant::INT && result.builtin_type == Variant::FLOAT) {
+				parser->push_warning(p_return, GDScriptWarning::NARROWING_CONVERSION);
+			} else if (result.is_variant()) {
+				mark_node_unsafe(p_return);
 #endif
+			}
 		}
 	}
 

modules/gdscript/gdscript_compiler.cpp

@@ -2089,77 +2089,18 @@ GDScriptFunction *GDScriptCompiler::_parse_function(Error &r_error, GDScript *p_
 
 Error GDScriptCompiler::_parse_setter_getter(GDScript *p_script, const GDScriptParser::ClassNode *p_class, const GDScriptParser::VariableNode *p_variable, bool p_is_setter) {
 	Error error = OK;
-	CodeGen codegen;
-	codegen.generator = memnew(GDScriptByteCodeGenerator);
-
-	codegen.class_node = p_class;
-	codegen.script = p_script;
 
-	StringName func_name;
+	GDScriptParser::FunctionNode *function;
 
 	if (p_is_setter) {
-		func_name = "@" + p_variable->identifier->name + "_setter";
+		function = p_variable->setter;
 	} else {
-		func_name = "@" + p_variable->identifier->name + "_getter";
+		function = p_variable->getter;
 	}
 
-	codegen.function_name = func_name;
-
-	GDScriptDataType return_type;
-	if (p_is_setter) {
-		return_type.has_type = true;
-		return_type.kind = GDScriptDataType::BUILTIN;
-		return_type.builtin_type = Variant::NIL;
-	} else {
-		return_type = _gdtype_from_datatype(p_variable->get_datatype(), p_script);
-	}
+	_parse_function(error, p_script, p_class, function);
 
-	codegen.generator->write_start(p_script, func_name, false, Multiplayer::RPCConfig(), return_type);
-
-	if (p_is_setter) {
-		uint32_t par_addr = codegen.generator->add_parameter(p_variable->setter_parameter->name, false, _gdtype_from_datatype(p_variable->get_datatype()));
-		codegen.parameters[p_variable->setter_parameter->name] = GDScriptCodeGenerator::Address(GDScriptCodeGenerator::Address::FUNCTION_PARAMETER, par_addr, _gdtype_from_datatype(p_variable->get_datatype()));
-	}
-
-	error = _parse_block(codegen, p_is_setter ? p_variable->setter : p_variable->getter);
-	if (error) {
-		memdelete(codegen.generator);
-		return error;
-	}
-
-	GDScriptFunction *gd_function = codegen.generator->write_end();
-
-	p_script->member_functions[func_name] = gd_function;
-
-#ifdef DEBUG_ENABLED
-	if (EngineDebugger::is_active()) {
-		String signature;
-		//path
-		if (p_script->get_path() != String()) {
-			signature += p_script->get_path();
-		}
-		//loc
-		signature += "::" + itos(p_is_setter ? p_variable->setter->start_line : p_variable->getter->start_line);
-
-		//function and class
-
-		if (p_class->identifier) {
-			signature += "::" + String(p_class->identifier->name) + "." + String(func_name);
-		} else {
-			signature += "::" + String(func_name);
-		}
-
-		codegen.generator->set_signature(signature);
-	}
-#endif
-	codegen.generator->set_initial_line(p_is_setter ? p_variable->setter->start_line : p_variable->getter->start_line);
-
-#ifdef TOOLS_ENABLED
-	p_script->member_lines[func_name] = p_is_setter ? p_variable->setter->start_line : p_variable->getter->start_line;
-#endif
-	memdelete(codegen.generator);
-
-	return OK;
+	return error;
 }
 
 Error GDScriptCompiler::_parse_class_level(GDScript *p_script, const GDScriptParser::ClassNode *p_class, bool p_keep_state) {

modules/gdscript/gdscript_parser.cpp

@@ -947,7 +947,7 @@ GDScriptParser::VariableNode *GDScriptParser::parse_property(VariableNode *p_var
 
 void GDScriptParser::parse_property_setter(VariableNode *p_variable) {
 	switch (p_variable->property) {
-		case VariableNode::PROP_INLINE:
+		case VariableNode::PROP_INLINE: {
 			consume(GDScriptTokenizer::Token::PARENTHESIS_OPEN, R"(Expected "(" after "set".)");
 			if (consume(GDScriptTokenizer::Token::IDENTIFIER, R"(Expected parameter name after "(".)")) {
 				p_variable->setter_parameter = parse_identifier();
@@ -955,9 +955,30 @@ void GDScriptParser::parse_property_setter(VariableNode *p_variable) {
 			consume(GDScriptTokenizer::Token::PARENTHESIS_CLOSE, R"*(Expected ")" after parameter name.)*");
 			consume(GDScriptTokenizer::Token::COLON, R"*(Expected ":" after ")".)*");
 
-			p_variable->setter = parse_suite("setter definition");
-			break;
+			IdentifierNode *identifier = alloc_node<IdentifierNode>();
+			identifier->name = "@" + p_variable->identifier->name + "_setter";
+
+			FunctionNode *function = alloc_node<FunctionNode>();
+			function->identifier = identifier;
+
+			FunctionNode *previous_function = current_function;
+			current_function = function;
+
+			ParameterNode *parameter = alloc_node<ParameterNode>();
+			parameter->identifier = p_variable->setter_parameter;
+
+			function->parameters_indices[parameter->identifier->name] = 0;
+			function->parameters.push_back(parameter);
+
+			SuiteNode *body = alloc_node<SuiteNode>();
+			body->add_local(parameter, function);
+
+			function->body = parse_suite("setter declaration", body);
 
+			p_variable->setter = function;
+			current_function = previous_function;
+			break;
+		}
 		case VariableNode::PROP_SETGET:
 			consume(GDScriptTokenizer::Token::EQUAL, R"(Expected "=" after "set")");
 			make_completion_context(COMPLETION_PROPERTY_METHOD, p_variable);
@@ -972,11 +993,25 @@ void GDScriptParser::parse_property_setter(VariableNode *p_variable) {
 
 void GDScriptParser::parse_property_getter(VariableNode *p_variable) {
 	switch (p_variable->property) {
-		case VariableNode::PROP_INLINE:
+		case VariableNode::PROP_INLINE: {
 			consume(GDScriptTokenizer::Token::COLON, R"(Expected ":" after "get".)");
 
-			p_variable->getter = parse_suite("getter definition");
+			IdentifierNode *identifier = alloc_node<IdentifierNode>();
+			identifier->name = "@" + p_variable->identifier->name + "_getter";
+
+			FunctionNode *function = alloc_node<FunctionNode>();
+			function->identifier = identifier;
+
+			FunctionNode *previous_function = current_function;
+			current_function = function;
+
+			SuiteNode *body = alloc_node<SuiteNode>();
+			function->body = parse_suite("getter declaration", body);
+
+			p_variable->getter = function;
+			current_function = previous_function;
 			break;
+		}
 		case VariableNode::PROP_SETGET:
 			consume(GDScriptTokenizer::Token::EQUAL, R"(Expected "=" after "get")");
 			make_completion_context(COMPLETION_PROPERTY_METHOD, p_variable);
@@ -4437,7 +4472,7 @@ void GDScriptParser::TreePrinter::print_variable(VariableNode *p_variable) {
 			if (p_variable->property == VariableNode::PROP_INLINE) {
 				push_line(":");
 				increase_indent();
-				print_suite(p_variable->getter);
+				print_suite(p_variable->getter->body);
 				decrease_indent();
 			} else {
 				push_line(" =");
@@ -4457,7 +4492,7 @@ void GDScriptParser::TreePrinter::print_variable(VariableNode *p_variable) {
 				}
 				push_line("):");
 				increase_indent();
-				print_suite(p_variable->setter);
+				print_suite(p_variable->setter->body);
 				decrease_indent();
 			} else {
 				push_line(" =");

modules/gdscript/gdscript_parser.h

@@ -1109,12 +1109,12 @@ public:
 
 		PropertyStyle property = PROP_NONE;
 		union {
-			SuiteNode *setter = nullptr;
+			FunctionNode *setter = nullptr;
 			IdentifierNode *setter_pointer;
 		};
 		IdentifierNode *setter_parameter = nullptr;
 		union {
-			SuiteNode *getter = nullptr;
+			FunctionNode *getter = nullptr;
 			IdentifierNode *getter_pointer;
 		};
 

modules/gdscript/language_server/gdscript_extend_parser.cpp

@@ -165,7 +165,7 @@ void ExtendGDScriptParser::parse_class_symbol(const GDScriptParser::ClassNode *p
 			case ClassNode::Member::VARIABLE: {
 				lsp::DocumentSymbol symbol;
 				symbol.name = m.variable->identifier->name;
-				symbol.kind = m.variable->property == VariableNode::PropertyStyle::PROP_NONE ? lsp::SymbolKind::Variable : lsp::SymbolKind::Property;
+				symbol.kind = m.variable->property == VariableNode::PROP_NONE ? lsp::SymbolKind::Variable : lsp::SymbolKind::Property;
 				symbol.deprecated = false;
 				symbol.range.start.line = LINE_NUMBER_TO_INDEX(m.variable->start_line);
 				symbol.range.start.character = LINE_NUMBER_TO_INDEX(m.variable->start_column);

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

@@ -0,0 +1,11 @@
+var _prop : int
+
+# Getter function has wrong return type.
+var prop : String:
+	get = get_prop
+
+func get_prop():
+	return _prop
+
+func test():
+	pass

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

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+Function with return type "int" cannot be used as getter for a property of type "String".

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

@@ -0,0 +1,11 @@
+var _prop : int
+
+# Setter function has wrong argument type.
+var prop : String:
+	set = set_prop
+
+func set_prop(value : int):
+	_prop = value
+
+func test():
+	pass

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

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+Function with argument type "int" cannot be used as setter for a property of type "String".

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

@@ -0,0 +1,9 @@
+var _prop : int
+
+# Inline getter returns int instead of String.
+var prop : String:
+	get:
+		return _prop
+
+func test():
+	pass

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

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+Cannot return value of type "int" because the function return type is "String".

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

@@ -0,0 +1,9 @@
+var _prop : int
+
+# Inline setter assigns String to int.
+var prop : String:
+	set(value):
+		_prop = value
+
+func test():
+	pass

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

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+Cannot assign a value of type "String" to a target of type "int".

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

@@ -0,0 +1,16 @@
+var _prop = 1
+var prop:
+	get = get_prop, set = set_prop
+
+func get_prop():
+	return _prop
+
+func set_prop(value):
+	_prop = value
+
+func test():
+	print(prop)
+
+	prop = 2
+
+	print(prop)

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

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

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

@@ -0,0 +1,46 @@
+# Untyped inline property
+var prop1:
+	get:
+		return prop1
+	set(value):
+		prop1 = value
+
+# Typed inline property
+var prop2 : int:
+	get:
+		return prop2
+	set(value):
+		prop2 = value
+
+# Typed inline property with default value
+var prop3 : int = 1:
+	get:
+		return prop3
+	set(value):
+		prop3 = value
+
+# Typed inline property with backing variable
+var _prop4 : int = 2
+var prop4: int:
+	get:
+		return _prop4
+	set(value):
+		_prop4 = value
+
+func test():
+	print(prop1)
+	print(prop2)
+	print(prop3)
+	print(prop4)
+
+	print()
+
+	prop1 = 1
+	prop2 = 2
+	prop3 = 3
+	prop4 = 4
+
+	print(prop1)
+	print(prop2)
+	print(prop3)
+	print(prop4)

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

@@ -0,0 +1,10 @@
+GDTEST_OK
+null
+0
+1
+2
+
+1
+2
+3
+4