ReactiveX / RxSwift

Reactive Programming in Swift
MIT License
24.36k stars 4.17k forks source link

Revisit typed errors? #1320

Closed hfossli closed 3 years ago

hfossli commented 7 years ago

We discussed typed errors here on github a while back. Maybe you've discussed on slack as well. I thought maybe it is time to revisit this question.

Unambiguous API The user of your API doesn't need to make assumptions. She/he will know what errors your API produces. So will your unit tests.

Let the compiler do the job Type safe languages have the benefit of letting the compiler do work that you normally have to unit test. Does PushHandler return any other error than PushHandlerError? No need to unit test mundane stuff like that if the compiler can tell you right away.

Sequences that can't fail Some sequences can't fail and that is valuable information when dealing with them as it makes code less engineered and less complex.

~~

RAC has a great writeup on how working with typed errors is like .

Would it be possible to change the API in such way that it is possible to opt-in to typed errors?

I love typed errors, but hey, maybe that's just me :)

hfossli commented 7 years ago

About opting in to typed errors, would that maybe require Swift being able to have default values for parameter type information? I don't know if that exists, is a good idea or anything – just popped in to my mind... E.g. class Observable<Element, ErrorType = Error> so that it is possible to both write Observable<String, MyError> and Observable<String>?

Or is there other ways of opting in?

freak4pc commented 7 years ago

@hfossli that's a great point in general but I also feel like it's a "developer concern" and not necessarily a RxSwift concern. The "official" ReactiveX standard is failing on sequence error.

I wouldn't be opposed to a type of Observable that can't fail / provides typed errors but I'm not sure it's exactly in the scope of this library ...

Also, some people rely on ReactiveX-based implementations to work the same across platforms so having this "discrete" implementation that is only relevant to RxSwift could be highly confusing and needs to be well thought out IMO.

In 99% of the cases these issues are resolved with using a Result type, which is just a "standard" part of RAC, really. RxSwift gives you the freedom of choosing how you want to model your results and doesn't "bind" (pun not intended) you into a specific format (for better or worse)

Just me $0.02

sergdort commented 7 years ago

Hi, guys!

I personally agains typed errors! :)

Also I feel like it violates duality math behind Rx

sergdort commented 7 years ago

Also in my opinion Observable based API are perfect example of Open Closed Principle, looking on API like:

func getSomething() -> Observable<Something>

The data may come from cache, network, streaming connection or both. It allows to model you app without worrying that changes of platform will break something. Adding typed error will most likely break the client if you change type of the error or add another case to your Error enum

hfossli commented 7 years ago

Awesome feedback! Thanks for sharing your insight in to this matter. The scale seems to be in favor of not introducing typed errors. :-)

Theoretically speaking, is it possible to opt-in, do you think? If so, then how? And if so, is it still a bad idea even if it is opt-in?

freak4pc commented 7 years ago

I still feel it would provide too much divergence and that it doesn't belong in the "Main" Repo. I could imagine people installing a RxResult extension that would just add some Observable overrides to change some default behaviors or provide a new type, but I don't get a good feel for anything like this on the main repo (IMO)

kzaher commented 7 years ago

Hi @hfossli ,

I didn't have a lot of time to follow development around Swift 4.0 so maybe I'm wrong about this but it seems to me that Swift doesn't support parametrized error throwing.

Something like this isn't supported:

let lambda: (a: String, b: Int) throws MyError -> NormalResult = ....

If this was supported in Swift then we could probably find some way to incorporate typed errors in RxSwift.

If Swift supported something like class Observable<Element, ErrorType = Error> then we could definitely make everyone happy and it would be a no brainer.

As far as I can tell Swift unfortunately doesn't support parametrized error throwing or default generic parameters. I've heard some rumors but haven't seen anything concrete on this.

If somebody has some new informations please let us know.

When I need something strongly typed I don't model it as error but treat it as normal return value.

You can write Single<MyResult> where MyResult can be either a normal enum or typealias MyResult = Result<String, MyError>.

We can only introduce this feature if Swift natively supports it otherwise naming project RxSwift would be weird IMHO.

freak4pc commented 7 years ago

I think this can be closed, @hfossli Do you have anything you'd want to add here?

hfossli commented 7 years ago

Thank you all for your very valuable feedback!

Sorry for the delay, but I just had to give result-enum a whirl and get some field experience from it. To me it doesn't make much difference using a result-enum. Let me illustrate using some basic example.

struct User {}

enum Result<Value, Error> {
    case success(Value)
    case failure(Error)
}

enum AuthenticationError {
    case badUsername
    case badPassword
    case networkError(underlying: Error)
}

Result-enum


func authenticate() -> Observable<Result<User, AuthenticationError>> {
    return Observable.just(.success(User()))
}

func login() {
    authenticate().subscribe(onNext: { (result) in
        switch result {
        case .success(let user):
            print("handle user \(user)")
        case .failure(let error):
            switch error {
            case .badUsername:
                print("handle bad password")
            case .badPassword:
                print("handle bad password")
            case .networkError(underlying: let underlying):
                print("handle network error \(underlying)")
            }
        }
    }, onError: { (error) in
        print("unknown error?") // or maybe fatalError()
    })
}

Plain

func authenticate() -> Observable<User> {
    return Observable.just(User())
}

func login() {
    authenticate().subscribe(onNext: { (user) in
        print("handle user \(user)")
    }, onError: { (error) in
        if let error = error as? AuthenticationError {
            switch error {
            case .badUsername:
                print("handle bad password")
            case .badPassword:
                print("handle bad password")
            case .networkError(underlying: let underlying):
                print("handle network error \(underlying)")
            }
        } else {
            print("unknown error?") // or maybe fatalError()
        }
    })
}

I think actually using result-enums makes it all a little worse as the cognitive load is increased IMO (when e.g. subscribing, mapping, bridging between plain observables and observables sending result-enum).

For those interested I found some related discussions. In Java they have typed exceptions, but still RxJava chooses not to have any typed errors in its Observable class https://github.com/ReactiveX/RxJava/issues/5221.

Let's revisit this topic if swift lang either gets typed errors in throwing functions or if it is possible to specify a default type for generics like class Observable<Element, ErrorType = Error>.

Again, thank you for your valuable feedback! 🎉

nikita-leonov commented 6 years ago

@hfossli is default associated type can help with default error type? https://twitter.com/johnsundell/status/997183435759382529

nikita-leonov commented 6 years ago

ps nope :(

kzaher commented 6 years ago

@nikita-leonov Did Swift compiler gained some new features regarding typed error throwing?

nikita-leonov commented 6 years ago

@kzaher they didn't, but I believe the question was different. How to provide a typed errors option without introducing breaking change into the code base that is already built around stander ErrorType. So we were looking into an option default generic arguments like described in Generic Manifesto — https://github.com/apple/swift/blob/master/docs/GenericsManifesto.md

With such option it would be possible to provide an option to override default ErrorType, so old code will work unchanged and all people demanding typed errors will retrieve it too. Probably it is time to create some buzz in forums.swift.org and get ball rolling for it.

kzaher commented 6 years ago

Hi @nikita-leonov ,

the main issue isn't default generic arguments, that part has multiple non elegant solutions. Default generic arguments are just polish.

The main issue is will Swift compiler allow typed error throwing.

nikita-leonov commented 6 years ago

Can you clarify why throwing affects decision on Rx design? Most of throws use in Rx is only related to RxCocoa, but not Rx itself. Moreover in scenarios where throws is used in Rx which is very limited it can be replaced with Result. In the last statement in this issue it was said that it is either typed throwing or default generic. So I guess with one of those it is possible to move furthere.

I also assume that typed throwing from Apple is just a confirmation that introducing typed errors in Rx is a right way to go that will make Rx more native to Swift. It is more an extra confirmation to justify a change, rather than an architectural need that limits an implementation.

freak4pc commented 6 years ago

I don’t think there’s any reason to replace anything with a Result type. That decision should be up to the user and I don’t feel like Rx should be opinionated in that way (like ReactiveSwift is).

I think that in general any sense of typed throws that actually respects the throwing type is today between a hack and a non-existing language feature and agree with @kzaher that it would be better to re-examine this if Apple itself introduced truly types error throws.

Otherwise you’re not really becoming “more swifty” and mainly spending your time bridging the gap for non-existing language features.

kzaher commented 6 years ago

RxSwift should use default Swift error mechanism since the library is meant for Swift language users.

Moreover in scenarios where throws is used in Rx which is very limited it can be replaced with Result

If Swift team decides to ship a built in Result type and proclaims that this is the default error handling mechanism, and if Apple is planning to migrate all of their existing APIs to conform to their new Result type, then yes, we will also migrate towards it.

I also assume that typed throwing from Apple is just a confirmation that introducing typed errors in Rx is a right way to go that will make Rx more native to Swift.

What do you mean by this? Links?

It is more an extra confirmation to justify a change, rather than an architectural need that limits an implementation.

I'm not sure what do you mean by this.

When I say parametrized typed error throwing, I mean something like this.

static func someMethod<T>() throws T {}

I was a bit out of the loop, so maybe I don't have the latest information. Is this coming?

nikita-leonov commented 6 years ago

Nope-nope. Sorry for miscommunication, it was an assumption, not a statement. I was saying that I assume you are looking for typed throwing as a justification for an effort and it is not an actual limitation of a language to implement it. Based on your answers I think I am correct.

Also your statement is more strict in compare with statement from @hfossli as he was saying that it is either one or another, typed throwing was not mandatory requirement. Anyway I think I understand a current state of things now. I think it is ok to have two FRP frameworks, one more strict (ReactiveSwift) another one is less strict (RxSwift) and let users choose what they want. Cheers. :)

freak4pc commented 6 years ago

RxSwift is more strict in regards to ReactiveX. ReactiveSwift chooses whichever way they want. Both are fine choices depending on the situation :)

kzaher commented 6 years ago

Anyway I think I understand a current state of things now. I think it is ok to have two FRP frameworks, one more strict (ReactiveSwift) another one is less strict (RxSwift) ...

I don't think this sentence characterizes the spirit of this project correctly and there isn't a parallel between errors from RxSwift and ReactiveSwift.

Both RxSwift and ReactiveSwift have something named error in their events, but they aren't conceptually interchangeable.

You would use error in RxSwift as a short switch:

It is not recommended to encode any business logic in errors, catch them and then parse the error information. This is anti-pattern IMHO.

I'm usually using resultish values as sequence elements because you want the compiler to notify you if you've forgotten to handle some case.

There are some type safe properties that this project offers that some other projects don't:

ReactiveSwift could also advocate that their Signal, Properties, etc... are the latest and coolest things ever.

... and let users choose what they want. Cheers. :)

Sure 😆 🥂 🥂 🥂

There is just one tiny detail nobody is mentioning, so I'm going to mention it anyway. Maybe somebody will find this information useful.

There are actually 3 events in Observable (or 4 in RXS/SignalProducer) and for some reason everybody is forgetting about Completable event (or Completable/Interrupted in RXS case).

I can't be entirely sure why people are advocating for parameterized errors, but I'm assuming that they are probably advocating this because they want to be sure that they've handled all of their errors, and their user interface won't die because they've forgot to handle an error (that's one of the reasons why we have Driver/Signal in RxCocoa).

BUT

Even if we parameterized Error type, that still wouldn't be enough to guarantee that your user interface won't die. Your sequence can Complete and you also need a guard against that.

This is not just theoretical concern, consider this code:

let iCanSometimesErrorOut: Observable<Int, Error>;
let iMSafeToBindToUI: Observable<Int, NoError> = iCanSometimesErrorOut
     .catchError(x => Observable.empty())

Hint: If you've found this comment, more useful approach is maybe retry family of operators but be careful with them, because you could accidentally overwhelm your service and create a DDOS attack on your service if you don't properly design a retry strategy, so try to implement some binary back-off retry strategy to prevent these kinds of issues.

This is perfectly safe to bind to user interface, but it doesn't prove compile time that your code is correct just because it has NoError. You UI will still die even if no error is thrown. Complete event is also a silent killer.

I'm not trying to say that reminding the user that somebody hasn't handled the error properly isn't a nice handy thing (I find this really useful with Driver/Signal from RxCocoa), but that it doesn't provide you with guarantees that one would assume from the code itself.

The case I've presented isn't theoretical. I've been watching people day after day people writing code like this (usually with Driver, but nothing in the interface would prevent people from doing the same thing with Observable if we added typed errors). If you are thinking the problem was people, I'm not that convinced, these were very capable people IMHO that I respect, but they just needed time like everyone else.

One could think, ok, let's add typed complete event.

let iCanSometimesErrorOutAndComplete: Observable<Int, Error, ()>;
let iMSafeToBindToUI: Observable<Int, NoError, NoComplete> = iCanSometimesErrorOut
     .catchError(x => Observable.never()) // <--- just change to never

This again won't work. One can just replace empty with never and you are back to square one. Your program type checked, your sequence will never complete, but your user interface is dead again because it won't ever receive any elements.

If errors regarding .catchError(x => Observable.empty()) are common, it is reasonable to assume that errors regarding .catchError(x => Observable.never()) would be common somewhere approx same order of magnitude.

But yes, we don't have NoError type in Observable, sorry ¯\_(ツ)_/¯

nikita-leonov commented 5 years ago

I hold my breathe to see how Swift APIs will change with appearance of Result. Do we see a typed error handling in Swift as a reason to discuss this topic for RxSwift one more time?

freak4pc commented 5 years ago

There’s not much to hold breath for unfortunately. Swift is open source and we can see how their result type looks; it’s not different than most implementations and it doesn’t have typed errors.

hfossli commented 5 years ago

To me it seems to have typed error https://github.com/apple/swift-evolution/blob/master/proposals/0235-add-result.md or maybe i am missing something.

The declaration is

public enum Result<Value, Error: Swift.Error>

Which allows to define the type, right?

kzaher commented 5 years ago

It looks like Swift 5 introduces Result<Value, Error: Swift.Error>.

This thing compiles:

struct Observable<Value, Completed, Error> { }

but it would be ideal if

struct Observable<Value, Completed = Never, Error = Never> { }

would also compile.

As I see it we could introduce something like

struct ObservableXXX<Value, Completed, Error> { }

and make

typealias Observable<Value> = ObservableXXX<Value, (), Error>

I think that if we did that it would be win/win situation because we would maintain backwards compatibility and provide more type safety options.

Once Swift adds default generic arguments, we could just make:

struct Observable<Value, Error = Never, Completed = Never> { }

1rst stage: First provide additional features and type safety together with more dynamic API. 2nd stage: Make a breaking change by effectively changing from:

struct Observable<Value, Completed = (), Error = Error> { }

to

typealias DynamicObservable<Value> = Observable<Value, Completed, Error>
struct Observable<Value, Completed = Never, Error = Never> { }

I think we could implement first stage for RxSwift 5.0. When should we make the second stage is hard to tell because there is a lot of legacy and educational code out there. It would be ideal if Swift 5.0 introduced default generic arguments and if we just made it all at once, but it seems like this is not the case.

For the second stage one could make a global rename of Observable -> DynamicObservable, so it's not a horrible breaking change.

Anyway, these are my thoughts.

nikita-leonov commented 5 years ago

@kzaher nice. I didn't expect Completed in the list for parametrization. Does it mean Completable can be expressed as Observable<Never, Void, Error>? Is there a plan to rethink all the traits in such way? As an alternative it is still possible to stick to Observable<Value, Error> and implement not completable as one more trait.

kzaher commented 5 years ago

@kzaher nice. I didn't expect Completed in the list for parametrization.

If we are doing this, then we are doing this right. Completed event is no less important than any other. Complete communicates does the state machine have completed state. Is the state machine finite.

I actually mostly use

Single<Value, Never>
Observable<Value, Never, Never> (wrapped in `Driver`)

All other combinations are rare IMHO.

Does it mean Completable can be expressed as Observable<Never, Void, Error>?

I guess you can do Observable<Never, Void, Error> or Observable<Never, Void, Never> depending on your use case. Define it as you wish.

Is there a plan to rethink all the traits in such way?

Yes, we would generalize all of them, not just Observable.

As an alternative it is still possible to stick to Observable<Value, Error> and implement not completable as one more trait.

I don't think ignoring Completed makes sense. You will need to typealias Observables anyway if you want to make your code sane. The most compelling purpose of introducing Error parameter is to be able to communicate that the state machine can never enter final error state. Final complete state is almost as bad if you want to play the type safety card. Read my posts above for rationale.

hfossli commented 5 years ago

Your proposal is very interesting and appealing to me. Being able to opt-in to compile-time guarantees about completed never being called etc should take some cognitive load away when reading and writing code. I do hope the cognitive load isn't burdened down by type declarations and semantics. What do you think?

PS: My opinions might not be representing the users of this project as I am not using Rx actively these days because of my current projects and constraints.

sergdort commented 5 years ago

@kzaher as I understand we want to be Completed = Void | Never. Am I correct?

kzaher commented 5 years ago

@sergdort Yes, these are the only values I've used.

sergdort commented 5 years ago

I just wonder if it would be technically possible to define a type Observable<String, Int, Error>?

kzaher commented 5 years ago

Yes, it would be.

freak4pc commented 5 years ago

Would you actually want Completable to have something which isn't Never or Void? Kind of strange. Wouldn't it make sense to limit these?

kzaher commented 5 years ago

@freak4pc How would you limit them?

nikita-leonov commented 5 years ago

@kzaher I think this should be enough.

protocol CompletableToken {}

struct Completes: CompletableToken {}
struct NeverCompletes: CompletableToken {
    private init() {}
}

struct Observable<E, C: CompletableToken, F: Error> { /*...*/ }

CompletableToken still available for engineers to implement their own completion tokens, but I think it is higher barrier in compare with using raw generic. Also eventually we can use opaque result types proposal when it will land to hide CompletableToken constraint completely I guess.

nikita-leonov commented 5 years ago

ps I think opaque result types would not have with proper isolation of CompletableToken protocol, but I think there will be at some point proposal that address this anyway, as it is common to limit use cases of exposed code. The same way as we have open, final etc.

nikita-leonov commented 5 years ago

I found that there is an ongoing discussion for ‘sealed’ protocols to cover exactly such case - https://forums.swift.org/t/sealed-protocols/19118 Meanwhile there is also a workaround that allows to achieve the same that should not affect dev experience match and ensure proper use of Completable tokens - https://gist.github.com/anandabits/bd73521e0c5c06371f4a268ab8c482c9

kzaher commented 5 years ago

Hi guys,

It has been a long time since I've responded here. Releasing 4.4.1 was a priority. I'm hoping that 4.4.1 is the last patch we need to release for 4.x version and that the next version will be directly 5.0. I guess this is the only major change for 5.x.

The plan for 5.x would be to first create a 5.x branch and create a prototype of generic Observable<Value> = ObservableSource<Value, (), Swift.Error>.

I'll try to do that this or next week, but can't make any promises. After that I guess it's a matter of porting all of the operators.

protocol CompletableToken {}

struct Completes: CompletableToken {}
struct NeverCompletes: CompletableToken {
  private init() {}
}

struct Observable<E, C: CompletableToken, F: Error> { /*...*/ }

@nikita-leonov I don't think this limits it to just NeverCompletes or Completes.

freak4pc commented 5 years ago

Should we rename E, C and F to - Element, Completion: CompletableToken, Error: Swift.Error ? That F doesn't make a lot of sense.

By the way, if you want to do some division of work and give me some operators to migrate I'd love to help out and take some work off your hands. Let me know :)

kzaher commented 5 years ago

Should we rename E, C and F to - Element, Completion: CompletableToken, Error: Swift.Error ? That F doesn't make a lot of sense.

We can take a look at Result terminology. I think they use failure terminology. I think error probably makes most sense although I would like to understand the rationale why did they name it failure.

Is there some difference between error and failure? Exception is probably the weirdest name since it's actually statistically the common case if you do fizz tests on some API :)

freak4pc commented 5 years ago

I think they named it failure because its a stateful enum case - "success" and "failure" are the enum cases. It was also a well-known community implementation in Swift and other languages specifically for the Result type.

kzaher commented 5 years ago

But why did they name it failure vs error? If they really wanted it to stand out they should have just used Haskell Either and left, right.

freak4pc commented 5 years ago

I think because error is a noun (like 'a ball') and failure is a state (like 'playing').

edit: I guess failure is a noun too but it more represents a state of failure, than a state of error. I'm not entirely sure why, to be honest. 🤔

kzaher commented 5 years ago

Hm, I really need to learn english well before I start with german. It seems to me that you can make state out of both of them.

Don't know, my brain just doesn't care about these things no matter how hard I try. I'm fine with (failure or error). Error is probably more consistent with our codebase, but if there are failure admirers out there, I'm all ears.

freak4pc commented 5 years ago

I prefer error as well. It represents the Swift.Error type anyways. I was mainly asking about the generic letter you assigned to it (F). I imagine you did this because E is for the element type, in which case it might make more sense to do Error: Swift.Error instead of F: Swift.Error. But alas, I don't mind that much :)

kzaher commented 5 years ago

That was not my code.

freak4pc commented 5 years ago

Sorry, didn't see the comment you copied it from.

nikita-leonov commented 5 years ago

@kzaher in way as I defined it, it limits to anything that implements CompletableToken, and I suggest that RxSwift will provide two default implementations of CompletableToken that will satisfy two major use cases of completes & never completes. IMHO it provide an extra constrain that informs engineers in way they will not make obvious mistakes. However it will be still possible to break it even in a proposed way.

kzaher commented 5 years ago

I'm playing with ideas for RxSwift 5.0 here. Didn't have much time to play with this, but hoping to get benchmarks running soon so we can estimate performance impact.

I'm trying to simplify the project structure while doing this, but there is bunch of code in the project :)

I'm publishing this just in case anyone is interested in the prototype and has early feedback.

Once benchmarks are running, we'll estimate the performance impact, optimize for the benchmarks and then port the rest of the project once we've profiled it.

At least that's the plan.

stephencelis commented 5 years ago

@nikita-leonov I don't think this limits it to just NeverCompletes or Completes.

You could use a closed class hierarchy instead of a protocol.

nikita-leonov commented 5 years ago

Yeah my fault, you are right @stephencelis . It should not be not a protocol, but a class with private init. So both NeverCompletes and Completes are implement it, but no one else outside of module can do so. Ugly trick, but what else we can do with the current state of the language? ps Thanks for course correction :)