Migrate errors to static enums

[jedel1043]

Our current design for TemporalError is essentially a mirror of ECMAScript's error types: https://github.com/boa-dev/temporal/blob/345ad548db498a5fe596ebebfc7f0ea6214fb079/src/error.rs#L39-L43

This works well for engines that have thousands of errors, with each one having a custom message. However, I don't think we need to do that for temporal_rs, and a more rusty approach will be better suited for this. thiserror sounds ideal for this (moreso because it recently added support for no_std).

[jasonwilliams]

This works well for engines that have thousands of errors, with each one having a custom message. However, I don't think we need to do that for temporal_rs

I think we do pass through error messages up from the underlying parsers and things, would this design still work with that?

Also, what advantage does thiserror bring over using static enums?

[jedel1043]

I think we do pass through error messages up from the underlying parsers and things, would this design still work with that?

Yep, because the underlying ixdtf uses this design for errors :)

https://docs.rs/ixdtf/latest/ixdtf/enum.ParserError.html

Also, what advantage does thiserror bring over using static enums?

It removes the boilerplate of having to wrap errors generated by inner calls, implement the Error trait and implement the Display trait. Though, we could do it manually if we see that it's not that hard to maintain.

[nekevss]

Yeah, the initial design was primarily focused on compatibility with Boa's error type. But it probably is not the most optimal error type for a general purpose library.

[Manishearth]

Thought:

struct TemporalError {
   kind: ErrorKind,
   msg: 
}

#[derive(Display)
enum TemporalErrorMsg {
    /// Invalid time
    InvalidTime,
    ...
    Custom(Box<str>),
}

And over time we migrate everyone over from Custom, eventually removing it.

One thing to consider: Some errors might wish to be parametrized. This will make things harder for FFI, but it might still be manageable if the number of parameters is limited and they are all themselves enums.

[nekevss]

My initial thought was something along the lines of:

pub enum TemporalError {
    Range(RangeError),
    Type(TypeError),
    ...
}

We could start with the custom on TypeError and RangeError, and then slowly build them out?

Maybe that's a bit too much in this case

[jedel1043]

We don't really need to store the ECMAScript error type in the error itself, we can just add a big match that returns the type of error, and users can build their ECMAScript errors from the static error and the error type:

pub enum TemporalError {
    Error1,
    Error2(SourceError)
    Error3 {
        msg: &'static str,
        inner: InnerError
    }
}

impl TemporalError {
    pub fn error_kind(&self) -> EcmaErrorKind {
        match self {
            TemporalError::Error1 => EcmaErrorKind::RangeError,
            TemporalError::Error2(err) => err.error_kind(),
            TemporalError::Error3 {inner, ..} => inner.error_kind()
        }
    }
}

error_kind could also be an implementable trait if needed.

[Manishearth]

I think the ECMAScript error kind and the message are essentially orthogonal concerns: the kind is a specced thing, the message is a UX thing.

So I actually do think the kind should be stored in the error: the spec dictates which errors get emitted where, and we should be setting that at the use site, not in some omnibus error_kind ctor.

[jedel1043]

I think the ECMAScript error kind and the message are essentially orthogonal concerns: the kind is a specced thing, the message is a UX thing.

I think precisely because they are orthogonal, they don't need to be stored in the same type. This makes it so that users who are not implementing an engine won't have to pay the memory price of an additional enum in their error types. Having said that, the cost of 8 additional bits for the ECMAScript error type should be virtually nonexistent, so it is a weak argument at the end of the day.

So I actually do think the kind should be stored in the error: the spec dictates which errors get emitted where, and we should be setting that at the use site, not in some omnibus error_kind ctor.

That's a nice approach if we are going to just have a big error enum with all the possible errors that the library can generate. However, the approach could be less ideal if we are going to specialize certain operations to specific error types, since we would need to create a generic error wrapper with the ECMAScript error type. However, I'm expecting that the ergonomics of matching against an inner enum should be pretty similar to matching against a plain enum; at most, it would be an additional method call or field access.

[Manishearth]

Having said that, the cost of 8 additional bits for the ECMAScript error type should be virtually nonexistent

Yep, essentially.

That's a nice approach if we are going to just have a big error enum with all the possible errors that the library can gene

I think that's fine! I don't actually think that the error enum needs to be matched on by users; it should just be used as an optimization, and an FFI transport mechanism.

[Manishearth]

I have a partial attempt in https://github.com/boa-dev/temporal/pull/355

I realized that there are two use cases for enums that are slightly different. One is for FFI to be able to pass around error messages without allocating a TemporalError (which needs to be heap allocated to be passed over FFI). Here we just need something that is integer-like that can be converted to a static string.

The other is for the user to be able to match on things.

The design in #355 creates an ErrorMessage enum that primarily serves the first purpose, but I think it can be made to serve the second with a transformation done in TemporalError.