Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request: Hook for tolerant error reporting #370

Closed
nzakas opened this issue Jan 7, 2016 · 9 comments
Closed

Request: Hook for tolerant error reporting #370

nzakas opened this issue Jan 7, 2016 · 9 comments

Comments

@nzakas
Copy link
Contributor

nzakas commented Jan 7, 2016

What I'm trying to do

Esprima has a tolerant mode that collects recoverable errors and puts them in an errors array on the AST. I'm trying to mimic that in Espree using Acorn. However, currently there is no distinction between errors of invalid syntax (missing comma) vs. validation errors (strict mode functions can't have duplicate named parameters).

Proposal

I'd like to propose adding unexpectedRecoverable() and raiseRecoverable() (or whatever names make sense) methods to the parser. I'll volunteer to go through and use these in place of unexpected() and raise() for strict mode violations and wherever else parsing doesn't need to terminate. In Acorn, I envision unexpectedRecoverable() and raiseRecoverable() to be set to the same function as unexpected() and raise(), so there would be no functional change to how Acorn works or reports errors. These methods would solely be there to allow developers (me!) to override them and do something else if desired.

Would you be open to such a change?

@RReverser
Copy link
Member

Sounds as a good idea to me. @marijnh ?

@marijnh
Copy link
Member

marijnh commented Jan 7, 2016

Sounds like a useful, low-impact things. I'd just like to see a somewhat stronger definition of which errors count as recoverable. Would you have any guarantees about the shape of the AST produced by a parse that continues on recoverable errors? (For example, when converting an expression that isn't a valid pattern to a pattern when parsing a destructuring assignment, is that recoverable? You can continue parsing, but the AST will be very weird. But then, you could consider any AST that violates a part of the spec as weird.)

@RReverser
Copy link
Member

I guess recoverable errors in this context are those that are so-called "early errors" in the spec which were not actually parsing errors and still reflect valid AST, but are rather extra validations.

@nzakas
Copy link
Contributor Author

nzakas commented Jan 7, 2016

Yes, I'm referring primarily to early errors, and other errors that are basically valid syntax but just not allowed. Things like:

  1. Using a reserved word as an identifier
  2. Strict mode early errors
  3. Using the y or u regular expression flag in ES5 mode

In theory, this would be for any errors that don't necessarily bork parsing any further. I can always start with just a few as examples and we can adjust as we go.

@marijnh
Copy link
Member

marijnh commented Jan 7, 2016

In theory, this would be for any errors that don't necessarily bork parsing any further.

I think we should specify up front whether the AST that comes out of a parse with ignored recoverable errors is something people can work with, or if all bets are off. There's arguments for both approaches, but as long as we pick one and stick with that I'm fine with either.

@nzakas
Copy link
Contributor Author

nzakas commented Jan 7, 2016

Can you define "something people can work with"? In my mind, the AST is perfectly fine, it just hasn't had any sort of inspection, so you can still traverse it.

You can try out what the current version of Espree is doing in tolerant mode here:
http://eslint.org/parser/

@RReverser
Copy link
Member

Offtopic, just FYI as might be a bit more convenient: ASTExplorer currently also uses [email protected], so you can check there as well. @nzakas

@marijnh
Copy link
Member

marijnh commented Jan 12, 2016

Can you define "something people can work with"?

In my experience, AST-processing code often makes assumptions about the AST, such as the lval of an assignment actually being a valid lval, or a function parameter being a valid pattern. Basically, the field types specified by the ESTree spec, such as the type of left in this description:

interface AssignmentExpression <: Expression {
    type: "AssignmentExpression";
    operator: AssignmentOperator;
    left: Pattern | Expression;
    right: Expression;
}

If we're going to output ASTs that violate this, it should be an explicit decision and well documented.

@nzakas
Copy link
Contributor Author

nzakas commented Jan 12, 2016

Ah, gotcha. No, the AST itself would be perfectly valid.

nzakas added a commit to nzakas/acorn that referenced this issue Jan 19, 2016
This allows consumers to override raiseRecoverable() errors if they want.
To start, I only added this in places where value validation failed.

Fixes acornjs#370
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants