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

Recover from errors at CU & Statement level #952

Merged
merged 7 commits into from Jun 11, 2017

Conversation

Projects
None yet
4 participants
@matozoid
Member

matozoid commented Jun 3, 2017

See #589 and #581.

Here's the implementation of the lamest version of recovering from parse errors: handle the exception at the very top - the compilation unit. It recovers from any error by moving forward to the end of the file. The CompilationUnit it produces has defaults for all fields, and a new isBad flag is set.

For discussion:

  • currently I'm just setting a flag, but I actually know which error happened to get this flag set. I think I should store the error (the Problem) right here in the tree. That would be a beautiful thing, but would also cause another List to appear in every Node. It would be beautiful because you can directly ask a node for its problems, instead of the situation we have now: a list of problems with no clear link to the AST. (Another solution would be to link every Problem to an Optional<Node>... Hmmm, that seems nice too...)
  • currently the isBad flag is only on CompilationUnit. When I keep going this way, the flag will be on Statement and uh, one other node type. I think it would be a good idea to simply move it to Node, since all nodes can theoretically be recovered.

New thoughts:

  • I'll put the flag at Node level so you can traverse the AST and look for the bad nodes.
  • I've added #955 to make the parse problems point to the nodes that can't parse.
  • Recoverting Statement is different from recovering CompilationUnit since Statement is not a leaf node - it is abstract. And I don't know which child of Statement to instantiate because there was a parse error. So there I am making a new node type: UnparsableStatement which always has the unparsable flag set.

More thoughts:

  • recovering a statement by skipping to ";" is easy. Recovering a statement by skipping to "}" is harder, since this token does not belong to the statement. This PR only does the easy thing.

@matozoid matozoid requested review from ftomassetti and SmiddyPence Jun 3, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 3, 2017

Coverage Status

Coverage increased (+0.03%) to 67.002% when pulling 155768b on matozoid:issue_589_recover_cu into 9991a25 on javaparser:master.

coveralls commented Jun 3, 2017

Coverage Status

Coverage increased (+0.03%) to 67.002% when pulling 155768b on matozoid:issue_589_recover_cu into 9991a25 on javaparser:master.

@ftomassetti

It seems in order to me, I just added a few comments. However I confess I am out of my depth here.

Maybe we could add a few more tests to better illustrate the feature

Show outdated Hide outdated ...parser-core/src/main/java/com/github/javaparser/ast/CompilationUnit.java Outdated
Show outdated Hide outdated ...parser-core/src/main/java/com/github/javaparser/ast/CompilationUnit.java Outdated
Show outdated Hide outdated pom.xml Outdated
int maxExpectedTokenSequenceLength = 0;
TreeSet<String> sortedOptions = new TreeSet<>();
for (int i = 0; i < exception.expectedTokenSequences.length; i++) {
if (maxExpectedTokenSequenceLength < exception.expectedTokenSequences[i].length) {

This comment has been minimized.

@ftomassetti

ftomassetti Jun 4, 2017

Member

maybe you could use Math.min here

@ftomassetti

ftomassetti Jun 4, 2017

Member

maybe you could use Math.min here

This comment has been minimized.

@matozoid

matozoid Jun 4, 2017

Member

I could, but should I? This is copied from the JavaCC source. Maybe it should be an issue on their project.

@matozoid

matozoid Jun 4, 2017

Member

I could, but should I? This is copied from the JavaCC source. Maybe it should be an issue on their project.

if (maxExpectedTokenSequenceLength < exception.expectedTokenSequences[i].length) {
maxExpectedTokenSequenceLength = exception.expectedTokenSequences[i].length;
}
for (int j = 0; j < exception.expectedTokenSequences[i].length; j++) {

This comment has been minimized.

@ftomassetti

ftomassetti Jun 4, 2017

Member

Could you use a for each here?

@ftomassetti

ftomassetti Jun 4, 2017

Member

Could you use a for each here?

This comment has been minimized.

@matozoid

matozoid Jun 4, 2017

Member

"I could, but should I? This is copied from the JavaCC source. Maybe it should be an issue on their project."

@matozoid

matozoid Jun 4, 2017

Member

"I could, but should I? This is copied from the JavaCC source. Maybe it should be an issue on their project."

sb.append("");
Token token = exception.currentToken.next;

This comment has been minimized.

@ftomassetti

ftomassetti Jun 4, 2017

Member

Maybe you could add some comment here because it is starting to be a long method

@ftomassetti

ftomassetti Jun 4, 2017

Member

Maybe you could add some comment here because it is starting to be a long method

This comment has been minimized.

@matozoid

matozoid Jun 4, 2017

Member

"I could, but should I? This is copied from the JavaCC source. Maybe it should be an issue on their project."

@matozoid

matozoid Jun 4, 2017

Member

"I could, but should I? This is copied from the JavaCC source. Maybe it should be an issue on their project."

@matozoid matozoid requested a review from jeandersonbc Jun 4, 2017

matozoid added some commits Jun 5, 2017

Merge remote-tracking branch 'javaparser/master' into issue_589_recov…
…er_cu

# Conflicts:
#	javaparser-core/src/main/javacc/java.jj
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 5, 2017

Coverage Status

Coverage increased (+0.03%) to 67.035% when pulling da056ca on matozoid:issue_589_recover_cu into 1aad07e on javaparser:master.

coveralls commented Jun 5, 2017

Coverage Status

Coverage increased (+0.03%) to 67.035% when pulling da056ca on matozoid:issue_589_recover_cu into 1aad07e on javaparser:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 5, 2017

Coverage Status

Coverage increased (+0.03%) to 67.035% when pulling da056ca on matozoid:issue_589_recover_cu into 1aad07e on javaparser:master.

coveralls commented Jun 5, 2017

Coverage Status

Coverage increased (+0.03%) to 67.035% when pulling da056ca on matozoid:issue_589_recover_cu into 1aad07e on javaparser:master.

@matozoid

This comment has been minimized.

Show comment
Hide comment
@matozoid

matozoid Jun 5, 2017

Member

@martinlippert - if you're interested in influencing the API, here's your chance :)

Member

matozoid commented Jun 5, 2017

@martinlippert - if you're interested in influencing the API, here's your chance :)

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 6, 2017

Coverage Status

Coverage increased (+0.06%) to 67.058% when pulling 6e27cb0 on matozoid:issue_589_recover_cu into 1aad07e on javaparser:master.

coveralls commented Jun 6, 2017

Coverage Status

Coverage increased (+0.06%) to 67.058% when pulling 6e27cb0 on matozoid:issue_589_recover_cu into 1aad07e on javaparser:master.

@matozoid matozoid changed the title from [DRAFT] Recover from errors at CU level to Recover from errors at CU level Jun 6, 2017

@matozoid matozoid changed the title from Recover from errors at CU level to Recover from errors at CU & Statement level Jun 6, 2017

@matozoid matozoid added this to the next release milestone Jun 11, 2017

@matozoid matozoid merged commit 0783c0b into javaparser:master Jun 11, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 67.058%
Details

@matozoid matozoid deleted the matozoid:issue_589_recover_cu branch Jun 11, 2017

@martinlippert

This comment has been minimized.

Show comment
Hide comment
@martinlippert

martinlippert Jun 13, 2017

@matozoid thanks for the update, much appreciated. Great to see the progress in this area, really great. Unfortunately I am heads down in other work for our upcoming release at the moment, so don't have a chance to take a look right away. But will take a closer look again once I am back to work on this area.

martinlippert commented Jun 13, 2017

@matozoid thanks for the update, much appreciated. Great to see the progress in this area, really great. Unfortunately I am heads down in other work for our upcoming release at the moment, so don't have a chance to take a look right away. But will take a closer look again once I am back to work on this area.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment