Monorepo: Suggestion for a better (and any-less) Error Handling

[holgerd77]

One of the most common any typing we still have in the monorepo is for typing of the error parameter in the try / catch blocks like here for an EVM precompile:

try {
  returnData = (opts._EVM as EVM)['_bn254'].mul(input)
} catch (e: any) {
  if (opts._debug !== undefined) {
    opts._debug(`${pName} failed: ${e.message}`)
  }
  return EvmErrorResult(e, opts.gasLimit)
}

This is non-beautiful for test code and problematic for production code, since in JavaScript, the type of e is just not defined and eventually this error code will not work (if an underlying library e.g. just throws a string). Tested with the following code snippets what will actually happen:

TypeScript is handling the JS error situation by just applying type unknown as a type for e, see here (which makes a lot of sense respectively is adequate). I tested:

We might nevertheless also want to set the useUnknownInCatchVariables flag.

So here is a suggestion for how we can improve the overall situation:

Production Code

For production code we remove any eventual any typing and accept the fact that a type for an error is just not known when being caught in JS. Then we add an exception value normalization as proposed here in the Mozilla docs somewhere down the line, like:

try {
  throw "Oops; this is not an Error object";
} catch (e) {
  if (!(e instanceof Error)) {
    e = new Error(e);
  }
  console.error(e.message);
}

This should make our (most of our) production code try/catch clauses more robust and safe to rely on.

Test Code

For test code, to make things easier I would suggest to type with Error here, since we are in a more controlled environment and I would personally find it a bit overblown to add this normalization also there (there are a lot (!) of places for this). This can easily achieved with some targeted (maybe per-library) search-and-replace on the spec.ts files.

Note that error handling is diverse. There might be situations where these "rules" from above cannot be applied 1:1. But this should give a good baseline.

Might be worth to do this on a per-library basis (or 2-3 libraries) to keep things under control since this work might break some test cases along the line.

[holgerd77]

Update: ah, just tested, we are not allowed to type with Error (for the tests e.g.). 🤔

Then we might want to just add this normalization there as well? Then it's just complete and we are done with it?

[ScottyPoi]

Looks like we would have to disable this no-ex-assign rule

[holgerd77]

Looks like we would have to disable this no-ex-assign rule

Ah, thanks for the note, didn't know about this rule. Yes, I would still be in favor, the reasoning (in the rule description) does not convince me respectively I think we are beyond the point that someone does such stupid things and re-uses the error variable for his arithmetics. 😂