JDK-8218170: Upgrade antlr to version 4.7.2 (port from antlr3 to antlr4, minimum changes)

[brcolow]

As requested in #360 by @kevinrushforth this is a PR that contains the minimum amount of changes to the code necessary to upgrade from antlr3 to antlr4.

One thing to notice is that the tests implementing fireLexerRule is removed because I couldn't find a way to fire a lexer rule in antlr4. I tried many things (like setting the current token, etc.) but none worked so I had to alter the semantics of the tests a bit.

[brcolow]

@parrt Any idea if there is a way to trigger a lexer rule in antlr4? In antlr3 it was, for example:

https://github.com/javafxports/openjdk-jfx/pull/372/files#diff-c3f9b203d77b3f52ce4e6272bbf82492L53

or if there is a better way of doing the same thing (testing lexing of specific text in the context of a lexer rule)?

Thanks!

[parrt]

Unfortunately, it all goes through a state machine now so no way to jump directly there.

[brcolow]

Okay, great. Thank you. @kevinrushforth So I believe that what I've done to change the test semantics is the best possible approach and shouldn't complicate reviewing.

[brcolow]

JBS: https://bugs.openjdk.java.net/browse/JDK-8218764

[kevinrushforth]

@brcolow I had previously filed JDK-8218170, but forgot to make it public (sorry about that). I closed JDK-8218764 as a duplicate and assigned JDK-8218170 to you.

Also, we now have the legal approval to upgrade to antlr 4.7.2.

[kevinrushforth]

I will review it soon, and have asked @arapte to review as well.

[kevinrushforth]

My initial testing looks good. I'll finish reviewing it next week.

@brcolow Can you send an RFR email to openjfx-dev?

[brcolow]

RFR Sent.

[kevinrushforth]

I did a test on all three platforms and all looks good. I can confirm that the generated shader files are identical except for the whitespace diff noted in #360. I also checked the final artifacts. All looks good.

I still need to do a review of the code changes, but at first glance they look fine.

[kevinrushforth]

While getting ready to start the code review, I see four main categories of changes:

1. Changes in the JSL grammar
2. Simple code changes to react to refactoring or other equivalent API
3. Less obvious changes that were needed due to functionality that has changed in antlr4
4. Changes to the tests that are needed for antlr4

In order to help us review and understand the changes, can you provide an overview of the changes for at least the JSL grammar file (one change I see is that the tokens needed to be turned into equivalent grammar rules, but there are several less obvious changes)? Also, if there is anything tricky that would fall in category 3, it would be helpful if you could highlight it.

[brcolow]

Changes to JSL.g:

1.) Renamed extension from .g to new .g4 extension.
2.) Removed backtrack option as it no longer exists in antlr4.
3.) Remove package declarations from header in favor of passing package argument
to antlr4 compiler.
4.) Moved error handling code from the grammar to Java using the new error listener mechanism.
5.) Replace instances like p != null with the equivalent antlr4 statement $p.ctx != null. The former causes antlr4 compilation to fail as the p without a $ is no longer valid inside the statement.
6.) Change old {$channel=HIDDEN;} syntax to new -> channel(HIDDEN) syntax.
7.) Replace old ( options {greedy=false;} : . ) syntax with non-greedy operator ?.
8.) Make the old tokens simple lexer rules as you can no longer assign a value to a token in antlr4. From Stack Overflow "but I believe you can no longer specify the value a token should match in the tokens section. Instead this is only for virtual tokens and everything else must be specified as normal lexer tokens." In doing this they must be moved to the bottom because of rule precedence.

Change to the tests:

As noted by @parrt above (thanks!), It is no longer possible to trigger a lexer rule directly and act from within that state. Therefore we had to change the semantics of the tests to be as close as possible but we have to lex each String as a complete program. Therefore instead of expecting assertRecognized to fail we added a assertNotRecognized method which checks what type of token should be lexed/parsed.

The new error handler mechanism throws a ParseCancellationException because we cannot throw a RecognitionException because it will be swallowed by the error handler mechanism. Using ParseCancellationException was suggested on this Stack Overflow answer.

Changes to String templates:

StringTemplate was renamed ST (along with StringTemplateGroup to STG).

The calls to glue.setAttribute were replaced with the new method name: glue.add.

Rendering the template no longer uses toString but render.

[arapte]

The changes look good to me. Added a minor change comment.

[kevinrushforth]

I plan to do a detailed review of JSL.g4 today (and I'll look at the rest as time permits). One general comment is that we now get the following warning:

warning(131): com/sun/scenario/effect/compiler/JSL.g4:614:19: greedy block ()* contains wildcard; the non-greedy syntax ()*? may be preferred

This suggests that one of the transformed rules might produce unexpected behavior as a result. It doesn't make a difference in the parsing of our existing JSL files, but could make the grammar more fragile. I'll try to look for places that could be a problem when I review the grammar file, but it would be worth your looking at this, too.

[brcolow]

Yes, that warning is issue on this bit:

GLUE_BLOCK
    : LEFT_FRENCH .* RIGHT_FRENCH
;

Given that a GLUE_BLOCK surrounds the entirety of the STG file and >> or << could feasibly be in Java code I think that we want greedy behavior, but I am certainly no regex expert :). If I understand correctly, if we had non-greedy behavior and a >> was inside the glue code (e.g. PrismGlue.stg) then it might match only the first part up to the >> but I could very easily be mistaken about that.

[kevinrushforth]

The << ... >> is also used in several of the Decora .jsl files (e.g., jsl-decora/Blend.jsl), where is isn't surrounding the whole file. As long as we would never want two such blocks in the same file it may be fine as-is.

I can see your point in the .stg file usage. There are embedded shift operators << in at least some of the blocks, so it seems that it needs to be greedy.

[kevinrushforth]

As a follow-on to the warning about using a greedy operator, I note that the rules in question didn't change between antlr3 and antlr4, so the behavior will be the same, we just get a warning now. So this is fine.

[kevinrushforth]

I have no additional comments. This looks good to me.

[arapte]

Looks good to me too.

[kevinrushforth]

I only noticed this after merging it in...

It seems that the JSLC unit tests are not being compiled or run.It looks like they haven't been wired up to the build since the Jigsaw refactoring back in the JDK 9 time frame.

Several of them get compilation errors, so what this means is that we don't have any automated way to test any new changes, which is a bit concerning for any future changes.

@brcolow How did you verify the changes to the tests that you made (or did you do the same thing I did and assume that if "gradle test" passed, then the changes must be good)?