Skip to content

Commit 6000209

Browse files
l46kokcopybara-github
authored andcommitted
Fix ConstantFoldingOptimizer to not treat true && dyn_x as a tautology
PiperOrigin-RevId: 952324181
1 parent 250bff3 commit 6000209

2 files changed

Lines changed: 44 additions & 8 deletions

File tree

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

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -583,14 +583,45 @@ private Optional<CelMutableAst> maybeShortCircuitCall(
583583
return Optional.of(astMutator.replaceSubtree(mutableAst, shortCircuitTarget, expr.id()));
584584
}
585585
if (newArgs.size() == 1) {
586-
return Optional.of(astMutator.replaceSubtree(mutableAst, newArgs.get(0), expr.id()));
586+
CelMutableExpr remainingArg = newArgs.get(0);
587+
if (isBoolean(mutableAst, remainingArg)) {
588+
return Optional.of(astMutator.replaceSubtree(mutableAst, remainingArg, expr.id()));
589+
}
590+
return Optional.empty();
587591
}
588592

589593
// TODO: Support folding variadic AND/ORs.
590594
throw new UnsupportedOperationException(
591595
"Folding variadic logical operator is not supported yet.");
592596
}
593597

598+
private boolean isBoolean(CelMutableAst mutableAst, CelMutableExpr expr) {
599+
if (expr.getKind().equals(Kind.CONSTANT)
600+
&& expr.constant().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)) {
601+
return true;
602+
}
603+
// The AST's type map relies on the type-checker having explicitly populated the type for a
604+
// given node. However, during the optimization pipeline, mutated intermediate nodes might
605+
// temporarily lack type metadata. Standard CEL operators like &&, ||, and == inherently
606+
// always return a boolean, so checking the function name provides a reliable fallback when
607+
// the type map is incomplete.
608+
if (expr.getKind().equals(Kind.CALL)) {
609+
String functionName = expr.call().function();
610+
if (functionName.equals(Operator.LOGICAL_AND.getFunction())
611+
|| functionName.equals(Operator.LOGICAL_OR.getFunction())
612+
|| functionName.equals(Operator.LOGICAL_NOT.getFunction())
613+
|| functionName.equals(Operator.EQUALS.getFunction())
614+
|| functionName.equals(Operator.NOT_EQUALS.getFunction())
615+
|| functionName.equals(Operator.LESS.getFunction())
616+
|| functionName.equals(Operator.LESS_EQUALS.getFunction())
617+
|| functionName.equals(Operator.GREATER.getFunction())
618+
|| functionName.equals(Operator.GREATER_EQUALS.getFunction())) {
619+
return true;
620+
}
621+
}
622+
return mutableAst.getType(expr.id()).map(SimpleType.BOOL::equals).orElse(false);
623+
}
624+
594625
private boolean isFoldedAggregateLiteral(CelMutableExpr expr) {
595626
if (expr.getKind().equals(Kind.CONSTANT)) {
596627
return true;

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

Lines changed: 12 additions & 7 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,14 @@ 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 && true', expected: 'bool_var'}")
132133
@TestParameters("{source: 'false && x', expected: 'false'}")
133134
@TestParameters("{source: 'x && false', expected: 'false'}")
134135
@TestParameters("{source: 'true || x', expected: 'true'}")
135136
@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'}")
137+
@TestParameters("{source: 'false || bool_var', expected: 'bool_var'}")
138+
@TestParameters("{source: 'bool_var || false', expected: 'bool_var'}")
141139
@TestParameters("{source: 'true ? x + 1 : x + 2', expected: 'x + 1'}")
142140
@TestParameters("{source: 'false ? x + 1 : x + 2', expected: 'x + 2'}")
143141
@TestParameters(
@@ -498,6 +496,13 @@ public void constantFold_macros_withoutMacroCallMetadata(String source) throws E
498496
@TestParameters("{source: '[true].exists(x, x == get_true())'}")
499497
@TestParameters("{source: 'get_list([1, 2]).map(x, x * 2)'}")
500498
@TestParameters("{source: '[(x - 1 > 3) ? (x - 1) : 5].exists(x, x - 1 > 3)'}")
499+
@TestParameters("{source: 'true && x'}")
500+
@TestParameters("{source: 'x && true'}")
501+
@TestParameters("{source: 'false || x'}")
502+
@TestParameters("{source: 'x || false'}")
503+
@TestParameters("{source: 'true && x && true && x'}")
504+
@TestParameters("{source: 'false || x || false || x'}")
505+
@TestParameters("{source: 'false || x || false || y'}")
501506
public void constantFold_noOp(String source) throws Exception {
502507
CelAbstractSyntaxTree ast = cel.compile(source).getAst();
503508

0 commit comments

Comments
 (0)