Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coproducts type inference issue #1284

Closed
JorgeCastilloPrz opened this Issue Feb 4, 2019 · 9 comments

Comments

Projects
None yet
3 participants
@JorgeCastilloPrz
Copy link
Member

JorgeCastilloPrz commented Feb 4, 2019

Coproducts created by @abergfeld are awesome, but they have a problem of type inference. Let's say I have this network service call:

fun getUserVerified(userId: UserId): IO<Coproduct4<Boolean, UnauthorizedError, UnenrolledError, UnknownError>> =
    userApi.getUserVerified(userId)
        .map { response ->
            when (response.code) {
                200 -> with(response.body<GetUserVerifiedResponse>()) {
                    this.enrolled.cop<Boolean, UnauthorizedError, UnenrolledError, UnknownError>()
                }
                else -> Unknown.cop<Boolean, UnauthorizedError, UnenrolledError, UnknownError>()
            }
        }

I would expect all the cop() call types to be inferred, but it's not able to, so Coproducts become a bit of a pain to use everywhere.

@abergfeld

This comment has been minimized.

Copy link
Contributor

abergfeld commented Feb 4, 2019

For added context, this appears to be directly related to the defaulted Unit parameters as part of the cop functions.

I think what's going on is that it's unable to infer because all 4 of the cop functions effectively have the same type signature without specifying generics because of the defaulted params.

@raulraja

This comment has been minimized.

Copy link
Member

raulraja commented Feb 4, 2019

You can make it work how you want if we change First etc to public and you do instead:

import arrow.generic.coproduct3.*

fun getUserVerified(userId: UserId): IO<Coproduct4<Boolean, UnauthorizedError, UnenrolledError, UnknownError>> =
    userApi.getUserVerified(userId)
        .map { response ->
            when (response.code) {
                200 -> with(response.body<GetUserVerifiedResponse>()) {
                    First(enrolled)
                }
                else -> Fourth(Unknown)
            }
        }

I'd be in favor of removing the cop and other similar methods as they clearly have inference issues with the dummy args as @abergfeld pointed out. The ADT constructors are much friendlier to use than the extensions.

@raulraja

This comment has been minimized.

Copy link
Member

raulraja commented Feb 4, 2019

The immediate task if you agree would be to deprecate cop to be removed in 0.9.1 and make the ADT nodes public instead of internal which is what they are now.

@JorgeCastilloPrz

This comment has been minimized.

Copy link
Member Author

JorgeCastilloPrz commented Feb 4, 2019

Sounds fine to me. Could we create smart constructors a(), b(), c(), d() for those?

@JorgeCastilloPrz

This comment has been minimized.

Copy link
Member Author

JorgeCastilloPrz commented Feb 4, 2019

Wanna take this one @abergfeld ?

@raulraja

This comment has been minimized.

Copy link
Member

raulraja commented Feb 4, 2019

You can create smart constructors but given the verbosity of Coproduct I think it's a better strategy to just keep the ADTs and name them by position like they are now. Note the imported First and Fourth are that of Coproduct3. You need versions of those for each Coproduct type and on top all the syntax which would spike the size of arrow-generic.

@abergfeld

This comment has been minimized.

Copy link
Contributor

abergfeld commented Feb 5, 2019

Yeah I can take this issue.

I was wondering if we would be okay with migrating by deprecating the cop methods as the type inference is not working and replacing it with something like this:

fun <A, B, C, D> A.first(): Coproduct4<A, B, C, D> = First(this)
fun <A, B, C, D> B.second(): Coproduct4<A, B, C, D> = Second(this)
fun <A, B, C, D> C.third(): Coproduct4<A, B, C, D> = Third(this)
fun <A, B, C, D> D.fourth(): Coproduct4<A, B, C, D> = Fourth(this)

We could then separately make the data classes not internal if we still want to but we'd keep the simple extension way of creating a Coproduct

@JorgeCastilloPrz

This comment has been minimized.

Copy link
Member Author

JorgeCastilloPrz commented Feb 5, 2019

That's what I was suggesting. I'd be fine with that. Maybe we want to have a separate artifact or something? arrow-generic-syntax ? that'd be an option if the problem is artifact size @raulraja .

@raulraja

This comment has been minimized.

Copy link
Member

raulraja commented Feb 5, 2019

Looks good and fine to place in just arrow-generic, no need for more modules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.