Merge pull request #75988 from dalexeev/gds-unsafe-call-argument

GDScript: Improve call analysis

modules/gdscript/gdscript_analyzer.cpp

@@ -248,7 +248,7 @@ Error GDScriptAnalyzer::check_native_member_name_conflict(const StringName &p_me
 		return ERR_PARSE_ERROR;
 	}
 
-	if (GDScriptParser::get_builtin_type(p_member_name) != Variant::VARIANT_MAX) {
+	if (GDScriptParser::get_builtin_type(p_member_name) < Variant::VARIANT_MAX) {
 		push_error(vformat(R"(The member "%s" cannot have the same name as a builtin type.)", p_member_name), p_member_node);
 		return ERR_PARSE_ERROR;
 	}
@@ -673,11 +673,6 @@ GDScriptParser::DataType GDScriptAnalyzer::resolve_datatype(GDScriptParser::Type
 				return bad_type;
 			}
 			result.kind = GDScriptParser::DataType::VARIANT;
-		} else if (first == SNAME("Object")) {
-			// Object is treated like a native type, not a built-in.
-			result.kind = GDScriptParser::DataType::NATIVE;
-			result.builtin_type = Variant::OBJECT;
-			result.native_type = SNAME("Object");
 		} else if (GDScriptParser::get_builtin_type(first) < Variant::VARIANT_MAX) {
 			// Built-in types.
 			if (p_type->type_chain.size() > 1) {
@@ -1708,7 +1703,7 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
 				}
 				parent_signature += ") -> ";
 
-				const String return_type = parent_return_type.is_hard_type() ? parent_return_type.to_string() : "Variant";
+				const String return_type = parent_return_type.to_string_strict();
 				if (return_type == "null") {
 					parent_signature += "void";
 				} else {
@@ -2899,19 +2894,20 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
 	if (!p_call->is_super && callee_type == GDScriptParser::Node::IDENTIFIER) {
 		// Call to name directly.
 		StringName function_name = p_call->function_name;
-		Variant::Type builtin_type = GDScriptParser::get_builtin_type(function_name);
 
+		if (function_name == SNAME("Object")) {
+			push_error(R"*(Invalid constructor "Object()", use "Object.new()" instead.)*", p_call);
+			p_call->set_datatype(call_type);
+			return;
+		}
+
+		Variant::Type builtin_type = GDScriptParser::get_builtin_type(function_name);
 		if (builtin_type < Variant::VARIANT_MAX) {
 			// Is a builtin constructor.
 			call_type.type_source = GDScriptParser::DataType::ANNOTATED_EXPLICIT;
 			call_type.kind = GDScriptParser::DataType::BUILTIN;
 			call_type.builtin_type = builtin_type;
 
-			if (builtin_type == Variant::OBJECT) {
-				call_type.kind = GDScriptParser::DataType::NATIVE;
-				call_type.native_type = function_name; // "Object".
-			}
-
 			bool safe_to_fold = true;
 			switch (builtin_type) {
 				// Those are stored by reference so not suited for compile-time construction.
@@ -2947,7 +2943,7 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
 
 				switch (err.error) {
 					case Callable::CallError::CALL_ERROR_INVALID_ARGUMENT:
-						push_error(vformat(R"(Invalid argument for %s constructor: argument %d should be "%s" but is "%s".)", Variant::get_type_name(builtin_type), err.argument + 1,
+						push_error(vformat(R"*(Invalid argument for "%s()" constructor: argument %d should be "%s" but is "%s".)*", Variant::get_type_name(builtin_type), err.argument + 1,
 										   Variant::get_type_name(Variant::Type(err.expected)), p_call->arguments[err.argument]->get_datatype().to_string()),
 								p_call->arguments[err.argument]);
 						break;
@@ -2963,10 +2959,10 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
 						push_error(vformat(R"(No constructor of "%s" matches the signature "%s".)", Variant::get_type_name(builtin_type), signature), p_call->callee);
 					} break;
 					case Callable::CallError::CALL_ERROR_TOO_MANY_ARGUMENTS:
-						push_error(vformat(R"(Too many arguments for %s constructor. Received %d but expected %d.)", Variant::get_type_name(builtin_type), p_call->arguments.size(), err.expected), p_call);
+						push_error(vformat(R"*(Too many arguments for "%s()" constructor. Received %d but expected %d.)*", Variant::get_type_name(builtin_type), p_call->arguments.size(), err.expected), p_call);
 						break;
 					case Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS:
-						push_error(vformat(R"(Too few arguments for %s constructor. Received %d but expected %d.)", Variant::get_type_name(builtin_type), p_call->arguments.size(), err.expected), p_call);
+						push_error(vformat(R"*(Too few arguments for "%s()" constructor. Received %d but expected %d.)*", Variant::get_type_name(builtin_type), p_call->arguments.size(), err.expected), p_call);
 						break;
 					case Callable::CallError::CALL_ERROR_INSTANCE_IS_NULL:
 					case Callable::CallError::CALL_ERROR_METHOD_NOT_CONST:
@@ -2977,21 +2973,27 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
 						break;
 				}
 			} else {
-				// TODO: Check constructors without constants.
-
 				// If there's one argument, try to use copy constructor (those aren't explicitly defined).
 				if (p_call->arguments.size() == 1) {
 					GDScriptParser::DataType arg_type = p_call->arguments[0]->get_datatype();
-					if (arg_type.is_variant()) {
-						mark_node_unsafe(p_call->arguments[0]);
-					} else {
+					if (arg_type.is_hard_type() && !arg_type.is_variant()) {
 						if (arg_type.kind == GDScriptParser::DataType::BUILTIN && arg_type.builtin_type == builtin_type) {
 							// Okay.
 							p_call->set_datatype(call_type);
 							return;
 						}
+					} else {
+#ifdef DEBUG_ENABLED
+						mark_node_unsafe(p_call);
+						// We don't know what type was expected since constructors support overloads.
+						// TODO: Improve this by checking for matching candidates?
+						parser->push_warning(p_call->arguments[0], GDScriptWarning::UNSAFE_CALL_ARGUMENT, "1", function_name, "<unknown type>", "Variant");
+#endif
+						p_call->set_datatype(call_type);
+						return;
 					}
 				}
+
 				List<MethodInfo> constructors;
 				Variant::get_constructor_list(builtin_type, &constructors);
 				bool match = false;
@@ -3008,24 +3010,34 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
 
 					for (int i = 0; i < p_call->arguments.size(); i++) {
 						GDScriptParser::DataType par_type = type_from_property(info.arguments[i], true);
-
-						if (!is_type_compatible(par_type, p_call->arguments[i]->get_datatype(), true)) {
+						GDScriptParser::DataType arg_type = p_call->arguments[i]->get_datatype();
+						if (!is_type_compatible(par_type, arg_type, true)) {
 							types_match = false;
 							break;
 #ifdef DEBUG_ENABLED
 						} else {
-							if (par_type.builtin_type == Variant::INT && p_call->arguments[i]->get_datatype().builtin_type == Variant::FLOAT && builtin_type != Variant::INT) {
-								parser->push_warning(p_call, GDScriptWarning::NARROWING_CONVERSION, p_call->function_name);
+							if (par_type.builtin_type == Variant::INT && arg_type.builtin_type == Variant::FLOAT && builtin_type != Variant::INT) {
+								parser->push_warning(p_call, GDScriptWarning::NARROWING_CONVERSION, function_name);
 							}
 #endif
 						}
 					}
 
 					if (types_match) {
 						for (int i = 0; i < p_call->arguments.size(); i++) {
+							GDScriptParser::DataType par_type = type_from_property(info.arguments[i], true);
 							if (p_call->arguments[i]->is_constant) {
-								update_const_expression_builtin_type(p_call->arguments[i], type_from_property(info.arguments[i], true), "pass");
+								update_const_expression_builtin_type(p_call->arguments[i], par_type, "pass");
+							}
+#ifdef DEBUG_ENABLED
+							if (!(par_type.is_variant() && par_type.is_hard_type())) {
+								GDScriptParser::DataType arg_type = p_call->arguments[i]->get_datatype();
+								if (arg_type.is_variant() || !arg_type.is_hard_type() || !is_type_compatible(arg_type, par_type, true)) {
+									mark_node_unsafe(p_call);
+									parser->push_warning(p_call->arguments[i], GDScriptWarning::UNSAFE_CALL_ARGUMENT, itos(i + 1), function_name, par_type.to_string(), arg_type.to_string_strict());
+								}
 							}
+#endif
 						}
 						match = true;
 						call_type = type_from_property(info.return_val);
@@ -3331,8 +3343,8 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
 #else
 			push_error(vformat(R"*(Function "%s()" not found in base %s.)*", p_call->function_name, base_name), p_call->is_super ? p_call : p_call->callee);
 #endif // SUGGEST_GODOT4_RENAMES
-		} else if (!found && (!p_call->is_super && base_type.is_hard_type() && base_type.kind == GDScriptParser::DataType::NATIVE && base_type.is_meta_type)) {
-			push_error(vformat(R"*(Static function "%s()" not found in base "%s".)*", p_call->function_name, base_type.native_type), p_call);
+		} else if (!found && (!p_call->is_super && base_type.is_hard_type() && base_type.is_meta_type)) {
+			push_error(vformat(R"*(Static function "%s()" not found in base "%s".)*", p_call->function_name, base_type.to_string()), p_call);
 		}
 	}
 
@@ -3820,6 +3832,7 @@ void GDScriptAnalyzer::reduce_identifier(GDScriptParser::IdentifierNode *p_ident
 #endif
 
 	// Not a local, so check members.
+
 	if (!found_source) {
 		reduce_identifier_from_base(p_identifier);
 		if (p_identifier->source != GDScriptParser::IdentifierNode::UNDEFINED_SOURCE || p_identifier->get_datatype().is_set()) {
@@ -3872,10 +3885,10 @@ void GDScriptAnalyzer::reduce_identifier(GDScriptParser::IdentifierNode *p_ident
 	StringName name = p_identifier->name;
 	p_identifier->source = GDScriptParser::IdentifierNode::UNDEFINED_SOURCE;
 
-	// Check globals. We make an exception for Variant::OBJECT because it's the base class for
-	// non-builtin types so we allow doing e.g. Object.new()
+	// Not a local or a member, so check globals.
+
 	Variant::Type builtin_type = GDScriptParser::get_builtin_type(name);
-	if (builtin_type != Variant::OBJECT && builtin_type < Variant::VARIANT_MAX) {
+	if (builtin_type < Variant::VARIANT_MAX) {
 		if (can_be_builtin) {
 			p_identifier->set_datatype(make_builtin_meta_type(builtin_type));
 			return;
@@ -5003,21 +5016,28 @@ void GDScriptAnalyzer::validate_call_arg(const List<GDScriptParser::DataType> &p
 		GDScriptParser::DataType arg_type = p_call->arguments[i]->get_datatype();
 
 		if (arg_type.is_variant() || !arg_type.is_hard_type()) {
+#ifdef DEBUG_ENABLED
 			// Argument can be anything, so this is unsafe (unless the parameter is a hard variant).
 			if (!(par_type.is_hard_type() && par_type.is_variant())) {
 				mark_node_unsafe(p_call->arguments[i]);
+				parser->push_warning(p_call->arguments[i], GDScriptWarning::UNSAFE_CALL_ARGUMENT, itos(i + 1), p_call->function_name, par_type.to_string(), arg_type.to_string_strict());
 			}
+#endif
 		} else if (par_type.is_hard_type() && !is_type_compatible(par_type, arg_type, true)) {
-			// Supertypes are acceptable for dynamic compliance, but it's unsafe.
-			mark_node_unsafe(p_call);
 			if (!is_type_compatible(arg_type, par_type)) {
 				push_error(vformat(R"*(Invalid argument for "%s()" function: argument %d should be "%s" but is "%s".)*",
 								   p_call->function_name, i + 1, par_type.to_string(), arg_type.to_string()),
 						p_call->arguments[i]);
+#ifdef DEBUG_ENABLED
+			} else {
+				// Supertypes are acceptable for dynamic compliance, but it's unsafe.
+				mark_node_unsafe(p_call);
+				parser->push_warning(p_call->arguments[i], GDScriptWarning::UNSAFE_CALL_ARGUMENT, itos(i + 1), p_call->function_name, par_type.to_string(), arg_type.to_string_strict());
+#endif
 			}
 #ifdef DEBUG_ENABLED
 		} else if (par_type.kind == GDScriptParser::DataType::BUILTIN && par_type.builtin_type == Variant::INT && arg_type.kind == GDScriptParser::DataType::BUILTIN && arg_type.builtin_type == Variant::FLOAT) {
-			parser->push_warning(p_call, GDScriptWarning::NARROWING_CONVERSION, p_call->function_name);
+			parser->push_warning(p_call->arguments[i], GDScriptWarning::NARROWING_CONVERSION, p_call->function_name);
 #endif
 		}
 	}
@@ -5049,7 +5069,7 @@ void GDScriptAnalyzer::is_shadowing(GDScriptParser::IdentifierNode *p_identifier
 			String class_path = ScriptServer::get_global_class_path(name).get_file();
 			parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_context, name, vformat(R"(global class defined in "%s")", class_path));
 			return;
-		} else if (GDScriptParser::get_builtin_type(name) != Variant::VARIANT_MAX) {
+		} else if (GDScriptParser::get_builtin_type(name) < Variant::VARIANT_MAX) {
 			parser->push_warning(p_identifier, GDScriptWarning::SHADOWED_GLOBAL_IDENTIFIER, p_context, name, "built-in type");
 			return;
 		}

modules/gdscript/gdscript_compiler.cpp

@@ -612,11 +612,8 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_expression(CodeGen &code
 				arguments.push_back(arg);
 			}
 
-			if (!call->is_super && call->callee->type == GDScriptParser::Node::IDENTIFIER && GDScriptParser::get_builtin_type(call->function_name) != Variant::VARIANT_MAX) {
-				// Construct a built-in type.
-				Variant::Type vtype = GDScriptParser::get_builtin_type(static_cast<GDScriptParser::IdentifierNode *>(call->callee)->name);
-
-				gen->write_construct(result, vtype, arguments);
+			if (!call->is_super && call->callee->type == GDScriptParser::Node::IDENTIFIER && GDScriptParser::get_builtin_type(call->function_name) < Variant::VARIANT_MAX) {
+				gen->write_construct(result, GDScriptParser::get_builtin_type(call->function_name), arguments);
 			} else if (!call->is_super && call->callee->type == GDScriptParser::Node::IDENTIFIER && Variant::has_utility_function(call->function_name)) {
 				// Variant utility function.
 				gen->write_call_utility(result, call->function_name, arguments);

modules/gdscript/gdscript_parser.cpp

@@ -52,11 +52,18 @@
 #include "editor/editor_settings.h"
 #endif
 
+// This function is used to determine that a type is "built-in" as opposed to native
+// and custom classes. So `Variant::NIL` and `Variant::OBJECT` are excluded:
+// `Variant::NIL` - `null` is literal, not a type.
+// `Variant::OBJECT` - `Object` should be treated as a class, not as a built-in type.
 static HashMap<StringName, Variant::Type> builtin_types;
 Variant::Type GDScriptParser::get_builtin_type(const StringName &p_type) {
-	if (builtin_types.is_empty()) {
-		for (int i = 1; i < Variant::VARIANT_MAX; i++) {
-			builtin_types[Variant::get_type_name((Variant::Type)i)] = (Variant::Type)i;
+	if (unlikely(builtin_types.is_empty())) {
+		for (int i = 0; i < Variant::VARIANT_MAX; i++) {
+			Variant::Type type = (Variant::Type)i;
+			if (type != Variant::NIL && type != Variant::OBJECT) {
+				builtin_types[Variant::get_type_name(type)] = type;
+			}
 		}
 	}
 

modules/gdscript/gdscript_parser.h

@@ -149,6 +149,7 @@ class GDScriptParser {
 		_FORCE_INLINE_ bool is_hard_type() const { return type_source > INFERRED; }
 
 		String to_string() const;
+		_FORCE_INLINE_ String to_string_strict() const { return is_hard_type() ? to_string() : "Variant"; }
 		PropertyInfo to_property_info(const String &p_name) const;
 
 		_FORCE_INLINE_ void set_container_element_type(const DataType &p_type) {
@@ -1530,7 +1531,7 @@ class GDScriptParser {
 	bool is_tool() const { return _is_tool; }
 	ClassNode *find_class(const String &p_qualified_name) const;
 	bool has_class(const GDScriptParser::ClassNode *p_class) const;
-	static Variant::Type get_builtin_type(const StringName &p_type);
+	static Variant::Type get_builtin_type(const StringName &p_type); // Excluding `Variant::NIL` and `Variant::OBJECT`.
 
 	CompletionContext get_completion_context() const { return completion_context; }
 	CompletionCall get_completion_call() const { return completion_call; }

modules/gdscript/gdscript_warning.cpp

@@ -108,7 +108,7 @@ String GDScriptWarning::get_message() const {
 			return vformat(R"(The value is cast to "%s" but has an unknown type.)", symbols[0]);
 		case UNSAFE_CALL_ARGUMENT:
 			CHECK_SYMBOLS(4);
-			return vformat(R"*(The argument %s of the function "%s()" requires a the subtype "%s" but the supertype "%s" was provided.)*", symbols[0], symbols[1], symbols[2], symbols[3]);
+			return vformat(R"*(The argument %s of the function "%s()" requires the subtype "%s" but the supertype "%s" was provided.)*", symbols[0], symbols[1], symbols[2], symbols[3]);
 		case UNSAFE_VOID_RETURN:
 			CHECK_SYMBOLS(2);
 			return vformat(R"*(The method "%s()" returns "void" but it's trying to return a call to "%s()" that can't be ensured to also be "void".)*", symbols[0], symbols[1]);

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

@@ -0,0 +1,7 @@
+# GH-73283
+
+class MyClass:
+	pass
+
+func test():
+	MyClass.not_existing_method()

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

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+Static function "not_existing_method()" not found in base "MyClass".

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

@@ -0,0 +1,4 @@
+# GH-73213
+
+func test():
+	print(Object())

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

@@ -0,0 +1,2 @@
+GDTEST_ANALYZER_ERROR
+Invalid constructor "Object()", use "Object.new()" instead.

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

@@ -23,6 +23,7 @@ func test() -> void:
 	typed = variant()
 	inferred = variant()
 
+	@warning_ignore("unsafe_call_argument") # TODO: Hard vs Weak vs Unknown.
 	param_weak(typed)
 	param_typed(typed)
 	param_inferred(typed)

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

@@ -6,10 +6,12 @@ var prop = null
 
 func check_arg(arg = null) -> void:
 	if arg != null:
+		@warning_ignore("unsafe_call_argument")
 		print(check(arg))
 
 func check_recur() -> void:
 	if recur != null:
+		@warning_ignore("unsafe_call_argument")
 		print(check(recur))
 	else:
 		recur = 1
@@ -22,11 +24,13 @@ func test() -> void:
 
 	if prop == null:
 		set('prop', 1)
+		@warning_ignore("unsafe_call_argument")
 		print(check(prop))
 		set('prop', null)
 
 	var loop = null
 	while loop != 2:
 		if loop != null:
+			@warning_ignore("unsafe_call_argument")
 			print(check(loop))
 		loop = 1 if loop == null else 2

modules/gdscript/tests/scripts/analyzer/warnings/get_node_without_onready.gd

@@ -14,4 +14,5 @@ func test():
 func do_add_node():
 	var node = Node.new()
 	node.name = "Node"
+	@warning_ignore("unsafe_call_argument")
 	add_child(node)

modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.gd

@@ -0,0 +1,37 @@
+func variant_func(x: Variant) -> void:
+	print(x)
+
+func int_func(x: int) -> void:
+	print(x)
+
+func float_func(x: float) -> void:
+	print(x)
+
+# We don't want to execute it because of errors, just analyze.
+func no_exec_test():
+	var untyped_int = 42
+	var untyped_string = "abc"
+	var variant_int: Variant = 42
+	var variant_string: Variant = "abc"
+	var typed_int: int = 42
+
+	variant_func(untyped_int) # No warning.
+	variant_func(untyped_string) # No warning.
+	variant_func(variant_int) # No warning.
+	variant_func(variant_string) # No warning.
+	variant_func(typed_int) # No warning.
+
+	int_func(untyped_int)
+	int_func(untyped_string)
+	int_func(variant_int)
+	int_func(variant_string)
+	int_func(typed_int) # No warning.
+
+	float_func(untyped_int)
+	float_func(untyped_string)
+	float_func(variant_int)
+	float_func(variant_string)
+	float_func(typed_int) # No warning.
+
+func test():
+	pass