Discussions on ReScript exception encoding

#1

Hi all, the current exception is encoded using an object literal attached with an error, e.g
Take Assert_false for example, it is encoded as below:

{
   RE_EXN_ID: "Assert_failure",
    _1: [
          "tscanf_test.ml",
          174,
          2
        ],
        Error: new Error()
 };

This works great when such exception is uncaught, the stacktrace is printed properly.
However, when such exception is caught by third party APIs, for example, mocha or bugsnag, they try to catch the exception and extract stack property which does not exist in our exception object. Therefore, such 3rd party tools will not report stacktrace properly.

I am proposing to change such exception encoding to make it work better with 3rd party tools:

Proposal 1

{
   RE_EXN_ID: "Assert_failure",
    _1: [
          "tscanf_test.ml",
          174,
          2
        ],
       stack: (new Error()).stack
 };

Proposal 2

Object.assign(new Error, {RE_EXN_ID: "Assert_failure",
    _1: [
          "tscanf_test.ml",
          174,
          2
        ]
})

Pros for Proposal 1

  • Easy to implement
  • Matches better with existing semantics
  • Payload is also displayed properly (_1 field)

Cons for Proposal 1

  • unclear stack alone would work better enough with existing tools

Pros for Proposal 2

  • Perfect integration with js toolset

Cons for Proposal 2

  • Payload not displayed properly (need more tweaks)
  • Semantics more deviation from existing encoding
  • More complexity (may need a small runtime)

I tend to implement proposal 1 due to its simplicity, but would like to know if I miss some obvious downside of proposal 1

cc @BlueHotDog

#2

I think that we should prioritize DevEx using rescript in the broader Js ecosystem(that’s was one of the reasons of making the split originally as i recall).
And for me personally, the less idiosyncrasies i have to remember(and document internally, and we, document as a community) the better(also taking into account the limited resources rescript community has).
Also, every such change from the Js Norm, especially for things like exceptions, which are so low level is super risky, there are tons of various tooling relating to exceptions and in my strong opinion, the risk of them not working is not worth it(rescript should be as safe as possible, having exceptions not working is problematic).

Due to the above, i’m strongly for option no.2. it’ll reduce developers mental overhead. and make my system safer to maintain.

Another option, why we can’t use something like
ES6 Custom Error Class(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Custom_Error_Types)?

2 Likes
#3

I understand your concern but there are some inherent complexity that we need to justify the benefit to adopt a more complex solution. Note the exception encoding should always be internal that you should not rely on.

Do you have some popular 3rd toolset that would not work with proposal 1?
That would help me to make a decision based on the trade-offs

#4

Option 2 is definitely more JS-like, and will make interop so much easier.

Tbh, I don’t see much point in going from the current form to proposal 1 since it won’t make the interop any better.

If I may add a suggestion for option 2, it would be great if we added a message to the error:

Object.assign(new Error("ReScript Error at tscanf_test.ml (174:2)"), {
    RE_EXN_ID: "Assert_failure",
    _1: [
          "tscanf_test.ml",
          174,
          2
        ]
})

It would help so much with debugging, since you don’t always the Object.assigned information when the error is printed.

#5

let me explain a bit clear.
The motivation is just to make stacktrace works better with other tools (it is not for interop).
My proposal 1 would work with mocha (I just tested) stacktrace reporting, I believe it should also work with bugsnag etc while the current encoding does not work.

I understand that the desire to make it just like js errors, but there are trade offs, it is more fragile in the internal and even if we do that, such encoding is still internal, it is not planned to make it public for FFI.

So what would be helpful is that for stacktrace reporting, any pros/cons between proposal 1 and 2

2 Likes
#6

Hey, what about option 3 that i suggested?

I think that the problem with exceptions in general is that they cross the boundary between ReScript and JS - and specifically for exceptions, not having them 100% aligned to JS is super risky as they’re such a low-level thing.
Same as if ReScript decided to implement Array in a weird way internally and have it “almost” an array. it’s such a risky move IMO.

I think the key here is that, it might work for the tools we test here, and then a month from now a new tool, that uses some obscure Js exception thing will gain popularity(React suspend considering, using exceptions as a mechanism is an example) will not work in a weird way, forcing us to change implementation yet again which is hard and very costly.

Could you please elaborate on the various tradeoffs?

  • Payload not displayed properly - Isnt that something we could tweak and eventually fix?
  • Semantics more deviation from existing encoding -> again, if that’s internal to the compiler, it will be good enough eventually, no?
  • More complexity -> no idea what this means so hard to comment.
#7

Got you.

It feels more of luck proposal 1 works with some tools - it’s relying upon the implementation details, and any tool that did more probing on the error might fall over. Where as 2 is the established way in JS of throwing errors, so if it’s not supported in a tool, it’s squarely the tool’s fault.

But at the same time, maybe there’s a lot of sense in going proposal 1 if it is a much easier change - and then see if and why it fails in some tools. Then if later down the line it turns out there are issues, there’ll be more information.

#8

If this change is not aiming for seamless interop, then I’d think Proposal 2 is too risky, and proposal 1 is more graceful. Extending instances of builtins is generally not a good idea, so I’d rather implement some catch handler in e.g. mocha and rethrow the unwrapped value afterwards.

#9

i strongly disagree. mocha is one thing. but my production system not able to catch exceptions is a different(and real-life) problem.
Can someone explain why option 3, e.g using the JS way of creating custom exceptions is a no-go?

#10

Another downside of Proposal 1: existing tools may rely on anything in the published interface of JavaScript Error: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error

  • message
  • name
  • stack
  • and so on

They may also do err instanceof Error and rely on it being actually an instance of the Error class. IMHO these are all valid ways to check that something is a JavaScript error.

#11

Not to derail too much, did you mean extending builtins or instances of builtins? Extending instances of builtins is really common in JS - especially (or mainly?) for errors and functions. Node has a lot of error creation stuff for attaching things like error codes that works pretty much the same as prop. 2.

2 Likes
#12

I’m a bit surprised to see so much debate about the internal implementation of rescript exceptions, do people here use exceptions so much in rescript? I had the impression the de facto standard was to use the result type, but maybe using exceptions is more common than I thought, what’s your use case for it?

2 Likes
#13

@tsnobip We use them for error logging in our serverless functions. It’s a convenient way to get a stack trace along with the ReScript code location without jumping through too many hoops… But we generally use the result type for error-handling logic. It seems to work well for us.

2 Likes
#14

I agree with @jacobp100 that extending intances of builtins is not as bad as extending prototypes and constructors, and I also like @BlueHotDog’s proposal of using a custom error class. A custom constructor would make it 100% legit, IMHO. Probably more legit than trying to conform to some random implementations (like Mocha, in this case) in an ad hoc way.

Then again, when you frame your question like plain “would you like a feature X?”, users will always say yes, because it seems free for them. If the question was: “do you want Proposal 1 published in a week or Proposal 2 in ~2 month (postponing features Y ans Z)”, or: “Would you rather have Proposal 2 or Feature AA”, maybe we’d answer differently :slight_smile:

4 Likes
#15

@tsnobip we rarely use them… but when they happen… i want them to Just Work and not deal with global capture and mutate to try and make some tool happy, or understanding ReScript representations. I’m very confused by this debate. if rescript can be a nice JS Citizen, than it should, that should be IMO a guiding factor in deciding such features.

If this is an internal representation i just honestly dont understand how making a solution that’s not 100% aligned with the standard is a good idea. i never want to know about the internal representation of ReScript. and i think that’s a very healthy thing to keep in mind.
JS is a crazy world… besides Mocha you’ve an endless list of things relaying on exceptions. they should just work.

2 Likes
#16

Yes, I mean extending both, prototype and instances as well. Just because Node does it, doesn’t mean every single web browser and framework respect that, and you will always have some tool not behaving correctly, no matter how you implement your duck-typed error objects.

I mean I do mostly speak from my pre ES6 experience, because I ran into a lot of footguns with extending objects arbitrarily with my own values, and I am sure I am not alone.

Regarding ES6 class inheritance: Isn’t this feature extremely dependent on the browser / node version? Just looking at the MDN doesn’t give me any good feeling for easy integration in old babel codebases or older browser targets.

EDIT: Also, I am probably fine with whatever is being implemented, I am just throwing out my thoughts, because apparently this is what Bob wanted when opening this topic?

1 Like
#17

My thoughts: I prefer something more like proposal 2, but I can see how the internal metadata could get removed (or maybe even modified) by third party tools.

One example is error normalization, where a library might assume it’s a standard Error instance and copy it with a modified message or stack property. If they aren’t copying all the object keys, then this would dump the ReScript metadata.

Granted, this sort of technique is generally frowned upon in the JS world, but I’ve seen it done in some frameworks & popular libraries.

2 Likes
#18

It looks like option two is the least problematic. I would just like to mention that I would choose a more unique property than _1 for the metadata, maybe RE_EXN_1.

Btw we can also set the message/name etc of the error to sane values (like the same as RE_EXN_ID.

Btw2: A custom error class is probably the best way to go, compliant and in line with how this would be solved in JavaScript.

#19

Since extensible variants and exceptions share the same implementation, how much does that factor into the decision? For example, if exceptions were compiled to a custom error class, would that make all extensible variants that same class? Or, if they did, would that be a problem?

2 Likes
#20

That’s one of underlying complexities and explains why I tend to proposal 1.
I understand the advantages of proposal 2 vs proposal 1, so repeating this is not that helpful.
What I asked is if there are any practical disadvantages (e.g, break some popular 3rd party frameworks) if we go to proposal 1, at best with some concrete examples

3 Likes