From e2a9e04fba2ff3594c16e5f1251a609545f3ef06 Mon Sep 17 00:00:00 2001 From: Thaddeus Crews Date: Tue, 10 Sep 2024 08:56:53 -0500 Subject: [PATCH] Core: Fix `operator[]` for typed dictionaries --- core/variant/dictionary.cpp | 44 +++++++++++++++---- core/variant/dictionary.h | 1 + core/variant/variant_setget.cpp | 35 +++------------ ...dictionary_assign_differently_typed_key.gd | 7 +++ ...ictionary_assign_differently_typed_key.out | 5 +++ ...ctionary_assign_differently_typed_value.gd | 13 ++++++ ...tionary_assign_differently_typed_value.out | 4 ++ 7 files changed, 72 insertions(+), 37 deletions(-) create mode 100644 modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_key.gd create mode 100644 modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_key.out create mode 100644 modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_value.gd create mode 100644 modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_value.out diff --git a/core/variant/dictionary.cpp b/core/variant/dictionary.cpp index f2522a4545cd..f4541ad5e9b9 100644 --- a/core/variant/dictionary.cpp +++ b/core/variant/dictionary.cpp @@ -84,22 +84,40 @@ Variant Dictionary::get_value_at_index(int p_index) const { } Variant &Dictionary::operator[](const Variant &p_key) { - if (unlikely(_p->read_only)) { - if (likely(_p->variant_map.has(p_key))) { - *_p->read_only = _p->variant_map[p_key]; + Variant key = p_key; + if (unlikely(!_p->typed_key.validate(key))) { + if (unlikely(!_p->typed_fallback)) { + _p->typed_fallback = memnew(Variant); + } + VariantInternal::initialize(_p->typed_fallback, _p->typed_value.type); + return *_p->typed_fallback; + } else if (unlikely(_p->read_only)) { + if (likely(_p->variant_map.has(key))) { + *_p->read_only = _p->variant_map[key]; } else { - *_p->read_only = Variant(); + VariantInternal::initialize(_p->read_only, _p->typed_value.type); } - return *_p->read_only; } else { - return _p->variant_map[p_key]; + if (unlikely(!_p->variant_map.has(key))) { + VariantInternal::initialize(&_p->variant_map[key], _p->typed_value.type); + } + return _p->variant_map[key]; } } const Variant &Dictionary::operator[](const Variant &p_key) const { - // Will not insert key, so no conversion is necessary. - return _p->variant_map[p_key]; + Variant key = p_key; + if (unlikely(!_p->typed_key.validate(key))) { + if (unlikely(!_p->typed_fallback)) { + _p->typed_fallback = memnew(Variant); + } + VariantInternal::initialize(_p->typed_fallback, _p->typed_value.type); + return *_p->typed_fallback; + } else { + // Will not insert key, so no initialization is necessary. + return _p->variant_map[key]; + } } const Variant *Dictionary::getptr(const Variant &p_key) const { @@ -158,6 +176,16 @@ Variant Dictionary::get_or_add(const Variant &p_key, const Variant &p_default) { return *result; } +bool Dictionary::set(const Variant &p_key, const Variant &p_value) { + ERR_FAIL_COND_V_MSG(_p->read_only, false, "Dictionary is in read-only state."); + Variant key = p_key; + ERR_FAIL_COND_V(!_p->typed_key.validate(key, "set"), false); + Variant value = p_value; + ERR_FAIL_COND_V(!_p->typed_value.validate(value, "set"), false); + _p->variant_map[key] = value; + return true; +} + int Dictionary::size() const { return _p->variant_map.size(); } diff --git a/core/variant/dictionary.h b/core/variant/dictionary.h index 57fbefc8f251..5f3ce40219bd 100644 --- a/core/variant/dictionary.h +++ b/core/variant/dictionary.h @@ -59,6 +59,7 @@ class Dictionary { Variant get_valid(const Variant &p_key) const; Variant get(const Variant &p_key, const Variant &p_default) const; Variant get_or_add(const Variant &p_key, const Variant &p_default); + bool set(const Variant &p_key, const Variant &p_value); int size() const; bool is_empty() const; diff --git a/core/variant/variant_setget.cpp b/core/variant/variant_setget.cpp index b60ff83cf198..cc192074ec8d 100644 --- a/core/variant/variant_setget.cpp +++ b/core/variant/variant_setget.cpp @@ -252,20 +252,7 @@ void Variant::set_named(const StringName &p_member, const Variant &p_value, bool } } else if (type == Variant::DICTIONARY) { Dictionary &dict = *VariantGetInternalPtr::get_ptr(this); - - if (dict.is_read_only()) { - r_valid = false; - return; - } - - Variant *v = dict.getptr(p_member); - if (v) { - *v = p_value; - } else { - dict[p_member] = p_value; - } - - r_valid = true; + r_valid = dict.set(p_member, p_value); } else { r_valid = false; } @@ -721,26 +708,16 @@ struct VariantIndexedSetGet_Dictionary { PtrToArg::encode(*ptr, member); } static void set(Variant *base, int64_t index, const Variant *value, bool *valid, bool *oob) { - if (VariantGetInternalPtr::get_ptr(base)->is_read_only()) { - *valid = false; - *oob = true; - return; - } - (*VariantGetInternalPtr::get_ptr(base))[index] = *value; - *oob = false; - *valid = true; + *valid = VariantGetInternalPtr::get_ptr(base)->set(index, *value); + *oob = VariantGetInternalPtr::get_ptr(base)->is_read_only(); } static void validated_set(Variant *base, int64_t index, const Variant *value, bool *oob) { - if (VariantGetInternalPtr::get_ptr(base)->is_read_only()) { - *oob = true; - return; - } - (*VariantGetInternalPtr::get_ptr(base))[index] = *value; - *oob = false; + VariantGetInternalPtr::get_ptr(base)->set(index, *value); + *oob = VariantGetInternalPtr::get_ptr(base)->is_read_only(); } static void ptr_set(void *base, int64_t index, const void *member) { Dictionary &v = *reinterpret_cast(base); - v[index] = PtrToArg::convert(member); + v.set(index, PtrToArg::convert(member)); } static Variant::Type get_index_type() { return Variant::NIL; } static uint32_t get_index_usage() { return PROPERTY_USAGE_DEFAULT; } diff --git a/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_key.gd b/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_key.gd new file mode 100644 index 000000000000..2f0b3bd0ebfa --- /dev/null +++ b/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_key.gd @@ -0,0 +1,7 @@ +func get_key() -> Variant: + return "key" + +func test(): + var typed: Dictionary[int, int] + typed[get_key()] = 0 + print('not ok') diff --git a/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_key.out b/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_key.out new file mode 100644 index 000000000000..44f5957556f5 --- /dev/null +++ b/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_key.out @@ -0,0 +1,5 @@ +GDTEST_RUNTIME_ERROR +>> ERROR +>> Method/function failed. Returning: false +>> Attempted to use a variable of type 'String' into a TypedDictionary.Key of type 'int'. +not ok diff --git a/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_value.gd b/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_value.gd new file mode 100644 index 000000000000..53eff28c66bd --- /dev/null +++ b/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_value.gd @@ -0,0 +1,13 @@ +func get_value() -> Variant: + return "value" + +func test(): + var typed: Dictionary[int, int] + + # TODO: This SHOULD fail, but the assignment operator detaches the value. + #typed[0] = get_value() + + # Will fail. + typed.get_or_add(0, get_value()) + + print("not ok") diff --git a/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_value.out b/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_value.out new file mode 100644 index 000000000000..4bbfe1810bdd --- /dev/null +++ b/modules/gdscript/tests/scripts/runtime/errors/typed_dictionary_assign_differently_typed_value.out @@ -0,0 +1,4 @@ +GDTEST_RUNTIME_ERROR +>> ERROR +>> Condition "!_p->typed_value.validate(value, "add")" is true. Returning: value +not ok