Skip to content

Commit 2d71775

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Fix ConstantFoldingOptimizer to not treat true && dyn_x as a tautology
true && bool_x continues to fold to bool_x PiperOrigin-RevId: 952324181
1 parent db6432f commit 2d71775

2 files changed

Lines changed: 100 additions & 46 deletions

File tree

‎optimizer/src/main/java/dev/cel/optimizer/optimizers/ConstantFoldingOptimizer.java‎

Lines changed: 68 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,19 @@
7373
* calls and select statements with their evaluated result.
7474
*/
7575
public final class ConstantFoldingOptimizer implements CelAstOptimizer {
76+
private static final ImmutableSet<String> BOOLEAN_RETURN_OPERATORS =
77+
ImmutableSet.of(
78+
Operator.LOGICAL_AND.getFunction(),
79+
Operator.LOGICAL_OR.getFunction(),
80+
Operator.LOGICAL_NOT.getFunction(),
81+
Operator.EQUALS.getFunction(),
82+
Operator.NOT_EQUALS.getFunction(),
83+
Operator.LESS.getFunction(),
84+
Operator.LESS_EQUALS.getFunction(),
85+
Operator.GREATER.getFunction(),
86+
Operator.GREATER_EQUALS.getFunction(),
87+
Operator.IN.getFunction());
88+
7689
private static final ConstantFoldingOptimizer INSTANCE =
7790
new ConstantFoldingOptimizer(ConstantFoldingOptions.newBuilder().build());
7891

@@ -123,7 +136,6 @@ public OptimizationResult optimize(CelAbstractSyntaxTree ast, Cel cel)
123136
}
124137
iterCount++;
125138
continueFolding = false;
126-
127139
ImmutableList<CelNavigableMutableExpr> foldableExprs =
128140
CelNavigableMutableAst.fromAst(mutableAst)
129141
.getRoot()
@@ -210,6 +222,9 @@ private boolean canFold(CelNavigableMutableExpr navigableExpr) {
210222

211223
if (functionName.equals(Operator.EQUALS.getFunction())
212224
|| functionName.equals(Operator.NOT_EQUALS.getFunction())) {
225+
if (hasComprehensionVar(navigableExpr)) {
226+
return false;
227+
}
213228
if (mutableCall.args().stream()
214229
.anyMatch(node -> isExprConstantOfKind(node, CelConstant.Kind.BOOLEAN_VALUE))
215230
|| mutableCall.args().stream()
@@ -219,7 +234,7 @@ private boolean canFold(CelNavigableMutableExpr navigableExpr) {
219234
}
220235

221236
if (functionName.equals(Operator.IN.getFunction())) {
222-
return canFoldInOperator(navigableExpr);
237+
return !hasComprehensionVar(navigableExpr);
223238
}
224239

225240
// Default case: all call arguments must be constants. If the argument is a container (ex:
@@ -248,32 +263,31 @@ private static boolean isCallTimestampOrDuration(CelMutableCall call) {
248263
|| call.function().equals(DURATION.functionName());
249264
}
250265

251-
private static boolean canFoldInOperator(CelNavigableMutableExpr navigableExpr) {
252-
ImmutableList<CelNavigableMutableExpr> allIdents =
253-
navigableExpr
254-
.allNodes()
255-
.filter(node -> node.getKind().equals(Kind.IDENT))
256-
.collect(toImmutableList());
257-
for (CelNavigableMutableExpr identNode : allIdents) {
258-
CelNavigableMutableExpr parent = identNode.parent().orElse(null);
259-
while (parent != null) {
260-
if (parent.getKind().equals(Kind.COMPREHENSION)) {
261-
String identName = identNode.expr().ident().name();
262-
CelMutableComprehension parentComprehension = parent.expr().comprehension();
263-
if (parentComprehension.accuVar().equals(identName)
264-
|| parentComprehension.iterVar().equals(identName)
265-
|| parentComprehension.iterVar2().equals(identName)) {
266-
// Prevent folding a subexpression if it contains a variable declared by a
267-
// comprehension. The subexpression cannot be compiled without the full context of the
268-
// surrounding comprehension.
269-
return false;
270-
}
271-
}
272-
parent = parent.parent().orElse(null);
273-
}
274-
}
275-
276-
return true;
266+
private static boolean hasComprehensionVar(CelNavigableMutableExpr expr) {
267+
return expr.allNodes()
268+
.filter(node -> node.getKind().equals(Kind.IDENT))
269+
.anyMatch(
270+
identNode -> {
271+
String identName = identNode.expr().ident().name();
272+
CelNavigableMutableExpr curr = identNode;
273+
Optional<CelNavigableMutableExpr> maybeParent = curr.parent();
274+
while (maybeParent.isPresent()) {
275+
CelNavigableMutableExpr parent = maybeParent.get();
276+
if (parent.getKind().equals(Kind.COMPREHENSION)) {
277+
CelMutableComprehension compre = parent.expr().comprehension();
278+
if ((compre.accuVar().equals(identName)
279+
|| compre.iterVar().equals(identName)
280+
|| compre.iterVar2().equals(identName))
281+
&& curr.id() != compre.iterRange().id()
282+
&& curr.id() != compre.accuInit().id()) {
283+
return true;
284+
}
285+
}
286+
curr = parent;
287+
maybeParent = parent.parent();
288+
}
289+
return false;
290+
});
277291
}
278292

279293
private static boolean areChildrenArgConstant(CelNavigableMutableExpr expr) {
@@ -311,6 +325,9 @@ private Optional<CelMutableAst> maybeFold(
311325
CelMutableAst mutableAst,
312326
CelNavigableMutableExpr node)
313327
throws CelOptimizationException, CelEvaluationException {
328+
if (!node.getKind().equals(Kind.COMPREHENSION) && hasComprehensionVar(node)) {
329+
return Optional.empty();
330+
}
314331
Object result;
315332
try {
316333
result = evaluateExpr(cel, node);
@@ -527,15 +544,15 @@ private Optional<CelMutableAst> maybePruneBranches(
527544
// If both args are const, don't prune any branches and let maybeFold method evaluate this
528545
// subExpr
529546
return Optional.empty();
530-
} else if (lhsIsBoolean) {
547+
} else if (lhsIsBoolean && isBoolean(mutableAst, rhs)) {
531548
boolean cond = invertCondition != lhs.constant().booleanValue();
532549
replacementExpr =
533550
Optional.of(
534551
cond
535552
? rhs
536553
: CelMutableExpr.ofCall(
537554
CelMutableCall.create(Operator.LOGICAL_NOT.getFunction(), rhs)));
538-
} else if (rhsIsBoolean) {
555+
} else if (rhsIsBoolean && isBoolean(mutableAst, lhs)) {
539556
boolean cond = invertCondition != rhs.constant().booleanValue();
540557
replacementExpr =
541558
Optional.of(
@@ -583,14 +600,34 @@ private Optional<CelMutableAst> maybeShortCircuitCall(
583600
return Optional.of(astMutator.replaceSubtree(mutableAst, shortCircuitTarget, expr.id()));
584601
}
585602
if (newArgs.size() == 1) {
586-
return Optional.of(astMutator.replaceSubtree(mutableAst, newArgs.get(0), expr.id()));
603+
CelMutableExpr remainingArg = newArgs.get(0);
604+
if (isBoolean(mutableAst, remainingArg)) {
605+
return Optional.of(astMutator.replaceSubtree(mutableAst, remainingArg, expr.id()));
606+
}
607+
return Optional.empty();
587608
}
588609

589610
// TODO: Support folding variadic AND/ORs.
590611
throw new UnsupportedOperationException(
591612
"Folding variadic logical operator is not supported yet.");
592613
}
593614

615+
private boolean isBoolean(CelMutableAst mutableAst, CelMutableExpr expr) {
616+
if (isExprConstantOfKind(expr, CelConstant.Kind.BOOLEAN_VALUE)) {
617+
return true;
618+
}
619+
// The AST's type map relies on the type-checker having explicitly populated the type for a
620+
// given node. However, during the optimization pipeline, mutated intermediate nodes might
621+
// temporarily lack type metadata. Standard CEL operators like &&, ||, and == inherently
622+
// always return a boolean, so checking the function name provides a reliable fallback when
623+
// the type map is incomplete.
624+
if (expr.getKind().equals(Kind.CALL)
625+
&& BOOLEAN_RETURN_OPERATORS.contains(expr.call().function())) {
626+
return true;
627+
}
628+
return mutableAst.getType(expr.id()).map(SimpleType.BOOL::equals).orElse(false);
629+
}
630+
594631
private boolean isFoldedAggregateLiteral(CelMutableExpr expr) {
595632
if (expr.getKind().equals(Kind.CONSTANT)) {
596633
return true;

‎optimizer/src/test/java/dev/cel/optimizer/optimizers/ConstantFoldingOptimizerTest.java‎

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ private static Cel setupEnv(CelBuilder celBuilder) {
8080
return celBuilder
8181
.addVar("x", SimpleType.DYN)
8282
.addVar("y", SimpleType.DYN)
83+
.addVar("bool_var", SimpleType.BOOL)
8384
.addVar("list_var", ListType.create(SimpleType.STRING))
8485
.addVar("map_var", MapType.create(SimpleType.STRING, SimpleType.STRING))
8586
.setStandardMacros(CelStandardMacro.STANDARD_MACROS)
@@ -127,17 +128,16 @@ private static Cel setupEnv(CelBuilder celBuilder) {
127128
@TestParameters("{source: 'false || false', expected: 'false'}")
128129
@TestParameters("{source: 'true && false || true', expected: 'true'}")
129130
@TestParameters("{source: 'false && true || false', expected: 'false'}")
130-
@TestParameters("{source: 'true && x', expected: 'x'}")
131-
@TestParameters("{source: 'x && true', expected: 'x'}")
131+
@TestParameters("{source: 'true && bool_var', expected: 'bool_var'}")
132+
@TestParameters("{source: 'bool_var && false', expected: 'false'}")
133+
@TestParameters("{source: 'bool_var && true', expected: 'bool_var'}")
134+
@TestParameters("{source: 'false || [1 + 2, x][0]', expected: 'false || [3, x][0]'}")
132135
@TestParameters("{source: 'false && x', expected: 'false'}")
133136
@TestParameters("{source: 'x && false', expected: 'false'}")
134137
@TestParameters("{source: 'true || x', expected: 'true'}")
135138
@TestParameters("{source: 'x || true', expected: 'true'}")
136-
@TestParameters("{source: 'false || x', expected: 'x'}")
137-
@TestParameters("{source: 'x || false', expected: 'x'}")
138-
@TestParameters("{source: 'true && x && true && x', expected: 'x && x'}")
139-
@TestParameters("{source: 'false || x || false || x', expected: 'x || x'}")
140-
@TestParameters("{source: 'false || x || false || y', expected: 'x || y'}")
139+
@TestParameters("{source: 'false || bool_var', expected: 'bool_var'}")
140+
@TestParameters("{source: 'bool_var || false', expected: 'bool_var'}")
141141
@TestParameters("{source: 'true ? x + 1 : x + 2', expected: 'x + 1'}")
142142
@TestParameters("{source: 'false ? x + 1 : x + 2', expected: 'x + 2'}")
143143
@TestParameters(
@@ -230,10 +230,10 @@ private static Cel setupEnv(CelBuilder celBuilder) {
230230
@TestParameters("{source: 'sets.contains([1], [1])', expected: 'true'}")
231231
@TestParameters(
232232
"{source: 'cel.bind(r0, [1, 2, 3], cel.bind(r1, 1 in r0, r1))', expected: 'true'}")
233-
@TestParameters("{source: 'x == true', expected: 'x'}")
234-
@TestParameters("{source: 'true == x', expected: 'x'}")
235-
@TestParameters("{source: 'x == false', expected: '!x'}")
236-
@TestParameters("{source: 'false == x', expected: '!x'}")
233+
@TestParameters("{source: 'bool_var == true', expected: 'bool_var'}")
234+
@TestParameters("{source: 'true == bool_var', expected: 'bool_var'}")
235+
@TestParameters("{source: 'bool_var == false', expected: '!bool_var'}")
236+
@TestParameters("{source: 'false == bool_var', expected: '!bool_var'}")
237237
@TestParameters("{source: 'true == false', expected: 'false'}")
238238
@TestParameters("{source: 'true == true', expected: 'true'}")
239239
@TestParameters("{source: 'false == true', expected: 'false'}")
@@ -257,10 +257,10 @@ private static Cel setupEnv(CelBuilder celBuilder) {
257257
@TestParameters("{source: 'false == false', expected: 'true'}")
258258
@TestParameters("{source: '10 == 42', expected: 'false'}")
259259
@TestParameters("{source: '42 == 42', expected: 'true'}")
260-
@TestParameters("{source: 'x != true', expected: '!x'}")
261-
@TestParameters("{source: 'true != x', expected: '!x'}")
262-
@TestParameters("{source: 'x != false', expected: 'x'}")
263-
@TestParameters("{source: 'false != x', expected: 'x'}")
260+
@TestParameters("{source: 'bool_var != true', expected: '!bool_var'}")
261+
@TestParameters("{source: 'true != bool_var', expected: '!bool_var'}")
262+
@TestParameters("{source: 'bool_var != false', expected: 'bool_var'}")
263+
@TestParameters("{source: 'false != bool_var', expected: 'bool_var'}")
264264
@TestParameters("{source: 'true != false', expected: 'true'}")
265265
@TestParameters("{source: 'true != true', expected: 'false'}")
266266
@TestParameters("{source: 'false != true', expected: 'true'}")
@@ -395,6 +395,7 @@ public void constantFold_protoMessageLiteral_success(String source, String expec
395395
@TestParameters(
396396
"{source: 'cel.bind(myMap, {\"foo\": \"bar\"}, myMap[?\"foo\"].optMap(x, x + \"baz\"))', "
397397
+ "expected: 'optional.of(\"barbaz\")'}")
398+
@TestParameters("{source: '(1 + 2 + 3 == x) && (x in [1, 2, x])', expected: '6 == x'}")
398399
public void constantFold_macros_macroCallMetadataPopulated(String source, String expected)
399400
throws Exception {
400401
Cel cel =
@@ -498,6 +499,22 @@ public void constantFold_macros_withoutMacroCallMetadata(String source) throws E
498499
@TestParameters("{source: '[true].exists(x, x == get_true())'}")
499500
@TestParameters("{source: 'get_list([1, 2]).map(x, x * 2)'}")
500501
@TestParameters("{source: '[(x - 1 > 3) ? (x - 1) : 5].exists(x, x - 1 > 3)'}")
502+
@TestParameters("{source: 'true && x'}")
503+
@TestParameters("{source: 'x && true'}")
504+
@TestParameters("{source: 'false || x'}")
505+
@TestParameters("{source: 'x || false'}")
506+
@TestParameters("{source: 'true && x && true && x'}")
507+
@TestParameters("{source: 'false || x || false || x'}")
508+
@TestParameters("{source: 'false || x || false || y'}")
509+
@TestParameters("{source: 'x == true'}")
510+
@TestParameters("{source: 'true == x'}")
511+
@TestParameters("{source: 'x == false'}")
512+
@TestParameters("{source: 'false == x'}")
513+
@TestParameters("{source: 'x != true'}")
514+
@TestParameters("{source: 'true != x'}")
515+
@TestParameters("{source: 'x != false'}")
516+
@TestParameters("{source: 'false != x'}")
517+
@TestParameters("{source: '[x].exists(item, item == true)'}")
501518
public void constantFold_noOp(String source) throws Exception {
502519
CelAbstractSyntaxTree ast = cel.compile(source).getAst();
503520

0 commit comments

Comments
 (0)