google / promises

Promises is a modern framework that provides a synchronization construct for Swift and Objective-C.
Apache License 2.0
3.8k stars 293 forks source link

Proposal: error handling with generic parameter #39

Closed gloooooooomy closed 6 years ago

gloooooooomy commented 6 years ago

Hello :)

Idea : make a overloaded catch function with generic parameter

extension Promise {

@discardableResult
public func `catch`<ErrorType: Error>(on queue: DispatchQueue = .promises, _ reject: @escaping (ErrorType) -> Void) -> Promise {
    return self.catch(on: queue) { (error) -> Void in
        guard let typedError = error as? ErrorType else {
            return
        }
       reject(typedError)
   }
}

and using like

promiseFunction().catch { (error: MyError) in
    // error is MyError
}.catch { error in
    // default
}

instead of

promiseFunction().catch { error in
    if let myError = error as? MyError {
        // handling error
    }
}
shoumikhin commented 6 years ago

Looks cool :) Do you propose to have it alongside the existing catch operator as an overload? I'm afraid it will interfere with the existing Error-only catch, and the complier would silently pick one overload over another, or complain unless you clearly state the error type. Anyhow, pull requests are more than welcome!

gloooooooomy commented 6 years ago

Yes, i proposed catch operator as an overloaded function. because compiler could infers the generic type only when state the error type.

.catch { (error: MyError) in
  // compiler picks generic catch operator.
}
.catch { (error: Error) in
  // compiler picks default catch. because generic type does not supports protocol.
}
.catch { error in
  // same as "error: Error"
}

also i made a pull requests, please review :)

shoumikhin commented 6 years ago

@gloooooooomy thank you, I love your proposal!

Just have a concern with the case like the following:

enum CustomError: Error { case test }

Promise<Void> {
  throw Test.Error.code42
}.then { _ in
  XCTFail()
}.catch { error in
  XCTAssertTrue(error == CustomError.test)  // Never executed.
}

I imagine, it's expected that the catch above to be executed w/o specifying the error type explicitly, since it must catch all Errors. Unfortunately, it's not, because the compiler translates that one-liner catch as your generic version and derives the type based on the condition in the test assert. I.e. it thinks the catch generic type must be CustomError. But Test.Error is thrown, in fact. So that catch is never executed, since types don't match. To fix that behavior, one should either specify the expected error type explicitly as (error: Error) in(which is not always convenient and can be omitted by mistake), or write more than one line of code inside the catch closure to force the compiler to pick a more suitable non-generic catch overload (and that is potentially error-prone, because the clients may not be aware about such subtle implications).

Ideally, we'd want something like default generic arguments be implemented in Swift to have only one version of catch, and actually make it generic, as you've proposed.

What do you think?

shoumikhin commented 6 years ago

@gloooooooomy let's close this issue for now, but don't hesitate to follow up if you have any ideas on how to deal with the current compiler limitations.

gloooooooomy commented 6 years ago

@shoumikhin sorry for reply too late. :( now i know exactly what you concerned. and I'll check if there's another way to avoid.