FWD » Core Development » Conversion Tools

We must replace our AnnotatedAst class with a new implementation that does not derive from antlr.CommonAST. In ANTLR 4, there is no AST implementation.

We will want to keep compatibility to a large degree, but for the API that comes from CommonAST, we want that to be largely thrown away and replaced with an API that better meets our needs.

Some things that are nasty about the existing AST implementation (and which we should handle better):

• Instantiation is handled by ANTLR via a factory, this makes is painful to do things like assignment of unique IDs (per AST node). See brainwash() to get an idea of some of the existing mess that can go away.
• The management of the tree structure (parent/child relationships) in CommonAST is designed with a one-way traversal in mind (e.g. a vistor pattern). It is NOT a good design for random relative node access, which is a common need for TRPL. For example, if you want to reference the the 3rd child, you have to go to the first child then next and next. I think we could have a much faster implementation.

Ultimately, we may edit the AnnotatedAst and Aast when we shift to ANTLR 4. But for now, let's build a new class for our early code.

Greg, let me summarize to make sure I’ve got the idea right, please let me know if this matches your vision:

• In ANTLR 4 there’s no CommonAST. The parser only produces a ParseTree, so if we want an AST-like structure, we’ll need to build and maintain our own.

• Our current AnnotatedAst extends antlr.CommonAST from ANTLR 2, which ties us to the old model and brings along several issues

• The goal is to implement a new AnnotatedAst that’s completely independent of ANTLR 2 and optimized for TRPL’s needs, using a modern, bidirectional tree structure (List of children, parent reference, O(1) child access).

• Each node will have its own unique ID (no global registry, so we can remove the whole brainwash() logic)

• The API should remain largely compatible at a higher level, but we can drop or redesign the old CommonAST-specific methods to better fit the new model.

• Longer term, this will serve as the foundation for migrating to ANTLR 4 while simplifying node creation and traversal.

Does this align with what you had in mind for the new AST implementation?

In ANTLR 4 there’s no CommonAST. The parser only produces a ParseTree, so if we want an AST-like structure, we’ll need to build and maintain our own.

Yes

Our current AnnotatedAst extends antlr.CommonAST from ANTLR 2, which ties us to the old model and brings along several issues

Yes

The goal is to implement a new AnnotatedAst that’s completely independent of ANTLR 2 and optimized for TRPL’s needs, using a modern, bidirectional tree structure (List of children, parent reference, O(1) child access).

Yes

Each node will have its own unique ID (no global registry, so we can remove the whole brainwash() logic)

Yes, we still have the unique ID but it does need to be globally unique. We want to remove brainwash(). The reason we had it was due to the fact that the ANTLR 2 factory support did not give us full control over the instantiation of new nodes, so we needed our own hook. With the new approach, I hope we can achieve the globally unique ID during construction of the instances.

The API should remain largely compatible at a higher level, but we can drop or redesign the old CommonAST-specific methods to better fit the new model.

Yes

Longer term, this will serve as the foundation for migrating to ANTLR 4 while simplifying node creation and traversal.

Yes

Greg, I’m still a bit unclear on how to properly test these changes. I created a small separate project with a simple grammar and used a base visitor to graft nodes, but I’m not sure how this translates to the Progress grammar for example, since the grafting logic there depends much more on the grammar’s transformation rules.

There are still gaps to fill in the implementation, but my plan is to proceed method by method, testing each piece individually before running broader. I think this incremental approach will keep things more manageable and easier to validate.

I’m not sure how this translates to the Progress grammar for example, since the grafting logic there depends much more on the grammar’s transformation rules.

Tomorrow during the status meeting, we'll review the tree building mechanisms used in the existing ANTLR v2 grammars. Then we can discuss approaches.

I intensely dislike the listener approach provided in ANTLR v4. It is overcomplicated, provides no flexibility in the walk and explodes type-specific methods for each rule being processed. We certainly won't use that.

I'm don't especially like their visitor implementation but we can consider it. My intuition is that we should put the tree building logic into the matching rules (avoiding a separate parse tree walk). Separating the tree building from the recognition is not a good idea in my experience.

Anyway, we'll discuss it more tomorrow.

I think creating a table will help clarify things for both of us. At this point, I’ve gone through the ascent/descent and up/down path methods, and many of them are very similar, some are even identical. I’ll prepare a post with a table that shows which methods were fully rewritten, partially updated, or kept as-is, or even dropped.

Also, for the helper methods, I think that introducing a dedicated interface might make the separation of responsibilities cleaner and easier to manage.

My suggestion is to wait until the end of the day, so I can finish covering most of the methods, prepare the comparison table, attach the updated ZIP, and add the necessary comments. This way, it will be much easier for you to perform an early review.

Paula Păstrăguș wrote:

I think creating a table will help clarify things for both of us. At this point, I’ve gone through the ascent/descent and up/down path methods, and many of them are very similar, some are even identical. I’ll prepare a post with a table that shows which methods were fully rewritten, partially updated, or kept as-is, or even dropped.

I like the idea. Please put all of this into AST Design and Implementation.

Also, for the helper methods, I think that introducing a dedicated interface might make the separation of responsibilities cleaner and easier to manage.

Agreed.

Paula Păstrăguș wrote:

My suggestion is to wait until the end of the day, so I can finish covering most of the methods, prepare the comparison table, attach the updated ZIP, and add the necessary comments. This way, it will be much easier for you to perform an early review.

I'm fine with the EOD. But instead of a zip, please branch from 10190a and add your changes there. The process:

1. Create branch 10804a on devsrv01, using the normal process. That branch will be based on trunk.
2. Checkout a local copy of 10190a but call it 10804a. Use bzr unbind in that new branch.
3. Make your changes to the local 10804a.
4. Use bzr push --overwrite ~/secure/code/p2j_repo/p2j/active/10804a to replace the devsrv01 version with your local version of 10804a.
5. Use bzr bind ~/secure/code/p2j_repo/p2j/active/10804a to shift back into "checkout" mode.

All subsequent development can be local and when you commit, I will be able to see the changes immediately. It is a lot easier than messing with zip files.

Yes, you’re right! I’ll take care of it. Really appreciate the detailed guidance.

Greg, I updated the wiki and pushed the new interface and implementation to 10804a. You should now be able to access the updated files directly under com/goldencode/ast.
I think 80% of work is done, only the Tree Local and shadow nodes related methods are missing (I think max 20 methods)

Greg, I've pushed a new version of 10804a. You should now be able to see the new classes. I've fully separated the ANTLR v2 logic from the v4 implementation, which also makes testing much easier on my side. The implementation is more complete now, the only remaining item is the RuleTracing.isOn check (+ others). As mentioned in the TODO, it should no longer depend on the com.goldencode.p2j package.

I also added the support interface. It's empty for the moment, but it will be filled with several helper methods. I'm not fully convinced that merging the versions behind a single interface is the right direction, adding something like a BaseAst would be possible, but things get messy quickly with DumpTree, shadow nodes, and related behavior. That’s why I kept the classes separate for now, v4 classes have the V4 suffix.

As for the shadow logic, I'm still unsure whether the fixups are actually needed. For the 4GL-to-Java flow, those paths aren't even reached, but I preserved as much of the original structure as possible for compatibility with the v2 implementation. I also updated the wiki as much as I can in order to explain things much better, if anything is unclear or if you cannot access the new revision please let me know. Thank you.

Aaand this is the output of a dumpTree(true) for this input: "1 + 2 * 3 + 4 * 5"
Related java testing code:

for (AastV4 node : ast)
        {
           if (node.isRoot())
           {
              node.setRegistry(new TokenTypeRegistryV4(ExprParser.VOCABULARY));
              node.putAnnotation("my annotation", false);
              System.out.println(node.dumpTree(true));
           }
        }

+ [<UNKNOWN>]:9 @1:10
      (my annotation=false)
   + [<UNKNOWN>]:5 @1:2
      1 [INT]:1 @1:0
      * [<UNKNOWN>]:4 @1:6
         2 [INT]:2 @1:4
         3 [INT]:3 @1:8
   * [<UNKNOWN>]:8 @1:14
      4 [INT]:6 @1:12
      5 [INT]:7 @1:16

Greg, I reviewed the hidden field, the ShadowNode structure, and the entire TreeLocal mechanism. The conclusion is that this logic is no longer relevant under the ANTLR v4 architecture and can safely(?) be removed.

The reason comes from a fundamental difference between ANTLR 2 and ANTLR 4 in how hidden tokens (whitespace, comments) are handled:

• In ANTLR 2, hidden tokens were part of the actual token stream: CommonHiddenStreamToken.
  They formed linked chains (prev/next), and the AST machinery (including ShadowNode and TreeLocal) had to incorporate them explicitly through shadow nodes, fixups, left/right mappings, and positional adjustments.

• In ANTLR 4, hidden tokens are not part of the parse tree.
  They are kept only in the TokenStream using channels, and the parser only consumes tokens on the DEFAULT channel. This means that the AST constructed from the parse tree will never naturally contain whitespace or comments, unless we manually insert them.

As a direct consequence:

• the hidden field in AST v4 nodes will always remain false

• shadow nodes can never appear naturally in the tree

• TreeLocal, inlinedShadows, shadowLeft/right, fixupInlinedShadows, and all related logic become effectively dead code

ANTLR v4 already exposes hidden tokens directly through CommonTokenStream.getHiddenTokensToLeft/Right, making the old shadow-node infrastructure unnecessary

Additionally, the FWD pipeline in active use is strictly 4GL to Java. The historical Java to 4GL path (for which shadow nodes were originally intended, right?) does not seem to be part of the current workflow. If any form of code reconstruction is still needed or planned, would it rely on the old ANTLR2-style shadow model, or would it be simpler to use token channels directly in v4?

For example, I moved whitespace into the HIDDEN channel using WS : [ \t\r\n]+ -> channel(HIDDEN); and then printed the text, line, and column for each hidden token (i.e., each whitespace).

Related java testing code:

    for (Token tok : stream.getTokens()) 
        {
           if (tok.getChannel() == Token.HIDDEN_CHANNEL) 
           {
               System.out.println(
                   "HIDDEN: '" + tok.getText() +
                   "' @ line " + tok.getLine() +
                   ", col " + tok.getCharPositionInLine()
               );
           }
        }

Output:

HIDDEN: ' ' @ line 1, col 1
HIDDEN: ' ' @ line 1, col 3
HIDDEN: ' ' @ line 1, col 5
HIDDEN: ' ' @ line 1, col 7
HIDDEN: ' ' @ line 1, col 9
HIDDEN: ' ' @ line 1, col 11
HIDDEN: ' ' @ line 1, col 13
HIDDEN: ' ' @ line 1, col 15

I think you are conflating parse-time processing with TRPL-processing. During TRPL, the AST must have the shadow node infrastructure which we have today. I don't see value in changing that. The downstream processing relies upon this for comment conversion (in the 4GL to Java case) and for anti-parsing 4GL code (the 4GL to 4GL case, see ProgressTransformDriver).

During lexing/parsing we will indeed need to create those shadow nodes differently because of ANTLR v4.

During testing, I realized I forgot to assign the parent AST to the shadow nodes. Right now, they’re orphans:), so I need to set their parent to the root node.

Another thing I noticed: I added the shadows nodes to the list after the left and right IDs were populated, which also needs to be corrected.

I do not think we should use left and right. The resulting tree is not always binary. One node may have multiple children. Each of them should be quickly accessible in random manner (unlike the current simple linked list) so the structure to store them will most likely be an array. Of course, this poses some problems when a new child is added or removed, therefore we will have to do some compromises. A first step is to create a constructor for the node which will create that array with an expected child count. Maybe add the type and text as well.

• the literals, comments, white-spaces, etc are always leaves so the array will not be created:

  new Node(INT_LITERAL, "3", 0)
  new Node(SINGLE_LINE_COMMENT, "some comment", 0)

• a specific attribute may have a single value:

  new Node(SERIALIZE_NAME, "SERIALIZE-NAME", 1)

• the usual binary arithmetic operators will have 2 children, as you noted:

  new Node(OP_MULTIPLY, "*", 2)

Of course, a lot of nodes (for example, DEFINE TEMP-TABLE will have at least a node for each FIELD and INDEX it defines, beside other attributes) have variable number of children which cannot be exactly estimated when the node is created. Therefore, the management methods (add(), insertAt(), remove(), maybe replace()) will have to be optimally implemented.

Ovidiu Maxiniuc wrote:

I do not think we should use left and right. The resulting tree is not always binary.

Ovidiu, my previous two notes were specifically about the shadow nodes, not the AST tree itself. The new implementation of AnnotatedAst does include a list of children. The example I used happened to build a binary tree, but in reality a node can have more than two children.

Regarding the shadow nodes: the left and right IDs are used to locate the nearest AST node for each shadow node, whether that node is hidden or visible. By hidden I mean another shadow node (whitespace, comment, etc.), and by visible I mean a real AST node produced from a grammar rule.
My understanding is that shadow nodes also appear in the tree, but their parent is always the root and they should be marked with the hidden flag.

If I overlooked something in this logic, please let me know.

I have a question regarding artificial nodes. These are the nodes that the grammar creates manually, for example using constructs like #[FORMAT_PHRASE, "format phrase"]. If such an artificial node is attached directly under the root, will this interfere with the shadow-node logic..? My understanding is that artificial nodes are used only for structural grouping, essentially to act as parent nodes for certain related AST elements, and they do not originate from real tokens in the input stream.

However, I am not entirely sure whether this is their full purpose or if there are additional implications I may be overlooking. Could someone please provide a bit more clarification on the actual role of these artificial nodes? Thanks in advance!

During the transpiling from 4GL to Java, FWD parser and TRPL rules will add these additional nodes. The semantic of these languages is different and we need to accommodate the extra structural constructs of Java language. In fact, I think this happens gradually, in both AST and later JAST processing, as new information becomes available from previous processing.

In conclusion, there is not a bijective relation between tokens from 4GL stream and final AST/JAST structure. Some tokens are dropped (possibly replaced by a more simple annotation on its parent) and at the same time, these artificial nodes gets created to store data acquired while processing nodes in previous iterations. They usually attached to very specific places rather than the root of the tree. This should not interfere with shadow logic.

BTW, for the comments, there is a conversion switch which actives their addition to final Java output. If this is this case, they are no longer 'in shadow'. This is nice to have especially when debugging. To do so, some processing is usually done on the texts, mainly to make sure they obey the Java syntax (character encoding, line-ending). However, in release, mode some customers prefer to remove them. The sole advantage being the more compact generated Java code.

Thank you, Ovidiu. I asked this because we need to create helper methods that allow us to build the AST directly from the grammar itself, as ANTLR2 did, but adapted to the stricter constraints of ANTLR4.

The shadow nodes should not be directly attached to the root. Here is an example:

...
  </ast>
  <tree-local>
    <shadow-node-list>
      <shadow-node col="1" id="12884901897" line="1" right-id="12884901896" text="/*&#10; * Copyright 2016-2017 Golden Code Development Corporation&#10; *&#10; * Permission is hereby granted, free of charge, to any person obtaining a copy of this software&#10; * and associated documentation files (the &quot;Software&quot;),  to deal in the Software without&#10; * restriction, including without limitation the rights to use, copy, modify, merge, publish,&#10; * distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the&#10; * Software is furnished to do so, subject to the following conditions:&#10; *&#10; * The above copyright notice and this permission notice shall be included in all copies or&#10; * substantial portions of the Software.&#10; *&#10; * THE SOFTWARE IS PROVIDED &quot;AS IS&quot;, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING&#10; * BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND&#10; * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,&#10; * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,&#10; * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.&#10; */" type="COMMENT"/>
      <shadow-node col="4" id="12884901896" left-id="12884901897" line="18" right-id="12884901895" text="&#10;&#10;&#10;&#10;" type="WS"/>
...

The tree-local section appears at the end of the AST, after all non-shadow AST nodes. The shadow-node instances then are further contained inside a shadow-node-list child of tree-local. Thus, it doesn't matter how the AST nodes are modified, the shadow nodes are always unaffected.

In regard to artificial nodes, it is true that some TRPL code will create them but the majority are created during th eparse itself. They exist primarily to create a cleaner structure to the tree than would otherwise be possible if we used "natural tokens" for the tree.

A classic example is something like the FORMAT_PHASE noted above. There is no single text in the input file which always prefixes the format phrase. It can have a large number of possible first tokens. The downstream matching is made much easier by always structuring the specific format phrase nodes under the artificial FORMAT_PHASE.

Yes, we also will put annotations in these nodes as needed, but the primary objective is to structure the AST for better downstream processing.

Just a quick note regarding my next steps: I’ll begin modifying the schema.g file to migrate it to ANTLR4. Working on a real grammar will help me identify which helper methods are needed, what limitations or bugs appear, where the implementation can be improved, and how the performance compares. I expect this to take some time, but trying to reason about everything without a practical example has proven much harder. By doing this, we will also gain a concrete reference example that can guide the migration of the remaining grammars.

Good plan.

Code Review Schema.g4

It is a good start and there is a lot to like about it. Some thoughts:

1. For an early POC, we can embed the lexer rules but we will need to use the v4 equivalent of the ProgressLexer. This allows common code but more importantly it ensures that the same TRPL processing that handles 4GL code ASTS will also be able to handle the schemata ASTs. In other words both kinds of AST must be ProgressAst instances with the same token types. This may already be what you are planning, but I just wanted to make it explicit.

2. As a guideline, keep the existing parser structure and even the structure of every rule, if possible. The reasoning here is that the existing logic exists for a reason. Most often it is because I felt that the rules would be most maintainable in that design OR because it most naturally encoded the tree building. In some cases, making changes might very subtly change the logic and thus we would introduce regressions.

I see that often you have maintained much of the structure. I like that. I do see some changes. For example, I prefer the original structure of the schema rule which uses (sequence | table)* instead of matching them in schema_item. The reason: it is easier to read (in my opinion) and I don't see the value in making a subrule there.

Not everything we wrote is perfect or even good. So, we will have some exceptions when we will have differences in the v4 grammar:

• The original structure is sub-optimal or otherwise not preferred.
• We must make a change in order to properly encode the matching or tree building.

If a change needs to be made, let's discuss the reason why it makes sense.

3. We need to discuss the tree building. I think the idea of returning the created subtree from each rule makes good sense. The part I'd like to change is that currently, all the tree building is in the exit actions. I'd like to see the alternative where the tree building is more in-line with each part of the matching rules. Let's compare them and discuss the right approach. I'd also like to see more of a defined idea of the tree building helpers.

4. In schema, let's not use lists to gather the results. It is costly to add the extra collections when we can instead just attach to the root as we go (this is a good reason to build inline as I noted in 3 above).

5. Let's call setBuildParseTree(false) on the parser so that we avoid ANTLR creating the unnecessary parse tree.

Greg Shah wrote:

3. We need to discuss the tree building. I think the idea of returning the created subtree from each rule makes good sense. The part I'd like to change is that currently, all the tree building is in the exit actions. I'd like to see the alternative where the tree building is more in-line with each part of the matching rules. Let's compare them and discuss the right approach.

You're completely right. Keeping the AST construction only in exit actions introduces several drawbacks. One issue is the extra memory overhead caused by the intermediate collections (lists). Another is that delaying AST construction can lead to subtle differences in behavior. I’ve already applied your suggestion and reorganized the schema.g4 file so that the AST is built inline, as the rule matches. I also added a few more rules methods. Even for me, this approach is cleaner and makes rewriting rules much easier compared to juggling collections.

I'd also like to see more of a defined idea of the tree building helpers.

My initial hope was that migrating schema.g would reveal some clear patterns that could be turned into helpers. So far, I haven’t found anything obvious that warrants a dedicated helper function. But as I continue rewriting more rules, I expect such patterns to emerge naturally. If nothing comes up, that’s actually ideal, it means the existing helpers are sufficient and we don’t need to introduce additional complexity. (but I think this will not be the case :)

4. In schema, let's not use lists to gather the results. It is costly to add the extra collections when we can instead just attach to the root as we go (this is a good reason to build inline as I noted in 3 above).

And yes, in schema, removing the use of lists aligns perfectly with this approach. Attaching nodes directly to the root as we parse is more efficient and avoids unnecessary intermediate structures. However, we'll discuss more in the upcoming meet.

I have some good news: I’m very close to finishing the rewrite of the schema file. The remaining work is mainly to review every token and verify its associated string value (plus a few minor gaps). The heavy part is basically done, a nice way to end the week!

I'll summarize what I noticed and how I handled the various 'challenges':

• In the tableProps rule, there is a call to getNumberOfChildren inherited from ANTLR2. The same call appears in indexProps. I replaced it with getNumImmediateChildren, which should behave the same. If needed, I can add a method named getNumberOfChildren that delegates to our existing one, just let me know, Greg.

• I noticed that in some places the AST isn't built automatically by ANTLR2; instead, it is constructed manually. For example:
  ## = #([VALMSG, "valmsg"], (EXPRESSION, "expression"], v)
  • Here we manually build the AST: we first create an artificial VALMSG node, then attach an artificial EXPRESSION node as its child, and finally attach v (the STRING token) as the child of that expression node.

• I observed the usage of rule! (for example valMsg!), which disables automatic AST construction so that the AST can be created manually, as shown above.

• HIDDEN is a reserved name in ANTLR4, so it cannot be used as a rule or token name. I renamed it accordingly.

• I also noticed that some rules do not use the caret symbol (^). This means their matched tokens must be manually attached to the caller's AST node. I encountered this in the trigger_params rule. In this case I used a list, which I don't think we can avoid.

• I also saw semantic predicates, such as the one used in the field rule. ANTLR4 still supports semantic predicates, but not syntactic predicates like ANTLR2 did.

• ANTLR4 removed lookahead hooks. ANTLR2 could look ahead by one token by default, and if the grammar was ambiguous at that lookahead, the parser couldn't determine which alternative to take.

I have some good news: I’m very close to finishing the rewrite of the schema file. The remaining work is mainly to review every token and verify its associated string value (plus a few minor gaps). The heavy part is basically done, a nice way to end the week!

Nice!

In the tableProps rule, there is a call to getNumberOfChildren inherited from ANTLR2. The same call appears in indexProps. I replaced it with getNumImmediateChildren, which should behave the same. If needed, I can add a method named getNumberOfChildren that delegates to our existing one, just let me know, Greg.

I would prefer to remove usage of getNumberOfChildren(). I'm not sure if it was used on purpose but in ANTLR4 I think we should not need it. Just keep our current AnnotatedAst methods (getNumChildren(), getNumImmediateChildren() etc...).

I also noticed that some rules do not use the caret symbol (^). This means their matched tokens must be manually attached to the caller's AST node. I encountered this in the trigger_params rule. In this case I used a list, which I don't think we can avoid.

I'm not sure why we did that, that flat approach it is not a good design in my opionion. Regardless, in such a case we can avoid the use of a collection by passing the parent node into trigger_params from the caller. Then the code in trigger_params can attach each node directly.

I also saw semantic predicates, such as the one used in the field rule. ANTLR4 still supports semantic predicates, but not syntactic predicates like ANTLR2 did.

This should be OK, we don't use syntactic predicates because our deep integration with actions makes backtracking impossible/impractical.

ANTLR4 removed lookahead hooks. ANTLR2 could look ahead by one token by default, and if the grammar was ambiguous at that lookahead, the parser couldn't determine which alternative to take.

What do you mean by "lookahead hooks"? In both ANTLR2 and ANTLR4 we can use LT() to lookahead at arbitrary tokens.

Greg Shah wrote:

What do you mean by "lookahead hooks"? In both ANTLR2 and ANTLR4 we can use LT() to lookahead at arbitrary tokens.

What I meant is that ANTLR removed syntactic predicates, which ANTLR 2 used to control lookahead decisions. ANTLR4 still uses semantic predicated instead and lets the parser choose the path automatically.

Paula Păstrăguș wrote:

Greg Shah wrote:

What do you mean by "lookahead hooks"? In both ANTLR2 and ANTLR4 we can use LT() to lookahead at arbitrary tokens.

What I meant is that ANTLR removed syntactic predicates, which ANTLR 2 used to control lookahead decisions. ANTLR4 still uses semantic predicated instead and lets the parser choose the path automatically.

Got it. This won't be a problem for us, since we don't use them.

Today I worked on the lexer rules and tokens, which are the final missing piece of the schema migration puzzle.
Here are my observations:

• These lexer rules are extremely fragile, a very small mistake can break the entire grammar. Because of this, I didn't manage to finish all the lexer rules migration. NUM_LITERAL is the only one that remains not implemented. The rest of the rules were migrated, hopefully correctly:), but they definitely require careful review.

• In ANTLR, ~ means NOT, used to negate a character or a character set.

• Square brackets [...] define a character set.
  • Example: [abc] matches exactly one character from {a, b, c}.
  • ~[...] means match any character except the ones listed.

• Literal tokens in ANTLR4 use single quotes, not double quotes, unlike ANTLR2.

• Fragment rules were introduced in ANTLR4. I haven’t yet created any, but they can be used whenever we want reusable lexer components.

• ANTLR2 supported mark(), rewind() and commit() directly in lexer actions, ANTLR4 no longer does.
  • In ANTLR4 we can only reposition the input cursor using _input.seek() together with _input.mark(). I have not seen an equivalent of commit(). However, I used the mark and seek approach for STR_OPTIONS lexer rule.

• For DSTRING / SSTRING rules, ANTLR2 used append and newline inside the lexer to modify the token text. ANTLR4 does not support this mechanism, so I replaced it with a StringBuilder and manually append characters as they are matched.
  • Related to the above: ANTLR4 does not allow uninitialized local variables inside lexer rules. Declaring a local StringBuilder triggers a may not have been initialized error, so I had to promote it to a lexer member field.. This part still feels a bit unusual and I may refine it later.

Another important point: to properly test the schema grammar, we need correct values for boolean lexer flags such as schema, unixEscapes, export and others. My plan is to inspect these during debugging and set their appropriate defaults values for the schema grammar.

• I like to recap things as I go, since I may have misunderstood something. Greg, please correct me where needed. Thanks!

For the Progress lexer, I'd like to see the standalone .g4 file for it. It is hard for me to give feedback without seeing it. Instead of attaching them here, I'd prefer them to be checked into the branch.

Another important point: to properly test the schema grammar, we need correct values for boolean lexer flags such as schema, unixEscapes, export and others.

Most of our existing lexers and parsers have a main() which provides a kind of "test program" for loading and running. I would suggest continuing this practice.

Greg Shah wrote:

For the Progress lexer, I'd like to see the standalone .g4 file for it. It is hard for me to give feedback without seeing it. Instead of attaching them here, I'd prefer them to be checked into the branch.

Sure, let me refine it a little bit and I'll commit the grammar afterwards.

Committed the grammar as rev 16006.

• The next step is to split the grammar into SchemaParser.g4 and SchemaLexer.g4. I realize the schema grammar normally depends on the Progress lexer, but for the POC this separation seems like the most practical approach. The current embedded lexer is quite hard to follow.

• I also noticed that in the ANTLR2 lexer the exclamation mark ! suppresses the matched character from the token text, something I overlooked. I got used to removing the ! on the parser side, which led to a mistake. I’ll explore alternatives to avoid using a StringBuilder, perhaps by manipulating the token text directly using getText() and setText() whenever a suppressed character is encountered. I’m not sure yet if this is the optimal solution, but it’s one possible direction.

• There are still a few Java syntax errors in the lexer, but I expect things will become clearer after the grammar split.

• I didn't want to commit the grammar in its current form because the rules still lack updated documentation, and some ANTLR2 comments cannot be ported as-is. My initial plan was to commit only a near-final version, but I now realize that was not ideal, it's better to commit the work incrementally so we catch wrong patterns early instead of later.

Greg, I'm a bit confused... I initially thought that splitting the grammar would solve this issue. Let me explain the situation: in the lexer we reference HEX_LITERAL and other 'artificial' tokens, tokens that do not correspond to any actual input text. In ANTLR4, these symbolic names are not visible in the lexer unless they are defined as real lexer tokens, which is not what we want.

At this point, the only alternative I can think of is to declare these artificial tokens inside the lexer’s @members section and assign them fixed integer values manually.. This way, the lexer can still reference them internally without actually producing them.

As a side note, I feel that rewriting the grammar was actually the easy part, what comes next is where the real complexity begins.

in the lexer we reference HEX_LITERAL and other 'artificial' tokens, tokens that do not correspond to any actual input text. In ANTLR4, these symbolic names are not visible in the lexer unless they are defined as real lexer tokens, which is not what we want.

Are you saying that you have added a tokens { ... HEX_LITERAL, ... } section to the lexer and the lexer rules cannot reference any of those tokens?

Kind of, we need that token for both, the parser and the lexer. But the int values will be different, depending on how we define those tokens..

Into the parser I have smth like this:

public static final int
        FIELD_BLOB=1, FIELD_CHAR=2, FIELD_CLASS=3, FIELD_CLOB=4, FIELD_COM_HANDLE=5, 
        FIELD_DATE=6, FIELD_DATETIME=7, FIELD_DATETIME_TZ=8, FIELD_DEC=9, FIELD_INT=10, 
        .... HEX_LITERAL=29 ....

Into the lexer I have smth like this:

public static final int
        HEX_LITERAL=1, DATE_LITERAL=2, DEC_LITERAL=3, KW_ADD=4, KW_UPDATE=5, KW_DATABASE=6, 
        KW_SEQUENCE=7, KW_TABLE=8, KW_INDEX=9, KW_OF=10, KW_BUFRPOOL=11, KW_CP=12, 
        ....

Hm, into the SchemaLexer.tokens, the value of HEX_LITERAL is 1, and into the SchemaParser.tokens, the value of HEX_LITERAL is 29 => different values. This means the HEX_LITERAL token from the parser is different from the HEX_LITERAL token from the lexer, right..?

A couple of housekeeping items:

• I would prefer if you are working on the ProgressLexer and just use that directly from the schema parser. I don't see the value in creating a subset parser, most of the core matching is the same.
• It will make it easier for me to compare with our existing lexers and parsers if you copy those and make edits rather than creating a new grammar from scratch. That comparison will be critical and the less change there is in the file, the easier it will be to compare.

Kind of, we need that token for both, the parser and the lexer.

You should not define the tokens in the parser, ever. In the parser, you specify the tokenVocab inside the options block for the parser itself.

The tokens section should only exist in the lexer.

You're right, this options block did the trick:

options
{
   tokenVocab = SchemaLexer;
}

Paula Păstrăguș wrote:

• Fragment rules were introduced in ANTLR4. I haven’t yet created any, but they can be used whenever we want reusable lexer components.

Actually, protected from antlr2 is equivalent to fragment from antlr4

We will need a mechanism to replicate the old ANTLR2 append() behavior and the ! operator when migrating to ANTLR4. In ANTLR2, these allowed us to either append extra text to the current token or remove part of the matched text. ANTLR4 no longer supports append() or !, so we must design an alternative solution for cases where we need to alter the token’s final text.

Another aspect we need to clarify is the equivalent for ANTLR2’s LA() lookahead function. At the moment, I used _input.LA() as a replacement, because this is the closest ANTLR4 API for inspecting upcoming characters. However, I’m not fully convinced this is the ideal or final solution. ANTLR4’s prediction engine handles most lookahead internally, so relying on _input.LA() may not always be the best or safest path, especially inside lexer rules.. We should evaluate whether there’s a cleaner or more robust strategy that fits better with ANTLR4’s architecture.

Paula Păstrăguș wrote:

Paula Păstrăguș wrote:

• Fragment rules were introduced in ANTLR4. I haven’t yet created any, but they can be used whenever we want reusable lexer components.

Actually, protected from antlr2 is equivalent to fragment from antlr4

Things are getting more complicated: ANTLR4 does not allow action blocks inside fragment rules. You can still write them, but the grammar will generate a large number of warnings because fragment rules are not permitted to contain actions...

warning(158): SchemaLexer.g4:4211:6: fragment rule SSTRING contains an action or command which can never be executed
warning(158): SchemaLexer.g4:4216:17: fragment rule SSTRING contains an action or command which can never be executed
warning(158): SchemaLexer.g4:4222:20: fragment rule SSTRING contains an action or command which can never be executed
warning(158): SchemaLexer.g4:4223:20: fragment rule SSTRING contains an action or command which can never be executed
warning(158): SchemaLexer.g4:4224:20: fragment rule SSTRING contains an action or command which can never be executed
warning(158): SchemaLexer.g4:4225:20: fragment rule SSTRING contains an action or command which can never be executed
warning(158): SchemaLexer.g4:4227:20: fragment rule SSTRING contains an action or command which can never be executed
.
.
.

I’ll rename SchemaLexer to ProgressLexer (+ I'll keep the same structure of the progress.g grammar lexer into the ProgressLexer.g4), as you might have noticed in my previous note:-)

Paula Păstrăguș wrote:

Things are getting more complicated: ANTLR4 does not allow action blocks inside fragment rules. You can still write them, but the grammar will generate a large number of warnings because fragment rules are not permitted to contain actions...

However, ANTLR4 still allows semantic predicates inside a fragment (protected) lexer rule.

Greg Shah wrote:

If a change needs to be made, let's discuss the reason why it makes sense.

I looked more closely at the YEAR_TRASH_MODE fragment and I still don’t fully understand why the lookahead is needed here:
YEAR_TRASH_MODE : ( "/" | "-" ) (DIGIT | '/' | '-' | { LA(2) >= '0' && LA(2) <= '9' }? '.')* ;

Wouldn’t a simpler form work as well?
YEAR_TRASH_MODE : ( "/" | "-" ) (DIGIT | '/' | '-' | '.' DIGIT)* ;

As far as I understand, this rule matches some extra "trash" characters that may appear after a date literal. So I’m not sure why we first need to look ahead to check whether the next character is a digit before allowing / matching the dot (aka the current character)

I used this rule as an example because the LA-based logic appears in multiple larger lexer rules, and I thought a smaller rule would help me understand the behavior better.

If I’m misunderstanding this, please let me know.

I looked more closely at the YEAR_TRASH_MODE fragment and I still don’t fully understand why the lookahead is needed here:
YEAR_TRASH_MODE : ( "/" | "-" ) (DIGIT | '/' | '-' | { LA(2) >= '0' && LA(2) <= '9' }? '.')* ;

Wouldn’t a simpler form work as well?
YEAR_TRASH_MODE : ( "/" | "-" ) (DIGIT | '/' | '-' | '.' DIGIT)* ;

It may work. We would need some testcases. I would expect I left some behind somewhere, probably in the Old UAST Testcases. For example, uast/date_literals_vs_dec_literals.p has some of the month trash cases. In the new Testcases, you can also try the tests/conversion/lexer/ tests. Add an action to the ANTLR2 lexer to output to stdout when YEAR_TRASH_MODE is matched. Then run the testcases and see if any of them trigger the code.

If you don't find good testcases, please write some that explore that code. Even if you find some testcases, we may need to expand them to cover the edge cases. Run on OE and find the limits to the matching. All of these testcases need to be written for ABLUnit and added to the new lexer tests. The bottom line, let's make sure our testcases trigger this rule and prove that the ANTLR4 version matches.

I think I am making some progress with the lexer. I managed to set up a small test project, and I am actually quite happy to see both recognition errors and some recognized tokens. This shows that the lexer is active and is processing the input.

The next step will be to further refine the lexer so that it fully matches the behavior of our current implementation and correctly recognizes all tokens. At this point, I feel that the foundation is in place and the setup is solid.

Test output:

line 1:0 token recognition error at: 'D'
line 1:1 token recognition error at: 'E'
line 1:2 token recognition error at: 'F'
line 1:3 token recognition error at: 'I'
line 1:4 token recognition error at: 'N'
line 1:5 token recognition error at: 'E'
line 1:7 token recognition error at: 'V'
line 1:8 token recognition error at: 'A'
line 1:9 token recognition error at: 'R'
line 1:10 token recognition error at: 'I'
line 1:11 token recognition error at: 'A'
line 1:12 token recognition error at: 'B'
line 1:13 token recognition error at: 'L'
line 1:14 token recognition error at: 'E'
SYMBOL                    line=1 col=16  text="a" 
SYMBOL                    line=1 col=18  text="as" 
line 1:21 token recognition error at: 'I'
line 1:22 token recognition error at: 'N'
line 1:23 token recognition error at: 'T'
line 1:25 token recognition error at: 'F'
line 1:26 token recognition error at: 'O'
line 1:27 token recognition error at: 'R'
line 1:28 token recognition error at: 'M'
line 1:29 token recognition error at: 'A'
line 1:30 token recognition error at: 'T'
DSTRING                   line=1 col=32  text="""X"X9"X99"X999"X9999"X99999" 
UNKNOWN_TOKEN             line=1 col=40  text=";" 

With a little bit of fine tuning, the test output in this moment looks like this:

type = SYMBOL, line = 1, col = 0, text = define
type = SYMBOL, line = 1, col = 7, text = variable
type = SYMBOL, line = 1, col = 16, text = a
type = SYMBOL, line = 1, col = 18, text = as
type = SYMBOL, line = 1, col = 21, text = int
type = SYMBOL, line = 1, col = 25, text = format
type = STRING, line = 1, col = 32, text = "X99999" 
type = UNKNOWN_TOKEN, line = 1, col = 40, text = ;

I'll need to see the equivalence of the case sensitivity options tho.

FYI, the testcase should have . instead of ; at the end of the first line.

I'm pleased to see the continued headway.

Greg Shah wrote:

FYI, the testcase should have . instead of ; at the end of the first line.

Good catch!

Just a quick update on this issue. At the moment, I’m investigating a memory leak in the lexer. Using the following testcase:

def var i as char initial 
"This table is the actual audit data created as a result of enabled audit events according to the audit policy. All audit events will be stored in this central table including both database and application level audit events, however certain fields are specific to database auditing events and the purpose and use of some of the fields may vary depending on the type of audit event.

The reason for maintaining a centralized table is to simplify the architecture and tools requirements. It may be necessary at some later stage to break this table down into sub tables purposed for specific types of auditing.

In the case of database auditing, the modified field values will be recorded in the child table _aud-audit-data-value, writing a seperate record for each modified field value. This is to facilitate the efficient searching for all modifications to a particular field.

This table schema will be used for both the on-line audit data as well as the archive audit data.

For non database (CRUD) auditing, the fields that sequence the order of the audit data could be used for the application. For example, the sequence fields could be used to keep an ordered count of failed login attempts, or perhaps the sequence of tasks within a workflow.

This is a WORM table - Write Once, Read Many - to avoid tampering of the data. Its contents can only be deleted by the registered archive manager.

The data in this table can be sealed with a Message Digest or MAC for non-repudiation.

There are a number of indexes required on this table to facilitate efficient reporting and these could be made inactive in the operational database and only enabled in an archive database if required to improve performance.

Records in this table are moved to an archive database as part of the archive process.
" 

According to VisualVM, the heap size grows extremely fast, reaching nearly 8 GB before an OutOfMemoryError is thrown. The problem appears to be triggered by the DSTRING lexer rule:

DSTRING
   :
      '"'
      { 
         sb = new StringBuilder("");
         sb.append("\"");                       // opening " 
         System.out.println("String opening matched!");
      }
      (
         '~"'      { sb.append("~\""); }         // allow ~" 
         // ANTLR mis-generates this lookup and only uses LA(1) and LA(2) and ignores
         // LA(3) which causes a recognition failure; we add LA(3) manually to fix it
         | {_input.LA(3) == '"' }?
           '"~"'   { sb.append("\"~\""); }      // allow "~" (the leading quote is not output in the 4GL, so this is the same as ~")
         | '~~'    { sb.append("~~"); }         // allow ~~ (consume as one)
         | '~\\'   { sb.append("~\\"); }        // allow ~\ (consume as one)
         | '~'     { sb.append("~"); }          // allow ~ by itself
         | { unixEscapes }?
           '\\"'   { sb.append("\\\""); }       // allow \" 

         // ANTLR mis-generates this lookup and only uses LA(1) and LA(2) and ignores
         // LA(3) which causes a recognition failure; we add LA(3) manually to fix it
         | { unixEscapes && _input.LA(3) == '"' }?
           '"\\"'  { sb.append("\"\\\""); }     // allow "\" (the leading quote is not output in the 4GL, so this is the same as \")
         | { unixEscapes }?
           '\\\\'  { sb.append("\\\\"); }       // allow \\ (consume as one)
         | { unixEscapes }?
           '\\~'   { sb.append("\\~"); }        // allow \~ (consume as one)
         | '\\'    { sb.append("\\"); }         // allow \ by itself
         | '""'    { sb.append("\"\""); }       // allow "" 

         | { schema }?                          // convert CRNL to a space char
           '\r\n'  { sb.append(" "); }          // when lexing a DF file PMP: TEST THIS ONE!!!!!!!!!!
         | { schema }?                          // convert NL or CR to a space
           ('\n' | '\r')  { sb.append(" "); }   // char when lexing a DF file  PMP: TEST THIS ONE!!!!!!!!!!

         | '\n'    { sb.append("\n"); }          // otherwise allow NL

         |   ~'"'  { sb.append((char) _input.LA(-1)); }            // anything else except closing " 

         // { System.out.println("DSTRING TEXT = " + getText()); }
      )*
      '"' 
      { 
         sb.append("\""); 
         setText(sb.toString()); 
         sb = new StringBuilder(""); 
      }                              // closing " 
      -> type(STRING)
   ;

The most likely root cause is the creation of multiple very large intermediate strings during lexing. This suggests that using a StringBuilder directly inside the lexer rule is not viable in this scenario. I’ll look for an alternative approach that avoids this behavior and report back with a proposed solution.

I think I’ve found a workaround for the DSTRING lexer rule, although it requires changing the original structure a bit.

DSTRING
  : 
      '"' DSTRING_ITEM* '"' 
      {
         // finally, after the string content is matched, we need to replace all the line breaks with a space:
         String txt = getText();
         txt = txt.replaceAll("\\R", " ");
         setText(txt);
      }
      -> type(STRING)
  ;

fragment 
DSTRING_ITEM
  : '~"'
  | { _input.LA(3) == '"' }? '"~"' 
  | '~~'
  | '~\\'
  | '~'
  | { unixEscapes }? '\\"'
  | { unixEscapes && _input.LA(3) == '"' }? '"\\"'
  | { unixEscapes }? '\\\\'
  | { unixEscapes }? '\\~'
  | '\\'
  | '""'
  | { schema }? '\r\n'
  | { schema }? ('\n' | '\r')
  | '\n'
  | ~'"'
  ;

With this version, the tokens from standard.df are now emitted and the memory leak is gone.

Greg, what do you think? Does this seem like a reasonable direction to proceed with, or should I dig deeper and look for an alternative approach?

Next, I plan to compare the tokens emitted by the ANTLR2 and ANTLR4 lexers for this .df file. It’s good that there are no hangs anymore, but given the size of the file, some differences may still show up (definitely:)

The core idea is reasonable.

Please keep as much of the original grammar as possible. For example, the code shown here in the fragment is missing the spacing and comments of my original. That makes it much harder for me to evaluate your changes.

Also: please do not use the "short cut" forms like DSTRING_ITEM* or TOKEN_NAME?. Use the long forms instead.

I used the test suite located in testcases/tests/conversion/lexer/4gl_testcases to evaluate areas where the current lexer implementation can be improved. The results indicate that there is still room for refinement. Out of 6792 test cases, only 4789 produce token streams identical to those generated by the ANTLR2 version, while 2003 files exhibit differences. Although this number is relatively large, my assumption is that many of these discrepancies stem from the same underlying issues and could be addressed through a small number of common fixes.

The first difference appears in the symbol_backslash.p test case, whose content is:

\\

The ANTLR2 lexer produces the following output:

type = FILENAME, line = 1, text = \\
type = DOT, line = 1, text = .

The ANTLR4 lexer produces the following output:

type = FILENAME, line = 1, text = \\

Why does the ANTLR2 version emit a DOT token in this case? The input file does not contain a dot at all, so it’s unclear where this token is being matched from. Any clarification on this behavior would be appreciated.

Also, I did not included the column number because it is a deal breaker:) In antlr4, the col number is starting from 0, not from 1, as antlr2 did.

Should invalid test cases also be expected to produce matching results...? For example, consider the invalid input */ /* */ /* . With ANTLR2, the lexer produces the following output:

type = MULTIPLY, line = 1, col = 1, text = *
type = DIVIDE, line = 1, col = 2, text = /

In contrast, antlr4 produces:

type = MULTIPLY, line = 1, col = 0, text = *
type = DIVIDE, line = 1, col = 1, text = /
type = DIVIDE, line = 1, col = 11, text = /
type = MULTIPLY, line = 1, col = 12, text = *

In this case, the difference is likely caused by how ANTLR2 and ANTLR4 handle invalid input, particularly in terms of recovery.

Where are the .lexer baselines in the testcases project? I don't see them.

Why does the ANTLR2 version emit a DOT token in this case?

Our ANTLR2 tests are run with a full toolset (preprocessor etc...). I think we may be adding an implicit . at the end of the external procedure because in OE it is allowed to leave the last 4GL statement unfinished. It is a dumb feature. I don't recall where we do this.

Should invalid test cases also be expected to produce matching results...?

Generally, yes. Where the lexer should not match something, it needs to fail as soon as we know the match is invalid.

Greg Shah wrote:

Where are the .lexer baselines in the testcases project? I don't see them.

Same here. Anyway, I created a small test class to iterate through all the test cases from e4gl_testcase and run them against both the v4 and v2 versions. After that, I used meld/diff to check where the differences were coming from. I didn’t actually apply the settings from there directly.

Greg Shah wrote:

Our ANTLR2 tests are run with a full toolset (preprocessor etc...). I think we may be adding an implicit . at the end of the external procedure because in OE it is allowed to leave the last 4GL statement unfinished. It is a dumb feature. I don't recall where we do this.

It makes sense, I'll look for it.

I also identified another significant issue related to the k = 4 setting. For example, given the input:

00001.00001.0005

ANTLR2 emits two decimal literals (00001.00001 and .0005), whereas ANTLR4 emits a single date literal corresponding to the entire input. I think this difference stems from the k lookahead value in ANTLR2.

Paula Păstrăguș wrote:

I didn’t actually apply the settings from there directly.

In generate_preproc_output.p we have:

COMPILE VALUE(inputFile) SAVE PREPROCESS VALUE(outputFile) NO-ERROR.

This means that before 'feeding' the lexer with a lot of files, we need to compile them? I'll add that step too and redo the tokens generation for v4. Greg, this is the piece of code responsible for adding that DOT?

Paula Păstrăguș wrote:

I also identified another significant issue related to the k = 4 setting. For example, given the input:
[...]

ANTLR2 emits two decimal literals (00001.00001 and .0005), whereas ANTLR4 emits a single date literal corresponding to the entire input. I think this difference stems from the k lookahead value in ANTLR2.

I doubt this is a k issue. We have some island lexing issues that we currently fix in the parser using relex(). We'll have to evaluate the rules on a case by case basis.

Paula Păstrăguș wrote:

Paula Păstrăguș wrote:

I didn’t actually apply the settings from there directly.

In generate_preproc_output.p we have:

COMPILE VALUE(inputFile) SAVE PREPROCESS VALUE(outputFile) NO-ERROR.

This means that before 'feeding' the lexer with a lot of files, we need to compile them? I'll add that step too and redo the tokens generation for v4. Greg, this is the piece of code responsible for adding that DOT?

No, this is something that is only used in OE. It is used to capture the preprocessor output from the OpenEdge compiler. For preprocessor testcases, we use this to capture the listing when a new testcase is added. It has nothing to do with the extra . or with lexer tests.

Greg Shah wrote:

Paula Păstrăguș wrote:

I also identified another significant issue related to the k = 4 setting. For example, given the input:
[...]

ANTLR2 emits two decimal literals (00001.00001 and .0005), whereas ANTLR4 emits a single date literal corresponding to the entire input. I think this difference stems from the k lookahead value in ANTLR2.

I doubt this is a k issue. We have some island lexing issues that we currently fix in the parser using relex(). We'll have to evaluate the rules on a case by case basis.

You're right! After digging deeper, I agree that this is not purely a k lookahead issue. The real cause is the following semantic predicate in the lexer (NUM_LITERAL rule):

                   | {
                        digs < 3 &&
                        (_input.LA(1) != '.' || 
                         (_input.LA(2) == '-' || (_input.LA(2) >= '0' && _input.LA(2) <= '9')))
                     }?

The key point is that digs < 3 evaluates to true at the moment the alternative is selected. In ANTLR4, semantic predicates are evaluated once, at the decision point where the lexer chooses an alternative, and they are not reevaluated as the rule continues to consume input. As a result, once this alternative is chosen, the lexer proceeds deterministically and can consume the entire input as a single token, ultimately producing one date literal.

In contrast, ANTLR2 used backtracking and could effectively reconsider earlier decisions. When digs later exceeded the threshold, ANTLR2 was able to abandon the current alternative, emit the partially matched decimal literal, and continue lexing the remaining input as a separate token.

Please post the generated Java code (in both ANTLR2 and ANTLR4) for an example rule that shows this. Evaluating all semantic predicates once for the entire rule is going to be a huge problem. Normally, if an alternative is in a loop, then the semantic predicate is evaluated each time thorugh the loop. If that is not how it works, then I need to see the generated code to understand the implications but "it won't be pretty".

Consider this example rule:

NUM
  : (DIGIT { count++; System.out.println("inc count (int) = " + count); })+
    (
        {count < 2}?                        
        '.' (DIGIT { count++; System.out.println("inc count (frac) = " + count); })+
        {
            System.out.println("ALT1 chosen, final count = " + count);
        }
      | '.' (DIGIT { count++; System.out.println("inc count (frac) = " + count); })+
        {
            System.out.println("ALT2 chosen, final count = " + count);
        }
    )
    {
        System.out.println("END NUM, count = " + count);
        count = 0;
    }
  ;

Input = 123.45

ANTRL2 output:

inc count (int) = 1
inc count (int) = 2
inc count (int) = 3
inc count (frac) = 4
inc count (frac) = 5
ALT2 chosen, final count = 5
END NUM, count = 5
type = 4, text = 123.45

ANTRL4 output:

inc count (int) = 1
inc count (int) = 2
inc count (int) = 3
inc count (frac) = 4
inc count (frac) = 5
ALT1 chosen, final count = 5
END NUM, count = 5
type = NUM, text = 123.45

ANTLR2 java code:

ShowHide

    public final void mNUM(boolean _createToken) throws RecognitionException, CharStreamException, TokenStreamException {
        int _ttype; Token _token=null; int _begin=text.length();
        _ttype = NUM;
        int _saveIndex;

        {
        int _cnt3=0;
        _loop3:
        do {
            if (((LA(1) >= '0' && LA(1) <= '9'))) {
                mDIGIT(false);
                count++; System.out.println("inc count (int) = " + count);
            }
            else {
                if ( _cnt3>=1 ) { break _loop3; } else {throw new NoViableAltForCharException((char)LA(1), getFilename(), getLine(), getColumn());}
            }

            _cnt3++;
        } while (true);
        }
        {
        if (((LA(1)=='.'))&&(count < 2)) {
            match('.');
            {
            int _cnt6=0;
            _loop6:
            do {
                if (((LA(1) >= '0' && LA(1) <= '9'))) {
                    mDIGIT(false);
                    count++; System.out.println("inc count (frac) = " + count);
                }
                else {
                    if ( _cnt6>=1 ) { break _loop6; } else {throw new NoViableAltForCharException((char)LA(1), getFilename(), getLine(), getColumn());}
                }

                _cnt6++;
            } while (true);
            }

            System.out.println("ALT1 chosen, final count = " + count);

        }
        else if ((LA(1)=='.')) {
            match('.');
            {
            int _cnt8=0;
            _loop8:
            do {
                if (((LA(1) >= '0' && LA(1) <= '9'))) {
                    mDIGIT(false);
                    count++; System.out.println("inc count (frac) = " + count);
                }
                else {
                    if ( _cnt8>=1 ) { break _loop8; } else {throw new NoViableAltForCharException((char)LA(1), getFilename(), getLine(), getColumn());}
                }

                _cnt8++;
            } while (true);
            }

            System.out.println("ALT2 chosen, final count = " + count);

        }
        else {
            throw new NoViableAltForCharException((char)LA(1), getFilename(), getLine(), getColumn());
        }

        }

        System.out.println("END NUM, count = " + count);
        count = 0;

        if ( _createToken && _token==null && _ttype!=Token.SKIP ) {
            _token = makeToken(_ttype);
            _token.setText(new String(text.getBuffer(), _begin, text.length()-_begin));
        }
        _returnToken = _token;
    }

ANTLLR4 java code:

ShowHide

private void NUM_action(RuleContext _localctx, int actionIndex) {
        switch (actionIndex) {
        case 0:
             count++; System.out.println("inc count (int) = " + count); 
            break;
        case 1:
             count++; System.out.println("inc count (frac) = " + count); 
            break;
        case 2:

                        System.out.println("ALT1 chosen, final count = " + count);

            break;
        case 3:
             count++; System.out.println("inc count (frac) = " + count); 
            break;
        case 4:

                        System.out.println("ALT2 chosen, final count = " + count);

            break;
        case 5:

                    System.out.println("END NUM, count = " + count);
                    count = 0;

            break;
        }
    }

Greg, here's the caller code for NUM_action..

    @Override
    public void action(RuleContext _localctx, int ruleIndex, int actionIndex) {
        switch (ruleIndex) {
        case 0:
            NUM_action((RuleContext)_localctx, actionIndex);
            break;
        }
    }

Please attach the entire lexer, I need to see more context.

The extra DOT appears when the Progress lexer is instantiated with kludge = true. I set this flag to false, which removed that noise from the analysis. After this change, only 688 files still show differences.

I want to correct what I said yesterday about ANTLR4 being fully deterministic. I ran into an odd case where 12 is first tokenized as a NUM_LITERAL (which is correct), but later the same value is also tokenized as a DOT. Here is the testcase:

FOR EACH tt NO-LOCK:
  DISPLAY tt.f1
    (SUB-AVERAGE) WITH FRAME frame6 12 DOWN. // --------> 12 = NUM_LITERAL 
END.

FOR EACH tt NO-LOCK:
  DISPLAY tt.f1
    (SUB-AVERAGE) WITH FRAME frame6 12 DOWN. // --------> 12 = DOT 
END.

The two FOR EACH blocks are identical and appear twice. I’m not yet sure why ANTLR4 behaves this way in this case, but I’ll investigate further to understand what’s happening.

Actually, that was my mistake, I forgot to reinitialize a value. Thankfully, this isn't an ANTLR4 issue after all.

After I fixed that, only 472 differences remains.

I managed to fix another issue, which reduced the number of files with differences to 90!! The fix was embedding the STR_OPTION rule into the STRING rule.

The number of files with differences has dropped to 86 after fixing another issue.

Around 30 tests are invalid ones, and the rest of them are caused by #10804-78, #10804-80.

I think it is time to get the lexer reworked as a one-for-one replacement of the progress.g lexer, including the javadoc, the order of the rules etc...

Then check it in so that I can be seeing the changes from day to day.

Committed the ProgressLexer.g4 as rev 16007.

I discovered something interesting that fixes roughly 20 tests, bringing the number of files with differences down to 62.

The issue turned out to be that the caseSensitiveLiterals option is no longer supported in ANTLR4. As a result, the SYMBOL lexer rule must explicitly account for uppercase letters as well. That’s why the following additional condition is needed (see the last line):

              (_input.LA(2) >= 'a' && _input.LA(2) <= 'z') ||
              (_input.LA(2) >= '0' && _input.LA(2) <= '9') ||
              _input.LA(2)  == '#' || _input.LA(2) == '$'  ||
              _input.LA(2)  == '%' || _input.LA(2) == '&'  ||
              _input.LA(2)  == '-' || _input.LA(2) == '_'  ||
              (_input.LA(2) >= 'A' && _input.LA(2) <= 'Z')

I also resolved slash/valid/backslash_slash.p test. 21 files remains.

The goal of slash/valid/backslash_slash.p is to drop a backslash when it is immediately followed by a slash. A typical example of this situation is \/something, where the backslash should be ignored.

However, as observed in other cases, ANTLR4 does not consider lexer actions when computing the prediction path. This is the same limitation encountered previously with the NUM_LITERAL rule (which is not solved yet), where state changes performed in actions are not visible during prediction.

To work around this, the problematic pattern was isolated into a dedicated lexer rule:

BS_BEFORE_SLASH
   : 
      { _input.LA(2) == '/' }? '\\' { setChannel(Token.HIDDEN_CHANNEL); }
   ;

This rule explicitly matches a backslash only when the next character in the input stream is a slash, and sends it to the hidden channel so it does not interfere with parsing.

Additionally, the SYMBOL lexer rule must be adjusted so that it does not eagerly match a backslash in this scenario. Instead of always matching BACKSLASH as the first character, the following guarded alternative is used:

         // here we only need to check for backslash when a slash is not the next character in the input stream.
         | { _input.LA(2) != '/' }? '\\'            // instead of the original '\\'

The issue turned out to be that the caseSensitiveLiterals option is no longer supported in ANTLR4. As a result, the SYMBOL lexer rule must explicitly account for uppercase letters as well. That’s why the following additional condition is needed (see the last line):

What about the lexer option caseInsensitive=true;?

The reason caseInsensitive wasn’t enough is that it only helps ANTLR match token patterns (literals / character ranges). It does not magically 'case-fold' your custom lookahead logic. In these cases we need to be more explicit when it comes to case sensitivity.

Remaining tests: 13

I think I solved the NUM_LITERAL digs issue, plus 4 tests from string/../valid.

Remaining tests: 8

All tests from comments/invalid are now resolved.

Previously, in ANTLR2, when a comment opening /* was encountered without a corresponding closing delimiter, the lexer itself raised an error, effectively stopping tokenization after the /*. I reproduced this behavior in ANTLR4 by using lexer modes for comments, which ensures that unterminated block comments halt further token processing. The previous handling logic was also preserved.

Three of the remaining failing tests may be false positives. In my local test project, I implemented a custom SymbolResolver with a simplified lookup method, as the original implementation relies heavily on FWD-specific infrastructure. I suspect the failures are related to abbreviation handling in this custom setup rather than to issues in the lexer itself.

As such, remaining tests: 5.

Remaining tests: 0 (out of a batch of almost 6792 thousand tests).

I resolved the invalid cases for both DSTRING and SSTRING. When inputs like "abc"" (or the equivalent using single quotes for SSTRING) are encountered, tokenization must not emit any token, mirroring the behavior in ANTLR2 where such constructs are considered invalid. I achieved this by splitting both DSTRING and SSTRING into dedicated lexer modes, similar to the approach used for comments.

Two additional tests were fixed by removing the sign variable from NUM_LITERAL and replacing it with a semantic predicate, in order to avoid state-related issues.

Remaining work includes some refactoring and additional documentation where extra code was required. After that, I’ll commit the version of the lexer on Monday.

I committed the stable lexer as revision 16008.

I corrected newline handling inside both single and double-quoted strings in schema mode, fixed nested block comment handling after identifying a case where comments prematurely stopped token generation, and cleaned up and documented all non-obvious lexer decisions. I also validated the standard.df schema file, the generated tokens now match exactly between ANTLR4 and ANTLR2.

The whole point of developing the ANTLR4 schema parser and the Progress lexer was to test the new annotation AST implementation, which is what I’ve done so far. However, I’m currently stuck on a mismatched input '1252' expecting SYMBOL error. In ANTLR2, this could be handled by modifying the token stream (for example, using setType), but in ANTLR4 this approach is no longer supported..

An idea will be to create a new lexer rule to match any_ridiculous_symbol and emit a SYMBOL token, but here we need to know exactly what we're matching, otherwise regressions can appear.

As discussed in the status meeting today, we must have token type mutability. We rely upon this heavily in the progress parser and elsewhere.

There are 2 strategies to try:

1. If ANTLR4 allows replacing the token class with our own subclass of the ANTLR4 token, then we should use that and provide mutability that way. We used a similar approach in ANTLR2 for putting our own token and AST classes into place.
2. We can implement a wrapper of each lexer that filters the tokens. Each token read can be transformed into our own token class instance and then returned to the caller. This is less nice than the other option.

Actually, things are easier than I initially thought! We just need to cast the token to a CommonToken and then call setType().

Currently, the trailer node isn’t part of the AST. I added it only to prove that the approach described above works:-D

   TRAILER [TRAILER]:23 @17:0
      cpstream [KW_CPSTREAM]:24 @17:0
         1252 [SYMBOL]:25 @17:9

Here we can see that the 1252 NUM_LITERAL was changed into a SYMBOL instead!

The standard.df file now produces a large ASTV4 (without throwing any error!). I need to compare it with the AST generated by our ANTLR2 implementation.

There is one remaining issue in the schema parser, originating from the relex method. For an input like " " (quote space quote), the input is cleaned so that only a single space remains. In this case, nextToken() does not return an EOF token as it did in ANTLR2, but instead returns the WS token itself.

That said, I’ll commit the latest version of the schema parser tomorrow morning, as it already generates identical ASTs for both ANTLR4 and ANTLR2. The only remaining step is to safely ensure that the relex method correctly treats this case as EOF.

After that, what should be the next steps? Do we need to migrate additional classes, and if so, in what order..? At the moment, it’s hard to fully validate the new annotated AST without the rest of the infrastructure in place.

Greg, what would you suggest?

I need to do a full review. Then there will be cleanups. Then I would expect to be able to merge some code into trunk and do a second phase.

I’ve committed the latest version of SchemaParser.g4 as revision 16009.

PS: Another commit may follow before EOD. I’ll see if I can find a proper fix for the failing case described in note #10804-108, and I’d also like to include the .df from a big app and compare the ASTs to ensure everything looks correct there as well. Note that the current revision contains a few TODOs, but they’re not critical, the corresponding method implementations haven’t been required so far. That said, another update may still come.

Committed as revision 16010.

I added a proper fix for the failing case described above (the quote space quote scenario). I also completed the implementations of followedByWhitespace and prefacedByWhitespace.

For testing, I expanded coverage by including additional .df files from multiple projects (two large GUI applications and one additional app). Everything looks good so far, the resulting ASTs are almost identical.

I did encounter a NPE in one large application .df, which turned out to be caused by an incorrect token type in columnLabel_sa. This happened because I reused an existing parser rule but missed updating the matched token type accordingly. That has now been fixed as well.

One minor difference remains, but I don’t think it’s an actual issue. In one case, the string
"£>>>>>>>9-" [STRING]: @1079
appears as
"�>>>>>>>9-" [STRING]: @1079
in ANTLR4. The � character is the Unicode replacement character and indicates a charset decoding mismatch, not a parsing problem. ANTLR4 reads input as UTF-8 by default, whereas some legacy inputs use a single-byte encoding (e.g. ISO-8859-1 or Windows-1252).

If needed, this can be addressed by explicitly specifying the charset when instantiating the CharStream (for example, using CharStreams.fromPath(path, Charset.forName("windows-1252"))), which restores the original £ character and matches the ANTLR2 behavior.

If needed, this can be addressed by explicitly specifying the charset when ...

It is definitely needed. The input file encoding is a configuration value and must be read and honored whenever we read from files. The current ANTLR2 implementation does this.

Code Review Task Branch 10804a Revisions 16004 through 16010 (Part 1)

There is a lot to like about these changes. I am not done with evaluating the code, especially the new grammars. I will post more feedback about the grammars later.

Unfortunately, the AastV4 and AnnotatedAstV4 classes were not sorted the same way as the original versions, so it was much harder to compare the code right now. I did spend a lot of time on comparing the changes in this area.

I understand the initial idea to build a parallel implementation that doesn't change the existing implementation. That approach was useful to prototype things. I think we have gotten to the point where we need to change the approach.

I would like you to please rebase the branch and carefully apply your edits to the existing files. Some of the changes (e.g. "V4" versions of TreeLocal or ShadowNode will just drop away or need minimal changes. The big job is for Aast and AnnotatedAst. In these it is important to minimize the edits and to leave the order of the code as intact as possible so that it is easier to understand the changes.

As part of this shift, I realize this will break things because anything still on ANTLR2 will fail. I think we must live with that for some period of time. Building this completely in parallel only can take us so far. This also means that the grammars in src/com/goldencode/p2j/uast/antlr4/ should be moved back to the original locations (i.e. directly in schema/ or uast/).

I am assuming some of the changes to the AST code are just because of the parallel/prototype approach. For example, the registry ids are being handled with a simplistic AtomicLong ID_GEN. In the real code we will still need to delegate this to the AstManager plugin and support the dual file portion and node portions of the 64-bit ID the same way we do today.

Another missing piece is the abstract design of the AnnotatedAst class where it has concrete implementations in ProgressAst and JavaAst subclasses. We want to maintain that approach.

I like the TokenTypeRegistry concept but the use of it needs to be more efficient. The specific "vocabulary" for any AST is something that is fixed at complication time AND it is strongly associated with the concreate AST type (ProgressAst, JavaAst...). Instead of walking upwards in the tree to find the root and then read the registry member, I would rather just keep a static member in the concreate class and use abstract methods for type access in the AnnotatedAst class.

My biggest concern with the AST approach is the implementation of the children member as a List. I don't think it is feasible to have a List collection instance (or even an array) for every non-leaf AST node. For large projects we will have millions of nodes in memory at one time. That is true for both conversion and for runtime needs (e.g. dynamic queries). This would represent a significant expansion of resources that is probably not something we can do.

The ANTLRv2 approach is much more efficient as a single direction linked list approach. It falls down for random access by index and for backwards traversal, which is used more than one would expect (e.g. anytime one uses the getPrevSibling()). Let's discuss alternatives but at first blush I think a doubly linked list might be a better approach. If you would like to analyze the List (and/or array) approach compared with a doubly linked list, in regard to performance and memory, I'd like to see the results.

Make a proposal and let's discuss this further.

While I continue to review the grammars, would you please prepare a guide for people converting lexers from v2 to v4? You now have a really good understanding of that process. Please document it carefully so that others can help with the lexer conversion.

When I am talking about a doubly linked list, I mean one that we create ourselves using "next" and "previous" AST references within each AST. I am not talking about using a collection class for this purpose.

• first child
• next sibling
• prev sibling

Can you also create a wiki page for the lexer guide, or would you prefer that I post it directly in this task?

Paula Păstrăguș wrote:

Aha, more exactly you're referring to only store 3 references instead of using a collection?

• first child
• next sibling
• prev sibling

Exactly right.

Paula Păstrăguș wrote:

Can you also create a wiki page for the lexer guide, or would you prefer that I post it directly in this task?

See ANTLR Migration Guide.

Thanks! And yes, some of the changes were introduced specifically for the prototype model.

You're absolutely right that, at this stage, we need to proceed somewhat incrementally blind until all related classes are migrated. That process will naturally take some time.

Initially, I assumed we intended to keep the ANTLR2 grammars and supporting infrastructure for compatibility reasons. However, I agree with your point that we should fully discard that approach and move forward with a proper migration to ANTLR4.

For now, our immediate goal should be to get the prototype working in the simplified form you suggested, without relying on collections for child storage. I think that approach makes sense, and it aligns better with the long-term direction. And after that I'll rebase the branch and hopefully I'll apply the changes without breaking something:)

Greg Shah wrote:

See ANTLR Migration Guide.

I'm not authorized to see that page (403 err)

Paula Păstrăguș wrote:

Greg Shah wrote:

See ANTLR Migration Guide.

I'm not authorized to see that page (403 err)

It just needed to be qualified by the project name. Try it now.

Now it's fine, thanks!

I need a bit of guidance regarding the inline shadow nodes. Are we planning to keep/support them?

From what I can tell, this feature looks effectively inactive right now: all the relevant entry points/methods are always invoked with inlineShadows = false If we do want to keep it, could you please clarify the expected behavior and point me to the right way to handle it in the migration?

I committed the first version of migration as rev 16464 (branch was also rebased). (I didn't delete v4 classes yet, but I'll do it as soon as there's nothing that needs to be ported from there)

The approach has a few clear advantages. First, it avoids the overhead of allocating a collection for nodes. However, in most ASTs, the majority of nodes have 0-2 children.It also makes operations like move, remove, or grafting subtrees constant-time, since they only require updating a few references rather than shifting elements inside a collection.

There are some trade-offs as well. The implementation is a bit more complex and requires careful maintenance of the sibling and parent pointers. Bugs can be harder to track if a pointer update is missed or done in the wrong order. Indexed access is also less convenient since retrieving the n-th child requires traversing the sibling chain.

While implementing the pointer-based approach I already ran into a small pointer consistency issue during move. Would it make sense to create a small test suite specifically for tree manipulation operations (graft, remove, move, etc.) to help catch these kinds of pointer issues more easily as we continue working on this?

I also ran the memory benchmark again, and with the custom pointer-based structure (parent, firstChild, tail, prevSibling, nextSibling) the results were:

allocatedParents = 10.8M
allocatedNodes   = 21.3M // 10.8 parents + children (21.3 - 10.8 = 10.5)
usedHeap         ≈ 1GB

Good so far. I'm OK with the reported trade offs.

While implementing the pointer-based approach I already ran into a small pointer consistency issue during move. Would it make sense to create a small test suite specifically for tree manipulation operations (graft, remove, move, etc.) to help catch these kinds of pointer issues more easily as we continue working on this?

Yes, you can go ahead with that so long as it doesn't take too long. I expect the problems of stablizing this will be mostly up front.

Paula Păstrăguș wrote:

I need a bit of guidance regarding the inline shadow nodes. Are we planning to keep/support them?

From what I can tell, this feature looks effectively inactive right now: all the relevant entry points/methods are always invoked with inlineShadows = false If we do want to keep it, could you please clarify the expected behavior and point me to the right way to handle it in the migration?

We use inline shadow nodes when we read templates and instantiate AST nodes as a result. At this time, I think this is only actively used for "progress to progress" mode. This would be stuff transformed using ProgressTransformDriver and reading templates from rules/progress/progress_samples.tpl. That flag is true for at least the example in rules/progress/rewrite_function_signature_2.xml.

The output seems consistent. I haven't checked the behavior of the specific methods versus their original meaning in the ANTLRv2 AST implementation. Are the semantics of setNextSibling(null) and setFirstChild(null) the same in our ANTLRv4 implementation as they were in ANTLRv2?

Yes, I kept the same behavior, even though it feels a bit awkward.. Currently, setFirstChild removes all existing children of the parent AST and keeps only the node passed as the parameter. Same happens with the setNextSibling, it removes all the sibling from the right of this AST. I think we can keep this for now, and improve it later.

Agreed. Please add some comments to both of those methods to explain this non-intuitive behavior AND to highlight that it is an area for improvement later.

Greg, I’m not entirely sure what the best approach is for migrating all the related classes to rely on ANTLR4 instead of ANTLR2. I tried several things, removing parts that are not yet migrated, commenting out code, and adding some missing methods, but every time I try to compile the FWD sources I run into a large number of errors. There are still many parsers and lexers (such as ProgressParser and others) that haven’t been migrated yet, and this creates a lot of inconsistencies.

My initial idea was to temporarily remove or disable the pieces that we haven’t migrated yet, just to see if the first step of the pipeline still 'works'. Specifically, I wanted to check whether the root AST can be generated from the .df files using the new infrastructure (the new ProgressLexer and SchemaParser). Unfortunately, there are still too many compilation errors and I wasn’t able to get through all of them.

At this point I feel a bit stuck and I’d really appreciate your advice on what the best strategy would be. One approach I attempted was to reduce FWD to a minimal version (by temporarily removing some classes) just to verify that we can get through the first step in the TransformDriver, meaning reaching the runSchemaLoader stage. Ideally, we should at least be able to reach this point:

SchemaLoader root = postProcessImport(root, metaAst, paths);

I realize that some of the post-processing there still depends on the ProgressParser, but at least the root AST should already be generated by that point.
After that, once the first step in TransformDriver is working properly, we can start adding the removed pieces back gradually, one by one..

The good news, at least from my perspective, unless you see it differently, is that in theory (as soon as I'll be able to compile the sources without getting any errors) we should be able to test this approach using only our new schema parser, the progress lexer, and our new Annotated AST for the first step in TransformDriver.front.

For early testing you could focus on getting just enough of the things compiled to run SchemaLoader. I wouldn't worry about getting the TransformDriver running.

After that, I do think you need to get more of FWD in place. Any grammar that does not need to generate an AST can remain as is in ANTLR2. I would think that would be e4gl.g, text.g, braces.g. I agree with the idea to bypass anything related to unmigrated grammars that rely upon AST processing. You can comment out, bypass or stub out methods and otherwise try to get things to compile. Migration of expression.g, fql.g and hql.g should probably be next. Then we can work on the progress.g parser. After that we handle the expression_evaluator.g replacement which will probably be a hand written AST walker.

At that point, we should be able to get all of FWD runinng and tested.

We can then migrate e4gl.g, text.g and braces.g.

I think I've managed to decouple the ANTLR4 logic from the existing ANTLR2 logic, and I finally got fwd to compile. I'll share more details about how I approached it and which path I chose.
But before that, I need to sort out a few errors that appear when importing the schema through SchemaLoader.

🚨 I managed to obtain the schema root AST using our new schema parser and Progress lexer, with a few local changes compared to the latest committed revision. (I have not compared the ASTs generated by ANTLR2 and ANTLR4 yet, but I will do that next.)

At this point, I still need to assign the AST IDs properly, because the committed solution is not correct yet. I talked a bit with Danut, and he clarified some aspects for me.

I compared the ASTs and they appear to be correct.. The only issue I noticed is that the type is not resolved properly. It seems that the new implementation is still relying on the ProgressParserTokenTypes interface in some way. However, ANTLR4 no longer generates the token types interface automatically, so I had to create one manually and replace all the classes that implemented ProgressParserTokenTypes with the new one I created. Even so, this did not resolve the type issue, so I will continue investigating it.

That said, once this problem is solved, I don't think we will need to worry about SchemaLoader anymore:) At the moment, it appears to be doing its job just fine.

also solved the id issue, now they are exactly the same as the ones generated by antlr2.

Paula Păstrăguș wrote:

I'll share more details about how I approached it and which path I chose.

I managed to get FWD up and running by separating the ANTLR2 classes from the ANTLR4 ones. For now, it is easier to keep the *V4 classes and gradually adapt components such as SchemaLoader. In practice, this required only a few replacements, for example using ProgressLexerV4 instead of ProgressLexer (yes, I renamed it because there were some duplicates errors and it was easier to work with different names).

We still need to keep both versions for the moment so we can progressively test and validate the approach. I'm aware that this will require additional cleanup later, such as renaming the files and replacing usages like AastV4 with Aast. However, most of that work should amount to straightforward find-and-replace all operations, which I don't expect to be very time-consuming.

Also, I created new V4 classes instead of modifying the existing ones. The V4 versions started as copies of the original classes (for example, Aast), and then I applied the changes required for ANTLR4. The same approach was used for classes such as AnnotatedAst and others, mainly to keep the method order and overall structure consistent with the originals.

There were also some build issues: classes were duplicated because ANTLR4 creates a .antlr folder (hidden folder) where it generates / copy the Java files again. As a result, the same classes (here I'm referring to the SchemaParserV4.java and ProgressLexerV4.java) existed in two locations, and I had to delete the duplicates manually to make the build work again.

Paula Păstrăguș wrote:

The only issue I noticed is that the type is not resolved properly. It seems that the new implementation is still relying on the ProgressParserTokenTypes interface in some way.

Actually the dump tree is the culprit here. It still uses old ProgressParser.lookupTokenType, lookupTokenName and getSymbolicTokenName.

Managed to fix this. It turned out not to be very difficult, since the ANTLR4 lexer provides a Vocabulary along with several utility methods, specifically getDisplayName(type), getLiteralName(type), getMaxTokenType(), and getSymbolicName(type). These are sufficient to implement our token lookup methods.

I also compared the ASTs produced in SchemaLoader.importSchema (for standard.df), now that the token types are resolved correctly, and they appear to be identical. I had to inspect the meld carefully because the annotations were not in the same order between trunk and 10804a, which is expected since they are stored in a map that does not guarantee ordering.

Greg, I'm not able to commit the changes. I keep getting Could not request local forwarding. Connection reset by peer errors..

Committed as rev 16465. Decoupled antlr2 logic from the antlr4 logic. (Also refined schema parser and the progress lexer.)

Next step: hql.g migration - wip

I have completed the migration of the HQL parser and lexer. I also did some initial testing of the grammar by comparing the ASTs produced in FqlPreprocessor.preprocess.

To do this, I created a custom parse method that uses the new lexer and parser. After that, I performed some smoke testing in Counteract and dumped the generated trees into two separate files. I then used meld to compare them and verify whether the ASTs match, and they do! 🥳

I will continue testing the HQL grammar, and if everything looks good, I expect to commit both the lexer and the parser by the end of the day.

Greg, please let me know which grammar I should focus on next.

fql.g

Committed as rev 16466. (the hql parser and grammar)

I could not find any cases where the HQL grammar produces different results compared to the ANTLR2 version. If there are specific scenarios you would like me to test, please let me know. I also started the migration of FQL (the lexer is already done).

Finished migrating the FQL parser. I also ran some tests using the existing FqlToSqlConverter. I'll continue testing, and if no issues appear, I plan to commit the FQL lexer and parser by the end of the day.

Greg, please let me know which grammar I should work on next.

com/goldencode/expr/expression.g

Committed the FQL lexer and parser as rev 16467.

I compared the AST produced and later manipulated in FqlToSqlConverter (in the preprocess method) using the same approach I used for HQL: smoke testing Counteract and dumping the trees into two separate files for each version (ANTLR2 and ANTLR4). The results appear to match:)

I also started the migration of expression.g

Finished migration of expression.g. I did some testing and found some differences that I believe are caused by the method parser rule. I'm currently investigating the issue.

Fixed it:-D

I'll continue testing, and if no issues arise, I'll commit the expression parser and lexer tomorrow morning.

Greg, please let me know what will be the next grammar.

It is now time for the Progress Parser. 😱

Should be done before coffee ☕ gets cold next morning 😄

Fearless and confident! Let's go!

Committed the migrated lexer and parser for expression.g as rev 16468.

I started working on the Progress parser, and I'm seeing that hidden tokens are used quite a bit in methods such as followedByWhitespace, checkExtraSpace, and likely others. I'm considering whether we could keep these tokens on the default channel initially, and then move them to the hidden channel manually at a later stage. (EDIT: if it will be the case..)

That approach would let us preserve the existing shadow-node logic, where the node is annotated with "original-token":

      // temporarily cache the original token until shadow node processing can occur
      if (token instanceof ManagedHiddenStreamToken)
      {
         this.putAnnotationImpl("original-token", token);
      }

The only adjustment would be that, instead of checking token instanceof ManagedHiddenStreamToken, we would need to identify it based on the token type. (EDIT: The token type is not sufficient, we need to 'mark' the tokens somehow, because there are backslashes that needs to be hidden or NOT.)

I started working on the Progress parser, and I'm seeing that hidden tokens are used quite a bit in methods such as followedByWhitespace, checkExtraSpace, and likely others.

Please re-read my post in #1757-19 about island grammars and how we may be able to eliminate some nastiness in our current ANTLRv2 approach. The reason why we are doing a lot of checking of whitespace is because the v2 lexer could not shift modes. We should look to eliminate these cases by enhancing the lexer to more naturally create the stream of tokens instead of having the parser rewrite the token stream.

Let's see how far we get by fixing this.

Hm.., so are we aiming to eliminate all of the parser-side token stream surgery?

Hm.., so are we aiming to eliminate all of the parser-side token stream surgery?

At least the forms I describe in that post.

The case where we rewrite the token type based on parser context would be expected to remain.

Will be enough to set the channel to HIDDEN_CHANNEL for the old hide() method?

Note to myself: investigate whether there are any differences between a token whose channel is set directly in the lexer and one whose channel is modified later in the parser.

Will be enough to set the channel to HIDDEN_CHANNEL for the old hide() method?

I doubt it.

Here is the code I was remembering when we spoke on the call today:

   /**
    * Modify the state of this token and the hidden/non-hidden tokens before and after so that
    * this token appears as if it was always a hidden token.  This is designed to be called from
    * a parser and is meant to be used on tokens that were matched by the parser and which will
    * be dropped from the tree.  Without this "patching" method, the tokens will be lost and that
    * means the original text of the source file will be incomplete.  This has implications for 
    * downstream transformations and anti-parsing, which are resolved by making the tokens
    * hidden.
    * <p>
    * Do NOT call this on an already hidden token.  Do NOT call this on the EOF token.
    */
   public void hide()
   {
      ManagedHiddenStreamToken hiddenB4    = (ManagedHiddenStreamToken) getHiddenBefore();
      ManagedHiddenStreamToken hiddenAfter = (ManagedHiddenStreamToken) getHiddenAfter();

      // left side
      if (prevNonHidden == null)
      {
         // "this" is the first non-hidden token in the file
         if (hiddenB4 == null)
         {
            // "this" is the first text in the file, there are no fixups needed for the left side
         }
         else
         {
            // there is existing hidden text at the beginning of the file, link "this" as a
            // hidden token
            hiddenB4.setHiddenAfter(this);
         }

         // fixup non-hidden links
         if (nextNonHidden != null)
         {
            // instead of pointing to "this", it will know that it is the first non-hidden token
            // in the file
            nextNonHidden.setPrevNonHidden(null);
         }
      }
      else
      {
         // there is non-hidden text before "this" 
         if (hiddenB4 == null)
         {
            // there is no existing hidden token between "this" and the prior non-hidden token
            // link "this" as a hidden token
            prevNonHidden.setHiddenAfter(this);
         }
         else
         {
            // there is existing hidden text between "this" and the prior non-hidden token, just
            // add to the end of that chain
            hiddenB4.setHiddenAfter(this);
         }

         // fixup non-hidden links
         if (nextNonHidden != null)
         {
            // instead of pointing to "this", it will point to our previous non-hidden node
            nextNonHidden.setPrevNonHidden(prevNonHidden);
         }
         // whether or not it is null, the previous non-hidden node now points to what "this" 
         // considered as the next non-hidden node
         prevNonHidden.setNextNonHidden(nextNonHidden);
      }

      // right side
      if (nextNonHidden == null)
      {
         // "this" is the last non-hidden token in the file (this should be EOF)
         if (hiddenAfter == null)
         {
            // "this" is the last text in the file, there are no fixups needed for the right side
         }
         else
         {
            // there is existing hidden text at the end of the file, link "this" as a
            // hidden token
            hiddenAfter.setHiddenBefore(this);
         }

         // fixup non-hidden links
         if (prevNonHidden != null)
         {
            // instead of pointing to "this", it will know that it is the last non-hidden token
            // in the file
            prevNonHidden.setNextNonHidden(null);
         }
      }
      else
      {
         // there is non-hidden text after "this" 
         if (hiddenAfter == null)
         {
            // there is no existing hidden token between "this" and the next non-hidden token
            // link "this" as a hidden token
            nextNonHidden.setHiddenBefore(this);
         }
         else
         {
            // there is existing hidden text between "this" and the next non-hidden token, just
            // add to the beginning of that chain
            hiddenAfter.setHiddenBefore(this);
         }

         // fixup non-hidden links
         if (prevNonHidden != null)
         {
            // instead of pointing to "this", it will point to our next non-hidden node
            prevNonHidden.setNextNonHidden(nextNonHidden);
         }
         // whether or not it is null, the next non-hidden node now points to what "this" 
         // considered as the previous non-hidden node
         nextNonHidden.setPrevNonHidden(prevNonHidden);
      }
   }

This is fixing up both the visible and hidden token streams. We are using it to look between tokens to "see" hidden tokens and from hidden tokens to find the non-hidden tokens that are adjacent. Unless the channels behavior is the same, we will likely need to find a better way to do this.

Used this grammar to see what happens when we hide a token in antlr4 using setChannel(Token.HIDDEN_CHANNEL):

grammar ParserHideTest;

prog
    : rule_* EOF
    ;

rule_
    : i=ID
      {
         ((CommonToken) $i).setChannel(Token.HIDDEN_CHANNEL);
      }
      s=SEMI
      {
         List<Token> hidden = ((CommonTokenStream)_input).getHiddenTokensToLeft($s.getTokenIndex());
         if (hidden == null) 
         {
            System.out.println("bad luck, no hidden tokens");
         } 
         else 
         {
            for (Token t : hidden) 
            {
               System.out.println(
                  "text=" + t.getText() +
                  ", index=" + t.getTokenIndex() +
                  ", channel=" + t.getChannel()
               );
            }
         }

         Token prev = _input.LT(-2);

         if (prev == null) 
         {
            System.out.println("No previous default-channel token");
         } 
         else 
         {
            System.out.println(
               "prev default: text=" + prev.getText() +
               " idx=" + prev.getTokenIndex() +
               " ch=" + prev.getChannel()
            );
         }
      }
    ;

ID   : [a-zA-Z_][a-zA-Z_0-9]*;
SEMI : ';';
WS   : [ \t\r\n]+ -> skip;

input = foo; bar;

• Whether the identifier appears in getHiddenTokensToLeft(idx)

• Whether default-channel navigation using LT(-2) still behaves correctly

Results:

text=foo, index=0, channel=1

No previous default-channel token

text=bar, index=2, channel=1

prev default: text=; idx=1 ch=0

According to the results, getHiddenTokensToLeft behaves correctly, it includes the ID token that was just moved to the hidden channel. LT(-2) no longer sees the ID token. After moving foo ID to the hidden channel, the identifier is completely ignored by LT(-2). After bar ID is moved to the hidden channel, when the second semicolon is matched, LT(-2) correctly sees the previous token from the default channel, aka the first semicolon (idx = 1), it does not wrongly return the bar ID.

setChannel(Token.HIDDEN_CHANNEL) likely does everything needed: the tokens aren't discarded, they're simply routed into the HIDDEN_CHANNEL, and their internal 'connections' are fixed.

OK, that would be a good result. The final exam will be if ProgressTransformDriver works properly.

Just a quick update: I managed to get the Progress Parser class close to compile, but there are still quite a few incompatibilities caused by the remaining V4 classes. Those should be resolved once I start removing the V4 classes and properly aligning everything. At this point, we essentially have a Progress parser that should compile cleanly once everything is fully integrated. I've already fixed a large number of issues from both the generation and compiling phases, and the only ones left seems to be related to the V4 classes.

Ah, one more thing worth mentioning: I haven't tackled the lexer-side parser surgery yet, but I did find some equivalent approaches for things like mergeUntilWs and similar logic. For now, I want to first make sure the parser is functional, and only after that I'll look into properly moving those parts into the lexer, where they actually should belong.

I finally managed to get a version of trunk that compiles. Now I'm trying to figure out why the transform driver is throwing errors, they keep shifting as I fix one issue, which is good progress. Still, it's pretty hard to trace where the problem actually originates inside the progress parser.. But at least I'm able to test it:)

Current failing test:

def new global shared temp-table fwd-embedded-window
   field pname as char
   field phandle as handle
   field menubar as handle
   field toolbar as handle
   field isAdm2 as logical
   field pages as logical extent 100
   field toolbars as char extent 100.

The issue here lies into the symbol rule. Both, menubar and toolbar should be parsed as SYMBOLs rather than KW_MENUBAR and KW_TOOLBAR, as per current behavior. I'm looking for a fix.

In the ANTLR2 version, this kind of situation was handled by mutating the token type inside the parser rule. That approach no longer works in ANTLR4. The reason is that ANTLR4 performs its prediction before entering the rule, using adaptivePredict, so by the time the rule is executed, the decision has already been made based on the original token type. Changing the token type at that point is simply too late to influence parsing.

I tried to work around this by forcing ANTLR4 to introduce an adaptivePredict earlier on top of the symbol parser rule. The idea was to 'feed' the prediction algorithm with the 'updated token type' by adding a semantic predicate that is always false (actually if there is at least a semantic predicate an adaptivePredict will be generated), effectively trying to trick the parser into using the full prediction mechanism again. However, it does not actually fix the issue, and it comes with a visible performance cost.

This aligns very well with what Sam Harwell describes here https://groups.google.com/g/antlr-discussion/c/ynvgmjjsZTw He emphasizes that semantic predicates can disable DFA optimizations and force the parser into slower execution paths. In other words, instead of helping, this kind of workaround pushes the parser into exactly the kind of expensive behavior we want to avoid. He also points out that context-sensitive decisions, where the same token sequence can lead to different interpretations, are one of the main sources of performance problems. This situation is a direct example of that: menubar and toolbar can be either keywords or variable names depending on context, and the parser cannot decide early which interpretation is correct.

The more important takeaway from his explanation is that these problems should not be solved by trying to influence the prediction engine. The correct approach is to refactor the grammar so that the parser can make decisions as early and as deterministically as possible. In ANTLR4, that also means ensuring that token types are correct at the lexer level..., rather than attempting to fix them later in the parser, as per current behavior.

At this point, it looks like the root cause is systemic rather than isolated. There are likely many identifiers in the language that overlap with keyword names, so relying on strict keyword tokenization in the lexer is problematic. This suggests that the current approach of emitting KW_* tokens for such words may need to be reconsidered.

One direction I am considering is to relax the lexer so that these tokens are produced as generic identifiers (aka SYMBOLs), and then handled contextually in the parser. Another possibility would be to allow these keyword tokens to also be accepted in identifier positions, but that still risks ambiguity and may not scale well. (meaning that instead of matching symbol, we would match the actual tokens instead of updating the token type to be a symbol, then match)

So far, the attempts to fix this at the parser level have not been successful:(( and have introduced performance issues, which is consistent with Sam's warning about predicates and complex prediction paths. The remaining viable direction seems to be a redesign at the lexer level (it may be the time to use lexer modes again), but I'm still exploring how best to approach that without breaking existing grammar expectations.

I also came across something quite interesting: a profiler for ANTLR4 grammars. It could help us pinpoint performance bottlenecks in the grammar, though I still need to search more for this one.

Any input on this would be very helpful. Thank you!

Yes, it is a systemic issue with the 4GL language design itself. This is not something we can control.

The more important takeaway from his explanation is that these problems should not be solved by trying to influence the prediction engine. The correct approach is to refactor the grammar so that the parser can make decisions as early and as deterministically as possible.

Yes, where this is possible and we don't already do this. I don't think there will be many places like that.

In ANTLR4, that also means ensuring that token types are correct at the lexer level..., rather than attempting to fix them later in the parser, as per current behavior.

Shifting everything to symbols won't work.

We care about performance but we also must be able to separate concerns to make the lexer and parser less complicated and easier to implement properly. To date, we've ensured that the lexer is able to resolve keywords and this eliminates a lot of complexity from the parser. There are fewer places where we have to coerce a keyword back into a symbol in the parser than where we would have to coerce a symbol into a keyword to match in the parser. In fact, it seems to me that this will create the same problem but in a much worse/more pervasive form. The parser MUST be able to match using keywords and if those keywords are not resolved in the lexer, then the parser will be a huge mess. I don't think this idea can work.

Greg Shah wrote:

Yes, it is a systemic issue with the 4GL language design itself. This is not something we can control.

The more important takeaway from his explanation is that these problems should not be solved by trying to influence the prediction engine. The correct approach is to refactor the grammar so that the parser can make decisions as early and as deterministically as possible.

Yes, where this is possible and we don't already do this. I don't think there will be many places like that.

Where it is possible, yes, but in this particular case I don’t immediately see a refactoring that would help. The core issue is that the temp-table definition allows an arbitrary number of fields, so the antlr4 syntax effectively looks like ( ... )*. The difficulty comes from the fact that symbol matching happens inside this loop. A similar situation (not with a loop) was handled in the schema parser, if I remember correctly, by changing the token type earlier using lookahead based logic + checks. However, the presence of the loop makes things more complicated here, since the parser has to repeatedly re-evaluate the same ambiguity. I’ll need to investigate further and try to find an approach that works specifically for this pattern.

In ANTLR4, that also means ensuring that token types are correct at the lexer level..., rather than attempting to fix them later in the parser, as per current behavior.

Shifting everything to symbols won't work.

We care about performance but we also must be able to separate concerns to make the lexer and parser less complicated and easier to implement properly. To date, we've ensured that the lexer is able to resolve keywords and this eliminates a lot of complexity from the parser. There are fewer places where we have to coerce a keyword back into a symbol in the parser than where we would have to coerce a symbol into a keyword to match in the parser. In fact, it seems to me that this will create the same problem but in a much worse/more pervasive form. The parser MUST be able to match using keywords and if those keywords are not resolved in the lexer, then the parser will be a huge mess. I don't think this idea can work.

Yes, I agree with that. My suggestion was not to shift everything to symbols, but rather to ensure that the lexer emits the correct token from the start, instead of relying on token type changes later in the parser. In other words, when KW_MENU_BAR is actually intended, the lexer should emit that token, and when it is meant to be an identifier, it should emit SYMBOL. That would avoid the need for post-processing or coercion in the parser.

That said, I realize this is likely the most complex approach, because it would require introducing additional context into the lexer, which is not straightforward. It’s probably the cleanest solution conceptually, but also the hardest to implement correctly.

Another aspect worth mentioning is related to the testing phase. If we move toward emitting tokens differently in the lexer, it raises the question of how we can reliably validate that the correct tokens are produced, without having to manually verify each case. With the current approach, we already have a baseline (antlr2 tokens), which makes it much easier to compare outputs and detect regressions.

Changing this behavior would make validation more difficult and could introduce subtle, hard to detect issues. In that sense, it might actually increase the risk of hidden bugs.. For now, refactoring the grammar seems to be the safer and more practical direction. I’ll continue exploring whether this specific case can be handles through refactoring + other tricks.

I’ll also do some additional research to see what can be done specifically for this test case. I haven’t committed the Progress grammar yet, but for example I’ve started using @init {} and @after {}, which are equivalent to the first and last actions. I didn’t use these in the schema parser earlier because I wasn’t aware of them, and they are only mentioned a handful of times in the ANTLR4 book.

What I’m trying to point out is that there may be hooks or mechanisms that are either undocumented or not very well documented, which could potentially help in cases like this. I’m hoping to identify such options and see if they can be used here.

My suggestion was not to shift everything to symbols, but rather to ensure that the lexer emits the correct token from the start, instead of relying on token type changes later in the parser. In other words, when KW_MENU_BAR is actually intended, the lexer should emit that token, and when it is meant to be an identifier, it should emit SYMBOL. That would avoid the need for post-processing or coercion in the parser.

Yes, I'm open to this. It will likely require using lexer modes. The success of this will depend on whether there is any buffered tokens (lookahead text in the lexer that has already been converted to tokens) that have to be dealt with when switch modes. I think the parser will likely have to switch the lexer mode with no buffered tokens. What I mean by this is that we will not be able to do such a switch in advance but will have to do it to match the very next token (i.e. shift into "symbol" mode for the next token).

I checked to see how many buffered tokens antlr4 tend to gather, here are some results:

• tested for the failing test, checked the values inside define statement rule.
  

       [java] Current index: 151
       [java] Buffered tokens: 249
  

• tested for the fwd-embedded-driver file, checked the values inside define statement rule.
  

       [java] Current index: 151
       [java] Buffered tokens: 11296
  

It means that antlr4 impl buffers much more than one might expect, so parser controlled lexer mode it's not a solution, unless I misunderstood the idea ( aka to pushMode from inside the parser when we have enough context about the next token). The buffering itself does not kill the idea, but it does make the stale token problem very plausible.

The tests above were done with BufferedTokenStream. One possible direction would be to try UnbufferedTokenStream, but that changes the behavior in an important way: the parser also starts seeing hidden channel tokens during parsing. In practice, that means tokens such as comments and whitespace are no longer transparently filtered the same way they are with the buffered setup, so this is not just a buffering change, but a parsing-behavior change as well.

An important detail here is that the old ANTLR2 parser also included hidden tokens, so from that perspective the unbuffered approach may be closer to the old behavior.

In ANTLR2 we introduced a very explicit k value that limited the number of tokens of lookahead. In the lexer it was set to 4 and in the parser it was set to 3. That should be all that is needed to resolve ambiguity.

Without a k value, ANTLR might lookahead more than those amounts but there would never be a reason to look ahead 11296 tokens or even 249. I don't understand what you are measuring here. It can't be the already resolved tokens. Is this a Schrödinger's cat situation? Could your observation of the buffered tokens have changed the buffered tokens?

The measurement is about token stream buffering, not actual ANTLR prediction depth.. I'll see how can I measure how many tokens ANTLR needs to decide.

Here's the code:

CommonTokenStream tokens = getInputStream();

int currentIndex = tokens.index();
int bufferedSize = tokens.getTokens().size();

System.out.println("Current index: " + currentIndex);
System.out.println("Buffered tokens: " + bufferedSize);

Here's the code:

If it is lexing all the way to the end of the file independently of the parser, that is not OK. We already know that we need to be able to use lexer modes to solve some of our lexing problems like OS-COMMAND. The lexer should only be processing enough tokens to fulfill the parser's need for lookahead.

I'll investigate this next and try to confirm whether the lexer is actually tokenizing ahead to the end of file, or whether something in the token stream ( if tokens.fill() is used, those values make sense, but I did not use the fill method at all ) is forcing that behavior.

The getTokens method is not the culprit here. I did some debugging with a very simple testcase like:

PAUSE 3.
PAUSE 3.
PAUSE 3.
...
PAUSE 3.
PAUSE 3.
PAUSE 3.

However, the values I observed earlier (249, 11296) are actually more or less accurate.

What seems to be happening is that inside the def_var_stmt parser rule there is some ambiguity, and because of that the parser cannot confidently choose a path. As a result, the lexer ends up fetching a large portion of the input ahead of time. In fact, by the time the first DEFINE VARIABLE is reached, the lexer has already consumed essentially all tokens, which is clearly not desirable.

So this is not a case of 'measurement affecting the system' (sorry, Schrödinger :-), but rather a real behavior caused by ambiguity in the grammar. Next, I'll see if I can find the ambiguity.

Regarding the repeated token fetching triggered from def_var_stmt, and more specifically around as_clause, my current impression is that the real issue may come from the reserved_or_symbol rule being used down there. Because of that, the behavior feels somewhat off: the excessive lookahead may be more of a consequence of the current symbol handling than of as_clause itself..

This is also why I keep wondering whether lexer modes could help here. In theory, they might allow us to emit the correct token. But I still do not see clearly how that could be applied in practice. It would likely require some custom Java code to switch modes, but then the question becomes where exactly that should happen.

And that leads to the main difficulty: if the parser only realizes inside rule x that a different lexer mode would have been needed, but by that point the lookahead has already gone too far and the token has already been emitted, then switching modes there would not be a good idea.

I commented out the problematic adaptivePredict, and it seems to resolve the issue of fetching tokens on and on. I'll look futher to see if there is a way to disable this thing.

You should debug into adaptivePredict() in that broken case and see why it goes wrong. It is best to know why now, even if disabling it is right in this case.

I'm guessing that lexer mode switches reset the token stream (and should flush the buffered tokens) otherwise mode switching would never work for any cases.

I think I found the root cause! 🥳 Let me break it down for you.

The flow for a DEFINE statement currently looks like this:
define_stmt (where the terminator DOT is handled, this is important!) -> def_var_stmt -> [..] ( [...] | as_clause | [...] )*.

The issue appears during adaptivePredict, which starts from the KW_AS token and tries to determine the correct closure. It fails to do so because the terminating DOT is not inside that loop or at least into the parent parser rule.

By moving the DOT matching directly into def_var_stmt, right after the loop, the parser can properly detect the closure boundary. This change prevents the parser from continuously consuming tokens and effectively resolves the issue of tokens being fetched indefinitely!

Current failing testcase:

if valid-handle(fwd-driver-handle)
then do:
   /* if the driver was already loaded, don't run the external program code.  the APIs, even if
      invoked via this instance, it will re-use the globally shared temp-table */
   return.
end.

I've also implemented a temporary workaround for the symbol rule: I added | KW_MENU_BAR | KW_BUTTON to allow ANTLR to enter that parser rule. Anyway, these tokens are treated as symbols by changing their types to SYMBOLs into the init action , which is acceptable for now until we identify a more robust solution.

I also noticed that inline comments such as // placeholder were not being routed to the hidden channel, and I've corrected that behavior too.

At this point, I'm continuing the investigation using the test case above.

PS: My goal is to get the fwd-embedded-driver converted correctly. I've made progress up to line 139 so far, where the failing test case mentioned above occurs.

Please check in all your changes, even if everything is broken. I can't see any of your parser work.

Would ProgressParser.g4 be sufficient? I do have a some additional changes as well, but those are currently in a trunk-based project rather than directly on 10804a.

If you want to test the parser I think I can upload my testing project on devsrv, tmp directory.

I don't understand how adaptivePredict() can be so stupid. From the original progress.g, that loop can only match these 37 possible tokens in LA(1):

• KW_AS from as_clause
• KW_LIKE_SEQ or KW_LIKE from like_clause
• KW_EXTENT from extent
• KW_INIT from initializer
• KW_SERIALZN from serialize_name
• KW_LABEL from label
• from ui_stuff
  • KW_BGCOLOR
  • KW_DCOLOR
  • KW_FGCOLOR
  • KW_PFCOLOR
  • KW_FONT
  • KW_CTX_H_ID
  • KW_COL
  • KW_COLUMNS
  • KW_ROW
  • KW_MOU_PTR
  • KW_COL_BGC
  • KW_COL_DC
  • KW_COL_FGC
  • KW_COL_FONT
  • KW_COL_PFC
  • KW_LAB_BGC
  • KW_LAB_DC
  • KW_LAB_FGC
  • KW_LAB_FONT
  • KW_LAB_PFC
  • KW_ACCEL
  • KW_WID_ID
• KW_FORMAT from format_string
• KW_COL_LAB from column_label
• KW_DECIMALS from decimals_clause
• KW_DROP_TAR
• KW_NOT or KW_CASE_SEN from case_sensitive
• KW_NO_UNDO
• KW_VIEW_AS from view_as_phrase
• KW_TRIGGERS from trigger_phrase

There is no ambiguity and the lookahead is only 1 token ever. The exit condition for the loop is simply: is the next token one of these 37 types?

Why am I complaining here? Because we have this kind of construct all over throughout the parser and if the ANTLR4 default behavior is really that stupid, then we will have to make edits all over the place. On top of that, it will make maintenance of the parser more work, forever.

If this is "working as designed", then can we disable it? Even from just a performance perspective this approach makes no sense.

Paula Păstrăguș wrote:

Would ProgressParser.g4 be sufficient? I do have a some additional changes as well, but those are currently in a trunk-based project rather than directly on 10804a.

I'd like to see the whole branch. Is your trunk branch a superset of 10804a? Worst case, why not just create a 10804b with that content?

Before going further, I think you should come up with a solution for the symbol problem. That is crucial for the parser.

my testing project is based on trunk 16493 + 10804 changes. What if I overwrite the current version of 10804a with this version?

Greg Shah wrote:

I don't understand how adaptivePredict() can be so stupid. From the original progress.g, that loop can only match these 37 possible tokens in LA(1):

[...]

Right! Based on the algorithm, no closure is detected until all tokens have been consumed. In practice, it never returns to define_stmt to match the terminating DOT, instead, adaptivePredict continues consuming tokens from within def_var_stmt.

Can we disable it?

I think by using parser.getInterpreter().setPredictionMode(...), but I need to see what are the available prediction modes.

Logically, it is an incorrect result, right? Besides being stupid it is broken. Not being able to exit the loop is a bug.

yes..
Here are the prediction modes: LL, LL_EXACT_AMBIG_DETECTION and SLL

However, I don't think adaptivePredict itself can be avoided. What we may be able to change instead is the prediction mode behind it, such as SLL, LL, or LL_EXACT_AMBIG_DETECTION.
I'm trying with each of them to see which one is better for our needs.

Neither prediction mode solved the token fetching issue after I reverted my fix. But let me surprise you even more: making the DOT matching group optional also resolves the problem. I can't explain how it that possible.

Here's an article about adaptive LL(*): https://www.antlr.org/papers/allstar-techreport.pdf I'll look into it.

Please provide a summary description of each of these prediction types. Also, what is the behavior of setting this value? Is it active for all following processing or is it scoped to a rule (and subrules) or something else?

• SLL is fast but context insensitive and it may fail on complex grammars
• LL is a bit slower, but accurate as it does allow full parser context
• LL_EXACT_AMBIG_DETECTION same as LL, but it also detects exact ambiguities.

The setting is global. After the parser is instantiated, that value can be set. By default antlr4 start with SLL because is faster, if that fails it retries with LL.

Let me create a short grammar to see if this is clearly an antlr4 bug or not. (I'm referring to tokens fetching)

• SLL is fast but context insensitive and it may fail on complex grammars
• LL is a bit slower, but accurate as it does allow full parser context
• LL_EXACT_AMBIG_DETECTION same as LL, but it also detects exact ambiguities.

Please describe how the algorithms work (at a high level).

I'll take care of it once the article has been read.

I especially would like to understand how they compare to the ANTLR2 LL(k) approach.

I did some additional debugging, and I'm starting to think this may not actually be an ANTLR4 issue, but rather a problem on my side.

My initial assumption was that the issue came from the symbol rule used downstream in def_var_stmt. That rule is involved, but after digging further, the actual problem seems more likely to be in reserved_or_symbol. My current understanding is that, when ANTLR4 computes the reach set, it also considers the surrounding possibilities, and this rule may be widening that set more than expected.

reserved_or_symbol returns [Aast node]
   @init
   {
      int oldtype = -1;

      if (_input.LA(1) != SYMBOL)
      {
         oldtype = _input.LA(1);
         ((CommonToken) _input.LT(1)).setType(SYMBOL);
      }
   }
   :
      ( 
           { _input.LA(1) >= BEGIN_RESERVED && _input.LA(1) <= END_RESERVED }?
           r=.      { $node = make($r); } // <---------- CULPRIT!
         | 
         s=symbol { $node = $s.node; }
      )
      {
         // save off original type from before token rewriting
         if (oldtype != -1)
            $node.putAnnotation("oldtype", Long.valueOf(oldtype));
      }
   ;

The r=. alternative may be the real culprit, since it allows any token to be matched. I'm still not completely certain, because as_clause should not be able to reach reserved_or_symbol directly in this case. However, I suspect it may still be pulled into the reach set by ANTLR4's prediction algorithm.

Sorry for all the uncertainty and the repeated 'may', and apologies for the confusion.

I committed my test project as rev 16494 (is based on 16493 + 10804a changes).

You should now be able to see the latest version of ProgressParser.g4, along with the other grammars. Please note that the v4 classes are no longer present, as I've moved everything to the correct naming. Also.. don't try running gradlew all, it will explode, in my local setup it worked, maybe I forgot to commit the build changes, I need to check, anyway that will be done on Monday.

Hmmm. We can't use the r=. approach for cases like this. The new code:

      ( 
           { _input.LA(1) >= BEGIN_RESERVED && _input.LA(1) <= END_RESERVED }?
           r=.      { $node = make($r); } // <---------- CULPRIT!
         | 
         s=symbol { $node = $s.node; }
      )

Here is the original grammar for comparison:

      ( 
           BEGIN_RESERVED..END_RESERVED
         | symbol
      )

Semantic predicates mostly opaque to ANTLR. It may do some simplistic modification of the expressions but mostly it doesn't understand the meaning of the expression itself. This means that any limits placed in semantic predicates are not considered when processing any prediction logic. Using r=. means that all possible tokens are predicted, which is definitely wrong. In most cases this won't work at all and even in cases where it works, it will lead to some very bad results.

I understand that the intention here was to replace the range matching operator .. that is missing in ANTLR4. This is a big miss by the way, I don't understand why it is missing. I guess most other languages don't need this feature but it was a very clean way for us to handle things.

The correct way to do this in our case (and this is nasty but better than any other way that I know of) is to implement a new rule like this:

reserved_keywords returns [Aast node]
   :
      @after
      {
         $node = make($v);
      }
        v=KW_U_CTRL
      | v=KW_U_MSG
      | v=KW_U_PCTRL
      ... you get the idea ...
      | v=KW_XOR
      | v=KW_XREF
      | v=KW_XREF_XML
   ;

I've not checked but I'm hoping we can reuse the v variable name this way and reference it from the after action. If not then the $node = make($v); would have to be an action after every match and possibly would have to have different var names for each match (I really hope not). If ANTLR doesn't allow that concept with the implicit variable created by the v label, could we create our local var of the right type in the init action and use that for the assigment?

Then the core matching logic is this:

      ( 
           r=reserved_keyword  { $node = $r.node; }
         | s=symbol            { $node = $s.node; }
      )

It is fragile because now we have to maintain the extra reserved_keyword rule whenever we change anything between BEGIN_RESERVED and END_RESERVED. But otherwise it should be nice and fast.

As an aside, my sense is that ANTLR4 is a step backwards in how we can express our grammar. Much of that is due to their decision to remove AST support but even little things like the missing range operator and changes in lexer support are going to add up to a result that is not as good as we started with. To be clear: that is not your fault.

I think we need more helper methods (outside of the parser) to reduce code like this (from func_return):

           a=KW_CHAR                                              
           { 
              $ftype = FUNC_CHAR; 
              if ($node != null)
              {
                 $node.graft(make($a));
              }
              else
              {
                 $node = make($a);
              }
           }
         | b=KW_COM_HNDL                                          
           { 
              $ftype = FUNC_COM_HANDLE; 
              if ($node != null)
              {
                 $node.graft(make($b));
              }
              else
              {
                 $node = make($b);
              }
           }
         | c=KW_DATE                                              
           { 
              $ftype = FUNC_DATE; 
              if ($node != null)
              {
                 $node.graft(make($c));
              }
              else
              {
                 $node = make($c);
              }
           }
...

There is a lot of code duplication here and it makes the grammar much harder to read and maintain. These actions need to be condensed into a single method call to a helper, passing whatever data is needed.

Here is a breakdown of how SLL, LL, adaptivePredict, the ATN, and the lookahead DFA all work together in the ALL(*) parsing algorithm.

To truly understand them, it helps to see them not as separate parts, but as an interconnected system where adaptivePredict acts as the "brain," the ATN is the "map", the DFA is the "memory," and SLL/LL are the two "modes of navigation."

• The function conceptually acts as a function of the decision nonterminal A (a nonterminal is represented by a rule name) and the current parser call stack γ0. Its ultimate goal is to analyze the grammar and return alt, which is the correct predicted production number to expand.
• Because adaptivePredict is only looking ahead and not actually consuming tokens, it must undo any changes made to the input cursor. It does this by capturing the current input index as start upon entry and rewinding to start using input.seek(start) before returning.
• The algorithm first checks if the current decision involves any productions with semantic predicates ∃ A → π_i α_i. Because prediction only evaluates predicates during a full LL simulation, adaptivePredict immediately delegates the task to LLpredict if predicates are present.
• If the decision is unpredicated but does not yet have a Lookahead DFA, adaptivePredict must create one from scratch.
  • It calls startState(A, #) to create initial DFA state, D_0 (D_0 represents the set of ATN configs reachable without traversing a terminal edge). # is a special wildcard indicating no parser stack information.
  • It then constructs the set of final states F_DFA assigning one final state for each production / alternative of parser rule A. After that, it will instantiate the DFA object with an empty set of transitions.
• However, for unpredicated decisions, the function relies on SLLpredict in order to compute the prediction alt. If the DFA was just created, SLLpredict will begin adding paths to it via ATN simulations. If the DFA already existed, SLLpredict will attempt to obtain a prediction by simply walking the existing DFA cache. If it encounters an "unfamiliar" input phrase, it will dynamically extend the DFA through ATN simulation (which can be very expensive if the grammar is a complex one).

• When adaptivePredict encounters an unfamiliar sequence, it must figure out the correct parsing path by launching subparsers
• When the parser hits a decision point (a nonterminal rule with multiple alternatives), it launches more subparsers. Specifically, it creates one subparser for each alternative production. If a rule has 4 possible ways to be parsed, 4 subparsers are created, each "sitting at the starting line" of their respective paths in the ATN.
• These subparsers do not take turns, they operate in pseudo parallel to explore all possible paths simultaneously. Each subparser carries its own "backpack" of information called a configuration, which tracks its current state in the ATN, the production it is trying to prove, and its specific call stack.
  • ATN configuration = a tuple (p, i, γ)
  • p is the current ATN state
  • i is the production number is trying to predict
  • γ is the ATN subparser call stack = the history of nonterminal rules it entered to get here

• The subparsers advance through the remaining input tokens in lockstep. They all look at the first lookahead token, attempt to move forward in the ATN, and then all wait and look at the second token together. Moving in lockstep prevents any single subparser from running infinitely ahead and consuming the whole file (which would cause exponential time complexity).
• As the subparsers evaluate the upcoming tokens against the ATN's rules, many will find that their path doesn't match the input. When a subparser's path fails to match the remaining input, that subparser simply "dies off" and is deleted.
• The lockstep advancement continues until the analysis identifies a sole survivor, meaning all subparsers except one have died off. The moment only one subparser remains, the algorithm stops. It has successfully found the minimum lookahead depth required to uniquely predict the correct alternative.

• To navigate the ATN (the grammar map), subparsers use a call stack to remember where they came from. If every subparser keeps its own separate stack, they duplicate a massive amount of work. This causes an exponential explosion in time and memory. The solution ANTLR4 comes with is to merge the stacks into a shared graph. The authors explicitly state that merging those individual stacks into a GSS "reduces the potential size from exponential to linear complexity"

• Running full ATN simulations for every single word in a file would be incredibly slow. (worst case scenario O(n^4)). This is where the DFA (Deterministic Finite Automaton) comes in.
• The entire purpose of building this DFA is to never repeat the same ATN simulation twice. Once a path is drawn in the DFA, the parser becomes blindingly fast for that specific grammar decision.

• Static LL builds the lookahead DFA at compile-time by analyzing the grammar. This relies on approximating all possible lookahead sequences with regular languages, which is fundamentally limited and can fail due to undecidability, forcing fallback to backtracking.
• Dynamic ALL(*) (ANTLR 4) constructs the DFA at parse-time, starting empty and learning decisions from actual input via ATN simulation. It only analyzes the input sequences that occur, avoiding the need for global, precomputed lookahead.

• SLL
• "Parsers that ignore the parser call stack for prediction are called Strong LL (SLL) parsers."
• When SLL simulates the ATN, it completely ignores the history of how the parser got to the current rule.
• In the ATN simulation, the call stack is normally represented by γ0. However, "stack-insensitive SLL prediction, on the other hand, ignores the parser call stack and uses an initial stack of #, indicating no stack information".
• Because # acts as a wildcard, when the SLL subparser reaches the end of a rule, it doesn't know where to return. Therefore, it "must consider all possible invocation sites".
• SLL is the only mode that is allowed to write to the Lookahead DFA. "Creating a different lookahead DFA for each possible parser call stack is not feasible since the number of stack permutations is exponential [...]". By keeping the simulation stack-insensitive, the resulting DFA is universally valid and can be safely cached for speed.
• LL
• While SLL is fast and easy to cache, it breaks down in situations where the same input can mean different things depending on context.
• As the paper points out, something like a Java method can be parsed differently depending on whether it appears inside a class or an interface, this is information that only the call stack can provide.
• When that happens, ALL(*) switches to full LL prediction, which uses the complete parser context to make the correct decision.
• The important difference is that LL is no longer just looking at the next tokens, but also at how it got there, which makes it precise but also harder to generalize.
• Because of that, you can't really cache LL decisions the same way you do with SLL. The result depends on context, not just input, so trying to store it in the DFA would mix together decisions that only apply in very specific situations.
• This is sometimes described as poisoning the DFA: you'd end up filling it with context heavy states that don't generalize well and hurt performance instead of helping it.

Greg Shah wrote:

I think we need more helper methods (outside of the parser) to reduce code like this (from func_return):

[...]

There is a lot of code duplication here and it makes the grammar much harder to read and maintain. These actions need to be condensed into a single method call to a helper, passing whatever data is needed.

Yes, that makes sense. My plan also includes reviewing the grammar and, as you pointed out, refactoring it where possible to make use of the helper methods, as well as comparing it against the original grammar file.

The supposed advantage of ALL(*) is wasted on us. They designed it to trade runtime performance for a more simple grammar that ignores the restrictions of LL(k). In other words, they designed it to be easier to write the grammar but to use runtime resources to make parsing decisions instead of compile-time decisions. That makes no sense for us. Our grammar is already written as LL(k). Their argument that it is generally faster than its expected worse case scenario is going to be wrong for us. Our scenario is its worst case. The ALL(*) algorithm is a huge waste of CPU and memory in our case.

Their design is "mostly deterministic" and uses regular expressions and lots of unnecessary prediction logic. None of that makes sense for us. Our grammar is already optimized for fully deterministic compile-time rules with only a small amount of lookahead. We deal with context outside of the grammar itself (e.g. dictionaries of scoped resources, schema lookups) using semantic predicates. There is no way around that. The 4GL language is highly ambiguous and highly irregular. That context is much more complex than can be encoded in the ANTLR4 grammar itself, so even if we wanted to do so, I don't think we could implement the parser context directly in the grammar.

They've optimized ANTLR4 for people that write lots of new parsers/different projects all the time instead of for people maintaining the same complex parser in one project. It is probably why they also felt they could eliminate AST support.

The argument that "computers now have plenty of resources to do this runtime parsing approach" is wasteful at best and wrong in enough real world cases to matter for us.

Unfortunately, I don't think we can disable ALL(*) completely and move back to LL(k) in ANTLR4. Please see if I'm wrong about that. The core value of moving to ANTLR4 (for us) is related to access to the ATN at runtime, so that IDE support is more naturally implemented. The more I look at this, the more harmful ANTLR4 seems.

BTW, we really should be discussing all of this in #1757 which is the core task for shifting to ANTLR4.

Greg Shah wrote:

Unfortunately, I don't think we can disable ALL(*) completely and move back to LL(k) in ANTLR4. Please see if I'm wrong about that.

You're absolutely right about that, I couldn't find any resources that explicitly state ALL(*) can be disabled.

We have a big decision to make.

It is not clear to me that ANTLR4 is the best choice. It has been optimized to make writing parsers faster instead of writing faster parsers. That is a poor design choice, in my opinion. You spend the time once to write a parser. But it is run millions or billions of times. Why would you want all of those times to be slower just so that you could save a little time up front?

But the ATN and improved error handling are both things that make ANTLR4 much better designed for IDE support than ANTLR2.

Give me some time to think on this.

I've just identified the issue with my latest commit. I think, due to the build.gradle, the *TokenTypes.java files were excluded, which is why the project doesn't compile at all.

I'll try an approach based on a semantic predicate, something like { someMethodThatChangesTokenType() }?. The idea is that the predicate must evaluate to true in order for parsing to continue, and that method would run first, updating the token type up front so it can be matched correctly afterward.

Paula Păstrăguș wrote:

I'll try an approach based on a semantic predicate, something like { someMethodThatChangesTokenType() }?. The idea is that the predicate must evaluate to true in order for parsing to continue, and that method would run first, updating the token type up front so it can be matched correctly afterward.

I don't think that is OK. In ANTLR4 semantic predicates must not have any side effects.

Update: Using SLL prediction mode together with reserved_keywords and unreserved_keywords, the fwd-embedded-driver.p file is now parsed entirely🥳. To reach EOF, I also had to fix several issues in the Progress parser logic. This is a significant improvement, previously, with the default prediction mode, parsing would stop around line 176.

That said, I suspect there may be a cycle 🫣 in the resulting AST. Attempting to dump it resulted in an OOM exception, so this needs further investigation. Still, today's results are encouraging:)

However, I had to comment SOME RUN statements in order to reach EOF, which also requires further investigation. I'm also seeing logs like Missing original-token annotation, which need to be looked into as well.

Additionally, I noticed some performance issues when matching specific input, for example avail, this might be an area for improvement. For now, I think the next step is to compare the produced ASTs.

I managed to get ant convert to run successfully and produce a .ast file. The build now completes, which is a solid step forward. I still encounter a null hmver map from within functions_procedures.rules, this doesn't impact the build itself, only the logic, so I'll address that first. There are also a few remaining issues to investigate, particularly around certain RUN statements and other errors.

Update: I compared the ASTs generated by ANTLR2 and ANTLR4 for fwd-embedded-driver.p (full file, without commenting anything out). After addressing several issues, the structures are now very close!! There are still around 3-4 differences remaining (I haven't compared the annotations yet, which will also need to be addressed). I'll resolve these after handling a higher-priority issue.

Update: The AST produced for the fwd-embedded-driver.p file now has the same structure. Next, I'll also verify the annotations.

Currently, ant convert takes about 2 minutes and 16 seconds to complete using ANTLR4, compared to only 35 seconds with the old version. I haven't focused on performance optimizations yet, but I believe there's room to improve this, hopefully.

Update: ant convert is now taking around 56 seconds to complete. This means the ANTLR4 based implementation is currently only around 2x slower compared to the original version, which is a significant improvement from the earlier state.

I've moved the testing to a larger project, Hotel GUI, to validate the parser changes in a more realistic environment. Right now I'm investigating an issue related to a forward function definition, which currently fails to parse correctly.

To be more specific, I reached a point where the conversion now generates all expected files, including the converted Java file, the .schema, .p2o, .dict and the other ones.

Update: All Hotel GUI files are now converted (66 files). I still need to check the generated Java files, ASTs, and annotations, because there are still some inconsistencies. Buuut, I think we are approaching the smoke testing phase. 🙂

I fixed the issue, and fortunately it was not a problem in the fql parser itself. I was accidentally calling the parser rule twice, and when trying to access the root the second time, there were no more tokens left to consume :)) Hopefully, that was the last major issue.

I also did some smoke testing and did not encounter any visible issues, which is really encouraging!

At this point, there are only three Java files left with minor differences, mainly related to comments being placed on different lines than in the original output. I'll investigate what is causing those mismatches.

After that, I plan to review the progress.g file and see if I can optimize the overall conversion time, because right now the Hotel_GUI project takes around 13-15 minutes to complete the conversion.

Then, maybe I can try running <app_c> as well. That would probably be an overnight conversion, but it could help expose additional edge cases that still haven't been solved yet.

I think we're getting really close to creating the b version and refactoring the code into a clean, commit-ready state!

Please commit what you have already. It doesn't matter if it isn't perfect. I want to see the revisions as you make them.

It is available at rev 16495 / 10804a.

Let me create the b version now and port all my changes there. After a bit of refactoring and cleanup, I'll commit the baseline, then we can start from that clean state in order to incrementally commit the future fixes / javadoc / history entries. What do you think?

I don't like losing history on a branch. That sounds like what will happen. Why is that clean slate needed?

Well, it all started because the old 10804a version no longer compiled, and I couldn't realistically make it work again since the baseline had become too outdated. Because of that, I moved all the changes onto a local trunk version, and things progressed much better there.

I don't think this can be cleanly handled with a rebase anymore, which is why I'm suggesting creating the b version. Things are evolving pretty quickly right now, and we need to keep the baseline as up to date as possible. Honestly, I think we've reached the point where it makes sense to do that.

You can create a b branch. I don't want to lose history again. In the future, find a way to make the rebase work.

Committed the baseline as rev 16576 / 10804b.

A ./gradlew antlr4All followed by ./gradlew jar should work now. After that, you can try running ant convert or ant deploy.all for the Hotel GUI project, these should work as well.

Current issue I'm working on:

I moved testing to <app_c>, at least to check whether the files parse correctly during the first few minutes. After that, I plan to let the conversion run overnight.

However, I'm hitting the tokens are fetched until EOF issue again. In pre_scan_class, we currently have:

      (
           using_stmt
         | { _input.LA(1) != KW_CLASS && _input.LA(1) != KW_INTERFAC && _input.LA(1) != KW_ENUM }?
           .
      )*

As we have already seen in other cases, the wildcard seems to be the culprit. What exactly is this . supposed to match here..? Any token aside from KW_CLASS, KW_INTERFAC and KW_ENUM, but does progress allow to match ANY token? What are the valid cases..?

In pre-scan mode we are trying to do a minimal parse to get some structural elements without worrying about the details. That is why we use the wildcard.

Rather than working on <app_c>, I think it is important to resolve the performance issue. If that can't be solved, then we cannot move ahead with ANTLRv4.

Most of the time is spent in primary_expr decision. I tried to split it.. but I had no luck, the total time for Hotel Gui was almost the same. I think the culprit here is due to matching same tokens / or rules (it applies to lvalue and any_non_reserved_symbol) as the beginning of something.

class_event can start with any_non_reserved_symbol, same applies for method_call. I think those needs to be merged together. If you have any other suggesting, please let me know.

Sorry for the delay, right now, when trying to commit today changes I find out the connection of off..

Greg Shah wrote:

In pre-scan mode we are trying to do a minimal parse to get some structural elements without worrying about the details. That is why we use the wildcard.

Okay, this means we can exclude the details directly from the lexer, aka redirecting to a custom channel (maybe named junk :). (not the HIDDEN one)

Here are some relevant values regarding performance issue:

For avail-rooms-frame.w, the total time spent into primary_expr decision making is 1614ms. For browser.p it's 4302 ms, aka 4 seconds, which is huge.

Most of the time is spent in primary_expr decision. I tried to split it.. but I had no luck, the total time for Hotel Gui was almost the same. I think the culprit here is due to matching same tokens / or rules (it applies to lvalue and any_non_reserved_symbol) as the beginning of something.

We have to be very careful about changes to this rule. It is the core for matching for expressions. It basically matches any operand or the start of sub-expressions that can be operands or any standalone expression (something that has no operators). The design is intentional and the order is important.

class_event can start with any_non_reserved_symbol, same applies for method_call. I think those needs to be merged together. If you have any other suggesting, please let me know.

It will be more than just those, you'd have to merge together a lot of rules. That would leave the grammar in a very bad place (hard to read and understand). I really want to avoid this.

We need to take some calculated risks.. otherwise, as you said, we will not be able to move forward with ANTLR4. I may still be able to find an elegant way to refactor the parser rules that are currently problematic from a performance perspective.

The main performance culprits still appear to be the "pull" alternatives. adaptivePredict needs these alternatives to remain visible, because tokens such as SYMBOL, reserved symbols, or non-reserved symbols must still be considered valid entry points for the rule. Once the rule is actually entered, the token type is rewritten in the @init action, and the parser then selects the proper alternative (it's usually the first alternative). If the rewritten type is not valid, an exception is thrown. (NoViableAltException)

In other words, these pull alternatives are necessary for prediction to reach the rule, but they also increase the prediction cost because ANTLR4 must keep considering broad alternatives before the token rewrite takes effect.

I am currently trying one more approach for the ANTLR4 performance issue around primary_expr.

The main problem is still caused by the ambiguity between alternatives such as method_call, func_call, class_event, object_data_member, user_defined_type_name, can_find_function, super_function, if_func, lvalue, and parenthesized expressions. Many of these alternatives can start with the same token shape, especially SYMBOL, reserved/non-reserved keywords, or tokens which are later retagged by parser actions.

The current experiment is to isolate this ambiguous part of primary_expr into a separate rule, something like ambiguous_primary_expr, and then override ParserATNSimulator.adaptivePredict() only for the decision associated with this grouped rule. Instead of letting ANTLR4 perform the expensive adaptive prediction for this specific decision, the custom simulator detects the decision number and manually returns the expected alternative based on the same semantic checks that were previously used as predicates in primary_expr.

The idea is to keep the grammar behavior mostly unchanged, including the broad alternatives still needed by func_call and class_event, but avoid the expensive outer prediction between all these competing alternatives. The custom prediction only decides which branch of ambiguous_primary_expr should be entered.

I already hit a few issues while testing this approach, mainly caused by mismatched alternative numbers and by specialized alternatives being routed incorrectly through the generic func_call path. Those are being fixed incrementally by making the manual predictor match the exact order of the grouped grammar alternatives and by adding stricter candidate checks..

If this approach works, it should let us bypass the problematic adaptivePredict. If it does not solve the performance issue either, I think I am close to running out of realistic ideas.

🤞

I've already reduced primary_expr prediction time from seconds down to nanoseconds, but that alone isn't enough. I still need to optimize several other hotspots. Below are the top 25 most time‑consuming decisions in the fwd‑embedded-driver file:

decision=946, rule=record_funcs, invocations=12, timeInPrediction=1.451233 s, SLL_TotalLook=47351, SLL_MaxLook=9839, 
decision=8, rule=block, invocations=871, timeInPrediction=1.391434 s, SLL_TotalLook=16950, SLL_MaxLook=543, 
decision=936, rule=lvalue, invocations=1050, timeInPrediction=0.744240 s, SLL_TotalLook=45892, SLL_MaxLook=9804, 
decision=278, rule=end_stmt, invocations=137, timeInPrediction=0.622442 s, SLL_TotalLook=6404, SLL_MaxLook=330, 
decision=254, rule=inner_block, invocations=118, timeInPrediction=0.617709 s, SLL_TotalLook=6045, SLL_MaxLook=543, 
decision=884, rule=chained_object_members, invocations=222, timeInPrediction=0.593825 s, SLL_TotalLook=578, SLL_MaxLook=7, 
decision=575, rule=def_var_stmt, invocations=228, timeInPrediction=0.510193 s, SLL_TotalLook=5117, SLL_MaxLook=280, 
decision=9, rule=single_block, invocations=888, timeInPrediction=0.509106 s, SLL_TotalLook=10549, SLL_MaxLook=550, 
decision=889, rule=chained_object_members, invocations=1913, timeInPrediction=0.491773 s, SLL_TotalLook=2566, SLL_MaxLook=9, 
decision=87, rule=define_stmt, invocations=142, timeInPrediction=0.467173 s, SLL_TotalLook=4708, SLL_MaxLook=277, 
decision=66, rule=function, invocations=18, timeInPrediction=0.332978 s, SLL_TotalLook=2764, SLL_MaxLook=435, 
decision=882, rule=un_type, invocations=1683, timeInPrediction=0.290290 s, SLL_TotalLook=2932, SLL_MaxLook=20, 
decision=20, rule=procedure, invocations=16, timeInPrediction=0.241396 s, SLL_TotalLook=2202, SLL_MaxLook=331, 
decision=872, rule=log_not_expr, invocations=1366, timeInPrediction=0.203534 s, SLL_TotalLook=1627, SLL_MaxLook=18, 
decision=878, rule=compare_expr, invocations=1366, timeInPrediction=0.195463 s, SLL_TotalLook=1366, SLL_MaxLook=1, 
decision=877, rule=compare_expr, invocations=1484, timeInPrediction=0.193430 s, SLL_TotalLook=1879, SLL_MaxLook=5, 
decision=870, rule=log_and_expr, invocations=1366, timeInPrediction=0.141987 s, SLL_TotalLook=1587, SLL_MaxLook=18, 
decision=876, rule=compare_expr, invocations=135, timeInPrediction=0.132006 s, SLL_TotalLook=283, SLL_MaxLook=18, 
decision=869, rule=expr, invocations=1316, timeInPrediction=0.131350 s, SLL_TotalLook=1451, SLL_MaxLook=28, 
decision=516, rule=malformed_symbol, invocations=47, timeInPrediction=0.110035 s, SLL_TotalLook=319, SLL_MaxLook=13, 
decision=879, rule=sum_expr, invocations=1664, timeInPrediction=0.106982 s, SLL_TotalLook=2213, SLL_MaxLook=21, 
decision=861, rule=filename, invocations=21, timeInPrediction=0.089179 s, SLL_TotalLook=435, SLL_MaxLook=68, 
decision=597, rule=var_type, invocations=185, timeInPrediction=0.083183 s, SLL_TotalLook=403, SLL_MaxLook=3, 
decision=251, rule=regular_parm, invocations=84, timeInPrediction=0.071911 s, SLL_TotalLook=1091, SLL_MaxLook=140, 
decision=921, rule=attribute_or_method, invocations=222, timeInPrediction=0.065174 s, SLL_TotalLook=230, SLL_MaxLook=3, 

I think it is time to document some findings and updates here.

At this point, I think I have stabilized the Progress parser as much as I reasonably can. I have both good news and bad news..

The good news is that the Hotel GUI parse time has dropped to 1 minute and 24 seconds, with a total conversion time of 5 minutes and 54 seconds.

The bad news is that my previous comparison baseline for the Hotel GUI parsing phase and total conversion time was incorrect. The original parsing phase was not around 1 minute; it was actually around 25 seconds at most. I noticed this after running the conversion with my IDE closed 🥲. Apparently, the IDE consumes a significant amount of resources and influences the results.

That being said, 1 minute and 24 seconds compared to 25 seconds is still acceptable, in my opinion. From a parser perspective, I do not think there are many more tricks left that could significantly reduce the time further. However, from a lexer perspective, there is still room for improvement. For Hotel GUI, the lexer currently takes around 50 seconds, while the parser itself spends around 34 seconds deciding which alternatives to take.

I think the next step should be to investigate how the lexer can be improved from a performance point of view.

Greg, I remember you suggested something related to this at some point, but I do not recall the exact details. Could you please advise again? Thanks!

The lexer doesn't appear to be the bottleneck. Using tokens.fill() and summing the elapsed times across all Hotel_GUI files gives a total of ~8.4 seconds, not 50! The slowdown must originate elsewhere, I'll continue the investigations.

Greg, I remember you suggested something related to this at some point, but I do not recall the exact details. Could you please advise again? Thanks!

I had suggested that if we really needed to optimize this, we could hand-write the lexer.

Update: Hotel GUI parse time has now dropped to 1 minute and 8 seconds, with a total conversion time of 5 minutes and 35 seconds.

One bottleneck I identified is the toolbar.p file. It throws an NPE caused by a no viable alternative, which is expected, since the same also happens with ANTLR2. The important difference is the time spent reaching that conclusion: ANTLR2 takes at most 1-2 seconds to detect that there is no viable alternative, while ANTLR4 takes around 10 seconds.

I also ran the conversion without this file, and the result was better, as expected: the parse time dropped to 58 seconds. This suggests that we may also need some special tricks for cases where a no viable alternative is expected to be thrown..

Greg, what do you think?

Another important thing I noticed is that the ANTLR4 prediction algorithm does not seem to provide a reliable way to exit structures like ()*. In the cases where the conversion hangs (within antlr4 adaptivePredict), it looks like it keeps consuming/fetching all available tokens, and even after that it still does not know where or how to stop.

Greg, is a ()* construct in ANTLR2 greedy or non-greedy by default? I only found three options blocks with greedy = false in the progress.g file.

One bottleneck I identified is the toolbar.p file. It throws an NPE caused by a no viable alternative, which is expected, since the same also happens with ANTLR2.

I would not expect an NPE from any valid 4GL source code. Does this file fail to parse in OE?

This suggests that we may also need some special tricks for cases where a no viable alternative is expected to be thrown..

I don't understand how valid 4GL source code can cause this.

Paula Păstrăguș wrote:

Another important thing I noticed is that the ANTLR4 prediction algorithm does not seem to provide a reliable way to exit structures like ()*. In the cases where the conversion hangs (within antlr4 adaptivePredict), it looks like it keeps consuming/fetching all available tokens, and even after that it still does not know where or how to stop.

ANTLRv2 and ANTLRv4 both have their loops and optional alternatives set to be greedy by default. In ANTLRv2, you disable it with an options block setting greedy = false. In ANTLRv4, you change the syntax of the loop or alternative itself to add a ? modifier.

• ()* is greedy (default)
• ()*? is non-greedy
• ()+ is greedy (default)
• ()+? is non-greedy
• ()? is greedy (default)
• ()?? is non-greedy

Greg Shah wrote:

This suggests that we may also need some special tricks for cases where a no viable alternative is expected to be thrown..

I don't understand how valid 4GL source code can cause this.

It is not valid, a dot is missing here:

      /* {get Prop var [handle]} */ ASSIGN ghProp = WIDGET-HANDLE(ENTRY(1, TARGET-PROCEDURE:ADM-DATA, CHR(1))) ghProp = ghProp:BUFFER-FIELD('Menu':U) lMenu = ghProp:BUFFER-VALUE // -----------------> DOT missing

   /* {get Prop var [handle]} */ ASSIGN ghProp = WIDGET-HANDLE(ENTRY(1, TARGET-PROCEDURE:ADM-DATA, CHR(1))) ghProp = ghProp:BUFFER-FIELD('RemoveMenuOnhide':U) lRemoveMenu = ghProp:BUFFER-VALUE 

    .
      /* if remove on hide then remove */ 

After the first ASSIGN stmt, a DOT is required, otherwise the 4GL code it not valid.

Paula Păstrăguș wrote:

Greg Shah wrote:

This suggests that we may also need some special tricks for cases where a no viable alternative is expected to be thrown..

I don't understand how valid 4GL source code can cause this.

It is not valid, a dot is missing here:
[...]

After the first ASSIGN stmt, a DOT is required, otherwise the 4GL code it's not valid.

Greg Shah wrote:

ANTLRv2 and ANTLRv4 both have their loops and optional alternatives set to be greedy by default. In ANTLRv2, you disable it with an options block setting greedy = false. In ANTLRv4, you change the syntax of the loop or alternative itself to add a ? modifier.

• ()* is greedy (default)
• ()*? is non-greedy
• ()+ is greedy (default)
• ()+? is non-greedy
• ()? is greedy (default)
• ()?? is non-greedy

Aha, this means os_delete_stmt and os_create_dir_stmt need to be adjusted so they preserve the intended non-greedy behavior.

It is not valid, a dot is missing here:

Hmmm. Something is wrong here with the project. You are working with Hotel GUI, right? Every file there passes parsing in trunk. You need to fix the project first and then focus on performance of the result. In other words, we are not trying to optimize performance in the case where there is broken 4GL code. I can accept slower results in that case. That is not a common case. Projects should not have broken 4GL code when running full conversion which is what we need to optimize.

Right, I remember having this error from the very beginning, basically two years ago:-)

Anyway, I think a fresh checkout of Hotel GUI might solve it, hopefully. For now, I removed that file so I can get the exact performance values.

Update: the current Hotel GUI parse time is 49 seconds. I think I can still improve this further through parser changes, but the lexer should indeed be manually rewritten as well, since it is also a bottleneck.

Please note that the original Hotel GUI parse time was around 25 seconds.

Greg, it is not 3x slower anymore, it is only around 2x now, and I think I can still push the boundaries:)

10804b is at rev 16583 now (the latest).

Update: Hotel GUI performance comparison between ANTLR2 and ANTLR4.

At this point, I think the parser has been optimized as much as reasonably possible. I will also attach a log file containing the metrics for each Hotel GUI file.

Please note that Hotel GUI is a relatively small project, so it may not reflect the exact percentage of the added slowness in larger projects. I will also test Counteract again, hopefully it will no longer hang.

Please also note that, from the 12.8 seconds spent in lexer + parser, the lexer alone seems to take at most 8-9 seconds, so this is still an area that should be improved.

In this 2X difference for the front end, 40% comes from the ANTLR4 code and 60% from something else. The other major change in this branch is the AST implementation so that is where I would expect to find the issue.

What does your profiling tell us about that other 60%?

I also attached a log file in my previous post. It includes the total time ANTLR4 spends in prediction mode for each Hotel GUI file.

Greg, when you have some time, please take a look. Most of the reported times are under 0.03 seconds per rule. Please let me know if that can be improved further..

10804b is at rev 16584 now (here you can find today's parser improvements).

Another slowdown source is the evaluateExpr method: under ANTLR2, Hotel GUI spends 5.3 seconds in it, whereas under ANTLR4 the time increases to 18.8 seconds. Please note that evaluateExpr relies on the Progress lexer and parser to evaluate 4GL expressions. Since the lexer is already known to be slow and due for a rewrite, I expect a significant reduction in this cost once the new lexer is in place.

Before moving forward, another project conversion progressed further than in previous attempts, it reached the server/jb directory. However, several AstException cases still need to be resolved. I've already fixed some of them and I'm continuing to debug the remaining errors.

I added temporary lexer profiling in order to measure the token generation cost per token type, aggregated across all processed files from Hotel Gui.

Overall lexer profile summary:

• Total lexer time: 8718.658 ms (~8.719 seconds)
• Total generated tokens: 826,835
• Average lexer time: 10.545 us/token
• Number of profiled files: 65

Main culprit token types by total lexer time:

The top three token types alone account for:

• COMMENT + WS + SYMBOL = 4632.198 ms (~4.632 seconds)
• This represents ~53.13% of the full lexer time.
• COMMENT is the most suspicious token type. It has both a high total time and a high average time per token (~62.112 us/token). This should probably be rewritten first.
• WS has a very high total time, but this is mostly because it is extremely frequent. The average cost per WS token is relatively low (~5.251 us/token), so it is not necessarily inefficient individually.
• SYMBOL is also important. It has a high total time and a non-trivial average cost (~17.694 us/token). Since many keyword tokens are likely produced through the SYMBOL rule and then retyped through keyword lookup, part of the KW_* token cost is probably also related to the SYMBOL rule.
• NUM_LITERAL does not look like a primary culprit in this profile. It had 14,597 tokens, total time 104.680 ms, average 7.171 us/token, which is only ~1.20% of the total lexer time.

I am moving to an alternative approach for the ANTLR4 lexer performance issue (based on the idea mentioned by Greg in the last meeting). Instead of continuing to rewrite and micro optimize the migrated ANTLR4 lexer, I am checking whether we can reuse the old ANTLR2 Progress lexer and adapt its emitted tokens to the ANTLR4 runtime.

The idea is to keep the old lexer in place for token generation, then introduce a small adapter layer which converts the ANTLR2 tokens into ANTLR4 compatible tokens. These tokens can then be exposed through an ANTLR4 TokenSource / CommonTokenStream, so the existing ANTLR4 parser can consume them normally.

This would allow us to isolate how much of the remaining conversion slowdown is caused specifically by the ANTLR4 lexer.

Rewriting the ANTLR4 lexer does not currently look like the best direction.. Even after optimizing expensive rules, the overall lexer time only decreases partially.., while other token types such as WS, STRING, and keyword related tokens still dominate the profile. I don't think we can improve it to 0.5 seconds (for Hotel Gui), only if we look at WS token generation time we can see is 3x slower that the original antlr2 lexer.

The approach seems to work! 🥳 I was able to get the same AST for standard.df. However, there are still a few remaining inconsistencies and errors that need to be fixed before the Progress parser can work again with this 'setup'.

For Hotel GUI, the total time needed to generate the tokens with the ANTLR2 based lexer and convert them to ANTLR4 compatible tokens is around 0.5 seconds (compared to around 8 seconds), which is a significant improvement.

For the first conversion phase, the time dropped from 45 seconds to around 38 seconds. However, there is still room for further improvement. I noticed that ExpressionEvaluator was not using the optimized ProgressParserATNSimulator, so I integrated it there as well. At the moment, I am getting some prediction errors caused by a function that is not found through sym.lookupFunction, and I am currently investigating this issue. If I manage to resolve it, I expect we may gain around another 8 seconds, but this still needs to be confirmed.

I also ran a full conversion, and the total time dropped from 05:17 to 04:30. There are still some exceptions that need to be resolved. Most of the issues so far were caused by ANTLR2 versus ANTLR4 token type mismatches, as well as by the column starting value. The column value is now back to 0, while in the previous ANTLR4 based lexer setup it started from 1.

Update: LATEST Hotel GUI performance comparison between ANTLR2 and ANTLR4. (Please note that all conversions were executed with no applications running, ensuring clean, uncontaminated:-D results)

One testcase is still failing: AdaptiveQuery.initialize now receives WHOLE-INDEX as a parameter, which is not expected and needs to be resolved. I also need to measure the combined lexer + parser time for Hotel_GUI, once that's done, I'll update the table.

Paula Păstrăguș wrote:

One testcase is still failing: AdaptiveQuery.initialize now receives WHOLE-INDEX as a parameter, which is not expected and needs to be resolved.

I resolved the issue, all the changes are committed as rev 16587 (including the adapter + the original antlr2 lexer).

Next, I'll rabase the branch, setup new big application, redo the conversion, and solve the remaining parser issues. Also, build.gradle needs to be refined.

10804b was rebased, new rev is 16621.

I've committed the build changes as rev 16622. ./gradlew all should work now.

I have some good news to share! One of the larger applications successfully passed the first phase of the conversion, including scanning, preprocessing, lexing, and parsing.

However, there's still one issue. The P2O generation fails for one of the DF files, and because of that, the ant deploy task hangs at that stage.

I'll investigate the issue on Monday.

The issue was solved, latest 10804b rev is 16627.

I'll run another overnight conversion, it shouldn't hang at the previous step anymore. I also identified a few suspicious files in the larger application that parse unusually slowly, I'll investigate potential optimizations.

After that, I'll verify whether the 10804b branch still has any gaps and implement whatever is missing. I recall some code was commented out. Once these checks are complete, the task will be ready for a full review. 🥹

10804b rev 16630 resolved a performance bottleneck by introducing predictSelectionListPhrase__561, improving the parser's ability to disambiguate selection_list_phrase. As part of this work, the AST generation for color_phrase was corrected, and applyMissingFormatPhraseSchemaValidation was introduced to preserve the original ANTLR2 schema validation behavior in cases where ANTLR4 cannot express an empty-alternative closure. In addition, obsolete logic was removed from the ANTLR2 token adapter, eliminating AST inconsistencies caused by mutating token types for parser disambiguation (e.g., after RUN, PROCEDURE, FUNCTION, and similar constructs). These cases are now handled directly by the parser through custom prediction, removing the need for lexer side token type adjustments.

10804b rev 16631 resolved a regression introduced in the previous revision, where RUN statements were no longer parsed correctly. The issue was addressed by extending the custom prediction for stmt_list and case_stmt, restoring the expected parsing behavior without reintroducing the previous adapter workarounds.

Additional changes included:

• ClassDefinition: Replaced the ANTLR2 NoViableAltException dependency with descriptive validation errors containing AST context for missing class names.
• Preprocessor: Removed obsolete TokenStreamException handling, as lexer exceptions are now handled by the ANTLR2 token source adapter.
• Miscellaneous code cleanup.

10804b rev 16632 moves the changes from progress.g revisions 437, 438 and 439 into the ProgressParser itself. It also restores the default HQL and FQL parser recognition error reporting to match the previous ANTLR2 behavior, adds parser filename support, fixes the custom prediction in predictLvalueAlt__937, removes obsolete migration code, and includes additional code cleanup.

One of the remaining gaps for this issue is DynamicQueryHelper. The unresolved part is the runtime parser error handling after the ANTLR4 migration (aka showConversionError). The previous ANTLR2 implementation relied on parser exceptions to report Progress-style runtime errors, while the ANTLR4 parser may recover and continue parsing instead. I'm currently investigating this to better understand the expected behavior and determine the correct migration approach.. If anyone is familiar with how this is intended to work, I'd appreciate any guidance.

While recovering from an error is usually a nice thing to have, in our case we should behave similar to 4GL parser. In most cases if not all, this is to stop at the current location and report the token and its position to ErrorManager to handle. In ANTLR2, this was performed using a CompileException or using showConversionError() as you mentioned.

Hm... I think we may have an issue in trunk. The test case above, along with several others, does not produce exactly the same error as Progress/4GL.

It should report:

Unable to understand after -- "ttCustomer.custNum"

instead of:

Unable to understand after -- "="

This suggests that using the (antlr4) offending token may actually be the correct approach, since it appears to match the behavior expected from Progress, rather than using the last token.

We have not spent time matching their exact token failures. The more important thing is to ensure that we do fail for the same problems as OE fails.

Indeed, the showConversionError() is using heuristics to compose the error message to be printed. I quote from its javadoc (line 1661):

This method does the best effort to match the Progress message by analyzing the failing token.

I understand that exception handling changed significantly between ANTLR2 and ANTLR4. I did not study the latter, but maybe since it is newer it may be configured to report the exceptions in a more flexible way?

ANTLR4 provides two built-in error strategies: DefaultErrorStrategy, which attempts recovery, and BailErrorStrategy, which stops parsing at the first error.

In our case, I am using BailErrorStrategy. Therefore, parser exceptions are wrapped in a ParseCancellationException, and we only need to retrieve the original exception from its cause before passing it to showConversionError().

However, our setup is more complicated because we are still using the ANTLR2 lexer through Antlr2TokenSourceAdapter, which converts ANTLR2 tokens into ANTLR4 tokens. If an exception occurs in the lexer, the adapter propagates it wrapped in a RuntimeException.. This case must also be detected and handled in showConversionError().

Currently we analyse ANTLR2's RecognitionException, CharStreamException and TokenStreamException when an error is reported. IMO, we should drop support for them along with all references to ANTLR2 and rewrite the showConversionError() and catch blocks from DynamicQueryHelper.parse() using ANTLR4 only. That is, avoid any wrapper/adapter between 2 and 4 in the final branch commit, even if in intermediary revisions they might be temporarily used.

10804b rev 16672 contains the necessary changes to preserve the legacy DynamicQueryHelper behavior (along with a few miscellaneous changes).

I used the following testcase to validate the changes. Each queryText variant was tested individually to verify that the reported error matches the original behavior from trunk. The NumberedException and UserGeneratedException paths have not yet been tested. If anyone knows of a testcase that triggers either of these exceptions (or can suggest one), I'd really appreciate the help so we can fully validate the solution.

DEFINE TEMP-TABLE ttCustomer NO-UNDO
   FIELD custNum AS INTEGER
   FIELD name    AS CHARACTER.

DEFINE QUERY qCustomer FOR ttCustomer.

DEFINE VARIABLE prepared  AS LOGICAL   NO-UNDO.
DEFINE VARIABLE queryText AS CHARACTER NO-UNDO.
DEFINE VARIABLE idx       AS INTEGER   NO-UNDO.

queryText = "FOR EACH ttCustomer WHERE ttCustomer.custNum =". 
// queryText = "FOR EACH ttCustomer WHERE )". 
// queryText = "FOR EACH ttCustomer WHERE THEN".
// queryText = 'FOR EACH ttCustomer WHERE ttCustomer.name = "Alice'. // unmatched quote found in procedure. (133, 7323)
// queryText = "FOR EACH ttCustomer /* unterminated comment". // an open comment string was found 134, 7323
prepared = QUERY qCustomer:QUERY-PREPARE(queryText).

MESSAGE
   "Predicate:" queryText SKIP
   "QUERY-PREPARE result:" prepared SKIP
   "ERROR-STATUS:ERROR:" ERROR-STATUS:ERROR SKIP
   "Messages:" ERROR-STATUS:NUM-MESSAGES
   VIEW-AS ALERT-BOX.

DO idx = 1 TO ERROR-STATUS:NUM-MESSAGES:
   MESSAGE
      "Error number:" ERROR-STATUS:GET-NUMBER(idx) SKIP
      "Error message:" ERROR-STATUS:GET-MESSAGE(idx)
      VIEW-AS ALERT-BOX.
END.

Ovidiu, you're right that the current solution is not the most elegant. Unfortunately, we cannot switch to the native ANTLR4 lexer because of the performance impact, so we still need to use the existing ANTLR2 Progress lexer through the adapter. The adapter allows us to keep the lexer performance while migrating the parser to ANTLR4 and preserving the legacy DynamicQueryHelper error-reporting behavior.