antlr4 icon indicating copy to clipboard operation
antlr4 copied to clipboard

Do not throw an error if defined token name conflicts with autogenerated

Open KvanTTT opened this issue 4 years ago • 4 comments

Fix #3020

KvanTTT avatar Dec 30 '20 13:12 KvanTTT

Why wouldn't it be simpler to simply check that the users rule name does not start with our magic T__ prefix?

parrt avatar Jan 02 '21 18:01 parrt

Just check if the defined token starts with T__ and restrict such names? I don't like it because:

  • In my opinion, such a suffix is not required at all because all token names can be deducted automatically based on string literals, see #2361
  • Yet another error message should be introduced token name starts with reserved prefix T__
  • Tokens that start with T__ are not presented in classes that extend ParseRuleContext. For instance, for grammar
grammar test;
root: Token;
Token: 'token';

The following code is generated:

public static class RootContext extends ParserRuleContext {
	public TerminalNode Token() { return getToken(testParser.Token, 0); }
	public RootContext(ParserRuleContext parent, int invokingState) {
		super(parent, invokingState);
	}
	@Override public int getRuleIndex() { return RULE_root; }
}

But if token is defined in implicit way:

grammar test;
root: 'token';

Then Token() method is missing in generated code:

public static class RootContext extends ParserRuleContext {
	public RootContext(ParserRuleContext parent, int invokingState) {
		super(parent, invokingState);
	}
	@Override public int getRuleIndex() { return RULE_root; }
}

KvanTTT avatar Jan 02 '21 19:01 KvanTTT

well this is not a big change, but any change at all in the tool has to have a really good reason given my low involvement and the huge community out there that could be relying on something. If you'd like to add an error message I'm okay with that but I'd rather not introduce any changes to the interface to methods etc..

parrt avatar Jan 02 '21 19:01 parrt

If you'd like to add an error message I'm okay with that

Anyway, this change does not require introducing the new error message at all.

but I'd rather not introduce any changes to the interface to methods etc..

It's extending, not changing. But if even it's changing, covering by bug number of tests almost ensures that everything is fine.

KvanTTT avatar Jan 03 '21 18:01 KvanTTT