Escape bad words during grammar generation

[KvanTTT]

fixes #1070

Deprecate USE_OF_BAD_WORD

Ok, I'm converting this to a pull request since we've already started reviewing and discussing.

[KvanTTT]

I didn't want to make a draft because of failed checks (I haven't completed Go). But actually, it does not care for now.

[KvanTTT]

To get an updated, combined list, the easiest would be to use trxgrep and look for anything that ends in an underscore. But, that would be a merged list for all targets.

I realized it's ok to get a list even for all targets. I'll create a big test with all these words and run it on different runtimes. Failed runtimes will indicate correct targets of these words :)

[kaby76]

Repeating what I said over in the closed Issue, here is the aggregated bad word list (with underscores) derived from symbols in grammars-v4.

all_underscore.txt

[KvanTTT]

@parrt I think it makes sense to review and merge it now since there are a lot of fixes already. I'll add other reserved words provided by @kaby76 and fix potential bugs in the following requests (but now all tests are okay).

I decided to use orginalName for a name from grammar and escapedName for a name in runtime.

[parrt]

I decided to use orginalName for a name from grammar and escapedName for a name in runtime.

I always worry about making the smallest possible change, but I guess this makes sense to highlight the difference now.

Just so I'm clear, the list of reserved words will always be specific to a target right? I don't think it makes sense to have a general list that is always escaped.

I will try to review this branch today

[parrt]

looking really good! thanks for your efforts here. I just have a number of tiny comments and then the one issue about naming. I think we want to leave <r.name> in the code generation templates as the original name and keep it consistent so that field name in support code should be the original name. We then have name and escapedName or whatever in the code generation templates, and a field if we need it that way.

[KvanTTT]

Just so I'm clear, the list of reserved words will always be specific to a target right? I don't think it makes sense to have a general list that is always escaped.

Generally yes, but I'm trying to figure out if common words exist (for instance, it looks like the word rule is common for all targets, but I haven't found out why it is so yet).

[parrt]

Generally yes, but I'm trying to figure out if common words exist (for instance, it looks like the word rule is common for all targets, but I haven't found out why it is so yet).

Yeah, that is really strange. There must be a quirk of code generation that I/we were trying to overcome. I say we try it without those weird reserved words and see what happens.

[KvanTTT]

I think we want to leave <r.name> in the code generation templates as the original name and keep it consistent so that field name in support code should be the original name. We then have name and escapedName or whatever in the code generation templates, and a field if we need it that way.

Do you mean escapedName should be always the escaped name independence of class package (tool or runtime)? originalName should be eliminated since name is always the original name. It's good for consistency and clarity but may cause more template changes that you'd like to get rid as I understand (but I can try, maybe not so massive as I think). Also, templates use classes both from tool package (Rule) and from codegen package (RuleFunction).

[parrt]

Do you mean escapedName should be always the escaped name independence of class package (tool or runtime)?

I would imagine that the simplest is that name is always the original name and escapedName is either the original name, or if necessary for that code gen target, the escaped name. In other words, the code generation templates would always refer to name when they know for a fact there's no collision, such as in a string "<r.name>". But, if there is a chance that the name could cause a problem in the generated code, we would always refer to it as <r.escapedName> even if the name is not actually escaped. Does that make sense?

originalName should be eliminated since name is always the original name.

Yep!

[KvanTTT]

But, if there is a chance that the name could cause a problem in the generated code, we would always refer to it as <r.escapedName> even if the name is not actually escaped. Does that make sense?

Yes, it sounds reasonable and I like that. The only thing I was afraid of is massive template changes. Ok, I will try to implement it.

[kaby76]

I'm also trying to envision how the changed code would work.

Let's suppose the grammar Foobar.g4 grammar Foobar; boolean: 'true' | 'false';, with pom.xml for the Antlr4 Maven Plugin that gives entry point "boolean". This grammar is valid in CSharp but invalid in Java.

• The grammar works for C#. I can generate a working C# parser, and there would be no compilation errors. The Antlr tool would work and the compilation of the generated files and existing code would work fine.
• This code will not work for Java. If I try to do this for the Java target (java -jar C:/Users/Kenne/Downloads/antlr-4.9.3-complete.jar -Dlanguage=Java -encoding utf-8 Foobar.g4), I cannot because the Antlr tool will emit error(134): Foobar.g4:2:0: symbol boolean conflicts with generated code in target language or runtime.

Up to now, if I wanted this "target agnostic", I'd have to change quite a bit. In the .g4 file, I would change boolean to boolean_; In the pom.xml, I would change <entryPoint>boolean</entryPoint> to <entryPoint>boolean_</entryPoint>. If I had written a listener or visitor, I would have to change that because the renamed symbol would result in new generated code declarations.

With this PR change, if I didn't make any changes to the grammar, would I have to change anything in the driver, visitors or listeners I wrote in C#? Is the entry point in C# still parser.boolean();?

For Java, it would all be new code because the Antlr tool wouldn't have even generated any Java code prior to the PR. What would be the entry point be? parser.boolean() works fine for C# but not for Java, right? Are you planning on a string-named entry point, e.g., parser.EntryPoint("boolean") instead of "parser.boolean()"?

[parrt]

Fantastic work @KvanTTT. beautiful. I just have that one nit about the naming of the superclass chunk... what you think?

[KvanTTT]

Also, dart tests are failed. But it looks like a problem on CI:

Error: Could not resolve the package 'antlr4' in 'package:antlr4/antlr4.dart'.

It works fine locally.

[KvanTTT]

As I noted here and here, I am working out a reserved word list per target.

Thank you for your work but wow I think it's enough to have global bad words list because I've already included them in a big test that I'm going to add in the following pull request. So, there is no need to split them by different runtimes.

Ok, reserved word lists per target are also useful because with them I can update reserved words in ANTLR tool, probably remove actually not reserved words.

[kaby76]

Here are "bad words" for C#, Java, Dart, JavaScript. Working out C++, Go, Python3, PHP.

final-list.javascript.txt
final-list.csharp.txt
final-list.dart.txt
final-list.java.txt

Again, this uses a custom Antlr 4.9.3 tool with minimal bad word list, on a grammar with the symbol (lexer or parser) in a tiny grammar, trgen to generate a parser, build of the parser, and a parse in order to test from start to end what is a bad word.

[kaby76]

Attached are the last two "bad words", for Cpp and Go.

final-list.cpp.txt
final-list.go.txt

I did a check to see if this PR fixes Issue #1070 that it claims to fix. Indeed, there are many "bad words" that are not listed in the tables at

antlr4/tool/src/org/antlr/v4/codegen/target/GoTarget.java

Line 27 in 7d2a37e

protected static final HashSet<String> reservedWords = new HashSet<>(Arrays.asList(

, and are not mangled on the generated files.

Checking the simple grammar grammar Foobar; x : Accept; Accept: 'xxx'; against the latest source for the Antlr tool, this works fine for CSharp, but it does not for Go: the "go build" fails. There is no name mangling for Accept.

This change does not fix "symbol conflicts": I will still have to change a "target agnostic" grammar for Go that declares a lexer rule for "Accept".

[parrt]

Ok, @KvanTTT: care to update the word lists?

[KvanTTT]

Sure, I'll update the word lists but a bit later. I've encountered yet another symbol conflicts problem and I'm thinking how to resolve them in a better way.

Also, I think there is another way of bad words extracting. They can be extracted analytically based on runtime code, not only empirically based on incorrect generated code. For instance, we have bad word terminal in Dart runtime because there is conflict with default listener method enterTerminal:

  /// The default implementation does nothing.
  @override
  void enterEveryRule(ParserRuleContext ctx) {}

  /// The default implementation does nothing.
  @override
  void exitEveryRule(ParserRuleContext ctx) {}

  /// The default implementation does nothing.
  @override
  void visitTerminal(TerminalNode node) {}

  /// The default implementation does nothing.
  @override
  void visitErrorNode(ErrorNode node) {}

But other problem words such as everyRule, errorNode can be extracted as well. This approach works with other runtimes as well.

[kaby76]

To compute the "bad word" list analytically, one would have to look at the .stg files (since the name that is generated is some string in the .stg file (e.g. Enter<lname; format="cap">), along with computed strings by the Tool), the Antlr runtimes (all public methods, fields, etc.), and any other libraries that one commonly uses. For the Cpp target, we include stddef.h, which defines NULL. Any preprocessor symbol could turn the generated code on its head, especially in the first table in the generated source code for the parser that enumerates the lexer grammar symbols. One can't anticipate all the symbol conflicts at tool invocation time because the generated code will exist in an environment that one cannot predict.

[KvanTTT]

Probably we should combine both approaches to extract the most general bad words list.

[kaby76]

Sure. An update to the script is here.
try-symbol.txt

But this illustrates why I've been an advocate of parameterizing templates, including the attributes and values themselves. This is exactly why I wrote trgen to allow the CI tester in grammars-v4 to specify the templates rather than just rely on hardwired templates, but doesn't go nearly far enough. Of course, it goes against "strict model-view separation".

[KvanTTT]

@kaby76 take attention the following words are reserved in ANTLR, it's not necessary to add them to the bad words list:

• catch
• finally
• fragment
• grammar
• import
• options
• throws

Also, for the following words ANTLR reports RESERVED_RULE_NAME or similar errors, they also shouldn't be added:

• EOF
• MORE
• SKIP

[KvanTTT]

Here are "bad words" for C#, Java, Dart, JavaScript. Working out C++, Go, Python3, PHP.

Hi! Do you have updates for Python and PHP?

[kaby76]

Hi! Do you have updates for Python and PHP?

Sorry Ivan, I hadn't prioritized working on this. I can work out the PHP keywords the next couple of days. Then, I'll check what was done for Python. (I've been working out a machine learning scraper for grammars from documents in Trash.)

[KvanTTT]

Ok, no worry, no rush. I'm just checking the status.

[kaby76]

In determining the reserved words in the PHP target, I tested the tool and target against the current list of reserved words in PHPTarget.java with a special-built tool that removes all reserved word checks for PHP. In other words, I emptied out the list, built and ran the tool against the latest "dev" branch code to observe any problems. Note, I left in the double underscore symbols alone, even though one cannot use them in a grammar, i.e., you cannot define a TOKEN_REF or RULE_REF with the underscore character leading the name.

It turns out almost all the so-called "reserved words" do not cause any problem whatsoever. Attached is a grammar that I used to check the list and the generated PHP code.
Test.txt
TestParser.txt
TestListener.txt
TestVisitor.txt

I am using is PHP 7.4.3 (cli) (built: Nov 25 2021 23:16:22) ( NTS ). I have not tested this against PHP 8. There is no stated minimum requirements in the online documentation (), but Composer for antlr/antlr4-php-runtime states it is for "PHP 7 and 8.0 runtime for ANTLR 4". Really, properly released software should test the software against a stated minimum and maximum version, and tested in the CI build.

I will continue the experimental check using a version of grammars-v4 modified to remove the trailing underscores in symbol names. (I can do this quickly using Trash trrename.)

[parrt]

ok, please let us know if we can simplify the list of naughty words. wow PHP is very permissive I see.

[kaby76]

Yes, the PHP naughty word list can be shortened. Testing the "_" word list in grammars-g4, but I have a feeling it will be a small list. I agree...PHP is very robust here.

[parrt]

Ok, please let me know here #3460 if you would like to make a PR to reduced things; maybe coordinate with @KvanTTT

[kaby76]

I performed the additional tests for symbol conflicts for the PHP target. Here are the results and discussion.

Reserved words for PHP

CLASS
addContextToParseTree
addParseListener
catch
consume
createErrorNode
createTerminalNode
dumpDFA
enterOuterAlt
enterRecursionRule
enterRule
exitRule
finally
getATNWithBypassAlts
getBuildParseTree
getContext
getCurrentRuleName
getCurrentToken
getDFAStrings
getErrorHandler
getExpectedTokens
getExpectedTokensWithinCurrentRule
getInputStream
getInvokingContext
getNumberOfSyntaxErrors
getParseListeners
getPrecedence
getRuleIndex
getRuleInvocationStack
getSourceName
getTokenFactory
getTokenStream
inContext
isExpectedToken
isMatchedEOF
isTrace
match
matchWildcard
notifyErrorListeners
parserRule
precpred
pushNewRecursionContext
removeParseListener
removeParseListeners
reset
setBuildParseTree
setContext
setErrorHandler
setInputStream
setTokenFactory
setTokenStream
setTrace
triggerEnterRuleEvent
triggerExitRuleEvent
unrollRecursionContexts

Discussion

1. PHP doesn't seem to tell me in one swoop all duplicated definitions. This was a problem in testing, forcing me to edit ONE non-terminal (aka "parser rule") at a time, build, and test. PHP seems to output one error, then quit. So, if there are multiple problems with the grammar, finding and correcting all problems is time consuming.
2. There is only one terminal (aka, "lexer rule") that results in a symbol conflict: CLASS. There may be others.
3. Most, but not all, methods in the parser class result in a symbol conflict. E.g., if the rule "match:'a';" is defined, then it will result in a symbol conflict. What this means is that this list is fragile: what is a "bad word" will depend on the runtime. If someone modifies the parser class with a new method, that method name will need to be tested and possibly entered in the "bad word" list. I haven't researched why "context" is a valid parser rule name when there is a method in the parser class by that name as well. There are other names like this.

Attached is a grammar file with the additional conflicts (i.e., the commented out rules).

Test.txt

I would prefer not to make the changes to the code. There are other PRs I have to complete, and I have a lot of work to do on Trash. Thanks.