[JENKINS-68652] Update ANTLR2 to ANTLR4

[slide]

See JENKINS-68652.

Testing done

Ran the LabelExpressionTest and Cron*Tests

Proposed changelog entries

• Update ANTLR2 grammars and code to ANTLR4.

Proposed upgrade guidelines

N/A

Submitter checklist

• The Jira issue, if it exists, is well-described.
• The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
  • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
• There is automated testing or an explanation as to why this change has no tests.
• [] New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
• New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
• New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
• For dependency updates, there are links to external changelogs and, if possible, full differentials.
• For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@basil @oleg-nenashev

Maintainer checklist

Before the changes are marked as ready-for-merge:

• There are at least two (2) approvals for the pull request and no outstanding requests for change.
• Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
• Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
• Proper changelog labels are set so that the changelog can be generated automatically.
• If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
• If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[slide]

I also built Jenkins with Java 18 and there were no issues during build.

[NotMyFault]

The .gitignore needs an update for generated files:

Launching Jenkins causes https://gist.githubusercontent.com/NotMyFault/0a4c3222354d31106ab2d41610709d7c/raw/d3e2506ab6dbd878ed89d67bcc56ee89e20bf779/gistfile1.txt

[basil]

Thanks for stepping up to contribute to this effort! I pushed a few commits to sort out minor linting issues.

Lots of plugins import antlr.ANTLRException from Jenkins core, so you will have to come up with a strategy for backward compatibility. For example you could define a new class in core/src/main/java/antlr/ANTLRException.java that either extends org.antlr.v4.runtime.misc.ParseCancellationException or org.antlr.v4.runtime.RecognitionException or stands on its own in the type hierarchy with utility methods to convert the ANTLR 4 exception classes to and from instances of core/src/main/java/antlr/ANTLRException.java. Make sure to test that plugins compiled against older Jenkins cores continue to work with these changes and that plugins compiled against these changes don't require source modifications.

[slide]

The code search link doesn't show me anything, is there something I need to setup in GitHub?

[basil]

You're probably just not part of the GitHub Code Search beta program. https://github.com/search?ref=simplesearch&type=Code&q=user%3Ajenkinsci+%22import+antlr%22 should work as long as you are logged into GitHub. You can ignore the results in jenkinsci/groovy since that repository is archived. But the basic point is, this PR cannot be accepted in its current form until compatibility with existing binary signatures is sorted out (and the Testing done section is updated as described in #7293 (review)).

[basil]

Also consider linting the ANTLR4 files by adding

diff --git a/pom.xml b/pom.xml
index b1ef08a9b0..840a7c449d 100644
--- a/pom.xml
+++ b/pom.xml
@@ -391,6 +391,9 @@ THE SOFTWARE.
           <artifactId>spotless-maven-plugin</artifactId>
           <version>${spotless.version}</version>
           <configuration>
+            <antlr4>
+              <antlr4Formatter />
+            </antlr4>
             <java>
               <endWithNewline />
               <importOrder />

and checking in the result of mvn spotless:apply.

[basil]

Launching Jenkins causes https://gist.githubusercontent.com/NotMyFault/0a4c3222354d31106ab2d41610709d7c/raw/d3e2506ab6dbd878ed89d67bcc56ee89e20bf779/gistfile1.txt

This is just a concrete example of the general compatibility problem I described above. The first error there is showing how this PR breaks compatibility with branch-api (something we cannot do) because e.g. OrganizationFolder#onCreatedFromScratch links against antlr.ANTLRException. You can see this at the bytecode level by running javap -verbose jenkins/branch/OrganizationFolder.class.

[slide]

Yes, I obviously don't want to break compatibility. I will work on that later today.

[basil]

Some tips from someone who has done this a few times: think about not only how to cross over into the new world but the long term desired end goal. From the consumers I looked at, a lot of them don't seem like they really care about ANTLRException but rather are just throwing or catching it because they were required to, due to it being a checked exception in the current API. Since org.antlr.v4.runtime.misc.ParseCancellationException is a runtime rather than a checked exception perhaps we could exploit this somehow. For example, if our new ANTLRException extended org.antlr.v4.runtime.misc.ParseCancellationException and we removed any throws ANTLRException, I think source and binary compatibility would be retained in the vast majority of cases we care about. Then we could, at our leisure, go through plugins and clean them up. If they were just throwing or catching ANTLRException to comply with some checked exception signature, the reference to ANTLRException could just be deleted. If they were catching the checked ANTLRException to rethrow it as a runtime exception, that could also be simplified because the new code would already be throwing a RuntimeException. And if plugins really wanted to catch a parse error (e.g. to log it) they could be converted to catch org.antlr.v4.runtime.misc.ParseCancellationException instead (or even the more generic java.util.concurrent.CancellationException or java.lang.IllegalStateException which org.antlr.v4.runtime.misc.ParseCancellationException extends). In that way we could slowly wean plugins off the deprecated APIs after the cutover point. These are just some ideas, but there is no substitute for playing around with a few concrete use cases and seeing what works and what doesn't.

[basil]

Effective Java 3rd Edition §74 says:

Use the Javadoc @throws tag to document each exception that a method can throw, but do not use the throws keyword on unchecked exceptions.

Here and elsewhere, any instances of throws ParseCancellationException should be deleted in favor of a Javadoc @throws tag.

Also consider loosening the contract in the @throws tag to specify something further up in ParseCancellationException's type hierarchy, such as CancellationException or IllegalStateException (and updating the @deprecated message in ANTLRException accordingly). The advantage in declaring the contract in terms of pure Java Platform types is that it prevents callers from relying on a type that is subject to change in the future. In other words, this would be a smart move that would make life easier for whoever is upgrading us from ANTLR4 to the next big thing. By defining our API in terms of pure Java types, we are free to throw a specific subtype in the implementation (like org.antlr.v4.runtime.misc.ParseCancellationException) but also leave ourselves the option of throwing some other type in the future (as long as it inherits from the Java Platform type we specify in the contract) rather than tying ourselves to the specific ANTLR4 implementation.

[basil]

Here and elsewhere, we need to throw our new subtype of ParseCancellationException (ANTLRException) in order to provide actual compatibility (not just linker pacification) for callers like https://github.com/jenkinsci/periodicbackup-plugin/blob/92b9ebaa0758333256e3478ea8cff5bc07b28085/src/main/java/org/jenkinsci/plugins/periodicbackup/PeriodicBackup.java#L76-L78 that are expecting an ANTLRException to be thrown in order to catch it and do something interesting. Although we expect those callers to eventually migrate to catching ParseCancellationException instead (due to the @deprecation Javadoc I added in commit c63d928), existing callers that were compiled against older cores still need to get the same behavior they are already expecting. It is only after we have made a good faith effort to migrate callers from ANTLRException to ParseCancellationException (and released those PRs!) that we can consider changing core's behavior to throw something that is not assignable to ANTLRException.

[slide]

Thanks @basil I will get to this feedback as soon as I can. I am not a full on Java guy, so feedback for these type of things is very much appreciated.

[basil]

I pushed some commits to complete this work:

• Use antlr4-runtime rather than antlr4 to avoid unnecessary JARs in the WAR
• Remove ANTLR4 exception types from API contracts in favor of IllegalArgumentException
• Configure both lexers/parsers consistently (no default error listener, consistent custom error listener, no BailErrorStrategy)
• Throw InputMismatchException in BaseParser for consistency with superclass
• Test compatibility path in unit tests
• Extract ANTLR version to Maven property for easier updating
• Remove unnecessary executions from top-level pom.xml
• Bonus: Add stack trace detail message to TimerTrigger#doCheckSpec

Testing done:

• Verified interactively in the GUI by validating label expressions and cron strings, both correct and incorrect, and confirming the expected error messages were displayed with appropriate line and column numbers
• Verified that cloudbees-folder as well as a custom plugin that both threw and caught ANTLRException, worked fine when compiled against 2.375 but executed against this PR
• Verified that cloudbees-folder as well as a custom plugin that both threw and caught ANTLRException compiled with no errors against the core from this PR
• Verified that cloudbees-folder as well as a custom plugin that defined a custom trigger did not compile against 2.375 after references to ANTLRException were removed but did compile against this PR
• Verified that the WAR file did not include any new unexpected JARs
• Verified that no unexpected log messages were printed to the Jenkins log

[NotMyFault]

Thanks for taking care of this task, @slide! This is an important step forward, not only the migration from a 15+ year old dependency, this upgrade allows compiling Jenkins with Java versions newer than 17 also 🎉

[basil]

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

[slide]

Thanks for the feedback for getting this merged.