Java Provide default implementations for ParseTreeListener exit methods and others

[incident-recipient]

Target: Java

Feature Request

We use ANTLR daily to create dozens of small, shared languages for testing large systems. These DSLs help us stay coordinated in a way that would otherwise not be possible.

When using Java we prefer to use the ParseTreeListener because it adds type safety that we need when someone modifies a DSL (because multiple listeners are implemented across different projects for the same grammar).

However we keep finding ourselves providing stubbed implementations for the exit methods. We also occasionally get burned by members forgetting to make visitErrorNode throw an exception.

We propose a middle ground in #3985 that will make it so that all enter methods must be implemented (guaranteeing type safety) while making the implementation for exit methods and most of the standard ParseTreeListener methods optional. We also propose throwing a runtime exception on visitErrorNode by default.

Since ANTLR upgraded to Java 8 we can now use the default keyword in the generated interface to provide this behavior without changing the API.

We feel this will make ANTLR a much more viable tool for cross functional projects.

[ericvergnaud]

May I ask why you're not using the generated BaseParserListener ?
It already provides the stub methods.
(I'll give you that we could now have all the stubs generated as default interface methods)

[ericvergnaud]

I'm a bit skeptical re your proposal to make enter methods mandatory while making exit ones optional. Many developers, including myself override exit methods and do not care about enter methods. As a library provider, not sure we can favor half of the community against the other half ?

[incident-recipient]

The problem with the base listener is it doesn't provide type safety when another team modifies the grammar by adding a new rule. It makes it possible for other implementations to silently do nothing without other teams realizing it. This causes code to break in ways that are difficult to track down at scale. What we want is for a compile error to occur until all other teams explicitly handle the new rule.

But since you've brought it up, it seems like many people using ANTLR only care about half of the listener methods. Would any of these alternatives be feasible?

1. If most people only care about the exit methods perhaps we could have the enter methods by default do nothing?
2. Generate an additional interface in addition to the parse tree listener or base listener that acts as a middle ground
3. I fork the project and modify it to be suitable for Google's use cases.

[ericvergnaud]

"It makes it possible for other implementations to silently do nothing without other teams realizing it"
If they're using the @Override annotation, then it's not silent, and if they don't it's not silent either, it it ?
Unless they turn warnings off ?
The strength of a build chain is determined by its weakest element, I'm not convinced that increasing requirements at code generation level will help.
A 4th option that does not require any change on Antlr side is to run a post-processor that removes all the 'exitXXX' methods from the generated BaseListener.

[incident-recipient]

I think there is a misunderstanding here. If someone extends the base listener there is no requirement for anyone to @OverRide any new added method because, well, it is already inherited with an empty implementation. This is where the silent failures can get introduced.

The problem with using the interface instead is that roughly half of all generated methods are no-ops and are forced to be given empty implementations. From a design point of view this is undesirable because it violates the Interface Segregation Principle. It's fairly safe to make this claim when most users of ANTLR (including both of us) are outright ignoring half of the methods because we don't use them. In my experience this has been somewhat egregious because there are often dozens of such rules being given empty implementations.

While I have no problem taking an alternative route (thank you for proposing the feasibility of using a post-processor) it doesn't feel like a strength when half of the generated code is being ignored by the community on a regular basis. I'm not looking to persuade you on this point, just highlighting that it is a pain point we all deal with.

If this still doesn't seem like a good fit for the library feel free to close the issue and I'll pursue one of the alternative solutions. Otherwise I'm happy to continue the discussion if it's deemed productive.

[ericvergnaud]

Am I correct in observing that you have an unusual setup, where grammar producers and grammar consumers sit in different teams ?
If I'm honest, I suspect that 99% of our users are both grammar producers and consumers. If that's the case, they don't have the problem that you're trying to solve, or should I say, they have it, but it's not a pain point because they know what change they need to implement in their listeners ? (I personally encounter this each time I modify a grammar)
I'll let @parrt opine if it's worth a change.
Technically speaking, my preferred approach would be to add options for not generating enterXXX or not generate exitXXX methods in the BaseListener (making it abstract). That would be non-breaking. Obviously we'd have to implement this with proper testing across the 10 language targets...

[incident-recipient]

You are correct. The producers and consumers sit in different teams.

If @parrt feels like it is a worthwhile change I'm happy to to make the changes and write tests for the language targets. The modifications to the stg files seem like they'd be simple enough anyway.

[parrt]

Hi @incident-recipient if this is an internal google thing, perhaps contact me inside the wall and you can tell me more specifically what's going on with the project. I'm nervous about changing this interface, particularly because it would require a change across nine other languages. It's getting very hard for me to keep up with all of the support, given the explosion of languages then tests etc... I wonder if it's a simple matter of providing a Java.stg during the build process via blaze, possibly with a tweet .bzl file from third_party