Improve exception handling

[jgremmen]

Even though the error handling in Antlr is sufficiently customizable (eg. ANTLRErrorListener), exception handling could be improved substantially.

If an enter/exit rule ParseTreeListener method throws an exception it propagates its way back up to the initial parser method representing the top rule. Along the way, every rule implementation has a finally block in which the exitRule method is invoked and because now the contexts are not appropriately initialized a NullPointerException is most likely to occur and the initially thrown exception is never to be found again.

The same thing happens when an error listener method (eg. syntaxError) throws an exception.

The current solution I've used so far, is setting a boolean cancelParsing, overriding the exitRule method and only calling the super method when cancelParsing = false. This way the exit rules are not invoked anymore and the originally thrown exception propagates as expected.

Even though this works I'm not quite happy with the hack I have to implement for each parser project. If there's a cleaner way to solve this problem, I'm happy to learn more about it.

With respect to error handling, I usually have the following requirements:

• a clean way to report an error in a rule. Maybe it's a lack of knowledge but my default solution is to throw a RecognitionException. This works for enter/exit rule methods which are handled nicely by the error listener except for the exit rule method in the top level rule
• create a visual representation of the reported error and wrap it in an exception to be thrown. As stated above, this exception is most likely to be replaced by a NullPointerException

[mikecargal]

I ran into exactly this situation yesterday. Putting together a tutorial using a Listener during the parse phase. I detected an error in a ternary expression, and threw a runtime exception. Since I had already popped items off of my stack, when it came back through, I got a, much more obscure, error from a stack underflow that should have been an indication of some fundamental error in implementing the interpreter.

Would really like to see a way to throw an exception that bails out of the parse entirely. Recalling a listener method multiple times "on the way out" is just highly unlikely to produce desired behavior.

Also, it appears that the original exception is lost (which, of course, is highly undesirable)

[jgremmen]

I solved this problem for now by subclassing the generated parser and overriding exitRule as follows:

@Override
public void exitRule()
{
  // fix ANTLR bug
  if (!getErrorHandler().inErrorRecoveryMode(this))
    super.exitRule();
}

This way the exit listener methods aren't invoked on the way out.
Yet, I hope this problem to be fixed consistently in antlr4 in the near future.

[mikecargal]

Are you doing a bit more than this perhaps? When I tried this, the error handler is not in error recovery mode (at least not as a simple result of me throwing an exception).

(Perhaps, you're also calling notifyErrorListeners() ??)

[jgremmen]

Yes, you're right. I'm not throwing any exceptions from the rule methods. Here's my approach:

• I have a customized error listener which overrides the syntaxError method and produces a nice error message. At the end of this method I throw a dedicated runtime exception
• exitRule is overridden as stated in my previous post
• from the listener rule methods I do not throw an exception but (as you expected already) invoke notifyErrorListeners on the parser instance and provide a RecognitionException with the context data from the rule method

This will guarantee that the parser is put in error recovery mode and will handle your exception using the configured error listeners. The syntaxError method can then throw any runtime exception and the overridden exitRule method will not swallow the exception as it propagates to the initial parser rule method.

In my opinion this is basic error handling functionality that should be part of antlr4. The fact that this doesn't seem to be a major issue makes me think that there's a better way of doing this but after analyzing the code for days I'm convinced these adaptations are required in order to skip parsing immediately and exit with an exception.

[jgremmen]

@parrt I was asked to put "issues" in the discussion area- so I did. This has been there for about a year now yet there has been no response from the Antlr maintainers so far.

[parrt]

Come to think of it that test for error recovery mode is not the issue.

First, I would recommend that you don't do anything complicated in these listeners because you are mixing exceptions from your semantic code with the exceptions used by the parser syntax err handling. That said, upon exception it should bail out of the parse, but at the moment it continues to execute listener methods because of the finally.

Second, this is all complicated by the fact that we need to do something for every target and not every target has exceptions; e.g., Go lang. In java, I guess what we would do is create a special exceptionOr actually just reuse the canceled parse exception. the problem is that the finally blocks are still going to be executed and there's no way to determine whether we've thrown an exception is there? at the very least this is going to be challenging as all targets will do this differently.

As I debugged this, I can see be that the parser contact is all screwed up and so the exit methods called multiple times because I have not bumpedThe context pointer to its parent because an exception was thrown and therefore that code got avoided.

Oh! I think I understand how to handle this but of course it will be per target. the good news is it doesn't require changes to the code generation templates.I can modify the function that triggers the exit events to trap an exception:

	protected void triggerExitRuleEvent() {
		if ( listenerExceptionOccurred ) return;
		try {
			// reverse order walk of listeners
			for (int i = _parseListeners.size() - 1; i >= 0; i--) {
				ParseTreeListener listener = _parseListeners.get(i);
				_ctx.exitRule(listener);
				listener.exitEveryRule(_ctx);
			}
		}
		catch (Throwable e) {
			// If an exception is thrown in the user's listener code, we need to bail out
			// completely out of the parser, without executing anymore user code. We
			// must also stop the parse otherwise other listener actions will attempt to execute
			// almost certainly with invalid results. So, record the fact an exception occurred
			listenerExceptionOccurred = true;
			throw e;
		}
	}

Let me work on a few more tests to verify this works.

[jgremmen]

I normally don't do complicated things in the listeners but every now and then there is some sort of construct where it is easier to handle this in the listener than to over complicate the grammar in order to get the desired result. And even then, it's not always possible to solve it through grammar changes.

So let's say I have a rule that matches one of 3 types (let's say: "bool", "number", "string"). Then there are various expressions but in a rare case "bool" is not allowed as a type for that expression. In the listener method I would like to report an error by providing the context and optionally an offending token (that's what I did with the 2 reportError methods I mentioned in a previous post). Sometimes it is sufficient to notify the listener and let the parsing continue/recover. But mostly there is a need to reject the input altogether. Providing a cancelParsing method on the context would do the trick for me. I can catch exceptions I produce in the listener methods myself and handle them appropriately.

As I come to think of it, this is actually a more generic way of doing this which works for all other targets as well.

Each parser rule method returns a context object. I have no problem checking this object in order to find out how the parsing was completed/aborted.

[parrt]

Well I think the traditional answer is probably better here: don't enforce semantics during the parse. Just collect syntactically valid stuff and then have a semantics check.

[parrt]

@mikecargal Will the mechanism I described work for you where I trap exceptions, record the fact that an exception occurred, then re throw?Here's an example I'm using:

		class ErrorListener extends CalcNoLRBaseListener {
			public boolean execExitS = false;
			public boolean execExitAtom = false;

			@Override
			public void exitS(CalcNoLRParser.SContext ctx) {
				execExitS = true;
			}

			@Override
			public void exitAtom(CalcNoLRParser.AtomContext ctx) {
				execExitAtom = true;
				throw new NullPointerException("bail out");
			}
		}

Notice the stack trace and the appropriate null pointer exception coming out:

java.lang.NullPointerException: bail out

	at org.antlr.v4.test.runtime.java.api.TestParseListener$2ErrorListener.exitAtom(TestParseListener.java:102)
	at org.antlr.v4.test.runtime.java.api.CalcNoLRParser$AtomContext.exitRule(CalcNoLRParser.java:311)
	at org.antlr.v4.runtime.Parser.triggerExitRuleEvent(Parser.java:412)
	at org.antlr.v4.runtime.Parser.exitRule(Parser.java:654)
	at org.antlr.v4.test.runtime.java.api.CalcNoLRParser.atom(CalcNoLRParser.java:336)
	at org.antlr.v4.test.runtime.java.api.CalcNoLRParser.add(CalcNoLRParser.java:261)
	at org.antlr.v4.test.runtime.java.api.CalcNoLRParser.expr(CalcNoLRParser.java:181)
	at org.antlr.v4.test.runtime.java.api.CalcNoLRParser.s(CalcNoLRParser.java:123)

[mikecargal]

(Sorry for not wading in earlier... I saw the exchange going by, but just now found the time to sit down and really read them)

The short answer is "yes", I think this addresses my concern that exceptions in a ParseTreeListener were being swallowed.

The slightly longer answer is that, I was working on an ANTLR introduction presentation, and in trying to look into "all the things", I came across the addParseListener(ParseTreeListener) method of the Parser class and wondered what it might be used for. It occurred to me that, maybe I could hook into the Parse and implement some sort of REPL. At this point, you're probably, knowingly, laughing at the naïveté of thinking that work work. In hindsight it was horribly misguided, but a "learning experience". I did work out a REPL, but NOT using a ParseTreeListener.

I agree, wholeheartedly, that semantic concerns need to wait until a parse tree is completed, and have reiterated that many times in answering StackOverflow questions.

That pretty much leaves the question of what ParseTreeListeners attached to the Parser are good for. I see that Parser.TraceListener uses it to get a trace of the parse that can be helpful in understanding/debugging. At this point, I don't know that I've come across another use for attaching them to a parser (for many of the reasons elaborated in this discussion).

That said, IF you find a reason to attach one to a parser, and have reason to throw an exception, I think it's a "good thing" to rethrow that exception in the finally block, so that you can either let it bubble all the way to the top to abort the parse, or perhaps to catch your particular exception somewhere and handle it (I think this still puts the parser into an "I'm outa here" state).

I also think this puts the "solution" in the realm of how to proper handle Java Exceptions in the Java target without much, if any, implications for other language targets.

[parrt]

Hi @mikecargal ! Yes,The parse listener (just realized that I ANTLR reused the same ParseTreeListener interface during the parse; sorry for the confusion) is definitely not meant be real work; it's meant for tracing and other sorts of things. I think I will simply update the documentation to include the information here, because with the override of that method, all of this will work it you want.

[parrt]

Added this (in dev branch): https://github.com/antlr/antlr4/blob/dev/doc/listeners.md

[mikecargal]

Ummm... Couldn't quite parse this sentence "The exception will properly or the parser to bailout and the exception will not be thrown out:" :)

[jgremmen]

I can live with this solution.

The latest posts helped me understand the difference between listening during parsing and listening using the ParseTreeWalker. I never used the walker before because parsing is expensive enough already and doing "my stuff" as soon as the rule is matched seemed like the most obvious choice. Except for error handling - which led to this post.

So using the walker, the exception handling suddenly becomes very comfortable. I will change my projects to use the walker.

[parrt]

Ummm... Couldn't quite parse this sentence "The exception will properly or the parser to bailout and the exception will not be thrown out:" :)

haha. voice recognition error.