Replace ANTLR parser with CongoCC - #958
Conversation
| } else if (ctx instanceof Token) { | ||
| Token token = (Token) ctx; | ||
| location = source.newLocation(token.getLine(), token.getCharPositionInLine()); | ||
| if (ctx instanceof Node) { |
There was a problem hiding this comment.
nit: arent we on java 17 now?
| if (e.isEmpty()) { | ||
| assertThat(actualErr).isEmpty(); | ||
| } else { | ||
| assertThat(actualErr).isNotEmpty(); |
There was a problem hiding this comment.
isnt this a serious loss of coverage i.e. the expected errors now dont get asserted?
| .isInstanceOf(ScriptCreateException.class) | ||
| .hasMessageStartingWith( | ||
| "parse failed: ERROR: <input>:1:3: Syntax error: mismatched input ',' expecting IDENTIFIER"); | ||
| "parse failed: ERROR: <input>:1:3: Syntax error: Encountered an error"); |
There was a problem hiding this comment.
is there no way to keep providing more context to the user?
|
|
||
| parser.addParseListener(parserListener); | ||
|
|
||
| parser.setErrorHandler(new RecoveryLimitErrorStrategy(options.getErrorRecoveryLimit())); |
There was a problem hiding this comment.
options.getErrorRecoveryLimit() still exists but is no longer respected ?
are we missing test coverage here ?
There was a problem hiding this comment.
Yea, that's a downside - documented it.
| return reportError(node, "invalid literal"); | ||
| } | ||
|
|
||
| private boolean isNegativeNumericLiteral(Node operand) { |
There was a problem hiding this comment.
pardon my ignorance, but where is the "negative" property checked in the method body?
| return text; | ||
| } | ||
|
|
||
| private Node firstExpressionChild(List<Node> children, Node ctx) { |
There was a problem hiding this comment.
cant many of these helper methods be static?
Move CEL grammar generation into cel-core using CongoCC and remove the generated-antlr module, shadow jar dependency, and ANTLR runtime wiring. The generated parser now builds internal AST nodes with typed CEL expression conversion hooks, keeping parser construction close to the grammar while preserving the existing parser API and source-location handling. This simplifies dependency management for downstream consumers, removes the relocated ANTLR artifact from the build graph, and makes future grammar changes easier to review in the core module.
XN137
left a comment
XN137
left a comment
There was a problem hiding this comment.
thanks for the updates
guess i'll trust your judgment that the benefits of switching to congocc outweigh the drawbacks mentioned
| * Returns the maximum number of parser error-recovery attempts. | ||
| * | ||
| * <p>This setting is currently not respected because CongoCC stops parsing after the first syntax | ||
| * error and does not provide error recovery. |
There was a problem hiding this comment.
should we just remove this option or is there a realistic chance we can support it again in the future?
There was a problem hiding this comment.
Just leaving it to not break existing call sites. Maybe there's a way to get that back with CongoCC, but that would then likely be a bigger change.
| } else if (e.contains(": Syntax error:")) { | ||
| // Parser implementations may format syntax diagnostics differently, but they must point to | ||
| // the same first invalid source location. | ||
| assertThat(actualErr).startsWith(firstErrorLocation(e) + ": Syntax error:"); |
There was a problem hiding this comment.
wondering:
why is it better to adjust the check here instead of updating our tests to the new error reporting format?
2 participants
Move CEL grammar generation into cel-core using CongoCC and remove the generated-antlr module, shadow jar dependency, and ANTLR runtime wiring.
The generated parser now builds internal AST nodes with typed CEL expression conversion hooks, keeping parser construction close to the grammar while preserving the existing parser API and source-location handling.
This simplifies dependency management for downstream consumers, removes the relocated ANTLR artifact from the build graph, and makes future grammar changes easier to review in the core module.