From 1916266a430620637877ed232b83fb61af14770e Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Fri, 13 Feb 2026 16:44:19 -0800 Subject: [PATCH] Fix function dispatch failure error messages to be properly formatted PiperOrigin-RevId: 869938871 --- .../java/dev/cel/runtime/planner/BUILD.bazel | 9 ++++--- .../dev/cel/runtime/planner/EvalHelpers.java | 26 +++++++++++++++++++ .../runtime/planner/EvalLateBoundCall.java | 8 +----- .../dev/cel/runtime/planner/EvalUnary.java | 8 +----- .../cel/runtime/planner/EvalVarArgsCall.java | 8 +----- .../cel/runtime/planner/EvalZeroArity.java | 8 +----- .../cel/runtime/planner/PlannedProgram.java | 6 ++++- .../cel/runtime/PlannerInterpreterTest.java | 6 ----- .../runtime/planner/ProgramPlannerTest.java | 6 +++-- 9 files changed, 44 insertions(+), 41 deletions(-) diff --git a/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel b/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel index 59f39fc91..99273f2fd 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel +++ b/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel @@ -199,10 +199,10 @@ java_library( name = "eval_zero_arity", srcs = ["EvalZeroArity.java"], deps = [ + ":eval_helpers", ":execution_frame", ":planned_interpretable", "//common/values", - "//common/values:cel_value", "//runtime:evaluation_exception", "//runtime:interpretable", "//runtime:resolved_overload", @@ -217,7 +217,6 @@ java_library( ":execution_frame", ":planned_interpretable", "//common/values", - "//common/values:cel_value", "//runtime:evaluation_exception", "//runtime:interpretable", "//runtime:resolved_overload", @@ -232,7 +231,6 @@ java_library( ":execution_frame", ":planned_interpretable", "//common/values", - "//common/values:cel_value", "//runtime:evaluation_exception", "//runtime:interpretable", "//runtime:resolved_overload", @@ -248,7 +246,6 @@ java_library( ":planned_interpretable", "//common/exceptions:overload_not_found", "//common/values", - "//common/values:cel_value", "//runtime:evaluation_exception", "//runtime:interpretable", "//runtime:resolved_overload", @@ -377,7 +374,11 @@ java_library( "//common:error_codes", "//common/exceptions:runtime_exception", "//common/values", + "//common/values:cel_value", + "//runtime:evaluation_exception", "//runtime:interpretable", + "//runtime:resolved_overload", + "@maven//:com_google_guava_guava", ], ) diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java index 7923682ee..08f4fa8a8 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java @@ -14,9 +14,14 @@ package dev.cel.runtime.planner; +import com.google.common.base.Joiner; import dev.cel.common.CelErrorCode; import dev.cel.common.exceptions.CelRuntimeException; +import dev.cel.common.values.CelValue; +import dev.cel.common.values.CelValueConverter; import dev.cel.common.values.ErrorValue; +import dev.cel.runtime.CelEvaluationException; +import dev.cel.runtime.CelResolvedOverload; import dev.cel.runtime.GlobalResolver; final class EvalHelpers { @@ -51,5 +56,26 @@ static Object evalStrictly( } } + static Object dispatch(CelResolvedOverload overload, CelValueConverter valueConverter, Object[] args) throws CelEvaluationException { + try { + Object result = overload.getDefinition().apply(args); + Object runtimeValue = valueConverter.toRuntimeValue(result); + if (runtimeValue instanceof CelValue) { + return valueConverter.unwrap((CelValue) runtimeValue); + } + + return runtimeValue; + } catch (CelRuntimeException e) { + // Function dispatch failure that's already been handled -- just propagate. + throw e; + } catch (RuntimeException e) { + // Unexpected function dispatch failure. + throw new IllegalArgumentException(String.format( + "Function '%s' failed with arg(s) '%s'", + overload.getOverloadId(), Joiner.on(", ").join(args)), + e); + } + } + private EvalHelpers() {} } diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalLateBoundCall.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalLateBoundCall.java index dedef58ca..a22ba8e94 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalLateBoundCall.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalLateBoundCall.java @@ -18,7 +18,6 @@ import com.google.common.collect.ImmutableList; import dev.cel.common.exceptions.CelOverloadNotFoundException; -import dev.cel.common.values.CelValue; import dev.cel.common.values.CelValueConverter; import dev.cel.runtime.CelEvaluationException; import dev.cel.runtime.CelResolvedOverload; @@ -48,12 +47,7 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) throws CelEval .findOverload(functionName, overloadIds, argVals) .orElseThrow(() -> new CelOverloadNotFoundException(functionName, overloadIds)); - Object result = resolvedOverload.getDefinition().apply(argVals); - Object runtimeValue = celValueConverter.toRuntimeValue(result); - if (runtimeValue instanceof CelValue) { - return celValueConverter.unwrap((CelValue) runtimeValue); - } - return runtimeValue; + return EvalHelpers.dispatch(resolvedOverload, celValueConverter, argVals); } static EvalLateBoundCall create( diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalUnary.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalUnary.java index e936c511b..c715ff032 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalUnary.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalUnary.java @@ -17,7 +17,6 @@ import static dev.cel.runtime.planner.EvalHelpers.evalNonstrictly; import static dev.cel.runtime.planner.EvalHelpers.evalStrictly; -import dev.cel.common.values.CelValue; import dev.cel.common.values.CelValueConverter; import dev.cel.runtime.CelEvaluationException; import dev.cel.runtime.CelResolvedOverload; @@ -37,12 +36,7 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) throws CelEval : evalNonstrictly(arg, resolver, frame); Object[] arguments = new Object[] {argVal}; - Object result = resolvedOverload.getDefinition().apply(arguments); - Object runtimeValue = celValueConverter.toRuntimeValue(result); - if (runtimeValue instanceof CelValue) { - return celValueConverter.unwrap((CelValue) runtimeValue); - } - return runtimeValue; + return EvalHelpers.dispatch(resolvedOverload, celValueConverter, arguments); } static EvalUnary create( diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalVarArgsCall.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalVarArgsCall.java index a7f2b643d..9f14f8bf9 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalVarArgsCall.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalVarArgsCall.java @@ -17,7 +17,6 @@ import static dev.cel.runtime.planner.EvalHelpers.evalNonstrictly; import static dev.cel.runtime.planner.EvalHelpers.evalStrictly; -import dev.cel.common.values.CelValue; import dev.cel.common.values.CelValueConverter; import dev.cel.runtime.CelEvaluationException; import dev.cel.runtime.CelResolvedOverload; @@ -43,12 +42,7 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) throws CelEval : evalNonstrictly(arg, resolver, frame); } - Object result = resolvedOverload.getDefinition().apply(argVals); - Object runtimeValue = celValueConverter.toRuntimeValue(result); - if (runtimeValue instanceof CelValue) { - return celValueConverter.unwrap((CelValue) runtimeValue); - } - return runtimeValue; + return EvalHelpers.dispatch(resolvedOverload, celValueConverter, argVals); } static EvalVarArgsCall create( diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalZeroArity.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalZeroArity.java index 1a4218860..5b3138207 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalZeroArity.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalZeroArity.java @@ -14,7 +14,6 @@ package dev.cel.runtime.planner; -import dev.cel.common.values.CelValue; import dev.cel.common.values.CelValueConverter; import dev.cel.runtime.CelEvaluationException; import dev.cel.runtime.CelResolvedOverload; @@ -28,12 +27,7 @@ final class EvalZeroArity extends PlannedInterpretable { @Override public Object eval(GlobalResolver resolver, ExecutionFrame frame) throws CelEvaluationException { - Object result = resolvedOverload.getDefinition().apply(EMPTY_ARRAY); - Object runtimeValue = celValueConverter.toRuntimeValue(result); - if (runtimeValue instanceof CelValue) { - return celValueConverter.unwrap((CelValue) runtimeValue); - } - return runtimeValue; + return EvalHelpers.dispatch(resolvedOverload, celValueConverter, EMPTY_ARRAY); } static EvalZeroArity create( diff --git a/runtime/src/main/java/dev/cel/runtime/planner/PlannedProgram.java b/runtime/src/main/java/dev/cel/runtime/planner/PlannedProgram.java index 22aff43c8..9fcd05447 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/PlannedProgram.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/PlannedProgram.java @@ -115,7 +115,11 @@ private CelEvaluationException newCelEvaluationException(long exprId, Exception } else if (e instanceof CelRuntimeException) { builder = CelEvaluationExceptionBuilder.newBuilder((CelRuntimeException) e); } else { - builder = CelEvaluationExceptionBuilder.newBuilder(e.getMessage()).setCause(e); + // Unhandled function dispatch failures wraps the original exception with a descriptive message + // (e.g: "Function foo failed with...") + // We need to unwrap the cause here to preserve the original exception message and its cause. + Throwable cause = e.getCause() != null ? e.getCause() : e; + builder = CelEvaluationExceptionBuilder.newBuilder(e.getMessage()).setCause(cause); } return builder.setMetadata(metadata(), exprId).build(); diff --git a/runtime/src/test/java/dev/cel/runtime/PlannerInterpreterTest.java b/runtime/src/test/java/dev/cel/runtime/PlannerInterpreterTest.java index 205fb2bed..00e72a884 100644 --- a/runtime/src/test/java/dev/cel/runtime/PlannerInterpreterTest.java +++ b/runtime/src/test/java/dev/cel/runtime/PlannerInterpreterTest.java @@ -52,12 +52,6 @@ public void typeComparisons() { skipBaselineVerification(); } - @Override - public void optional_errors() { - // TODO: Fix error message for function dispatch failures - skipBaselineVerification(); - } - @Override public void jsonFieldNames() throws Exception { // TODO: Support JSON field names for planner diff --git a/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java b/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java index ca862ed27..9154b5d8b 100644 --- a/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java +++ b/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java @@ -397,10 +397,12 @@ public void plan_call_zeroArgs() throws Exception { public void plan_call_throws() throws Exception { CelAbstractSyntaxTree ast = compile("error()"); Program program = PLANNER.plan(ast); + String expectedOverloadId = isParseOnly ? "error" : "error_overload"; CelEvaluationException e = assertThrows(CelEvaluationException.class, program::eval); - assertThat(e).hasMessageThat().contains("evaluation error at :5: Intentional error"); - assertThat(e).hasCauseThat().isInstanceOf(IllegalArgumentException.class); + assertThat(e).hasMessageThat().contains("evaluation error at :5: Function '" + expectedOverloadId + "' failed with arg(s) ''"); + assertThat(e.getCause()).isInstanceOf(IllegalArgumentException.class); + assertThat(e.getCause().getMessage()).contains("Intentional error"); } @Test