Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GDScript: Improve error messages for invalid indexing #82639

Merged
merged 1 commit into from
Jan 4, 2024
Merged

GDScript: Improve error messages for invalid indexing #82639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions core/variant/variant.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
40 changes: 38 additions & 2 deletions core/variant/variant_setget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<StringName>::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<int64_t>::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<String>::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<double>::get_ptr(&p_index), p_value, valid, obb);
if (obb) {
valid = false;
if (err_code) {
*err_code = VariantSetError::SET_INDEXED_ERR;
}
}
}
if (r_valid) {
Expand All @@ -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<StringName>::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<int64_t>::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<String>::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<double>::get_ptr(&p_index), valid, obb);
if (obb) {
valid = false;
if (err_code) {
*err_code = VariantGetError::GET_INDEXED_ERR;
}
}
}
if (r_valid) {
Expand Down
2 changes: 1 addition & 1 deletion modules/gdscript/gdscript_analyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
45 changes: 28 additions & 17 deletions modules/gdscript/gdscript_vm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -2619,7 +2630,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
Variant::construct(ret_type, retvalue, const_cast<const Variant **>(&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

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
Expand All @@ -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<Script>(base_type)));
#endif // DEBUG_ENABLED
OPCODE_BREAK;
Expand All @@ -2745,7 +2756,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
ScriptInstance *ret_inst = ret_obj->get_script_instance();
if (!ret_inst) {
#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(), GDScript::debug_get_script_name(Ref<GDScript>(base_type)));
#endif // DEBUG_ENABLED
OPCODE_BREAK;
Expand All @@ -2764,7 +2775,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a

if (!valid) {
#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".)",
GDScript::debug_get_script_name(ret_obj->get_script_instance()->get_script()), GDScript::debug_get_script_name(Ref<GDScript>(base_type)));
#endif // DEBUG_ENABLED
OPCODE_BREAK;
Expand Down
2 changes: 1 addition & 1 deletion modules/gdscript/tests/scripts/analyzer/errors/outer_class_constants.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ GDTEST_RUNTIME_ERROR
>> on function: test()
>> analyzer/errors/outer_class_constants.gd
>> 8
>> Invalid get index 'OUTER_CONST' (on base: 'GDScript').
>> Invalid access to property or key 'OUTER_CONST' on a base object of type 'GDScript'.
2 changes: 1 addition & 1 deletion modules/gdscript/tests/scripts/analyzer/errors/outer_class_constants_as_variant.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ GDTEST_RUNTIME_ERROR
>> on function: test()
>> analyzer/errors/outer_class_constants_as_variant.gd
>> 9
>> Invalid get index 'OUTER_CONST' (on base: 'GDScript').
>> Invalid access to property or key 'OUTER_CONST' on a base object of type 'GDScript'.
2 changes: 1 addition & 1 deletion modules/gdscript/tests/scripts/analyzer/errors/outer_class_instance_constants.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ GDTEST_RUNTIME_ERROR
>> on function: test()
>> analyzer/errors/outer_class_instance_constants.gd
>> 8
>> Invalid get index 'OUTER_CONST' (on base: 'RefCounted (Inner)').
>> Invalid access to property or key 'OUTER_CONST' on a base object of type 'RefCounted (Inner)'.
2 changes: 1 addition & 1 deletion modules/gdscript/tests/scripts/analyzer/errors/outer_class_instance_constants_as_variant.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ GDTEST_RUNTIME_ERROR
>> on function: test()
>> analyzer/errors/outer_class_instance_constants_as_variant.gd
>> 9
>> Invalid get index 'OUTER_CONST' (on base: 'RefCounted (Inner)').
>> Invalid access to property or key 'OUTER_CONST' on a base object of type 'RefCounted (Inner)'.
2 changes: 1 addition & 1 deletion modules/gdscript/tests/scripts/runtime/errors/constant_array_is_deep.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ GDTEST_RUNTIME_ERROR
>> on function: test()
>> runtime/errors/constant_array_is_deep.gd
>> 6
>> Invalid set index '0' (on base: 'Dictionary') with value of type 'int'
>> Invalid assignment of property or key '0' with value of type 'int' on a base object of type 'Dictionary'.
2 changes: 1 addition & 1 deletion modules/gdscript/tests/scripts/runtime/errors/constant_dictionary_is_deep.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ GDTEST_RUNTIME_ERROR
>> on function: test()
>> runtime/errors/constant_dictionary_is_deep.gd
>> 6
>> Invalid set index '0' (on base: 'Array') with value of type 'int'
>> Invalid assignment of index '0' (on base: 'Array') with value of type 'int'.
Loading