utopia-rise / godot-kotlin-jvm

Godot Kotlin JVM Module
MIT License
637 stars 46 forks source link

Feature/4.0/default values from instance #434

Closed piiertho closed 1 year ago

piiertho commented 1 year ago

This cause troubles with exported lateinit.
When it tries to get it, it throws an exception because it is not initialized.

Godot-JVM: Try to create res://src/main/kotlin/godot/tests/Invocation.kt instance.
Hello Invocation!
Godot-JVM: Instantiated an object of type res://src/main/kotlin/godot/tests/Invocation.kt
kotlin.UninitializedPropertyAccessException: lateinit property lateinitString has not been initialized
        at godot.tests.Invocation.getLateinitString(Invocation.kt:77)
        at godot.godot.tests.InvocationRegistrar$register$1$1$87.get(InvocationRegistrar.kt:279)
        at godot.core.KtProperty.callGet(Properties.kt:33)
ERROR: Godot-JVM: An exception has occurred!
   at: (modules\kotlin_jvm\src\jni\env.cpp:68)
kotlin.UninitializedPropertyAccessException: lateinit property registerObject has not been initialized
        at godot.tests.Invocation.getRegisterObject(Invocation.kt:80)
        at godot.godot.tests.InvocationRegistrar$register$1$1$89.get(InvocationRegistrar.kt:280)
        at godot.core.KtProperty.callGet(Properties.kt:33)
ERROR: Godot-JVM: An exception has occurred!
   at: (modules\kotlin_jvm\src\jni\env.cpp:68)
Godot-JVM: Try to create res://src/main/kotlin/godot/tests/Invocation.kt instance.
Hello Invocation!
Godot-JVM: Instantiated an object of type res://src/main/kotlin/godot/tests/Invocation.kt
kotlin.UninitializedPropertyAccessException: lateinit property lateinitString has not been initialized
        at godot.tests.Invocation.getLateinitString(Invocation.kt:77)
        at godot.godot.tests.InvocationRegistrar$register$1$1$87.get(InvocationRegistrar.kt:279)
        at godot.core.KtProperty.callGet(Properties.kt:33)
ERROR: Godot-JVM: An exception has occurred!
   at: (modules\kotlin_jvm\src\jni\env.cpp:68)

The default value is set to null (as it is not initialized it results in a NIL variant), even if property type is not nullable. I think this is misleading for user and can lead to big troubles.

When thinking with @CedNaru we went to a point that's a non sense to export lateinit. As exports needs default values that should be resolved during construct time of Object.


Edit @chippmann: Documentation: after some discussion on discord we decided to keep the lateinit case for exported properties. There are still some valid usecases where it makes sense to export a lateinit property. Like exporting nodepaths where one expects them to be set from the inspector. To circumvent the problem described in the PR description, this PR now catches exceptions while creating the instance of the class and while retrieving the default values from it. If an exception arises, the types default value is used instead. This also gives some safety when the user does something fishy during the constructing phase of the class which leads to an exception if the class is being constructed outside of its intended usage (like here in the editor)