arrow-kt / arrow-core

Λrrow Core is part of Λrrow, a functional companion to Kotlin's Standard Library
http://arrow-kt.io
Other
81 stars 30 forks source link

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

Open LordRaydenMK opened 4 years ago

LordRaydenMK commented 4 years ago

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

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

raulraja commented 4 years ago

Sounds great, thanks Stojan!

On Wed, Sep 16, 2020, 9:10 AM Stojan Anastasov notifications@github.com 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 .

1Jajen1 commented 4 years ago

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:

peterfigure commented 4 years ago

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.

raulraja commented 4 years ago

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.

mjvmroz commented 4 years ago

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?

raulraja commented 4 years ago

@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.

LordRaydenMK commented 4 years ago

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.

mjvmroz commented 4 years ago

Awesome, thanks for the info :)

SiennaSaito commented 4 years ago

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 👀

1Jajen1 commented 3 years ago

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:

This move has two benefits:

Zordid commented 3 years ago

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.

raulraja commented 3 years ago

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.

Zordid commented 3 years ago

@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.

raulraja commented 3 years ago

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.

Zordid commented 3 years ago

@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...

1Jajen1 commented 3 years ago

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:

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!

raulraja commented 3 years ago

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.

Kernald commented 3 years ago

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.

raulraja commented 3 years ago

@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.

larryng commented 3 years ago

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?

raulraja commented 3 years ago

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?

larryng commented 3 years ago

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

peterfigure commented 3 years ago

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?

nomisRev commented 3 years ago

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.

larryng commented 3 years ago

@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.