Implement typed dictionaries

[Repiteo]

This aims to add the ability to bind typed dictionaries to script & GDScript. The implementation takes heavy inspiration from the existing typed array format. The primary difference is the ability to specify type for just a key, leaving the value as typeless much like current dictionaries. Both key and value need their types specified for typed dictionaries, with Variant allowing for a typeless equivalent.

Syntax

var typed_key_value: Dictionary[int, String] = { 1: "first value", 2: "second value", 3: "etc" }
var typed_key: Dictionary[int, Variant] = { 0: "any value", 10: 3.14, 100: null }
var typed_value: Dictionary[Variant, int] = { "any value": 0, 123: 456, null: -1 }

Editor

Issues

All core issues are resolved!
There are a fair number of areas that still require fixes, in addition to whatever lingering bugs/errors there are. In particular:

• No way currently to make a typeless key and a typed value
• Variant text appears as white instead of green in GDSCript if used as the value:
  
• Editor fails to update creation key/value sections if changing type
• Editor incorrectly perceives the initial typed key/value as null
• Editor fails to setup exported dictionaries during runtime
• Editor won't recognize a typed key passed with the incorrect type as invalid in a GDScript function (that is, when using the [] operator); it throws an error if attempting to do so in runtime & does recognize invalid keys when creating a dictionary initially
• Editor will treat any Object variant as a "Resource" regardless of provided type
• Dictionary fails to recognize local script types in curly-bracket initialization during runtime
• Potential memory leak with container_element_type being a list now instead of a single item. (This is inexperience with the engine on my end, dunno if it needs something special to memdelete array entries)
• Implementation of GDScript tests

Closes godotengine/godot-proposals#56

[lufog]

I would prefer:

var my_dictionary: Dictionary[int, Variant]
var my_dictionary: Dictionary[Variant, int]

Just Dictionary[int] a little confusing.

[Repiteo]

@lufog Great point! I adjusted the code to handle exactly that, but it revealed some other pitfalls in the process (mainly the process of transfering container types from one DataType to another) so I'll have to iron that out before showing off a new version

In the meantime I've added documentation descriptions & made it possible to use a constructor to make a typed dictionary in GDScript, much like typed arrays

[Repiteo]

GDScript tests implemented! They're largely lifted from the tests given to typed Arrays so more specialization will be required, but they're a solid starting point to see what's awry off the bat

Namely, typed arrays seem to be instantiating with null keys & values, though they're recognized as typed. I'm not quite sure yet where the incorrect implementation is happening, but that's my current focus as it very well may be the linchpin of the editor errors

[Bromeon]

With typed arrays, we have the variance problem:

var typed: Array[int] = [1, 2, 3, 4]
var as_untyped: Array = typed

as_untyped[0] = "string" # silently fails at runtime

I guess the same problem exists here, but for both keys and values?
How do we deal with that?

[dalexeev]

With typed arrays, we have the variance problem

The exception for Array (aka Array[Variant]) was intentionally added as a convenience to users of untyped GDScript.

var a: Array[Node2D]
var b: Array[Node] = a # Error
var c: Array[Variant] = a # Unsafe, but ok.
var d: Array = a # Unsafe, but ok.

[Bromeon]

The exception for Array (aka Array[Variant]) was intentionally added as a convenience to users of untyped GDScript.

Yes, but it's still a hole in the type system. I understand the desire to pass typed arrays to APIs accepting untyped ones, and for reading, this is completely sound. The covariance violation occurs when writing to an Array that points to a Array[T]. This cannot be checked at parse time, it's a runtime check (and IIRC it's silent do-nothing, no error at the moment).

My question is, is the same covariance conversion ("upcast") planned for dictionaries on each of the generic parameters? That is:

• Dictionary[int, String] -> Dictionary[int, Variant]
• Dictionary[int, String] -> Dictionary[Variant, String]
• Dictionary[int, Variant] -> Dictionary[Variant, Variant] == Dictionary

And if yes, are we fully aware of the pitfalls?

[dalexeev]

This cannot be checked at parse time, it's a runtime check (and IIRC it's silent do-nothing, no error at the moment).

However, this is not directly related to the PR. PRs of this kind tend to garner large numbers of comments that are difficult to navigate. If you'd like to discuss this, I suggest the #gdscript channel on Rocket Chat.

[Bromeon]

Thanks! My point was not to discuss typed arrays, but the covariance problem regarding dictionaries, and what conversions we allow (see above). As such it's quite on-topic I believe.

[Repiteo]

The holes in the type system certainly have been a bit of a headache to navigate, but attempting to overhaul that system as a whole is probably a bit beyond this PR (at least as I initially envisioned it). This is more about setting up the system that currently exists to dictionaries, warts and all. If the current implementation were to somehow inhibit the ability to implement typed dictionaries in the first place, then changes to the system would make sense

On that note, I've figured out why variants were appearing as null & not changing when swapping types, so both those fixes are in place. As it stands, the two biggest remaining issues are:

• The editor treats any Object variant as "Resource", regardless of the provided type (need to figure out how typed arrays handle this)
• A dictionary fails to properly parse provided key/value if there's unorthodox syntax (eg: typed_dictionary_assign_wrong_to_typed.out has the wrong error, thinks an entirely different type is passed on as the value (from reading the key?))

[dalexeev]

Overall it looks good to me, the implementation is consistent with typed arrays. We debated for a long time whether we should resolve the typed array issues first, but then we came to the conclusion that waiting would take too long. So if we have to break typed collection compatibility in the future, it will be both arrays and dictionaries.

In my opinion, this PR is already in a very good state to be merged. However, it's worth waiting for approval from @vnen as the main GDScript maintainer.

Of course, there are some things we might have missed or overlooked, but I think we could address them in follow-up PRs.

1. Attribute access does not have static checks, unlike index access. See the review comments.
2. Add support for binary serialization of typed dictionaries, like in Core: Add typed array support for binary serialization #78219. Since this is a sensitive part, it's good to do in a separate PR.
3. PROPERTY_HINT_DICTIONARY_TYPE vs PROPERTY_HINT_TYPE_STRING. These hints have different formats and are sometimes mixed up in the engine.
4. The inspector part can be improved in the future. Currently, keys are displayed as string representations, which is not very convenient for viewing and editing.
5. Perhaps it makes sense to adjust the documentation, GDScript tests, disassembler, etc.

Thanks for your great work, @Repiteo!

---

Patch

diff --git a/modules/gdscript/tests/scripts/utils.notest.gd b/modules/gdscript/tests/scripts/utils.notest.gd
index 5d615d8557..1e2788f765 100644
--- a/modules/gdscript/tests/scripts/utils.notest.gd
+++ b/modules/gdscript/tests/scripts/utils.notest.gd
@@ -24,6 +24,11 @@ static func get_type(property: Dictionary, is_return: bool = false) -> String:
 				if str(property.hint_string).is_empty():
 					return "Array[<unknown type>]"
 				return "Array[%s]" % property.hint_string
+		TYPE_DICTIONARY:
+			if property.hint == PROPERTY_HINT_DICTIONARY_TYPE:
+				if str(property.hint_string).is_empty():
+					return "Dictionary[<unknown type>, <unknown type>]"
+				return "Dictionary[%s]" % str(property.hint_string).replace(";", ", ")
 		TYPE_OBJECT:
 			if not str(property.class_name).is_empty():
 				return property.class_name
@@ -188,6 +193,8 @@ static func get_property_hint_name(hint: PropertyHint) -> String:
 			return "PROPERTY_HINT_INT_IS_POINTER"
 		PROPERTY_HINT_ARRAY_TYPE:
 			return "PROPERTY_HINT_ARRAY_TYPE"
+		PROPERTY_HINT_DICTIONARY_TYPE:
+			return "PROPERTY_HINT_DICTIONARY_TYPE"
 		PROPERTY_HINT_LOCALE_ID:
 			return "PROPERTY_HINT_LOCALE_ID"
 		PROPERTY_HINT_LOCALIZABLE_STRING:

[dalexeev]

Note that the is_array is used to apply export annotations to array elements, see #82952 for details. Not sure if a similar feature makes sense for dictionaries, perhaps your code needs it to simplify handling of key and value types.

[Repiteo]

I can't speak for the other types of export annotations, but this is still needed for the basic @export scenario; otherwise, all exported dictionaries would show up as untyped in the inspector

[dalexeev]

This code may be in the wrong place. if (is_array) is one level higher, after if (use_default_variable_type_check).

[Repiteo]

I thiiink this is fine. This bit was tricky because I had to parse key & value independently, so there was a fair bit of repeat code as a result. It does the same logic as the outer if (is_array) check, albeit resolved earlier.

[vnen]

I think this is fine. But eventually we should extract the logic to avoid the repetition and make sure it's consistent.

[AThousandShips]

Serialisation now makes {}} in some cases

[AThousandShips]

This shows we need some unit tests for this as the unit tests completely missed this, only the doc generator showed it which is an issue

[vnen]

Reviewed mostly the GDScript part and have some small notes but nothing blocking. Thanks a lot for working on this and sorry again for taking so long to review.

[ajreckof]

I checked the code modifications on EditorPropertyDictionary changes and tested it. This looks good.

[akien-mga]

Thanks! Amazing work 🎉 🥇

This is one of the most awaited features for GDScript users, and thus a major highlight for 4.4!