Throw RuntimeException in FluxParser (#421)

[dr0i]

If a RecognitionException occurs in FluxParser it was catched. So callers had no chance to be aware of the Exception. As it's not possible to create a new RecognitionException with a customized error message (and throw this new RecognitionException) and also to be backwards compatibel (RuntimeExceptions are referred to as unchecked exceptions and the code would compile just as before) no checked Exception is thrown but a RuntimeException with the customized error message.

Resolves #421.

[dr0i]

For code reviewers: The Flux.g is used by ANTLR to generate metafacture-flux/build/generated-src/antlr/main/org/metafacture/flux/parser/FluxParser.java.

[blackwinter]

Where is the RecognitionException caught?

I think I've found it. It's in the generated FluxParser source. That's also where you got the error handling that you copied over, right?

[dr0i]

unfamiliarity with ANTLR

Yeah, me too :) (traced the stack, found the crux, browsed the net and found the solution. Quite happy debugging :) )

I think I've found it. It's in the generated FluxParser source. That's also where you got the error handling that you copied over, right?

yep

Re your inline comments:

1. No - if it's not thrown here it's missing in flow where it's thrown to be catchable by calling FluxCompiler.

In the flow section (the other catch block I added) it's not sufficient to just rethrow RecognitionException because:

As it's not possible to create a new RecognitionException with a customized error message [...]

The error message of the RecognitionException uses character codes (e.g. ASCII 59 instead a ;) which makes it harder for end users to understand what's wrong. From ANTLRsRecognitionException:

[...] to generally make things as flexible as possible, these exceptions are not created with strings, but rather the information necessary to generate an error [...]

So_ it's easier for the catcher to produce proper error messages - they let this up to the implementers. Just catching this in flux lacks references to produce these messages so it's computed and added to the RuntimeExcption.

2. Tests ... hmhm. Extra work. I don't fear regression here. But you are right, being bold and ignorant is a weak excuse :| ... will add one.

[dr0i]

@blackwinter added the test

[blackwinter]

Great :) :+1:

I also understand now why the RecognitionException has to be caught and rethrown in flow: Otherwise a default catch handler would be generated that just swallows the exception. It would be sufficient to only install the flow catch handler in order to pass the test, but I've added one to varDef as well (and simplified them) so they both end up in the same place. The only thing left to do would be to add the (minimal) catch handler to all the other expressions...

[blackwinter]

BTW: Would it make sense to throw a MetafactureException instead of a RuntimeException?

[dr0i]

Throws now a FluxParseException (which extends MetafactureException).

The only thing left to do would be to add the (minimal) catch handler to all the other expressions...

... which (just a note here) could also be done by adding a @rulecatch:

@rulecatch {
      catch (RecognitionException e) {
          throw e;
      }
  }

to the grammar Flux.g - but that prevents sometimes more detailed messages (e.g. when doing this with the atom expression). I think this is at least a good start to make many more errors catchable by calling programs (see e.g. https://github.com/metafacture/metafacture-playground/issues/84).

[dr0i]

Merged this. We can always improve later, if necessary.