babel icon indicating copy to clipboard operation
babel copied to clipboard

More consistent API when using errorRecovery

Open Me1000 opened this issue 2 years ago • 3 comments

Q A
Fixed Issues? Fixes #14175
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? No
License MIT

As described in the original issue #14175, this PR intends to make all errors recoverable when using the errorRecovery option, such that only "legitimate programming mistakes" on our part cause an error to (unintentionally) be thrown from parse. This significantly simplifies usage of error recovery, by having one consistent place to look for errors: the errors property of the returned object. This is in contrast to the way things currently work, where if you turn on errorRecovery, you both have to check the errors property "on success", as well as catching non-recoverable errors "on failure", which itself could be a "parse error" or of course a "logic error" on our part. Often this results in the user having to wrap parse in their own wrapper function to consolidate these results (we for example do this in Babel's own tests). From a type perspective, you can think of this as changing the type of parse from something like:

Either<ParserOutput, SyntaxError | Error>

to just:

Either<ParserOutput, Error>

In order to accomplish this across the board on all errors, the idea is to simply return an empty Program for the AST in the cases that are currently "unrecoverable". The thinking here is that the AST isn't trustworthy when in an errored state anyways, and is at best a "best guess", so an empty Program represents the "best guess" of a (currently) unrecoverable error. As we make more errors fully recoverable, their corresponding Programs will thus also flesh out more. This is analogous to the current state of affairs when adding recoverability to existing unrecoverable errors, and is also the way these "wrapper" functions are usually implemented in userland.

Implementation

Although original goal was to make errorRecovery: true behave in the manner described above, we discovered that some consumers rely on the particularities of the current behavior, so we decided it would make more sense to simply have a new possible value for errorRecovery of "always", and propose that this should be the behavior for true in Babel 8 (at which point we'd return to only allowing false and true). This allows us to, for example, use this behavior in the Babel tests (and we'd actually also use it at RunKit), but not cause any waves for anyone else that is using errorRecovery unless they specifically turn it on.

Going forward/proposal

I believe this new option value should be temporary and should probably just be removed in Babel 8 in favor of true. However given the unknown number of existing projects which might break from this change, I thought it best that the change be made for a major version upgrade, not a minor update.

Specific implementation notes

  1. Sometimes an empty file/program is returned, but in the case of parseExpression there is no such thing as an "empty expression", so what's returned is just an object with errors and comments (because comments is a required field for ParserOutputs).
  2. In a couple places the parser tries a few different passes before failing. In particular it will try to parse certain code as JSX and when it fails will go on to parse it differently. If that second parse pass fails the original JSX parse error gets thrown. In this case I've restored the parse state before the error gets thrown. This ensures that the parse state matches the control flow, and as a result the JSX error is already in the state.errors list.
  3. The test fixtures have been updated because most errors no longer throw (some unicode invalid character errors still throw though, but that's independent of the parser itself). So the fixtures have moved most of those errors from the options file to the output file.
  4. Babel 8 can sometimes recover better than Babel 7. That means Babel 8 sometimes has more errors in the error list than babel 7. Because of this I added a mechanism to the test suite so that we can specify a different output file (if running babel 8 and the outputs are indeed different).

Me1000 avatar Apr 22 '22 19:04 Me1000

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51719/

babel-bot avatar Apr 22 '22 19:04 babel-bot

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51719/

babel-bot avatar Apr 22 '22 19:04 babel-bot

This PR also gets us one step closer to being able to update our testing infrastructure to test errors both with errorRecovery true ("always" in this case) and false to check that they produce the same kind of errors (which would catch cases like this: https://github.com/babel/babel/issues/14146 ).

tolmasky avatar Apr 26 '22 15:04 tolmasky