Skip to content

Commit 9719d52

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Fix an issue with incorrect boolean evaluation when short-circuiting is disabled
PiperOrigin-RevId: 617539191
1 parent 6d6ecd4 commit 9719d52

2 files changed

Lines changed: 197 additions & 35 deletions

File tree

‎runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java‎

Lines changed: 50 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -464,12 +464,14 @@ private IntermediateResult evalConditional(ExecutionFrame frame, CelCall callExp
464464
}
465465
return evalInternal(frame, callExpr.args().get(2));
466466
} else {
467-
IntermediateResult lhs = evalInternal(frame, callExpr.args().get(1));
468-
IntermediateResult rhs = evalInternal(frame, callExpr.args().get(2));
467+
IntermediateResult lhs = evalNonstrictly(frame, callExpr.args().get(1));
468+
IntermediateResult rhs = evalNonstrictly(frame, callExpr.args().get(2));
469469
if (isUnknownValue(condition.value())) {
470470
return condition;
471471
}
472-
return (boolean) condition.value() ? lhs : rhs;
472+
Object result =
473+
InterpreterUtil.strict((boolean) condition.value() ? lhs.value() : rhs.value());
474+
return IntermediateResult.create(result);
473475
}
474476
}
475477

@@ -497,7 +499,7 @@ private enum ShortCircuitableOperators {
497499
}
498500

499501
private boolean canShortCircuit(IntermediateResult result, ShortCircuitableOperators operator) {
500-
if (!celOptions.enableShortCircuiting() || !(result.value() instanceof Boolean)) {
502+
if (!(result.value() instanceof Boolean)) {
501503
return false;
502504
}
503505

@@ -511,39 +513,65 @@ private boolean canShortCircuit(IntermediateResult result, ShortCircuitableOpera
511513

512514
private IntermediateResult evalLogicalOr(ExecutionFrame frame, CelCall callExpr)
513515
throws InterpreterException {
514-
IntermediateResult left = evalBooleanNonstrict(frame, callExpr.args().get(0));
515-
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_OR)) {
516-
return left;
517-
}
516+
IntermediateResult left;
517+
IntermediateResult right;
518+
if (celOptions.enableShortCircuiting()) {
519+
left = evalBooleanNonstrict(frame, callExpr.args().get(0));
520+
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_OR)) {
521+
return left;
522+
}
518523

519-
IntermediateResult right = evalBooleanNonstrict(frame, callExpr.args().get(1));
520-
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_OR)) {
521-
return right;
524+
right = evalBooleanNonstrict(frame, callExpr.args().get(1));
525+
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_OR)) {
526+
return right;
527+
}
528+
} else {
529+
left = evalBooleanNonstrict(frame, callExpr.args().get(0));
530+
right = evalBooleanNonstrict(frame, callExpr.args().get(1));
531+
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_OR)) {
532+
return left;
533+
}
534+
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_OR)) {
535+
return right;
536+
}
522537
}
523538

524-
// both false.
539+
// both are booleans.
525540
if (right.value() instanceof Boolean && left.value() instanceof Boolean) {
526-
return left;
541+
return IntermediateResult.create((Boolean) right.value() || (Boolean) left.value());
527542
}
528543

529544
return mergeBooleanUnknowns(left, right);
530545
}
531546

532547
private IntermediateResult evalLogicalAnd(ExecutionFrame frame, CelCall callExpr)
533548
throws InterpreterException {
534-
IntermediateResult left = evalBooleanNonstrict(frame, callExpr.args().get(0));
535-
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_AND)) {
536-
return left;
537-
}
549+
IntermediateResult left;
550+
IntermediateResult right;
551+
if (celOptions.enableShortCircuiting()) {
552+
left = evalBooleanNonstrict(frame, callExpr.args().get(0));
553+
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_AND)) {
554+
return left;
555+
}
538556

539-
IntermediateResult right = evalBooleanNonstrict(frame, callExpr.args().get(1));
540-
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_AND)) {
541-
return right;
557+
right = evalBooleanNonstrict(frame, callExpr.args().get(1));
558+
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_AND)) {
559+
return right;
560+
}
561+
} else {
562+
left = evalBooleanNonstrict(frame, callExpr.args().get(0));
563+
right = evalBooleanNonstrict(frame, callExpr.args().get(1));
564+
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_AND)) {
565+
return left;
566+
}
567+
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_AND)) {
568+
return right;
569+
}
542570
}
543571

544-
// both true.
572+
// both are booleans.
545573
if (right.value() instanceof Boolean && left.value() instanceof Boolean) {
546-
return left;
574+
return IntermediateResult.create((Boolean) right.value() && (Boolean) left.value());
547575
}
548576

549577
return mergeBooleanUnknowns(left, right);

‎runtime/src/test/java/dev/cel/runtime/CelRuntimeTest.java‎

Lines changed: 147 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import com.google.protobuf.DescriptorProtos.FileDescriptorSet;
2929
import com.google.protobuf.DynamicMessage;
3030
import com.google.rpc.context.AttributeContext;
31+
import com.google.testing.junit.testparameterinjector.TestParameter;
3132
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
3233
import com.google.testing.junit.testparameterinjector.TestParameters;
3334
import dev.cel.bundle.Cel;
@@ -403,28 +404,75 @@ public void trace_withVariableResolver() throws Exception {
403404
}
404405

405406
@Test
406-
public void trace_shortCircuitingDisabled_logicalAndAllBranchesVisited() throws Exception {
407+
public void trace_shortCircuitingDisabled_logicalAndAllBranchesVisited(
408+
@TestParameter boolean first, @TestParameter boolean second, @TestParameter boolean third)
409+
throws Exception {
410+
String expression = String.format("%s && %s && %s", first, second, third);
407411
ImmutableList.Builder<Boolean> branchResults = ImmutableList.builder();
408412
CelEvaluationListener listener =
409413
(expr, res) -> {
410414
if (expr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)) {
411415
branchResults.add((Boolean) res);
412416
}
413417
};
418+
Cel celWithShortCircuit =
419+
CelFactory.standardCelBuilder()
420+
.setOptions(CelOptions.current().enableShortCircuiting(true).build())
421+
.build();
422+
Cel cel =
423+
CelFactory.standardCelBuilder()
424+
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
425+
.build();
426+
CelAbstractSyntaxTree ast = cel.compile(expression).getAst();
427+
428+
boolean result = (boolean) cel.createProgram(ast).trace(listener);
429+
boolean shortCircuitedResult =
430+
(boolean)
431+
celWithShortCircuit
432+
.createProgram(celWithShortCircuit.compile(expression).getAst())
433+
.eval();
434+
435+
assertThat(result).isEqualTo(shortCircuitedResult);
436+
assertThat(branchResults.build()).containsExactly(first, second, third).inOrder();
437+
}
438+
439+
@Test
440+
@TestParameters("{source: 'false && false && x'}")
441+
@TestParameters("{source: 'false && x && false'}")
442+
@TestParameters("{source: 'x && false && false'}")
443+
public void trace_shortCircuitingDisabledWithUnknownsAndedToFalse_returnsFalse(String source)
444+
throws Exception {
445+
ImmutableList.Builder<Object> branchResults = ImmutableList.builder();
446+
CelEvaluationListener listener =
447+
(expr, res) -> {
448+
if (expr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)
449+
|| expr.identOrDefault().name().equals("x")) {
450+
if (InterpreterUtil.isUnknown(res)) {
451+
branchResults.add("x"); // Swap unknown result with a sentinel value for testing
452+
} else {
453+
branchResults.add(res);
454+
}
455+
}
456+
};
414457
Cel cel =
415458
CelFactory.standardCelBuilder()
459+
.addVar("x", SimpleType.BOOL)
416460
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
417461
.build();
418-
CelAbstractSyntaxTree ast = cel.compile("false && true && false").getAst();
462+
CelAbstractSyntaxTree ast = cel.compile(source).getAst();
419463

420464
boolean result = (boolean) cel.createProgram(ast).trace(listener);
421465

422466
assertThat(result).isFalse();
423-
assertThat(branchResults.build()).containsExactly(false, true, false);
467+
assertThat(branchResults.build()).containsExactly(false, false, "x");
424468
}
425469

426470
@Test
427-
public void trace_shortCircuitingDisabled_logicalAndWithUnknowns() throws Exception {
471+
@TestParameters("{source: 'true && true && x'}")
472+
@TestParameters("{source: 'true && x && true'}")
473+
@TestParameters("{source: 'x && true && true'}")
474+
public void trace_shortCircuitingDisabledWithUnknownAndedToTrue_returnsUnknown(String source)
475+
throws Exception {
428476
ImmutableList.Builder<Object> branchResults = ImmutableList.builder();
429477
CelEvaluationListener listener =
430478
(expr, res) -> {
@@ -438,37 +486,53 @@ public void trace_shortCircuitingDisabled_logicalAndWithUnknowns() throws Except
438486
.addVar("x", SimpleType.BOOL)
439487
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
440488
.build();
441-
CelAbstractSyntaxTree ast = cel.compile("false && false && x").getAst();
489+
CelAbstractSyntaxTree ast = cel.compile(source).getAst();
442490

443491
Object unknownResult = cel.createProgram(ast).trace(listener);
444492

445493
assertThat(InterpreterUtil.isUnknown(unknownResult)).isTrue();
446-
assertThat(branchResults.build()).containsExactly(false, false, unknownResult);
494+
assertThat(branchResults.build()).containsExactly(true, true, unknownResult);
447495
}
448496

449497
@Test
450-
public void trace_shortCircuitingDisabled_logicalOrAllBranchesVisited() throws Exception {
498+
public void trace_shortCircuitingDisabled_logicalOrAllBranchesVisited(
499+
@TestParameter boolean first, @TestParameter boolean second, @TestParameter boolean third)
500+
throws Exception {
501+
String expression = String.format("%s || %s || %s", first, second, third);
451502
ImmutableList.Builder<Boolean> branchResults = ImmutableList.builder();
452503
CelEvaluationListener listener =
453504
(expr, res) -> {
454505
if (expr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)) {
455506
branchResults.add((Boolean) res);
456507
}
457508
};
509+
Cel celWithShortCircuit =
510+
CelFactory.standardCelBuilder()
511+
.setOptions(CelOptions.current().enableShortCircuiting(true).build())
512+
.build();
458513
Cel cel =
459514
CelFactory.standardCelBuilder()
460515
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
461516
.build();
462-
CelAbstractSyntaxTree ast = cel.compile("true || false || true").getAst();
517+
CelAbstractSyntaxTree ast = cel.compile(expression).getAst();
463518

464519
boolean result = (boolean) cel.createProgram(ast).trace(listener);
465-
466-
assertThat(result).isTrue();
467-
assertThat(branchResults.build()).containsExactly(true, false, true);
520+
boolean shortCircuitedResult =
521+
(boolean)
522+
celWithShortCircuit
523+
.createProgram(celWithShortCircuit.compile(expression).getAst())
524+
.eval();
525+
526+
assertThat(result).isEqualTo(shortCircuitedResult);
527+
assertThat(branchResults.build()).containsExactly(first, second, third).inOrder();
468528
}
469529

470530
@Test
471-
public void trace_shortCircuitingDisabled_logicalOrWithUnknowns() throws Exception {
531+
@TestParameters("{source: 'false || false || x'}")
532+
@TestParameters("{source: 'false || x || false'}")
533+
@TestParameters("{source: 'x || false || false'}")
534+
public void trace_shortCircuitingDisabledWithUnknownsOredToFalse_returnsUnknown(String source)
535+
throws Exception {
472536
ImmutableList.Builder<Object> branchResults = ImmutableList.builder();
473537
CelEvaluationListener listener =
474538
(expr, res) -> {
@@ -482,14 +546,45 @@ public void trace_shortCircuitingDisabled_logicalOrWithUnknowns() throws Excepti
482546
.addVar("x", SimpleType.BOOL)
483547
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
484548
.build();
485-
CelAbstractSyntaxTree ast = cel.compile("false || false || x").getAst();
549+
CelAbstractSyntaxTree ast = cel.compile(source).getAst();
486550

487551
Object unknownResult = cel.createProgram(ast).trace(listener);
488552

489553
assertThat(InterpreterUtil.isUnknown(unknownResult)).isTrue();
490554
assertThat(branchResults.build()).containsExactly(false, false, unknownResult);
491555
}
492556

557+
@Test
558+
@TestParameters("{source: 'true || true || x'}")
559+
@TestParameters("{source: 'true || x || true'}")
560+
@TestParameters("{source: 'x || true || true'}")
561+
public void trace_shortCircuitingDisabledWithUnknownOredToTrue_returnsTrue(String source)
562+
throws Exception {
563+
ImmutableList.Builder<Object> branchResults = ImmutableList.builder();
564+
CelEvaluationListener listener =
565+
(expr, res) -> {
566+
if (expr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)
567+
|| expr.identOrDefault().name().equals("x")) {
568+
if (InterpreterUtil.isUnknown(res)) {
569+
branchResults.add("x"); // Swap unknown result with a sentinel value for testing
570+
} else {
571+
branchResults.add(res);
572+
}
573+
}
574+
};
575+
Cel cel =
576+
CelFactory.standardCelBuilder()
577+
.addVar("x", SimpleType.BOOL)
578+
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
579+
.build();
580+
CelAbstractSyntaxTree ast = cel.compile(source).getAst();
581+
582+
boolean result = (boolean) cel.createProgram(ast).trace(listener);
583+
584+
assertThat(result).isTrue();
585+
assertThat(branchResults.build()).containsExactly(true, true, "x");
586+
}
587+
493588
@Test
494589
public void trace_shortCircuitingDisabled_ternaryAllBranchesVisited() throws Exception {
495590
ImmutableList.Builder<Boolean> branchResults = ImmutableList.builder();
@@ -537,6 +632,45 @@ public void trace_shortCircuitingDisabled_ternaryWithUnknowns(String source) thr
537632
assertThat(branchResults.build()).containsExactly(false, unknownResult, true);
538633
}
539634

635+
@Test
636+
@TestParameters(
637+
"{expression: 'false ? (1 / 0) > 2 : false', firstVisited: false, secondVisited: false}")
638+
@TestParameters(
639+
"{expression: 'false ? (1 / 0) > 2 : true', firstVisited: false, secondVisited: true}")
640+
@TestParameters(
641+
"{expression: 'true ? false : (1 / 0) > 2', firstVisited: true, secondVisited: false}")
642+
@TestParameters(
643+
"{expression: 'true ? true : (1 / 0) > 2', firstVisited: true, secondVisited: true}")
644+
public void trace_shortCircuitingDisabled_ternaryWithError(
645+
String expression, boolean firstVisited, boolean secondVisited) throws Exception {
646+
ImmutableList.Builder<Object> branchResults = ImmutableList.builder();
647+
CelEvaluationListener listener =
648+
(expr, res) -> {
649+
if (expr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)) {
650+
branchResults.add(res);
651+
}
652+
};
653+
Cel celWithShortCircuit =
654+
CelFactory.standardCelBuilder()
655+
.setOptions(CelOptions.current().enableShortCircuiting(true).build())
656+
.build();
657+
Cel cel =
658+
CelFactory.standardCelBuilder()
659+
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
660+
.build();
661+
CelAbstractSyntaxTree ast = cel.compile(expression).getAst();
662+
663+
boolean result = (boolean) cel.createProgram(ast).trace(listener);
664+
boolean shortCircuitedResult =
665+
(boolean)
666+
celWithShortCircuit
667+
.createProgram(celWithShortCircuit.compile(expression).getAst())
668+
.eval();
669+
670+
assertThat(result).isEqualTo(shortCircuitedResult);
671+
assertThat(branchResults.build()).containsExactly(firstVisited, secondVisited).inOrder();
672+
}
673+
540674
@Test
541675
public void standardEnvironmentDisabledForRuntime_throws() throws Exception {
542676
CelCompiler celCompiler =

0 commit comments

Comments
 (0)