DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
504 stars 63 forks source link

Change error handling #148

Closed DelSkayn closed 1 year ago

DelSkayn commented 1 year ago

This PR restructures the way we handle Javascript exceptions.

Previously we handled all errors thrown by quickjs by transforming them into Error::Exception{ ... }. Error::Exception expected an exception to be an instance of the javascript class Error and then retrieved fields from the error object and stored them as rust values. If the throw exception was not an instance of Error the library actually panicked before #141.

This approach has a rather large problem, it is impossible to retrieve the original thrown exception value (see #138).

The most straight forward way to solve this would be to store the original value into the Error::Exception variant. Using this approach would mean that Error would need to have a lifetime to ensure the possible value in exception is handled correctly. When experimenting with this approach I found it to be a rather cumbersome change. A lot of the errors raised by the library contain no quickjs values yet they still need an lifetime just for the the Exception variant.

Instead I opted to remove any data from the Exception variant and have it be a marker that an javascript error was thrown. If one wants to retrieve the thrown value this can be done with Ctx::catch. This method returns the last thrown error.

example

if let Err(Error::Exception) = some_func(){
    let error_val = ctx.catch();
    handle_error_val(error_val)
}

For convenience I also introduced CaughtError which is an error type which has the possible exception value stored as part of the error. Defined as:

enum CaughtError<'js>{
    Exception(Exception<'js>),
    Value(Value<'js>),
    Error(Error)
}

Exception<'js> is a wrapper around object for instances of Error which implements Display for printing the error. If the thrown javascript error is not an instance of Error it is stored in CaughtError::Value. I also introduced various methods to easily convert between Error and CaughtError. The above example can also be written as:

if let Err(CaughtError::Exception(error_val)) = some_func().catch(ctx){
    handle_error_val(error_val)
}

Advantages

Merging this would fix #138.

RReverser commented 1 year ago

Using this approach would mean that Error would need to have a lifetime to ensure the possible value in exception is handled correctly.

Couldn't it store Exception as a Persistent member instead?

DelSkayn commented 1 year ago

Couldn't it store Exception as a Persistent member instead?

We could, but the API to access errors would be similar. If you wanted to access the actual error value you would still need to use Ctx to unpack the value:

if let Err(Exception::Value(error)) = some_func(){
    let error = error.restore(ctx).unwrap();
    handle_error_val(error)
}

I didn't like the unwrap here as the error is always guaranteed to be of the correct runtime. It would also introduce a slight overhead as persistent checks that the runtime is the correct one every time it is unpacked but it would remove the extra error state.

Overall I preferred the solution from this PR as it is closer to the quickjs C api.

DelSkayn commented 1 year ago

Also the Error type would not be able to print the javascript error messages since it would be unsafe to access the persistent value outside of Context::with which an Error type using Persistent can escape. So if you wanted nice error messages we would still need to implement a different error type which extracted javascript value from the persistent value.

Similar to Error and CaughtError in this PR.

DelSkayn commented 1 year ago

After using this version of the library I found that this way of handling errors has some inconveniences here and there but I think those can be addressed. I think this way of handling errors is a good basis and allows us to implement more convenient abstraction on top of it in the future. So I am merging it.

RReverser commented 1 year ago

Thanks!