haxetink / tink_await

Haxe async/await
MIT License
58 stars 15 forks source link

Typing all failures as Error #4

Closed benmerckx closed 5 years ago

benmerckx commented 8 years ago

I've implemented this in e732154 but I don't know if it's the right way to go. Before this was possible:

@async function throwMeaningfullException() throw SomeEnum.SpecificError;

@async function catchError() {
  try
    @await throwMeaningfullException();
  catch(e: SomeEnum)
    // switch
  catch(e: Dynamic)
    // something else went wrong
}

Now this is a bit less natural:

@async function catchError() {
  try
    @await throwMeaningfullException();
  catch(e: Error)
    if (Std.is(e.data, SomeEnum))
      // switch
    else
      // something else went wrong
}

The Error object currently doesn't expose much more useful data. The position will almost always resolve to here (unless you throw an Error yourself). I could change that to include the real position but it would create way too much position info in the generated output. Once the callstack is included it will however have some advantages.

Maybe we simply need a better way to work with TypedErrors? @back2dos? Maybe I'm just using Error all wrong? I'd like an easy way to switch over the type. This is sadly not possible as the type must exist at runtime:

@async function catchError() {
  try
    @await throwMeaningfullException();
  catch(e: TypedError<SomeEnum>)
    // switch
  catch(e: Error)
    // something else went wrong
}

I'm unsure if marking TypedError as @:generic could facilitate something like this.

back2dos commented 8 years ago

I'll need some time to read the code and think about it.

In the meantime, what caused you to wrap everything in errors in the first place?

benmerckx commented 8 years ago

This came about after the discussion here: https://github.com/haxetink/tink_await/issues/2#issuecomment-222499293 Now that I've partially implemented it, it feels like a bad idea and I'd like to revert it :)

kevinresol commented 8 years ago

Looking back, not sure why I made that comment for wrapping everything in errors.

But the discussion originated from a request of specifying the error type for the function. Now I think it is unsolvable because Haxe does not enforce the type of a throw. In other words, you can't specify that you can only throw a String (or any specify type) in a function.

As a result, it is not possible to enforce the Failure type, because that is a consequence of a try/catch process. That also means I have to always put a catch(e:Dynamic) as a final catch. But again, that's the language's problem. Maybe we can't do much on that.

kevinresol commented 8 years ago

Ok, my memories are back. I mentioned positions and stacks. When an error "bubbles up", it is caught by a try/catch block in each layer and then being thrown again from there. That causes the loss of stack, that means CallStack.exceptionStack() only traces the error back to the "most recent throw", but not to original throw pos. So wrapping the error in Error, plus the stack implementation, the exception stack can be preserved.

benmerckx commented 8 years ago

I've reverted this and released as 0.1.0 as -lib tink_await. My reasoning is it's easy to throw an Error yourself which will also contain much more useful information (position etc.). And the previous/current behaviour is much more in line with what you would expect as compared to synchronous code. We could eventually try and throw all unexpected exceptions as Error objects, but when you're explicitly choosing a type yourself I don't think we should force it to Error.

kevinresol commented 8 years ago

I started to bump into walls these days. I have switched most of my code base to await and I find it really hard to debug unexpected exceptions such as null field access. As mentioned before, the problem is that while the error message may give you some hint about the error itself but there is basically no information about the exact position where the error occurred because the error has been rethrown and the original stack has been overwritten.

I am not sure what the solution should be, but this is really frustrating. If not by default, can it be a flag to enable tink.Error wrapping? So when I encounter the aforementioned problem I can at least switch that on to get more information?

benmerckx commented 8 years ago

So your problems arise mostly from unexpected behaviour? Because I think we can follow a different path for those than we do for throw statements/failures. For example I think it should be possible to either:

kevinresol commented 8 years ago

Can it be a flag to switch between these two options?

Because I am currently working on a web server. During development I would like it to halt right away, which is usually easier to debug while for production I would like it to get wrapped so the server won't crash.

benmerckx commented 8 years ago

Okay, to get you on your way the last update should help. In 0.1.3 there's -D await_catch_none which disables the automatic catching of unexpected errors, which can be useful during development. I'll see if I can reliably wrap unexpected exceptions as Error with proper position/stack information next.

kevinresol commented 8 years ago

great! I will give it a try

kevinresol commented 5 years ago

I think this is getting a bit complex:

@:async function errored()
  throw new Error('Errored');

$type(errored); // Void->Promise<T>

@:await function foo()
  try errored() catch(e:Dynamic) trace(e); // null, because it is the data field of Error

function bar()
  errored().handle(function(o) switch o {
    case Success(_):
    case Failure(e): trace(e); // [object Error]
  });

Currently I made throw new Error doesn't get wrapped in an extra layer of Error which is good for people who runs .handle directly on the transformed function. Because they get the exact Error object originally thrown.

But for people who use the transformed function with tink_await @async and @await plus try catch will only get the data field of the object

I am not sure what to do with this. Maybe the idea of using Promise isn't that good at all.

Edit: could we somehow add some extra flag to the auto-wrapped error https://github.com/haxetink/tink_await/blob/master/src/tink/await/Error.hx#L12 and use that to determine if we should unwrap in a catch clause?