Possible fix for unnecessary casts to KProperty1 inside validatableOf calls

[bejibx]

I notice there are several places where library code have difficulties using definitely non-nullable types:

• https://github.com/nesk/akkurate/blob/492b84710d3e1028422586926bc0f7e0963ad0ca/akkurate-core/src/commonTest/kotlin/dev/nesk/akkurate/validatables/ValidatableTest.kt#L106
• https://github.com/nesk/akkurate/blob/492b84710d3e1028422586926bc0f7e0963ad0ca/akkurate-core/src/commonTest/kotlin/dev/nesk/akkurate/validatables/ValidatableTest.kt#L118
• https://github.com/nesk/akkurate/blob/492b84710d3e1028422586926bc0f7e0963ad0ca/akkurate-ksp-plugin/src/main/kotlin/dev/nesk/akkurate/ksp/ValidateAnnotationProcessor.kt#L229

After checking referenced kotlin issue, it seems it is not an actual root of this problem. Actual referenced issue should be this one. Looking at this second issue right now my guess it will stay in backlog for a very long time. And it is definitely not fixed in 1.9.20 and even in 2.0.21 (I checked). I think this could actually be fixed internally just by using nullable type bounds instead of definitely non-nullable type on one method. Instead of

@JvmName("nullableValidatableOfProperty")
public fun <T : Any?, V> Validatable<T>.validatableOf(getter: KProperty1<T & Any, V>): Validatable<V?> {
    return Validatable(unwrap()?.let { getter.get(it) }, getter.name, this)
}

we can do

@JvmName("nullableValidatableOfProperty")
public fun <T : Any, V> Validatable<T?>.validatableOf(getter: KProperty1<T, V>): Validatable<V?> {
    return Validatable(unwrap()?.let { getter.get(it) }, getter.name, this)
}

I already tried this change and everything seems to build/run/work without any problem and with KProperty1 casts removed.

After checking kotlin documentation I feel there is no need for definitely non-nullable type here. Some quotes:

The most common use case for declaring definitely non-nullable types is when you want to override a Java method that contains @NotNull as an argument. For example, consider the load() method

When working only with Kotlin, it's unlikely that you will need to declare definitely non-nullable types explicitly because Kotlin's type inference takes care of this for you.

[nesk]

After checking referenced kotlin issue, it seems it is not an actual root of this problem. Actual referenced issue should be this one.

Yeah, I'm the author of both issues, just forgot to update the comments 😅

I already tried this change and everything seems to build/run/work without any problem and with KProperty1 casts removed.

The issue is with nullable validatables, did you manage to make this example work?

Validator<String?> {
    validatableOf(String::length)
}

Note that those methods shouldn't be used in userland at the moment, only the accessors generated by the KSP plugin should use them. I might have to put them behind an opt-in requirement.

Since they shouldn't be used in userland, I don't think the cast is really an issue?

[bejibx]

Yeah, I'm the author of both issues, just forgot to update the comments 😅

Oh, I notice you're the author only in second one 😁

The issue is with nullable validatables, did you manage to make this example work?

I mean, looks like both tests I mention before are checking exactly this - nullable chaining scenaries and both tests were green after changes. Just to be sure I pasted your example into this method and compiler looked pretty happy:

Right now linter is liying to me about those casts:

But without this cast I get error during build:

None of the following functions can be called with the arguments supplied: 
public fun <T, V> Validatable<TypeVariable(T)>.validatableOf(getter: KFunction1<TypeVariable(T) & Any, TypeVariable(V)>): Validatable<TypeVariable(V)?> defined in dev.nesk.akkurate.validatables
public fun <T, V> Validatable<TypeVariable(T)>.validatableOf(getter: KProperty1<TypeVariable(T) & Any, TypeVariable(V)>): Validatable<TypeVariable(V)?> defined in dev.nesk.akkurate.validatables

When changing method like this

Cast become unnecessary, tests green and nothing seems to explode.

Since they shouldn't be used in userland, I don't think the cast is really an issue?

Right now methods are available. If you really decide to restrict access to those, some users most likely would pretty upset about it. And even in case this method be internal, this cast is adding several completely pointless bytecode operations to every invocation. Not a huge deal, but I like it better without ugly cast and pointless bytecode operations 😊

[nesk]

When changing method like this

I honestly can't make it work the way you did:

Would you mind sharing a reproducible example?

[bejibx]

Can you please try again with added annotation?

@JvmName("nullableValidatableOfProperty")
@JsName("nullableValidatableOfProperty") // <-- THIS ONE
public fun <T : Any, V> Validatable<T?>.validatableOf(getter: KProperty1<T, V>): Validatable<V?> {
    return Validatable(unwrap()?.let { getter.get(it) }, getter.name, this)
}

[nesk]

Seems to work, that's good news. Thank you for the insight!

However, I have some doubts on the reasons that led me to use definitely non-nullable types, I'm a bit afraid I might break something I'm not aware of now that the code is written. (I should have add more comments 😅 )

This issue doesn't seem to be a priority for the moment. Let's keep it open but I want to postpone it for the day I'll work on the K2 compiler plugin, those are related.

[bejibx]

Yeah, I'm also worrying this change could somehow break something for none-jvm users, so absolutely agree with your resolution