arrow-core icon indicating copy to clipboard operation
arrow-core copied to clipboard

["Request"] Undeprecate `Option` and move it to a separate module

Open LordRaydenMK opened this issue 3 years ago • 25 comments

What version are you currently using?

0.11

What would you like to see?

Option NOT @Deprecated

Short summary:

Everything Option can do can be achieved with nullable types in a more performant way and it's more idiomatic kotlin code. However for Java compatibility when using frameworks like RxJava (popular on Android) and Project Reactor (popular in Spring world) that is not possible since they explicitly prohibit Null values: https://github.com/ReactiveX/RxJava/wiki/What's-different-in-2.0#nulls

The proposed workaround (with typealiasing Either https://github.com/arrow-kt/arrow-core/issues/114#issuecomment-641211639) is not ideal, confuses FP beginners.

More context in this slack thread: https://kotlinlang.slack.com/archives/C5UPMM0A0/p1600160092340000

  • Create new repository/module arrow-option
  • Move Option to new module and remove @Deprecated
  • Provide interop with Java's Optional
  • Explore optimizing Option like Kotlin's Result

@raulraja feel free to add anything I have missed. Thanks.

LordRaydenMK avatar Sep 16 '20 07:09 LordRaydenMK

Sounds great, thanks Stojan!

On Wed, Sep 16, 2020, 9:10 AM Stojan Anastasov [email protected] wrote:

What version are you currently using?

0.11

What would you like to see?

Option NOT @Deprecated

Short summary:

Everything Option can do can be achieved with nullable types in a more performant way and it's more idiomatic kotlin code. However for Java compatibility when using frameworks like RxJava (popular on Android) and Project Reactor (popular in Spring world) that is not possible since they explicitly prohibit Null values: https://github.com/ReactiveX/RxJava/wiki/What's-different-in-2.0#nulls

The proposed workaround (with typealiasing Either #114 (comment) https://github.com/arrow-kt/arrow-core/issues/114#issuecomment-641211639) is not ideal, confuses FP beginners.

More context in this slack thread: https://kotlinlang.slack.com/archives/C5UPMM0A0/p1600160092340000

  • Create new repository/module arrow-option
  • Move Option to new module and remove @Deprecated
  • Provide interop with Java's Optional
  • Explore optimizing Option like Kotlin's Result

@raulraja https://github.com/raulraja feel free to add anything I have missed. Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/arrow-kt/arrow-core/issues/238, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADPQXAWFUSTDS5DCMYKXLDSGBQHFANCNFSM4ROLNH2Q .

raulraja avatar Sep 19 '20 06:09 raulraja

It isn't just for java compat: Every framework that uses null for control flow on generics will break when nullable types are used on it. (I've fallen into this quite a bit on the STM implementation...)

class MyControlFlowImpl<A>(val result: A?)

This will run into problems as soon as someone uses a nullable A because kotlin cannot handle those nested cases.

~As for optimization, it would need benchmarks but I am pretty sure we could do:~ Forget this, it fails the same ways as nullable types do.

inline class Option<A>(val res: Any?) {
  object None
  fun getOrNull(): A? = res.takeIf { it !== None }
}

fun <A> Some(a: A): Option<A> = Option(a)
fun None(): Option<Nothing> = Option(Option.None)

~Something similar should work just fine.~

Regarding deprecation, imo we should leave it deprecated and link to a doc page explaining where it went and when to use/not use it.

I haven't actually checked any code here, it is all just guesswork :see_no_evil:

1Jajen1 avatar Sep 21 '20 12:09 1Jajen1

Not to hijack this issue but is there an idiomatic way to do something akin to monad comprehensions with Kotlin's nullables, ie something that doesn't degrade into nested hell? Currently, that's a strong use case to me for maintaining Option.

peterfigure avatar Sep 21 '20 16:09 peterfigure

Hi @peterfigure, it is possible and we plan on adding nullable to the list of comprehensions based on the new arrow-continuations module which is gonna fuel the impl of all monad comprehensions and effect like handlers. Jannis, Simon and I already saw two versions of that working and we need to port it to the next release.

raulraja avatar Sep 21 '20 19:09 raulraja

Considering the long history of null as a special value on the JVM, the relatively ubiquitous use of Option/Maybe by FP enthusiasts, and the relatively imminent arrival of inline classes, I was very surprised to see Option tagged as deprecated when I bumped my Arrow dependency in a project yesterday.

It appears that there are a reasonable number of people who share these views, and – thankfully – this request appears to have legs. But that inference is all I have to go on, which is not super comforting. Can I reasonably assume that Option will soon be undeprecated?

mjvmroz avatar Sep 23 '20 00:09 mjvmroz

@mirichan it may not be in core and may no longer appear in effect type classes etc but you will have an option data type with most of the API we have even if it's in a separate dependency. @LordRaydenMK was looking into it based on the discussion.

raulraja avatar Sep 23 '20 09:09 raulraja

I am already working on this. Unfortunately it's not as simple as I first assumed. There is a circular dependency between the new arrow-option module and arrow-core.

Also there are methods in core that work with Option that don't have nullable counterparts. I am trying to fix that here: https://github.com/arrow-kt/arrow-core/pull/239

Only after all public API methods that have Option in them are removed, we can extract Option to a separate module.

LordRaydenMK avatar Sep 23 '20 17:09 LordRaydenMK

Awesome, thanks for the info :)

mjvmroz avatar Sep 23 '20 18:09 mjvmroz

Just poppin' in from Swift land to say that I'm shocked that typealias Optional<T> = Either<Void, T> is being considered as justification for removing Optional because

Optional<Int>.none.map { 1 }

Is nowhere near the same as

Either<Unit, Whatever>.left(()).mapLeft { 1 }

And I think that Option should be left in as is and not moved to a different module because if I'm pulling an FP lib I kinda want FP things regardless of performance and people who don't want these things can just not import them.

But that's like, just my opinion 👀

SiennaSaito avatar Sep 28 '20 09:09 SiennaSaito

Well Option<Int>.map(f) is equal to Either<Unit, Int>.map(f) and mapLeft is an entirely different method altogether.

Option<A> == Either<Unit, A> is a fact and easy to prove. However it is not what I'd use as a replacement either way:

  • If possible nullable types should be used. The special lang support enables a lot of convenient features, among the best are refractoring doesn't necessarily force changes on call-sites and easy compiler optimization.
  • If that won't work because you need a nested A?? (which is the only use case of Option nullable types cannot cover) then there will be this module to use.

This move has two benefits:

  • it promotes nullable types as default (simply by not having an alternative in core)
  • it slims down core. And less code is always a good thing^^

1Jajen1 avatar Oct 08 '20 15:10 1Jajen1

Even more! It isn't true that a nullable type can do everything that an Option can do!

I use an Option of a nullable type deliberately because the value, if present, can also be null. This cannot be done with a nullable type as I cannot distinguish between "not there" and "there, but null".

Moving the type - okay. But depreciation? That's not okay.

Zordid avatar Oct 08 '20 20:10 Zordid

Thanks for your comments @Zordid . Fortunately the arrow maintainers decide what's ok or not in the arrow data types. Having said that you will have access to Option if you want but it won't be in arrow core.

raulraja avatar Oct 09 '20 08:10 raulraja

@raulraja I do not really understand why you had to put it this way, Raul. No offense, but the reasoning for depraction of Option type is simply not holding to the facts, as I pointed out. There are things, nullable types cannot do - namely distinguish between the absence or presence of a nullable datatype. Thus, all I say is: one should be more careful deprecating types and deciding on drastic measures - because in real production code, any warning like that will raise a lot of attention - unnecessarily if it's "only" to be moved.

That said: in FP, Option / Maybe is an essential type - to deprecate it in a release version of an FP library will be confusing for many people.

Zordid avatar Oct 09 '20 08:10 Zordid

You said deprecation is not ok, we are gonna move it and even in that case it most likely won't preserve the namespace so the type needs to be deprecated even if the change is just package imports. Also we should not copy or do what 'FP' is supposed to be but what makes sense of what we think FP in Kotlin should be. If we follow these rationales about IO or Option we are not getting anywhere new with Arrow and falling behind compared to what the lang itself provides. I didn't mean to be rude, my apologies I came out like that, but I'm tired of seeing comments and rants about Option and other that come with no code or rational that can be discussed beside what people think FP is o which data-types should be included without proof. I've already proposed many alternatives to save Option but all it's defense is that we mostly get hot takes and edge cases where it all points to bad decisions made on the other side. For example using null for other purposes on Rx. Not directed to you specifically but If you all want Option in Arrow the way to save it is help @LordRaydenMK make sense out of its location, optimizations and API. Most uses cases we've seen for Option in Kotlin are wrong compared to usage in nullable types.

Additionally I don't think having null as a value in the Some case is a good use case to preserve Option but happy to hear a convincing argument against or use case that justifies keeping all this code around.

raulraja avatar Oct 09 '20 09:10 raulraja

@raulraja yeah, I can see how people get frustrated - just like I did. I mean, I would not have started that much of a discussion if the deprecation comment was true - but it isn't, and that's what strikes me. You behind Arrow are geniuses compared to me, but even I see that saying "Option is not performant, use nullable types of Kotlin instead" is simply wrong. As I pointed out, there are absolutely usages that we need and make a lot of sense.

I cannot follow the reasoning that you even care whether people use Option correctly or not. It is implemented correctly like it is and it is useful. I think library designers should not try to educate their users like this. If I use Option where I also could simply use a nullable type, that's my possible loss of performance, don't you think?

I just want to understand why Option is such a big deal for the library not to be able to be evolved further...

Zordid avatar Oct 09 '20 09:10 Zordid

Option will be moved. The change of the deprecation message needs to happen and is afaik part of a pr to remove the option api and replace it with nullables, but that is blocked by kapt. This is on a snapshot so do give it time...

For everyone who plans to keep using Option: Ignore that deprecation warning and when we do remove it (aiming for a short cycle here) just add a dep to the new option module.

As for why:

  • Nullable types cover most (not all, but most) use cases and provide easier interop with the kotlin ecosystem. It is the way forward for kotlin projects. All cases that cannot be covered will be covered by the Option module.
  • Not being in core will ease adoption due to less confusion. It promotes nullable by default which more users should be familiar with.
  • Nullable is faster and easier to optimize for the jvm. The hit by using algebraic types isn't too hard simply because they aren't hot code paths most of the time so this isn't that much of an argument. Just wanted to add it^^

I just want to understand why Option is such a big deal for the library not to be able to be evolved further...

Library adoption. Less working against kotlin standards and more with them. Hence the switch of IO -> suspend and Option -> A? (Only that IO -> suspend is actually isomorphic and Option -> A? isn't hence we are moving Option but slowly removing IO). Also breaking down core to more manageable pieces. Currently core has too much stuff in it that is sometimes more exotic than useful.

Its all a bunch of tradeoffs and we do welcome everyone do keep adding to these discussions to see all sides of it, so thanks for your input!

1Jajen1 avatar Oct 09 '20 09:10 1Jajen1

Thanks @1Jajen1 for dissecting our view so well.

@Zordid

I think library designers should not try to educate their users like this

I personally have other interests in arrow beside educational. The performance even at the micro level matters to me because I'm attempting not just remove Option but lift as many concerns as possible to the compiler before the final stable 1.x release.

If you have this level of interest in arrow I'd like to invite you to get involved. There is no geniuses or gods here, just people trying to improve it and I'm sorry for being upset in my earlier comments.

Also if you would like to chat in person over meet about Option or any other topic that concerns you of Arrow we are always a call away. Most of the discussion of option has happened in the #arrow-contributors and #arrow channels in slack.kotlinlang.org.

raulraja avatar Oct 09 '20 10:10 raulraja

As for why:

  • Nullable types cover most (not all, but most) use cases and provide easier interop with the kotlin ecosystem. It is the way forward for kotlin projects. All cases that cannot be covered will be covered by the Option module.

I don't see how the interop is an argument. A reusable module (being a library or otherwise, it doesn't matter) using Arrow will likely expose other data types as well (e.g. Either). Removing Option doesn't change much here.

  • Not being in core will ease adoption due to less confusion. It promotes nullable by default which more users should be familiar with.

Promoting nullable by default is a valid argument, but I think it will create more confusion, not less. It's just my opinion though - but it might be more representative of other users' vs maintainers.

  • Nullable is faster and easier to optimize for the jvm. The hit by using algebraic types isn't too hard simply because they aren't hot code paths most of the time so this isn't that much of an argument. Just wanted to add it^^

(I'm ignoring this one as, as you stated, the impact is minor - it's mostly the justification behind promoting nullable by default, if anything.)

I don't have a strong opinion on the matter as I'll just use that additional artifact once it's available and call it a day. But that sounds to me like a lot of noise and effort for little value, if any.

Kernald avatar Oct 09 '20 10:10 Kernald

@Kernald thanks for your opinion and points. I think at this point we are giving people a choice to just change an import and add an additional module and the compromise to maintain it and we won't release again until that is ready. We believe that is a good compromise and still leaves the door open for people wanting to use Option maintained by the arrow authors in the same state it is now and getting improvements down the road.

raulraja avatar Oct 09 '20 22:10 raulraja

Hi Arrow maintainers, I appreciate the work y'all are doing, but this is causing a bit of an issue for our production medical app.

IMO, the plan to deprecate and move Option is fine and good, however steps are being taken out of order. Deprecating Option in 0.11.0 was premature as there is no feasible migration path for those of us who have allWarningsAsErrors / -Werror enabled. The suggestion to use Either<Unit, A> does not make sense if we know that Option is simply being moved to another module in the near future.

We would like to upgrade to Arrow 0.11.0 since it's the first release to use Kotlin 1.4 but are blocked from doing so without either a) compromising on code quality by disabling allWarningsAsErrors, or b) completely re-implementing Option ourselves in the interim.

Is there any downside for undeprecating Option in a bugfix release (0.11.1?) until it is actually migrated to a separate module?

larryng avatar Jan 05 '21 18:01 larryng

Hi @larryng , we are about to release 0.12.0 soon with Option undeprecated and as part of arrow-core. would that solve your issue or would you still need a patch release for the 0.11.x series?

raulraja avatar Feb 09 '21 13:02 raulraja

@raulraja It should solve this particular issue of ours, yes.

larryng avatar Feb 09 '21 20:02 larryng

hi @raulraja - if you are going to preserve Option (🎉) - can I ask why fun <B> map(f: (A) -> B): Option<B> is not declared as inline? unlike Option.fold or Either.map etc.

Currently, this makes it not possible to use with a suspending transformation if I'm not mistaken?

peterfigure avatar Feb 09 '21 20:02 peterfigure

Hey @peterfigure, That has already been fixed here.

Hey @larryng, You can temporarly fix your Option code by annotating it with @Suppress("DEPRECATION") if it's a blocker for you. The 0.12.0 release shoudk happen quite soon though.

nomisRev avatar Feb 10 '21 09:02 nomisRev

@nomisRev I'm well aware of @Suppress. We'd have to add the annotation hundreds of times (fewer if we broadened the scope to the file-level, but that would suppress too much). There are better alternatives if we were willing to change that many LOC.

Fortunately, upgrading tools isn't an urgent concern, so we can wait. But it was a big disappointment to run into this blocker when I was working on it at the time.

larryng avatar Feb 10 '21 16:02 larryng