From c31bc896ddb8a29e8a8d47291a46c977dbdd6af0 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Thu, 10 Oct 2024 15:39:57 -0700 Subject: [PATCH 01/63] Add description field to CelPolicy.Variable. PiperOrigin-RevId: 684602440 --- .../src/main/java/dev/cel/policy/CelPolicy.java | 6 ++++++ .../java/dev/cel/policy/CelPolicyYamlParser.java | 3 +++ .../dev/cel/policy/CelPolicyYamlParserTest.java | 16 ++++++++++++++++ 3 files changed, 25 insertions(+) diff --git a/policy/src/main/java/dev/cel/policy/CelPolicy.java b/policy/src/main/java/dev/cel/policy/CelPolicy.java index 33940c692..21bf53b7d 100644 --- a/policy/src/main/java/dev/cel/policy/CelPolicy.java +++ b/policy/src/main/java/dev/cel/policy/CelPolicy.java @@ -231,6 +231,8 @@ public abstract static class Variable { public abstract ValueString expression(); + public abstract Optional description(); + /** Builder for {@link Variable}. */ @AutoValue.Builder public abstract static class Builder implements RequiredFieldsChecker { @@ -239,10 +241,14 @@ public abstract static class Builder implements RequiredFieldsChecker { abstract Optional expression(); + abstract Optional description(); + public abstract Builder setName(ValueString name); public abstract Builder setExpression(ValueString expression); + public abstract Builder setDescription(ValueString description); + @Override public ImmutableList requiredFields() { return ImmutableList.of( diff --git a/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java b/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java index 5e527e73f..3eb8cdbf0 100644 --- a/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java +++ b/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java @@ -287,6 +287,9 @@ public CelPolicy.Variable parseVariable( case "expression": builder.setExpression(ctx.newValueString(valueNode)); break; + case "description": + builder.setDescription(ctx.newValueString(valueNode)); + break; default: tagVisitor.visitVariableTag(ctx, keyId, keyName, valueNode, policyBuilder, builder); break; diff --git a/policy/src/test/java/dev/cel/policy/CelPolicyYamlParserTest.java b/policy/src/test/java/dev/cel/policy/CelPolicyYamlParserTest.java index a62b7255e..90f6da0eb 100644 --- a/policy/src/test/java/dev/cel/policy/CelPolicyYamlParserTest.java +++ b/policy/src/test/java/dev/cel/policy/CelPolicyYamlParserTest.java @@ -48,6 +48,22 @@ public void parser_setEmpty() throws Exception { assertThrows(CelPolicyValidationException.class, () -> POLICY_PARSER.parse("", "")); } + @Test + public void parseYamlPolicy_withDescription() throws Exception { + String policySource = + "rule:\n" + + " variables:\n" + + " - name: 'variable_with_description'\n" + + " description: 'this is a description of the variable'\n" + + " expression: 'true'"; + + CelPolicy policy = POLICY_PARSER.parse(policySource); + + assertThat(policy.rule().variables()).hasSize(1); + assertThat(Iterables.getOnlyElement(policy.rule().variables()).description()) + .hasValue(ValueString.of(10, "this is a description of the variable")); + } + @Test public void parseYamlPolicy_withExplanation() throws Exception { String policySource = From abf857f1ffe05c8fb2e6b82e6c7a76c33df7afaf Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Fri, 11 Oct 2024 14:48:14 -0700 Subject: [PATCH 02/63] Move evaluateExpr utility methods to validator and optimizer PiperOrigin-RevId: 684961932 --- common/ast/BUILD.bazel | 5 -- .../main/java/dev/cel/common/ast/BUILD.bazel | 16 ----- .../java/dev/cel/common/ast/CelExprUtil.java | 71 ------------------- .../dev/cel/optimizer/optimizers/BUILD.bazel | 1 - .../optimizers/ConstantFoldingOptimizer.java | 15 +++- .../dev/cel/validator/validators/BUILD.bazel | 5 +- .../validators/LiteralValidator.java | 25 ++++++- 7 files changed, 40 insertions(+), 98 deletions(-) delete mode 100644 common/src/main/java/dev/cel/common/ast/CelExprUtil.java diff --git a/common/ast/BUILD.bazel b/common/ast/BUILD.bazel index 0a27ae0bc..c4e2455d5 100644 --- a/common/ast/BUILD.bazel +++ b/common/ast/BUILD.bazel @@ -30,11 +30,6 @@ java_library( exports = ["//common/src/main/java/dev/cel/common/ast:expr_factory"], ) -java_library( - name = "expr_util", - exports = ["//common/src/main/java/dev/cel/common/ast:expr_util"], -) - java_library( name = "mutable_expr", exports = ["//common/src/main/java/dev/cel/common/ast:mutable_expr"], diff --git a/common/src/main/java/dev/cel/common/ast/BUILD.bazel b/common/src/main/java/dev/cel/common/ast/BUILD.bazel index a5736cff9..a30f144e6 100644 --- a/common/src/main/java/dev/cel/common/ast/BUILD.bazel +++ b/common/src/main/java/dev/cel/common/ast/BUILD.bazel @@ -103,22 +103,6 @@ java_library( ], ) -java_library( - name = "expr_util", - srcs = ["CelExprUtil.java"], - tags = [ - ], - deps = [ - ":ast", - "//bundle:cel", - "//common", - "//common:compiler_common", - "//runtime", - "@maven//:com_google_errorprone_error_prone_annotations", - "@maven//:com_google_guava_guava", - ], -) - java_library( name = "mutable_expr", srcs = MUTABLE_EXPR_SOURCES, diff --git a/common/src/main/java/dev/cel/common/ast/CelExprUtil.java b/common/src/main/java/dev/cel/common/ast/CelExprUtil.java deleted file mode 100644 index 20217128e..000000000 --- a/common/src/main/java/dev/cel/common/ast/CelExprUtil.java +++ /dev/null @@ -1,71 +0,0 @@ -// Copyright 2023 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package dev.cel.common.ast; - -import com.google.errorprone.annotations.CanIgnoreReturnValue; -import dev.cel.bundle.Cel; -import dev.cel.common.CelAbstractSyntaxTree; -import dev.cel.common.CelSource; -import dev.cel.common.CelValidationException; -import dev.cel.runtime.CelEvaluationException; - -/** Utility class for working with CelExpr. */ -public final class CelExprUtil { - - /** - * Type-checks and evaluates a CelExpr. This method should be used in the context of validating or - * optimizing an AST. - * - * @return Evaluated result. - * @throws CelValidationException if CelExpr fails to type-check. - * @throws CelEvaluationException if CelExpr fails to evaluate. - */ - @CanIgnoreReturnValue - public static Object evaluateExpr(Cel cel, CelExpr expr) - throws CelValidationException, CelEvaluationException { - CelAbstractSyntaxTree ast = - CelAbstractSyntaxTree.newParsedAst(expr, CelSource.newBuilder().build()); - ast = cel.check(ast).getAst(); - - return cel.createProgram(ast).eval(); - } - - /** - * Type-checks and evaluates a CelExpr. The evaluated result is then checked to see if it's the - * expected result type. - * - *

This method should be used in the context of validating or optimizing an AST. - * - * @return Evaluated result. - * @throws CelValidationException if CelExpr fails to type-check. - * @throws CelEvaluationException if CelExpr fails to evaluate. - * @throws IllegalStateException if the evaluated result is not of type {@code - * expectedResultType}. - */ - @CanIgnoreReturnValue - public static Object evaluateExpr(Cel cel, CelExpr expr, Class expectedResultType) - throws CelValidationException, CelEvaluationException { - Object result = evaluateExpr(cel, expr); - if (!expectedResultType.isInstance(result)) { - throw new IllegalStateException( - String.format( - "Expected %s type but got %s instead", - expectedResultType.getName(), result.getClass().getName())); - } - return result; - } - - private CelExprUtil() {} -} diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel b/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel index 396925ef7..fe1341a6c 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel @@ -23,7 +23,6 @@ java_library( "//common:compiler_common", "//common:mutable_ast", "//common/ast", - "//common/ast:expr_util", "//common/ast:mutable_expr", "//common/navigation:mutable_navigation", "//extensions:optional_library", diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/ConstantFoldingOptimizer.java b/optimizer/src/main/java/dev/cel/optimizer/optimizers/ConstantFoldingOptimizer.java index 7cf1ce1c3..dd5a3c211 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/ConstantFoldingOptimizer.java +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/ConstantFoldingOptimizer.java @@ -24,10 +24,11 @@ import dev.cel.bundle.Cel; import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.CelMutableAst; +import dev.cel.common.CelSource; import dev.cel.common.CelValidationException; import dev.cel.common.ast.CelConstant; +import dev.cel.common.ast.CelExpr; import dev.cel.common.ast.CelExpr.ExprKind.Kind; -import dev.cel.common.ast.CelExprUtil; import dev.cel.common.ast.CelMutableExpr; import dev.cel.common.ast.CelMutableExpr.CelMutableCall; import dev.cel.common.ast.CelMutableExpr.CelMutableList; @@ -248,7 +249,7 @@ private Optional maybeFold( throws CelOptimizationException { Object result; try { - result = CelExprUtil.evaluateExpr(cel, CelMutableExprConverter.fromMutableExpr(node.expr())); + result = evaluateExpr(cel, CelMutableExprConverter.fromMutableExpr(node.expr())); } catch (CelValidationException | CelEvaluationException e) { throw new CelOptimizationException( "Constant folding failure. Failed to evaluate subtree due to: " + e.getMessage(), e); @@ -591,6 +592,16 @@ private CelMutableAst pruneOptionalStructElements(CelMutableAst ast, CelMutableE return ast; } + @CanIgnoreReturnValue + private static Object evaluateExpr(Cel cel, CelExpr expr) + throws CelValidationException, CelEvaluationException { + CelAbstractSyntaxTree ast = + CelAbstractSyntaxTree.newParsedAst(expr, CelSource.newBuilder().build()); + ast = cel.check(ast).getAst(); + + return cel.createProgram(ast).eval(); + } + /** Options to configure how Constant Folding behave. */ @AutoValue public abstract static class ConstantFoldingOptions { diff --git a/validator/src/main/java/dev/cel/validator/validators/BUILD.bazel b/validator/src/main/java/dev/cel/validator/validators/BUILD.bazel index 37868fa3a..7986fd014 100644 --- a/validator/src/main/java/dev/cel/validator/validators/BUILD.bazel +++ b/validator/src/main/java/dev/cel/validator/validators/BUILD.bazel @@ -94,10 +94,13 @@ java_library( visibility = ["//visibility:private"], deps = [ "//bundle:cel", + "//common", + "//common:compiler_common", "//common/ast", "//common/ast:expr_factory", - "//common/ast:expr_util", "//common/navigation", + "//runtime", "//validator:ast_validator", + "@maven//:com_google_errorprone_error_prone_annotations", ], ) diff --git a/validator/src/main/java/dev/cel/validator/validators/LiteralValidator.java b/validator/src/main/java/dev/cel/validator/validators/LiteralValidator.java index 0848870cc..2f23dab4c 100644 --- a/validator/src/main/java/dev/cel/validator/validators/LiteralValidator.java +++ b/validator/src/main/java/dev/cel/validator/validators/LiteralValidator.java @@ -14,13 +14,17 @@ package dev.cel.validator.validators; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import dev.cel.bundle.Cel; +import dev.cel.common.CelAbstractSyntaxTree; +import dev.cel.common.CelSource; +import dev.cel.common.CelValidationException; import dev.cel.common.ast.CelExpr; import dev.cel.common.ast.CelExpr.ExprKind.Kind; import dev.cel.common.ast.CelExprFactory; -import dev.cel.common.ast.CelExprUtil; import dev.cel.common.navigation.CelNavigableAst; import dev.cel.common.navigation.CelNavigableExpr; +import dev.cel.runtime.CelEvaluationException; import dev.cel.validator.CelAstValidator; /** @@ -57,7 +61,7 @@ public void validate(CelNavigableAst navigableAst, Cel cel, IssuesFactory issues CelExpr callExpr = exprFactory.newGlobalCall(functionName, exprFactory.newConstant(expr.constant())); try { - CelExprUtil.evaluateExpr(cel, callExpr, expectedResultType); + evaluateExpr(cel, callExpr, expectedResultType); } catch (Exception e) { issuesFactory.addError( expr.id(), @@ -66,4 +70,21 @@ public void validate(CelNavigableAst navigableAst, Cel cel, IssuesFactory issues } }); } + + @CanIgnoreReturnValue + private static Object evaluateExpr(Cel cel, CelExpr expr, Class expectedResultType) + throws CelValidationException, CelEvaluationException { + CelAbstractSyntaxTree ast = + CelAbstractSyntaxTree.newParsedAst(expr, CelSource.newBuilder().build()); + ast = cel.check(ast).getAst(); + Object result = cel.createProgram(ast).eval(); + + if (!expectedResultType.isInstance(result)) { + throw new IllegalStateException( + String.format( + "Expected %s type but got %s instead", + expectedResultType.getName(), result.getClass().getName())); + } + return result; + } } From 1021e70ec2a64be1a17f8c01ae098334634e2103 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Sun, 13 Oct 2024 23:11:52 -0700 Subject: [PATCH 03/63] Cleanup of unused methods and build targets PiperOrigin-RevId: 685583668 --- runtime/BUILD.bazel | 12 ------- .../dev/cel/runtime/DefaultDispatcher.java | 33 ------------------- 2 files changed, 45 deletions(-) diff --git a/runtime/BUILD.bazel b/runtime/BUILD.bazel index 8c0c72a6a..3635c0961 100644 --- a/runtime/BUILD.bazel +++ b/runtime/BUILD.bazel @@ -10,12 +10,6 @@ java_library( exports = ["//runtime/src/main/java/dev/cel/runtime"], ) -java_library( - name = "base", - visibility = ["//visibility:public"], - exports = ["//runtime/src/main/java/dev/cel/runtime:base"], -) - java_library( name = "interpreter", visibility = ["//visibility:public"], @@ -47,9 +41,3 @@ java_library( name = "evaluation_listener", exports = ["//runtime/src/main/java/dev/cel/runtime:evaluation_listener"], ) - -java_library( - name = "runtime_type_provider_legacy", - visibility = ["//visibility:public"], - exports = ["//runtime/src/main/java/dev/cel/runtime:runtime_type_provider_legacy"], -) diff --git a/runtime/src/main/java/dev/cel/runtime/DefaultDispatcher.java b/runtime/src/main/java/dev/cel/runtime/DefaultDispatcher.java index cfc16bee7..bc20a8a0b 100644 --- a/runtime/src/main/java/dev/cel/runtime/DefaultDispatcher.java +++ b/runtime/src/main/java/dev/cel/runtime/DefaultDispatcher.java @@ -17,16 +17,13 @@ import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.errorprone.annotations.Immutable; import javax.annotation.concurrent.ThreadSafe; import com.google.errorprone.annotations.concurrent.GuardedBy; import com.google.protobuf.MessageLite; import dev.cel.common.CelErrorCode; import dev.cel.common.CelOptions; -import dev.cel.common.ExprFeatures; import dev.cel.common.annotations.Internal; -import dev.cel.common.internal.DefaultMessageFactory; import dev.cel.common.internal.DynamicProto; import java.util.ArrayList; import java.util.HashMap; @@ -43,36 +40,6 @@ @ThreadSafe @Internal public final class DefaultDispatcher implements Dispatcher, Registrar { - /** - * Creates a new dispatcher with all standard functions. - * - * @deprecated Migrate to fluent APIs. See {@link CelRuntimeFactory}. - */ - @Deprecated - public static DefaultDispatcher create() { - return create(CelOptions.LEGACY); - } - - /** - * Creates a new dispatcher with all standard functions. - * - * @deprecated Migrate to fluent APIs. See {@link CelRuntimeFactory}. - */ - @Deprecated - public static DefaultDispatcher create(ImmutableSet features) { - return create(CelOptions.fromExprFeatures(features)); - } - - /** - * Creates a new dispatcher with all standard functions. - * - * @deprecated Migrate to fluent APIs. See {@link CelRuntimeFactory}. - */ - @Deprecated - public static DefaultDispatcher create(CelOptions celOptions) { - DynamicProto dynamicProto = DynamicProto.create(DefaultMessageFactory.INSTANCE); - return create(celOptions, dynamicProto, true); - } public static DefaultDispatcher create( CelOptions celOptions, DynamicProto dynamicProto, boolean enableStandardEnvironment) { From 6dc2f16866f8d20c284ae87997903b21041e1e2a Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Tue, 15 Oct 2024 12:18:59 -0700 Subject: [PATCH 04/63] Update baseline tests for java checker: - use unique overload_ids - fix file with typo PiperOrigin-RevId: 686195856 --- .../java/dev/cel/checker/ExprCheckerTest.java | 18 +++++++++--------- .../abstractTypeParameterized.baseline | 14 +++++++------- .../abstractTypeParameterizedError.baseline | 6 +++--- ...ractTypeParameterizedInListLiteral.baseline | 8 ++++---- .../resources/jsonStructTypeError.baseline | 2 +- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/checker/src/test/java/dev/cel/checker/ExprCheckerTest.java b/checker/src/test/java/dev/cel/checker/ExprCheckerTest.java index c232441fc..29ad5146d 100644 --- a/checker/src/test/java/dev/cel/checker/ExprCheckerTest.java +++ b/checker/src/test/java/dev/cel/checker/ExprCheckerTest.java @@ -805,13 +805,13 @@ public void abstractTypeParameterized() throws Exception { "vector", // Declare the function 'vector' to create the abstract type. globalOverload( - "vector", + "vector_type", ImmutableList.of(CelTypes.create(typeParam)), ImmutableList.of("T"), CelTypes.create(abstractType)), // Declare a function to create a new value of abstract type based on a list. globalOverload( - "vector", + "vector_list", ImmutableList.of(createList(typeParam)), ImmutableList.of("T"), abstractType)); @@ -820,7 +820,7 @@ public void abstractTypeParameterized() throws Exception { declareFunction( "at", memberOverload( - "at", + "vector_at_int", ImmutableList.of(abstractType, CelTypes.INT64), ImmutableList.of("T"), typeParam)); @@ -843,13 +843,13 @@ public void abstractTypeParameterizedInListLiteral() throws Exception { "vector", // Declare the function 'vector' to create the abstract type. globalOverload( - "vector", + "vector_type", ImmutableList.of(CelTypes.create(typeParam)), ImmutableList.of("T"), CelTypes.create(abstractType)), // Declare a function to create a new value of abstract type based on a list. globalOverload( - "vector", + "vector_list", ImmutableList.of(createList(typeParam)), ImmutableList.of("T"), abstractType)); @@ -870,23 +870,23 @@ public void abstractTypeParameterizedError() throws Exception { "vector", // Declare the function 'vector' to create the abstract type. globalOverload( - "vector", + "vector_type", ImmutableList.of(CelTypes.create(typeParam)), ImmutableList.of("T"), CelTypes.create(abstractType)), // Declare a function to create a new value of abstract type based on a list. globalOverload( - "vector", + "vector_list", ImmutableList.of(createList(typeParam)), ImmutableList.of("T"), abstractType)); declareFunction( "add", globalOverload( - "add", + "add_vector_type", ImmutableList.of(CelTypes.create(abstractType), CelTypes.create(abstractType)), ImmutableList.of("T"), - abstractType)); + CelTypes.create(abstractType))); source = "add(vector([1, 2]), vector([2u, -1])) == vector([1, 2, 2u, -1])"; runTest(); } diff --git a/checker/src/test/resources/abstractTypeParameterized.baseline b/checker/src/test/resources/abstractTypeParameterized.baseline index 948f61ec9..28cc0000a 100644 --- a/checker/src/test/resources/abstractTypeParameterized.baseline +++ b/checker/src/test/resources/abstractTypeParameterized.baseline @@ -1,10 +1,10 @@ Source: type(vector([1])) == vector(dyn) && vector([1]).at(0) == 1 declare vector { - function vector (type(T)) -> type(vector(T)) - function vector (list(T)) -> vector(T) + function vector_type (type(T)) -> type(vector(T)) + function vector_list (list(T)) -> vector(T) } declare at { - function at vector(T).(int) -> T + function vector_at_int vector(T).(int) -> T } =====> _&&_( @@ -14,20 +14,20 @@ _&&_( [ 1~int ]~list(int) - )~vector(int)^vector + )~vector(int)^vector_list )~type(vector(int))^type, vector( dyn~type(dyn)^dyn - )~type(vector(dyn))^vector + )~type(vector(dyn))^vector_type )~bool^equals, _==_( vector( [ 1~int ]~list(int) - )~vector(int)^vector.at( + )~vector(int)^vector_list.at( 0~int - )~int^at, + )~int^vector_at_int, 1~int )~bool^equals )~bool^logical_and diff --git a/checker/src/test/resources/abstractTypeParameterizedError.baseline b/checker/src/test/resources/abstractTypeParameterizedError.baseline index 140202ab1..8ef50993e 100644 --- a/checker/src/test/resources/abstractTypeParameterizedError.baseline +++ b/checker/src/test/resources/abstractTypeParameterizedError.baseline @@ -1,10 +1,10 @@ Source: add(vector([1, 2]), vector([2u, -1])) == vector([1, 2, 2u, -1]) declare vector { - function vector (type(T)) -> type(vector(T)) - function vector (list(T)) -> vector(T) + function vector_type (type(T)) -> type(vector(T)) + function vector_list (list(T)) -> vector(T) } declare add { - function add (type(vector(T)), type(vector(T))) -> vector(T) + function add_vector_type (type(vector(T)), type(vector(T))) -> type(vector(T)) } =====> ERROR: test_location:1:4: found no matching overload for 'add' applied to '(vector(int), vector(dyn))' (candidates: (type(vector(%T4)), type(vector(%T4)))) diff --git a/checker/src/test/resources/abstractTypeParameterizedInListLiteral.baseline b/checker/src/test/resources/abstractTypeParameterizedInListLiteral.baseline index e07a05598..3425e0325 100644 --- a/checker/src/test/resources/abstractTypeParameterizedInListLiteral.baseline +++ b/checker/src/test/resources/abstractTypeParameterizedInListLiteral.baseline @@ -1,7 +1,7 @@ Source: size([vector([1, 2]), vector([2u, -1])]) == 2 declare vector { - function vector (type(T)) -> type(vector(T)) - function vector (list(T)) -> vector(T) + function vector_type (type(T)) -> type(vector(T)) + function vector_list (list(T)) -> vector(T) } =====> _==_( @@ -12,13 +12,13 @@ _==_( 1~int, 2~int ]~list(int) - )~vector(int)^vector, + )~vector(int)^vector_list, vector( [ 2u~uint, -1~int ]~list(dyn) - )~vector(dyn)^vector + )~vector(dyn)^vector_list ]~list(vector(dyn)) )~int^size_list, 2~int diff --git a/checker/src/test/resources/jsonStructTypeError.baseline b/checker/src/test/resources/jsonStructTypeError.baseline index 83d6bd717..f13507a0d 100644 --- a/checker/src/test/resources/jsonStructTypeError.baseline +++ b/checker/src/test/resources/jsonStructTypeError.baseline @@ -1,4 +1,4 @@ -ource: x["iss"] != TestAllTypes{single_int32: 1} +Source: x["iss"] != TestAllTypes{single_int32: 1} declare x { value google.protobuf.Struct } From 027241155158a519404419002ed221eb96ff1727 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Wed, 16 Oct 2024 01:41:26 -0700 Subject: [PATCH 05/63] Add expr ID set as a field to CelUnknownSet. This serves as a native-type representation for unknown values, intended to replace ExprValue from eval.proto. PiperOrigin-RevId: 686415221 --- .../test/java/dev/cel/bundle/CelImplTest.java | 34 ++++++++++ .../main/java/dev/cel/common/CelOptions.java | 13 ++++ .../src/main/java/dev/cel/runtime/BUILD.bazel | 2 + .../dev/cel/runtime/CallArgumentChecker.java | 25 ++++++-- .../java/dev/cel/runtime/CelUnknownSet.java | 31 ++++++++- .../dev/cel/runtime/DefaultInterpreter.java | 16 +++-- .../java/dev/cel/runtime/InterpreterUtil.java | 64 +++++++++++++------ .../cel/runtime/RuntimeUnknownResolver.java | 35 +++++++--- 8 files changed, 174 insertions(+), 46 deletions(-) diff --git a/bundle/src/test/java/dev/cel/bundle/CelImplTest.java b/bundle/src/test/java/dev/cel/bundle/CelImplTest.java index db8f311e9..38d1c2218 100644 --- a/bundle/src/test/java/dev/cel/bundle/CelImplTest.java +++ b/bundle/src/test/java/dev/cel/bundle/CelImplTest.java @@ -1926,6 +1926,40 @@ public void program_functionParamWithWellKnownType() throws Exception { assertThat(result).isTrue(); } + @Test + public void program_nativeTypeUnknownsEnabled_asIdentifiers() throws Exception { + Cel cel = + CelFactory.standardCelBuilder() + .addVar("x", SimpleType.BOOL) + .addVar("y", SimpleType.BOOL) + .setOptions(CelOptions.current().adaptUnknownValueSetToNativeType(true).build()) + .build(); + CelAbstractSyntaxTree ast = cel.compile("x || y").getAst(); + + CelUnknownSet result = (CelUnknownSet) cel.createProgram(ast).eval(); + + assertThat(result.unknownExprIds()).containsExactly(1L, 3L); + assertThat(result.attributes()).isEmpty(); + } + + @Test + public void program_nativeTypeUnknownsEnabled_asCallArguments() throws Exception { + Cel cel = + CelFactory.standardCelBuilder() + .addVar("x", SimpleType.BOOL) + .addFunctionDeclarations( + newFunctionDeclaration( + "foo", newGlobalOverload("foo_bool", SimpleType.BOOL, SimpleType.BOOL))) + .setOptions(CelOptions.current().adaptUnknownValueSetToNativeType(true).build()) + .build(); + CelAbstractSyntaxTree ast = cel.compile("foo(x)").getAst(); + + CelUnknownSet result = (CelUnknownSet) cel.createProgram(ast).eval(); + + assertThat(result.unknownExprIds()).containsExactly(2L); + assertThat(result.attributes()).isEmpty(); + } + @Test public void toBuilder_isImmutable() { CelBuilder celBuilder = CelFactory.standardCelBuilder(); diff --git a/common/src/main/java/dev/cel/common/CelOptions.java b/common/src/main/java/dev/cel/common/CelOptions.java index 2568099fb..7f7cd7328 100644 --- a/common/src/main/java/dev/cel/common/CelOptions.java +++ b/common/src/main/java/dev/cel/common/CelOptions.java @@ -109,6 +109,8 @@ public enum ProtoUnsetFieldOptions { public abstract ProtoUnsetFieldOptions fromProtoUnsetFieldOption(); + public abstract boolean adaptUnknownValueSetToNativeType(); + public abstract Builder toBuilder(); public ImmutableSet toExprFeatures() { @@ -200,6 +202,7 @@ public static Builder newBuilder() { .enableCelValue(false) .comprehensionMaxIterations(-1) .unwrapWellKnownTypesOnFunctionDispatch(true) + .adaptUnknownValueSetToNativeType(false) .fromProtoUnsetFieldOption(ProtoUnsetFieldOptions.BIND_DEFAULT); } @@ -504,6 +507,16 @@ public abstract static class Builder { */ public abstract Builder fromProtoUnsetFieldOption(ProtoUnsetFieldOptions value); + /** + * If enabled, when the result of an evaluation is a set of unknown values, a native-type + * equivalent {@code CelUnknownSet} is returned as a result. Otherwise, ExprValue from {@code + * eval.proto} is returned instead. + * + *

This is a temporary flag. It will be removed once the consumers have migrated to the + * native-type representation. + */ + public abstract Builder adaptUnknownValueSetToNativeType(boolean value); + public abstract CelOptions build(); } } diff --git a/runtime/src/main/java/dev/cel/runtime/BUILD.bazel b/runtime/src/main/java/dev/cel/runtime/BUILD.bazel index e74420b74..095f69565 100644 --- a/runtime/src/main/java/dev/cel/runtime/BUILD.bazel +++ b/runtime/src/main/java/dev/cel/runtime/BUILD.bazel @@ -19,6 +19,7 @@ BASE_SOURCES = [ "TypeResolver.java", ] +# keep sorted INTERPRETER_SOURCES = [ "Activation.java", "CallArgumentChecker.java", @@ -249,6 +250,7 @@ java_library( ], deps = [ ":base", + ":unknown_attributes", "//common/annotations", "@cel_spec//proto/cel/expr:expr_java_proto", "@maven//:com_google_errorprone_error_prone_annotations", diff --git a/runtime/src/main/java/dev/cel/runtime/CallArgumentChecker.java b/runtime/src/main/java/dev/cel/runtime/CallArgumentChecker.java index a3bd6e4b7..74894a213 100644 --- a/runtime/src/main/java/dev/cel/runtime/CallArgumentChecker.java +++ b/runtime/src/main/java/dev/cel/runtime/CallArgumentChecker.java @@ -32,13 +32,13 @@ @Internal class CallArgumentChecker { private final ArrayList exprIds; - private Optional unknowns; private final RuntimeUnknownResolver resolver; private final boolean acceptPartial; + private Optional unknowns; private CallArgumentChecker(RuntimeUnknownResolver resolver, boolean acceptPartial) { - exprIds = new ArrayList<>(); - unknowns = Optional.empty(); + this.exprIds = new ArrayList<>(); + this.unknowns = Optional.empty(); this.resolver = resolver; this.acceptPartial = acceptPartial; } @@ -76,8 +76,13 @@ void checkArg(DefaultInterpreter.IntermediateResult arg) { // support for ExprValue unknowns. if (InterpreterUtil.isUnknown(arg.value())) { - ExprValue exprValue = (ExprValue) arg.value(); - exprIds.addAll(exprValue.getUnknown().getExprsList()); + if (InterpreterUtil.isExprValueUnknown(arg.value())) { + ExprValue exprValue = (ExprValue) arg.value(); + exprIds.addAll(exprValue.getUnknown().getExprsList()); + } else if (resolver.getAdaptUnknownValueSetOption()) { + CelUnknownSet unknownSet = (CelUnknownSet) arg.value(); + exprIds.addAll(unknownSet.unknownExprIds()); + } } } @@ -98,8 +103,14 @@ Optional maybeUnknowns() { } if (!exprIds.isEmpty()) { - return Optional.of( - ExprValue.newBuilder().setUnknown(UnknownSet.newBuilder().addAllExprs(exprIds)).build()); + if (resolver.getAdaptUnknownValueSetOption()) { + return Optional.of(CelUnknownSet.create(exprIds)); + } else { + return Optional.of( + ExprValue.newBuilder() + .setUnknown(UnknownSet.newBuilder().addAllExprs(exprIds)) + .build()); + } } return Optional.empty(); diff --git a/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java b/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java index 8cde13361..e53d8be94 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java +++ b/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java @@ -28,21 +28,46 @@ */ @AutoValue public abstract class CelUnknownSet { + + /** + * Set of attributes with a series of selection or index operations marked unknown. This set is + * always empty if enableUnknownTracking is disabled in {@code CelOptions}. + */ public abstract ImmutableSet attributes(); - public static CelUnknownSet create(ImmutableSet attributes) { - return new AutoValue_CelUnknownSet(attributes); - } + /** Set of subexpression IDs that were decided to be unknown and in the critical path. */ + public abstract ImmutableSet unknownExprIds(); public static CelUnknownSet create(CelAttribute attribute) { return create(ImmutableSet.of(attribute)); } + public static CelUnknownSet create(ImmutableSet attributes) { + return create(attributes, ImmutableSet.of()); + } + + static CelUnknownSet create(Long... unknownExprIds) { + return create(ImmutableSet.copyOf(unknownExprIds)); + } + + static CelUnknownSet create(Iterable unknownExprIds) { + return create(ImmutableSet.of(), ImmutableSet.copyOf(unknownExprIds)); + } + + private static CelUnknownSet create( + ImmutableSet attributes, ImmutableSet unknownExprIds) { + return new AutoValue_CelUnknownSet(attributes, unknownExprIds); + } + public static CelUnknownSet union(CelUnknownSet lhs, CelUnknownSet rhs) { return create( ImmutableSet.builder() .addAll(lhs.attributes()) .addAll(rhs.attributes()) + .build(), + ImmutableSet.builder() + .addAll(lhs.unknownExprIds()) + .addAll(rhs.unknownExprIds()) .build()); } diff --git a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java index babec5dda..dc633e1a9 100644 --- a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java +++ b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java @@ -108,10 +108,6 @@ static IntermediateResult create(Object value) { } } - public DefaultInterpreter(RuntimeTypeProvider typeProvider, Dispatcher dispatcher) { - this(typeProvider, dispatcher, CelOptions.LEGACY); - } - /** * Creates a new interpreter * @@ -167,7 +163,10 @@ public Object eval(GlobalResolver resolver) throws InterpreterException { @Override public Object eval(GlobalResolver resolver, CelEvaluationListener listener) throws InterpreterException { - return evalTrackingUnknowns(RuntimeUnknownResolver.fromResolver(resolver), listener); + return evalTrackingUnknowns( + RuntimeUnknownResolver.fromResolver( + resolver, celOptions.adaptUnknownValueSetToNativeType()), + listener); } @Override @@ -333,7 +332,9 @@ private IntermediateResult evalFieldSelect( Object fieldValue = typeProvider.selectField(operand, field); return IntermediateResult.create( - attribute, InterpreterUtil.valueOrUnknown(fieldValue, expr.id())); + attribute, + InterpreterUtil.valueOrUnknown( + fieldValue, expr.id(), celOptions.adaptUnknownValueSetToNativeType())); } private IntermediateResult evalCall(ExecutionFrame frame, CelExpr expr, CelCall callExpr) @@ -486,7 +487,8 @@ private IntermediateResult mergeBooleanUnknowns(IntermediateResult lhs, Intermed // Otherwise fallback to normal impl return IntermediateResult.create( - InterpreterUtil.shortcircuitUnknownOrThrowable(lhs.value(), rhs.value())); + InterpreterUtil.shortcircuitUnknownOrThrowable( + lhs.value(), rhs.value(), celOptions.adaptUnknownValueSetToNativeType())); } private enum ShortCircuitableOperators { diff --git a/runtime/src/main/java/dev/cel/runtime/InterpreterUtil.java b/runtime/src/main/java/dev/cel/runtime/InterpreterUtil.java index 7f2808c0e..7c6a90468 100644 --- a/runtime/src/main/java/dev/cel/runtime/InterpreterUtil.java +++ b/runtime/src/main/java/dev/cel/runtime/InterpreterUtil.java @@ -17,9 +17,7 @@ import dev.cel.expr.ExprValue; import dev.cel.expr.UnknownSet; import dev.cel.common.annotations.Internal; -import java.util.Arrays; import java.util.LinkedHashSet; -import java.util.List; import java.util.Set; import org.jspecify.annotations.Nullable; @@ -56,8 +54,19 @@ public static Object strict(Object valueOrThrowable) throws InterpreterException * @return boolean value if object is unknown. */ public static boolean isUnknown(Object obj) { - return obj instanceof ExprValue - && ((ExprValue) obj).getKindCase() == ExprValue.KindCase.UNKNOWN; + if (isExprValueUnknown(obj)) { + return true; + } + return obj instanceof CelUnknownSet; + } + + /** TODO: Remove once clients have been migrated. */ + static boolean isExprValueUnknown(Object obj) { + if (obj instanceof ExprValue) { + return ((ExprValue) obj).getKindCase() == ExprValue.KindCase.UNKNOWN; + } + + return false; } /** @@ -67,7 +76,7 @@ public static boolean isUnknown(Object obj) { * @return A new ExprValue object which has all unknown expr ids from input objects, without * duplication. */ - public static ExprValue combineUnknownExprValue(Object... objs) { + static ExprValue combineUnknownProtoExprValue(Object... objs) { UnknownSet.Builder unknownsetBuilder = UnknownSet.newBuilder(); Set ids = new LinkedHashSet<>(); for (Object object : objs) { @@ -79,21 +88,25 @@ public static ExprValue combineUnknownExprValue(Object... objs) { return ExprValue.newBuilder().setUnknown(unknownsetBuilder).build(); } - /** Create a {@code ExprValue} for one or more {@code ids} representing an unknown set. */ - public static ExprValue createUnknownExprValue(Long... ids) { - return createUnknownExprValue(Arrays.asList(ids)); + static CelUnknownSet combineUnknownExprValue(Object... objs) { + Set ids = new LinkedHashSet<>(); + for (Object object : objs) { + if (isUnknown(object)) { + ids.addAll(((CelUnknownSet) object).unknownExprIds()); + } + } + + return CelUnknownSet.create(ids); } /** * Create an ExprValue object has UnknownSet, from a list of unknown expr ids * - * @param ids List of unknown expr ids + * @param id unknown expr id * @return A new ExprValue object which has all unknown expr ids from input list */ - public static ExprValue createUnknownExprValue(List ids) { - ExprValue.Builder exprValueBuilder = ExprValue.newBuilder(); - exprValueBuilder.setUnknown(UnknownSet.newBuilder().addAllExprs(ids)); - return exprValueBuilder.build(); + private static ExprValue createUnknownExprValue(long id) { + return ExprValue.newBuilder().setUnknown(UnknownSet.newBuilder().addExprs(id)).build(); } /** @@ -104,11 +117,14 @@ public static ExprValue createUnknownExprValue(List ids) { * from any boolean arguments alone. This allows us to consolidate the error/unknown handling for * both of these operators. */ - public static Object shortcircuitUnknownOrThrowable(Object left, Object right) + public static Object shortcircuitUnknownOrThrowable( + Object left, Object right, boolean adaptUnknownValueSetToNativeType) throws InterpreterException { // unknown unknown ==> unknown combined if (InterpreterUtil.isUnknown(left) && InterpreterUtil.isUnknown(right)) { - return InterpreterUtil.combineUnknownExprValue(left, right); + return adaptUnknownValueSetToNativeType + ? InterpreterUtil.combineUnknownExprValue(left, right) + : InterpreterUtil.combineUnknownProtoExprValue(left, right); } // unknown ==> unknown // unknown t|f ==> unknown @@ -132,18 +148,24 @@ public static Object shortcircuitUnknownOrThrowable(Object left, Object right) "Left or/and right object is neither bool, unknown nor error, unexpected behavior."); } - public static Object valueOrUnknown(@Nullable Object valueOrThrowable, Long id) { + public static Object valueOrUnknown( + @Nullable Object valueOrThrowable, Long id, boolean adaptUnknownValueSet) { // Handle the unknown value case. if (isUnknown(valueOrThrowable)) { - ExprValue value = (ExprValue) valueOrThrowable; - if (value.getUnknown().getExprsCount() != 0) { - return valueOrThrowable; + if (adaptUnknownValueSet) { + return CelUnknownSet.create(id); + } else { + // TODO: Remove once clients have been migrated. + ExprValue value = (ExprValue) valueOrThrowable; + if (value.getUnknown().getExprsCount() != 0) { + return valueOrThrowable; + } + return createUnknownExprValue(id); } - return createUnknownExprValue(id); } // Handle the null value case. if (valueOrThrowable == null) { - return createUnknownExprValue(id); + return adaptUnknownValueSet ? CelUnknownSet.create(id) : createUnknownExprValue(id); } return valueOrThrowable; } diff --git a/runtime/src/main/java/dev/cel/runtime/RuntimeUnknownResolver.java b/runtime/src/main/java/dev/cel/runtime/RuntimeUnknownResolver.java index 1c1be9b94..9a08dea10 100644 --- a/runtime/src/main/java/dev/cel/runtime/RuntimeUnknownResolver.java +++ b/runtime/src/main/java/dev/cel/runtime/RuntimeUnknownResolver.java @@ -40,20 +40,29 @@ public class RuntimeUnknownResolver { private final boolean attributeTrackingEnabled; + /** TODO: Remove after callers have been migrated */ + private final boolean adaptUnknownValueSet; + private RuntimeUnknownResolver( GlobalResolver resolver, CelAttributeResolver attributeResolver, - boolean attributeTrackingEnabled) { + boolean attributeTrackingEnabled, + boolean adaptUnknownValueSet) { this.resolver = resolver; this.attributeResolver = attributeResolver; this.attributeTrackingEnabled = attributeTrackingEnabled; + this.adaptUnknownValueSet = adaptUnknownValueSet; } - public static RuntimeUnknownResolver fromResolver(GlobalResolver resolver) { + public static RuntimeUnknownResolver fromResolver( + GlobalResolver resolver, boolean adaptUnknownValueSet) { // This prevents calculating the attribute trail if it will never be used for // efficiency, but doesn't change observable behavior. return new RuntimeUnknownResolver( - resolver, DEFAULT_RESOLVER, /* attributeTrackingEnabled= */ false) {}; + resolver, + DEFAULT_RESOLVER, + /* attributeTrackingEnabled= */ false, + /* adaptUnknownValueSet= */ adaptUnknownValueSet); } public static Builder builder() { @@ -83,7 +92,7 @@ public Builder setAttributeResolver(CelAttributeResolver resolver) { } public RuntimeUnknownResolver build() { - return new RuntimeUnknownResolver(resolver, attributeResolver, true); + return new RuntimeUnknownResolver(resolver, attributeResolver, true, false); } } @@ -111,7 +120,7 @@ DefaultInterpreter.IntermediateResult resolveSimpleName(String name, Long exprId Object result = resolver.resolve(name); return DefaultInterpreter.IntermediateResult.create( - attr, InterpreterUtil.valueOrUnknown(result, exprId)); + attr, InterpreterUtil.valueOrUnknown(result, exprId, adaptUnknownValueSet)); } void cacheLazilyEvaluatedResult(String name, DefaultInterpreter.IntermediateResult result) { @@ -127,7 +136,12 @@ Optional resolveAttribute(CelAttribute attr) { } ScopedResolver withScope(Map vars) { - return new ScopedResolver(this, vars); + return new ScopedResolver(this, vars, adaptUnknownValueSet); + } + + /** TODO: Remove after callers have been migrated */ + boolean getAdaptUnknownValueSetOption() { + return adaptUnknownValueSet; } static final class ScopedResolver extends RuntimeUnknownResolver { @@ -137,8 +151,13 @@ static final class ScopedResolver extends RuntimeUnknownResolver { private ScopedResolver( RuntimeUnknownResolver parent, - Map shadowedVars) { - super(parent.resolver, parent.attributeResolver, parent.attributeTrackingEnabled); + Map shadowedVars, + boolean adaptUnknownValueSet) { + super( + parent.resolver, + parent.attributeResolver, + parent.attributeTrackingEnabled, + adaptUnknownValueSet); this.parent = parent; this.shadowedVars = shadowedVars; this.lazyEvalResultCache = new HashMap<>(); From 7e6578c965733e09a8c8838397d35a31ada4816e Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Wed, 16 Oct 2024 09:50:10 -0700 Subject: [PATCH 06/63] Correctly suppress AutoValueMutable warnings PiperOrigin-RevId: 686542214 --- .../main/java/dev/cel/common/internal/BasicCodePointArray.java | 2 +- .../main/java/dev/cel/common/internal/Latin1CodePointArray.java | 2 +- .../dev/cel/common/internal/SupplementalCodePointArray.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java b/common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java index a240df763..9f1bb5cb6 100644 --- a/common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java +++ b/common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java @@ -38,7 +38,7 @@ @SuppressWarnings("Immutable") // char[] is not exposed externally, thus cannot be mutated. public abstract class BasicCodePointArray extends CelCodePointArray { - @SuppressWarnings("AutoValueImmutableFields") + @SuppressWarnings("mutable") abstract char[] codePoints(); abstract int offset(); diff --git a/common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java b/common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java index 9e54c3a6c..d9536cbb4 100644 --- a/common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java +++ b/common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java @@ -38,7 +38,7 @@ @SuppressWarnings("Immutable") // byte[] is not exposed externally, thus cannot be mutated. public abstract class Latin1CodePointArray extends CelCodePointArray { - @SuppressWarnings("AutoValueImmutableFields") + @SuppressWarnings("mutable") abstract byte[] codePoints(); abstract int offset(); diff --git a/common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java b/common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java index dc3cb10a4..80e6c93d3 100644 --- a/common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java +++ b/common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java @@ -38,7 +38,7 @@ @SuppressWarnings("Immutable") // int[] is not exposed externally, thus cannot be mutated. public abstract class SupplementalCodePointArray extends CelCodePointArray { - @SuppressWarnings("AutoValueImmutableFields") + @SuppressWarnings("mutable") abstract int[] codePoints(); abstract int offset(); From ea78c86d69cbe1b467f90cdb3208b9ac1cfee705 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Wed, 16 Oct 2024 11:44:33 -0700 Subject: [PATCH 07/63] Enforce strictness of type function This ensures that: - type(unknown) -> unknown - type(error) -> error PiperOrigin-RevId: 686585884 --- .../java/dev/cel/runtime/CelUnknownSet.java | 2 +- .../dev/cel/runtime/DefaultInterpreter.java | 14 +++++++--- .../test/resources/unknownResultSet.baseline | 27 +++++++++++++++++++ .../dev/cel/testing/BaseInterpreterTest.java | 8 ++++++ 4 files changed, 46 insertions(+), 5 deletions(-) diff --git a/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java b/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java index e53d8be94..d05be8133 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java +++ b/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java @@ -46,7 +46,7 @@ public static CelUnknownSet create(ImmutableSet attributes) { return create(attributes, ImmutableSet.of()); } - static CelUnknownSet create(Long... unknownExprIds) { + public static CelUnknownSet create(Long... unknownExprIds) { return create(ImmutableSet.copyOf(unknownExprIds)); } diff --git a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java index dc633e1a9..a0dd1f242 100644 --- a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java +++ b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java @@ -222,10 +222,14 @@ private IntermediateResult evalInternal(ExecutionFrame frame, CelExpr expr) } } - private boolean isUnknownValue(Object value) { + private static boolean isUnknownValue(Object value) { return value instanceof CelUnknownSet || InterpreterUtil.isUnknown(value); } + private static boolean isUnknownOrError(Object value) { + return isUnknownValue(value) || value instanceof Exception; + } + private Object evalConstant( ExecutionFrame unusedFrame, CelExpr unusedExpr, CelConstant constExpr) { switch (constExpr.getKind()) { @@ -593,6 +597,10 @@ private IntermediateResult evalType(ExecutionFrame frame, CelCall callExpr) throws InterpreterException { CelExpr typeExprArg = callExpr.args().get(0); IntermediateResult argResult = evalInternal(frame, typeExprArg); + // Type is a strict function. Early return if the argument is an error or an unknown. + if (isUnknownOrError(argResult.value())) { + return argResult; + } CelType checkedType = ast.getType(typeExprArg.id()) @@ -682,9 +690,7 @@ private IntermediateResult evalBoolean(ExecutionFrame frame, CelExpr expr, boole throws InterpreterException { IntermediateResult value = strict ? evalInternal(frame, expr) : evalNonstrictly(frame, expr); - if (!(value.value() instanceof Boolean) - && !isUnknownValue(value.value()) - && !(value.value() instanceof Exception)) { + if (!(value.value() instanceof Boolean) && !isUnknownOrError(value.value())) { throw new InterpreterException.Builder("expected boolean value, found: %s", value.value()) .setErrorCode(CelErrorCode.INVALID_ARGUMENT) .setLocation(metadata, expr.id()) diff --git a/runtime/src/test/resources/unknownResultSet.baseline b/runtime/src/test/resources/unknownResultSet.baseline index 2b2c61f62..7cc661627 100644 --- a/runtime/src/test/resources/unknownResultSet.baseline +++ b/runtime/src/test/resources/unknownResultSet.baseline @@ -515,3 +515,30 @@ result: unknown { exprs: 3 exprs: 6 } + + +Source: type(x.single_int32) +declare x { + value google.api.expr.test.v1.proto3.TestAllTypes +} +declare f { + function f int.(int) -> bool +} +=====> +bindings: {} +result: unknown { + exprs: 2 +} + + +Source: type(1 / 0 > 2) +declare x { + value google.api.expr.test.v1.proto3.TestAllTypes +} +declare f { + function f int.(int) -> bool +} +=====> +bindings: {} +error: evaluation error: / by zero +error_code: DIVIDE_BY_ZERO diff --git a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java index 68f343a19..17c3bb809 100644 --- a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java +++ b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java @@ -1298,6 +1298,14 @@ public void unknownResultSet() { // message with multiple unknowns => unknownSet source = "TestAllTypes{single_int32: x.single_int32, single_int64: x.single_int64}"; runTest(); + + // type(unknown) -> unknown + source = "type(x.single_int32)"; + runTest(); + + // type(error) -> error + source = "type(1 / 0 > 2)"; + runTest(); } @Test From 9b9d0d1559787fc7e2f63f609813904b3c857873 Mon Sep 17 00:00:00 2001 From: Justin King Date: Thu, 17 Oct 2024 12:13:53 -0700 Subject: [PATCH 08/63] Use an immutable copy of `DefaultDispatcher` to avoid synchronization PiperOrigin-RevId: 687000721 --- .../src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java b/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java index f5f96e5ca..81beec7cc 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java +++ b/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java @@ -266,7 +266,9 @@ public CelRuntimeLegacyImpl build() { } return new CelRuntimeLegacyImpl( - new DefaultInterpreter(runtimeTypeProvider, dispatcher, options), options, this); + new DefaultInterpreter(runtimeTypeProvider, dispatcher.immutableCopy(), options), + options, + this); } private static CelDescriptorPool newDescriptorPool( From ec4fa39bc7c6136dc29fdbb1f688a5759e05fa61 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Fri, 18 Oct 2024 11:11:56 -0700 Subject: [PATCH 09/63] Rollback: Enforce strictness of type function This ensures that: - type(unknown) -> unknown - type(error) -> error PiperOrigin-RevId: 687363881 --- .../java/dev/cel/runtime/CelUnknownSet.java | 2 +- .../dev/cel/runtime/DefaultInterpreter.java | 14 +++------- .../test/resources/unknownResultSet.baseline | 27 ------------------- .../dev/cel/testing/BaseInterpreterTest.java | 8 ------ 4 files changed, 5 insertions(+), 46 deletions(-) diff --git a/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java b/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java index d05be8133..e53d8be94 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java +++ b/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java @@ -46,7 +46,7 @@ public static CelUnknownSet create(ImmutableSet attributes) { return create(attributes, ImmutableSet.of()); } - public static CelUnknownSet create(Long... unknownExprIds) { + static CelUnknownSet create(Long... unknownExprIds) { return create(ImmutableSet.copyOf(unknownExprIds)); } diff --git a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java index a0dd1f242..dc633e1a9 100644 --- a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java +++ b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java @@ -222,14 +222,10 @@ private IntermediateResult evalInternal(ExecutionFrame frame, CelExpr expr) } } - private static boolean isUnknownValue(Object value) { + private boolean isUnknownValue(Object value) { return value instanceof CelUnknownSet || InterpreterUtil.isUnknown(value); } - private static boolean isUnknownOrError(Object value) { - return isUnknownValue(value) || value instanceof Exception; - } - private Object evalConstant( ExecutionFrame unusedFrame, CelExpr unusedExpr, CelConstant constExpr) { switch (constExpr.getKind()) { @@ -597,10 +593,6 @@ private IntermediateResult evalType(ExecutionFrame frame, CelCall callExpr) throws InterpreterException { CelExpr typeExprArg = callExpr.args().get(0); IntermediateResult argResult = evalInternal(frame, typeExprArg); - // Type is a strict function. Early return if the argument is an error or an unknown. - if (isUnknownOrError(argResult.value())) { - return argResult; - } CelType checkedType = ast.getType(typeExprArg.id()) @@ -690,7 +682,9 @@ private IntermediateResult evalBoolean(ExecutionFrame frame, CelExpr expr, boole throws InterpreterException { IntermediateResult value = strict ? evalInternal(frame, expr) : evalNonstrictly(frame, expr); - if (!(value.value() instanceof Boolean) && !isUnknownOrError(value.value())) { + if (!(value.value() instanceof Boolean) + && !isUnknownValue(value.value()) + && !(value.value() instanceof Exception)) { throw new InterpreterException.Builder("expected boolean value, found: %s", value.value()) .setErrorCode(CelErrorCode.INVALID_ARGUMENT) .setLocation(metadata, expr.id()) diff --git a/runtime/src/test/resources/unknownResultSet.baseline b/runtime/src/test/resources/unknownResultSet.baseline index 7cc661627..2b2c61f62 100644 --- a/runtime/src/test/resources/unknownResultSet.baseline +++ b/runtime/src/test/resources/unknownResultSet.baseline @@ -515,30 +515,3 @@ result: unknown { exprs: 3 exprs: 6 } - - -Source: type(x.single_int32) -declare x { - value google.api.expr.test.v1.proto3.TestAllTypes -} -declare f { - function f int.(int) -> bool -} -=====> -bindings: {} -result: unknown { - exprs: 2 -} - - -Source: type(1 / 0 > 2) -declare x { - value google.api.expr.test.v1.proto3.TestAllTypes -} -declare f { - function f int.(int) -> bool -} -=====> -bindings: {} -error: evaluation error: / by zero -error_code: DIVIDE_BY_ZERO diff --git a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java index 17c3bb809..68f343a19 100644 --- a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java +++ b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java @@ -1298,14 +1298,6 @@ public void unknownResultSet() { // message with multiple unknowns => unknownSet source = "TestAllTypes{single_int32: x.single_int32, single_int64: x.single_int64}"; runTest(); - - // type(unknown) -> unknown - source = "type(x.single_int32)"; - runTest(); - - // type(error) -> error - source = "type(1 / 0 > 2)"; - runTest(); } @Test From 0a9ae34b243a9f9eb18ef65ad7ca271261fdc883 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Fri, 18 Oct 2024 15:56:20 -0700 Subject: [PATCH 10/63] Update visibility for CelUnknownSet::create PiperOrigin-RevId: 687449942 --- runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java b/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java index e53d8be94..d05be8133 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java +++ b/runtime/src/main/java/dev/cel/runtime/CelUnknownSet.java @@ -46,7 +46,7 @@ public static CelUnknownSet create(ImmutableSet attributes) { return create(attributes, ImmutableSet.of()); } - static CelUnknownSet create(Long... unknownExprIds) { + public static CelUnknownSet create(Long... unknownExprIds) { return create(ImmutableSet.copyOf(unknownExprIds)); } From a32d6e35a85516582bc0192c0ed1da99b87f3d2b Mon Sep 17 00:00:00 2001 From: Ori Dagan Date: Sun, 20 Oct 2024 11:45:42 +0300 Subject: [PATCH 11/63] fixed parsing a macro that has an arg call expression which is receiver style --- parser/src/main/java/dev/cel/parser/Parser.java | 1 + .../java/dev/cel/parser/CelUnparserImplTest.java | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/parser/src/main/java/dev/cel/parser/Parser.java b/parser/src/main/java/dev/cel/parser/Parser.java index 3ec9350ed..61caff017 100644 --- a/parser/src/main/java/dev/cel/parser/Parser.java +++ b/parser/src/main/java/dev/cel/parser/Parser.java @@ -613,6 +613,7 @@ private CelExpr buildMacroCallArgs(CelExpr expr) { // means that the depth check on the AST during parsing will catch recursion overflows // before we get to here. expr.call().args().forEach(arg -> callExpr.addArgs(buildMacroCallArgs(arg))); + expr.call().target().ifPresent(target -> callExpr.setTarget(buildMacroCallArgs(target))); return resultExpr.setCall(callExpr.build()).build(); } return expr; diff --git a/parser/src/test/java/dev/cel/parser/CelUnparserImplTest.java b/parser/src/test/java/dev/cel/parser/CelUnparserImplTest.java index be95fcaef..7f6029cad 100644 --- a/parser/src/test/java/dev/cel/parser/CelUnparserImplTest.java +++ b/parser/src/test/java/dev/cel/parser/CelUnparserImplTest.java @@ -273,4 +273,16 @@ public void unparse_comprehensionWithoutMacroCallTracking_throwsException() thro "Comprehension unparsing requires macro calls to be populated. Ensure the option is" + " enabled."); } + + @Test + public void unparse_macroWithReceiverStyleArg() throws Exception { + CelParser parser = + CelParserImpl.newBuilder() + .setOptions(CelOptions.newBuilder().populateMacroCalls(true).build()) + .setStandardMacros(CelStandardMacro.STANDARD_MACROS) + .build(); + CelAbstractSyntaxTree ast = parser.parse("[\"a\"].all(x, x.trim().lowerAscii().contains(\"b\"))").getAst(); + + assertThat(unparser.unparse(ast)).isEqualTo("[\"a\"].all(x, x.trim().lowerAscii().contains(\"b\"))"); + } } From 167957b59f417697958ab05fd667f58831eb17a5 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Tue, 22 Oct 2024 18:49:21 -0700 Subject: [PATCH 12/63] Rollback: Enforce strictness of type function This ensures that: - type(unknown) -> unknown - type(error) -> error PiperOrigin-RevId: 688765902 --- .../dev/cel/runtime/DefaultInterpreter.java | 14 +++++++--- .../test/resources/unknownResultSet.baseline | 27 +++++++++++++++++++ .../dev/cel/testing/BaseInterpreterTest.java | 8 ++++++ 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java index dc633e1a9..a0dd1f242 100644 --- a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java +++ b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java @@ -222,10 +222,14 @@ private IntermediateResult evalInternal(ExecutionFrame frame, CelExpr expr) } } - private boolean isUnknownValue(Object value) { + private static boolean isUnknownValue(Object value) { return value instanceof CelUnknownSet || InterpreterUtil.isUnknown(value); } + private static boolean isUnknownOrError(Object value) { + return isUnknownValue(value) || value instanceof Exception; + } + private Object evalConstant( ExecutionFrame unusedFrame, CelExpr unusedExpr, CelConstant constExpr) { switch (constExpr.getKind()) { @@ -593,6 +597,10 @@ private IntermediateResult evalType(ExecutionFrame frame, CelCall callExpr) throws InterpreterException { CelExpr typeExprArg = callExpr.args().get(0); IntermediateResult argResult = evalInternal(frame, typeExprArg); + // Type is a strict function. Early return if the argument is an error or an unknown. + if (isUnknownOrError(argResult.value())) { + return argResult; + } CelType checkedType = ast.getType(typeExprArg.id()) @@ -682,9 +690,7 @@ private IntermediateResult evalBoolean(ExecutionFrame frame, CelExpr expr, boole throws InterpreterException { IntermediateResult value = strict ? evalInternal(frame, expr) : evalNonstrictly(frame, expr); - if (!(value.value() instanceof Boolean) - && !isUnknownValue(value.value()) - && !(value.value() instanceof Exception)) { + if (!(value.value() instanceof Boolean) && !isUnknownOrError(value.value())) { throw new InterpreterException.Builder("expected boolean value, found: %s", value.value()) .setErrorCode(CelErrorCode.INVALID_ARGUMENT) .setLocation(metadata, expr.id()) diff --git a/runtime/src/test/resources/unknownResultSet.baseline b/runtime/src/test/resources/unknownResultSet.baseline index 2b2c61f62..7cc661627 100644 --- a/runtime/src/test/resources/unknownResultSet.baseline +++ b/runtime/src/test/resources/unknownResultSet.baseline @@ -515,3 +515,30 @@ result: unknown { exprs: 3 exprs: 6 } + + +Source: type(x.single_int32) +declare x { + value google.api.expr.test.v1.proto3.TestAllTypes +} +declare f { + function f int.(int) -> bool +} +=====> +bindings: {} +result: unknown { + exprs: 2 +} + + +Source: type(1 / 0 > 2) +declare x { + value google.api.expr.test.v1.proto3.TestAllTypes +} +declare f { + function f int.(int) -> bool +} +=====> +bindings: {} +error: evaluation error: / by zero +error_code: DIVIDE_BY_ZERO diff --git a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java index 68f343a19..17c3bb809 100644 --- a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java +++ b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java @@ -1298,6 +1298,14 @@ public void unknownResultSet() { // message with multiple unknowns => unknownSet source = "TestAllTypes{single_int32: x.single_int32, single_int64: x.single_int64}"; runTest(); + + // type(unknown) -> unknown + source = "type(x.single_int32)"; + runTest(); + + // type(error) -> error + source = "type(1 / 0 > 2)"; + runTest(); } @Test From fccefae7c57bd5895dcbc374c1e70c78a0be8750 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Thu, 24 Oct 2024 14:03:14 -0700 Subject: [PATCH 13/63] Fix feature_request.md config blocks It appears that the second config block was left on accident. --- .github/ISSUE_TEMPLATE/feature_request.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md index 67062e439..59a173b91 100644 --- a/.github/ISSUE_TEMPLATE/feature_request.md +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -6,12 +6,6 @@ labels: '' --- ---- -name: Feature request -about: Suggest an idea for this project - ---- - **Feature request checklist** - [ ] There are no issues that match the desired change From 45641a8790acb6ace9044b57695c1a202ecf99e9 Mon Sep 17 00:00:00 2001 From: Tristan Swadell Date: Thu, 24 Oct 2024 15:42:17 -0700 Subject: [PATCH 14/63] Sync with GitHub PiperOrigin-RevId: 689541392 --- .../test/java/dev/cel/conformance/BUILD.bazel | 4 +-- .../dev/cel/conformance/ConformanceTest.java | 27 +++++++------------ 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/conformance/src/test/java/dev/cel/conformance/BUILD.bazel b/conformance/src/test/java/dev/cel/conformance/BUILD.bazel index 57a83b955..e32d18382 100644 --- a/conformance/src/test/java/dev/cel/conformance/BUILD.bazel +++ b/conformance/src/test/java/dev/cel/conformance/BUILD.bazel @@ -30,10 +30,10 @@ java_library( "//parser:macro", "//parser:parser_builder", "//runtime", + "//third_party/cel/spec/proto/cel/expr/conformance/proto2:test_all_types_java_proto", + "//third_party/cel/spec/proto/cel/expr/conformance/proto3:test_all_types_java_proto", "@cel_spec//proto/cel/expr:expr_java_proto", "@cel_spec//proto/test/v1:simple_java_proto", - "@cel_spec//proto/test/v1/proto2:test_all_types_java_proto", - "@cel_spec//proto/test/v1/proto3:test_all_types_java_proto", "@com_google_googleapis//google/api/expr/v1alpha1:expr_java_proto", "@maven//:com_google_guava_guava", "@maven//:com_google_protobuf_protobuf_java", diff --git a/conformance/src/test/java/dev/cel/conformance/ConformanceTest.java b/conformance/src/test/java/dev/cel/conformance/ConformanceTest.java index 32c10bf4d..f1d3c1e75 100644 --- a/conformance/src/test/java/dev/cel/conformance/ConformanceTest.java +++ b/conformance/src/test/java/dev/cel/conformance/ConformanceTest.java @@ -19,7 +19,6 @@ import dev.cel.expr.Decl; import com.google.api.expr.test.v1.SimpleProto.SimpleTest; -import com.google.api.expr.test.v1.proto2.TestAllTypesExtensions; import com.google.api.expr.v1alpha1.ExprValue; import com.google.api.expr.v1alpha1.ListValue; import com.google.api.expr.v1alpha1.MapValue; @@ -69,8 +68,7 @@ public final class ConformanceTest extends Statement { private static ExtensionRegistry newDefaultExtensionRegistry() { ExtensionRegistry extensionRegistry = ExtensionRegistry.newInstance(); - com.google.api.expr.test.v1.proto2.TestAllTypesExtensions.registerAllExtensions( - extensionRegistry); + dev.cel.expr.conformance.proto2.TestAllTypesExtensions.registerAllExtensions(extensionRegistry); return extensionRegistry; } @@ -79,12 +77,9 @@ private static TypeRegistry newDefaultTypeRegistry() { CelDescriptors allDescriptors = CelDescriptorUtil.getAllDescriptorsFromFileDescriptor( ImmutableList.of( - com.google.api.expr.test.v1.proto2.TestAllTypesProto.TestAllTypes.getDescriptor() - .getFile(), - com.google.api.expr.test.v1.proto3.TestAllTypesProto.TestAllTypes.getDescriptor() - .getFile(), - com.google.api.expr.test.v1.proto2.TestAllTypesExtensions.getDescriptor() - .getFile())); + dev.cel.expr.conformance.proto2.TestAllTypes.getDescriptor().getFile(), + dev.cel.expr.conformance.proto3.TestAllTypes.getDescriptor().getFile(), + dev.cel.expr.conformance.proto2.TestAllTypesExtensions.getDescriptor().getFile())); return TypeRegistry.newBuilder().add(allDescriptors.messageTypeDescriptors()).build(); } @@ -140,7 +135,7 @@ private static CelChecker getChecker(SimpleTest test) throws Exception { .setOptions(OPTIONS) .setContainer(test.getContainer()) .addDeclarations(decls.build()) - .addFileTypes(TestAllTypesExtensions.getDescriptor()) + .addFileTypes(dev.cel.expr.conformance.proto2.TestAllTypesExtensions.getDescriptor()) .addLibraries( CelExtensions.bindings(), CelExtensions.encoders(), @@ -148,10 +143,8 @@ private static CelChecker getChecker(SimpleTest test) throws Exception { CelExtensions.sets(), CelExtensions.strings(), CelOptionalLibrary.INSTANCE) - .addMessageTypes( - com.google.api.expr.test.v1.proto2.TestAllTypesProto.TestAllTypes.getDescriptor()) - .addMessageTypes( - com.google.api.expr.test.v1.proto3.TestAllTypesProto.TestAllTypes.getDescriptor()) + .addMessageTypes(dev.cel.expr.conformance.proto2.TestAllTypes.getDescriptor()) + .addMessageTypes(dev.cel.expr.conformance.proto3.TestAllTypes.getDescriptor()) .build(); } @@ -164,10 +157,8 @@ private static CelChecker getChecker(SimpleTest test) throws Exception { CelExtensions.sets(), CelExtensions.strings(), CelOptionalLibrary.INSTANCE) - .addMessageTypes( - com.google.api.expr.test.v1.proto2.TestAllTypesProto.TestAllTypes.getDescriptor()) - .addMessageTypes( - com.google.api.expr.test.v1.proto3.TestAllTypesProto.TestAllTypes.getDescriptor()) + .addMessageTypes(dev.cel.expr.conformance.proto2.TestAllTypes.getDescriptor()) + .addMessageTypes(dev.cel.expr.conformance.proto3.TestAllTypes.getDescriptor()) .build(); private static ImmutableMap getBindings(SimpleTest test) throws Exception { From f96dd0b6d550b3f11e1eada917155f6ca78a9713 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Fri, 25 Oct 2024 06:01:21 -0700 Subject: [PATCH 15/63] Internal Changes PiperOrigin-RevId: 689760093 --- checker/src/test/java/dev/cel/checker/BUILD.bazel | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checker/src/test/java/dev/cel/checker/BUILD.bazel b/checker/src/test/java/dev/cel/checker/BUILD.bazel index e3e410429..4bb8544d5 100644 --- a/checker/src/test/java/dev/cel/checker/BUILD.bazel +++ b/checker/src/test/java/dev/cel/checker/BUILD.bazel @@ -11,7 +11,6 @@ java_library( srcs = glob(["*Test.java"]), resources = ["//checker/src/test/resources:baselines"], deps = [ - "@maven//:com_google_protobuf_protobuf_java", # "//java/com/google/testing/testsize:annotations", "//:auto_value", "//checker", @@ -50,6 +49,7 @@ java_library( "@cel_spec//proto/test/v1/proto3:test_all_types_java_proto", "@com_google_googleapis//google/rpc/context:attribute_context_java_proto", "@maven//:com_google_guava_guava", + "@maven//:com_google_protobuf_protobuf_java", ], ) From 3cb263a4f0a501bc39bb9fab1ae89ecde61c3086 Mon Sep 17 00:00:00 2001 From: Tristan Swadell Date: Mon, 11 Nov 2024 16:25:22 -0800 Subject: [PATCH 16/63] Remove the legacy createInterpretable(CheckedExpr) PiperOrigin-RevId: 695519680 --- runtime/src/main/java/dev/cel/runtime/BUILD.bazel | 2 -- .../main/java/dev/cel/runtime/DefaultInterpreter.java | 8 -------- .../src/main/java/dev/cel/runtime/Interpreter.java | 11 ----------- 3 files changed, 21 deletions(-) diff --git a/runtime/src/main/java/dev/cel/runtime/BUILD.bazel b/runtime/src/main/java/dev/cel/runtime/BUILD.bazel index 095f69565..c3fe0074c 100644 --- a/runtime/src/main/java/dev/cel/runtime/BUILD.bazel +++ b/runtime/src/main/java/dev/cel/runtime/BUILD.bazel @@ -45,7 +45,6 @@ java_library( ], deps = [ ":runtime_helper", - "//:auto_value", "//common", "//common:error_codes", "//common:options", @@ -85,7 +84,6 @@ java_library( "//common:error_codes", "//common:features", "//common:options", - "//common:proto_ast", "//common:runtime_exception", "//common/annotations", "//common/ast", diff --git a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java index a0dd1f242..36f07c4b5 100644 --- a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java +++ b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java @@ -14,7 +14,6 @@ package dev.cel.runtime; -import dev.cel.expr.CheckedExpr; import dev.cel.expr.Value; import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; @@ -24,7 +23,6 @@ import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.CelErrorCode; import dev.cel.common.CelOptions; -import dev.cel.common.CelProtoAbstractSyntaxTree; import dev.cel.common.annotations.Internal; import dev.cel.common.ast.CelConstant; import dev.cel.common.ast.CelExpr; @@ -122,12 +120,6 @@ public DefaultInterpreter( this.celOptions = celOptions; } - @Override - @Deprecated - public Interpretable createInterpretable(CheckedExpr checkedExpr) { - return createInterpretable(CelProtoAbstractSyntaxTree.fromCheckedExpr(checkedExpr).getAst()); - } - @Override public Interpretable createInterpretable(CelAbstractSyntaxTree ast) { return new DefaultInterpretable(typeProvider, dispatcher, ast, celOptions); diff --git a/runtime/src/main/java/dev/cel/runtime/Interpreter.java b/runtime/src/main/java/dev/cel/runtime/Interpreter.java index c6fe08077..5c316da1a 100644 --- a/runtime/src/main/java/dev/cel/runtime/Interpreter.java +++ b/runtime/src/main/java/dev/cel/runtime/Interpreter.java @@ -14,7 +14,6 @@ package dev.cel.runtime; -import dev.cel.expr.CheckedExpr; import javax.annotation.concurrent.ThreadSafe; import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.annotations.Internal; @@ -28,16 +27,6 @@ @Internal public interface Interpreter { - /** - * Creates an interpretable for the given expression. - * - *

This method may run pre-processing and partial evaluation of the expression it gets passed. - * - * @deprecated Use {@link #createInterpretable(CelAbstractSyntaxTree)} instead. - */ - @Deprecated - Interpretable createInterpretable(CheckedExpr checkedExpr) throws InterpreterException; - /** * Creates an interpretable for the given expression. * From 8d2f1009b1e7f826ec6646f3eecf1573f3d4b7ea Mon Sep 17 00:00:00 2001 From: Tristan Swadell Date: Thu, 14 Nov 2024 17:07:35 -0800 Subject: [PATCH 17/63] BUILD dependency bundling PiperOrigin-RevId: 696697160 --- conformance/src/test/java/dev/cel/conformance/BUILD.bazel | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/conformance/src/test/java/dev/cel/conformance/BUILD.bazel b/conformance/src/test/java/dev/cel/conformance/BUILD.bazel index e32d18382..78350e1a0 100644 --- a/conformance/src/test/java/dev/cel/conformance/BUILD.bazel +++ b/conformance/src/test/java/dev/cel/conformance/BUILD.bazel @@ -30,9 +30,9 @@ java_library( "//parser:macro", "//parser:parser_builder", "//runtime", - "//third_party/cel/spec/proto/cel/expr/conformance/proto2:test_all_types_java_proto", - "//third_party/cel/spec/proto/cel/expr/conformance/proto3:test_all_types_java_proto", "@cel_spec//proto/cel/expr:expr_java_proto", + "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto", + "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto", "@cel_spec//proto/test/v1:simple_java_proto", "@com_google_googleapis//google/api/expr/v1alpha1:expr_java_proto", "@maven//:com_google_guava_guava", From 7dce5b4ee33856f1401a86e4c58606dcd4486562 Mon Sep 17 00:00:00 2001 From: Tristan Swadell Date: Fri, 15 Nov 2024 11:46:22 -0800 Subject: [PATCH 18/63] Introduce late-bound functions into CEL-Java PiperOrigin-RevId: 696954942 --- .../dev/cel/extensions/CelExtensionsTest.java | 4 +- .../src/main/java/dev/cel/runtime/BUILD.bazel | 8 + .../cel/runtime/CelEvaluationException.java | 4 + .../dev/cel/runtime/CelFunctionOverload.java | 28 +-- .../dev/cel/runtime/CelFunctionResolver.java | 24 +++ .../cel/runtime/CelLateFunctionBindings.java | 70 +++++++ .../dev/cel/runtime/CelResolvedOverload.java | 56 ++++++ .../main/java/dev/cel/runtime/CelRuntime.java | 55 +++++- .../dev/cel/runtime/DefaultDispatcher.java | 177 ++++++++---------- .../dev/cel/runtime/DefaultInterpreter.java | 85 ++++++++- .../main/java/dev/cel/runtime/Dispatcher.java | 20 +- .../dev/cel/runtime/FunctionOverload.java | 47 +++++ .../dev/cel/runtime/FunctionResolver.java | 45 +++++ .../java/dev/cel/runtime/Interpretable.java | 19 ++ .../dev/cel/runtime/InterpreterException.java | 28 ++- .../main/java/dev/cel/runtime/Registrar.java | 27 +-- .../dev/cel/runtime/ResolvedOverload.java | 64 +++++++ .../runtime/UnknownTrackingInterpretable.java | 15 +- .../src/test/java/dev/cel/runtime/BUILD.bazel | 1 + .../runtime/CelLateFunctionBindingsTest.java | 146 +++++++++++++++ .../cel/runtime/CelResolvedOverloadTest.java | 67 +++++++ .../java/dev/cel/runtime/CelRuntimeTest.java | 2 +- .../cel/runtime/DefaultDispatcherTest.java | 60 ++++++ .../resources/lateBoundFunctions.baseline | 7 + .../src/main/java/dev/cel/testing/BUILD.bazel | 1 + .../dev/cel/testing/BaseInterpreterTest.java | 66 ++++++- 26 files changed, 930 insertions(+), 196 deletions(-) create mode 100644 runtime/src/main/java/dev/cel/runtime/CelFunctionResolver.java create mode 100644 runtime/src/main/java/dev/cel/runtime/CelLateFunctionBindings.java create mode 100644 runtime/src/main/java/dev/cel/runtime/CelResolvedOverload.java create mode 100644 runtime/src/main/java/dev/cel/runtime/FunctionOverload.java create mode 100644 runtime/src/main/java/dev/cel/runtime/FunctionResolver.java create mode 100644 runtime/src/main/java/dev/cel/runtime/ResolvedOverload.java create mode 100644 runtime/src/test/java/dev/cel/runtime/CelLateFunctionBindingsTest.java create mode 100644 runtime/src/test/java/dev/cel/runtime/CelResolvedOverloadTest.java create mode 100644 runtime/src/test/java/dev/cel/runtime/DefaultDispatcherTest.java create mode 100644 runtime/src/test/resources/lateBoundFunctions.baseline diff --git a/extensions/src/test/java/dev/cel/extensions/CelExtensionsTest.java b/extensions/src/test/java/dev/cel/extensions/CelExtensionsTest.java index 26c51b1f4..80fe77f98 100644 --- a/extensions/src/test/java/dev/cel/extensions/CelExtensionsTest.java +++ b/extensions/src/test/java/dev/cel/extensions/CelExtensionsTest.java @@ -89,7 +89,9 @@ public void addStringExtensionsForCompilerOnly_throwsEvaluationException() throw assertThat(exception) .hasMessageThat() - .contains("Unknown overload id 'string_substring_int_int' for function 'substring'"); + .contains( + "No matching overload for function 'substring'. Overload candidates:" + + " string_substring_int_int"); } @Test diff --git a/runtime/src/main/java/dev/cel/runtime/BUILD.bazel b/runtime/src/main/java/dev/cel/runtime/BUILD.bazel index c3fe0074c..dd9abfd28 100644 --- a/runtime/src/main/java/dev/cel/runtime/BUILD.bazel +++ b/runtime/src/main/java/dev/cel/runtime/BUILD.bazel @@ -10,10 +10,13 @@ package( BASE_SOURCES = [ "DefaultMetadata.java", + "FunctionResolver.java", + "FunctionOverload.java", "InterpreterException.java", "MessageProvider.java", "Metadata.java", "Registrar.java", + "ResolvedOverload.java", "StandardFunctions.java", "StandardTypeResolver.java", "TypeResolver.java", @@ -45,6 +48,7 @@ java_library( ], deps = [ ":runtime_helper", + "//:auto_value", "//common", "//common:error_codes", "//common:options", @@ -58,6 +62,7 @@ java_library( "//common/types", "//common/types:type_providers", "@cel_spec//proto/cel/expr:expr_java_proto", + "@maven//:com_google_code_findbugs_annotations", "@maven//:com_google_errorprone_error_prone_annotations", "@maven//:com_google_guava_guava", "@maven//:com_google_protobuf_protobuf_java", @@ -132,6 +137,9 @@ java_library( RUNTIME_SOURCES = [ "CelEvaluationException.java", "CelFunctionOverload.java", + "CelFunctionResolver.java", + "CelLateFunctionBindings.java", + "CelResolvedOverload.java", "CelRuntime.java", "CelRuntimeBuilder.java", "CelRuntimeFactory.java", diff --git a/runtime/src/main/java/dev/cel/runtime/CelEvaluationException.java b/runtime/src/main/java/dev/cel/runtime/CelEvaluationException.java index 313077ecf..a4ca1dd65 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelEvaluationException.java +++ b/runtime/src/main/java/dev/cel/runtime/CelEvaluationException.java @@ -31,6 +31,10 @@ public CelEvaluationException(String message, Throwable cause) { super(message, cause); } + public CelEvaluationException(String message, Throwable cause, CelErrorCode errorCode) { + super(message, cause, errorCode); + } + CelEvaluationException(InterpreterException cause) { super(cause.getMessage(), cause.getCause()); } diff --git a/runtime/src/main/java/dev/cel/runtime/CelFunctionOverload.java b/runtime/src/main/java/dev/cel/runtime/CelFunctionOverload.java index 2ea1cb529..a098f7835 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelFunctionOverload.java +++ b/runtime/src/main/java/dev/cel/runtime/CelFunctionOverload.java @@ -1,4 +1,4 @@ -// Copyright 2022 Google LLC +// Copyright 2024 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -19,28 +19,4 @@ /** Interface describing the general signature of all CEL custom function implementations. */ @Immutable @FunctionalInterface -public interface CelFunctionOverload { - - /** Evaluate a set of arguments throwing a {@code CelEvaluationException} on error. */ - Object apply(Object[] args) throws CelEvaluationException; - - /** - * Helper interface for describing unary functions where the type-parameter is used to improve - * compile-time correctness of function bindings. - */ - @Immutable - @FunctionalInterface - interface Unary { - Object apply(T arg) throws CelEvaluationException; - } - - /** - * Helper interface for describing binary functions where the type parameters are used to improve - * compile-time correctness of function bindings. - */ - @Immutable - @FunctionalInterface - interface Binary { - Object apply(T1 arg1, T2 arg2) throws CelEvaluationException; - } -} +public interface CelFunctionOverload extends FunctionOverload {} diff --git a/runtime/src/main/java/dev/cel/runtime/CelFunctionResolver.java b/runtime/src/main/java/dev/cel/runtime/CelFunctionResolver.java new file mode 100644 index 000000000..7571a8803 --- /dev/null +++ b/runtime/src/main/java/dev/cel/runtime/CelFunctionResolver.java @@ -0,0 +1,24 @@ +// Copyright 202 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package dev.cel.runtime; + +import javax.annotation.concurrent.ThreadSafe; + +/** + * Interface to a resolver for CEL functions based on the function name, overload ids, and + * arguments. + */ +@ThreadSafe +public interface CelFunctionResolver extends FunctionResolver {} diff --git a/runtime/src/main/java/dev/cel/runtime/CelLateFunctionBindings.java b/runtime/src/main/java/dev/cel/runtime/CelLateFunctionBindings.java new file mode 100644 index 000000000..0fea43b0d --- /dev/null +++ b/runtime/src/main/java/dev/cel/runtime/CelLateFunctionBindings.java @@ -0,0 +1,70 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package dev.cel.runtime; + +import static com.google.common.collect.ImmutableMap.toImmutableMap; + +import com.google.common.collect.ImmutableMap; +import com.google.errorprone.annotations.Immutable; +import dev.cel.common.CelException; +import java.util.Arrays; +import java.util.List; +import java.util.Optional; + +/** + * Collection of {@link CelFunctionBinding} values which are intended to be created once + * per-evaluation, rather than once per-program setup. + */ +@Immutable +public final class CelLateFunctionBindings implements CelFunctionResolver { + + private final ImmutableMap functions; + + private CelLateFunctionBindings(ImmutableMap functions) { + this.functions = functions; + } + + @Override + public Optional findOverload( + String functionName, List overloadIds, Object[] args) throws CelException { + return DefaultDispatcher.findOverload(functionName, overloadIds, functions, args); + } + + public static CelLateFunctionBindings from(CelRuntime.CelFunctionBinding... functions) { + return from(Arrays.asList(functions)); + } + + public static CelLateFunctionBindings from(List functions) { + return new CelLateFunctionBindings( + functions.stream() + .collect( + toImmutableMap( + CelRuntime.CelFunctionBinding::getOverloadId, + CelLateFunctionBindings::createResolvedOverload))); + } + + private static ResolvedOverload createResolvedOverload(CelRuntime.CelFunctionBinding binding) { + return CelResolvedOverload.of( + binding.getOverloadId(), + binding.getArgTypes(), + (args) -> { + try { + return binding.getDefinition().apply(args); + } catch (CelException e) { + throw InterpreterException.wrapOrThrow(e); + } + }); + } +} diff --git a/runtime/src/main/java/dev/cel/runtime/CelResolvedOverload.java b/runtime/src/main/java/dev/cel/runtime/CelResolvedOverload.java new file mode 100644 index 000000000..e23749f15 --- /dev/null +++ b/runtime/src/main/java/dev/cel/runtime/CelResolvedOverload.java @@ -0,0 +1,56 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package dev.cel.runtime; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.annotations.Immutable; + +/** + * Representation of a function overload which has been resolved to a specific set of argument types + * and a function definition. + */ +@AutoValue +@Immutable +public abstract class CelResolvedOverload implements ResolvedOverload { + + /** The overload id of the function. */ + @Override + public abstract String getOverloadId(); + + /** The types of the function parameters. */ + @Override + public abstract ImmutableList> getParameterTypes(); + + /** The function definition. */ + @Override + public abstract CelFunctionOverload getDefinition(); + + /** + * Creates a new resolved overload from the given overload id, parameter types, and definition. + */ + public static CelResolvedOverload of( + String overloadId, Class[] parameterTypes, CelFunctionOverload definition) { + return of(overloadId, ImmutableList.copyOf(parameterTypes), definition); + } + + /** + * Creates a new resolved overload from the given overload id, parameter types, and definition. + */ + public static CelResolvedOverload of( + String overloadId, ImmutableList> parameterTypes, CelFunctionOverload definition) { + return new AutoValue_CelResolvedOverload(overloadId, parameterTypes, definition); + } +} diff --git a/runtime/src/main/java/dev/cel/runtime/CelRuntime.java b/runtime/src/main/java/dev/cel/runtime/CelRuntime.java index 2b28d015b..854b064fa 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelRuntime.java +++ b/runtime/src/main/java/dev/cel/runtime/CelRuntime.java @@ -25,6 +25,7 @@ import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.CelOptions; import java.util.Map; +import java.util.Optional; /** * The CelRuntime creates executable {@code Program} instances from {@code CelAbstractSyntaxTree} @@ -64,6 +65,18 @@ public Object eval(CelVariableResolver resolver) throws CelEvaluationException { return evalInternal((name) -> resolver.find(name).orElse(null)); } + /** + * Evaluate a compiled program with a custom variable {@code resolver} and late-bound functions + * {@code lateBoundFunctionResolver}. + */ + public Object eval(CelVariableResolver resolver, CelFunctionResolver lateBoundFunctionResolver) + throws CelEvaluationException { + return evalInternal( + (name) -> resolver.find(name).orElse(null), + lateBoundFunctionResolver, + CelEvaluationListener.noOpListener()); + } + /** * Trace evaluates a compiled program without any variables and invokes the listener as * evaluation progresses through the AST. @@ -99,6 +112,20 @@ public Object trace(CelVariableResolver resolver, CelEvaluationListener listener return evalInternal((name) -> resolver.find(name).orElse(null), listener); } + /** + * Trace evaluates a compiled program using a custom variable {@code resolver} and late-bound + * functions {@code lateBoundFunctionResolver}. The listener is invoked as evaluation progresses + * through the AST. + */ + public Object trace( + CelVariableResolver resolver, + CelFunctionResolver lateBoundFunctionResolver, + CelEvaluationListener listener) + throws CelEvaluationException { + return evalInternal( + (name) -> resolver.find(name).orElse(null), lateBoundFunctionResolver, listener); + } + /** * Advance evaluation based on the current unknown context. * @@ -109,23 +136,36 @@ public Object trace(CelVariableResolver resolver, CelEvaluationListener listener * UnknownTracking} is disabled, this is equivalent to eval. */ public Object advanceEvaluation(UnknownContext context) throws CelEvaluationException { - return evalInternal(context, CelEvaluationListener.noOpListener()); + return evalInternal(context, Optional.empty(), CelEvaluationListener.noOpListener()); } private Object evalInternal(GlobalResolver resolver) throws CelEvaluationException { - return evalInternal(resolver, CelEvaluationListener.noOpListener()); + return evalInternal( + UnknownContext.create(resolver), Optional.empty(), CelEvaluationListener.noOpListener()); } private Object evalInternal(GlobalResolver resolver, CelEvaluationListener listener) throws CelEvaluationException { - return evalInternal(UnknownContext.create(resolver), listener); + return evalInternal(UnknownContext.create(resolver), Optional.empty(), listener); + } + + private Object evalInternal( + GlobalResolver resolver, + CelFunctionResolver lateBoundFunctionResolver, + CelEvaluationListener listener) + throws CelEvaluationException { + return evalInternal( + UnknownContext.create(resolver), Optional.of(lateBoundFunctionResolver), listener); } /** * Evaluate an expr node with an UnknownContext (an activation annotated with which attributes * are unknown). */ - private Object evalInternal(UnknownContext context, CelEvaluationListener listener) + private Object evalInternal( + UnknownContext context, + Optional lateBoundFunctionResolver, + CelEvaluationListener listener) throws CelEvaluationException { try { Interpretable impl = getInterpretable(); @@ -141,8 +181,12 @@ private Object evalInternal(UnknownContext context, CelEvaluationListener listen .setResolver(context.variableResolver()) .setAttributeResolver(context.createAttributeResolver()) .build(), + lateBoundFunctionResolver, listener); } else { + if (lateBoundFunctionResolver.isPresent()) { + return impl.eval(context.variableResolver(), lateBoundFunctionResolver.get(), listener); + } return impl.eval(context.variableResolver(), listener); } } catch (InterpreterException e) { @@ -177,8 +221,7 @@ private static CelEvaluationException unwrapOrCreateEvaluationException( * *

While the CEL function has a human-readable {@code camelCase} name, overload ids should use * the following convention where all {@code } names should be ASCII lower-cased. For types - * prefer the unparameterized simple name of time, or unqualified package name of any proto-based - * type: + * prefer the unparameterized simple name of time, or unqualified name of any proto-based type: * *