utopia-rise / godot-kotlin-native

Kotlin bindings for Godot Engine
MIT License
290 stars 16 forks source link

What to do with Dictionary #200

Open raniejade opened 4 years ago

raniejade commented 4 years ago

I'm not a fan of the current Dictionary implementation mainly because of:

(1) It's not idiomatic, in Kotlin parlance Dictionary is essentially a MutableMap<Any, Any>. (2) Written code is long as it enumerate all possible combinations of valid key/value types.

Although, it has some clear advantage: It's impossible to pass in invalid types as keys and values.

Criteria

The next section list down several Dictionary implementations, sighting how it compares to a kotlin.MutableMap - specifically in the following key areas:

  1. Setting a value
  2. Getting a value
  3. Iteration.

Additionally, it will also compare the implementation to how a Dictionary is used in GDScript.

Implementations

[1] Using Variant and specialized methods (current)

class Dictionary : Iterable<Map.Entry<Variant, Variant>> {
    fun set(key: Long, value: Long)
    fun set(key: Long, value: Double)
    // and so on
    fun set(key: String, value: Long)
    fun set(key: String, value: Double)
    // and so on

    fun get(key: Long): Variant
    fun get(key: String): Variant
    // and so on
}

The idea behind this implementation is to prevent the user from passing on a wrong value type to the Dictionary by providing specialized methods that only accept the correct value types. wrong value types are either types that can't be converted to a Variant or types that when passed to a Variant losses it's type identity - an example of this are the Int and Float types as Variant only work with the mentioned type's wider cousins: Long and Double, respectively. When creating a Variant from an Int, the value will be converted to a Long first before storing it - so when unwrapping the value from the Variant it's impossible to know if the initial value passed to it was an Int or a Long.

Setting a value

dic[1L] = 2L
dic["hello"] = 0.5
dic[1] = "hi" // does not compile, key here is an Int
dic["foo"] = 0.1f // does not compile, value here is a Float
dic[SomeKotlinClass()] = 2L // does not compile as SomeKotlinClass is not Variant compatible

In this area, setting a value is similar with kotlin.MutableMap.

Getting a value

dic["hello"].asString() // returns a variant, use asXXX to convert it to the expected type
dic.get("hello", 1L) // returns a Long value with a default of 1L if not present

In a way the second line is similar to how a kotlin.MutableMap works, however in the first line you to deal with a Variant first then convert it to the expected type.

Iteration

dic.forEach { (keyAsVariant, valueAsVariant) ->
    // do something here
}

for ((keyAsVariant, valueAsVariant) in dic) {
    // do something here
}

It is almost similar to kotlin.MutableMap however the biggest downside is you have to deal with Variants.

GDScript comparison

dic[1] = 2
dic["hello"] = 0.5
dic[1] = "hi"
dic["foo"] = 0.1

print(dic["Hello"]) // no explicit Variant conversion.

Everything is a Variant in GDScript, but the type itself is not exposed to users - but they are automatically converted. The problems with Int and Float is not present here because GDScript is using Long and Double for natural and real numbers, respectively. If you think about it a Variant in Kotlin parlance is Any, while in C# it is an object.

[2] Dictionary extending AbstractMutableMap<Any, Any>

class Dictionary : AbstractMutableMap<Any, Any>() {
    // ...
}

This is inspired by Godot C#, essentially Dictionary is a IMap<object, object>. The idea behind this implementation is to make usage of Dictionary idiomatic in Kotlin but retains its behaviour in GDScript (i.e, You can assign any key and value - as long as they are Variant compatible). A big bonus we get here is that we can use all the API defined for kotlin.MutableMap.

Setting a value

dic[1L] = 2L
dic["hello"] = 0.5
dic[1] = "hi" // compiles but the type of they key when queried will be a Long (1)
dic["foo"] = 0.1f // compiles but the type of the value when queried will be a Double (2)
dic[SomeKotlinClass()] = 2L // compiles but fails at runtime because SomeKotlinClass is not Variant compatible (3)

In a way it's similar to the first implementation except for the last three lines: (1) and (2) will lose the type information of it's key and value, respectively - but alternatively we can change the behaviour to throw an exception at runtime in a fail-fast manner. Failing fast here means the user can pick up the mistake during development instead of discovering it after the code has been shipped.

Getting a value

dic["hello"] as String // explicit casting to a String
dic.getOrDefault("Hello", 0) // returns an Int, via implicit casting.

Since values are Any, we have to cast to the appropriate type. This is similar to the first implementation, but doesn't use a Variant which in my honest opinion - shouldn't be used by users (It's not exposed in GDScript and C# - why should we? In C# it is only used to namespace: Variant.Type and Variant.OP: https://godotsharp.net/api/3.1.0/Godot.Variant/)

Iteration

dic.forEach { (keyAsAny, valueAsAny) ->
    // do something here
}

for ((keyAsAny, valueAsAny) in dic) {
    // do something here
}

Again, similar to the first implementation but not using Variant.

GDScript Comparison

This has a lot of similarity with GDScript, specifically setting a value. The difference lies in retrieving the value, you have to cast to the expected type in Kotlin due to it being statically typed. Ignoring the difference in typing nature of Kotlin and GDScript, this implementation is equal to the GDScript one.

[3] Typed Dictionary

class Dictionary<K: Any, V: Any> : AbstractMutableMap<K, V>() {
    // ...
}

This approach is a specialization of the second implementation where we allow users to specify more information about the types of its keys and values. This is the most idiomatic compared to the two previous implementation, essentially what you have here is a kotlin.MutableMap with a different name. One of the downside of this approach is that it's impossible to prevent users from using types that are not Variant compatible (i.e.Dictionary<Throwable, SomeKotlinClass>).

Setting a value

val dic: Dictionary<String, Int> = ...
dic["Hello"] = 12
dic["Foo"] = true // does not compile, dic can only store Ints values
dic[1] = 12 // does not compile, dic can only use String keys

val dic: Dictionary<Any, Any> = ...
dic["Foo"] = true // works!
dic[1] = 12 // works!

This approach allows you to restrict the type you can pass as its keys and values. If you want a Dictionary which behaves similarly in GDScript, then you can declare it as Dictionary<Any, Any>.

Getting a value

val dic: Dictionary<String, Int> = ...
var a = dic["Hello"] + 1 // works!

This is the area where this approach shines, you don't have to do any explicit casting to the expected type unless you declared it as Dictionary<Any, Any>.

Iteration

val dic: Dictionary<String, Int> = ...
dic.forEach { (key, value) ->
    // do something here
}

for ((key, value) in dic) {
    // do something here
}

As mentioned in the previous section, this approach shines in reads - no more explicit casting to the expected type.

GDScript Comparison

This is very different to GDScript but it is more idiomatic in Kotlin. It is hard to compare how reads are done in both languages due to the difference in typing.

raniejade commented 4 years ago

I personally prefer [2], [3] is nice and is very Kotlin-esque but it hides the original behaviour of a GDScript Dictionary (which is essentialy a MutableMap<Any, Any>). I guess the same pattern can be applied with VariantArray.

CedNaru commented 4 years ago

My 4th implementation with a dirty trick but that will allow us to freely use Int/Float (not like implem 2/3)

open class <K: Any, V: Any> Dictionary internal constructor(keyClass: Class<K>, valueClass<V>) : AbstractMutableMap<K, V>() {}

//"Typed Dictionary", works the same as implementation 3 but can accept Int/Float
fun <reified K: Any, refied V: Any> Dictionary() : Dictionary<K, V> {
      isValidGodotType<K>() // will throw an error if invalid type
      isValidGodotType<V>()  // will throw an error if invalid type
      return Dictionary(K.class, V.class)

With that method, we also keep the original type of the variable. We don't even need to query the type of Variant with a C call to Godot and can properly cast the Double/Long in the underlying Variant to their original Float/Int types. Other than that, it's pretty much the same as [2][3] but with more safety at the cost of 2 extra variables and fake constructors.

piiertho commented 4 years ago

Personnally I prefer 3 over 2 (even if 2 seems good too). I agree with ranie that we should not expose Variant and Dictionary should be a MutableMap<Any, Any> I also like @CedNaru hack. But only one question: We cannot use constructor of Dictionary and check for validity in init block ?

CedNaru commented 4 years ago

No, we can't. To check the class we need a reified T, that's why I'm using a fake constructor because there is no class level reified type.

raniejade commented 4 years ago

So the solution that we came up in discord is:

class <K: Any, V: Any> Dictionary internal constructor(keyClass: Class<K>, valueClass<V>) : MutableMap<K, V> {}

fun <reified K: Any, refied V: Any> Dictionary() : Dictionary<K, V> {
      isValidGodotType<K>() // will throw an error if invalid type, unless its Any
      isValidGodotType<V>()  // will throw an error if invalid type, unless its Any
      return Dictionary(K.class, V.class)

Which is a good compromise between [2] and [3]. To represent a true GDScript dictionary you can declare Dictionary<Any, Any>, note that there are caveats when using this, mainly: