From 0c2202c56e4c87c53dde17b35c8677974985ae81 Mon Sep 17 00:00:00 2001 From: Danil Alexeev Date: Fri, 30 Jun 2023 20:40:02 +0300 Subject: [PATCH] GDScript: Fix incorrect error message for utility functions --- core/variant/variant_utility.cpp | 134 ++++++++++++++---- doc/classes/@GlobalScope.xml | 6 +- modules/gdscript/gdscript_analyzer.cpp | 34 ++--- .../gdscript/gdscript_utility_functions.cpp | 11 +- modules/gdscript/gdscript_vm.cpp | 15 +- .../errors/gd_utility_function_wrong_arg.gd | 2 + .../errors/gd_utility_function_wrong_arg.out | 2 + .../errors/utility_function_wrong_arg.gd | 2 + .../errors/utility_function_wrong_arg.out | 2 + .../errors/gd_utility_function_wrong_arg.gd | 3 + .../errors/gd_utility_function_wrong_arg.out | 6 + .../errors/utility_function_wrong_arg.gd | 3 + .../errors/utility_function_wrong_arg.out | 6 + 13 files changed, 167 insertions(+), 59 deletions(-) create mode 100644 modules/gdscript/tests/scripts/analyzer/errors/gd_utility_function_wrong_arg.gd create mode 100644 modules/gdscript/tests/scripts/analyzer/errors/gd_utility_function_wrong_arg.out create mode 100644 modules/gdscript/tests/scripts/analyzer/errors/utility_function_wrong_arg.gd create mode 100644 modules/gdscript/tests/scripts/analyzer/errors/utility_function_wrong_arg.out create mode 100644 modules/gdscript/tests/scripts/runtime/errors/gd_utility_function_wrong_arg.gd create mode 100644 modules/gdscript/tests/scripts/runtime/errors/gd_utility_function_wrong_arg.out create mode 100644 modules/gdscript/tests/scripts/runtime/errors/utility_function_wrong_arg.gd create mode 100644 modules/gdscript/tests/scripts/runtime/errors/utility_function_wrong_arg.out diff --git a/core/variant/variant_utility.cpp b/core/variant/variant_utility.cpp index 6f334d185930..cc48394b64c6 100644 --- a/core/variant/variant_utility.cpp +++ b/core/variant/variant_utility.cpp @@ -121,16 +121,27 @@ Variant VariantUtilityFunctions::floor(Variant x, Callable::CallError &r_error) case Variant::VECTOR2: { return VariantInternalAccessor::get(&x).floor(); } break; + case Variant::VECTOR2I: { + return VariantInternalAccessor::get(&x); + } break; case Variant::VECTOR3: { return VariantInternalAccessor::get(&x).floor(); } break; + case Variant::VECTOR3I: { + return VariantInternalAccessor::get(&x); + } break; case Variant::VECTOR4: { return VariantInternalAccessor::get(&x).floor(); } break; + case Variant::VECTOR4I: { + return VariantInternalAccessor::get(&x); + } break; default: { - r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - return Variant(); - } + r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; + r_error.argument = 0; + r_error.expected = Variant::NIL; + return R"(Argument "x" must be "int", "float", "Vector2", "Vector2i", "Vector3", "Vector3i", "Vector4", or "Vector4i".)"; + } break; } } @@ -154,16 +165,27 @@ Variant VariantUtilityFunctions::ceil(Variant x, Callable::CallError &r_error) { case Variant::VECTOR2: { return VariantInternalAccessor::get(&x).ceil(); } break; + case Variant::VECTOR2I: { + return VariantInternalAccessor::get(&x); + } break; case Variant::VECTOR3: { return VariantInternalAccessor::get(&x).ceil(); } break; + case Variant::VECTOR3I: { + return VariantInternalAccessor::get(&x); + } break; case Variant::VECTOR4: { return VariantInternalAccessor::get(&x).ceil(); } break; + case Variant::VECTOR4I: { + return VariantInternalAccessor::get(&x); + } break; default: { - r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - return Variant(); - } + r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; + r_error.argument = 0; + r_error.expected = Variant::NIL; + return R"(Argument "x" must be "int", "float", "Vector2", "Vector2i", "Vector3", "Vector3i", "Vector4", or "Vector4i".)"; + } break; } } @@ -187,16 +209,27 @@ Variant VariantUtilityFunctions::round(Variant x, Callable::CallError &r_error) case Variant::VECTOR2: { return VariantInternalAccessor::get(&x).round(); } break; + case Variant::VECTOR2I: { + return VariantInternalAccessor::get(&x); + } break; case Variant::VECTOR3: { return VariantInternalAccessor::get(&x).round(); } break; + case Variant::VECTOR3I: { + return VariantInternalAccessor::get(&x); + } break; case Variant::VECTOR4: { return VariantInternalAccessor::get(&x).round(); } break; + case Variant::VECTOR4I: { + return VariantInternalAccessor::get(&x); + } break; default: { - r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - return Variant(); - } + r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; + r_error.argument = 0; + r_error.expected = Variant::NIL; + return R"(Argument "x" must be "int", "float", "Vector2", "Vector2i", "Vector3", "Vector3i", "Vector4", or "Vector4i".)"; + } break; } } @@ -236,9 +269,11 @@ Variant VariantUtilityFunctions::abs(const Variant &x, Callable::CallError &r_er return VariantInternalAccessor::get(&x).abs(); } break; default: { - r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - return Variant(); - } + r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; + r_error.argument = 0; + r_error.expected = Variant::NIL; + return R"(Argument "x" must be "int", "float", "Vector2", "Vector2i", "Vector3", "Vector3i", "Vector4", or "Vector4i".)"; + } break; } } @@ -278,9 +313,11 @@ Variant VariantUtilityFunctions::sign(const Variant &x, Callable::CallError &r_e return VariantInternalAccessor::get(&x).sign(); } break; default: { - r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - return Variant(); - } + r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; + r_error.argument = 0; + r_error.expected = Variant::NIL; + return R"(Argument "x" must be "int", "float", "Vector2", "Vector2i", "Vector3", "Vector3i", "Vector4", or "Vector4i".)"; + } break; } } @@ -333,14 +370,40 @@ int VariantUtilityFunctions::step_decimals(float step) { } Variant VariantUtilityFunctions::snapped(const Variant &x, const Variant &step, Callable::CallError &r_error) { - r_error.error = Callable::CallError::CALL_OK; - if (x.get_type() != step.get_type() && !((x.get_type() == Variant::INT && step.get_type() == Variant::FLOAT) || (x.get_type() == Variant::FLOAT && step.get_type() == Variant::INT))) { - r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; - r_error.argument = 1; - r_error.expected = x.get_type(); - return Variant(); + switch (x.get_type()) { + case Variant::INT: + case Variant::FLOAT: + case Variant::VECTOR2: + case Variant::VECTOR2I: + case Variant::VECTOR3: + case Variant::VECTOR3I: + case Variant::VECTOR4: + case Variant::VECTOR4I: + break; + default: + r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; + r_error.argument = 0; + r_error.expected = Variant::NIL; + return R"(Argument "x" must be "int", "float", "Vector2", "Vector2i", "Vector3", "Vector3i", "Vector4", or "Vector4i".)"; + } + + if (x.get_type() != step.get_type()) { + if (x.get_type() == Variant::INT || x.get_type() == Variant::FLOAT) { + if (step.get_type() != Variant::INT && step.get_type() != Variant::FLOAT) { + r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; + r_error.argument = 1; + r_error.expected = Variant::NIL; + return R"(Argument "step" must be "int" or "float".)"; + } + } else { + r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; + r_error.argument = 1; + r_error.expected = x.get_type(); + return Variant(); + } } + r_error.error = Callable::CallError::CALL_OK; switch (step.get_type()) { case Variant::INT: { return snappedi(x, VariantInternalAccessor::get(&step)); @@ -367,9 +430,8 @@ Variant VariantUtilityFunctions::snapped(const Variant &x, const Variant &step, return VariantInternalAccessor::get(&x).snapped(VariantInternalAccessor::get(&step)); } break; default: { - r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - return Variant(); - } + return Variant(); // Already handled. + } break; } } @@ -382,7 +444,23 @@ int64_t VariantUtilityFunctions::snappedi(double x, int64_t step) { } Variant VariantUtilityFunctions::lerp(const Variant &from, const Variant &to, double weight, Callable::CallError &r_error) { - r_error.error = Callable::CallError::CALL_OK; + switch (from.get_type()) { + case Variant::INT: + case Variant::FLOAT: + case Variant::VECTOR2: + case Variant::VECTOR3: + case Variant::VECTOR4: + case Variant::QUATERNION: + case Variant::BASIS: + case Variant::COLOR: + break; + default: + r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; + r_error.argument = 0; + r_error.expected = Variant::NIL; + return R"(Argument "from" must be "int", "float", "Vector2", "Vector3", "Vector4", "Quaternion", "Basis, or "Color".)"; + } + if (from.get_type() != to.get_type()) { r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT; r_error.argument = 1; @@ -390,6 +468,7 @@ Variant VariantUtilityFunctions::lerp(const Variant &from, const Variant &to, do return Variant(); } + r_error.error = Callable::CallError::CALL_OK; switch (from.get_type()) { case Variant::INT: { return lerpf(VariantInternalAccessor::get(&from), to, weight); @@ -416,9 +495,8 @@ Variant VariantUtilityFunctions::lerp(const Variant &from, const Variant &to, do return VariantInternalAccessor::get(&from).lerp(VariantInternalAccessor::get(&to), weight); } break; default: { - r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - return Variant(); - } + return Variant(); // Already handled. + } break; } } diff --git a/doc/classes/@GlobalScope.xml b/doc/classes/@GlobalScope.xml index 35e1933473ac..b06c28311a5e 100644 --- a/doc/classes/@GlobalScope.xml +++ b/doc/classes/@GlobalScope.xml @@ -196,7 +196,7 @@ - Rounds [param x] upward (towards positive infinity), returning the smallest whole number that is not less than [param x]. Supported types: [int], [float], [Vector2], [Vector3], [Vector4]. + Rounds [param x] upward (towards positive infinity), returning the smallest whole number that is not less than [param x]. Supported types: [int], [float], [Vector2], [Vector2i], [Vector3], [Vector3i], [Vector4], [Vector4i]. [codeblock] var i = ceil(1.45) # i is 2.0 i = ceil(1.001) # i is 2.0 @@ -421,7 +421,7 @@ - Rounds [param x] downward (towards negative infinity), returning the largest whole number that is not more than [param x]. Supported types: [int], [float], [Vector2], [Vector3], [Vector4]. + Rounds [param x] downward (towards negative infinity), returning the largest whole number that is not more than [param x]. Supported types: [int], [float], [Vector2], [Vector2i], [Vector3], [Vector3i], [Vector4], [Vector4i]. [codeblock] var a = floor(2.99) # a is 2.0 a = floor(-2.99) # a is -3.0 @@ -1133,7 +1133,7 @@ - Rounds [param x] to the nearest whole number, with halfway cases rounded away from 0. Supported types: [int], [float], [Vector2], [Vector3], [Vector4]. + Rounds [param x] to the nearest whole number, with halfway cases rounded away from 0. Supported types: [int], [float], [Vector2], [Vector2i], [Vector3], [Vector3i], [Vector4], [Vector4i]. [codeblock] round(2.4) # Returns 2 round(2.5) # Returns 3 diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp index 831971c3f3d7..0a2dc0f87a34 100644 --- a/modules/gdscript/gdscript_analyzer.cpp +++ b/modules/gdscript/gdscript_analyzer.cpp @@ -3134,12 +3134,16 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a GDScriptUtilityFunctions::get_function(function_name)(&value, (const Variant **)args.ptr(), args.size(), err); switch (err.error) { - case Callable::CallError::CALL_ERROR_INVALID_ARGUMENT: { - PropertyInfo wrong_arg = function_info.arguments[err.argument]; - push_error(vformat(R"*(Invalid argument for "%s()" function: argument %d should be "%s" but is "%s".)*", function_name, err.argument + 1, - type_from_property(wrong_arg, true).to_string(), p_call->arguments[err.argument]->get_datatype().to_string()), - p_call->arguments[err.argument]); - } break; + case Callable::CallError::CALL_ERROR_INVALID_ARGUMENT: + if (value.get_type() == Variant::STRING && !value.operator String().is_empty()) { + push_error(vformat(R"*(Invalid argument for "%s()" function: %s)*", function_name, value), p_call->arguments[err.argument]); + } else { + // Do not use `type_from_property()` for expected type, since utility functions use their own checks. + push_error(vformat(R"*(Invalid argument for "%s()" function: argument %d should be "%s" but is "%s".)*", function_name, 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; case Callable::CallError::CALL_ERROR_INVALID_METHOD: push_error(vformat(R"(Invalid call for function "%s".)", function_name), p_call); break; @@ -3181,18 +3185,16 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a Variant::call_utility_function(function_name, &value, (const Variant **)args.ptr(), args.size(), err); switch (err.error) { - case Callable::CallError::CALL_ERROR_INVALID_ARGUMENT: { - String expected_type_name; - if (err.argument < function_info.arguments.size()) { - expected_type_name = type_from_property(function_info.arguments[err.argument], true).to_string(); + case Callable::CallError::CALL_ERROR_INVALID_ARGUMENT: + if (value.get_type() == Variant::STRING && !value.operator String().is_empty()) { + push_error(vformat(R"*(Invalid argument for "%s()" function: %s)*", function_name, value), p_call->arguments[err.argument]); } else { - expected_type_name = Variant::get_type_name((Variant::Type)err.expected); + // Do not use `type_from_property()` for expected type, since utility functions use their own checks. + push_error(vformat(R"*(Invalid argument for "%s()" function: argument %d should be "%s" but is "%s".)*", function_name, 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]); } - - push_error(vformat(R"*(Invalid argument for "%s()" function: argument %d should be "%s" but is "%s".)*", function_name, err.argument + 1, - expected_type_name, p_call->arguments[err.argument]->get_datatype().to_string()), - p_call->arguments[err.argument]); - } break; + break; case Callable::CallError::CALL_ERROR_INVALID_METHOD: push_error(vformat(R"(Invalid call for function "%s".)", function_name), p_call); break; diff --git a/modules/gdscript/gdscript_utility_functions.cpp b/modules/gdscript/gdscript_utility_functions.cpp index 69a0b42d89eb..40c564c36b76 100644 --- a/modules/gdscript/gdscript_utility_functions.cpp +++ b/modules/gdscript/gdscript_utility_functions.cpp @@ -97,6 +97,9 @@ struct GDScriptUtilityFunctionsDefinitions { } else { Variant::construct(Variant::Type(type), *r_ret, p_args, 1, r_error); + if (r_error.error != Callable::CallError::CALL_OK) { + *r_ret = vformat(RTR(R"(Cannot convert "%s" to "%s".)"), Variant::get_type_name(p_args[0]->get_type()), Variant::get_type_name(Variant::Type(type))); + } } } #endif // DISABLE_DEPRECATED @@ -130,8 +133,8 @@ struct GDScriptUtilityFunctionsDefinitions { } Error err = arr.resize(count); if (err != OK) { + *r_ret = RTR("Cannot resize array."); r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - *r_ret = Variant(); return; } @@ -155,8 +158,8 @@ struct GDScriptUtilityFunctionsDefinitions { } Error err = arr.resize(to - from); if (err != OK) { + *r_ret = RTR("Cannot resize array."); r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - *r_ret = Variant(); return; } for (int i = from; i < to; i++) { @@ -199,8 +202,8 @@ struct GDScriptUtilityFunctionsDefinitions { Error err = arr.resize(count); if (err != OK) { + *r_ret = RTR("Cannot resize array."); r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD; - *r_ret = Variant(); return; } @@ -370,7 +373,7 @@ struct GDScriptUtilityFunctionsDefinitions { *r_ret = gdscr->_new(nullptr, -1 /*skip initializer*/, r_error); if (r_error.error != Callable::CallError::CALL_OK) { - *r_ret = Variant(); + *r_ret = RTR("Cannot instantiate GDScript class."); return; } diff --git a/modules/gdscript/gdscript_vm.cpp b/modules/gdscript/gdscript_vm.cpp index 75dc2e4f8b7f..b723ecc18582 100644 --- a/modules/gdscript/gdscript_vm.cpp +++ b/modules/gdscript/gdscript_vm.cpp @@ -2067,11 +2067,11 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a #ifdef DEBUG_ENABLED if (err.error != Callable::CallError::CALL_OK) { String methodstr = function; - if (dst->get_type() == Variant::STRING) { + if (dst->get_type() == Variant::STRING && !dst->operator String().is_empty()) { // Call provided error string. - err_text = "Error calling utility function '" + methodstr + "': " + String(*dst); + err_text = vformat(R"*(Error calling utility function "%s()": %s)*", methodstr, *dst); } else { - err_text = _get_call_error(err, "utility function '" + methodstr + "'", (const Variant **)argptrs); + err_text = _get_call_error(err, vformat(R"*(utility function "%s()")*", methodstr), (const Variant **)argptrs); } OPCODE_BREAK; } @@ -2123,13 +2123,12 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a #ifdef DEBUG_ENABLED if (err.error != Callable::CallError::CALL_OK) { - // TODO: Add this information in debug. - String methodstr = ""; - if (dst->get_type() == Variant::STRING) { + String methodstr = gds_utilities_names[_code_ptr[ip + 2]]; + if (dst->get_type() == Variant::STRING && !dst->operator String().is_empty()) { // Call provided error string. - err_text = "Error calling GDScript utility function '" + methodstr + "': " + String(*dst); + err_text = vformat(R"*(Error calling GDScript utility function "%s()": %s)*", methodstr, *dst); } else { - err_text = _get_call_error(err, "GDScript utility function '" + methodstr + "'", (const Variant **)argptrs); + err_text = _get_call_error(err, vformat(R"*(GDScript utility function "%s()")*", methodstr), (const Variant **)argptrs); } OPCODE_BREAK; } diff --git a/modules/gdscript/tests/scripts/analyzer/errors/gd_utility_function_wrong_arg.gd b/modules/gdscript/tests/scripts/analyzer/errors/gd_utility_function_wrong_arg.gd new file mode 100644 index 000000000000..c06fbd89ff26 --- /dev/null +++ b/modules/gdscript/tests/scripts/analyzer/errors/gd_utility_function_wrong_arg.gd @@ -0,0 +1,2 @@ +func test(): + print(len(Color())) # GDScript utility function. diff --git a/modules/gdscript/tests/scripts/analyzer/errors/gd_utility_function_wrong_arg.out b/modules/gdscript/tests/scripts/analyzer/errors/gd_utility_function_wrong_arg.out new file mode 100644 index 000000000000..9cb04f62403c --- /dev/null +++ b/modules/gdscript/tests/scripts/analyzer/errors/gd_utility_function_wrong_arg.out @@ -0,0 +1,2 @@ +GDTEST_ANALYZER_ERROR +Invalid argument for "len()" function: Value of type 'Color' can't provide a length. diff --git a/modules/gdscript/tests/scripts/analyzer/errors/utility_function_wrong_arg.gd b/modules/gdscript/tests/scripts/analyzer/errors/utility_function_wrong_arg.gd new file mode 100644 index 000000000000..dc6e26e68218 --- /dev/null +++ b/modules/gdscript/tests/scripts/analyzer/errors/utility_function_wrong_arg.gd @@ -0,0 +1,2 @@ +func test(): + print(floor(Color())) # Built-in utility function. diff --git a/modules/gdscript/tests/scripts/analyzer/errors/utility_function_wrong_arg.out b/modules/gdscript/tests/scripts/analyzer/errors/utility_function_wrong_arg.out new file mode 100644 index 000000000000..27d2504dd06d --- /dev/null +++ b/modules/gdscript/tests/scripts/analyzer/errors/utility_function_wrong_arg.out @@ -0,0 +1,2 @@ +GDTEST_ANALYZER_ERROR +Invalid argument for "floor()" function: Argument "x" must be "int", "float", "Vector2", "Vector2i", "Vector3", "Vector3i", "Vector4", or "Vector4i". diff --git a/modules/gdscript/tests/scripts/runtime/errors/gd_utility_function_wrong_arg.gd b/modules/gdscript/tests/scripts/runtime/errors/gd_utility_function_wrong_arg.gd new file mode 100644 index 000000000000..340fc8c150bb --- /dev/null +++ b/modules/gdscript/tests/scripts/runtime/errors/gd_utility_function_wrong_arg.gd @@ -0,0 +1,3 @@ +func test(): + var x = Color() + print(len(x)) # GDScript utility function. diff --git a/modules/gdscript/tests/scripts/runtime/errors/gd_utility_function_wrong_arg.out b/modules/gdscript/tests/scripts/runtime/errors/gd_utility_function_wrong_arg.out new file mode 100644 index 000000000000..6d2938dcf3e0 --- /dev/null +++ b/modules/gdscript/tests/scripts/runtime/errors/gd_utility_function_wrong_arg.out @@ -0,0 +1,6 @@ +GDTEST_RUNTIME_ERROR +>> SCRIPT ERROR +>> on function: test() +>> runtime/errors/gd_utility_function_wrong_arg.gd +>> 3 +>> Error calling GDScript utility function "len()": Value of type 'Color' can't provide a length. diff --git a/modules/gdscript/tests/scripts/runtime/errors/utility_function_wrong_arg.gd b/modules/gdscript/tests/scripts/runtime/errors/utility_function_wrong_arg.gd new file mode 100644 index 000000000000..6568155bae9a --- /dev/null +++ b/modules/gdscript/tests/scripts/runtime/errors/utility_function_wrong_arg.gd @@ -0,0 +1,3 @@ +func test(): + var x = Color() + print(floor(x)) # Built-in utility function. diff --git a/modules/gdscript/tests/scripts/runtime/errors/utility_function_wrong_arg.out b/modules/gdscript/tests/scripts/runtime/errors/utility_function_wrong_arg.out new file mode 100644 index 000000000000..b311bfa38a8d --- /dev/null +++ b/modules/gdscript/tests/scripts/runtime/errors/utility_function_wrong_arg.out @@ -0,0 +1,6 @@ +GDTEST_RUNTIME_ERROR +>> SCRIPT ERROR +>> on function: test() +>> runtime/errors/utility_function_wrong_arg.gd +>> 3 +>> Error calling utility function "floor()": Argument "x" must be "int", "float", "Vector2", "Vector2i", "Vector3", "Vector3i", "Vector4", or "Vector4i".