From ba96d4f63160a0c20f35906ecb536dbb2ae53f94 Mon Sep 17 00:00:00 2001 From: Danil Alexeev Date: Fri, 29 Sep 2023 22:43:56 +0300 Subject: [PATCH] GDScript: Fix `UNSAFE_CALL_ARGUMENT` warning for `Variant` constructors --- modules/gdscript/gdscript_analyzer.cpp | 28 +++++++++---- modules/gdscript/gdscript_warning.cpp | 4 +- .../analyzer/warnings/unsafe_call_argument.gd | 17 ++++++++ .../warnings/unsafe_call_argument.out | 40 +++++++++++++++---- .../scripts/runtime/features/stringify.gd | 1 - 5 files changed, 72 insertions(+), 18 deletions(-) diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp index 882c246706dd..0d06597bc0e7 100644 --- a/modules/gdscript/gdscript_analyzer.cpp +++ b/modules/gdscript/gdscript_analyzer.cpp @@ -3022,9 +3022,23 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a } 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, "", "Variant"); + // Constructors support overloads. + Vector types; + for (int i = 0; i < Variant::VARIANT_MAX; i++) { + if (i != builtin_type && Variant::can_convert_strict((Variant::Type)i, builtin_type)) { + types.push_back(Variant::get_type_name((Variant::Type)i)); + } + } + String expected_types = function_name; + if (types.size() == 1) { + expected_types += "\" or \"" + types[0]; + } else if (types.size() >= 2) { + for (int i = 0; i < types.size() - 1; i++) { + expected_types += "\", \"" + types[i]; + } + expected_types += "\", or \"" + types[types.size() - 1]; + } + parser->push_warning(p_call->arguments[0], GDScriptWarning::UNSAFE_CALL_ARGUMENT, "1", "constructor", function_name, expected_types, "Variant"); #endif p_call->set_datatype(call_type); return; @@ -3069,9 +3083,9 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a #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)) { + if (arg_type.is_variant() || !arg_type.is_hard_type()) { 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()); + parser->push_warning(p_call->arguments[i], GDScriptWarning::UNSAFE_CALL_ARGUMENT, itos(i + 1), "constructor", function_name, par_type.to_string(), arg_type.to_string_strict()); } } #endif @@ -5057,7 +5071,7 @@ void GDScriptAnalyzer::validate_call_arg(const List &p // 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()); + parser->push_warning(p_call->arguments[i], GDScriptWarning::UNSAFE_CALL_ARGUMENT, itos(i + 1), "function", 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)) { @@ -5069,7 +5083,7 @@ void GDScriptAnalyzer::validate_call_arg(const List &p } 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()); + parser->push_warning(p_call->arguments[i], GDScriptWarning::UNSAFE_CALL_ARGUMENT, itos(i + 1), "function", p_call->function_name, par_type.to_string(), arg_type.to_string_strict()); #endif } #ifdef DEBUG_ENABLED diff --git a/modules/gdscript/gdscript_warning.cpp b/modules/gdscript/gdscript_warning.cpp index cabac07ef94d..1fe9b0425c83 100644 --- a/modules/gdscript/gdscript_warning.cpp +++ b/modules/gdscript/gdscript_warning.cpp @@ -107,8 +107,8 @@ String GDScriptWarning::get_message() const { CHECK_SYMBOLS(1); 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 the subtype "%s" but the supertype "%s" was provided.)*", symbols[0], symbols[1], symbols[2], symbols[3]); + CHECK_SYMBOLS(5); + return vformat(R"*(The argument %s of the %s "%s()" requires the subtype "%s" but the supertype "%s" was provided.)*", symbols[0], symbols[1], symbols[2], symbols[3], symbols[4]); 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]); diff --git a/modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.gd b/modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.gd index 573060ae0ff4..c6d9b37485ab 100644 --- a/modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.gd +++ b/modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.gd @@ -7,8 +7,12 @@ func int_func(x: int) -> void: func float_func(x: float) -> void: print(x) +func node_func(x: Node) -> void: + print(x) + # We don't want to execute it because of errors, just analyze. func no_exec_test(): + var variant: Variant = null var untyped_int = 42 var untyped_string = "abc" var variant_int: Variant = 42 @@ -33,5 +37,18 @@ func no_exec_test(): float_func(variant_string) float_func(typed_int) # No warning. + node_func(variant) + node_func(Object.new()) + node_func(Node.new()) # No warning. + node_func(Node2D.new()) # No warning. + + # GH-82529 + print(Callable(self, "test")) # No warning. + print(Callable(variant, "test")) + + print(Dictionary(variant)) + print(Vector2(variant)) + print(int(variant)) + func test(): pass diff --git a/modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.out b/modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.out index b8fcb671581a..30845152335a 100644 --- a/modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.out +++ b/modules/gdscript/tests/scripts/analyzer/warnings/unsafe_call_argument.out @@ -1,33 +1,57 @@ GDTEST_OK >> WARNING ->> Line: 24 +>> Line: 28 >> UNSAFE_CALL_ARGUMENT >> The argument 1 of the function "int_func()" requires the subtype "int" but the supertype "Variant" was provided. >> WARNING ->> Line: 25 +>> Line: 29 >> UNSAFE_CALL_ARGUMENT >> The argument 1 of the function "int_func()" requires the subtype "int" but the supertype "Variant" was provided. >> WARNING ->> Line: 26 +>> Line: 30 >> UNSAFE_CALL_ARGUMENT >> The argument 1 of the function "int_func()" requires the subtype "int" but the supertype "Variant" was provided. >> WARNING ->> Line: 27 +>> Line: 31 >> UNSAFE_CALL_ARGUMENT >> The argument 1 of the function "int_func()" requires the subtype "int" but the supertype "Variant" was provided. >> WARNING ->> Line: 30 +>> Line: 34 >> UNSAFE_CALL_ARGUMENT >> The argument 1 of the function "float_func()" requires the subtype "float" but the supertype "Variant" was provided. >> WARNING ->> Line: 31 +>> Line: 35 >> UNSAFE_CALL_ARGUMENT >> The argument 1 of the function "float_func()" requires the subtype "float" but the supertype "Variant" was provided. >> WARNING ->> Line: 32 +>> Line: 36 >> UNSAFE_CALL_ARGUMENT >> The argument 1 of the function "float_func()" requires the subtype "float" but the supertype "Variant" was provided. >> WARNING ->> Line: 33 +>> Line: 37 >> UNSAFE_CALL_ARGUMENT >> The argument 1 of the function "float_func()" requires the subtype "float" but the supertype "Variant" was provided. +>> WARNING +>> Line: 40 +>> UNSAFE_CALL_ARGUMENT +>> The argument 1 of the function "node_func()" requires the subtype "Node" but the supertype "Variant" was provided. +>> WARNING +>> Line: 41 +>> UNSAFE_CALL_ARGUMENT +>> The argument 1 of the function "node_func()" requires the subtype "Node" but the supertype "Object" was provided. +>> WARNING +>> Line: 47 +>> UNSAFE_CALL_ARGUMENT +>> The argument 1 of the constructor "Callable()" requires the subtype "Object" but the supertype "Variant" was provided. +>> WARNING +>> Line: 49 +>> UNSAFE_CALL_ARGUMENT +>> The argument 1 of the constructor "Dictionary()" requires the subtype "Dictionary" but the supertype "Variant" was provided. +>> WARNING +>> Line: 50 +>> UNSAFE_CALL_ARGUMENT +>> The argument 1 of the constructor "Vector2()" requires the subtype "Vector2" or "Vector2i" but the supertype "Variant" was provided. +>> WARNING +>> Line: 51 +>> UNSAFE_CALL_ARGUMENT +>> The argument 1 of the constructor "int()" requires the subtype "int", "bool", or "float" but the supertype "Variant" was provided. diff --git a/modules/gdscript/tests/scripts/runtime/features/stringify.gd b/modules/gdscript/tests/scripts/runtime/features/stringify.gd index 1e66d8f34a29..0dbb252b0e3f 100644 --- a/modules/gdscript/tests/scripts/runtime/features/stringify.gd +++ b/modules/gdscript/tests/scripts/runtime/features/stringify.gd @@ -24,7 +24,6 @@ func test(): print(StringName("hello")) print(NodePath("hello/world")) var node := Node.new() - @warning_ignore("unsafe_call_argument") print(RID(node)) # TODO: Why is the constructor (or implicit cast) not documented? print(node.get_name) print(node.property_list_changed)