cloud icon indicating copy to clipboard operation
cloud copied to clipboard

Support of default value for parameter in Kotlin

Open Distractic opened this issue 2 years ago • 7 comments

Current Behavior

When we use the annotations to set argument, the default value from kotlin declaration is not supported.

With the current system, the command bellow cannot be executed when the player doesn't define the values for args :

@CommandMethod("test [x] [y] [z]")
@CommandDescription("Test command")
suspend fun testCmd(
        player: Player,
        @Argument("x") x: Double = player.location.x,
        @Argument("y") y: Double = player.location.y,
        @Argument("z") z: Double = player.location.z
) {
 println("parameters : $x $y $z")
}

If the user doesn't add an argument, a null value will be used to set the arguments.

Effectively, in the @Argument, there is a String to define the default value, but in the most cases, it's not enough.

Expected Behavior

The command defined above can be used if the user writes : /test /test 1 /test 1 2 /test 1 2 3

Distractic avatar May 26 '22 07:05 Distractic

When we use the annotations to set argument, the default value from kotlin declaration is not supported.

Default arguments in Kotlin just end up generating overloaded methods. So, in order to support these default arguments we'd have to support overloaded methods. While possible, I am not entirely convinced that this is something we want.

A strategy for implementing this system would be to find all methods with identical @CommandMethod annotations and then pick the most verbose method and infer argument types from that one. We'd also have to have to add support for multiple methods in the command executor.

Citymonstret avatar May 26 '22 09:05 Citymonstret

I don't know if this can help you for the implementation, but with the kotlin reflection, you can call a function using the default variable (if you put not enough parameters) through the method KFunction.callBy. This is an exemple :

class A {

    fun myMethod(
        value1: Int,
        value2: String = "test"
    ) {
        println("$value1 : $value2")
    }

}
fun main() {
    val instance = A()
    val kfun = instance::class.functions.find { it.name == "myMethod" }!!
    val parameters = kfun.parameters
    kfun.callBy(mapOf(parameters[0] to instance, parameters[1] to 1))
}

The console will print 1 : test.

So if you use the KFunction instance, you can directly call the function with default parameter. And for the suspending method, you can use the method KFunction.callSuspendBy.

Distractic avatar May 26 '22 10:05 Distractic

To add on to this:

As per the docs, you can check if it's optional by checking KParameter#isOptional. If it is optional, then it can be omitted when invoking KFunction#callBy.

It seems there is no way to get what the default value is via the kotlin reflection api, however you can still use the default without knowledge of what it is.

solonovamax avatar May 26 '22 13:05 solonovamax

Perhaps a new execution handle, similar to SuspendingExecutionHandler could be introduced for this. It could be added to a module with a name like kotlin-core, perhaps?

Of course, the SuspendingExecutionHandler would also need to be reworked to incorporate these changes as well, as you'd want feature parity for suspending and non suspending functions. It may be possible to just have it extend this theoretical KotlinExecutionHandler. (Any better ideas for names?)

solonovamax avatar May 26 '22 13:05 solonovamax

Apparently overloads are only generated if you use @JvmOverloads. I was under the impression that this was the default behaviour 🥴

If someone wants to go ahead and implement this, then it could probably become a part of cloud. It would have to go in some new module cloud-kotlin/cloud-kotlin-default-annotations or something (similar to the couroutine support module)

Citymonstret avatar May 26 '22 15:05 Citymonstret

Have you any clue to give us in order to start ? Which type of class must have a new implementation for that for example (I'm not fluent source code of cloud)

Distractic avatar May 26 '22 19:05 Distractic

If someone wants to go ahead and implement this, then it could probably become a part of cloud. It would have to go in some new module cloud-kotlin/cloud-kotlin-default-annotations or something (similar to the couroutine support module)

That's why I proposed something like cloud-kotlin/cloud-kotlin-core (in hindsight, I wasn't that clear about it)

And, @Distractic, look at SuspendingExecutionHandler and its references.

solonovamax avatar May 27 '22 02:05 solonovamax