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

tap analog for catch #589

Closed
Artazor opened this issue Apr 16, 2015 · 23 comments
Closed

tap analog for catch #589

Artazor opened this issue Apr 16, 2015 · 23 comments

Comments

@Artazor
Copy link
Contributor

Artazor commented Apr 16, 2015

What about .tapError() with the same signature as .catch (with filters) but the following semantics? (analog to #116)

var noError = {};
Promise.prototype.tapError = function() {
    var e = noError;
    var p = this.catch(function(reason){
        throw e = reason; 
    });
    return p.catch.apply(p,arguments).then(function(value){
        if (e === noError) {
            return value;
        } else {
            throw e;
        }
    })
}

Please understand the motivation (!)

It is motivated by the TypeScript usage (@spion you are welcome to comment)
When you write a catch handler only for logging purposes you never return and always throw, but type signature of resulting promise is completely vanished. e.g.

the problem

return Promise
    .resolve("MyString")
    .then(transform)
    .catch(TransformError, (e: TransformError) => {
        console.log("Transormation failed", e.tranformFailureReason);
        throw e;
    }).then ...   // Here we have Promise<void> instead of Promise<string>

failed attempt

return Promise
    .resolve("MyString")
    .then(transform)
    .catch(TransformError, (e: TransformError): string => {
        console.log("Transormation failed", e.tranformFailureReason);
        throw e;
    }).then ...   // Does not compile with TS1.4 - error TS2355

It fails with

error TS2355: A function whose declared type is neither 'void' nor 'any' must return a value or consist of a single 'throw' statement.

Technically this approach fails only because of microsoft/TypeScript#1613 and will work as soon as static flow analysis will be implemented inTypeScript. However I suppose that it is wrong to specify a return type for this handler. It is not intended to return.

ugly yet successfull

return Promise
    .resolve("MyString")
    .then(transform)
    .catch(TransformError, (e: TransformError): Promise<string> => {
        console.log("Transormation failed", e.tranformFailureReason);
        return Promise.reject<string>(e);
    }).then ...   // Here we have Promise<string> 

proposed

return Promise
    .resolve("MyString")
    .then(transform)
    .tapError(TransformError, (e: TransformError) => {
        console.log("Transormation failed", e.tranformFailureReason);
    }).then ...   // Here we have Promise<string> !!!

Does it sound reasonable?

@Artazor
Copy link
Contributor Author

Artazor commented Apr 16, 2015

Signature for the .tapError would be

declare class Promise<T> {
   tapError<U>(ErrorClass, handler:(reason: any) => U|Promise<U>): Promise<T>;
}

@petkaantonov
Copy link
Owner

I think this is possibly a duplicate

@Artazor
Copy link
Contributor Author

Artazor commented Apr 16, 2015

I suspect that it can be implemented via Promise.prototype._passThrough with some Error filter magic (same as in .catch())

I think this is possibly a duplicate

At least it is not implemented. Is it?

@benjamingr
Copy link
Collaborator

@petkaantonov that is correct.

Duplicate of #517

@Artazor
Copy link
Contributor Author

Artazor commented Apr 17, 2015

@benjamingr technically it resembles the duplicate. However here is absolutely different motivation and strong indications that it is a lacking functionality.

Please answer to the one question:

// Let's consider a classical signature for `catch`
Promise<T>::catch<U>((reason: any) => U|Thenable<U>) => Promise<U>

// wrap :: t -> [t]
function wrap<T>(value: T): Array<T> {
    return [value];
}

// unwrap :: [t] -> t
function wrap<T>(value: Array<T>): T {
    return value[0];
}

// (???) What signature should have the following function
function logAndThrow(e) {
   console.log(e);
   throw e;
}

// ... to be correctly used in the following chain (there should be no type-errors)
var p:Promise<string> = Promise.resolve("test")
    .then(wrap)
       .then(wrap)
         .then(wrap)
           .catch(logAndThrow)
         .then(unwrap)
       .then(unwrap)
    .then(unwrap)
    .then(wrap)
       .catch(logAndThrow)
    .then(unwrap)

@spion - you are welcome to solve this nice type-theory puzzle too :)

@petkaantonov
Copy link
Owner

@Artazor log and throw is an anti pattern that is not embraced by bluebird, that was already discussed in the duplicate thread

@benjamingr
Copy link
Collaborator

@Artazor your type annotations are weird, sometimes you use Haskell and sometimes you use :: for prototype method but ok. If you want implicit side effects (the logging, the type signature is simply:

Promise<T>::logAndThrow(message:String) => Promise<T>

In "Haskell":

logAndThrow:: Promise T -> String -> Promise T 

It's quite trivial, it takes a Promise and a string message and returns a Promise over the same resolution value.

Although, it's worth mentioning your Promise type signature isn't really type safe since catch takes the any type.

@Artazor
Copy link
Contributor Author

Artazor commented Apr 17, 2015

your type annotations are weird

Sorry, my bad -) I've tried to use Haskell in comments only as a clarification.

I'm with you against logAndThrow in the catch-all style. But my question was not about logging. I'm talking about correct typing.

var x:Promise<string> = someRequest()
    .catch(LowLevelError, e => throw new BusinessError(e))
    .then(res => res.stringField)  // fails to compile

Is it a valid use-case?

@Artazor
Copy link
Contributor Author

Artazor commented Apr 17, 2015

@benjamingr

I just want to point out that sometimes inability to assign a correct type to a function means that the function is cluttered with several responsibilities and may be split into several functions with better signatures/implementations/semantics.

Here I suspect that catch is responsible for two distinct things:

  1. Recovering form the exception in previous stage (if new exception is thrown here, then it is a new fresh exception that is not expected, and is completely unrelated to the exception being caught)
  2. Transforming the exception. This one is not intended to interfere with success-callbacks of the downstream. It should simply transform an existing exception into (possibly) another one.

The first one definitely should affect the signature of the resulting Promise
The last one definitely should not (in fact it should return a new Exception, instead of throwing it)

@spion, @petkaantonov ?

@Artazor
Copy link
Contributor Author

Artazor commented Apr 17, 2015

In fact I would be happy if Promise::catchThrow could use an error transformer function as an alternative to the error object.

var x:Promise<string> = someRequest()
    .catchThrow(LowLevelError, e => new BusinessError(e))
    .then(res => res.stringField)  

@benjamingr
Copy link
Collaborator

It's perfectly possible to type catch well.

The reason people don't do it is because people hate checked exceptions and this is effectively the same thing. I talk about this here: http://stackoverflow.com/questions/29286018/typescript-typings-for-failure-reason-in-various-promises-implementations/29287864#29287864 although the types are somewhat wrong for didactic reasons.

Additionally we're never breaking .catch, it's in ES6

@benjamingr
Copy link
Collaborator

Also, JS is not Haskell :)

@Artazor
Copy link
Contributor Author

Artazor commented May 7, 2015

@benjamingr thank you for your response!

I know about checked exceptions. And I've read you answer on StackOverflow. Unfortunately, catch itself can hardly be correctly typed. And checked exceptions are not related to this problem at all (!!!)

Nevertheless, to be more relevant to the real world in your type system there should be type unions (T1 + T2) , exclusions (T1 - T2) and exception monad/checked exceptions, that for the sake of simplicity we will replace with the special type (t ^ e) that means either success with t or failure with e. Promise<t,e> here will be denoted as (P t e)

Thus, we may try to start with the following type:

catch :: (P t e) -> (e' -> (s ^ e'')) -> P s (e - e' + e'')

But this still is not a correct type either.

The core problem for the catch typing is the conditional nature of the catch. It has an ability to conditionally change the type of the success value depending on the state of the source promise. If the promise is rejected then catch handler has a chance to be executed and change a result type. If the promise is fulfilled then entire catch section should be transparent and pass the result through itself along with its type. Voila! Contradiction! Depending on the runtime behavior we have different resulting types.

The only correct typing for catch is

catch :: (P t e) -> (e' -> (t ^ e'')) -> P t (e - e' + e'')

It means that to be semantically correct you should never change the type of the success in a catch handler.

Expressed in TypeScript it looks like:

declare class Promise<T> {
     catch(handler: (reason: any) => T|Promise<T>): Promise<T>
}

I'm working on Bluebird.d.ts for the v 3.0 (I'll publish it soon, I hope). And in fact I'd place such a constraint there. I suspect that all popular Promise library d.ts definitions are vulnerable to this semantic error. Because it is an inherent problem of the Promise.catch.

At the same time, people write correct programs with semantically incorrect tools.
And It would be nice to have a catch alternative that by definition does not change the type of the result.

That is why I've proposed the tapError. It would be a semantically correct version of the .catch().
I would let the .catch() alone for those who will take the risk to use it. Personally, I'll try to avoid using catch in my programs in favour of proposed #612 that is semantically correct.

Also, want to hear from @petkaantonov and @spion

@benjamingr
Copy link
Collaborator

And checked exceptions are not related to this problem at all (!!!)

Putting the type of exception something might throw is exactly what checked exceptions means. Also, I'm sorry for being annoying but can you please use standard type notation? I don't care if you go with Java, c++ or Haskell or whatever but this syntax is weird.

It means that to be semantically correct you should never change the type of the success in a catch handler.

No, it just means that the return type of catch is a Promise for a T E is not bound by Promise for a T ? but rather Promise T|S ?.

It has an ability to conditionally change the type of the success value depending on the state of the source promise.

Yes, that's what it can do right now. This is why the parameter of the return value is a union type and not just T.

It means that to be semantically correct you should never change the type of the success in a catch handler.

No, but it does sound like a good idea, I don't think I've ever changed the return type and it's a good sanity check. At the moment this is not the contract at all but part of typing is that we can enforce stricter contracts than what the language allows. Just because you can pass a promise and not a function to then per-the-spec doesn't mean it's a good idea to do so :)

Note, that while your version doesn't imply it catch performs thenable assimilation, so the >>=d function doesn't return an e of some sort but an either between e and a Promise<e>, which actually makes some sense because >>=ing over an Either only maps one side and that could be assimilation, but meh :)

@spion
Copy link
Collaborator

spion commented May 7, 2015

AFAIK TypeScript can models this properly now with union types. Method would be

declare class Promise<T> {
  catch<U>(handler: (reason: any) => U|Promise<U>): Promise<T|U>
}

If T = U, great. Otherwise, the compiler will complain and expect you to check / cast / use type guards to get the appropriate type (or if you're only using methods common to both T and U, let you go on your merry way)

@Artazor
Copy link
Contributor Author

Artazor commented May 7, 2015

You are right about T|S

But as I see it's not reflected in

and possibly other libraries

declare class Promise<T> {
    catch<U>(
        handler: (reason: any) => U | Promise<U>
    ): Promise<T|U>;
    then<U>(
        fulfilled?: (result: T) => U | Promise<U>,
        rejected?: (reason: any) => U | Promise<U>
    ): Promise<U>
}

Unfortunately, this signature spoils the result.

promiseOfString.catch(ApiError, function(e) {
   throw new BusinessError(e);
});

will have type Promise<string|void> instead of Promise<string>

that is why I wanted for tapError not to change the type of the result. Or, alternatively, catchThrow to have an exception transformer option.

Possibly it is an issue with TypeScript itself. It should have a special bottom type for the result of functions that always throw. And it should be a unit against the type union. i.e.

( bottom | T ) = ( T | bottom ) = T

Probably it would be an ideal solution.

@spion
Copy link
Collaborator

spion commented May 7, 2015

You're right - I fogrot about the pathological case of no return value :D Whoops.

@Artazor
Copy link
Contributor Author

Artazor commented May 7, 2015

Just created an issue in microsoft/TypeScript#3076 - any correction/justification?

@Artazor
Copy link
Contributor Author

Artazor commented May 9, 2015

@benjamingr

Note, that while your version doesn't imply it catch performs thenable assimilation, so the >>=d function doesn't return an e of some sort but an either between e and a Promise, which actually makes some sense because >>=ing over an Either only maps one side and that could be assimilation, but meh :)

Do you mean that I can throw thenables? And they will be unwrapped at the catch side? I've thought catch doesn't perform thenable assimilation from the upstream.

Promise.reject(Promise.reject(new Error())).catch(function(e){
    console.log(e.constructor.name); // it is "Promise" not an "Error" 
})

Or have I incorrectly interpreted your comment?

@petkaantonov
Copy link
Owner

You can throw literally anything (but in 3.0 this gives you a warning if you throw non-errors), thrown thenables or promises are not unwrapped

@Artazor
Copy link
Contributor Author

Artazor commented May 9, 2015

@petkaantonov exactly, I've just wanted to clarify what @benjamingr has meant

@benjamingr
Copy link
Collaborator

You can return rejected thenables whose error state will be assumed. That's what I meant.

@Artazor
Copy link
Contributor Author

Artazor commented May 9, 2015

@benjamingr

You can return rejected thenables whose error state will be assumed. That's what I meant.

Oh, I know that, just not included into the signature for the sake of simplicity.

Also, I'm sorry for being annoying but can you please use standard type notation? I don't care if you go with Java, c++ or Haskell or whatever, but this syntax is weird.

My odd notation came from my abstract algebra background, where at the beginning of any article, report, etc., we always defined a syntax for a following lemmas and proofs.

I've just used the following system for (possibly high order) types:

τ ::= tcon | α | (τ τ) | (Λα.τ) | (τ → τ) | (τ × τ) | ⊤ | ⊥ | (τ ⋂ τ) | (τ ⋃ τ) | (!τ) | (τ ^ τ)

where

  • tcon — type constructor like Either, Maybe, Promise, Int etc.;
  • α — formal type;
  • (τ τ) — type application e.g. Maybe Int;
  • (Λα.τ) — type abstraction (generic type) (suggested Type aliases requiring type parameters microsoft/TypeScript#1616);
  • (τ → τ) — function type;
  • (τ × τ) — tuple type, associative;
  • ⊤ — top type (equivalent to any);
  • ⊥ — bottom type (suggested Special bottom type microsoft/TypeScript#3076);
  • (τ ⋂ τ) — type intersection, associative, commutative;
  • (τ ⋃ τ) — type union, associative, commutative;
  • (!τ) — type negation (type complement to ⊤);
  • (τ ^ τ) — checked exceptions, associative.

This system has usual parentheses elimination conventions and two aliases:

  • τ1 + τ2 = τ1 ⋃ τ2
  • τ1 - τ2 = τ1 ⋂ !τ2

Checked exceptions (τresult ^ τerror) have two simple properties:

  • 1 ^ τ2) ^ τ3 = τ1 ^ (τ2 ⋃ τ3) — exception propagation from inner computations;
  • τ1 ^ (τ2 ^ τ3) = τ1 ^ (τ2 ⋃ τ3) — exception union.

that makes (τ ^ τ) associative. (there are more rules, of course)

My apologies for using this notation before defining!
Anyway, I'll try to use TypeScript whenever the signature is expressible in TS.

-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants