Overload Arrow FX ParZip for tupling #2821

[lgtout]

The one weird thing I need to call out is that there's ambiguity between the overloads of ParZip, as currently implemented.
For example:

val result = parZip(Dispatchers.IO, { Unit }, { Unit }, { Unit })

causes problems for the compiler because it can't tell if we intend to call this:

public suspend inline fun <A, B, C> parZip(
  ctx: CoroutineContext = EmptyCoroutineContext,
  crossinline fa: suspend CoroutineScope.() -> A,
  crossinline fb: suspend CoroutineScope.() -> B,
  crossinline f: suspend CoroutineScope.(A, B) -> C
): C

or this:

public suspend inline fun <A, B, C> parZip(
  context: CoroutineContext = EmptyCoroutineContext,
  crossinline fa: suspend CoroutineScope.() -> A,
  crossinline fb: suspend CoroutineScope.() -> B,
  crossinline fc: suspend CoroutineScope.() -> C,
):  Triple<A, B, C>

In the KNIT examples I added, I disambiguated this way:

val result = parZip(Dispatchers.IO, { Unit }, { Unit }, fc = { Unit })

to select for the Triple-returning override.
This would work, too:

val result = parZip(Dispatchers.IO, { Unit }, { Unit }, { -> Unit })

Neither seem ideal, so please provide guidance on whether or how to improve this.
Thank you!

[nomisRev]

causes problems for the compiler because it can't tell if we intend to call this:

Oof, that is really bad 😞 but a bit strange since the lambda doesn't match 🤔
I thought { -> Unit } didn't match (A, B) -> C since it instead needs to be { _, _ -> Unit }.

[lgtout]

but a bit strange since the lambda doesn't match

Sorry, I don't follow...

I thought { -> Unit } didn't match (A, B) -> C since it instead needs to be { _, _ -> Unit }.

Yes, if I wanted to target the non-triple-returning parZip, I would need to have used { _, _ -> Unit }. But I was targeting the triple-returning overload, so either of the two ways of calling it that I mentioned do that.

[lgtout]

@raulraja asked for a sample that illustrates the overload resolution ambiguity of parZip:

fun <A, B> f(fa: () -> A, f: (A) -> B): B = TODO()
fun <A, B> f(fa: () -> A, fb: () -> B): Pair<A, B> = TODO()

// Compiler errors
val r1 = f({ 1 }, { "one" })
val r2: Pair<Int, String> = f({ 1 }, { "one" })
val r3: String = f({ 1 }, { "one" })

// OK
val r4 = f({ 1 }, f = { "one" })
val r5 = f({ 1 }, fb = { "one" })
val r6 = f({ 1 }, { _ -> "one" })
val r7 = f({ 1 }, { -> "one" })

If the parameters f and fb had the same name, only the last two lines (r6, r7) would be free of the resolution error.

Here's the compiler error:

Overload resolution ambiguity: 
public fun <A, B> f(fa: () -> TypeVariable(A), fb: () -> TypeVariable(B)): Pair<TypeVariable(A), TypeVariable(B)> defined in ...
public fun <A, B> f(fa: () -> TypeVariable(A), f: (TypeVariable(A)) -> TypeVariable(B)): TypeVariable(B) defined in ...

Per Raul's suggestion, I tried applying @OverloadResolutionByLambdaReturnType and @BuilderInference to both fs, but the error persisted.

[nomisRev]

fun <A, B> f(fa: () -> A, f: (A) -> B): B = TODO()
fun <A, B> f(fa: () -> A, fb: () -> B): Pair<A, B> = TODO()

These are not good examples, and don't occur in parZip. There is not ambiguity between { a -> ... } and { ... } but the same is not true for { _,_ -> ... } and { ... } since { ... } doesn't match { _,_ -> ... } type.

I don't think we can release this improvement if you need to specify the lambda name, the goal of this new API was to simplify the API in case you don't want to provide a transform lambda. This API doesn't offer a good improvement, since the API now results in ambiguity and I think it will result in more confusion then it will smoothen things.

cc\ @StylianosGakis originally proposed this API on KotlinLang. WDYT?

[lgtout]

@nomisRev Sorry, I don't understand. If there's no ambiguity between { a -> ... } and { ... }, as you say, why does the compiler say there's resolution ambiguity between the 2 versions of f in the sample I provided?

[nomisRev]

Sorry, maybe I didn't phrase myself well. What I was trying the say is the following:

The Kotlin compiler can infer { ... } to (A) -> B by ignoring the A parameter, so there is ambiguity between () -> B and (A) -> B if you just provide { ... } lambda. Since { ... } can match for both () -> B and (A) -> B.

This is however not the case for (A, B) -> C, since in that case { ... } will not match the lambda type. Kotlin requires explicitly ignoring 2 (or more) parameters by writing { _, _ -> ... }.

For this reason I was under the impression that these functions could co-exist:

fun <A, B, C> f(fa: () -> A, fb: () -> B, f: (A, B) -> C): C = TODO()
fun <A, B, C> f(fa: () -> A, fb: () -> B, fc: () -> C): Triple<A, B, C> = TODO()

Since () -> C doesn't match the (A, B) -> C lambda, so I expected the compiler to be able to disambiguate.

[lgtout]

I don't think we can release this improvement if you need to specify the lambda name

Or, one can also explicitly specify the lambda parameters.

[lgtout]

For this reason I was under the impression that these functions could co-exist

They can co-exist. It's at the call-points that the error occurs, not at the definitions of the functions. That said, the situation is not ideal for an API.

[nomisRev]

That said, the situation is not ideal for an API.

Right, that's what I meant. It can co-exists at the definition site, but it cannot co-exists from the caller side. Requiring to specify lambda parameter name is very foreign in Kotlin.

I think one of the more annoying parts is also that it Kotlin cannot ignore an unused lambda receiver, so doing something like this is not possible parZip(fa, fb, fc, ::Triple) because ::Triple doesn't match the CoroutineScope.(A, B, C) -> D lambda 😞

[StylianosGakis]

Oof, that's just so unfortunate.
needing to provide the parameter name sounds like a no-go for me too. Imagine trying to get someone to get to use arrow who's not that proficient in Kotlin and all they get as an error when trying to use this function is something about ambiguity. A lot of people will have trouble resolving this themselves imo.
I guess needing to do

parZip(
  { ... },
  { ... },
) { _, _ -> }

on the call-site is gonna have to suffice in this case. I don't have any better ideas atm personally.
Thanks for the ping btw!

[nomisRev]

needing to provide the parameter name sounds like a no-go for me too.

Agreed

I don't have any better ideas atm personally.

Only alternative is introducing an different name, parTupled but not sure if adding this improves the status-quo.

Thanks for the ping btw!

Thank you for the feedback @StylianosGakis ☺️

[serras]

@lgtout we've discussed this PR during the biweekly Arrow meeting, and we've agreed to keep parZip as they are now, and introduce parTupled as new functions returning Pair/Triple/Tuple4/... Would you be up for implementing it? Otherwise one of us can pick the work you've done.

By the way, we also talked about the name. parTupled is used in Cats, and it seems nice to keep some continuity between different FP libraries.

[lgtout]

@serras Thanks for the update. No problem. I'm happy to make the change to parTupled.

[lgtout]

@nomisRev @serras This is ready for review. I renamed the tuple-returning parZips to parTupled. Thanks!

[serras]

LGTM

[nomisRev]

I think .gradletasknamecache is incorrectly checked in? 🤔 It adds 6,434 lines 😱
If this is an optimisation for Gradle, can we move it to a separate PR? And could you please clarify how it works 😅 🙏 It's the first time I've seen it.

2 small comments:

• We avoid star-imports in Kotlin
• One suggestions to improve documentation ☺️

Thanks for the great work @lgtout!! 🙏 🥳

[nomisRev]

We avoid star-imports in the Arrow codebase.

[nomisRev]

Avoid star imports.

[lgtout]

Oops! Yes gradletasknamecache should not have been committed. Thanks for catching that.

No problem re the other changes. I'll make them and resubmit.

[nomisRev]

Looks perfect @lgtout! Thank you so much for this contribution! 🙌 🥳