Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Provide nullable continuation based on DelimitedScope #251

Merged
merged 15 commits into from Nov 10, 2020

Conversation

kioba
Copy link
Contributor

@kioba kioba commented Oct 15, 2020

Providing nullable continuation implementation after a conversation in the Kotlin Slack channel.

The nullable continuation will allow short circuit null values inside the block.

Questions

This is the first iteration with multiple questions regarding the implementation:

  • DelimContScope used null to represent the end of the computation block but we need to keep the null value open for the results. The atomic values have been nested within the Shift sealed class to allow nullable values and also be able to represent the end of the block. is this effective enough or should we approach it from a different angle introducing an extra atomic value?
  • To be able to use DelimitedScope, arrow-core-data needs to depend on arrow-continuations. Is this the right idea or should nullable continuation be part of the arrow-continuations module?
  • A new BindSyntax has been introduced to allow the existing syntax to be used.example:
nullable {
val a = "a".bind()
a.length.bind()
}

Should we stick to the bind() syntax or just simply use DelimitedScope and allow the user to use shift and reset?

@kioba kioba marked this pull request as draft October 15, 2020 00:06
@raulraja
Copy link
Member

Arrow core should depend on arrow-continuations 👍

@raulraja
Copy link
Member

Bind syntax should change to suspend operator fun invoke. We discussed simplifying the act of binding for suspended application of any data type to deprecate bind and ! and use just suspended invoke

Comment on lines 22 to 26
interface BindSyntax {
suspend fun <A> A?.bind(): A

suspend operator fun <A> A?.not(): A = bind()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
interface BindSyntax {
suspend fun <A> A?.bind(): A
suspend operator fun <A> A?.not(): A = bind()
}
//this will become interface fun when they support suspend in the next release
interface BindSyntax {
suspend operator fun <A> A?.invoke(): A
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extended the comment with a //TODO just to allow discovery on the next release.


/**
* Variable for the next shift block to (partially) run, if it is empty that usually means we are done.
*/
private val nextShift = atomic<(suspend () -> R)?>(null)
private val nextShift = atomic<Any?>(EMPTY_VALUE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually think this is necessary here. Only the result var actually ever has values of type R and thus can lead to nested nulls, this one cannot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this in the meantime, you were right, I confused the nextShift with the result atomic. Thanks for pointing that out! 👍

@@ -11,6 +11,7 @@ apply from: "$DOC_CREATION"

dependencies {
api project(":arrow-annotations")
api project(":arrow-continuations")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think api visibility is a good idea for this as this isn't meant to be used by anyone using core directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that is a good point! Will use implementation here 👍

Comment on lines 8 to 11
override suspend fun <A> A?.invoke(): A =
scope.shift { cont ->
this@invoke?.let { cont(it) }
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be implemented to only call shift when this is null instead of calling it every time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 shift promises to convert A? to A (nullable into a non-nullable). If my understanding is correct, scope.shift is doing this with the continuation and if the value is null the continuation is not called.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the invoke() is executed on a null value there will be no more invoke function called. am I assuming this correctly?

just the illustrate with an example:

nullable {
val a = null.invoke() // last swift call, short circuit to the end

val b = a.invoke() // wont be called because of short circuit 
b.invoke() // wont be called because of short circuit 
} == null

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shift returns whatever is given to it's argument:

println(shift { it(3) }) // => 3
println(shift { it(9); it(5) }) // => 9\n5 (assuming multi shot is used)

So the return value of shift is what we pass to the continuation. But what if we never pass anything to it? Then it never returns anything: (The type of this is Nothing and infinite suspension, throwing and infinite recursion are the only ways to create a "value" of such a type. And since Nothing can be used in place of any type it does indeed also return A)

println(shift { _ -> null }) // => nothing here because we never resume

The function passed to shift has to return the final argument type of the delimited continuation R but how it does so doesn't actually matter. It is free to completely ignore the continuation passed to it.

val a: Int? = null
reset<Int?> {
  val b: Int = a ?: shift { null } // null is perfectly valid for `Int?` and `shift { null }` is `Nothing`
  b
} // => null
val a: Int? = null
reset<Int?> {
  val b: Int = a ?: shift { it(8) }
  b
} // => 8

There are a few things to note:
shift { it(x) } == x if, and only if, invoking the continuation is all you do (no other effects)
shift { x } has type Nothing since it short circuits
shift { it(x); it(y) } will first run the continuation with x and then with y (except we don't really support multishot so this will throw unless you use the multi shot continuation scope which, quite terribly, emulates this behavior)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the assumption that shift converts A? to A is wrong. It simply returns what is passed to the continuation. This is further evident by its type: fun <A> shift(f: ((A) -> R) -> R): A.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok,it started to click. Do you think we could include this explanation in the documentation?

now I see the reason behind what you mentioned.

only call shift when this is null

Will apply the changes quickly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jannis, excellent disection of shift, agreed we need something like that in the docs.

@kioba kioba marked this pull request as ready for review October 20, 2020 23:15
Comment on lines +6 to +12
* ```
* nullable {
* val one = 1.invoke() // using invoke
* val bigger = (one.takeIf{ it > 1 }).invoke() // using invoke on expression
* bigger
* }
* ```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code snippet is more applicable to the nullable object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version of BindSyntax only applies to the nullable implementation usually looking to use the Kinded version. also NullableBindingSyntax will be removed once fun interfaces are allowed to have suspendable functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am currently looking into how could we remove the NullableBindSyntax until fun interfaces are resolved. There is not much benefit of it outside of nullable

package arrow.core.computations.suspended

import arrow.continuations.generic.DelimitedScope

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a description should be added here. Something like:
Binding syntax for a [nullable] within a continuation context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved NullableBindSyntax.kt into nullable as a private class

Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @kioba 😍

@kioba
Copy link
Contributor Author

kioba commented Nov 5, 2020

Awesome work @kioba 😍

Thank you, That means a lot for me! :)

Copy link
Member

@1Jajen1 1Jajen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response :)

This looks good to me 👍

@kioba
Copy link
Contributor Author

kioba commented Nov 10, 2020

Sorry for the late response :)

This looks good to me 👍

No worries, I really appreciate your help and guidance <3

@nomisRev
Copy link
Member

Thank you, That means a lot for me! :)

❤️

@nomisRev nomisRev merged commit 0a5a0a9 into arrow-kt:master Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants