Skip to content

Commit

Permalink
Tolerate recoverable parse errors.
Browse files Browse the repository at this point in the history
@jdalton Looks like acorn@5.0.0 got a bit more strict about duplicate
variable declarations and import specifiers.
  • Loading branch information
benjamn committed Mar 28, 2017
1 parent aefa703 commit 19add56
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 0 deletions.
8 changes: 8 additions & 0 deletions lib/parsers/acorn.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,16 @@ exports.options = {

function acornParse(code) {
var parser = new acorn.Parser(exports.options, code);

// It's not Reify's job to enforce strictness.
parser.strict = false;

// Tolerate recoverable parse errors.
parser.raiseRecoverable = noopRaiseRecoverable;

return parser.parse();
}

function noopRaiseRecoverable() {}

exports.parse = acornParse;
10 changes: 10 additions & 0 deletions lib/parsers/top-level.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,19 @@ function quickParseBlock() {

function topLevelParse(code) {
var parser = new acorn.Parser(exports.options, code);

// Override the Parser's parseBlock method.
parser.parseBlock = quickParseBlock;

// It's not Reify's job to enforce strictness.
parser.strict = false;

// Tolerate recoverable parse errors.
parser.raiseRecoverable = noopRaiseRecoverable;

return parser.parse();
}

function noopRaiseRecoverable() {}

exports.parse = topLevelParse;

5 comments on commit 19add56

@jdalton
Copy link
Contributor

@jdalton jdalton commented on 19add56 Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like acorn@5.0.0 got a bit more strict about duplicate
variable declarations and import specifiers.

Aren't duplicate let/const and import specifiers invalid JS?
Does reify rely on that or some part of nested imports?
I'm unclear on how its affecting reify and its tests.

@benjamn
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the switch issue you mentioned the other day:

reify/test/export-tests.js

Lines 250 to 267 in 2254a6b

it("should support switch-case nested imports", function () {
assert.strictEqual(typeof x, "undefined");
for (var i = 0; i < 2; ++i) {
switch (i) {
case 0:
import { a as x } from "./abc";
break;
case 1:
import { b as x } from "./abc";
break;
}
assert.strictEqual(x, i ? "b": "a");
}
assert.strictEqual(x, "b");
});

Just to reiterate, I really believe Reify should not implicitly enforce any form of unnecessary strictness.

@jdalton
Copy link
Contributor

@jdalton jdalton commented on 19add56 Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to reiterate, I really believe Reify should not implicitly enforce any form of unnecessary strictness.

Yep, I agree. Enforcing is what the runtime does. Does acorn do its validation as it's generating its ast? Are there other gotchas? Maybe the acorn/dist/acorn_loose.js and parse_dammit is a safer way to go.

For example I do dumb stuff all the time while deving. Would a syntax error on my part throw first in reify or the runtime (I'm hoping runtime).

@benjamn
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It happens in the middle of parsing, looks like: https://travis-ci.org/benjamn/reify/jobs/215909574

An unrecoverable syntax error would probably keep Reify from parsing the AST. Perhaps we should think about whether Reify should just swallow parse errors and leave the code untouched. There's a chance the code would still work in Node, and Node should probably be the one to throw the error that the developer sees.

@jdalton
Copy link
Contributor

@jdalton jdalton commented on 19add56 Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eeee

I'd be curious to see what parse_dammit does too. I'd want to avoid leaving import/export around as much as possible as that will surely be the first fail masking others (imports are usually at the top). So maybe a hybrid of loose parsing and shallowing parse errors.

Please sign in to comment.