From 5efbed51cce62cdd9a2927638030e76bf688cdf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Verschelde?= Date: Wed, 5 Oct 2022 20:49:35 +0200 Subject: [PATCH] GDScript: Improve error messages for invalid indexing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These errors are very common when using an invalid property name or calling on an object of the wrong type, and the previous message was a bit cryptic for users. Co-authored-by: Rémi Verschelde Co-authored-by: golfinq --- core/variant/variant.h | 17 +++++-- core/variant/variant_setget.cpp | 40 ++++++++++++++++- modules/gdscript/gdscript_analyzer.cpp | 2 +- modules/gdscript/gdscript_vm.cpp | 45 ++++++++++++------- .../analyzer/errors/outer_class_constants.out | 2 +- .../outer_class_constants_as_variant.out | 2 +- .../errors/outer_class_instance_constants.out | 2 +- ...er_class_instance_constants_as_variant.out | 2 +- .../runtime/errors/constant_array_is_deep.out | 2 +- .../errors/constant_dictionary_is_deep.out | 2 +- 10 files changed, 87 insertions(+), 29 deletions(-) diff --git a/core/variant/variant.h b/core/variant/variant.h index d698f857546a..190f4e130d0d 100644 --- a/core/variant/variant.h +++ b/core/variant/variant.h @@ -703,9 +703,20 @@ class Variant { bool has_key(const Variant &p_key, bool &r_valid) const; /* Generic */ - - void set(const Variant &p_index, const Variant &p_value, bool *r_valid = nullptr); - Variant get(const Variant &p_index, bool *r_valid = nullptr) const; + enum VariantSetError { + SET_OK, + SET_KEYED_ERR, + SET_NAMED_ERR, + SET_INDEXED_ERR + }; + enum VariantGetError { + GET_OK, + GET_KEYED_ERR, + GET_NAMED_ERR, + GET_INDEXED_ERR + }; + void set(const Variant &p_index, const Variant &p_value, bool *r_valid = nullptr, VariantSetError *err_code = nullptr); + Variant get(const Variant &p_index, bool *r_valid = nullptr, VariantGetError *err_code = nullptr) const; bool in(const Variant &p_index, bool *r_valid = nullptr) const; bool iter_init(Variant &r_iter, bool &r_valid) const; diff --git a/core/variant/variant_setget.cpp b/core/variant/variant_setget.cpp index 05f7abf32c99..40e0601b0f62 100644 --- a/core/variant/variant_setget.cpp +++ b/core/variant/variant_setget.cpp @@ -1166,30 +1166,48 @@ bool Variant::has_key(const Variant &p_key, bool &r_valid) const { } } -void Variant::set(const Variant &p_index, const Variant &p_value, bool *r_valid) { +void Variant::set(const Variant &p_index, const Variant &p_value, bool *r_valid, VariantSetError *err_code) { + if (err_code) { + *err_code = VariantSetError::SET_OK; + } if (type == DICTIONARY || type == OBJECT) { bool valid; set_keyed(p_index, p_value, valid); if (r_valid) { *r_valid = valid; + if (!valid && err_code) { + *err_code = VariantSetError::SET_KEYED_ERR; + } } } else { bool valid = false; if (p_index.get_type() == STRING_NAME) { set_named(*VariantGetInternalPtr::get_ptr(&p_index), p_value, valid); + if (!valid && err_code) { + *err_code = VariantSetError::SET_NAMED_ERR; + } } else if (p_index.get_type() == INT) { bool obb; set_indexed(*VariantGetInternalPtr::get_ptr(&p_index), p_value, valid, obb); if (obb) { valid = false; + if (err_code) { + *err_code = VariantSetError::SET_INDEXED_ERR; + } } } else if (p_index.get_type() == STRING) { // less efficient version of named set_named(*VariantGetInternalPtr::get_ptr(&p_index), p_value, valid); + if (!valid && err_code) { + *err_code = VariantSetError::SET_NAMED_ERR; + } } else if (p_index.get_type() == FLOAT) { // less efficient version of indexed bool obb; set_indexed(*VariantGetInternalPtr::get_ptr(&p_index), p_value, valid, obb); if (obb) { valid = false; + if (err_code) { + *err_code = VariantSetError::SET_INDEXED_ERR; + } } } if (r_valid) { @@ -1198,31 +1216,49 @@ void Variant::set(const Variant &p_index, const Variant &p_value, bool *r_valid) } } -Variant Variant::get(const Variant &p_index, bool *r_valid) const { +Variant Variant::get(const Variant &p_index, bool *r_valid, VariantGetError *err_code) const { + if (err_code) { + *err_code = VariantGetError::GET_OK; + } Variant ret; if (type == DICTIONARY || type == OBJECT) { bool valid; ret = get_keyed(p_index, valid); if (r_valid) { *r_valid = valid; + if (!valid && err_code) { + *err_code = VariantGetError::GET_KEYED_ERR; + } } } else { bool valid = false; if (p_index.get_type() == STRING_NAME) { ret = get_named(*VariantGetInternalPtr::get_ptr(&p_index), valid); + if (!valid && err_code) { + *err_code = VariantGetError::GET_NAMED_ERR; + } } else if (p_index.get_type() == INT) { bool obb; ret = get_indexed(*VariantGetInternalPtr::get_ptr(&p_index), valid, obb); if (obb) { valid = false; + if (err_code) { + *err_code = VariantGetError::GET_INDEXED_ERR; + } } } else if (p_index.get_type() == STRING) { // less efficient version of named ret = get_named(*VariantGetInternalPtr::get_ptr(&p_index), valid); + if (!valid && err_code) { + *err_code = VariantGetError::GET_NAMED_ERR; + } } else if (p_index.get_type() == FLOAT) { // less efficient version of indexed bool obb; ret = get_indexed(*VariantGetInternalPtr::get_ptr(&p_index), valid, obb); if (obb) { valid = false; + if (err_code) { + *err_code = VariantGetError::GET_INDEXED_ERR; + } } } if (r_valid) { diff --git a/modules/gdscript/gdscript_analyzer.cpp b/modules/gdscript/gdscript_analyzer.cpp index 882c246706dd..4c55573e5917 100644 --- a/modules/gdscript/gdscript_analyzer.cpp +++ b/modules/gdscript/gdscript_analyzer.cpp @@ -3595,7 +3595,7 @@ void GDScriptAnalyzer::reduce_identifier_from_base(GDScriptParser::IdentifierNod switch (base.builtin_type) { case Variant::NIL: { if (base.is_hard_type()) { - push_error(vformat(R"(Invalid get index "%s" on base Nil)", name), p_identifier); + push_error(vformat(R"(Cannot get property "%s" on a null object.)", name), p_identifier); } return; } diff --git a/modules/gdscript/gdscript_vm.cpp b/modules/gdscript/gdscript_vm.cpp index be18dee2cfea..c0edaf5f6516 100644 --- a/modules/gdscript/gdscript_vm.cpp +++ b/modules/gdscript/gdscript_vm.cpp @@ -888,8 +888,12 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a GET_VARIANT_PTR(value, 2); bool valid; +#ifdef DEBUG_ENABLED + Variant::VariantSetError err_code; + dst->set(*index, *value, &valid, &err_code); +#else dst->set(*index, *value, &valid); - +#endif #ifdef DEBUG_ENABLED if (!valid) { Object *obj = dst->get_validated_object(); @@ -906,7 +910,10 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a } else { v = "of type '" + _get_var_type(index) + "'"; } - err_text = "Invalid set index " + v + " (on base: '" + _get_var_type(dst) + "') with value of type '" + _get_var_type(value) + "'"; + err_text = "Invalid assignment of property or key " + v + " with value of type '" + _get_var_type(value) + "' on a base object of type '" + _get_var_type(dst) + "'."; + if (err_code == Variant::VariantSetError::SET_INDEXED_ERR) { + err_text = "Invalid assignment of index " + v + " (on base: '" + _get_var_type(dst) + "') with value of type '" + _get_var_type(value) + "'."; + } } OPCODE_BREAK; } @@ -937,7 +944,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a } else { v = "of type '" + _get_var_type(index) + "'"; } - err_text = "Invalid set index " + v + " (on base: '" + _get_var_type(dst) + "') with value of type '" + _get_var_type(value) + "'"; + err_text = "Invalid assignment of property or key " + v + " with value of type '" + _get_var_type(value) + "' on a base object of type '" + _get_var_type(dst) + "'."; OPCODE_BREAK; } #endif @@ -987,7 +994,8 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a bool valid; #ifdef DEBUG_ENABLED // Allow better error message in cases where src and dst are the same stack position. - Variant ret = src->get(*index, &valid); + Variant::VariantGetError err_code; + Variant ret = src->get(*index, &valid, &err_code); #else *dst = src->get(*index, &valid); @@ -1000,7 +1008,10 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a } else { v = "of type '" + _get_var_type(index) + "'"; } - err_text = "Invalid get index " + v + " (on base: '" + _get_var_type(src) + "')."; + err_text = "Invalid access to property or key " + v + " on a base object of type '" + _get_var_type(src) + "'."; + if (err_code == Variant::VariantGetError::GET_INDEXED_ERR) { + err_text = "Invalid access of index " + v + " on a base object of type: '" + _get_var_type(src) + "'."; + } OPCODE_BREAK; } *dst = ret; @@ -1036,7 +1047,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a } else { v = "of type '" + _get_var_type(key) + "'"; } - err_text = "Invalid get index " + v + " (on base: '" + _get_var_type(src) + "')."; + err_text = "Invalid access to property or key " + v + " on a base object of type '" + _get_var_type(src) + "'."; OPCODE_BREAK; } *dst = ret; @@ -1101,7 +1112,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a if (read_only_property) { err_text = vformat(R"(Cannot set value into property "%s" (on base "%s") because it is read-only.)", String(*index), _get_var_type(dst)); } else { - err_text = "Invalid set index '" + String(*index) + "' (on base: '" + _get_var_type(dst) + "') with value of type '" + _get_var_type(value) + "'."; + err_text = "Invalid assignment of property or key '" + String(*index) + "' with value of type '" + _get_var_type(value) + "' on a base object of type '" + _get_var_type(dst) + "'."; } OPCODE_BREAK; } @@ -1146,7 +1157,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a #endif #ifdef DEBUG_ENABLED if (!valid) { - err_text = "Invalid get index '" + index->operator String() + "' (on base: '" + _get_var_type(src) + "')."; + err_text = "Invalid access to property or key '" + index->operator String() + "' on a base object of type '" + _get_var_type(src) + "'."; OPCODE_BREAK; } *dst = ret; @@ -2619,7 +2630,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a Variant::construct(ret_type, retvalue, const_cast(&r), 1, ce); } else { #ifdef DEBUG_ENABLED - err_text = vformat(R"(Trying to return value of type "%s" from a function which the return type is "%s".)", + err_text = vformat(R"(Trying to return value of type "%s" from a function whose return type is "%s".)", Variant::get_type_name(r->get_type()), Variant::get_type_name(ret_type)); #endif // DEBUG_ENABLED @@ -2649,9 +2660,9 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a if (r->get_type() != Variant::ARRAY) { #ifdef DEBUG_ENABLED - err_text = vformat(R"(Trying to return a value of type "%s" where expected return type is "Array[%s]".)", - _get_var_type(r), _get_element_type(builtin_type, native_type, *script_type)); -#endif // DEBUG_ENABLED + err_text = vformat(R"(Trying to return value of type "%s" from a function whose return type is "Array[%s]".)", + Variant::get_type_name(r->get_type()), Variant::get_type_name(builtin_type)); +#endif OPCODE_BREAK; } @@ -2682,7 +2693,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a GD_ERR_BREAK(!nc); if (r->get_type() != Variant::OBJECT && r->get_type() != Variant::NIL) { - err_text = vformat(R"(Trying to return value of type "%s" from a function which the return type is "%s".)", + err_text = vformat(R"(Trying to return value of type "%s" from a function whose return type is "%s".)", Variant::get_type_name(r->get_type()), nc->get_name()); OPCODE_BREAK; } @@ -2700,7 +2711,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a #endif // DEBUG_ENABLED if (ret_obj && !ClassDB::is_parent_class(ret_obj->get_class_name(), nc->get_name())) { #ifdef DEBUG_ENABLED - err_text = vformat(R"(Trying to return value of type "%s" from a function which the return type is "%s".)", + err_text = vformat(R"(Trying to return value of type "%s" from a function whose return type is "%s".)", ret_obj->get_class_name(), nc->get_name()); #endif // DEBUG_ENABLED OPCODE_BREAK; @@ -2723,7 +2734,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a if (r->get_type() != Variant::OBJECT && r->get_type() != Variant::NIL) { #ifdef DEBUG_ENABLED - err_text = vformat(R"(Trying to return value of type "%s" from a function which the return type is "%s".)", + err_text = vformat(R"(Trying to return value of type "%s" from a function whose return type is "%s".)", Variant::get_type_name(r->get_type()), GDScript::debug_get_script_name(Ref