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

Added option to suppress parse errors #12

Merged
merged 1 commit into from
Apr 15, 2016
Merged

Added option to suppress parse errors #12

merged 1 commit into from
Apr 15, 2016

Conversation

nevadascout
Copy link
Contributor

I've added a flag to allow you to suppress errors when parsing a PHP file. This means that if a parse error is enountered, the AST generated up to that point will be returned instead of throwing an exception.

Default behaviour is to not suppress errors (how it used to be).

Usage:

var phpParser = require("php-parser");
phpParser.parser.suppressErrors = true;

This also fixes a bug where the line number is not being returned in the exception thrown.

@@ -34,12 +34,13 @@ module.exports = function(engine) {
* The basic parser api
*/
var api = {
// le lexer
Copy link
Member

Choose a reason for hiding this comment

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

french touch :)

@ichiriac ichiriac merged commit 466b8b7 into glayzzle:master Apr 15, 2016
@nevadascout nevadascout deleted the suppress-errors branch April 15, 2016 10:01
@ichiriac
Copy link
Member

Great functionnality, I really appreciate your help !

I've merged it but there is 2 mistakes :

--> The parser is not stopped by the throw - so the parser should be instable and make some inconsistent AST. To fix this you have 2 options :

  • put the current token position at EOF (or let if think thats the end of stream)
  • handle code errors gracefully (like shopify/liquid library for example) - but it's the hard way

Also I would prefer to avoid sending things to console as this is a library. It would be more usefull to store the last error directly a a variable with the message, the line, etc ... and it's up to the developper to check if the was errors during the parsing.

'\nat line ' + this.lexer.yylloc.first_line
);
var errorMessage = 'Parse Error : unexpected ' + token + msgExpect + ' at line ' + this.lexer.yylloc.first_line;
if (suppressErrors) {
Copy link
Contributor Author

@nevadascout nevadascout Apr 15, 2016

Choose a reason for hiding this comment

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

Should be if (this.suppressErrors) {

@nevadascout
Copy link
Contributor Author

nevadascout commented Apr 15, 2016

For my use case of the parser, ideally it should be able to recover from a parse error and continue parsing the rest of the file.

But for now, putting the token at the EOF would be a good compromise. Is this correct?
HvyIndustries@d69c544

For errors, should we have an additional sub-array with a list of the errors thrown while parsing? Something like [[<ast>], ["errors", [<list of errors>]]

I'll be submitting a new pull request to fix the bug that I introduced (forgot to use this. in the if), and I'll include a fix for the EOF as well once I figure out how to do it :)

@ichiriac
Copy link
Member

Already done the changes and commit them (the same as you have done).

I'm interested into your proposal to add a support for gracefull parsing (with another option for example) and put into the ast the error.

The tricky part is to recover a clean state when you will want to continue the parsing, so you will have to eat every bad token... but avoid eating good ones, and leave the function as early as possible ...

It should be a trick for inserting automatically the error node by decorating the each eater function (maybe automatically) and it could be a solution to keep the exception system so catching the error and avoiding the eater to be too greedy.

I'll open an issue for that

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

Successfully merging this pull request may close these issues.

None yet

2 participants