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 59093bcf2..99206ade6 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel +++ b/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel @@ -454,6 +454,7 @@ java_library( "//common/exceptions:runtime_exception", "//common/values", "//runtime:accumulated_unknowns", + "//runtime:concatenated_list_view", "//runtime:evaluation_exception", "//runtime:interpretable", "//runtime:interpreter_util", @@ -1119,6 +1120,7 @@ cel_android_library( "//common/exceptions:runtime_exception", "//common/values:values_android", "//runtime:accumulated_unknowns_android", + "//runtime:concatenated_list_view", "//runtime:evaluation_exception", "//runtime:interpretable_android", "//runtime:interpreter_util_android", diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalFold.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalFold.java index 6f9776362..d10fb7a74 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalFold.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalFold.java @@ -62,16 +62,13 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) throws CelEva } Folder folder = new Folder(resolver, frame, accuInit, accuVar, iterVar, iterVar2); - Object result; if (iterRangeRaw instanceof Map) { - result = evalMap((Map) iterRangeRaw, folder, frame); + return evalMap((Map) iterRangeRaw, folder, frame); } else if (iterRangeRaw instanceof Collection) { - result = evalList((Collection) iterRangeRaw, folder, frame); + return evalList((Collection) iterRangeRaw, folder, frame); } else { throw new IllegalArgumentException("Unexpected iter_range type: " + iterRangeRaw.getClass()); } - - return maybeUnwrapAccumulator(result); } private Object evalMap(Map iterRange, Folder folder, ExecutionFrame frame) @@ -94,16 +91,14 @@ private Object evalMap(Map iterRange, Folder folder, ExecutionFrame frame) } boolean cond = (boolean) condResult; if (!cond) { - folder.computeResult = true; - return result.eval(folder, frame); + break; } Object stepResult = loopStep.eval(folder, frame); folder.accuVal = mergeAccumulator(folder.accuVal, stepResult); folder.initialized = true; } - folder.computeResult = true; - return result.eval(folder, frame); + return folder.evalResult(result); } private Object evalList(Collection iterRange, Folder folder, ExecutionFrame frame) @@ -129,8 +124,7 @@ private Object evalList(Collection iterRange, Folder folder, ExecutionFrame f } boolean cond = (boolean) condResult; if (!cond) { - folder.computeResult = true; - return result.eval(folder, frame); + break; } Object stepResult = loopStep.eval(folder, frame); @@ -138,8 +132,7 @@ private Object evalList(Collection iterRange, Folder folder, ExecutionFrame f folder.initialized = true; index++; } - folder.computeResult = true; - return result.eval(folder, frame); + return folder.evalResult(result); } private static Object mergeAccumulator(@Nullable Object currentAccu, Object newVal) { @@ -159,16 +152,6 @@ private static Object maybeWrapAccumulator(Object val) { return val; } - private static Object maybeUnwrapAccumulator(Object val) { - if (val instanceof ConcatenatedListView) { - return ImmutableList.copyOf((ConcatenatedListView) val); - } - if (val instanceof MutableMapValue) { - return ImmutableMap.copyOf((MutableMapValue) val); - } - return val; - } - private EvalFold( CelExpr expr, String accuVar, @@ -220,7 +203,11 @@ public boolean isLocallyBound(String name) { if (!initialized) { initialized = true; try { - accuVal = maybeWrapAccumulator(accuInit.eval(resolver, frame)); + Object initVal = accuInit.eval(resolver, frame); + accuVal = + !computeResult && frame.enableShortCircuiting() + ? maybeWrapAccumulator(initVal) + : initVal; } catch (CelEvaluationException e) { throw new LazyEvaluationRuntimeException(e); } @@ -241,6 +228,17 @@ public boolean isLocallyBound(String name) { return resolver.resolve(name); } + private Object evalResult(PlannedInterpretable result) throws CelEvaluationException { + computeResult = true; + // Materialize the mutable accumulator so the result expr never observes in-place mutation. + if (accuVal instanceof ConcatenatedListView) { + accuVal = ImmutableList.copyOf((ConcatenatedListView) accuVal); + } else if (accuVal instanceof MutableMapValue) { + accuVal = ImmutableMap.copyOf((MutableMapValue) accuVal); + } + return result.eval(this, frame); + } + private Folder( GlobalResolver resolver, ExecutionFrame frame, 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 1579714da..012432f45 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java @@ -23,6 +23,7 @@ import dev.cel.runtime.CelEvaluationException; import dev.cel.runtime.CelResolvedOverload; import dev.cel.runtime.CelUnknownSet; +import dev.cel.runtime.ConcatenatedListView; import dev.cel.runtime.GlobalResolver; import dev.cel.runtime.InterpreterUtil; @@ -130,7 +131,10 @@ static Object maybeAdaptNonStrictArg(Object val) { * adapts any public {@link CelUnknownSet} instances into internal {@link AccumulatedUnknowns} for * AST evaluation. */ - private static Object convertAndAdaptResult(CelValueConverter valueConverter, Object result) { + static Object convertAndAdaptResult(CelValueConverter valueConverter, Object result) { + if (result instanceof ConcatenatedListView) { + return result; + } return InterpreterUtil.maybeAdaptToAccumulatedUnknowns( valueConverter.maybeUnwrap(valueConverter.toRuntimeValue(result))); } diff --git a/runtime/src/main/java/dev/cel/runtime/planner/ExecutionFrame.java b/runtime/src/main/java/dev/cel/runtime/planner/ExecutionFrame.java index d6034e46f..23dd735d4 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/ExecutionFrame.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/ExecutionFrame.java @@ -32,6 +32,7 @@ final class ExecutionFrame { private final int comprehensionIterationLimit; + private final boolean enableShortCircuiting; private final CelFunctionResolver functionResolver; private final @Nullable PartialVars partialVars; private final @Nullable CelEvaluationListener listener; @@ -45,11 +46,7 @@ static ExecutionFrame create( @Nullable PartialVars partialVars, @Nullable CelEvaluationListener listener) { return new ExecutionFrame( - functionResolver, - getComprehensionMaxIterations(celOptions), - partialVars, - listener, - /* asyncTracker= */ null); + functionResolver, celOptions, partialVars, listener, /* asyncTracker= */ null); } static ExecutionFrame createForAsync( @@ -59,12 +56,7 @@ static ExecutionFrame createForAsync( @Nullable CelEvaluationListener listener, AsyncCallStateTracker asyncTracker) { checkNotNull(asyncTracker, "asyncTracker"); - return new ExecutionFrame( - functionResolver, - getComprehensionMaxIterations(celOptions), - partialVars, - listener, - asyncTracker); + return new ExecutionFrame(functionResolver, celOptions, partialVars, listener, asyncTracker); } private static int getComprehensionMaxIterations(CelOptions celOptions) { @@ -109,6 +101,10 @@ AsyncCallStateTracker asyncTracker() { return asyncTracker; } + boolean enableShortCircuiting() { + return enableShortCircuiting; + } + Optional partialVars() { return Optional.ofNullable(partialVars); } @@ -119,11 +115,12 @@ Optional partialVars() { private ExecutionFrame( CelFunctionResolver functionResolver, - int limit, + CelOptions celOptions, @Nullable PartialVars partialVars, @Nullable CelEvaluationListener listener, @Nullable AsyncCallStateTracker asyncTracker) { - this.comprehensionIterationLimit = limit; + this.comprehensionIterationLimit = getComprehensionMaxIterations(celOptions); + this.enableShortCircuiting = celOptions.enableShortCircuiting(); this.functionResolver = functionResolver; this.partialVars = partialVars; this.listener = listener; diff --git a/runtime/src/main/java/dev/cel/runtime/planner/NamespacedAttribute.java b/runtime/src/main/java/dev/cel/runtime/planner/NamespacedAttribute.java index 5228ea669..0adc3e47f 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/NamespacedAttribute.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/NamespacedAttribute.java @@ -198,7 +198,7 @@ static Object applyQualifiers( } } - return celValueConverter.maybeUnwrap(celValueConverter.toRuntimeValue(obj)); + return EvalHelpers.convertAndAdaptResult(celValueConverter, obj); } private static Optional findPartialMatchingPattern( 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 beca27e34..9db4a8e82 100644 --- a/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java +++ b/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java @@ -166,7 +166,8 @@ public final class ProgramPlannerTest { newMemberOverload( "bytes_concat_bytes", SimpleType.BYTES, SimpleType.BYTES, SimpleType.BYTES))) .addMessageTypes(TestAllTypes.getDescriptor()) - .addLibraries(CelExtensions.optional(), CelExtensions.comprehensions()) + .addLibraries( + CelExtensions.optional(), CelExtensions.comprehensions(), CelExtensions.bindings()) .setContainer(CEL_CONTAINER) .build(); @@ -200,6 +201,14 @@ private static DefaultDispatcher newDispatcher() { DescriptorTypeResolver.create(TYPE_PROVIDER, CelValueConverter.getDefaultInstance())); addBindingsToDispatcher( builder, typeFunction.newFunctionBindings(CEL_OPTIONS, RUNTIME_EQUALITY)); + addBindingsToDispatcher( + builder, + CelFunctionBinding.fromOverloads( + "cel.@mapInsert", + CelFunctionBinding.from( + "cel_@mapInsert_map_key_value", + ImmutableList.of(Map.class, Object.class, Object.class), + args -> args[0]))); // Custom functions addBindingsToDispatcher( @@ -1002,6 +1011,9 @@ public void plan_select_badPresenceTest_throws() throws Exception { @TestParameters("{expression: '[1,2,3].exists(i, v, i >= 0 && v > 0) == true'}") @TestParameters("{expression: '[1,2,3].exists(i, v, i < 0 || v < 0) == false'}") @TestParameters("{expression: '[1,2,3].map(x, x + 1) == [2,3,4]'}") + @TestParameters( + "{expression: 'cel.bind(x, [1, 2], [x + [3], x + [4]]) == [[1, 2, 3], [1, 2, 4]]'}") + @TestParameters("{expression: 'cel.bind(m, {\"a\": 1}, [m]) == [{\"a\": 1}]'}") public void plan_comprehension_lists(String expression) throws Exception { CelAbstractSyntaxTree ast = compile(expression); Program program = PLANNER.plan(ast); @@ -1016,6 +1028,7 @@ public void plan_comprehension_lists(String expression) throws Exception { @TestParameters("{expression: '{\"a\": 1, \"b\": 2}.exists(k, k == \"c\") == false'}") @TestParameters("{expression: '{\"a\": \"b\", \"c\": \"c\"}.exists(k, v, k == v)'}") @TestParameters("{expression: '{\"a\": 1, \"b\": 2}.exists(k, v, v == 3) == false'}") + @TestParameters("{expression: '({}.map(k, k) + [1])[0] == 1'}") public void plan_comprehension_maps(String expression) throws Exception { CelAbstractSyntaxTree ast = compile(expression); Program program = PLANNER.plan(ast); @@ -2054,6 +2067,95 @@ public void plan_exhaustiveConditional_untakenBranchError_evaluatesSuccessfully( assertThat(result).isEqualTo(42L); } + @Test + public void plan_exhaustiveComprehension_filterDoesNotMutateUntakenBranch() throws Exception { + CelAbstractSyntaxTree ast = CEL_COMPILER.compile("[1, 2, 3].filter(x, x > 1)").getAst(); + ProgramPlanner planner = + newPlannerWithOptions(CelOptions.current().enableShortCircuiting(false).build()); + Program program = planner.plan(ast); + + Object result = program.eval(); + + assertThat(result).isEqualTo(ImmutableList.of(2L, 3L)); + } + + @Test + @TestParameters("{expression: '[1, 2, 3].map(x, x + 1) == [2, 3, 4]'}") + @TestParameters("{expression: '{\"a\": 1, \"b\": 2}.transformMap(k, v, v) == {}'}") + @SuppressWarnings("Immutable") // Test only + public void plan_comprehension_accumulationSkipsIntermediateConversion(String expression) + throws Exception { + int[] containerConversions = new int[1]; + CelValueConverter countingConverter = + new CelValueConverter() { + @Override + public Object toRuntimeValue(Object value) { + if (value instanceof Iterable || value instanceof ImmutableMap) { + containerConversions[0]++; + } + return super.toRuntimeValue(value); + } + }; + ProgramPlanner planner = + ProgramPlanner.newPlanner( + TYPE_PROVIDER, + VALUE_PROVIDER, + newDispatcher(), + countingConverter, + CEL_CONTAINER, + CEL_OPTIONS, + ImmutableSet.of(), + RUNTIME_EQUALITY, + CelAsyncEvaluationOptions.defaultOptions(), + /* asyncExecutor= */ null); + Program program = planner.plan(CEL_COMPILER.compile(expression).getAst()); + + boolean result = (boolean) program.eval(); + + assertThat(result).isTrue(); + assertThat(containerConversions[0]).isEqualTo(1); + } + + @Test + public void plan_nonStrictFunction_withErrorArg_adaptsToException() throws Exception { + CelCompiler compiler = + CelCompilerFactory.standardCelCompilerBuilder() + .addFunctionDeclarations( + newFunctionDeclaration( + "is_error", newGlobalOverload("is_error_int", SimpleType.BOOL, SimpleType.INT))) + .build(); + DefaultDispatcher.Builder builder = DefaultDispatcher.newBuilder(); + addBindingsToDispatcher( + builder, + CelStandardFunctions.newBuilder() + .includeFunctions(StandardFunction.DIVIDE) + .build() + .newFunctionBindings(RUNTIME_EQUALITY, CEL_OPTIONS)); + builder.addOverload( + "is_error", + "is_error_int", + ImmutableList.of(Long.class), + /* isStrict= */ false, + args -> args[0] instanceof Exception); + ProgramPlanner planner = + ProgramPlanner.newPlanner( + TYPE_PROVIDER, + VALUE_PROVIDER, + builder.build(), + CEL_VALUE_CONVERTER, + CEL_CONTAINER, + CEL_OPTIONS, + ImmutableSet.of(), + RUNTIME_EQUALITY, + CelAsyncEvaluationOptions.defaultOptions(), + /* asyncExecutor= */ null); + Program program = planner.plan(compiler.compile("is_error(1 / 0)").getAst()); + + Object result = program.eval(); + + assertThat(result).isEqualTo(true); + } + private static ProgramPlanner newPlannerWithOptions(CelOptions options) { return ProgramPlanner.newPlanner( TYPE_PROVIDER,