Report InputMismatchException with original context information

[sharwell]

Fixes #1922

📝 This needs more test cases, particularly for LL(k) failures where k>1. However, my current understanding is these cases have been impacted by this bug for much longer (since 4.2.2) than the issue being addressed for the k=1 case.

[sharwell]

@Et999 @michaelpj @PayneSun @marcospassos It should be possible to apply this change to your code by extending DefaultErrorStrategy and overriding sync and reportInputMismatch. Can one of you try this and let me know if it resolves the issue in your test suites? Since the InputMismatchException constructor used in this code is not available, you'll have to do a bit of trickery to make this work right:

int priorState = recognizer.getState();
ParserRuleContext priorContext = recognizer.getContext();
try {
  recognizer.setState(nextTokensState);
  recognizer._ctx = nextTokensContext;
  throw new InputMismatchException(recognizer);
} finally {
  recognizer.setState(priorState);
  recognizer._ctx = priorContext;
}

⚠️ Note that the message does still change in some cases relative to 4.6 - you get "mismatched input" instead of "extraneous token". However, the expected set should be reporting good information again.

[michaelpj]

Thanks enormously for looking into this! I'll give this a try and see how it looks.

[michaelpj]

Built locally, tested. Fixes the regressions in 4.7, and actually improves the errors in several other cases. Big 👍 from me.

[parrt]

@Et999 and @marcospassos can you build locally and confirm this works for you?

[sharwell]

@parrt After confirmation, I'll start the work to implement this for other targets.

[marcospassos]

I'll test it by tomorrow :)

[parrt]

@antlr/antlr-targets please note Sam's improvements to the error reporting in this PR.

[parrt]

ugh. we appear to be getting random travis fluctuations

[marcospassos]

@sharwell we caught 160+ tests failing due to the BC break introduced in 4.7, even applying the patch you provided.

The errors can be grouped into the following cases:

• No viable alternative at <EOF> is now Mismatched input <EOF> (no big deal)
• Missing token ) is now Mismatched input <EOF> (worst error reporting)
• Mismatched input <EOF> is now Extraneous token <EOF> (really weird)

Besides this, we found several cases where error recovery got worst. Given the following grammar and invalid input, notice how the error recovery produces worst guesses:

root: variableDeclarationList? expression;

variableDeclarationList
    : variableDeclaration + ;

variableDeclaration
    : 'let' variableDeclarator (',' variableDeclarator)* ';' ;

variableDeclarator
    : identifier '=' expression ;

expression: ...;

Invalid input:

let a = , b = 2; true

In 4.6, the previously input was recognized as:

let a = b; true

While in 4.7 the same input is recognized as:

let a = b;  2

This difference is probably related to the same issue, right?

[sharwell]

@marcospassos If you are testing error messages, I would not expect this change to reduce the number of "regressing tests", since the messages will still change. Is your project open source so I can help triage the differences?

Also, before we go too far we should make sure the change was properly applied since the result discrepancy between @michaelpj and @marcospassos is so large.

💭 I notice that your examples all mention EOF. Do all of your failing tests involve EOF handling?

[marcospassos]

@sharwell the project is not open source. However, we're not testing only error messages, but also the generated parse tree. Why don't you consider the error message a BC break? This fix not only changed the error messages but also made the parser produce different parse trees, which is a serious BC break.

Btw, is "Extraneous token <EOF>" supposed to be a valid error message?

[sharwell]

@marcospassos I sent you an email 👍

[sharwell]

After talking with @marcospassos, it appears the negative outcomes from this experiment were likely influenced by uncommon characteristics of the language being parsed. In particular:

1. Input text is typically short, so error handling strategies which favor error locality to error recovery are favorable
2. Input is closer to natural language than most, so distinctive tokens that tend to yield good error recovery in many languages (e.g. the braces in Java) don't help for this one

For this case, I suggested creating a type derived from DefaultErrorStrategy using the implementation of sync prior to my change, since it yields better results for this language.

[marcospassos]

Thank you @sharwell!

[parrt]

Ok, sounds like we have no objections to this improvement.

[parrt]

Thanks, @sharwell !!