Skip to content

Commit

Permalink
Fix an issue with incorrect boolean evaluation when short-circuiting …
Browse files Browse the repository at this point in the history 
…is disabled

PiperOrigin-RevId: 617539191
  • Loading branch information
@l46kok @copybara-github
l46kok authored and copybara-github committed Mar 20, 2024
1 parent 6d6ecd4 commit 9719d52
Show file tree
Hide file tree
Showing 2 changed files with 197 additions and 35 deletions.
72 changes: 50 additions & 22 deletions runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -464,12 +464,14 @@ private IntermediateResult evalConditional(ExecutionFrame frame, CelCall callExp
}
return evalInternal(frame, callExpr.args().get(2));
} else {
IntermediateResult lhs = evalInternal(frame, callExpr.args().get(1));
IntermediateResult rhs = evalInternal(frame, callExpr.args().get(2));
IntermediateResult lhs = evalNonstrictly(frame, callExpr.args().get(1));
IntermediateResult rhs = evalNonstrictly(frame, callExpr.args().get(2));
if (isUnknownValue(condition.value())) {
return condition;
}
return (boolean) condition.value() ? lhs : rhs;
Object result =
InterpreterUtil.strict((boolean) condition.value() ? lhs.value() : rhs.value());
return IntermediateResult.create(result);
}
}

Expand Down Expand Up @@ -497,7 +499,7 @@ private enum ShortCircuitableOperators {
}

private boolean canShortCircuit(IntermediateResult result, ShortCircuitableOperators operator) {
if (!celOptions.enableShortCircuiting() || !(result.value() instanceof Boolean)) {
if (!(result.value() instanceof Boolean)) {
return false;
}

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

private IntermediateResult evalLogicalOr(ExecutionFrame frame, CelCall callExpr)
throws InterpreterException {
IntermediateResult left = evalBooleanNonstrict(frame, callExpr.args().get(0));
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_OR)) {
return left;
}
IntermediateResult left;
IntermediateResult right;
if (celOptions.enableShortCircuiting()) {
left = evalBooleanNonstrict(frame, callExpr.args().get(0));
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_OR)) {
return left;
}

IntermediateResult right = evalBooleanNonstrict(frame, callExpr.args().get(1));
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_OR)) {
return right;
right = evalBooleanNonstrict(frame, callExpr.args().get(1));
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_OR)) {
return right;
}
} else {
left = evalBooleanNonstrict(frame, callExpr.args().get(0));
right = evalBooleanNonstrict(frame, callExpr.args().get(1));
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_OR)) {
return left;
}
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_OR)) {
return right;
}
}

// both false.
// both are booleans.
if (right.value() instanceof Boolean && left.value() instanceof Boolean) {
return left;
return IntermediateResult.create((Boolean) right.value() || (Boolean) left.value());
}

return mergeBooleanUnknowns(left, right);
}

private IntermediateResult evalLogicalAnd(ExecutionFrame frame, CelCall callExpr)
throws InterpreterException {
IntermediateResult left = evalBooleanNonstrict(frame, callExpr.args().get(0));
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_AND)) {
return left;
}
IntermediateResult left;
IntermediateResult right;
if (celOptions.enableShortCircuiting()) {
left = evalBooleanNonstrict(frame, callExpr.args().get(0));
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_AND)) {
return left;
}

IntermediateResult right = evalBooleanNonstrict(frame, callExpr.args().get(1));
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_AND)) {
return right;
right = evalBooleanNonstrict(frame, callExpr.args().get(1));
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_AND)) {
return right;
}
} else {
left = evalBooleanNonstrict(frame, callExpr.args().get(0));
right = evalBooleanNonstrict(frame, callExpr.args().get(1));
if (canShortCircuit(left, ShortCircuitableOperators.LOGICAL_AND)) {
return left;
}
if (canShortCircuit(right, ShortCircuitableOperators.LOGICAL_AND)) {
return right;
}
}

// both true.
// both are booleans.
if (right.value() instanceof Boolean && left.value() instanceof Boolean) {
return left;
return IntermediateResult.create((Boolean) right.value() && (Boolean) left.value());
}

return mergeBooleanUnknowns(left, right);
Expand Down
160 changes: 147 additions & 13 deletions runtime/src/test/java/dev/cel/runtime/CelRuntimeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.protobuf.DescriptorProtos.FileDescriptorSet;
import com.google.protobuf.DynamicMessage;
import com.google.rpc.context.AttributeContext;
import com.google.testing.junit.testparameterinjector.TestParameter;
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import com.google.testing.junit.testparameterinjector.TestParameters;
import dev.cel.bundle.Cel;
Expand Down Expand Up @@ -403,28 +404,75 @@ public void trace_withVariableResolver() throws Exception {
}

@Test
public void trace_shortCircuitingDisabled_logicalAndAllBranchesVisited() throws Exception {
public void trace_shortCircuitingDisabled_logicalAndAllBranchesVisited(
@TestParameter boolean first, @TestParameter boolean second, @TestParameter boolean third)
throws Exception {
String expression = String.format("%s && %s && %s", first, second, third);
ImmutableList.Builder<Boolean> branchResults = ImmutableList.builder();
CelEvaluationListener listener =
(expr, res) -> {
if (expr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)) {
branchResults.add((Boolean) res);
}
};
Cel celWithShortCircuit =
CelFactory.standardCelBuilder()
.setOptions(CelOptions.current().enableShortCircuiting(true).build())
.build();
Cel cel =
CelFactory.standardCelBuilder()
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
.build();
CelAbstractSyntaxTree ast = cel.compile(expression).getAst();

boolean result = (boolean) cel.createProgram(ast).trace(listener);
boolean shortCircuitedResult =
(boolean)
celWithShortCircuit
.createProgram(celWithShortCircuit.compile(expression).getAst())
.eval();

assertThat(result).isEqualTo(shortCircuitedResult);
assertThat(branchResults.build()).containsExactly(first, second, third).inOrder();
}

@Test
@TestParameters("{source: 'false && false && x'}")
@TestParameters("{source: 'false && x && false'}")
@TestParameters("{source: 'x && false && false'}")
public void trace_shortCircuitingDisabledWithUnknownsAndedToFalse_returnsFalse(String source)
throws Exception {
ImmutableList.Builder<Object> branchResults = ImmutableList.builder();
CelEvaluationListener listener =
(expr, res) -> {
if (expr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)
|| expr.identOrDefault().name().equals("x")) {
if (InterpreterUtil.isUnknown(res)) {
branchResults.add("x"); // Swap unknown result with a sentinel value for testing
} else {
branchResults.add(res);
}
}
};
Cel cel =
CelFactory.standardCelBuilder()
.addVar("x", SimpleType.BOOL)
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
.build();
CelAbstractSyntaxTree ast = cel.compile("false && true && false").getAst();
CelAbstractSyntaxTree ast = cel.compile(source).getAst();

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

assertThat(result).isFalse();
assertThat(branchResults.build()).containsExactly(false, true, false);
assertThat(branchResults.build()).containsExactly(false, false, "x");
}

@Test
public void trace_shortCircuitingDisabled_logicalAndWithUnknowns() throws Exception {
@TestParameters("{source: 'true && true && x'}")
@TestParameters("{source: 'true && x && true'}")
@TestParameters("{source: 'x && true && true'}")
public void trace_shortCircuitingDisabledWithUnknownAndedToTrue_returnsUnknown(String source)
throws Exception {
ImmutableList.Builder<Object> branchResults = ImmutableList.builder();
CelEvaluationListener listener =
(expr, res) -> {
Expand All @@ -438,37 +486,53 @@ public void trace_shortCircuitingDisabled_logicalAndWithUnknowns() throws Except
.addVar("x", SimpleType.BOOL)
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
.build();
CelAbstractSyntaxTree ast = cel.compile("false && false && x").getAst();
CelAbstractSyntaxTree ast = cel.compile(source).getAst();

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

assertThat(InterpreterUtil.isUnknown(unknownResult)).isTrue();
assertThat(branchResults.build()).containsExactly(false, false, unknownResult);
assertThat(branchResults.build()).containsExactly(true, true, unknownResult);
}

@Test
public void trace_shortCircuitingDisabled_logicalOrAllBranchesVisited() throws Exception {
public void trace_shortCircuitingDisabled_logicalOrAllBranchesVisited(
@TestParameter boolean first, @TestParameter boolean second, @TestParameter boolean third)
throws Exception {
String expression = String.format("%s || %s || %s", first, second, third);
ImmutableList.Builder<Boolean> branchResults = ImmutableList.builder();
CelEvaluationListener listener =
(expr, res) -> {
if (expr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)) {
branchResults.add((Boolean) res);
}
};
Cel celWithShortCircuit =
CelFactory.standardCelBuilder()
.setOptions(CelOptions.current().enableShortCircuiting(true).build())
.build();
Cel cel =
CelFactory.standardCelBuilder()
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
.build();
CelAbstractSyntaxTree ast = cel.compile("true || false || true").getAst();
CelAbstractSyntaxTree ast = cel.compile(expression).getAst();

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

assertThat(result).isTrue();
assertThat(branchResults.build()).containsExactly(true, false, true);
boolean shortCircuitedResult =
(boolean)
celWithShortCircuit
.createProgram(celWithShortCircuit.compile(expression).getAst())
.eval();

assertThat(result).isEqualTo(shortCircuitedResult);
assertThat(branchResults.build()).containsExactly(first, second, third).inOrder();
}

@Test
public void trace_shortCircuitingDisabled_logicalOrWithUnknowns() throws Exception {
@TestParameters("{source: 'false || false || x'}")
@TestParameters("{source: 'false || x || false'}")
@TestParameters("{source: 'x || false || false'}")
public void trace_shortCircuitingDisabledWithUnknownsOredToFalse_returnsUnknown(String source)
throws Exception {
ImmutableList.Builder<Object> branchResults = ImmutableList.builder();
CelEvaluationListener listener =
(expr, res) -> {
Expand All @@ -482,14 +546,45 @@ public void trace_shortCircuitingDisabled_logicalOrWithUnknowns() throws Excepti
.addVar("x", SimpleType.BOOL)
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
.build();
CelAbstractSyntaxTree ast = cel.compile("false || false || x").getAst();
CelAbstractSyntaxTree ast = cel.compile(source).getAst();

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

assertThat(InterpreterUtil.isUnknown(unknownResult)).isTrue();
assertThat(branchResults.build()).containsExactly(false, false, unknownResult);
}

@Test
@TestParameters("{source: 'true || true || x'}")
@TestParameters("{source: 'true || x || true'}")
@TestParameters("{source: 'x || true || true'}")
public void trace_shortCircuitingDisabledWithUnknownOredToTrue_returnsTrue(String source)
throws Exception {
ImmutableList.Builder<Object> branchResults = ImmutableList.builder();
CelEvaluationListener listener =
(expr, res) -> {
if (expr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)
|| expr.identOrDefault().name().equals("x")) {
if (InterpreterUtil.isUnknown(res)) {
branchResults.add("x"); // Swap unknown result with a sentinel value for testing
} else {
branchResults.add(res);
}
}
};
Cel cel =
CelFactory.standardCelBuilder()
.addVar("x", SimpleType.BOOL)
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
.build();
CelAbstractSyntaxTree ast = cel.compile(source).getAst();

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

assertThat(result).isTrue();
assertThat(branchResults.build()).containsExactly(true, true, "x");
}

@Test
public void trace_shortCircuitingDisabled_ternaryAllBranchesVisited() throws Exception {
ImmutableList.Builder<Boolean> branchResults = ImmutableList.builder();
Expand Down Expand Up @@ -537,6 +632,45 @@ public void trace_shortCircuitingDisabled_ternaryWithUnknowns(String source) thr
assertThat(branchResults.build()).containsExactly(false, unknownResult, true);
}

@Test
@TestParameters(
"{expression: 'false ? (1 / 0) > 2 : false', firstVisited: false, secondVisited: false}")
@TestParameters(
"{expression: 'false ? (1 / 0) > 2 : true', firstVisited: false, secondVisited: true}")
@TestParameters(
"{expression: 'true ? false : (1 / 0) > 2', firstVisited: true, secondVisited: false}")
@TestParameters(
"{expression: 'true ? true : (1 / 0) > 2', firstVisited: true, secondVisited: true}")
public void trace_shortCircuitingDisabled_ternaryWithError(
String expression, boolean firstVisited, boolean secondVisited) throws Exception {
ImmutableList.Builder<Object> branchResults = ImmutableList.builder();
CelEvaluationListener listener =
(expr, res) -> {
if (expr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)) {
branchResults.add(res);
}
};
Cel celWithShortCircuit =
CelFactory.standardCelBuilder()
.setOptions(CelOptions.current().enableShortCircuiting(true).build())
.build();
Cel cel =
CelFactory.standardCelBuilder()
.setOptions(CelOptions.current().enableShortCircuiting(false).build())
.build();
CelAbstractSyntaxTree ast = cel.compile(expression).getAst();

boolean result = (boolean) cel.createProgram(ast).trace(listener);
boolean shortCircuitedResult =
(boolean)
celWithShortCircuit
.createProgram(celWithShortCircuit.compile(expression).getAst())
.eval();

assertThat(result).isEqualTo(shortCircuitedResult);
assertThat(branchResults.build()).containsExactly(firstVisited, secondVisited).inOrder();
}

@Test
public void standardEnvironmentDisabledForRuntime_throws() throws Exception {
CelCompiler celCompiler =
Expand Down

0 comments on commit 9719d52

Please sign in to comment.