@babel/parser error recovery by nicolo-ribaudo · Pull Request #10363 · babel/babel

nicolo-ribaudo · 2019-08-22T20:02:27Z

Q	A
Fixed Issues?	Fixes #6702, fixes #10316
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
Documentation PR Link	babel/website#2114
Any Dependency Changes?
License	MIT

Note for reviewers: #10363 (comment)

OLD MESSAGE: I'm manually reviewing all the tests with a different error. You can find the script I used to generate this list at #10363 (comment).

Review progress

The following tests have more than one error:

The following tests generated a different error:

The following tests threw a different error:

The following input files had to be changed:

packages/babel-parser/test/fixtures/es2015/destructuring/binding-this/input.js
packages/babel-parser/test/fixtures/es2015/generators/invalid-escape-yield/input.js
packages/babel-parser/test/fixtures/es2015/uncategorised/125/input.js
packages/babel-parser/test/fixtures/es2015/uncategorised/222/input.js
packages/babel-parser/test/fixtures/es2015/uncategorised/223/input.js
packages/babel-parser/test/fixtures/es2015/uncategorised/232/input.js
packages/babel-parser/test/fixtures/es2015/uncategorised/233/input.js
packages/babel-parser/test/fixtures/es2015/uncategorised/234/input.js
packages/babel-parser/test/fixtures/esprima/es2015-destructuring-assignment-object-pattern/invalid-pattern-with-method/input.js
packages/babel-parser/test/fixtures/esprima/expression-primary-array/migrated_0012/input.js
packages/babel-parser/test/fixtures/esprima/invalid-syntax/migrated_0162/input.js
packages/babel-parser/test/fixtures/esprima/invalid-syntax/migrated_0169/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-52/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-53/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-56/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-57/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-58/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-59/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-60/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-61/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-62/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-63/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-64/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-68/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-69/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-71/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-73/input.js
packages/babel-parser/test/fixtures/experimental/numeric-separator/invalid-74/input.js

packages/babel-parser/test/fixtures/core/scope/undecl-export-if/output.json

nicolo-ribaudo · 2019-08-22T21:03:59Z

packages/babel-parser/test/fixtures/core/uncategorised/108/output.json

+    "SyntaxError: Invalid regular expression flag (1:19)",
+    "SyntaxError: Invalid regular expression flag (1:20)",
+    "SyntaxError: Invalid regular expression flag (1:21)",
+    "SyntaxError: Invalid regular expression flag (1:22)"


The input file is var x = /[P QR]/\u0067. It throws one error for each character of \u0067, but I think that it is ok?

nicolo-ribaudo · 2019-08-22T21:19:39Z

Before reviewing this PR: I'm slowly going through all the 2000 test files changed, to check if all of them are correct. I will leave comments for significant tests.

nicolo-ribaudo

Progress: 280/2054

packages/babel-parser/test/fixtures/core/uncategorised/367/output.json

+  },
+  "errors": [
+    "SyntaxError: Invalid left-hand side in assignment expression (1:0)",
+    "SyntaxError: Invalid left-hand side in assignment expression (1:0)"


packages/babel-parser/test/fixtures/core/uncategorised/374/output.json

+  },
+  "errors": [
+    "SyntaxError: Invalid left-hand side in for-in statement (1:5)",
+    "SyntaxError: Invalid left-hand side in for-in statement (1:5)"


nicolo-ribaudo · 2019-08-23T09:57:26Z

packages/babel-parser/test/fixtures/core/uncategorised/446/output.json

+  },
+  "errors": [
+    "SyntaxError: Expecting Unicode escape sequence \\uXXXX (1:1)",
+    "SyntaxError: Expecting Unicode escape sequence \\uXXXX (1:2)"


This is reported twice correctly, because the input file is

\\

nicolo-ribaudo · 2019-08-23T09:59:40Z

packages/babel-parser/test/fixtures/core/uncategorised/453/options.json

 {
-  "throws": "Bad character escape sequence (1:3)"
-}
+  "throws": "Unterminated string constant (1:0)"


This change is ok, because the input file is

"\u

TODO: For tests which throw, also test the error that is generated without the errorRecovery option. For tests which don't throw this isn't needed, since the thrown error will always be the first one in the errors array.

packages/babel-parser/test/fixtures/core/uncategorised/482/output.json

+  },
+  "errors": [
+    "SyntaxError: Binding 'eval' in strict mode (1:41)",
+    "SyntaxError: Binding 'eval' in strict mode (1:41)"


nicolo-ribaudo · 2019-08-23T10:49:12Z

packages/babel-parser/test/fixtures/core/uncategorised/499/output.json

+  },
+  "errors": [
+    "SyntaxError: Octal literal in strict mode (1:35)",
+    "SyntaxError: Octal literal in strict mode (1:35)"


This is duplicated. The input code is

function hello() { 'use strict'; "\1"; }

nicolo-ribaudo · 2019-08-23T10:50:57Z

packages/babel-parser/test/fixtures/core/uncategorised/500/output.json

+    }
+  },
+  "errors": [
+    "SyntaxError: Legacy octal literals are not allowed in strict mode (1:33)",


Duplicated.

function hello() { 'use strict'; 021; }

packages/babel-parser/test/fixtures/core/uncategorised/505/output.json

+  },
+  "errors": [
+    "SyntaxError: Unexpected reserved word 'interface' (1:37)",
+    "SyntaxError: Binding 'interface' in strict mode (1:37)"


packages/babel-parser/test/fixtures/core/uncategorised/513/output.json

+  "errors": [
+    "SyntaxError: Unexpected reserved word 'static' (1:23)",
+    "SyntaxError: Binding 'static' in strict mode (1:23)",
+    "SyntaxError: Binding 'static' in strict mode (1:23)"


nicolo-ribaudo

347

nicolo-ribaudo · 2019-08-23T12:34:00Z

packages/babel-parser/test/fixtures/es2015/destructuring/error-operator-for-default/output.json

+  },
+  "errors": [
+    "SyntaxError: Only '=' operator can be used for specifying default value. (1:3)",
+    "SyntaxError: Invalid left-hand side in array destructuring pattern (1:2)"


This second error should be removed

JLHwung · 2019-08-24T01:05:20Z

Here is a helper script (executed at project root) to output those fixtures whose errors.length > 1, I wish it could help

const glob = require("glob");
const fs = require("fs");
glob("packages/babel-parser/test/fixtures/**/output.json", (er, files) => {
  if (er) {
    console.error(er);
    return;
  }
  files.forEach((file) => {
    fs.readFile(file, (err, data) => {
      const json = JSON.parse(data);
      if (json.errors && json.errors.length > 1) {
        console.log(file);
      }
    })
  })
})

JLHwung · 2019-08-24T01:33:19Z

packages/babel-parser/test/fixtures/es2015/identifiers/invalid-escape-seq-export/output.json

+  },
+  "errors": [
+    "SyntaxError: Escape sequence in keyword export (1:15)",
+    "SyntaxError: Unexpected keyword 'export' (1:4)"


This error should be removed.

Consider change test case to

expor\u{74} default 123;

options.json

{ "sourceType": "module" }

I think that it isn't what this test is for. Since it is in the identifiers folder, I think that here we are testing that export can't be used as an identifier, even if it contains escape sequences.

The identifiers/invalid-escape-seq-if is actually testing invalid escape only:

\u0069\u{66} (true) {}

We may create new test cases invalid-escape-seq alongside identifiers so we can test different error types.

JLHwung · 2019-08-24T01:35:49Z

packages/babel-parser/test/fixtures/es2015/identifiers/invalid-escape-seq-import/output.json

+  },
+  "errors": [
+    "SyntaxError: Escape sequence in keyword import (1:40)",
+    "SyntaxError: Unexpected keyword 'import' (1:4)"


This error should be removed.
Consider change test case to

\u{69}\u{6d}\u{70}\u{6f}\u{72}\u{74} foo from "foo";

options.json

{ "sourceType": "module" }

packages/babel-parser/test/fixtures/es2015/meta-properties/new-invalid-prop/output.json

+  },
+  "errors": [
+    "SyntaxError: The only valid meta property for new is new.target (1:4)",
+    "SyntaxError: new.target can only be used in functions (1:0)"


nicolo-ribaudo · 2019-08-24T11:04:46Z

Thanks for your script! I have modified it so that it reports

output.json files with more than one error
output.json files with one error different from the one which was previously specified in options.json
options.json files whose throws has changed.

Code

const glob = require("glob");
const pfy = require("util").promisify;

const preq = (pkg, method) => pfy(require(pkg)[method]);

const readFile = preq("fs", "readFile");
const exists = preq("fs", "exists");
const exec = preq("child_process", "exec");

const messageUpdates = [
  {
    old: "Invalid or unexpected token",
    cur: "A numeric separator is only allowed between two digits",
  },
];

glob("packages/babel-parser/test/fixtures/**/input.*", async (er, files) => {
  if (er) {
    console.error(er);
    return;
  }

  const result = {
    multiple: [],
    different: [],
    throws: [],
    input: [],
  };

  await Promise.all(
    files.map(async file => {
      const output = file.replace(/input\.[a-z]+$/, "output.json");
      const options = file.replace(/input\.[a-z]+$/, "options.json");
      try {
        if (await exists(output)) {
          const { errors } = JSON.parse(await readFile(output));
          if (!errors || errors.length === 0) return;

          if (errors.length > 1) {
            result.multiple.push(file);
            return;
          }

          const { throws } = JSON.parse(
            (await exec(`git show master:${options}`)).stdout
          );

          const error = errors[0].replace("SyntaxError: ", "");
          const updated = messageUpdates.some(
            ({ old, cur }) => throws.includes(old) && error.includes(cur)
          );

          if (error !== throws && !updated) {
            result.different.push(file);
            return;
          }
        } else {
          const data = await Promise.all([
            readFile(options),
            exec(`git show master:${options}`),
          ]);

          const { throws } = JSON.parse(data[0]);
          const { throws: oldThrows } = JSON.parse(data[1].stdout);

          if (throws !== oldThrows) {
            result.throws.push(file);
            return;
          }
        }

        const data = await Promise.all([
          readFile(file),
          exec(`git show master:${file}`),
        ]);

        if (data[0].toString() !== data[1].stdout.toString()) {
          result.input.push(file);
        }
      } catch (e) {
        console.error(file, e);
        process.exit();
      }
    })
  );

  console.log("**The following tests have more than one error:**");
  console.log(list(result.multiple));
  console.log("\n");
  console.log("**The following tests generated a different error:**");
  console.log(list(result.different));
  console.log("\n");
  console.log("**The following tests threw a different error:**");
  console.log(list(result.throws));
  console.log("\n");
  console.log("**The following input files had to be changed:**");
  console.log(list(result.input));
});

function list(arr) {
  return arr
    .sort()
    .map(x => ` - [ ] \`${x}\``)
    .join("\n");
}

I will track review progress, based on the output of this script, in the PR description.

packages/babel-parser/test/fixtures/es2015/uncategorised/221/output.json

+  },
+  "errors": [
+    "SyntaxError: Invalid left-hand side in assignment expression (1:1)",
+    "SyntaxError: Invalid left-hand side in array destructuring pattern (1:1)"


nicolo-ribaudo · 2019-08-24T11:55:44Z

packages/babel-parser/test/fixtures/es2015/uncategorised/242/output.json

+  },
+  "errors": [
+    "SyntaxError: Assigning to 'eval' in strict mode (1:15)",
+    "SyntaxError: Binding 'eval' in strict mode (1:15)"


This error should be deduplicated.

Input code:

"use strict"; (eval = 10) => 42

It might be hard to do, because we are first parsing eval = 10 as an assignment expression, and then converting it to a paremeter later when we find =>

nicolo-ribaudo · 2019-08-24T19:12:45Z

.../babel-parser/test/fixtures/es2015/yield/parameter-name-arrow-inside-generator-2/output.json

+  },
+  "errors": [
+    "SyntaxError: Yield cannot be used as name inside a generator function (2:3)",
+    "SyntaxError: Binding invalid left-hand side in function paramter list (2:3)"


This should be deduplicated.

Input code:

function* fn() { (yield fn) => {}; }

nicolo-ribaudo · 2019-08-24T20:37:47Z

packages/babel-parser/test/fixtures/experimental/dynamic-import/direct-calls-only/output.json

+  },
+  "errors": [
+    "SyntaxError: Dynamic imports require a parameter: import('a.js') (2:9)",
+    "SyntaxError: The only valid meta property for import is import.meta (2:16)"


I like this double error, since it hints about the two possible interpretations.

Input code:

function failsParse() { return import.then(); }

KFlash · 2019-08-25T03:21:21Z

@nicolo-ribaudo Interesting feature!! 👍 What about the lexer? Code like this
throw this.raise(this.state.pos - 2, "Unterminated comment"); will still throw. It can easily be prevented if you return e.g. a Token.Invalid and save the errors somewhere.

BTW. it should be possible to recover from all errors if you either 1) return to statement level if invalid expression or 2) Insert a dummy AST node.

I started to experiment with this myself and just done with a lexer that recover just fine, so I can give you ideas if you need them :)

nicolo-ribaudo · 2019-08-25T05:25:42Z

Thanks for these insights!

Since this PR is already really big and will be a pain to review, I wanted to only avoid "easy" parser errors for now. I'll make more and more errors recoverable in future iterations.

I started to experiment with this myself and just done with a lexer that recover just fine, so I can give you ideas if you need them :)

If it's open source I'd love to see it!

KFlash · 2019-08-26T14:59:29Z

@nicolo-ribaudo My lexer recovery code can be seen here. Just a WIP for now :)

nicolo-ribaudo · 2019-08-27T07:20:52Z

Thanks! I won't have time to update this PR in the following, but I'll try to take a look at yours.

KFlash · 2019-08-31T05:59:27Z

@nicolo-ribaudo I'm more or less done with my lexer now in case you plan to take a look. It's located here. It recover from all errors as seen in this tests.

babel-bot · 2019-09-01T00:07:45Z

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

babel-bot · 2019-09-01T00:07:46Z

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

nicolo-ribaudo · 2019-11-05T00:43:34Z

✔️ I added the last 5 commits during the review of this PR

JLHwung self-requested a review August 22, 2019 20:28

nicolo-ribaudo commented Aug 22, 2019

View reviewed changes

packages/babel-parser/test/fixtures/core/scope/undecl-export-if/output.json Show resolved Hide resolved

nicolo-ribaudo commented Aug 22, 2019

View reviewed changes

nicolo-ribaudo commented Aug 23, 2019

View reviewed changes

JLHwung mentioned this pull request Aug 23, 2019

Fault-tolerant @bable/parser parsing #10316

Closed

JLHwung reviewed Aug 24, 2019

View reviewed changes

nicolo-ribaudo commented Aug 24, 2019

View reviewed changes

packages/babel-parser/test/fixtures/es2015/uncategorised/221/output.json

},

"errors": [

"SyntaxError: Invalid left-hand side in assignment expression (1:1)",

"SyntaxError: Invalid left-hand side in array destructuring pattern (1:1)"

This comment was marked as resolved.

Sign in to view

nicolo-ribaudo commented Aug 24, 2019

View reviewed changes

nicolo-ribaudo force-pushed the parser-error-recovery branch 2 times, most recently from 6664b40 to fd9ca7c Compare August 24, 2019 13:36

nicolo-ribaudo commented Aug 24, 2019

View reviewed changes

nicolo-ribaudo added pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories labels Aug 24, 2019

KFlash mentioned this pull request Aug 26, 2019

Error recovery meriyah/meriyah#39

Closed

nicolo-ribaudo force-pushed the parser-error-recovery branch 2 times, most recently from 11f1fc9 to d0c330a Compare August 31, 2019 23:58

nicolo-ribaudo added 11 commits November 5, 2019 01:03

Don't report invalid super() twice

86b6275

Remove dup error about reserved param for expr arrows

3b44131

Remove double escapes in migrated tests

3497179

Dedupe errors about invalid escapes in identifiers

cfc1363

Remove duplicated error about decorated constructor

8316a6a

Remove duplicated error about spread in flow class

1eabd28

Don't throw for invalid super usage

050ceb4

Don't fail for object decorators with stage 2

78e4946

Fix flow inexact type errors

d8c0436

Fix flow

a5c1c5c

Fix errors about escapes in keywords (ref: babel#10455)

d9a3f2b

nicolo-ribaudo force-pushed the parser-error-recovery branch from 98fa30a to d9a3f2b Compare November 5, 2019 00:03

nicolo-ribaudo added 5 commits November 5, 2019 01:19

Update after rebase

9fd7069

Fix todo

04e64e6

Remove duplicated error when using += for defaults

c3f64a7

Remove unnecessary throw

7740a45

Nit: use ??

ebc2ce0

nicolo-ribaudo removed the PR: Needs Review A pull request awaiting more approvals label Nov 5, 2019

nicolo-ribaudo merged commit 87feda7 into babel:master Nov 5, 2019

nicolo-ribaudo deleted the parser-error-recovery branch November 5, 2019 09:15

thorn0 mentioned this pull request Nov 5, 2019

Release 1.19 checklist prettier/prettier#6469

Closed

12 tasks

snyk-bot mentioned this pull request Nov 14, 2019

[Snyk] Upgrade @babel/runtime from 7.6.3 to 7.7.1 cds-snc/report-a-cybercrime#936

Merged

This was referenced Nov 28, 2019

[Snyk] Upgrade @babel/runtime from 7.0.0-beta.42 to 7.7.2 ali8889/mist#4

Open

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.7.4 dyna-dot/CyberChef#3

Open

This was referenced Dec 5, 2019

[Snyk] Upgrade @babel/runtime from 7.3.1 to 7.7.4 dyna-dot/bit-javascript#5

Open

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.7.4 jakeherp/todo-list#1

Merged

snyk-bot mentioned this pull request Jan 12, 2020

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.7.7 mboocannon/msgraph-sdk-javascript#1

Open

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 4, 2020

lock bot locked as resolved and limited conversation to collaborators Feb 4, 2020

Uh oh!

Conversation

nicolo-ribaudo commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Aug 22, 2019

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLHwung commented Aug 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLHwung Aug 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

nicolo-ribaudo commented Aug 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KFlash commented Aug 25, 2019

Uh oh!

nicolo-ribaudo commented Aug 25, 2019

Uh oh!

KFlash commented Aug 26, 2019

Uh oh!

nicolo-ribaudo commented Aug 27, 2019

Uh oh!

KFlash commented Aug 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Sep 1, 2019

Uh oh!

babel-bot commented Sep 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo commented Nov 5, 2019

Uh oh!

Reviewers

nicolo-ribaudo commented Aug 22, 2019 •

edited

Loading

JLHwung Aug 24, 2019 •

edited

Loading

nicolo-ribaudo commented Aug 24, 2019 •

edited

Loading

KFlash commented Aug 31, 2019 •

edited

Loading

babel-bot commented Sep 1, 2019 •

edited

Loading