Skip to content

Fix error propagation and non-strict/exhaustive evaluation in planner #1248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
copybara-service wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix error propagation and non-strict/exhaustive evaluation in planner #1248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions common/src/main/java/dev/cel/common/values/ErrorValue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public abstract class ErrorValue extends CelValue {
public abstract long exprId();

@Override
public abstract Exception value();
public abstract RuntimeException value();

@Override
public boolean isZeroValue() {
Expand All @@ -48,7 +48,9 @@ public CelType celType() {
return SimpleType.ERROR;
}

public static ErrorValue create(long exprId, Exception value) {
public static ErrorValue create(long exprId, RuntimeException value) {
return new AutoValue_ErrorValue(exprId, value);
}

ErrorValue() {}
}
12 changes: 10 additions & 2 deletions runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ java_library(
name = "eval_conditional",
srcs = ["EvalConditional.java"],
deps = [
":eval_helpers",
":planned_interpretable",
"//common/ast",
"//runtime:accumulated_unknowns",
Expand Down Expand Up @@ -373,6 +374,7 @@ java_library(
"//runtime:accumulated_unknowns",
"//runtime:interpretable",
"@maven//:com_google_errorprone_error_prone_annotations",
"@maven//:com_google_guava_guava",
],
)

Expand All @@ -383,10 +385,11 @@ java_library(
":eval_helpers",
":planned_interpretable",
"//common/ast",
"//common/values",
"//runtime:accumulated_unknowns",
"//runtime:evaluation_exception",
"//runtime:interpretable",
"@maven//:com_google_errorprone_error_prone_annotations",
"@maven//:com_google_guava_guava",
],
)

Expand All @@ -401,6 +404,7 @@ java_library(
"//runtime:accumulated_unknowns",
"//runtime:interpretable",
"@maven//:com_google_errorprone_error_prone_annotations",
"@maven//:com_google_guava_guava",
],
)

Expand Down Expand Up @@ -942,6 +946,7 @@ cel_android_library(
name = "eval_conditional_android",
srcs = ["EvalConditional.java"],
deps = [
":eval_helpers_android",
":planned_interpretable_android",
"//common/ast:ast_android",
"//runtime:evaluation_exception",
Expand Down Expand Up @@ -979,6 +984,7 @@ cel_android_library(
"//runtime:interpretable_android",
"//runtime/src/main/java/dev/cel/runtime:accumulated_unknowns_android",
"@maven//:com_google_errorprone_error_prone_annotations",
"@maven_android//:com_google_guava_guava",
],
)

Expand All @@ -989,10 +995,11 @@ cel_android_library(
":eval_helpers_android",
":planned_interpretable_android",
"//common/ast:ast_android",
"//runtime:evaluation_exception",
"//common/values:values_android",
"//runtime:interpretable_android",
"//runtime/src/main/java/dev/cel/runtime:accumulated_unknowns_android",
"@maven//:com_google_errorprone_error_prone_annotations",
"@maven_android//:com_google_guava_guava",
],
)

Expand All @@ -1007,6 +1014,7 @@ cel_android_library(
"//runtime:interpretable_android",
"//runtime/src/main/java/dev/cel/runtime:accumulated_unknowns_android",
"@maven//:com_google_errorprone_error_prone_annotations",
"@maven_android//:com_google_guava_guava",
],
)

Expand Down
10 changes: 2 additions & 8 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalAnd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

package dev.cel.runtime.planner;

import static dev.cel.runtime.planner.EvalHelpers.evalNonstrictly;
import static dev.cel.runtime.planner.EvalHelpers.evalBooleanNonstrictly;

import com.google.common.base.Preconditions;
import dev.cel.common.ast.CelExpr;
Expand All @@ -32,7 +32,7 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) {
ErrorValue errorValue = null;
AccumulatedUnknowns unknowns = null;
for (PlannedInterpretable arg : args) {
Object argVal = evalNonstrictly(arg, resolver, frame);
Object argVal = evalBooleanNonstrictly(arg, resolver, frame);
if (argVal instanceof Boolean) {
// Short-circuit on false
if (!((boolean) argVal)) {
Expand All @@ -45,12 +45,6 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) {
}
} else if (argVal instanceof AccumulatedUnknowns) {
unknowns = AccumulatedUnknowns.maybeMerge(unknowns, argVal);
} else {
errorValue =
ErrorValue.create(
arg.expr().id(),
new IllegalArgumentException(
String.format("Expected boolean value, found: %s", argVal)));
}
}

Expand Down
11 changes: 6 additions & 5 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalConditional.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@

package dev.cel.runtime.planner;

import com.google.common.base.Preconditions;
import static com.google.common.base.Preconditions.checkArgument;
import static dev.cel.runtime.planner.EvalHelpers.evalStrictly;

import dev.cel.common.ast.CelExpr;
import dev.cel.runtime.AccumulatedUnknowns;
import dev.cel.runtime.CelEvaluationException;
Expand All @@ -30,16 +32,15 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) throws CelEva
PlannedInterpretable condition = args[0];
PlannedInterpretable truthy = args[1];
PlannedInterpretable falsy = args[2];
Object condResult = condition.eval(resolver, frame);
Object condResult = evalStrictly(condition, resolver, frame);
if (condResult instanceof AccumulatedUnknowns) {
return condResult;
}
if (!(condResult instanceof Boolean)) {
throw new IllegalArgumentException(
String.format("Expected boolean value, found :%s", condResult));
String.format("Expected boolean value, found: %s", condResult));
}

// TODO: Handle exhaustive eval
if ((boolean) condResult) {
return truthy.eval(resolver, frame);
}
Expand All @@ -53,7 +54,7 @@ static EvalConditional create(CelExpr expr, PlannedInterpretable[] args) {

private EvalConditional(CelExpr expr, PlannedInterpretable[] args) {
super(expr);
Preconditions.checkArgument(args.length == 3);
checkArgument(args.length == 3);
this.args = args;
}
}
17 changes: 7 additions & 10 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalExhaustiveAnd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@

package dev.cel.runtime.planner;

import static dev.cel.runtime.planner.EvalHelpers.evalNonstrictly;
import static com.google.common.base.Preconditions.checkArgument;
import static dev.cel.runtime.planner.EvalHelpers.evalBooleanNonstrictly;

import com.google.errorprone.annotations.Immutable;
import dev.cel.common.ast.CelExpr;
Expand All @@ -41,11 +42,9 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) {
boolean hasFalse = false;

for (PlannedInterpretable arg : args) {
Object argVal = evalNonstrictly(arg, resolver, frame);
if (argVal instanceof Boolean) {
if (!((boolean) argVal)) {
hasFalse = true;
}
Object argVal = evalBooleanNonstrictly(arg, resolver, frame);
if (argVal instanceof Boolean && !(boolean) argVal) {
hasFalse = true;
}

// If we already encountered a false, we do not need to accumulate unknowns or errors
Expand All @@ -55,10 +54,7 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) {
}

if (argVal instanceof AccumulatedUnknowns) {
accumulatedUnknowns =
accumulatedUnknowns == null
? (AccumulatedUnknowns) argVal
: accumulatedUnknowns.merge((AccumulatedUnknowns) argVal);
accumulatedUnknowns = AccumulatedUnknowns.maybeMerge(accumulatedUnknowns, argVal);
} else if (argVal instanceof ErrorValue) {
if (errorValue == null) {
errorValue = (ErrorValue) argVal;
Expand Down Expand Up @@ -87,6 +83,7 @@ static EvalExhaustiveAnd create(CelExpr expr, PlannedInterpretable[] args) {

private EvalExhaustiveAnd(CelExpr expr, PlannedInterpretable[] args) {
super(expr);
checkArgument(args.length == 2);
this.args = args;
}
}
16 changes: 7 additions & 9 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalExhaustiveConditional.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@

package dev.cel.runtime.planner;

import static com.google.common.base.Preconditions.checkArgument;
import static dev.cel.runtime.planner.EvalHelpers.evalBooleanNonstrictly;
import static dev.cel.runtime.planner.EvalHelpers.evalNonstrictly;

import com.google.errorprone.annotations.Immutable;
import dev.cel.common.ast.CelExpr;
import dev.cel.common.values.ErrorValue;
import dev.cel.runtime.AccumulatedUnknowns;
import dev.cel.runtime.CelEvaluationException;
import dev.cel.runtime.GlobalResolver;

/**
Expand All @@ -36,24 +38,19 @@ final class EvalExhaustiveConditional extends PlannedInterpretable {
private final PlannedInterpretable[] args;

@Override
Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) throws CelEvaluationException {
Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) {
PlannedInterpretable condition = args[0];
PlannedInterpretable truthy = args[1];
PlannedInterpretable falsy = args[2];

Object condResult = condition.eval(resolver, frame);
Object condResult = evalBooleanNonstrictly(condition, resolver, frame);
Object truthyVal = evalNonstrictly(truthy, resolver, frame);
Object falsyVal = evalNonstrictly(falsy, resolver, frame);

if (condResult instanceof AccumulatedUnknowns) {
if (condResult instanceof AccumulatedUnknowns || condResult instanceof ErrorValue) {
return condResult;
}

if (!(condResult instanceof Boolean)) {
throw new IllegalArgumentException(
String.format("Expected boolean value, found :%s", condResult));
}

return (boolean) condResult ? truthyVal : falsyVal;
}

Expand All @@ -63,6 +60,7 @@ static EvalExhaustiveConditional create(CelExpr expr, PlannedInterpretable[] arg

private EvalExhaustiveConditional(CelExpr expr, PlannedInterpretable[] args) {
super(expr);
checkArgument(args.length == 3);
this.args = args;
}
}
17 changes: 7 additions & 10 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalExhaustiveOr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@

package dev.cel.runtime.planner;

import static dev.cel.runtime.planner.EvalHelpers.evalNonstrictly;
import static com.google.common.base.Preconditions.checkArgument;
import static dev.cel.runtime.planner.EvalHelpers.evalBooleanNonstrictly;

import com.google.errorprone.annotations.Immutable;
import dev.cel.common.ast.CelExpr;
Expand All @@ -41,11 +42,9 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) {
boolean hasTrue = false;

for (PlannedInterpretable arg : args) {
Object argVal = evalNonstrictly(arg, resolver, frame);
if (argVal instanceof Boolean) {
if ((boolean) argVal) {
hasTrue = true;
}
Object argVal = evalBooleanNonstrictly(arg, resolver, frame);
if (argVal instanceof Boolean && (boolean) argVal) {
hasTrue = true;
}

// If we already encountered a true, we do not need to accumulate unknowns or errors
Expand All @@ -55,10 +54,7 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) {
}

if (argVal instanceof AccumulatedUnknowns) {
accumulatedUnknowns =
accumulatedUnknowns == null
? (AccumulatedUnknowns) argVal
: accumulatedUnknowns.merge((AccumulatedUnknowns) argVal);
accumulatedUnknowns = AccumulatedUnknowns.maybeMerge(accumulatedUnknowns, argVal);
} else if (argVal instanceof ErrorValue) {
if (errorValue == null) {
errorValue = (ErrorValue) argVal;
Expand Down Expand Up @@ -87,6 +83,7 @@ static EvalExhaustiveOr create(CelExpr expr, PlannedInterpretable[] args) {

private EvalExhaustiveOr(CelExpr expr, PlannedInterpretable[] args) {
super(expr);
checkArgument(args.length == 2);
this.args = args;
}
}
29 changes: 23 additions & 6 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,20 +31,33 @@ final class EvalHelpers {
static Object evalNonstrictly(
PlannedInterpretable interpretable, GlobalResolver resolver, ExecutionFrame frame) {
try {
return interpretable.eval(resolver, frame);
return evalStrictly(interpretable, resolver, frame);
} catch (LocalizedEvaluationException e) {
return ErrorValue.create(e.exprId(), e);
}
}

static Object evalBooleanNonstrictly(
PlannedInterpretable interpretable, GlobalResolver resolver, ExecutionFrame frame) {
try {
Object val = evalStrictly(interpretable, resolver, frame);
if (!(val instanceof Boolean) && !(val instanceof AccumulatedUnknowns)) {
throw new LocalizedEvaluationException(
new IllegalArgumentException(String.format("Expected boolean value, found: %s", val)),
CelErrorCode.INTERNAL_ERROR,
interpretable.expr().id());
}
return val;
} catch (LocalizedEvaluationException e) {
// Intercept the localized exception to get a more specific expr ID for error reporting
// Example: foo [1] && strict_err [2] -> ID 2 is propagated.
return ErrorValue.create(e.exprId(), e);
} catch (Exception e) {
return ErrorValue.create(interpretable.expr().id(), e);
}
}

static Object evalStrictly(
PlannedInterpretable interpretable, GlobalResolver resolver, ExecutionFrame frame) {
Object val;
try {
return interpretable.eval(resolver, frame);
val = interpretable.eval(resolver, frame);
} catch (LocalizedEvaluationException e) {
// Already localized - propagate as-is to preserve inner expression ID
throw e;
Expand All @@ -56,6 +69,10 @@ static Object evalStrictly(
throw new LocalizedEvaluationException(
e, CelErrorCode.INTERNAL_ERROR, interpretable.expr().id());
}
if (val instanceof ErrorValue) {
throw ((ErrorValue) val).value();
}
return val;
}

static Object dispatch(
Expand Down
10 changes: 2 additions & 8 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalOr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

package dev.cel.runtime.planner;

import static dev.cel.runtime.planner.EvalHelpers.evalNonstrictly;
import static dev.cel.runtime.planner.EvalHelpers.evalBooleanNonstrictly;

import com.google.common.base.Preconditions;
import dev.cel.common.ast.CelExpr;
Expand All @@ -32,7 +32,7 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) {
ErrorValue errorValue = null;
AccumulatedUnknowns unknowns = null;
for (PlannedInterpretable arg : args) {
Object argVal = evalNonstrictly(arg, resolver, frame);
Object argVal = evalBooleanNonstrictly(arg, resolver, frame);
if (argVal instanceof Boolean) {
// Short-circuit on true
if (((boolean) argVal)) {
Expand All @@ -45,12 +45,6 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) {
}
} else if (argVal instanceof AccumulatedUnknowns) {
unknowns = AccumulatedUnknowns.maybeMerge(unknowns, argVal);
} else {
errorValue =
ErrorValue.create(
arg.expr().id(),
new IllegalArgumentException(
String.format("Expected boolean value, found: %s", argVal)));
}
}

Expand Down
Loading
Loading