Copilot commented on code in PR #7096:
URL: https://github.com/apache/incubator-kie/pull/7096#discussion_r3976591373
##########
drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/Drl6ExprParser.java:
##########
@@ -68,6 +70,26 @@ public ConstraintConnectiveDescr parse( final String text ) {
return constraint;
}
+ /**
+ * Returns whether to preserve the eval wrapper around the supplied
contents.
+ * The constraint parser enters at conditionalOrExpression, which is below
+ * ternaryExpression in the grammar, so a top-level ternary's branches are
+ * silently discarded. Question-mark tokens at any nesting depth may
indicate
+ * such a ternary. Strings and comments are ignored.
+ * Lexer errors also preserve the wrapper, leaving validation to
compilation.
+ * This is a conservative check, not validation of ternary syntax.
+ */
+ public static boolean shouldPreserveEval(String expression) {
+ DRL6Lexer lexer = new DRL6Lexer(new ANTLRStringStream(expression));
+ for (Token token = lexer.nextToken(); token.getType() != Token.EOF;
token = lexer.nextToken()) {
+ // QUESTION_DIV also covers a ternary immediately followed by a
comment: x?/*...*/y:z.
+ if (token.getType() == DRL6Lexer.QUESTION || token.getType() ==
DRL6Lexer.QUESTION_DIV) {
+ return true;
+ }
+ }
+ return !lexer.getErrors().isEmpty();
+ }
+
Review Comment:
This heuristic returns `true` for any `?` token (not just ternary `?:`),
which will prevent `normalizeEval(...)` from turning an `eval(...)` into an
indexable/reactive constraint in many non-ternary cases (e.g., null-safe
navigation `?.`). If the intent is specifically to avoid ternary truncation,
consider tightening the detection to ternary-like patterns (for example,
requiring a matching `:` outside strings/comments and respecting nesting) so
that `eval(...)` is preserved only when necessary—reducing the chance of
degrading constraint indexing/reactivity and compilation performance for common
non-ternary expressions.
##########
drools-compiler/src/main/java/org/drools/compiler/rule/builder/PatternBuilder.java:
##########
@@ -1805,7 +1806,12 @@ protected ConstraintConnectiveDescr
parseExpression(final RuleBuildContext conte
final BaseDescr
original,
final String
expression) {
DrlExprParser parser =
DrlExprParserFactory.getDrlExprParser(context.getConfiguration().getOption(LanguageLevelOption.KEY));
- ConstraintConnectiveDescr result =
parser.parse(normalizeEval(expression));
+ String toParse = normalizeEval(expression);
+ if (!toParse.equals(expression) &&
Drl6ExprParser.shouldPreserveEval(toParse)) {
Review Comment:
`parseExpression(...)` selects a `DrlExprParser` based on
`LanguageLevelOption`, but the preservation decision is hard-wired to
`Drl6ExprParser` / `DRL6Lexer`. For non-DRL6 language levels, this can produce
incorrect “preserve vs normalize” decisions (including preservation triggered
by DRL6-lexer errors on otherwise valid expressions in another language level),
changing the parse path unexpectedly. Consider making the preservation check
language-level aware (e.g., expose it via the selected `DrlExprParser`
implementation / factory, or move to a language-level neutral scanner that
doesn’t depend on DRL6 tokenization).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
