From 4dfaf39f7fdcd37ef5f4aef554142d571a7a07b2 Mon Sep 17 00:00:00 2001 From: tswadell Date: Fri, 9 Oct 2020 13:59:09 -0400 Subject: [PATCH 01/19] Enable timestamp tests and add support for fixed timezones. PiperOrigin-RevId: 336325540 --- Dockerfile | 15 -------- cloudbuild.yaml | 2 +- conformance/BUILD | 19 ++++++---- conformance/server.cc | 10 +++--- eval/eval/attribute_trail.cc | 2 +- eval/eval/attribute_utility.cc | 2 +- eval/eval/evaluator_core.cc | 2 +- eval/eval/evaluator_core.h | 2 +- eval/eval/function_step.cc | 2 +- eval/eval/function_step.h | 2 +- eval/public/builtin_func_registrar.cc | 31 ++++++++++++---- eval/public/builtin_func_test.cc | 52 ++++++++++++++++++++++++++- eval/public/cel_function_adapter.h | 2 +- eval/public/cel_value.h | 2 +- eval/tests/BUILD | 4 +-- parser/parser.h | 2 +- protoutil/type_registry.h | 2 +- 17 files changed, 107 insertions(+), 46 deletions(-) delete mode 100644 Dockerfile diff --git a/Dockerfile b/Dockerfile deleted file mode 100644 index 2561f3a82..000000000 --- a/Dockerfile +++ /dev/null @@ -1,15 +0,0 @@ -FROM ubuntu:bionic - -ENV DEBIAN_FRONTEND=noninteractive - -RUN rm -rf /var/lib/apt/lists/* \ - && apt-get update --fix-missing -qq \ - && apt-get install -qqy --no-install-recommends ca-certificates tzdata wget git clang-10 patch \ - && apt-get clean && rm -rf /var/lib/apt/lists/* - -RUN wget https://github.com/bazelbuild/bazelisk/releases/download/v1.5.0/bazelisk-linux-amd64 && chmod +x bazelisk-linux-amd64 && mv bazelisk-linux-amd64 /bin/bazel - -ENV CC=clang-10 -ENV CXX=clang++-10 - -ENTRYPOINT ["/bin/bazel"] diff --git a/cloudbuild.yaml b/cloudbuild.yaml index 5845145b7..de8dee01f 100644 --- a/cloudbuild.yaml +++ b/cloudbuild.yaml @@ -1,5 +1,5 @@ steps: -- name: 'gcr.io/cel-analysis/bazel:bionic-3.0.0' +- name: 'gcr.io/cel-analysis/bazel:3.0.0' entrypoint: bazel args: ['test', '--test_output=errors', '...'] id: bazel-test diff --git a/conformance/BUILD b/conformance/BUILD index 24db6ce08..20e051eb4 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -21,6 +21,7 @@ ALL_TESTS = [ "@com_google_cel_spec//tests/simple:testdata/namespace.textproto", "@com_google_cel_spec//tests/simple:testdata/plumbing.textproto", "@com_google_cel_spec//tests/simple:testdata/string.textproto", + "@com_google_cel_spec//tests/simple:testdata/timestamps.textproto", "@com_google_cel_spec//tests/simple:testdata/unknowns.textproto", ] @@ -39,6 +40,7 @@ DASHBOARD_TESTS = [ "@com_google_cel_spec//tests/simple:testdata/namespace.textproto", "@com_google_cel_spec//tests/simple:testdata/plumbing.textproto", "@com_google_cel_spec//tests/simple:testdata/string.textproto", + "@com_google_cel_spec//tests/simple:testdata/timestamps.textproto", "@com_google_cel_spec//tests/simple:testdata/unknowns.textproto", ] @@ -52,13 +54,13 @@ cc_binary( "//eval/public:transform_utility", "//eval/public/containers:container_backed_list_impl", "//eval/public/containers:container_backed_map_impl", + "//google/api/expr/v1alpha1:conformance_service_proto_grpc_cc", "//internal:proto_util", + "//net/grpc:grpc++", "//parser", - "@com_github_grpc_grpc//:grpc++", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_googleapis//google/api/expr/v1alpha1:checked_cc_proto", - "@com_google_googleapis//google/api/expr/v1alpha1:conformance_service_cc_grpc", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", "@com_google_googleapis//google/rpc:code_cc_proto", "@com_google_protobuf//:protobuf", @@ -72,7 +74,12 @@ cc_binary( args = [ "$(location @com_google_cel_spec//tests/simple:simple_test)", "--server=$(location :server)", - "--check_server=$(location @com_google_cel_go//server/main:cel_server)", + "--check_server=$(location //third_party/cel/go/server/main:cel_server)", + # Missing proto-based test support + "--skip_test=timestamps/duration_converters/get_milliseconds", + # Missing timestamp conversion functions (type / string) + "--skip_test=timestamps/timestamp_conversions/toType_timestamp,toString_timestamp", + "--skip_test=timestamps/duration_conversions/toType_duration,toString_duration", # TODO(issues/78): Missing bytes() conversion functions "--skip_test=conversions/bytes", # TODO(issues/79): Missing double() conversion functions @@ -111,7 +118,7 @@ cc_binary( ] + ["$(location " + test + ")" for test in ALL_TESTS], data = [ ":server", - "@com_google_cel_go//server/main:cel_server", + "//third_party/cel/go/server/main:cel_server", "@com_google_cel_spec//tests/simple:simple_test", ] + ALL_TESTS, ) @@ -127,11 +134,11 @@ sh_test( args = [ "$(location @com_google_cel_spec//tests/simple:simple_test)", "--server=$(location :server)", - "--check_server=$(location @com_google_cel_go//server/main:cel_server)", + "--check_server=$(location //third_party/cel/go/server/main:cel_server)", ] + ["$(location " + test + ")" for test in DASHBOARD_TESTS], data = [ ":server", - "@com_google_cel_go//server/main:cel_server", + "//third_party/cel/go/server/main:cel_server", "@com_google_cel_spec//tests/simple:simple_test", ] + DASHBOARD_TESTS, visibility = [ diff --git a/conformance/server.cc b/conformance/server.cc index cd3e22aee..91bb95be9 100644 --- a/conformance/server.cc +++ b/conformance/server.cc @@ -8,7 +8,7 @@ #include "google/protobuf/struct.pb.h" #include "google/protobuf/timestamp.pb.h" #include "google/rpc/code.pb.h" -#include "grpcpp/grpcpp.h" +#include "net/grpc/public/include/grpcpp/grpcpp.h" #include "absl/strings/str_split.h" #include "eval/public/builtin_func_registrar.h" #include "eval/public/cel_expr_builder_factory.h" @@ -17,7 +17,7 @@ #include "eval/public/transform_utility.h" #include "internal/proto_util.h" #include "parser/parser.h" -#include "absl/status/statusor.h" +#include "util/task/statusor.h" using ::grpc::Status; @@ -30,7 +30,7 @@ namespace expr { namespace runtime { class ConformanceServiceImpl final - : public v1alpha1::ConformanceService::Service { + : public v1alpha1::grpc_gen::ConformanceService::Service { public: ConformanceServiceImpl(std::unique_ptr builder) : builder_(std::move(builder)) {} @@ -145,7 +145,7 @@ int RunServer(std::string server_address) { grpc::InsecureServerCredentials(), &port); grpc_builder.RegisterService(&service); std::unique_ptr server(grpc_builder.BuildAndStart()); - std::cout << "Listening on 127.0.0.1:" << port << std::endl; + std::cout << "Listening on [::1]:" << port << std::endl; fflush(stdout); server->Wait(); return 0; @@ -157,6 +157,6 @@ int RunServer(std::string server_address) { } // namespace google int main(int argc, char** argv) { - std::string server_address = "127.0.0.1:0"; + std::string server_address = "[::1]:0"; return google::api::expr::runtime::RunServer(server_address); } diff --git a/eval/eval/attribute_trail.cc b/eval/eval/attribute_trail.cc index b019f83d2..9a3972112 100644 --- a/eval/eval/attribute_trail.cc +++ b/eval/eval/attribute_trail.cc @@ -2,7 +2,7 @@ #include "absl/status/status.h" #include "eval/public/cel_value.h" -#include "absl/status/statusor.h" +#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/attribute_utility.cc b/eval/eval/attribute_utility.cc index 70fea5398..a40acb29c 100644 --- a/eval/eval/attribute_utility.cc +++ b/eval/eval/attribute_utility.cc @@ -4,7 +4,7 @@ #include "eval/public/cel_value.h" #include "eval/public/unknown_attribute_set.h" #include "eval/public/unknown_set.h" -#include "absl/status/statusor.h" +#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index 45ccfc9eb..1d66a19a5 100644 --- a/eval/eval/evaluator_core.cc +++ b/eval/eval/evaluator_core.cc @@ -6,7 +6,7 @@ #include "eval/eval/attribute_trail.h" #include "eval/public/cel_value.h" #include "base/status_macros.h" -#include "absl/status/statusor.h" +#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index fa37733df..4e092823a 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -24,7 +24,7 @@ #include "eval/public/cel_expression.h" #include "eval/public/cel_value.h" #include "eval/public/unknown_attribute_set.h" -#include "absl/status/statusor.h" +#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/function_step.cc b/eval/eval/function_step.cc index 35a2f47e2..174771537 100644 --- a/eval/eval/function_step.cc +++ b/eval/eval/function_step.cc @@ -28,7 +28,7 @@ #include "eval/public/unknown_function_result_set.h" #include "eval/public/unknown_set.h" #include "base/status_macros.h" -#include "absl/status/statusor.h" +#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/function_step.h b/eval/eval/function_step.h index 5d02fc15c..20f1d84e3 100644 --- a/eval/eval/function_step.h +++ b/eval/eval/function_step.h @@ -12,7 +12,7 @@ #include "eval/public/cel_function.h" #include "eval/public/cel_function_registry.h" #include "eval/public/cel_value.h" -#include "absl/status/statusor.h" +#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc index 8b8becb14..cca501a49 100644 --- a/eval/public/builtin_func_registrar.cc +++ b/eval/public/builtin_func_registrar.cc @@ -8,6 +8,7 @@ #include "absl/strings/match.h" #include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" +#include "absl/strings/str_replace.h" #include "eval/public/cel_builtins.h" #include "eval/public/cel_function_adapter.h" #include "eval/public/cel_function_registry.h" @@ -491,15 +492,33 @@ const absl::Status FindTimeBreakdown(absl::Time timestamp, absl::string_view tz, absl::TimeZone::CivilInfo* breakdown) { absl::TimeZone time_zone; - if (!tz.empty()) { - bool found = absl::LoadTimeZone(std::string(tz), &time_zone); - if (!found) { - return absl::InvalidArgumentError("Invalid timezone"); + // Early return if there is no timezone. + if (tz.empty()) { + *breakdown = time_zone.At(timestamp); + return absl::OkStatus(); + } + + // Check to see whether the timezone is an IANA timezone. + if (absl::LoadTimeZone(std::string(tz), &time_zone)) { + *breakdown = time_zone.At(timestamp); + return absl::OkStatus(); + } + + // Check for times of the format: [+-]HH:MM and convert them into durations + // specified as [+-]HHhMMm. + if (absl::StrContains(tz, ":")) { + std::string dur = absl::StrCat(tz, "m"); + absl::StrReplaceAll({{":", "h"}}, &dur); + absl::Duration d; + if (absl::ParseDuration(dur, &d)) { + timestamp += d; + *breakdown = time_zone.At(timestamp); + return absl::OkStatus(); } } - *breakdown = time_zone.At(timestamp); - return absl::OkStatus(); + // Otherwise, error. + return absl::InvalidArgumentError("Invalid timezone"); } CelValue GetTimeBreakdownPart( diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index b4b27400d..98ae6b183 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -524,7 +524,7 @@ TEST_F(BuiltinsTest, TestTimestampFunctions) { TestFunctions(builtin::kDayOfWeek, CelProtoWrapper::CreateTimestamp(&ref), 0L); - // Test timestamp functions w/ timezone + // Test timestamp functions w/ IANA timezone ref.set_seconds(1L); ref.set_nanos(11000000L); std::vector params; @@ -560,6 +560,56 @@ TEST_F(BuiltinsTest, TestTimestampFunctions) { ref.set_seconds(-1L); ref.set_nanos(0L); + TestFunctions(builtin::kFullYear, CelProtoWrapper::CreateTimestamp(&ref), + 1969L); + TestFunctions(builtin::kMonth, CelProtoWrapper::CreateTimestamp(&ref), 11L); + TestFunctions(builtin::kDayOfYear, CelProtoWrapper::CreateTimestamp(&ref), + 364L); + TestFunctions(builtin::kDayOfMonth, CelProtoWrapper::CreateTimestamp(&ref), + 30L); + TestFunctions(builtin::kDate, CelProtoWrapper::CreateTimestamp(&ref), 31L); + TestFunctions(builtin::kHours, CelProtoWrapper::CreateTimestamp(&ref), 23L); + TestFunctions(builtin::kMinutes, CelProtoWrapper::CreateTimestamp(&ref), 59L); + TestFunctions(builtin::kSeconds, CelProtoWrapper::CreateTimestamp(&ref), 59L); + TestFunctions(builtin::kDayOfWeek, CelProtoWrapper::CreateTimestamp(&ref), + 3L); + + // Test timestamp functions w/ fixed timezone + ref.set_seconds(1L); + ref.set_nanos(11000000L); + const std::string fixedzone = "-08:00"; + params.clear(); + params.push_back(CelValue::CreateString(&fixedzone)); + + TestFunctionsWithParams(builtin::kFullYear, + CelProtoWrapper::CreateTimestamp(&ref), params, + 1969L); + TestFunctionsWithParams(builtin::kMonth, + CelProtoWrapper::CreateTimestamp(&ref), params, 11L); + TestFunctionsWithParams(builtin::kDayOfYear, + CelProtoWrapper::CreateTimestamp(&ref), params, 364L); + TestFunctionsWithParams(builtin::kDayOfMonth, + CelProtoWrapper::CreateTimestamp(&ref), params, 30L); + TestFunctionsWithParams(builtin::kDate, + CelProtoWrapper::CreateTimestamp(&ref), params, 31L); + TestFunctionsWithParams(builtin::kHours, + CelProtoWrapper::CreateTimestamp(&ref), params, 16L); + TestFunctionsWithParams(builtin::kMinutes, + CelProtoWrapper::CreateTimestamp(&ref), params, 0L); + TestFunctionsWithParams(builtin::kSeconds, + CelProtoWrapper::CreateTimestamp(&ref), params, 1L); + TestFunctionsWithParams(builtin::kMilliseconds, + CelProtoWrapper::CreateTimestamp(&ref), params, 11L); + + ref.set_seconds(259200L); + ref.set_nanos(0L); + TestFunctionsWithParams(builtin::kDayOfWeek, + CelProtoWrapper::CreateTimestamp(&ref), params, 6L); + + // Test timestamp functions with negative value + ref.set_seconds(-1L); + ref.set_nanos(0L); + TestFunctions(builtin::kFullYear, CelProtoWrapper::CreateTimestamp(&ref), 1969L); TestFunctions(builtin::kMonth, CelProtoWrapper::CreateTimestamp(&ref), 11L); diff --git a/eval/public/cel_function_adapter.h b/eval/public/cel_function_adapter.h index 74ab5848b..8a6d3062a 100644 --- a/eval/public/cel_function_adapter.h +++ b/eval/public/cel_function_adapter.h @@ -7,7 +7,7 @@ #include "eval/public/cel_function.h" #include "eval/public/cel_function_registry.h" #include "eval/public/structs/cel_proto_wrapper.h" -#include "absl/status/statusor.h" +#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/public/cel_value.h b/eval/public/cel_value.h index 0c7057e7d..333aafb49 100644 --- a/eval/public/cel_value.h +++ b/eval/public/cel_value.h @@ -25,7 +25,7 @@ #include "absl/time/time.h" #include "absl/types/optional.h" #include "eval/public/cel_value_internal.h" -#include "absl/status/statusor.h" +#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/tests/BUILD b/eval/tests/BUILD index 2753cecb8..2c45cba95 100644 --- a/eval/tests/BUILD +++ b/eval/tests/BUILD @@ -24,8 +24,8 @@ cc_test( "//eval/public:cel_value", "//eval/public/containers:container_backed_list_impl", "//eval/public/structs:cel_proto_wrapper", - "@com_github_google_googlebench//:benchmark", - "@com_github_google_googlebench//:benchmark_main", + "@com_github_google_benchmark//:benchmark", + "@com_github_google_benchmark//:benchmark_main", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/container:node_hash_set", "@com_google_absl//absl/strings", diff --git a/parser/parser.h b/parser/parser.h index 57ad7de8a..0128d9b8b 100644 --- a/parser/parser.h +++ b/parser/parser.h @@ -5,7 +5,7 @@ #include "absl/types/optional.h" #include "parser/macro.h" #include "parser/source_factory.h" -#include "absl/status/statusor.h" +#include "util/task/statusor.h" namespace google { namespace api { diff --git a/protoutil/type_registry.h b/protoutil/type_registry.h index 158ff78bf..9b5a96da4 100644 --- a/protoutil/type_registry.h +++ b/protoutil/type_registry.h @@ -131,7 +131,7 @@ class TypeRegistry { * callable it is. * * @tparam ProtoType the concrete protobuf message class. - * @tparam C the constructor type. Templatized to allow inline of lambda + * @tparam C the constructor type. Templitzed to allow inline of lambda * invocation. * @returns false if a constructor was previously registered. */ From 85a8fe309e10a031a2b002e594238fb8f9804acb Mon Sep 17 00:00:00 2001 From: kmilsht Date: Fri, 9 Oct 2020 17:03:01 -0400 Subject: [PATCH 02/19] Add support for CelType to FunctionAdapter. Add type() builtin function implementation to CEL C++ evaluator. PiperOrigin-RevId: 336360964 --- eval/public/builtin_func_registrar.cc | 15 ++++++++ eval/public/builtin_func_test.cc | 55 +++++++++++++++++++++++++++ eval/public/cel_builtins.h | 1 + eval/public/cel_function_adapter.h | 6 +++ 4 files changed, 77 insertions(+) diff --git a/eval/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc index cca501a49..1eca3b8ba 100644 --- a/eval/public/builtin_func_registrar.cc +++ b/eval/public/builtin_func_registrar.cc @@ -253,6 +253,9 @@ CelValue Equal(Arena* arena, CelValue t1, CelValue t2) { return Equal(arena, t1.ListOrDie(), t2.ListOrDie()); case CelValue::Type::kMap: return Equal(arena, t1.MapOrDie(), t2.MapOrDie()); + case CelValue::Type::kCelType: + return Equal(arena, t1.CelTypeOrDie(), + t2.CelTypeOrDie()); default: break; } @@ -707,6 +710,9 @@ absl::Status RegisterComparisonFunctions(CelFunctionRegistry* registry, status = RegisterEqualityFunctionsForType(registry); if (!status.ok()) return status; + status = RegisterEqualityFunctionsForType(registry); + if (!status.ok()) return status; + return absl::OkStatus(); } @@ -1452,6 +1458,15 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, if (!status.ok()) return status; } + status = + FunctionAdapter::CreateAndRegister( + builtin::kType, false, + [](Arena*, CelValue value) -> CelValue::CelTypeHolder { + return value.ObtainCelType().CelTypeOrDie(); + }, + registry); + if (!status.ok()) return status; + return absl::OkStatus(); } diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index 98ae6b183..25caf3c13 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -1526,6 +1526,61 @@ TEST_F(BuiltinsTest, StringToString) { EXPECT_EQ(result_value.StringOrDie().value(), "abcd"); } +// Type operations +TEST_F(BuiltinsTest, TypeComparisons) { + ::google::protobuf::Arena arena; + + std::vector> paired_values; + + paired_values.push_back( + {CelValue::CreateBool(false), CelValue::CreateBool(true)}); + paired_values.push_back( + {CelValue::CreateInt64(-1), CelValue::CreateInt64(1)}); + paired_values.push_back( + {CelValue::CreateUint64(1), CelValue::CreateUint64(2)}); + paired_values.push_back( + {CelValue::CreateDouble(1.), CelValue::CreateDouble(2.)}); + + std::string str1 = "test1"; + std::string str2 = "test2"; + paired_values.push_back( + {CelValue::CreateString(&str1), CelValue::CreateString(&str2)}); + paired_values.push_back( + {CelValue::CreateBytes(&str1), CelValue::CreateBytes(&str2)}); + + FakeList cel_list1({CelValue::CreateBool(false)}); + FakeList cel_list2({CelValue::CreateBool(true)}); + paired_values.push_back( + {CelValue::CreateList(&cel_list1), CelValue::CreateList(&cel_list2)}); + + std::map data1; + std::map data2; + FakeInt64Map cel_map1(data1); + FakeInt64Map cel_map2(data2); + paired_values.push_back( + {CelValue::CreateMap(&cel_map1), CelValue::CreateMap(&cel_map2)}); + + for (int i = 0; i < paired_values.size(); i++) { + for (int j = 0; j < paired_values.size(); j++) { + CelValue result1; + CelValue result2; + + PerformRun(builtin::kType, {}, {paired_values[i].first}, &result1); + PerformRun(builtin::kType, {}, {paired_values[j].second}, &result2); + + ASSERT_TRUE(result1.IsCelType()) << "Unexpected result for value at index" + << i << ":" << result1.DebugString(); + ASSERT_TRUE(result2.IsCelType()) << "Unexpected result for value at index" + << j << ":" << result2.DebugString(); + if (i == j) { + ASSERT_EQ(result1.CelTypeOrDie(), result2.CelTypeOrDie()); + } else { + ASSERT_TRUE(result1.CelTypeOrDie() != result2.CelTypeOrDie()); + } + } + } +} + } // namespace } // namespace runtime diff --git a/eval/public/cel_builtins.h b/eval/public/cel_builtins.h index 259c801f7..bff10e819 100644 --- a/eval/public/cel_builtins.h +++ b/eval/public/cel_builtins.h @@ -73,6 +73,7 @@ constexpr char kMilliseconds[] = "getMilliseconds"; // TODO(issues/23): Add other type conversion methods. constexpr char kInt[] = "int"; constexpr char kString[] = "string"; +constexpr char kType[] = "type"; } // namespace builtin diff --git a/eval/public/cel_function_adapter.h b/eval/public/cel_function_adapter.h index 8a6d3062a..dae0f050d 100644 --- a/eval/public/cel_function_adapter.h +++ b/eval/public/cel_function_adapter.h @@ -266,6 +266,12 @@ class FunctionAdapter : public CelFunction { return absl::OkStatus(); } + static absl::Status CreateReturnValue(CelValue::CelTypeHolder value, + ::google::protobuf::Arena*, CelValue* result) { + *result = CelValue::CreateCelType(value); + return absl::OkStatus(); + } + static absl::Status CreateReturnValue(const CelError* value, ::google::protobuf::Arena*, CelValue* result) { if (value == nullptr) { From 5877e2331c45304af7ae1bb1a9499758739e3316 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Mon, 12 Oct 2020 15:45:12 -0400 Subject: [PATCH 03/19] Makes CEL parser tolerate trailing commas in `CreateMessage` and `CreateStruct` constructs. PiperOrigin-RevId: 336722378 --- parser/Cel.g4 | 4 ++-- parser/parser_test.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/parser/Cel.g4 b/parser/Cel.g4 index 62d8cf8d0..6034d652b 100644 --- a/parser/Cel.g4 +++ b/parser/Cel.g4 @@ -44,14 +44,14 @@ member : primary # PrimaryExpr | member op='.' id=IDENTIFIER (open='(' args=exprList? ')')? # SelectOrCall | member op='[' index=expr ']' # Index - | member op='{' entries=fieldInitializerList? '}' # CreateMessage + | member op='{' entries=fieldInitializerList? ','? '}' # CreateMessage ; primary : leadingDot='.'? id=IDENTIFIER (op='(' args=exprList? ')')? # IdentOrGlobalCall | '(' e=expr ')' # Nested | op='[' elems=exprList? ','? ']' # CreateList - | op='{' entries=mapInitializerList? '}' # CreateStruct + | op='{' entries=mapInitializerList? ','? '}' # CreateStruct | literal # ConstantLiteral ; diff --git a/parser/parser_test.cc b/parser/parser_test.cc index 1e0b35d4b..62fb88861 100644 --- a/parser/parser_test.cc +++ b/parser/parser_test.cc @@ -124,7 +124,7 @@ std::vector test_cases = { {"{", "", "ERROR: :1:2: Syntax error: mismatched input '' expecting " "{'[', " - "'{', '}', '(', '.', '-', '!', 'true', 'false', 'null', NUM_FLOAT, " + "'{', '}', '(', '.', ',', '-', '!', 'true', 'false', 'null', NUM_FLOAT, " "NUM_INT, " "NUM_UINT, STRING, BYTES, IDENTIFIER}\n | {\n" " | .^"}, @@ -399,7 +399,7 @@ std::vector test_cases = { "ERROR: :4294967295:0: <> parsetree\n | \n | ^"}, {"t{>C}", "", "ERROR: :1:3: Syntax error: extraneous input '>' expecting {'}', " - "IDENTIFIER}\n | t{>C}\n | ..^\nERROR: :1:5: Syntax error: " + "',', IDENTIFIER}\n | t{>C}\n | ..^\nERROR: :1:5: Syntax error: " "mismatched input '}' expecting ':'\n | t{>C}\n | ....^"}, // Macro tests From 187e72291fb1b9ddc76a513abad62dd33ba6a9ce Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Mon, 12 Oct 2020 19:49:16 -0400 Subject: [PATCH 04/19] Internal change. PiperOrigin-RevId: 336772611 --- conformance/BUILD | 1 - conformance/server.cc | 1 - parser/parser.h | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index 20e051eb4..f50d46057 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -58,7 +58,6 @@ cc_binary( "//internal:proto_util", "//net/grpc:grpc++", "//parser", - "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_googleapis//google/api/expr/v1alpha1:checked_cc_proto", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", diff --git a/conformance/server.cc b/conformance/server.cc index 91bb95be9..808dae4f6 100644 --- a/conformance/server.cc +++ b/conformance/server.cc @@ -17,7 +17,6 @@ #include "eval/public/transform_utility.h" #include "internal/proto_util.h" #include "parser/parser.h" -#include "util/task/statusor.h" using ::grpc::Status; diff --git a/parser/parser.h b/parser/parser.h index 0128d9b8b..46bb561d1 100644 --- a/parser/parser.h +++ b/parser/parser.h @@ -2,10 +2,10 @@ #define THIRD_PARTY_CEL_CPP_PARSER_PARSER_H_ #include "google/api/expr/v1alpha1/syntax.pb.h" +#include "absl/status/statusor.h" #include "absl/types/optional.h" #include "parser/macro.h" #include "parser/source_factory.h" -#include "util/task/statusor.h" namespace google { namespace api { From 682123732881489c207804d35105e73a172323c9 Mon Sep 17 00:00:00 2001 From: tswadell Date: Wed, 14 Oct 2020 12:52:25 -0400 Subject: [PATCH 05/19] Fix signed and unsigned integer overflow cases within C++ evaluator. PiperOrigin-RevId: 337110846 --- conformance/BUILD | 1 - eval/public/BUILD | 2 + eval/public/builtin_func_registrar.cc | 127 ++++++++++++++++++++++---- eval/public/builtin_func_test.cc | 89 ++++++++++++++---- 4 files changed, 185 insertions(+), 34 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index f50d46057..502fac165 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -107,7 +107,6 @@ cc_binary( "--skip_test=fields/in/singleton", # Requires qualified bindings error message relaxation "--skip_test=fields/qualified_identifier_resolution/ident_with_longest_prefix_check,int64_field_select_unsupported,list_field_select_unsupported,map_key_null,qualified_identifier_resolution_unchecked", - "--skip_test=integer_math/int64_math/int64_overflow_positive,int64_overflow_negative,uint64_overflow_positive,uint64_overflow_negative", "--skip_test=string/size/one_unicode,unicode", "--skip_test=string/bytes_concat/left_unit", # TODO(issues/85): The exists one macro should not short-circuit false. diff --git a/eval/public/BUILD b/eval/public/BUILD index 747aa9fe3..2e4d42eca 100644 --- a/eval/public/BUILD +++ b/eval/public/BUILD @@ -182,6 +182,7 @@ cc_library( ":cel_function_registry", ":cel_options", "//eval/public/containers:container_backed_list_impl", + "@com_google_absl//absl/numeric:int128", "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_protobuf//:protobuf", @@ -464,6 +465,7 @@ cc_test( ":cel_function_registry", "//base:status_macros", "//eval/public/structs:cel_proto_wrapper", + "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", "@com_google_googletest//:gtest_main", diff --git a/eval/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc index 1eca3b8ba..473d398d2 100644 --- a/eval/public/builtin_func_registrar.cc +++ b/eval/public/builtin_func_registrar.cc @@ -4,6 +4,7 @@ #include #include "google/protobuf/util/time_util.h" +#include "absl/numeric/int128.h" #include "absl/status/status.h" #include "absl/strings/match.h" #include "absl/strings/numbers.h" @@ -24,6 +25,10 @@ using google::protobuf::Arena; namespace { +const int64_t kIntMax = std::numeric_limits::max(); +const int64_t kIntMin = std::numeric_limits::min(); +const uint64_t kUintMax = std::numeric_limits::max(); + // Comparison template functions template CelValue Inequal(Arena*, Type t1, Type t2) { @@ -310,18 +315,91 @@ absl::Status RegisterComparisonFunctionsForType(CelFunctionRegistry* registry) { // Template functions providing arithmetic operations template -Type Add(Arena*, Type v0, Type v1) { - return v0 + v1; +CelValue Add(Arena*, Type v0, Type v1); + +template <> +CelValue Add(Arena* arena, int64_t v0, int64_t v1) { + absl::int128 bv = v0; + bv += v1; + if (bv < kIntMin || bv > kIntMax) { + return CreateErrorValue(arena, "integer overflow", + absl::StatusCode::kOutOfRange); + } + return CelValue::CreateInt64(absl::Int128Low64(bv)); +} + +template <> +CelValue Add(Arena* arena, uint64_t v0, uint64_t v1) { + absl::uint128 bv = v0; + bv += v1; + if (bv > kUintMax) { + return CreateErrorValue(arena, "unsigned integer overflow", + absl::StatusCode::kOutOfRange); + } + return CelValue::CreateUint64(absl::Uint128Low64(bv)); +} + +template <> +CelValue Add(Arena*, double v0, double v1) { + return CelValue::CreateDouble(v0 + v1); } template -Type Sub(Arena*, Type v0, Type v1) { - return v0 - v1; +CelValue Sub(Arena*, Type v0, Type v1); + +template <> +CelValue Sub(Arena* arena, int64_t v0, int64_t v1) { + absl::int128 bv = v0; + bv -= v1; + if (bv < kIntMin || bv > kIntMax) { + return CreateErrorValue(arena, "integer overflow", + absl::StatusCode::kOutOfRange); + } + return CelValue::CreateInt64(absl::Int128Low64(bv)); +} + +template <> +CelValue Sub(Arena* arena, uint64_t v0, uint64_t v1) { + if (v1 > v0) { + return CreateErrorValue(arena, "unsigned integer overflow", + absl::StatusCode::kOutOfRange); + } + return CelValue::CreateUint64(v0 - v1); +} + +template <> +CelValue Sub(Arena*, double v0, double v1) { + return CelValue::CreateDouble(v0 - v1); } template -Type Mul(Arena*, Type v0, Type v1) { - return v0 * v1; +CelValue Mul(Arena*, Type v0, Type v1); + +template <> +CelValue Mul(Arena* arena, int64_t v0, int64_t v1) { + absl::int128 bv = v0; + bv *= v1; + if (bv < kIntMin || bv > kIntMax) { + return CreateErrorValue(arena, "integer overflow", + absl::StatusCode::kOutOfRange); + } + return CelValue::CreateInt64(absl::Int128Low64(bv)); +} + +template <> +CelValue Mul(Arena* arena, uint64_t v0, uint64_t v1) { + absl::uint128 bv = v0; + bv *= v1; + if (bv > kUintMax) { + return CreateErrorValue(arena, "unsigned integer overflow", + absl::StatusCode::kOutOfRange); + } + return CelValue::CreateUint64(absl::Uint128Low64(bv)); +} + +template <> +CelValue Mul(Arena*, double v0, double v1) { + return CelValue::CreateDouble(v0 * v1); } template @@ -337,6 +415,11 @@ CelValue Div(Arena* arena, int64_t v0, int64_t v1) { // TODO(issues/25) Which code? return CreateErrorValue(arena, "Division by 0"); } + // Overflow case for two's complement: -INT_MIN/-1 + if (v0 == kIntMin && v1 == -1) { + return CreateErrorValue(arena, "integer overflow", + absl::StatusCode::kOutOfRange); + } return CelValue::CreateInt64(v0 / v1); } @@ -370,7 +453,11 @@ CelValue Modulo(Arena* arena, int64_t value, int64_t value2) { if (value2 == 0) { return CreateErrorValue(arena, "Modulo by 0"); } - + // Handle the two's complement case. + if (value == kIntMin && value2 == -1) { + return CreateErrorValue(arena, "integer overflow", + absl::StatusCode::kOutOfRange); + } return CelValue::CreateInt64(value % value2); } @@ -387,15 +474,16 @@ CelValue Modulo(Arena* arena, uint64_t value, uint64_t value2) { // Registers all arithmetic functions for template parameter type. template absl::Status RegisterArithmeticFunctionsForType(CelFunctionRegistry* registry) { - absl::Status status = FunctionAdapter::CreateAndRegister( - builtin::kAdd, false, Add, registry); + absl::Status status = + FunctionAdapter::CreateAndRegister( + builtin::kAdd, false, Add, registry); if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( + status = FunctionAdapter::CreateAndRegister( builtin::kSubtract, false, Sub, registry); if (!status.ok()) return status; - status = FunctionAdapter::CreateAndRegister( + status = FunctionAdapter::CreateAndRegister( builtin::kMultiply, false, Mul, registry); if (!status.ok()) return status; @@ -941,8 +1029,16 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, if (!status.ok()) return status; // Negation group - status = FunctionAdapter::CreateAndRegister( - builtin::kNeg, false, [](Arena*, int64_t value) -> int64_t { return -value; }, + status = FunctionAdapter::CreateAndRegister( + builtin::kNeg, false, + [](Arena* arena, int64_t value) -> CelValue { + // Handle overflow from two's complement. + if (value == kIntMin) { + return CreateErrorValue(arena, "integer overflow", + absl::StatusCode::kOutOfRange); + } + return CelValue::CreateInt64(-value); + }, registry); if (!status.ok()) return status; @@ -1330,8 +1426,7 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, status = FunctionAdapter::CreateAndRegister( builtin::kInt, false, [](Arena* arena, double v) { - if ((v > (double)std::numeric_limits::max()) || - (v < (double)std::numeric_limits::min())) { + if ((v > (double)kIntMax) || (v < (double)kIntMin)) { return CreateErrorValue(arena, "double out of int range", absl::StatusCode::kInvalidArgument); } @@ -1347,7 +1442,7 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, status = FunctionAdapter::CreateAndRegister( builtin::kInt, false, [](Arena* arena, uint64_t v) { - if (v > (uint64_t)std::numeric_limits::max()) { + if (v > (uint64_t)kIntMax) { return CreateErrorValue(arena, "uint out of int range", absl::StatusCode::kInvalidArgument); } diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index 25caf3c13..6d3bdd652 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -2,6 +2,7 @@ #include "google/protobuf/util/time_util.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "eval/public/activation.h" #include "eval/public/builtin_func_registrar.h" @@ -221,6 +222,17 @@ class BuiltinsTest : public ::testing::Test { ASSERT_EQ(result_value.Int64OrDie(), result) << operation; } + // Helper for testing invalid signed integer arithmetic operations. + void TestArithmeticalErrorInt64(absl::string_view operation, int64_t v1, + int64_t v2, absl::StatusCode code) { + CelValue result_value; + ASSERT_NO_FATAL_FAILURE(PerformRun( + operation, {}, {CelValue::CreateInt64(v1), CelValue::CreateInt64(v2)}, + &result_value)); + ASSERT_EQ(result_value.IsError(), true); + ASSERT_EQ(result_value.ErrorOrDie()->code(), code) << operation; + } + // Helper method to test arithmetical operations for Uint64 void TestArithmeticalOperationUint64(absl::string_view operation, uint64_t v1, uint64_t v2, uint64_t result) { @@ -232,6 +244,17 @@ class BuiltinsTest : public ::testing::Test { ASSERT_EQ(result_value.Uint64OrDie(), result) << operation; } + // Helper for testing invalid unsigned integer arithmetic operations. + void TestArithmeticalErrorUint64(absl::string_view operation, uint64_t v1, + uint64_t v2, absl::StatusCode code) { + CelValue result_value; + ASSERT_NO_FATAL_FAILURE(PerformRun( + operation, {}, {CelValue::CreateUint64(v1), CelValue::CreateUint64(v2)}, + &result_value)); + ASSERT_EQ(result_value.IsError(), true); + ASSERT_EQ(result_value.ErrorOrDie()->code(), code) << operation; + } + // Helper method to test arithmetical operations for Double void TestArithmeticalOperationDouble(absl::string_view operation, double v1, double v2, double result) { @@ -331,6 +354,29 @@ TEST_F(BuiltinsTest, TestNotOp) { EXPECT_EQ(result.BoolOrDie(), false); } +// Test negation operation for numeric types. +TEST_F(BuiltinsTest, TestNegOp) { + CelValue result; + ASSERT_NO_FATAL_FAILURE( + PerformRun(builtin::kNeg, {}, {CelValue::CreateInt64(-1)}, &result)); + ASSERT_TRUE(result.IsInt64()); + EXPECT_EQ(result.Int64OrDie(), 1); + + ASSERT_NO_FATAL_FAILURE( + PerformRun(builtin::kNeg, {}, {CelValue::CreateDouble(-1.0)}, &result)); + ASSERT_TRUE(result.IsDouble()); + EXPECT_EQ(result.DoubleOrDie(), 1.0); +} + +// Test integer negation overflow. +TEST_F(BuiltinsTest, TestNegIntOverflow) { + CelValue result; + ASSERT_NO_FATAL_FAILURE(PerformRun( + builtin::kNeg, {}, + {CelValue::CreateInt64(std::numeric_limits::min())}, &result)); + ASSERT_TRUE(result.IsError()); +} + // Test Equality/Non-Equality operation for Bool TEST_F(BuiltinsTest, TestBoolEqual) { CelValue ref = CelValue::CreateBool(true); @@ -1309,32 +1355,41 @@ TEST_F(BuiltinsTest, TestInt64Arithmetics) { TestArithmeticalOperationInt64(builtin::kDivide, 10, 5, 2); } -TEST_F(BuiltinsTest, TestInt64DivisionByZero) { - CelValue result_value; - - ASSERT_NO_FATAL_FAILURE(PerformRun( - builtin::kDivide, {}, - {CelValue::CreateInt64(1), CelValue::CreateInt64(0)}, &result_value)); - - ASSERT_TRUE(result_value.IsError()); +TEST_F(BuiltinsTest, TestInt64ArithmeticOverflow) { + int64_t min = std::numeric_limits::min(); + int64_t max = std::numeric_limits::max(); + TestArithmeticalErrorInt64(builtin::kAdd, max, 1, + absl::StatusCode::kOutOfRange); + TestArithmeticalErrorInt64(builtin::kSubtract, min, max, + absl::StatusCode::kOutOfRange); + TestArithmeticalErrorInt64(builtin::kMultiply, max, 2, + absl::StatusCode::kOutOfRange); + TestArithmeticalErrorInt64(builtin::kModulo, min, -1, + absl::StatusCode::kOutOfRange); + TestArithmeticalErrorInt64(builtin::kDivide, min, -1, + absl::StatusCode::kOutOfRange); + TestArithmeticalErrorInt64(builtin::kDivide, min, 0, + absl::StatusCode::kUnknown); } TEST_F(BuiltinsTest, TestUint64Arithmetics) { TestArithmeticalOperationUint64(builtin::kAdd, 2, 3, 5); - TestArithmeticalOperationUint64(builtin::kSubtract, 2, 3, - static_cast(-1)); + TestArithmeticalOperationUint64(builtin::kSubtract, 3, 2, 1); TestArithmeticalOperationUint64(builtin::kMultiply, 2, 3, 6); TestArithmeticalOperationUint64(builtin::kDivide, 10, 5, 2); } -TEST_F(BuiltinsTest, TestUint64DivisionByZero) { +TEST_F(BuiltinsTest, TestUint64ArithmeticOverflow) { CelValue result_value; - - ASSERT_NO_FATAL_FAILURE(PerformRun( - builtin::kDivide, {}, - {CelValue::CreateUint64(1), CelValue::CreateUint64(0)}, &result_value)); - - ASSERT_TRUE(result_value.IsError()); + uint64_t max = std::numeric_limits::max(); + TestArithmeticalErrorUint64(builtin::kAdd, max, 1, + absl::StatusCode::kOutOfRange); + TestArithmeticalErrorUint64(builtin::kSubtract, 2, 3, + absl::StatusCode::kOutOfRange); + TestArithmeticalErrorUint64(builtin::kMultiply, max, 2, + absl::StatusCode::kOutOfRange); + TestArithmeticalErrorUint64(builtin::kDivide, 1, 0, + absl::StatusCode::kUnknown); } TEST_F(BuiltinsTest, TestDoubleArithmetics) { From 5efd741d8c50948b225d083477ea47f1ac1ae646 Mon Sep 17 00:00:00 2001 From: tswadell Date: Wed, 14 Oct 2020 14:18:15 -0400 Subject: [PATCH 06/19] Enable proto2/proto3 test inputs for C++ Conformance server. PiperOrigin-RevId: 337131059 --- conformance/BUILD | 18 ++++++++++++++++-- conformance/server.cc | 21 ++++++++++++++++++++- eval/eval/create_struct_step.cc | 3 ++- 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index 502fac165..8c5f1c302 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -20,6 +20,8 @@ ALL_TESTS = [ "@com_google_cel_spec//tests/simple:testdata/macros.textproto", "@com_google_cel_spec//tests/simple:testdata/namespace.textproto", "@com_google_cel_spec//tests/simple:testdata/plumbing.textproto", + "@com_google_cel_spec//tests/simple:testdata/proto2.textproto", + "@com_google_cel_spec//tests/simple:testdata/proto3.textproto", "@com_google_cel_spec//tests/simple:testdata/string.textproto", "@com_google_cel_spec//tests/simple:testdata/timestamps.textproto", "@com_google_cel_spec//tests/simple:testdata/unknowns.textproto", @@ -38,6 +40,8 @@ DASHBOARD_TESTS = [ "@com_google_cel_spec//tests/simple:testdata/logic.textproto", "@com_google_cel_spec//tests/simple:testdata/macros.textproto", "@com_google_cel_spec//tests/simple:testdata/namespace.textproto", + "@com_google_cel_spec//tests/simple:testdata/proto2.textproto", + "@com_google_cel_spec//tests/simple:testdata/proto3.textproto", "@com_google_cel_spec//tests/simple:testdata/plumbing.textproto", "@com_google_cel_spec//tests/simple:testdata/string.textproto", "@com_google_cel_spec//tests/simple:testdata/timestamps.textproto", @@ -59,6 +63,8 @@ cc_binary( "//net/grpc:grpc++", "//parser", "@com_google_absl//absl/strings", + "@com_google_cel_spec//proto/test/v1/proto2:test_all_types_cc_proto", + "@com_google_cel_spec//proto/test/v1/proto3:test_all_types_cc_proto", "@com_google_googleapis//google/api/expr/v1alpha1:checked_cc_proto", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", "@com_google_googleapis//google/rpc:code_cc_proto", @@ -74,9 +80,9 @@ cc_binary( "$(location @com_google_cel_spec//tests/simple:simple_test)", "--server=$(location :server)", "--check_server=$(location //third_party/cel/go/server/main:cel_server)", - # Missing proto-based test support + # TODO(issues/93): Inconsistent Duration.getMilliseconds() behavior. "--skip_test=timestamps/duration_converters/get_milliseconds", - # Missing timestamp conversion functions (type / string) + # TODO(issues/94): Missing timestamp conversion functions (type / string) "--skip_test=timestamps/timestamp_conversions/toType_timestamp,toString_timestamp", "--skip_test=timestamps/duration_conversions/toType_duration,toString_duration", # TODO(issues/78): Missing bytes() conversion functions @@ -95,6 +101,14 @@ cc_binary( "--skip_test=conversions/type", # TODO(issues/84): Missing uint() conversion functions "--skip_test=conversions/uint", + # TODO(issues/96): + "--skip_test=proto2/literal_wellknown", + "--skip_test=proto3/literal_wellknown", + "--skip_test=proto2/empty_field/wkt", + "--skip_test=proto3/empty_field/wkt", + # TODO(issues/86): + "--skip_test=proto2/has/undefined,repeated_none_implicit,repeated_none_explicit,repeated_one,repeated_many,map_none_implicit,map_none_explicit,map_one_default,map_one,map_many,optional_enum_set,optional_enum_set_zero,oneof_other_set,oneof_set_default", + "--skip_test=proto3/has/undefined,repeated_none_implicit,repeated_none_explicit,repeated_one,repeated_many,map_none_implicit,map_none_explicit,map_one_default,map_one,map_many,optional_enum_set,optional_enum_set_zero,oneof_other_set,oneof_set_default,single_enum_set,single_enum_set_zero", # Requires container support "--skip_test=namespace/namespace/self_eval_container_lookup_unchecked", "--skip_test=basic/namespace/self_eval_container_lookup,self_eval_container_lookup_unchecked", diff --git a/conformance/server.cc b/conformance/server.cc index 808dae4f6..ef55432fe 100644 --- a/conformance/server.cc +++ b/conformance/server.cc @@ -17,6 +17,8 @@ #include "eval/public/transform_utility.h" #include "internal/proto_util.h" #include "parser/parser.h" +#include "proto/test/v1/proto2/test_all_types.pb.h" +#include "proto/test/v1/proto3/test_all_types.pb.h" using ::grpc::Status; @@ -32,7 +34,12 @@ class ConformanceServiceImpl final : public v1alpha1::grpc_gen::ConformanceService::Service { public: ConformanceServiceImpl(std::unique_ptr builder) - : builder_(std::move(builder)) {} + : builder_(std::move(builder)), + proto2Tests_(&google::api::expr::test::v1::proto2::TestAllTypes:: + default_instance()), + proto3Tests_(&google::api::expr::test::v1::proto3::TestAllTypes:: + default_instance()) {} + Status Parse(grpc::ServerContext* context, const v1alpha1::ParseRequest* request, v1alpha1::ParseResponse* response) override { @@ -51,11 +58,13 @@ class ConformanceServiceImpl final } return Status::OK; } + Status Check(grpc::ServerContext* context, const v1alpha1::CheckRequest* request, v1alpha1::CheckResponse* response) override { return Status(StatusCode::UNIMPLEMENTED, "Check is not supported"); } + Status Eval(grpc::ServerContext* context, const v1alpha1::EvalRequest* request, v1alpha1::EvalResponse* response) override { @@ -117,6 +126,8 @@ class ConformanceServiceImpl final private: std::unique_ptr builder_; + const google::api::expr::test::v1::proto2::TestAllTypes* proto2Tests_; + const google::api::expr::test::v1::proto3::TestAllTypes* proto3Tests_; }; int RunServer(std::string server_address) { @@ -132,6 +143,14 @@ int RunServer(std::string server_address) { std::unique_ptr builder = CreateCelExpressionBuilder(options); + builder->AddResolvableEnum( + google::api::expr::test::v1::proto2::GlobalEnum_descriptor()); + builder->AddResolvableEnum( + google::api::expr::test::v1::proto3::GlobalEnum_descriptor()); + builder->AddResolvableEnum(google::api::expr::test::v1::proto2::TestAllTypes:: + NestedEnum_descriptor()); + builder->AddResolvableEnum(google::api::expr::test::v1::proto3::TestAllTypes:: + NestedEnum_descriptor()); auto register_status = RegisterBuiltinFunctions(builder->GetRegistry()); if (!register_status.ok()) { return 1; diff --git a/eval/eval/create_struct_step.cc b/eval/eval/create_struct_step.cc index 987ec77cf..53b8933d2 100644 --- a/eval/eval/create_struct_step.cc +++ b/eval/eval/create_struct_step.cc @@ -283,7 +283,8 @@ absl::StatusOr> CreateCreateStructStep( if (desc == nullptr) { return absl::InvalidArgumentError( - "Error configuring message creation: message descriptor not found"); + "Error configuring message creation: message descriptor not found: " + + create_struct_expr->message_name()); } for (const auto& entry : create_struct_expr->entries()) { From c20d88ded5b9d06c2cdf5a1859571e33e8d961d1 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Fri, 16 Oct 2020 18:04:43 -0400 Subject: [PATCH 07/19] Internal change. PiperOrigin-RevId: 337585239 --- eval/compiler/flat_expr_builder.h | 1 + eval/eval/BUILD | 2 -- eval/eval/attribute_trail.cc | 1 - eval/eval/attribute_utility.cc | 1 - eval/eval/const_value_step.h | 1 + eval/eval/container_access_step.h | 1 + eval/eval/create_list_step.h | 3 ++- eval/eval/create_struct_step.h | 1 + eval/eval/evaluator_core.cc | 1 - eval/eval/evaluator_core.h | 2 +- eval/eval/function_step.cc | 1 - eval/eval/function_step.h | 2 +- eval/eval/ident_step.h | 1 + eval/eval/jump_step.h | 3 ++- eval/eval/logic_step.h | 3 ++- eval/eval/select_step.h | 1 + eval/eval/ternary_step.h | 3 ++- eval/public/BUILD | 3 ++- eval/public/cel_expression.h | 1 + eval/public/cel_function_adapter.h | 2 +- eval/public/cel_function_provider.h | 1 + eval/public/cel_value.h | 1 - eval/public/transform_utility.cc | 1 + eval/public/transform_utility.h | 1 + eval/tests/BUILD | 1 + eval/tests/mock_cel_expression.h | 1 + 26 files changed, 25 insertions(+), 15 deletions(-) diff --git a/eval/compiler/flat_expr_builder.h b/eval/compiler/flat_expr_builder.h index a9591b857..f68da0d2f 100644 --- a/eval/compiler/flat_expr_builder.h +++ b/eval/compiler/flat_expr_builder.h @@ -2,6 +2,7 @@ #define THIRD_PARTY_CEL_CPP_EVAL_COMPILER_FLAT_EXPR_BUILDER_H_ #include "google/api/expr/v1alpha1/syntax.pb.h" +#include "absl/status/statusor.h" #include "eval/public/cel_expression.h" namespace google { diff --git a/eval/eval/BUILD b/eval/eval/BUILD index 409a52f00..67e5fa18b 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -509,7 +509,6 @@ cc_library( "//eval/public:cel_value", "//eval/public:unknown_attribute_set", "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/types:optional", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", "@com_google_protobuf//:protobuf", @@ -544,7 +543,6 @@ cc_library( "//eval/public:unknown_attribute_set", "//eval/public:unknown_set", "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/types:optional", "@com_google_absl//absl/types:span", "@com_google_protobuf//:protobuf", diff --git a/eval/eval/attribute_trail.cc b/eval/eval/attribute_trail.cc index 9a3972112..d7bfe2fa8 100644 --- a/eval/eval/attribute_trail.cc +++ b/eval/eval/attribute_trail.cc @@ -2,7 +2,6 @@ #include "absl/status/status.h" #include "eval/public/cel_value.h" -#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/attribute_utility.cc b/eval/eval/attribute_utility.cc index a40acb29c..02bc7121c 100644 --- a/eval/eval/attribute_utility.cc +++ b/eval/eval/attribute_utility.cc @@ -4,7 +4,6 @@ #include "eval/public/cel_value.h" #include "eval/public/unknown_attribute_set.h" #include "eval/public/unknown_set.h" -#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/const_value_step.h b/eval/eval/const_value_step.h index 267dea7b9..e3c685cbf 100644 --- a/eval/eval/const_value_step.h +++ b/eval/eval/const_value_step.h @@ -1,6 +1,7 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_CONST_VALUE_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_CONST_VALUE_STEP_H_ +#include "absl/status/statusor.h" #include "eval/eval/evaluator_core.h" #include "eval/public/activation.h" #include "eval/public/cel_value.h" diff --git a/eval/eval/container_access_step.h b/eval/eval/container_access_step.h index 72a285e5c..2fcc8bdbc 100644 --- a/eval/eval/container_access_step.h +++ b/eval/eval/container_access_step.h @@ -1,6 +1,7 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_CONTAINER_ACCESS_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_CONTAINER_ACCESS_STEP_H_ +#include "absl/status/statusor.h" #include "eval/eval/evaluator_core.h" #include "eval/public/activation.h" #include "eval/public/cel_value.h" diff --git a/eval/eval/create_list_step.h b/eval/eval/create_list_step.h index 11bd38eb3..fdd51fdcd 100644 --- a/eval/eval/create_list_step.h +++ b/eval/eval/create_list_step.h @@ -1,9 +1,10 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_CREATE_LIST_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_CREATE_LIST_STEP_H_ +#include "absl/status/statusor.h" +#include "absl/types/span.h" #include "eval/eval/evaluator_core.h" #include "eval/eval/expression_step_base.h" -#include "absl/types/span.h" namespace google { namespace api { diff --git a/eval/eval/create_struct_step.h b/eval/eval/create_struct_step.h index 93ce3c9b3..1c5035192 100644 --- a/eval/eval/create_struct_step.h +++ b/eval/eval/create_struct_step.h @@ -1,6 +1,7 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_CREATE_STRUCT_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_CREATE_STRUCT_STEP_H_ +#include "absl/status/statusor.h" #include "eval/eval/evaluator_core.h" #include "eval/eval/expression_step_base.h" diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index 1d66a19a5..fc68104ba 100644 --- a/eval/eval/evaluator_core.cc +++ b/eval/eval/evaluator_core.cc @@ -6,7 +6,6 @@ #include "eval/eval/attribute_trail.h" #include "eval/public/cel_value.h" #include "base/status_macros.h" -#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index 4e092823a..1ad1008c6 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -14,6 +14,7 @@ #include "google/api/expr/v1alpha1/syntax.pb.h" #include "google/protobuf/arena.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/types/optional.h" #include "absl/types/span.h" @@ -24,7 +25,6 @@ #include "eval/public/cel_expression.h" #include "eval/public/cel_value.h" #include "eval/public/unknown_attribute_set.h" -#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/function_step.cc b/eval/eval/function_step.cc index 174771537..989c7daee 100644 --- a/eval/eval/function_step.cc +++ b/eval/eval/function_step.cc @@ -28,7 +28,6 @@ #include "eval/public/unknown_function_result_set.h" #include "eval/public/unknown_set.h" #include "base/status_macros.h" -#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/function_step.h b/eval/eval/function_step.h index 20f1d84e3..36b3da61e 100644 --- a/eval/eval/function_step.h +++ b/eval/eval/function_step.h @@ -6,13 +6,13 @@ #include #include "google/api/expr/v1alpha1/syntax.pb.h" +#include "absl/status/statusor.h" #include "eval/eval/evaluator_core.h" #include "eval/eval/expression_build_warning.h" #include "eval/public/activation.h" #include "eval/public/cel_function.h" #include "eval/public/cel_function_registry.h" #include "eval/public/cel_value.h" -#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/eval/ident_step.h b/eval/eval/ident_step.h index 8e18fa637..663b1f63c 100644 --- a/eval/eval/ident_step.h +++ b/eval/eval/ident_step.h @@ -1,6 +1,7 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_IDENT_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_IDENT_STEP_H_ +#include "absl/status/statusor.h" #include "eval/eval/evaluator_core.h" #include "eval/public/activation.h" #include "eval/public/cel_value.h" diff --git a/eval/eval/jump_step.h b/eval/eval/jump_step.h index 3dca34d27..e84342a77 100644 --- a/eval/eval/jump_step.h +++ b/eval/eval/jump_step.h @@ -1,11 +1,12 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_JUMP_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_JUMP_STEP_H_ +#include "google/api/expr/v1alpha1/syntax.pb.h" +#include "absl/status/statusor.h" #include "eval/eval/evaluator_core.h" #include "eval/eval/expression_step_base.h" #include "eval/public/activation.h" #include "eval/public/cel_value.h" -#include "google/api/expr/v1alpha1/syntax.pb.h" namespace google { namespace api { diff --git a/eval/eval/logic_step.h b/eval/eval/logic_step.h index 397e90061..09bc60dc4 100644 --- a/eval/eval/logic_step.h +++ b/eval/eval/logic_step.h @@ -1,11 +1,12 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_LOGIC_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_LOGIC_STEP_H_ +#include "google/api/expr/v1alpha1/syntax.pb.h" +#include "absl/status/statusor.h" #include "eval/eval/evaluator_core.h" #include "eval/public/activation.h" #include "eval/public/cel_function.h" #include "eval/public/cel_value.h" -#include "google/api/expr/v1alpha1/syntax.pb.h" namespace google { namespace api { diff --git a/eval/eval/select_step.h b/eval/eval/select_step.h index d288c1654..cfb8afafc 100644 --- a/eval/eval/select_step.h +++ b/eval/eval/select_step.h @@ -1,6 +1,7 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_SELECT_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_SELECT_STEP_H_ +#include "absl/status/statusor.h" #include "eval/eval/evaluator_core.h" #include "eval/public/activation.h" #include "eval/public/cel_value.h" diff --git a/eval/eval/ternary_step.h b/eval/eval/ternary_step.h index f86747644..49696da88 100644 --- a/eval/eval/ternary_step.h +++ b/eval/eval/ternary_step.h @@ -1,11 +1,12 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_TERNARY_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_TERNARY_STEP_H_ +#include "google/api/expr/v1alpha1/syntax.pb.h" +#include "absl/status/statusor.h" #include "eval/eval/evaluator_core.h" #include "eval/public/activation.h" #include "eval/public/cel_function.h" #include "eval/public/cel_value.h" -#include "google/api/expr/v1alpha1/syntax.pb.h" namespace google { namespace api { diff --git a/eval/public/BUILD b/eval/public/BUILD index 2e4d42eca..afaa3dd5c 100644 --- a/eval/public/BUILD +++ b/eval/public/BUILD @@ -25,7 +25,6 @@ cc_library( deps = [ ":cel_value_internal", "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:str_format", "@com_google_absl//absl/time", @@ -218,6 +217,7 @@ cc_library( ":cel_function", ":cel_function_registry", ":cel_value", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", ], @@ -610,6 +610,7 @@ cc_library( "//eval/public/structs:cel_proto_wrapper", "//internal:proto_util", "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_googleapis//google/api/expr/v1alpha1:value_cc_proto", "@com_google_protobuf//:protobuf", diff --git a/eval/public/cel_expression.h b/eval/public/cel_expression.h index cf189b281..a2cd76c00 100644 --- a/eval/public/cel_expression.h +++ b/eval/public/cel_expression.h @@ -4,6 +4,7 @@ #include #include "google/api/expr/v1alpha1/syntax.pb.h" +#include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "eval/public/activation.h" #include "eval/public/cel_function.h" diff --git a/eval/public/cel_function_adapter.h b/eval/public/cel_function_adapter.h index dae0f050d..a26391cb0 100644 --- a/eval/public/cel_function_adapter.h +++ b/eval/public/cel_function_adapter.h @@ -3,11 +3,11 @@ #include #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "eval/public/cel_function.h" #include "eval/public/cel_function_registry.h" #include "eval/public/structs/cel_proto_wrapper.h" -#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/public/cel_function_provider.h b/eval/public/cel_function_provider.h index 3f13d2b33..aa85920a9 100644 --- a/eval/public/cel_function_provider.h +++ b/eval/public/cel_function_provider.h @@ -1,6 +1,7 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_CEL_FUNCTION_PROVIDER_H_ #define THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_CEL_FUNCTION_PROVIDER_H_ +#include "absl/status/statusor.h" #include "eval/public/activation.h" #include "eval/public/cel_function.h" diff --git a/eval/public/cel_value.h b/eval/public/cel_value.h index 333aafb49..6e84cf108 100644 --- a/eval/public/cel_value.h +++ b/eval/public/cel_value.h @@ -25,7 +25,6 @@ #include "absl/time/time.h" #include "absl/types/optional.h" #include "eval/public/cel_value_internal.h" -#include "util/task/statusor.h" namespace google { namespace api { diff --git a/eval/public/transform_utility.cc b/eval/public/transform_utility.cc index 0ae4b44e8..e10a27a99 100644 --- a/eval/public/transform_utility.cc +++ b/eval/public/transform_utility.cc @@ -5,6 +5,7 @@ #include "google/protobuf/struct.pb.h" #include "google/protobuf/arena.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "eval/public/cel_value.h" #include "eval/public/containers/container_backed_list_impl.h" diff --git a/eval/public/transform_utility.h b/eval/public/transform_utility.h index ca601a8d7..2e4c92c1a 100644 --- a/eval/public/transform_utility.h +++ b/eval/public/transform_utility.h @@ -3,6 +3,7 @@ #include "google/api/expr/v1alpha1/value.pb.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "eval/public/cel_value.h" namespace google { diff --git a/eval/tests/BUILD b/eval/tests/BUILD index 2c45cba95..f53726bca 100644 --- a/eval/tests/BUILD +++ b/eval/tests/BUILD @@ -104,6 +104,7 @@ cc_library( deps = [ "//eval/public:activation", "//eval/public:cel_expression", + "@com_google_absl//absl/status:statusor", "@com_google_googletest//:gtest", ], ) diff --git a/eval/tests/mock_cel_expression.h b/eval/tests/mock_cel_expression.h index bd04e831e..fad18e82d 100644 --- a/eval/tests/mock_cel_expression.h +++ b/eval/tests/mock_cel_expression.h @@ -4,6 +4,7 @@ #include #include "gmock/gmock.h" +#include "absl/status/statusor.h" #include "eval/public/activation.h" #include "eval/public/cel_expression.h" From 729b8f39550d2bc9e0b13bcbbaad05fdc8c82c81 Mon Sep 17 00:00:00 2001 From: kmilsht Date: Mon, 26 Oct 2020 14:29:13 -0400 Subject: [PATCH 08/19] Adding support for bool keys to FieldBackedMapImpl PiperOrigin-RevId: 339080710 --- eval/public/containers/field_backed_map_impl.cc | 7 +++++++ .../containers/field_backed_map_impl_test.cc | 17 +++++++++++++++++ eval/testutil/test_message.proto | 1 + 3 files changed, 25 insertions(+) diff --git a/eval/public/containers/field_backed_map_impl.cc b/eval/public/containers/field_backed_map_impl.cc index 9cf6c5b12..96e94240d 100644 --- a/eval/public/containers/field_backed_map_impl.cc +++ b/eval/public/containers/field_backed_map_impl.cc @@ -122,6 +122,10 @@ absl::optional FieldBackedMapImpl::operator[](CelValue key) const { // Fast implementation. google::protobuf::MapKey inner_key; switch (key.type()) { + case CelValue::Type::kBool: { + inner_key.SetBoolValue(key.BoolOrDie()); + break; + } case CelValue::Type::kInt64: { inner_key.SetInt64Value(key.Int64OrDie()); break; @@ -201,6 +205,9 @@ absl::optional FieldBackedMapImpl::operator[](CelValue key) const { bool match = false; switch (key.type()) { + case CelValue::Type::kBool: + match = key.BoolOrDie() == inner_key.BoolOrDie(); + break; case CelValue::Type::kInt64: match = key.Int64OrDie() == inner_key.Int64OrDie(); break; diff --git a/eval/public/containers/field_backed_map_impl_test.cc b/eval/public/containers/field_backed_map_impl_test.cc index 3e2f3f08e..41b214aa1 100644 --- a/eval/public/containers/field_backed_map_impl_test.cc +++ b/eval/public/containers/field_backed_map_impl_test.cc @@ -41,6 +41,23 @@ TEST(FieldBackedMapImplTest, IntKeyTest) { EXPECT_EQ((*cel_map)[CelValue::CreateInt64(3)].has_value(), false); } +TEST(FieldBackedMapImplTest, BoolKeyTest) { + TestMessage message; + auto field_map = message.mutable_bool_int32_map(); + (*field_map)[false] = 1; + + google::protobuf::Arena arena; + + auto cel_map = CreateMap(&message, "bool_int32_map", &arena); + + EXPECT_EQ((*cel_map)[CelValue::CreateBool(false)]->Int64OrDie(), 1); + // Look up nonexistent key + EXPECT_EQ((*cel_map)[CelValue::CreateBool(true)].has_value(), false); + + (*field_map)[true] = 2; + EXPECT_EQ((*cel_map)[CelValue::CreateBool(true)]->Int64OrDie(), 2); +} + TEST(FieldBackedMapImplTest, UintKeyTest) { TestMessage message; auto field_map = message.mutable_uint64_int32_map(); diff --git a/eval/testutil/test_message.proto b/eval/testutil/test_message.proto index 22fb71c70..38a08b428 100644 --- a/eval/testutil/test_message.proto +++ b/eval/testutil/test_message.proto @@ -64,6 +64,7 @@ message TestMessage { map int64_int32_map = 201; map uint64_int32_map = 202; map string_int32_map = 203; + map bool_int32_map = 204; // Well-known types. google.protobuf.Any any_value = 300; From 70b68127f6f64729bdfe506213497899824b57c2 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Mon, 26 Oct 2020 17:12:26 -0400 Subject: [PATCH 09/19] Print out the optional field name in NoSuchFieldError to make debugging easier. PiperOrigin-RevId: 339114639 --- eval/eval/select_step.cc | 2 +- eval/public/cel_value.cc | 7 +++++-- eval/public/cel_value.h | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index 23d3e6432..53e258d43 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -51,7 +51,7 @@ absl::Status SelectStep::CreateValueFromField(const google::protobuf::Message* m const FieldDescriptor* field_desc = desc->FindFieldByName(field_); if (field_desc == nullptr) { - *result = CreateNoSuchFieldError(arena); + *result = CreateNoSuchFieldError(arena, field_); return absl::OkStatus(); } diff --git a/eval/public/cel_value.cc b/eval/public/cel_value.cc index 414ace71e..2d47bdbb0 100644 --- a/eval/public/cel_value.cc +++ b/eval/public/cel_value.cc @@ -16,6 +16,7 @@ namespace { using google::protobuf::Arena; constexpr char kErrNoMatchingOverload[] = "No matching overloads found"; +constexpr char kErrNoSuchField[] = "no_such_field"; constexpr char kErrNoSuchKey[] = "Key not found in map"; constexpr absl::string_view kErrUnknownValue = "Unknown value "; // Error name for MissingAttributeError indicating that evaluation has @@ -190,8 +191,10 @@ bool CheckNoMatchingOverloadError(CelValue value) { kErrNoMatchingOverload); } -CelValue CreateNoSuchFieldError(google::protobuf::Arena* arena) { - return CreateErrorValue(arena, "no_such_field", absl::StatusCode::kNotFound); +CelValue CreateNoSuchFieldError(google::protobuf::Arena* arena, absl::string_view field) { + return CreateErrorValue( + arena, absl::StrCat(kErrNoSuchField, !field.empty() ? " : " : "", field), + absl::StatusCode::kNotFound); } CelValue CreateNoSuchKeyError(google::protobuf::Arena* arena, absl::string_view key) { diff --git a/eval/public/cel_value.h b/eval/public/cel_value.h index 6e84cf108..76be6f511 100644 --- a/eval/public/cel_value.h +++ b/eval/public/cel_value.h @@ -447,7 +447,8 @@ CelValue CreateNoMatchingOverloadError(google::protobuf::Arena *arena, absl::string_view fn); bool CheckNoMatchingOverloadError(CelValue value); -CelValue CreateNoSuchFieldError(google::protobuf::Arena *arena); +CelValue CreateNoSuchFieldError(google::protobuf::Arena *arena, + absl::string_view field = ""); CelValue CreateNoSuchKeyError(google::protobuf::Arena *arena, absl::string_view key); bool CheckNoSuchKeyError(CelValue value); From 6dfb7c8cc98d029d21b0c6ebd4d8d7a959ca1641 Mon Sep 17 00:00:00 2001 From: kmilsht Date: Mon, 26 Oct 2020 19:02:38 -0400 Subject: [PATCH 10/19] Switch FieldBackedMapImpl to use proto2::Reflection::LookupMapValue instead of the pair ContainsMapKey/InsertOrLookupValue PiperOrigin-RevId: 339137778 --- eval/public/containers/field_access.cc | 8 ++-- eval/public/containers/field_access.h | 2 +- .../containers/field_backed_map_impl.cc | 39 ++++++++----------- 3 files changed, 21 insertions(+), 28 deletions(-) diff --git a/eval/public/containers/field_access.cc b/eval/public/containers/field_access.cc index eab074b83..23442b186 100644 --- a/eval/public/containers/field_access.cc +++ b/eval/public/containers/field_access.cc @@ -19,7 +19,7 @@ namespace { using ::google::protobuf::Arena; using ::google::protobuf::FieldDescriptor; -using ::google::protobuf::MapValueRef; +using ::google::protobuf::MapValueConstRef; using ::google::protobuf::Message; using ::google::protobuf::Reflection; @@ -265,7 +265,7 @@ class RepeatedFieldAccessor : public FieldAccessor { class MapValueAccessor : public FieldAccessor { public: MapValueAccessor(const Message* msg, const FieldDescriptor* field_desc, - const MapValueRef* value_ref) + const MapValueConstRef* value_ref) : FieldAccessor(msg, field_desc), value_ref_(value_ref) {} bool GetBool() const { return value_ref_->GetBoolValue(); } @@ -293,7 +293,7 @@ class MapValueAccessor : public FieldAccessor { const Reflection* GetReflection() const { return msg_->GetReflection(); } private: - const MapValueRef* value_ref_; + const MapValueConstRef* value_ref_; }; // Helper classes that should retrieve values from CelValue, @@ -345,7 +345,7 @@ absl::Status CreateValueFromRepeatedField(const google::protobuf::Message* msg, absl::Status CreateValueFromMapValue(const google::protobuf::Message* msg, const FieldDescriptor* desc, - const MapValueRef* value_ref, + const MapValueConstRef* value_ref, google::protobuf::Arena* arena, CelValue* result) { MapValueAccessor accessor(msg, desc, value_ref); return accessor.CreateValueFromFieldAccessor(arena, result); diff --git a/eval/public/containers/field_access.h b/eval/public/containers/field_access.h index 63ed38369..711cf1744 100644 --- a/eval/public/containers/field_access.h +++ b/eval/public/containers/field_access.h @@ -39,7 +39,7 @@ absl::Status CreateValueFromRepeatedField(const google::protobuf::Message* msg, // result pointer to CelValue to store the result in. absl::Status CreateValueFromMapValue(const google::protobuf::Message* msg, const google::protobuf::FieldDescriptor* desc, - const google::protobuf::MapValueRef* value_ref, + const google::protobuf::MapValueConstRef* value_ref, google::protobuf::Arena* arena, CelValue* result); // Assigns content of CelValue to singular message field. diff --git a/eval/public/containers/field_backed_map_impl.cc b/eval/public/containers/field_backed_map_impl.cc index 96e94240d..76c45be61 100644 --- a/eval/public/containers/field_backed_map_impl.cc +++ b/eval/public/containers/field_backed_map_impl.cc @@ -23,11 +23,11 @@ class CelMapReflectionFriend { return reflection->ContainsMapKey(message, field, key); } - static bool InsertOrLookupMapValue(const Reflection* reflection, - Message* message, - const FieldDescriptor* field, - const MapKey& key, MapValueRef* val) { - return reflection->InsertOrLookupMapValue(message, field, key, val); + static bool LookupMapValue(const Reflection* reflection, + const Message& message, + const FieldDescriptor* field, const MapKey& key, + MapValueConstRef* val) { + return reflection->LookupMapValue(message, field, key, val); } }; @@ -46,7 +46,7 @@ namespace { using google::protobuf::Arena; using google::protobuf::Descriptor; using google::protobuf::FieldDescriptor; -using google::protobuf::MapValueRef; +using google::protobuf::MapValueConstRef; using google::protobuf::Message; // Map entries have two field tags @@ -139,27 +139,20 @@ absl::optional FieldBackedMapImpl::operator[](CelValue key) const { inner_key.SetStringValue(std::string(str.begin(), str.end())); break; } - default: { return {}; } + default: { + return {}; + } } - // Performance issue. Currently the only way to do a lookup is - // InsertOrLookupMapValue. This function will modify the map if the key - // doesn't exist, that is why we have to call ContainsMapKey first, which - // results in hashing the key more than once. - if (!google::protobuf::expr::CelMapReflectionFriend::ContainsMapKey( - reflection_, *message_, descriptor_, inner_key)) { + MapValueConstRef value_ref; + // Look the value up + if (!google::protobuf::expr::CelMapReflectionFriend::LookupMapValue( + reflection_, *message_, descriptor_, inner_key, &value_ref)) { return {}; } - MapValueRef value_ref; - // InsertOrLookupMapValue is not marked as const (but it is const in this - // scenario when ContainsMapKey returns true), so we use const_cast. - if (google::protobuf::expr::CelMapReflectionFriend::InsertOrLookupMapValue( - reflection_, const_cast(message_), descriptor_, - inner_key, &value_ref)) { - GOOGLE_LOG(ERROR) << "The map was expected to have the key, but it didn't."; - } + // Get value descriptor treating it as a repeated field. // All values in protobuf map have the same type. - // The map is not empty, because ContainsMapKey returned true. + // The map is not empty, because LookuMapValue returned true. const Message* entry = &reflection_->GetRepeatedMessage(*message_, descriptor_, 0); if (entry == nullptr) { @@ -176,7 +169,7 @@ absl::optional FieldBackedMapImpl::operator[](CelValue key) const { return CreateErrorValue(arena_, status.message()); } return result; -#else // GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND +#else // GOOGLE_PROTOBUF_HAS_CEL_MAP_REFLECTION_FRIEND // Slow implementation. CelValue result = CelValue::CreateNull(); CelValue inner_key = CelValue::CreateNull(); From 5200c84b79fb1a5c52707a0cfda44bc796130930 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Wed, 28 Oct 2020 16:00:45 -0400 Subject: [PATCH 11/19] Clean up unused protobuf-lite dependency PiperOrigin-RevId: 339520858 --- eval/public/BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/eval/public/BUILD b/eval/public/BUILD index afaa3dd5c..6a06715df 100644 --- a/eval/public/BUILD +++ b/eval/public/BUILD @@ -229,7 +229,6 @@ cc_library( hdrs = ["source_position.h"], deps = [ "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", - "@com_google_protobuf//:protobuf_lite", ], ) From 4e2d9aecff95fc7643c1c6a6ed4c4d42c4c296b7 Mon Sep 17 00:00:00 2001 From: tswadell Date: Mon, 2 Nov 2020 14:56:12 -0500 Subject: [PATCH 12/19] Update the has() macro support for C++ and fix enum resolution behavior when the qualified enum id is encoded within an identifier and not just within parse-only select expressions. PiperOrigin-RevId: 340285064 --- conformance/BUILD | 12 +-- eval/compiler/flat_expr_builder.cc | 15 ++- eval/compiler/flat_expr_builder_test.cc | 124 ++++++++++++++++++++++++ eval/eval/BUILD | 1 + eval/eval/create_struct_step.cc | 1 + eval/eval/select_step.cc | 28 +++++- 6 files changed, 165 insertions(+), 16 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index 8c5f1c302..878921227 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -5,7 +5,6 @@ package(default_visibility = ["//visibility:public"]) licenses(["notice"]) # Apache 2.0 -# TODO(issues/77): Add support for the proto2, proto3 conformance tests. ALL_TESTS = [ "@com_google_cel_spec//tests/simple:testdata/basic.textproto", "@com_google_cel_spec//tests/simple:testdata/comparisons.textproto", @@ -101,17 +100,16 @@ cc_binary( "--skip_test=conversions/type", # TODO(issues/84): Missing uint() conversion functions "--skip_test=conversions/uint", - # TODO(issues/96): + # TODO(issues/96): Well-known type conversion support. "--skip_test=proto2/literal_wellknown", "--skip_test=proto3/literal_wellknown", "--skip_test=proto2/empty_field/wkt", "--skip_test=proto3/empty_field/wkt", - # TODO(issues/86): - "--skip_test=proto2/has/undefined,repeated_none_implicit,repeated_none_explicit,repeated_one,repeated_many,map_none_implicit,map_none_explicit,map_one_default,map_one,map_many,optional_enum_set,optional_enum_set_zero,oneof_other_set,oneof_set_default", - "--skip_test=proto3/has/undefined,repeated_none_implicit,repeated_none_explicit,repeated_one,repeated_many,map_none_implicit,map_none_explicit,map_one_default,map_one,map_many,optional_enum_set,optional_enum_set_zero,oneof_other_set,oneof_set_default,single_enum_set,single_enum_set_zero", + # TODO(issues/92): Support for parse-only proto message creation within a container. + "--skip_test=proto2/has/undefined", + "--skip_test=proto3/has/undefined", # Requires container support "--skip_test=namespace/namespace/self_eval_container_lookup_unchecked", - "--skip_test=basic/namespace/self_eval_container_lookup,self_eval_container_lookup_unchecked", "--skip_test=basic/self_eval_nonzeroish/self_eval_bytes_invalid_utf8", # Requires heteregenous equality spec clarification "--skip_test=comparisons/eq_literal/eq_bytes", @@ -120,7 +118,7 @@ cc_binary( "--skip_test=comparisons/in_map_literal/key_in_mixed_key_type_map_error", "--skip_test=fields/in/singleton", # Requires qualified bindings error message relaxation - "--skip_test=fields/qualified_identifier_resolution/ident_with_longest_prefix_check,int64_field_select_unsupported,list_field_select_unsupported,map_key_null,qualified_identifier_resolution_unchecked", + "--skip_test=fields/qualified_identifier_resolution/int64_field_select_unsupported,list_field_select_unsupported,map_key_null,qualified_identifier_resolution_unchecked", "--skip_test=string/size/one_unicode,unicode", "--skip_test=string/bytes_concat/left_unit", # TODO(issues/85): The exists one macro should not short-circuit false. diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index 61df4fb01..ec35e18fb 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -230,11 +230,14 @@ class FlatExprVisitor : public AstVisitor { return; } - // Generate namespace map - + // Attempt to resolve the identifier as an enum. const google::protobuf::EnumValueDescriptor* value_desc = nullptr; + auto it = enum_map_.find(path); + if (it != enum_map_.end()) { + value_desc = it->second; + } - // Fill out namespace map for wrapping Select's + // Attempt to resolve a parsed-only expression as a namespaced identifier. while (!namespace_stack_.empty()) { const auto& select_node = namespace_stack_.back(); // Generate path in format ".....". @@ -247,7 +250,6 @@ class FlatExprVisitor : public AstVisitor { resolved_select_expr_ = select_node.first; value_desc = it->second; } - namespace_stack_.pop_back(); } @@ -259,6 +261,10 @@ class FlatExprVisitor : public AstVisitor { AddStep(CreateConstValueStep(value_desc, resolved_select_expr_->id())); return; } + if (value_desc) { + AddStep(CreateConstValueStep(value_desc, expr->id())); + return; + } AddStep(CreateIdentStep(ident_expr, expr->id())); } @@ -430,7 +436,6 @@ class FlatExprVisitor : public AstVisitor { if (!progress_status_.ok()) { return; } - AddStep(CreateCreateStructStep(struct_expr, expr->id())); } diff --git a/eval/compiler/flat_expr_builder_test.cc b/eval/compiler/flat_expr_builder_test.cc index f263e57e0..cac177595 100644 --- a/eval/compiler/flat_expr_builder_test.cc +++ b/eval/compiler/flat_expr_builder_test.cc @@ -655,6 +655,37 @@ TEST(FlatExprBuilderTest, SimpleEnumTest) { EXPECT_THAT(result.Int64OrDie(), Eq(TestMessage::TEST_ENUM_1)); } +TEST(FlatExprBuilderTest, SimpleEnumIdentTest) { + TestMessage message; + + Expr expr; + SourceInfo source_info; + + constexpr char enum_name[] = + "google.api.expr.runtime.TestMessage.TestEnum.TEST_ENUM_1"; + + Expr* cur_expr = &expr; + cur_expr->mutable_ident_expr()->set_name(enum_name); + + FlatExprBuilder builder; + builder.AddResolvableEnum(TestMessage::TestEnum_descriptor()); + + auto build_status = builder.CreateExpression(&expr, &source_info); + ASSERT_OK(build_status); + + auto cel_expr = std::move(build_status.value()); + + google::protobuf::Arena arena; + Activation activation; + auto eval_status = cel_expr->Evaluate(activation, &arena); + + ASSERT_OK(eval_status); + CelValue result = eval_status.value(); + + ASSERT_TRUE(result.IsInt64()); + EXPECT_THAT(result.Int64OrDie(), Eq(TestMessage::TEST_ENUM_1)); +} + TEST(FlatExprBuilderTest, ContainerStringFormat) { Expr expr; SourceInfo source_info; @@ -778,6 +809,99 @@ TEST(FlatExprBuilderTest, PartialQualifiedEnumResolution) { EXPECT_THAT(result.Int64OrDie(), Eq(TestMessage::TEST_ENUM_1)); } +TEST(FlatExprBuilderTest, MapFieldPresence) { + SourceInfo source_info; + Expr expr; + google::protobuf::TextFormat::ParseFromString(R"( + id: 1, + select_expr{ + operand { + id: 2 + ident_expr{ name: "msg" } + } + field: "string_int32_map" + test_only: true + })", + &expr); + + FlatExprBuilder builder; + auto build_status = builder.CreateExpression(&expr, &source_info); + ASSERT_OK(build_status); + auto cel_expr = std::move(build_status.value()); + + google::protobuf::Arena arena; + { + TestMessage message; + auto strMap = message.mutable_string_int32_map(); + strMap->insert({"key", 1}); + Activation activation; + activation.InsertValue("msg", + CelProtoWrapper::CreateMessage(&message, &arena)); + auto eval_status = cel_expr->Evaluate(activation, &arena); + ASSERT_OK(eval_status); + auto result = eval_status.value(); + ASSERT_TRUE(result.IsBool()); + ASSERT_TRUE(result.BoolOrDie()); + } + { + TestMessage message; + Activation activation; + activation.InsertValue("msg", + CelProtoWrapper::CreateMessage(&message, &arena)); + auto eval_status = cel_expr->Evaluate(activation, &arena); + ASSERT_OK(eval_status); + auto result = eval_status.value(); + ASSERT_TRUE(result.IsBool()); + ASSERT_FALSE(result.BoolOrDie()); + } +} + +TEST(FlatExprBuilderTest, RepeatedFieldPresence) { + SourceInfo source_info; + Expr expr; + google::protobuf::TextFormat::ParseFromString(R"( + id: 1, + select_expr{ + operand { + id: 2 + ident_expr{ name: "msg" } + } + field: "int32_list" + test_only: true + })", + &expr); + + FlatExprBuilder builder; + auto build_status = builder.CreateExpression(&expr, &source_info); + ASSERT_OK(build_status); + auto cel_expr = std::move(build_status.value()); + + google::protobuf::Arena arena; + { + TestMessage message; + message.add_int32_list(1); + Activation activation; + activation.InsertValue("msg", + CelProtoWrapper::CreateMessage(&message, &arena)); + auto eval_status = cel_expr->Evaluate(activation, &arena); + ASSERT_OK(eval_status); + auto result = eval_status.value(); + ASSERT_TRUE(result.IsBool()); + ASSERT_TRUE(result.BoolOrDie()); + } + { + TestMessage message; + Activation activation; + activation.InsertValue("msg", + CelProtoWrapper::CreateMessage(&message, &arena)); + auto eval_status = cel_expr->Evaluate(activation, &arena); + ASSERT_OK(eval_status); + auto result = eval_status.value(); + ASSERT_TRUE(result.IsBool()); + ASSERT_FALSE(result.BoolOrDie()); + } +} + absl::Status RunTernaryExpression(CelValue selector, CelValue value1, CelValue value2, google::protobuf::Arena* arena, CelValue* result) { diff --git a/eval/eval/BUILD b/eval/eval/BUILD index 67e5fa18b..9dcfafae4 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -161,6 +161,7 @@ cc_library( "//eval/public/containers:field_access", "//eval/public/containers:field_backed_list_impl", "//eval/public/containers:field_backed_map_impl", + "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", ], diff --git a/eval/eval/create_struct_step.cc b/eval/eval/create_struct_step.cc index 53b8933d2..abd92e8d8 100644 --- a/eval/eval/create_struct_step.cc +++ b/eval/eval/create_struct_step.cc @@ -274,6 +274,7 @@ absl::StatusOr> CreateCreateStructStep( const google::api::expr::v1alpha1::Expr::CreateStruct* create_struct_expr, int64_t expr_id) { if (!create_struct_expr->message_name().empty()) { + // TODO(issues/92): Support resolving a type name within a container. // Make message-creating step. std::vector entries; diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index 53e258d43..507c13e54 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -56,16 +56,36 @@ absl::Status SelectStep::CreateValueFromField(const google::protobuf::Message* m } if (field_desc->is_map()) { - *result = CelValue::CreateMap(google::protobuf::Arena::Create( - arena, msg, field_desc, arena)); + // When the map field appears in a has(msg.map_field) expression, the map + // is considered 'present' when it is non-empty. Since maps are repeated + // fields they don't participate with standard proto presence testing since + // the repeated field is always at least empty. + if (test_field_presence_) { + *result = + CelValue::CreateBool(reflection->FieldSize(*msg, field_desc) != 0); + return absl::OkStatus(); + } + CelMap* map = google::protobuf::Arena::Create(arena, msg, + field_desc, arena); + *result = CelValue::CreateMap(map); return absl::OkStatus(); } if (field_desc->is_repeated()) { - *result = CelValue::CreateList(google::protobuf::Arena::Create( - arena, msg, field_desc, arena)); + // When the list field appears in a has(msg.list_field) expression, the list + // is considered 'present' when it is non-empty. + if (test_field_presence_) { + *result = + CelValue::CreateBool(reflection->FieldSize(*msg, field_desc) != 0); + return absl::OkStatus(); + } + CelList* list = google::protobuf::Arena::Create( + arena, msg, field_desc, arena); + *result = CelValue::CreateList(list); return absl::OkStatus(); } + if (test_field_presence_) { + // Standard proto presence test for non-repeated fields. *result = CelValue::CreateBool(reflection->HasField(*msg, field_desc)); return absl::OkStatus(); } From e541ad5501a9c7f91fc377c43c725bea733e284e Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Wed, 4 Nov 2020 19:31:02 -0500 Subject: [PATCH 13/19] Add function reference renaming to reference resolver. PiperOrigin-RevId: 340749678 --- eval/compiler/BUILD | 4 + eval/compiler/qualified_reference_resolver.cc | 370 +++++++++++++----- eval/compiler/qualified_reference_resolver.h | 9 +- .../qualified_reference_resolver_test.cc | 338 ++++++++++++---- 4 files changed, 547 insertions(+), 174 deletions(-) diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index ce0a4847b..ed454e5f3 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -164,6 +164,10 @@ cc_test( deps = [ ":qualified_reference_resolver", "//base:status_macros", + "//eval/public:builtin_func_registrar", + "//eval/public:cel_builtins", + "//eval/public:cel_function", + "//eval/public:cel_function_registry", "//testutil:util", "@com_google_absl//absl/status", "@com_google_absl//absl/types:optional", diff --git a/eval/compiler/qualified_reference_resolver.cc b/eval/compiler/qualified_reference_resolver.cc index da137df0d..4dcc921b1 100644 --- a/eval/compiler/qualified_reference_resolver.cc +++ b/eval/compiler/qualified_reference_resolver.cc @@ -1,8 +1,12 @@ #include "eval/compiler/qualified_reference_resolver.h" +#include + #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" +#include "absl/strings/str_split.h" +#include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "eval/eval/const_value_step.h" #include "eval/eval/expression_build_warning.h" @@ -20,112 +24,180 @@ namespace { using google::api::expr::v1alpha1::Expr; using google::api::expr::v1alpha1::Reference; +// Determines if function is implemented with custom evaluation step instead of +// registered. +bool IsSpecialFunction(absl::string_view function_name) { + return function_name == builtin::kAnd || function_name == builtin::kOr || + function_name == builtin::kIndex || function_name == builtin::kTernary; +} + +// Convert a select expr sub tree into a namespace name if possible. +// If any operand of the top element is a not a select or an ident node, +// return nullopt. +absl::optional ToNamespace(const Expr& expr) { + absl::optional maybe_parent_namespace; + switch (expr.expr_kind_case()) { + case Expr::kIdentExpr: + return expr.ident_expr().name(); + case Expr::kSelectExpr: + if (expr.select_expr().test_only()) { + return absl::nullopt; + } + maybe_parent_namespace = ToNamespace(expr.select_expr().operand()); + if (!maybe_parent_namespace.has_value()) { + return absl::nullopt; + } + return absl::StrCat(maybe_parent_namespace.value(), ".", + expr.select_expr().field()); + default: + return absl::nullopt; + } +} + +// Shape matcher for CelFunctions. +// TODO(issues/91): this is the same behavior as parsed exprs in the CPP +// evaluator (just check the right call style and number of arguments), but we +// should have enough type information in a checked expr to find a more +// specific candidate list. +std::vector ArgumentMatcher(int argument_count) { + std::vector argument_matcher(argument_count); + for (int i = 0; i < argument_count; i++) { + argument_matcher[i] = CelValue::Type::kAny; + } + return argument_matcher; +} + +bool OverloadExists(const CelFunctionRegistry& registry, absl::string_view name, + const std::vector& argument_matcher, + bool receiver_style = false) { + return !registry.FindOverloads(name, receiver_style, argument_matcher) + .empty() || + !registry.FindLazyOverloads(name, receiver_style, argument_matcher) + .empty(); +} + +// Handles checking for the most specific (most qualified) function that matches +// the call shape. +class ContainerLookupHelper { + public: + ContainerLookupHelper(absl::string_view container, + const CelFunctionRegistry& registry) + : registry_(registry) { + auto container_elements = absl::StrSplit(container, '.'); + std::string prefix = ""; + namespace_prefixes_.push_back(prefix); + for (const auto& elem : container_elements) { + // Tolerate trailing / leading '.'. + if (elem.empty()) { + continue; + } + absl::StrAppend(&prefix, elem, "."); + namespace_prefixes_.insert(namespace_prefixes_.begin(), prefix); + } + } + + // Return the qualified name of the most qualified matching overload, or + // nullopt if no matches are found. + absl::optional BestOverloadMatch(absl::string_view base_name, + int argument_count) { + if (IsSpecialFunction(base_name)) { + return std::string(base_name); + } + auto argument_matcher = ArgumentMatcher(argument_count); + // Check from most qualified to least qualified for a matching overload. + for (auto iter = namespace_prefixes_.begin(); + iter != namespace_prefixes_.end(); ++iter) { + std::string resolved_name = absl::StrCat(*iter, base_name); + if (OverloadExists(registry_, resolved_name, argument_matcher)) { + return resolved_name; + } + } + return absl::nullopt; + } + + private: + // Namespace prefixes to check in most to least specific order. + std::vector namespace_prefixes_; + const CelFunctionRegistry& registry_; +}; + class ReferenceResolver { public: ReferenceResolver(const google::protobuf::Map& reference_map, - BuilderWarnings* warnings) - : reference_map_(reference_map), warnings_(warnings) {} + const CelFunctionRegistry& registry, + BuilderWarnings* warnings, absl::string_view container) + : reference_map_(reference_map), + container_lookup_helper_(container, registry), + registry_(registry), + warnings_(warnings) {} // Attempt to resolve references in expr. Return true if part of the // expression was rewritten. + // TODO(issues/95): If possible, it would be nice to write a general utility + // for running the preprocess steps when traversing the AST instead of having + // one pass per transform. absl::StatusOr Rewrite(Expr* out) { - absl::StatusOr maybe_rewrite_result = MaybeRewriteReferences(out); - RETURN_IF_ERROR(maybe_rewrite_result.status()); - - if (maybe_rewrite_result.value()) { - return true; + const auto reference_iter = reference_map_.find(out->id()); + const Reference* reference = nullptr; + if (reference_iter != reference_map_.end()) { + if (!reference_iter->second.has_value()) { + // TODO(issues/71): Add support for compile time constants like enum + // values. + reference = &reference_iter->second; + } } - // If we don't have a rewrite rule, continue traversing the AST. + bool updated = false; + switch (out->expr_kind_case()) { case Expr::kConstExpr: { return false; } case Expr::kIdentExpr: - return false; - case Expr::kSelectExpr: { - return Rewrite(out->mutable_select_expr()->mutable_operand()); - } + return MaybeUpdateIdentNode(out, reference); + case Expr::kSelectExpr: + return MaybeUpdateSelectNode(out, reference); case Expr::kCallExpr: { - auto* call_expr = out->mutable_call_expr(); - const bool receiver_style = call_expr->has_target(); - const int arg_num = call_expr->args_size(); - bool args_updated = false; - if (receiver_style) { - absl::StatusOr rewrite_result = - Rewrite(call_expr->mutable_target()); - RETURN_IF_ERROR(rewrite_result.status()); - args_updated = args_updated || rewrite_result.value(); - } - for (int i = 0; i < arg_num; i++) { - absl::StatusOr rewrite_result = - Rewrite(call_expr->mutable_args(i)); - RETURN_IF_ERROR(rewrite_result.status()); - args_updated = args_updated || rewrite_result.value(); - } - return args_updated; + return MaybeUpdateCallNode(out, reference); } case Expr::kListExpr: { auto* list_expr = out->mutable_list_expr(); int list_size = list_expr->elements_size(); - bool args_updated = false; for (int i = 0; i < list_size; i++) { absl::StatusOr rewrite_result = Rewrite(list_expr->mutable_elements(i)); RETURN_IF_ERROR(rewrite_result.status()); - args_updated = args_updated || rewrite_result.value(); + updated = updated || rewrite_result.value(); } - return args_updated; + return updated; } case Expr::kStructExpr: { - auto* struct_expr = out->mutable_struct_expr(); - int entries_size = struct_expr->entries_size(); - bool args_updated = false; - for (int i = 0; i < entries_size; i++) { - auto* new_entry = struct_expr->mutable_entries(i); - switch (new_entry->key_kind_case()) { - case Expr::CreateStruct::Entry::kFieldKey: - // Nothing to do. - break; - case Expr::CreateStruct::Entry::kMapKey: - args_updated = - Rewrite(new_entry->mutable_map_key()).ok() || args_updated; - break; - default: - GOOGLE_LOG(ERROR) << "Unsupported Entry kind: " - << new_entry->key_kind_case(); - break; - } - args_updated = - Rewrite(new_entry->mutable_value()).ok() || args_updated; - } - return args_updated; + return MaybeUpdateStructNode(out, reference); } case Expr::kComprehensionExpr: { auto* out_expr = out->mutable_comprehension_expr(); - bool args_updated = false; absl::StatusOr rewrite_result; rewrite_result = Rewrite(out_expr->mutable_accu_init()); RETURN_IF_ERROR(rewrite_result.status()); - args_updated = args_updated || rewrite_result.value(); + updated = updated || rewrite_result.value(); rewrite_result = Rewrite(out_expr->mutable_iter_range()); RETURN_IF_ERROR(rewrite_result.status()); - args_updated = args_updated || rewrite_result.value(); + updated = updated || rewrite_result.value(); rewrite_result = Rewrite(out_expr->mutable_loop_condition()); RETURN_IF_ERROR(rewrite_result.status()); - args_updated = args_updated || rewrite_result.value(); + updated = updated || rewrite_result.value(); rewrite_result = Rewrite(out_expr->mutable_loop_step()); RETURN_IF_ERROR(rewrite_result.status()); - args_updated = args_updated || rewrite_result.value(); + updated = updated || rewrite_result.value(); rewrite_result = Rewrite(out_expr->mutable_result()); RETURN_IF_ERROR(rewrite_result.status()); - args_updated = args_updated || rewrite_result.value(); + updated = updated || rewrite_result.value(); - return args_updated; + return updated; } default: GOOGLE_LOG(ERROR) << "Unsupported Expr kind: " << out->expr_kind_case(); @@ -134,55 +206,146 @@ class ReferenceResolver { } private: - // Attempts to apply rewrites for reference map. Returns true if rewrites - // occur. - absl::StatusOr MaybeRewriteReferences(Expr* expr) { - const auto iter = reference_map_.find(expr->id()); - if (iter == reference_map_.end()) { - return false; + // Attempt to update a function call node. This disambiguates + // receiver call verses namespaced names in parse if possible. + // + // TODO(issues/95): This duplicates some of the overload matching behavior + // for parsed expressions. We should refactor to consolidate the code. + absl::StatusOr MaybeUpdateCallNode(Expr* out, + const Reference* reference) { + auto* call_expr = out->mutable_call_expr(); + if (reference != nullptr && reference->overload_id_size() == 0) { + RETURN_IF_ERROR(warnings_->AddWarning(absl::InvalidArgumentError( + absl::StrCat("Reference map doesn't provide overloads for ", + out->call_expr().function())))); + } + bool receiver_style = call_expr->has_target(); + bool updated = false; + int arg_num = call_expr->args_size(); + if (receiver_style) { + // First check the target to see if the reference map indicates it + // should be rewritten. + absl::StatusOr rewrite_result = + Rewrite(call_expr->mutable_target()); + RETURN_IF_ERROR(rewrite_result.status()); + bool target_updated = rewrite_result.value(); + updated = target_updated; + if (!target_updated) { + // If the function receiver was not rewritten, check to see if it's + // actually a namespace for the function. + auto maybe_namespace = ToNamespace(call_expr->target()); + if (maybe_namespace.has_value()) { + std::string resolved_name = + absl::StrCat(maybe_namespace.value(), ".", call_expr->function()); + auto maybe_resolved_function = + container_lookup_helper_.BestOverloadMatch(resolved_name, + arg_num); + if (maybe_resolved_function.has_value()) { + call_expr->set_function(maybe_resolved_function.value()); + call_expr->clear_target(); + updated = true; + } + } + } + } else { + // Not a receiver style function call. Check to see if it is a namespaced + // function using a shorthand inside the expression container. + auto maybe_resolved_function = container_lookup_helper_.BestOverloadMatch( + call_expr->function(), arg_num); + if (!maybe_resolved_function.has_value()) { + RETURN_IF_ERROR(warnings_->AddWarning(absl::InvalidArgumentError( + absl::StrCat("No overload found in reference resolve step for ", + call_expr->function())))); + } else if (maybe_resolved_function.value() != call_expr->function()) { + call_expr->set_function(maybe_resolved_function.value()); + updated = true; + } } - const Reference& reference = iter->second; - if (reference.has_value() || !reference.overload_id().empty()) { - // TODO(issues/71): Add support for functions and compile time - // constants. - return false; + // For parity, if we didn't rewrite the receiver call style function, + // check that an overload is provided in the builder. + if (call_expr->has_target() && + !OverloadExists(registry_, call_expr->function(), + ArgumentMatcher(arg_num + 1), + /* receiver_style= */ true)) { + RETURN_IF_ERROR(warnings_->AddWarning(absl::InvalidArgumentError( + absl::StrCat("No overload found in reference resolve step for ", + call_expr->function())))); } + for (int i = 0; i < arg_num; i++) { + absl::StatusOr rewrite_result = Rewrite(call_expr->mutable_args(i)); + RETURN_IF_ERROR(rewrite_result.status()); + updated = updated || rewrite_result.value(); + } + return updated; + } - switch (expr->expr_kind_case()) { - case Expr::ExprKindCase::kIdentExpr: - if (reference.name() != expr->ident_expr().name()) { - // Possibly shorthand for a namespaced name. - expr->clear_ident_expr(); - expr->mutable_ident_expr()->set_name(reference.name()); - return true; - } else { - return false; - } - case Expr::ExprKindCase::kStructExpr: - // reference to a create struct message type. nothing to do. - // TODO(issues/72): annotating the execution plan with this may help - // identify problems with the environment setup. This will probably - // also require the type map information from a checked expression. - return false; - case Expr::ExprKindCase::kSelectExpr: - if (expr->select_expr().test_only()) { - RETURN_IF_ERROR(warnings_->AddWarning( - absl::InvalidArgumentError("Reference map points to a presence " - "test -- has(container.attr)"))); - return false; - } - expr->clear_select_expr(); - expr->mutable_ident_expr()->set_name(reference.name()); + // Attempt to resolve a select node. If reference is not-null and valid, + // replace the select node with the fully qualified ident node. Otherwise, + // continue recursively rewriting the Expr. + absl::StatusOr MaybeUpdateSelectNode(Expr* out, + const Reference* reference) { + if (reference != nullptr) { + if (out->select_expr().test_only()) { + RETURN_IF_ERROR(warnings_->AddWarning( + absl::InvalidArgumentError("Reference map points to a presence " + "test -- has(container.attr)"))); + } else if (!reference->name().empty()) { + out->clear_select_expr(); + out->mutable_ident_expr()->set_name(reference->name()); return true; - default: - RETURN_IF_ERROR( - warnings_->AddWarning(absl::InvalidArgumentError(absl::StrCat( - "Unsupported reference kind: ", expr->expr_kind_case())))); - return false; + } + } + return Rewrite(out->mutable_select_expr()->mutable_operand()); + } + + // Attempt to resolve an ident node. If reference is not-null and valid, + // replace the node with the fully qualified ident node. + bool MaybeUpdateIdentNode(Expr* out, const Reference* reference) { + if (reference != nullptr && !reference->name().empty() && + reference->name() != out->ident_expr().name()) { + out->mutable_ident_expr()->set_name(reference->name()); + return true; + } + return false; + } + + // Update a create struct node. Currently, just handles recursing. + // + // TODO(issues/72): annotating the execution plan with this may help + // identify problems with the environment setup. This will probably + // also require the type map information from a checked expression. + absl::StatusOr MaybeUpdateStructNode(Expr* out, + const Reference* reference) { + auto* struct_expr = out->mutable_struct_expr(); + int entries_size = struct_expr->entries_size(); + bool updated = false; + for (int i = 0; i < entries_size; i++) { + auto* new_entry = struct_expr->mutable_entries(i); + switch (new_entry->key_kind_case()) { + case Expr::CreateStruct::Entry::kFieldKey: + // Nothing to do. + break; + case Expr::CreateStruct::Entry::kMapKey: { + auto key_updated = Rewrite(new_entry->mutable_map_key()); + RETURN_IF_ERROR(key_updated.status()); + updated = updated || key_updated.value(); + break; + } + default: + GOOGLE_LOG(ERROR) << "Unsupported Entry kind: " + << new_entry->key_kind_case(); + break; + } + auto value_updated = Rewrite(new_entry->mutable_value()); + RETURN_IF_ERROR(value_updated.status()); + updated = updated || value_updated.value(); } + return updated; } const google::protobuf::Map& reference_map_; + ContainerLookupHelper container_lookup_helper_; + const CelFunctionRegistry& registry_; BuilderWarnings* warnings_; }; @@ -190,9 +353,10 @@ class ReferenceResolver { absl::StatusOr> ResolveReferences( const Expr& expr, const google::protobuf::Map& reference_map, + const CelFunctionRegistry& registry, absl::string_view container, BuilderWarnings* warnings) { Expr out(expr); - ReferenceResolver resolver(reference_map, warnings); + ReferenceResolver resolver(reference_map, registry, warnings, container); absl::StatusOr rewrite_result = resolver.Rewrite(&out); if (!rewrite_result.ok()) { return rewrite_result.status(); diff --git a/eval/compiler/qualified_reference_resolver.h b/eval/compiler/qualified_reference_resolver.h index 98e1e630b..2d14e94e2 100644 --- a/eval/compiler/qualified_reference_resolver.h +++ b/eval/compiler/qualified_reference_resolver.h @@ -6,7 +6,9 @@ #include "google/protobuf/map.h" #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/string_view.h" #include "eval/eval/expression_build_warning.h" +#include "eval/public/cel_function_registry.h" namespace google { namespace api { @@ -14,12 +16,13 @@ namespace expr { namespace runtime { // A transformation over input expression that produces a new expression with -// select subexpressions replaced by idents referring to the fully-qualified -// variable name. Returns modified expr if updates found. Otherwise, returns -// nullopt. +// select subexpressions replaced by appropriate expressions referring to the +// fully-qualified entity name. Returns modified expr if updates found. +// Otherwise, returns nullopt. absl::StatusOr> ResolveReferences( const google::api::expr::v1alpha1::Expr& expr, const google::protobuf::Map& reference_map, + const CelFunctionRegistry& registry, absl::string_view container, BuilderWarnings* warnings); } // namespace runtime diff --git a/eval/compiler/qualified_reference_resolver_test.cc b/eval/compiler/qualified_reference_resolver_test.cc index 19a96e38a..844f59bf5 100644 --- a/eval/compiler/qualified_reference_resolver_test.cc +++ b/eval/compiler/qualified_reference_resolver_test.cc @@ -5,6 +5,10 @@ #include "gtest/gtest.h" #include "absl/status/status.h" #include "absl/types/optional.h" +#include "eval/public/builtin_func_registrar.h" +#include "eval/public/cel_builtins.h" +#include "eval/public/cel_function.h" +#include "eval/public/cel_function_registry.h" #include "testutil/util.h" #include "base/status_macros.h" @@ -18,7 +22,9 @@ using google::api::expr::v1alpha1::Expr; using google::api::expr::v1alpha1::Reference; using testing::ElementsAre; using testing::Eq; +using testing::IsEmpty; using testing::Optional; +using testing::UnorderedElementsAre; using testutil::EqualsProto; // foo.bar.var1 && bar.foo.var2 @@ -61,6 +67,11 @@ constexpr char kExpr[] = R"( } )"; +MATCHER_P(StatusCodeIs, x, "") { + const absl::Status& status = arg; + return status.code() == x; +} + Expr ParseTestProto(const std::string& pb) { Expr expr; EXPECT_TRUE(google::protobuf::TextFormat::ParseFromString(pb, &expr)); @@ -73,7 +84,8 @@ TEST(ResolveReferences, Basic) { reference_map[2].set_name("foo.bar.var1"); reference_map[5].set_name("bar.foo.var2"); BuilderWarnings warnings; - auto result = ResolveReferences(expr, reference_map, &warnings); + CelFunctionRegistry registry; + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); ASSERT_OK(result); EXPECT_THAT(result.value(), Optional(EqualsProto(R"( id: 1 @@ -94,8 +106,9 @@ TEST(ResolveReferences, ReturnsNulloptIfNoChanges) { Expr expr = ParseTestProto(kExpr); google::protobuf::Map reference_map; BuilderWarnings warnings; - auto result = ResolveReferences(expr, reference_map, &warnings); - ASSERT_OK(result); + CelFunctionRegistry registry; + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); EXPECT_THAT(result.value(), Eq(absl::nullopt)); } @@ -103,10 +116,12 @@ TEST(ResolveReferences, NamespacedIdent) { Expr expr = ParseTestProto(kExpr); google::protobuf::Map reference_map; BuilderWarnings warnings; + CelFunctionRegistry registry; + reference_map[2].set_name("foo.bar.var1"); reference_map[7].set_name("namespace_x.bar"); - auto result = ResolveReferences(expr, reference_map, &warnings); - ASSERT_OK(result); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); EXPECT_THAT(result.value(), Optional(EqualsProto(R"( id: 1 call_expr { @@ -134,60 +149,6 @@ TEST(ResolveReferences, NamespacedIdent) { })"))); } -TEST(ResolveReferences, WarningOnUnsupportedExprKind) { - Expr expr = ParseTestProto( - R"( - id: 1 - comprehension_expr { - accu_init { - id: 2 - const_expr { int64_value: 1 } - } - accu_var: "__result__" - iter_var: "x" - iter_range { - id: 3 - list_expr { - elements { - id: 4 - const_expr { int64_value: 1 } - } - } - } - result { - id: 6 - ident_expr { name: "__result__" } - } - loop_condition { - id: 5 - const_expr { bool_value: true } - } - loop_step { - id: 7 - call_expr { - function: "_+_" - args { - id: 8 - ident_expr { name: "x" } - } - args { - id: 9 - ident_expr { name: "__result__" } - } - } - } - })"); - google::protobuf::Map reference_map; - BuilderWarnings warnings; - reference_map[1].set_name("foo"); - auto result = ResolveReferences(expr, reference_map, &warnings); - ASSERT_OK(result); - EXPECT_THAT(result.value(), Eq(absl::nullopt)); - EXPECT_THAT(warnings.warnings(), - ElementsAre(Eq(absl::Status(absl::StatusCode::kInvalidArgument, - "Unsupported reference kind: 9")))); -} - TEST(ResolveReferences, WarningOnPresenceTest) { Expr expr = ParseTestProto(R"( id: 1 @@ -207,9 +168,10 @@ TEST(ResolveReferences, WarningOnPresenceTest) { })"); google::protobuf::Map reference_map; BuilderWarnings warnings; + CelFunctionRegistry registry; reference_map[1].set_name("foo.bar.var1"); - auto result = ResolveReferences(expr, reference_map, &warnings); - ASSERT_OK(result); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); EXPECT_THAT(result.value(), Eq(absl::nullopt)); EXPECT_THAT( warnings.warnings(), @@ -221,11 +183,13 @@ TEST(ResolveReferences, WarningOnPresenceTest) { TEST(ResolveReferences, ConstReferenceSkipped) { Expr expr = ParseTestProto(kExpr); google::protobuf::Map reference_map; + CelFunctionRegistry registry; + ASSERT_OK(RegisterBuiltinFunctions(®istry)); reference_map[2].set_name("foo.bar.var1"); reference_map[2].mutable_value()->set_bool_value(true); reference_map[5].set_name("bar.foo.var2"); BuilderWarnings warnings; - auto result = ResolveReferences(expr, reference_map, &warnings); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); ASSERT_OK(result); EXPECT_THAT(result.value(), Optional(EqualsProto(R"( id: 1 @@ -254,15 +218,253 @@ TEST(ResolveReferences, ConstReferenceSkipped) { })"))); } -TEST(ResolveReferences, FunctionReferenceSkipped) { - Expr expr = ParseTestProto(kExpr); +constexpr char kExtensionAndExpr[] = R"( +id: 1 +call_expr { + function: "boolean_and" + args { + id: 2 + const_expr { + bool_value: true + } + } + args { + id: 3 + const_expr { + bool_value: false + } + } +})"; + +TEST(ResolveReferences, FunctionReferenceBasic) { + Expr expr = ParseTestProto(kExtensionAndExpr); google::protobuf::Map reference_map; + CelFunctionRegistry registry; + ASSERT_OK(registry.RegisterLazyFunction( + CelFunctionDescriptor("boolean_and", false, + { + CelValue::Type::kBool, + CelValue::Type::kBool, + }))); BuilderWarnings warnings; - reference_map[1].set_name("@user_defined_boolean_and"); - reference_map[1].add_overload_id("@user_defined_boolean_and_overload1"); - auto result = ResolveReferences(expr, reference_map, &warnings); - ASSERT_OK(result); + reference_map[1].add_overload_id("udf_boolean_and"); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); + EXPECT_THAT(result.value(), Eq(absl::nullopt)); +} + +TEST(ResolveReferences, FunctionReferenceMissingOverloadDetected) { + Expr expr = ParseTestProto(kExtensionAndExpr); + google::protobuf::Map reference_map; + CelFunctionRegistry registry; + BuilderWarnings warnings; + reference_map[1].add_overload_id("udf_boolean_and"); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); + EXPECT_THAT(result.value(), Eq(absl::nullopt)); + EXPECT_THAT(warnings.warnings(), + ElementsAre(StatusCodeIs(absl::StatusCode::kInvalidArgument))); +} + +TEST(ResolveReferences, SpecialBuiltinsNotWarned) { + Expr expr = ParseTestProto(R"( + id: 1 + call_expr { + function: "*" + args { + id: 2 + const_expr { bool_value: true } + } + args { + id: 3 + const_expr { bool_value: false } + } + })"); + + std::vector special_builtins{builtin::kAnd, builtin::kOr, + builtin::kTernary, builtin::kIndex}; + for (const char* builtin_fn : special_builtins) { + google::protobuf::Map reference_map; + // Builtins aren't in the function registry. + CelFunctionRegistry registry; + BuilderWarnings warnings; + reference_map[1].add_overload_id(absl::StrCat("builtin.", builtin_fn)); + expr.mutable_call_expr()->set_function(builtin_fn); + auto result = + ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); + EXPECT_THAT(result.value(), Eq(absl::nullopt)); + EXPECT_THAT(warnings.warnings(), IsEmpty()); + } +} + +TEST(ResolveReferences, + FunctionReferenceMissingOverloadDetectedAndMissingReference) { + Expr expr = ParseTestProto(kExtensionAndExpr); + google::protobuf::Map reference_map; + CelFunctionRegistry registry; + BuilderWarnings warnings; + reference_map[1].set_name("udf_boolean_and"); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); + EXPECT_THAT(result.value(), Eq(absl::nullopt)); + EXPECT_THAT( + warnings.warnings(), + UnorderedElementsAre( + Eq(absl::InvalidArgumentError( + "No overload found in reference resolve step for boolean_and")), + Eq(absl::InvalidArgumentError( + "Reference map doesn't provide overloads for boolean_and")))); +} + +TEST(ResolveReferences, FunctionReferenceToWrongExprKind) { + Expr expr = ParseTestProto(kExtensionAndExpr); + google::protobuf::Map reference_map; + BuilderWarnings warnings; + CelFunctionRegistry registry; + reference_map[2].add_overload_id("udf_boolean_and"); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); + EXPECT_THAT(result.value(), Eq(absl::nullopt)); + EXPECT_THAT(warnings.warnings(), + ElementsAre(StatusCodeIs(absl::StatusCode::kInvalidArgument))); +} + +constexpr char kReceiverCallExtensionAndExpr[] = R"( +id: 1 +call_expr { + function: "boolean_and" + target { + id: 2 + ident_expr { + name: "ext" + } + } + args { + id: 3 + const_expr { + bool_value: false + } + } +})"; + +TEST(ResolveReferences, FunctionReferenceWithTargetNoChange) { + Expr expr = ParseTestProto(kReceiverCallExtensionAndExpr); + google::protobuf::Map reference_map; + BuilderWarnings warnings; + CelFunctionRegistry registry; + ASSERT_OK(registry.RegisterLazyFunction(CelFunctionDescriptor( + "boolean_and", true, {CelValue::Type::kBool, CelValue::Type::kBool}))); + reference_map[1].add_overload_id("udf_boolean_and"); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); + EXPECT_THAT(result.value(), Eq(absl::nullopt)); + EXPECT_THAT(warnings.warnings(), IsEmpty()); +} + +TEST(ResolveReferences, + FunctionReferenceWithTargetNoChangeMissingOverloadDetected) { + Expr expr = ParseTestProto(kReceiverCallExtensionAndExpr); + google::protobuf::Map reference_map; + BuilderWarnings warnings; + CelFunctionRegistry registry; + reference_map[1].add_overload_id("udf_boolean_and"); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); + EXPECT_THAT(result.value(), Eq(absl::nullopt)); + EXPECT_THAT(warnings.warnings(), + ElementsAre(StatusCodeIs(absl::StatusCode::kInvalidArgument))); +} + +TEST(ResolveReferences, FunctionReferenceWithTargetToNamespacedFunction) { + Expr expr = ParseTestProto(kReceiverCallExtensionAndExpr); + google::protobuf::Map reference_map; + BuilderWarnings warnings; + CelFunctionRegistry registry; + ASSERT_OK(registry.RegisterLazyFunction(CelFunctionDescriptor( + "ext.boolean_and", false, {CelValue::Type::kBool}))); + reference_map[1].add_overload_id("udf_boolean_and"); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); + EXPECT_THAT(result.value(), Optional(EqualsProto(R"( + id: 1 + call_expr { + function: "ext.boolean_and" + args { + id: 3 + const_expr { bool_value: false } + } + } + )"))); + EXPECT_THAT(warnings.warnings(), IsEmpty()); +} + +TEST(ResolveReferences, + FunctionReferenceWithTargetToNamespacedFunctionInContainer) { + Expr expr = ParseTestProto(kReceiverCallExtensionAndExpr); + google::protobuf::Map reference_map; + BuilderWarnings warnings; + CelFunctionRegistry registry; + reference_map[1].add_overload_id("udf_boolean_and"); + ASSERT_OK(registry.RegisterLazyFunction(CelFunctionDescriptor( + "com.google.ext.boolean_and", false, {CelValue::Type::kBool}))); + auto result = + ResolveReferences(expr, reference_map, registry, "com.google", &warnings); + ASSERT_OK(result.status()); + EXPECT_THAT(result.value(), Optional(EqualsProto(R"( + id: 1 + call_expr { + function: "com.google.ext.boolean_and" + args { + id: 3 + const_expr { bool_value: false } + } + } + )"))); + EXPECT_THAT(warnings.warnings(), IsEmpty()); +} + +// has(ext.option).boolean_and(false) +constexpr char kReceiverCallHasExtensionAndExpr[] = R"( +id: 1 +call_expr { + function: "boolean_and" + target { + id: 2 + select_expr { + test_only: true + field: "option" + operand { + id: 3 + ident_expr { + name: "ext" + } + } + } + } + args { + id: 4 + const_expr { + bool_value: false + } + } +})"; + +TEST(ResolveReferences, FunctionReferenceWithHasTargetNoChange) { + Expr expr = ParseTestProto(kReceiverCallHasExtensionAndExpr); + google::protobuf::Map reference_map; + BuilderWarnings warnings; + CelFunctionRegistry registry; + ASSERT_OK(registry.RegisterLazyFunction(CelFunctionDescriptor( + "boolean_and", true, {CelValue::Type::kBool, CelValue::Type::kBool}))); + ASSERT_OK(registry.RegisterLazyFunction(CelFunctionDescriptor( + "ext.option.boolean_and", true, {CelValue::Type::kBool}))); + reference_map[1].add_overload_id("udf_boolean_and"); + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result.status()); + // The target is unchanged because it is a test_only select. EXPECT_THAT(result.value(), Eq(absl::nullopt)); + EXPECT_THAT(warnings.warnings(), IsEmpty()); } } // namespace From 26599a13c22c05aaf6feed998d0156a5e45e26ed Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Thu, 12 Nov 2020 12:49:53 -0500 Subject: [PATCH 14/19] Introduce PostVisitTarget() into the ast_visitor, to visit the target of a receiver style Call expression before visiting any of the arguments. PiperOrigin-RevId: 342068658 --- eval/compiler/flat_expr_builder.cc | 3 +++ eval/public/BUILD | 2 -- eval/public/ast_traverse.cc | 10 +++++++-- eval/public/ast_traverse_test.cc | 36 ++++++++++++++++++++++++++++-- eval/public/ast_visitor.h | 6 +++++ eval/public/ast_visitor_base.h | 5 +++++ 6 files changed, 56 insertions(+), 6 deletions(-) diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index ec35e18fb..8a31de088 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -418,6 +418,9 @@ class FlatExprVisitor : public AstVisitor { } } + // Nothing to do. + void PostVisitTarget(const Expr* expr, const SourcePosition*) override {} + // CreateList node handler. // Invoked after child nodes are processed. void PostVisitCreateList(const CreateList* list_expr, const Expr* expr, diff --git a/eval/public/BUILD b/eval/public/BUILD index 6a06715df..a49c6d1c7 100644 --- a/eval/public/BUILD +++ b/eval/public/BUILD @@ -239,7 +239,6 @@ cc_library( ], deps = [ ":source_position", - "@com_google_absl//absl/strings", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", ], ) @@ -266,7 +265,6 @@ cc_library( deps = [ ":ast_visitor", ":source_position", - "@com_google_absl//absl/strings", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", ], ) diff --git a/eval/public/ast_traverse.cc b/eval/public/ast_traverse.cc index c87e5fc88..e4cd717d5 100644 --- a/eval/public/ast_traverse.cc +++ b/eval/public/ast_traverse.cc @@ -37,6 +37,7 @@ namespace { struct StackRecord { public: static constexpr int kNotCallArg = -1; + static constexpr int kTarget = -2; StackRecord(const Expr *e, const SourceInfo *info) : expr(e), @@ -116,7 +117,11 @@ void PostVisit(const StackRecord &record, AstVisitor *visitor) { if (record.call_arg != StackRecord::kNotCallArg && record.calling_expr != nullptr) { - visitor->PostVisitArg(record.call_arg, record.calling_expr, &position); + if (record.call_arg == StackRecord::kTarget) { + visitor->PostVisitTarget(record.calling_expr, &position); + } else { + visitor->PostVisitArg(record.call_arg, record.calling_expr, &position); + } } visitor->PostVisitExpr(expr, &position); } @@ -138,7 +143,8 @@ void PushCallDeps(const Call *call_expr, const Expr *expr, } // Are we receiver-style? if (call_expr->has_target()) { - stack->push(StackRecord(&call_expr->target(), record.source_info)); + stack->push(StackRecord(&call_expr->target(), record.source_info, expr, + StackRecord::kTarget)); } } diff --git a/eval/public/ast_traverse_test.cc b/eval/public/ast_traverse_test.cc index da71ea80f..a2d5638db 100644 --- a/eval/public/ast_traverse_test.cc +++ b/eval/public/ast_traverse_test.cc @@ -89,6 +89,8 @@ class MockAstVisitor : public AstVisitor { // We provide finer granularity for Call and Comprehension node callbacks // to allow special handling for short-circuiting. + MOCK_METHOD(void, PostVisitTarget, + (const Expr* expr, const SourcePosition* position), (override)); MOCK_METHOD(void, PostVisitArg, (int arg_num, const Expr* expr, const SourcePosition* position), (override)); @@ -163,13 +165,41 @@ TEST(AstCrawlerTest, CheckCrawlSelect) { AstTraverse(&expr, &source_info, &handler); } -// Test handling of Call node -TEST(AstCrawlerTest, CheckCrawlCall) { +// Test handling of Call node without receiver +TEST(AstCrawlerTest, CheckCrawlCallNoReceiver) { + SourceInfo source_info; + MockAstVisitor handler; + + Expr expr; + auto call_expr = expr.mutable_call_expr(); + auto arg0 = call_expr->add_args(); + auto const_expr = arg0->mutable_const_expr(); + auto arg1 = call_expr->add_args(); + auto ident_expr = arg1->mutable_ident_expr(); + + testing::InSequence seq; + + // Lowest level entry will be called first + EXPECT_CALL(handler, PreVisitCall(call_expr, &expr, _)).Times(1); + EXPECT_CALL(handler, PostVisitTarget(_, _)).Times(0); + EXPECT_CALL(handler, PostVisitConst(const_expr, arg0, _)).Times(1); + EXPECT_CALL(handler, PostVisitArg(0, &expr, _)).Times(1); + EXPECT_CALL(handler, PostVisitIdent(ident_expr, arg1, _)).Times(1); + EXPECT_CALL(handler, PostVisitArg(1, &expr, _)).Times(1); + EXPECT_CALL(handler, PostVisitCall(call_expr, &expr, _)).Times(1); + + AstTraverse(&expr, &source_info, &handler); +} + +// Test handling of Call node with receiver +TEST(AstCrawlerTest, CheckCrawlCallReceiver) { SourceInfo source_info; MockAstVisitor handler; Expr expr; auto call_expr = expr.mutable_call_expr(); + auto target = call_expr->mutable_target(); + auto target_ident = target->mutable_ident_expr(); auto arg0 = call_expr->add_args(); auto const_expr = arg0->mutable_const_expr(); auto arg1 = call_expr->add_args(); @@ -179,6 +209,8 @@ TEST(AstCrawlerTest, CheckCrawlCall) { // Lowest level entry will be called first EXPECT_CALL(handler, PreVisitCall(call_expr, &expr, _)).Times(1); + EXPECT_CALL(handler, PostVisitIdent(target_ident, target, _)).Times(1); + EXPECT_CALL(handler, PostVisitTarget(&expr, _)).Times(1); EXPECT_CALL(handler, PostVisitConst(const_expr, arg0, _)).Times(1); EXPECT_CALL(handler, PostVisitArg(0, &expr, _)).Times(1); EXPECT_CALL(handler, PostVisitIdent(ident_expr, arg1, _)).Times(1); diff --git a/eval/public/ast_visitor.h b/eval/public/ast_visitor.h index af143c666..bb805de39 100644 --- a/eval/public/ast_visitor.h +++ b/eval/public/ast_visitor.h @@ -98,6 +98,11 @@ class AstVisitor { const google::api::expr::v1alpha1::Expr*, const SourcePosition*) = 0; + // Invoked after target node is processed. + // Expr is the call expression. + virtual void PostVisitTarget(const google::api::expr::v1alpha1::Expr*, + const SourcePosition*) = 0; + // Invoked before all child nodes are processed. virtual void PreVisitComprehension( const google::api::expr::v1alpha1::Expr::Comprehension*, @@ -111,6 +116,7 @@ class AstVisitor { // Invoked after each argument node processed. // For Call arg_num is the index of the argument. // For Comprehension arg_num is specified by ComprehensionArg. + // Expr is the call expression. virtual void PostVisitArg(int arg_num, const google::api::expr::v1alpha1::Expr*, const SourcePosition*) = 0; diff --git a/eval/public/ast_visitor_base.h b/eval/public/ast_visitor_base.h index d41458e15..317253118 100644 --- a/eval/public/ast_visitor_base.h +++ b/eval/public/ast_visitor_base.h @@ -80,9 +80,14 @@ class AstVisitorBase : public AstVisitor { // Invoked after each argument node processed. // For Call arg_num is the index of the argument. // For Comprehension arg_num is specified by ComprehensionArg. + // Expr is the call expression. void PostVisitArg(int, const google::api::expr::v1alpha1::Expr*, const SourcePosition*) override {} + // Invoked after target node processed. + void PostVisitTarget(const google::api::expr::v1alpha1::Expr*, + const SourcePosition*) override {} + // CreateList node handler // Invoked after child nodes are processed. void PostVisitCreateList(const google::api::expr::v1alpha1::Expr::CreateList*, From 03d782fd844f7a7fb683435cc24e9ce7fba9eae7 Mon Sep 17 00:00:00 2001 From: kuat Date: Thu, 12 Nov 2020 21:44:31 -0500 Subject: [PATCH 15/19] Allow disabling the check phase. PiperOrigin-RevId: 342177150 --- conformance/BUILD | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index 878921227..ab8518253 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -19,8 +19,9 @@ ALL_TESTS = [ "@com_google_cel_spec//tests/simple:testdata/macros.textproto", "@com_google_cel_spec//tests/simple:testdata/namespace.textproto", "@com_google_cel_spec//tests/simple:testdata/plumbing.textproto", - "@com_google_cel_spec//tests/simple:testdata/proto2.textproto", - "@com_google_cel_spec//tests/simple:testdata/proto3.textproto", + # TODO(issues/92): Support for parse-only proto message creation within a container. + # "@com_google_cel_spec//tests/simple:testdata/proto2.textproto", + # "@com_google_cel_spec//tests/simple:testdata/proto3.textproto", "@com_google_cel_spec//tests/simple:testdata/string.textproto", "@com_google_cel_spec//tests/simple:testdata/timestamps.textproto", "@com_google_cel_spec//tests/simple:testdata/unknowns.textproto", @@ -78,7 +79,7 @@ cc_binary( args = [ "$(location @com_google_cel_spec//tests/simple:simple_test)", "--server=$(location :server)", - "--check_server=$(location //third_party/cel/go/server/main:cel_server)", + "--skip_check", # TODO(issues/93): Inconsistent Duration.getMilliseconds() behavior. "--skip_test=timestamps/duration_converters/get_milliseconds", # TODO(issues/94): Missing timestamp conversion functions (type / string) @@ -125,10 +126,15 @@ cc_binary( "--skip_test=macros/exists_one/list_no_shortcircuit", # TODO(issues/86): Map macro may produce incorrect results on error. "--skip_test=macros/map/list_error", + # TODO(issues/97): Parse-only qualified variable lookup "x.y" wtih binding "x.y" or "y" within container "x" fails + "--skip_test=namespace/qualified/self_eval_qualified_lookup", + "--skip_test=namespace/namespace/self_eval_container_lookup", + "--skip_test=fields/qualified_identifier_resolution/qualified_ident", + "--skip_test=fields/qualified_identifier_resolution/map_field_select", + "--skip_test=fields/qualified_identifier_resolution/ident_with_longest_prefix_check", ] + ["$(location " + test + ")" for test in ALL_TESTS], data = [ ":server", - "//third_party/cel/go/server/main:cel_server", "@com_google_cel_spec//tests/simple:simple_test", ] + ALL_TESTS, ) @@ -144,11 +150,10 @@ sh_test( args = [ "$(location @com_google_cel_spec//tests/simple:simple_test)", "--server=$(location :server)", - "--check_server=$(location //third_party/cel/go/server/main:cel_server)", + "--skip_check", ] + ["$(location " + test + ")" for test in DASHBOARD_TESTS], data = [ ":server", - "//third_party/cel/go/server/main:cel_server", "@com_google_cel_spec//tests/simple:simple_test", ] + DASHBOARD_TESTS, visibility = [ From dbe7c5477f5c39012db9c4a5ed46a2b9492064f6 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Mon, 16 Nov 2020 11:55:42 -0500 Subject: [PATCH 16/19] Add overloads for building checked exprs. Add rewriting routine for resolving references. PiperOrigin-RevId: 342646217 --- eval/compiler/BUILD | 6 + eval/compiler/flat_expr_builder.cc | 69 +++++- eval/compiler/flat_expr_builder.h | 14 ++ eval/compiler/flat_expr_builder_test.cc | 274 ++++++++++++++++++++++++ eval/public/BUILD | 1 + eval/public/cel_expression.h | 25 ++- 6 files changed, 381 insertions(+), 8 deletions(-) diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index ed454e5f3..8e654c3bf 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -16,6 +16,7 @@ cc_library( ], deps = [ ":constant_folding", + ":qualified_reference_resolver", "//base:status_macros", "//eval/eval:comprehension_step", "//eval/eval:const_value_step", @@ -38,6 +39,7 @@ cc_library( "@com_google_absl//absl/container:node_hash_map", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", + "@com_google_googleapis//google/api/expr/v1alpha1:checked_cc_proto", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", ], ) @@ -54,14 +56,18 @@ cc_test( "//eval/public:cel_attribute", "//eval/public:cel_builtins", "//eval/public:cel_expression", + "//eval/public:cel_function_adapter", "//eval/public:cel_options", "//eval/public:cel_value", "//eval/public:unknown_attribute_set", "//eval/public:unknown_set", + "//eval/public/containers:container_backed_map_impl", "//eval/public/structs:cel_proto_wrapper", "//eval/testutil:test_message_cc_proto", "@com_google_absl//absl/status", "@com_google_absl//absl/strings", + "@com_google_absl//absl/types:span", + "@com_google_googleapis//google/api/expr/v1alpha1:checked_cc_proto", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", "@com_google_googletest//:gtest_main", "@com_google_protobuf//:protobuf", diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index 8a31de088..95dee4528 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -1,5 +1,6 @@ #include "eval/compiler/flat_expr_builder.h" +#include "google/api/expr/v1alpha1/checked.pb.h" #include "stack" #include "absl/container/node_hash_map.h" #include "absl/status/statusor.h" @@ -7,6 +8,7 @@ #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" #include "eval/compiler/constant_folding.h" +#include "eval/compiler/qualified_reference_resolver.h" #include "eval/eval/comprehension_step.h" #include "eval/eval/const_value_step.h" #include "eval/eval/container_access_step.h" @@ -535,7 +537,10 @@ void BinaryCondVisitor::PostVisitArg(int arg_num, const Expr* expr) { visitor_->AddStep(std::move(jump_step_status)); } } + void BinaryCondVisitor::PostVisit(const Expr* expr) { + // TODO(issues/41): shortcircuit behavior is non-obvious: should add + // documentation and structure the code a bit better. visitor_->AddStep((cond_value_) ? CreateOrStep(expr->id()) : CreateAndStep(expr->id())); if (short_circuiting_) { @@ -709,9 +714,10 @@ void ComprehensionVisitor::PostVisit(const Expr*) {} } // namespace absl::StatusOr> -FlatExprBuilder::CreateExpression(const Expr* expr, - const SourceInfo* source_info, - std::vector* warnings) const { +FlatExprBuilder::CreateExpressionImpl( + const Expr* expr, const SourceInfo* source_info, + const google::protobuf::Map* reference_map, + std::vector* warnings) const { ExecutionPath execution_path; BuilderWarnings warnings_builder(fail_on_warnings_); @@ -722,10 +728,34 @@ FlatExprBuilder::CreateExpression(const Expr* expr, absl::flat_hash_map idents; + const Expr* effective_expr = expr; // transformed expression preserving expression IDs - Expr out; + Expr rewrite_buffer; + // TODO(issues/98): A type checker may perform these rewrites, but there + // currently isn't a signal to expose that in an expression. If that becomes + // available, we can skip the reference resolve step here if it's already + // done. + if (reference_map != nullptr && !reference_map->empty()) { + absl::StatusOr> rewritten = + ResolveReferences(*effective_expr, *reference_map, *GetRegistry(), + container(), &warnings_builder); + if (!rewritten.ok()) { + return rewritten.status(); + } + if (rewritten.value().has_value()) { + rewrite_buffer = std::move(rewritten)->value(); + effective_expr = &rewrite_buffer; + } + // TODO(issues/99): we could setup a check step here that confirms all of + // references are defined before actually evaluating. + } + if (constant_folding_) { - FoldConstants(*expr, *this->GetRegistry(), constant_arena_, idents, &out); + Expr buffer; + FoldConstants(*effective_expr, *this->GetRegistry(), constant_arena_, + idents, &buffer); + rewrite_buffer = std::move(buffer); + effective_expr = &rewrite_buffer; } std::set iter_variable_names; @@ -734,7 +764,7 @@ FlatExprBuilder::CreateExpression(const Expr* expr, idents, enable_comprehension_, &warnings_builder, &iter_variable_names); - AstTraverse(constant_folding_ ? &out : expr, source_info, &visitor); + AstTraverse(effective_expr, source_info, &visitor); if (!visitor.progress_status().ok()) { return visitor.progress_status(); @@ -752,10 +782,35 @@ FlatExprBuilder::CreateExpression(const Expr* expr, return std::move(expression_impl); } +absl::StatusOr> +FlatExprBuilder::CreateExpression(const Expr* expr, + const SourceInfo* source_info, + std::vector* warnings) const { + return CreateExpressionImpl(expr, source_info, /*reference_map=*/nullptr, + warnings); +} + absl::StatusOr> FlatExprBuilder::CreateExpression(const Expr* expr, const SourceInfo* source_info) const { - return CreateExpression(expr, source_info, nullptr); + return CreateExpressionImpl(expr, source_info, /*reference_map=*/nullptr, + /*warnings=*/nullptr); +} + +absl::StatusOr> +FlatExprBuilder::CreateExpression(const CheckedExpr* checked_expr, + std::vector* warnings) const { + return CreateExpressionImpl(&checked_expr->expr(), + &checked_expr->source_info(), + &checked_expr->reference_map(), warnings); +} + +absl::StatusOr> +FlatExprBuilder::CreateExpression(const CheckedExpr* checked_expr) const { + return CreateExpressionImpl(&checked_expr->expr(), + &checked_expr->source_info(), + &checked_expr->reference_map(), + /*warnings=*/nullptr); } } // namespace runtime diff --git a/eval/compiler/flat_expr_builder.h b/eval/compiler/flat_expr_builder.h index f68da0d2f..3b18ba9da 100644 --- a/eval/compiler/flat_expr_builder.h +++ b/eval/compiler/flat_expr_builder.h @@ -1,6 +1,7 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_COMPILER_FLAT_EXPR_BUILDER_H_ #define THIRD_PARTY_CEL_CPP_EVAL_COMPILER_FLAT_EXPR_BUILDER_H_ +#include "google/api/expr/v1alpha1/checked.pb.h" #include "google/api/expr/v1alpha1/syntax.pb.h" #include "absl/status/statusor.h" #include "eval/public/cel_expression.h" @@ -73,6 +74,19 @@ class FlatExprBuilder : public CelExpressionBuilder { const google::api::expr::v1alpha1::SourceInfo* source_info, std::vector* warnings) const override; + absl::StatusOr> CreateExpression( + const google::api::expr::v1alpha1::CheckedExpr* checked_expr) const override; + + absl::StatusOr> CreateExpression( + const google::api::expr::v1alpha1::CheckedExpr* checked_expr, + std::vector* warnings) const override; + + absl::StatusOr> CreateExpressionImpl( + const google::api::expr::v1alpha1::Expr* expr, + const google::api::expr::v1alpha1::SourceInfo* source_info, + const google::protobuf::Map* reference_map, + std::vector* warnings) const; + private: bool enable_unknowns_; bool enable_unknown_function_results_; diff --git a/eval/compiler/flat_expr_builder_test.cc b/eval/compiler/flat_expr_builder_test.cc index cac177595..d3d73da55 100644 --- a/eval/compiler/flat_expr_builder_test.cc +++ b/eval/compiler/flat_expr_builder_test.cc @@ -1,5 +1,8 @@ #include "eval/compiler/flat_expr_builder.h" +#include + +#include "google/api/expr/v1alpha1/checked.pb.h" #include "google/api/expr/v1alpha1/syntax.pb.h" #include "google/protobuf/field_mask.pb.h" #include "google/protobuf/text_format.h" @@ -8,12 +11,15 @@ #include "absl/status/status.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" +#include "absl/types/span.h" #include "eval/public/builtin_func_registrar.h" #include "eval/public/cel_attribute.h" #include "eval/public/cel_builtins.h" #include "eval/public/cel_expression.h" +#include "eval/public/cel_function_adapter.h" #include "eval/public/cel_options.h" #include "eval/public/cel_value.h" +#include "eval/public/containers/container_backed_map_impl.h" #include "eval/public/structs/cel_proto_wrapper.h" #include "eval/public/unknown_attribute_set.h" #include "eval/public/unknown_set.h" @@ -32,6 +38,7 @@ using google::api::expr::v1alpha1::SourceInfo; using google::protobuf::FieldMask; using testing::Eq; +using testing::HasSubstr; using testing::Not; class ConcatFunction : public CelFunction { @@ -339,6 +346,273 @@ TEST(FlatExprBuilderTest, MapComprehension) { EXPECT_TRUE(result.BoolOrDie()); } +TEST(FlatExprBuilderTest, BasicCheckedExprSupport) { + CheckedExpr expr; + // foo && bar + google::protobuf::TextFormat::ParseFromString(R"( + expr { + id: 1 + call_expr { + function: "_&&_" + args { + id: 2 + ident_expr { + name: "foo" + } + } + args { + id: 3 + ident_expr { + name: "bar" + } + } + } + })", + &expr); + + FlatExprBuilder builder; + ASSERT_OK(RegisterBuiltinFunctions(builder.GetRegistry())); + auto build_status = builder.CreateExpression(&expr); + ASSERT_OK(build_status); + + auto cel_expr = std::move(build_status.value()); + + Activation activation; + activation.InsertValue("foo", CelValue::CreateBool(true)); + activation.InsertValue("bar", CelValue::CreateBool(true)); + google::protobuf::Arena arena; + auto result_or = cel_expr->Evaluate(activation, &arena); + ASSERT_OK(result_or); + CelValue result = result_or.value(); + ASSERT_TRUE(result.IsBool()); + EXPECT_TRUE(result.BoolOrDie()); +} + +TEST(FlatExprBuilderTest, CheckedExprWithReferenceMap) { + CheckedExpr expr; + // `foo.var1` && `bar.var2` + google::protobuf::TextFormat::ParseFromString(R"( + reference_map { + key: 2 + value { + name: "foo.var1" + } + } + reference_map { + key: 4 + value { + name: "bar.var2" + } + } + expr { + id: 1 + call_expr { + function: "_&&_" + args { + id: 2 + select_expr { + field: "var1" + operand { + id: 3 + ident_expr { + name: "foo" + } + } + } + } + args { + id: 4 + select_expr { + field: "var2" + operand { + ident_expr { + name: "bar" + } + } + } + } + } + })", + &expr); + + FlatExprBuilder builder; + ASSERT_OK(RegisterBuiltinFunctions(builder.GetRegistry())); + auto build_status = builder.CreateExpression(&expr); + ASSERT_OK(build_status); + + auto cel_expr = std::move(build_status.value()); + + Activation activation; + activation.InsertValue("foo.var1", CelValue::CreateBool(true)); + activation.InsertValue("bar.var2", CelValue::CreateBool(true)); + google::protobuf::Arena arena; + auto result_or = cel_expr->Evaluate(activation, &arena); + ASSERT_OK(result_or); + CelValue result = result_or.value(); + ASSERT_TRUE(result.IsBool()); + EXPECT_TRUE(result.BoolOrDie()); +} + +TEST(FlatExprBuilderTest, CheckedExprWithReferenceMapFunction) { + CheckedExpr expr; + // ext.and(var1, bar.var2) + google::protobuf::TextFormat::ParseFromString(R"( + reference_map { + key: 1 + value { + overload_id: "com.foo.ext.and" + } + } + reference_map { + key: 3 + value { + name: "com.foo.var1" + } + } + reference_map { + key: 4 + value { + name: "bar.var2" + } + } + expr { + id: 1 + call_expr { + function: "and" + target { + id: 2 + ident_expr { + name: "ext" + } + } + args { + id: 3 + ident_expr { + name: "var1" + } + } + args { + id: 4 + select_expr { + field: "var2" + operand { + id: 5 + ident_expr { + name: "bar" + } + } + } + } + } + })", + &expr); + + FlatExprBuilder builder; + builder.set_container("com.foo"); + ASSERT_OK(RegisterBuiltinFunctions(builder.GetRegistry())); + ASSERT_OK((FunctionAdapter::CreateAndRegister( + "com.foo.ext.and", false, + [](google::protobuf::Arena*, bool lhs, bool rhs) { return lhs && rhs; }, + builder.GetRegistry()))); + auto build_status = builder.CreateExpression(&expr); + ASSERT_OK(build_status); + + auto cel_expr = std::move(build_status.value()); + + Activation activation; + activation.InsertValue("com.foo.var1", CelValue::CreateBool(true)); + activation.InsertValue("bar.var2", CelValue::CreateBool(true)); + google::protobuf::Arena arena; + auto result_or = cel_expr->Evaluate(activation, &arena); + ASSERT_OK(result_or); + CelValue result = result_or.value(); + ASSERT_TRUE(result.IsBool()); + EXPECT_TRUE(result.BoolOrDie()); +} + +TEST(FlatExprBuilderTest, CheckedExprActivationMissesReferences) { + CheckedExpr expr; + // && . + google::protobuf::TextFormat::ParseFromString(R"( + reference_map { + key: 2 + value { + name: "foo.var1" + } + } + reference_map { + key: 5 + value { + name: "bar" + } + } + expr { + id: 1 + call_expr { + function: "_&&_" + args { + id: 2 + select_expr { + field: "var1" + operand { + id: 3 + ident_expr { + name: "foo" + } + } + } + } + args { + id: 4 + select_expr { + field: "var2" + operand { + id: 5 + ident_expr { + name: "bar" + } + } + } + } + } + })", + &expr); + + FlatExprBuilder builder; + ASSERT_OK(RegisterBuiltinFunctions(builder.GetRegistry())); + auto build_status = builder.CreateExpression(&expr); + ASSERT_OK(build_status); + + auto cel_expr = std::move(build_status.value()); + + Activation activation; + activation.InsertValue("foo.var1", CelValue::CreateBool(true)); + // Activation tries to bind a namespaced variable but the reference map refers + // to the container 'bar'. + activation.InsertValue("bar.var2", CelValue::CreateBool(true)); + google::protobuf::Arena arena; + auto result_or = cel_expr->Evaluate(activation, &arena); + ASSERT_OK(result_or); + CelValue result = result_or.ValueOrDie(); + ASSERT_TRUE(result.IsError()); + EXPECT_THAT( + *result.ErrorOrDie(), + Eq(absl::UnknownError("No value with name \"bar\" found in Activation"))); + + // Re-run with the expected interpretation of `bar`.`var2` + std::vector> map_pairs{ + {CelValue::CreateStringView("var2"), CelValue::CreateBool(false)}}; + + std::unique_ptr map_value = + CreateContainerBackedMap(absl::MakeSpan(map_pairs)); + activation.InsertValue("bar", CelValue::CreateMap(map_value.get())); + result_or = cel_expr->Evaluate(activation, &arena); + ASSERT_OK(result_or); + result = result_or.ValueOrDie(); + ASSERT_TRUE(result.IsBool()); + EXPECT_FALSE(result.BoolOrDie()); +} + TEST(FlatExprBuilderTest, ComprehensionWorksForError) { Expr expr; // {}[0].all(x, x) should evaluate OK but return an error value diff --git a/eval/public/BUILD b/eval/public/BUILD index a49c6d1c7..2ec095c48 100644 --- a/eval/public/BUILD +++ b/eval/public/BUILD @@ -219,6 +219,7 @@ cc_library( ":cel_value", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", + "@com_google_googleapis//google/api/expr/v1alpha1:checked_cc_proto", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", ], ) diff --git a/eval/public/cel_expression.h b/eval/public/cel_expression.h index a2cd76c00..524051cc1 100644 --- a/eval/public/cel_expression.h +++ b/eval/public/cel_expression.h @@ -3,6 +3,7 @@ #include +#include "google/api/expr/v1alpha1/checked.pb.h" #include "google/api/expr/v1alpha1/syntax.pb.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" @@ -75,7 +76,7 @@ class CelExpression { class CelExpressionBuilder { public: CelExpressionBuilder() - : registry_(absl::make_unique()) {} + : registry_(absl::make_unique()), container_("") {} virtual ~CelExpressionBuilder() {} @@ -93,6 +94,28 @@ class CelExpressionBuilder { const google::api::expr::v1alpha1::SourceInfo* source_info, std::vector* warnings) const = 0; + // Creates CelExpression object from a checked expression. + // This includes an AST, source info, type hints and ident hints. + // checked_expr ptr must outlive any expressions that are built from it. + virtual absl::StatusOr> CreateExpression( + const google::api::expr::v1alpha1::CheckedExpr* checked_expr) const { + // Default implementation just passes through the expr and source info. + return CreateExpression(&checked_expr->expr(), + &checked_expr->source_info()); + } + + // Creates CelExpression object from a checked expression. + // This includes an AST, source info, type hints and ident hints. + // checked_expr ptr must outlive any expressions that are built from it. + // non-fatal build warnings are written to warnings if encountered. + virtual absl::StatusOr> CreateExpression( + const google::api::expr::v1alpha1::CheckedExpr* checked_expr, + std::vector* warnings) const { + // Default implementation just passes through the expr and source_info. + return CreateExpression(&checked_expr->expr(), &checked_expr->source_info(), + warnings); + } + // CelFunction registry. Extension function should be registered with it // prior to expression creation. CelFunctionRegistry* GetRegistry() const { return registry_.get(); } From 28cffc2281ec1a57c44ddfcfc905f085a11f7528 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Wed, 18 Nov 2020 04:20:15 -0500 Subject: [PATCH 17/19] Use Enum Const References Replace Enum idents that have constant value in the reference map with PiperOrigin-RevId: 343033308 --- eval/compiler/qualified_reference_resolver.cc | 12 ++++- eval/compiler/qualified_reference_resolver.h | 5 +- .../qualified_reference_resolver_test.cc | 53 +++++++++++++++++++ 3 files changed, 66 insertions(+), 4 deletions(-) diff --git a/eval/compiler/qualified_reference_resolver.cc b/eval/compiler/qualified_reference_resolver.cc index 4dcc921b1..d92f9f292 100644 --- a/eval/compiler/qualified_reference_resolver.cc +++ b/eval/compiler/qualified_reference_resolver.cc @@ -141,9 +141,17 @@ class ReferenceResolver { const Reference* reference = nullptr; if (reference_iter != reference_map_.end()) { if (!reference_iter->second.has_value()) { - // TODO(issues/71): Add support for compile time constants like enum - // values. reference = &reference_iter->second; + } else { + if (out->expr_kind_case() == Expr::kIdentExpr && + reference_iter->second.value().constant_kind_case() == + Constant::kInt64Value) { + // Replace enum idents with const reference value. + out->clear_ident_expr(); + out->mutable_const_expr()->set_int64_value( + reference_iter->second.value().int64_value()); + return true; + } } } bool updated = false; diff --git a/eval/compiler/qualified_reference_resolver.h b/eval/compiler/qualified_reference_resolver.h index 2d14e94e2..fe8bd8b54 100644 --- a/eval/compiler/qualified_reference_resolver.h +++ b/eval/compiler/qualified_reference_resolver.h @@ -16,8 +16,9 @@ namespace expr { namespace runtime { // A transformation over input expression that produces a new expression with -// select subexpressions replaced by appropriate expressions referring to the -// fully-qualified entity name. Returns modified expr if updates found. +// subexpressions replaced by appropriate expressions referring to the +// fully-qualified entity name or constant expressions in case of enums. +// Returns modified expr if updates found. // Otherwise, returns nullopt. absl::StatusOr> ResolveReferences( const google::api::expr::v1alpha1::Expr& expr, diff --git a/eval/compiler/qualified_reference_resolver_test.cc b/eval/compiler/qualified_reference_resolver_test.cc index 844f59bf5..695aef4e3 100644 --- a/eval/compiler/qualified_reference_resolver_test.cc +++ b/eval/compiler/qualified_reference_resolver_test.cc @@ -180,6 +180,59 @@ TEST(ResolveReferences, WarningOnPresenceTest) { "Reference map points to a presence test -- has(container.attr)")))); } +// foo.bar.var1 == bar.foo.Enum.ENUM_VAL1 +constexpr char kEnumExpr[] = R"( + id: 1 + call_expr { + function: "_==_" + args { + id: 2 + select_expr { + field: "var1" + operand { + id: 3 + select_expr { + field: "bar" + operand { + id: 4 + ident_expr { name: "foo" } + } + } + } + } + } + args { + id: 5 + ident_expr { name: "bar.foo.Enum.ENUM_VAL1" } + } + } +)"; +TEST(ResolveReferences, EnumConstReferenceUsed) { + Expr expr = ParseTestProto(kEnumExpr); + google::protobuf::Map reference_map; + CelFunctionRegistry registry; + ASSERT_OK(RegisterBuiltinFunctions(®istry)); + reference_map[2].set_name("foo.bar.var1"); + reference_map[5].set_name("bar.foo.Enum.ENUM_VAL1"); + reference_map[5].mutable_value()->set_int64_value(9); + BuilderWarnings warnings; + auto result = ResolveReferences(expr, reference_map, registry, "", &warnings); + ASSERT_OK(result); + EXPECT_THAT(result.value(), Optional(EqualsProto(R"( + id: 1 + call_expr { + function: "_==_" + args { + id: 2 + ident_expr { name: "foo.bar.var1" } + } + args { + id: 5 + const_expr { int64_value: 9 } + } + })"))); +} + TEST(ResolveReferences, ConstReferenceSkipped) { Expr expr = ParseTestProto(kExpr); google::protobuf::Map reference_map; From 684848dda38317e6ed5f862781df3a3d2ce6297b Mon Sep 17 00:00:00 2001 From: kuat Date: Thu, 19 Nov 2020 15:28:55 -0500 Subject: [PATCH 18/19] Implement a pipe protocol for cel-spec conformance client/server. This allows removal of gRPC dependency from cel-cpp workspace. PiperOrigin-RevId: 343346002 --- conformance/BUILD | 18 +++---- conformance/opt-test.sh | 3 -- conformance/server.cc | 101 +++++++++++++++++++++++++--------------- conformance/test.sh | 2 - 4 files changed, 73 insertions(+), 51 deletions(-) delete mode 100755 conformance/opt-test.sh delete mode 100755 conformance/test.sh diff --git a/conformance/BUILD b/conformance/BUILD index ab8518253..7464686b7 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -58,14 +58,14 @@ cc_binary( "//eval/public:transform_utility", "//eval/public/containers:container_backed_list_impl", "//eval/public/containers:container_backed_map_impl", - "//google/api/expr/v1alpha1:conformance_service_proto_grpc_cc", "//internal:proto_util", - "//net/grpc:grpc++", "//parser", + "@com_google_absl//absl/flags:parse", "@com_google_absl//absl/strings", "@com_google_cel_spec//proto/test/v1/proto2:test_all_types_cc_proto", "@com_google_cel_spec//proto/test/v1/proto3:test_all_types_cc_proto", "@com_google_googleapis//google/api/expr/v1alpha1:checked_cc_proto", + "@com_google_googleapis//google/api/expr/v1alpha1:conformance_service_cc_proto", "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", "@com_google_googleapis//google/rpc:code_cc_proto", "@com_google_protobuf//:protobuf", @@ -74,12 +74,13 @@ cc_binary( [ sh_test( - name = "simple-" + driver, - srcs = [":" + driver], + name = "simple" + arg, + srcs = ["@com_google_cel_spec//tests:conftest.sh"], args = [ "$(location @com_google_cel_spec//tests/simple:simple_test)", - "--server=$(location :server)", + "--server=\"$(location :server) " + arg + "\"", "--skip_check", + "--pipe", # TODO(issues/93): Inconsistent Duration.getMilliseconds() behavior. "--skip_test=timestamps/duration_converters/get_milliseconds", # TODO(issues/94): Missing timestamp conversion functions (type / string) @@ -138,9 +139,9 @@ cc_binary( "@com_google_cel_spec//tests/simple:simple_test", ] + ALL_TESTS, ) - for driver in [ - "test.sh", - "opt-test.sh", + for arg in [ + "", + "--opt", ] ] @@ -151,6 +152,7 @@ sh_test( "$(location @com_google_cel_spec//tests/simple:simple_test)", "--server=$(location :server)", "--skip_check", + "--pipe", ] + ["$(location " + test + ")" for test in DASHBOARD_TESTS], data = [ ":server", diff --git a/conformance/opt-test.sh b/conformance/opt-test.sh deleted file mode 100755 index 087fb576d..000000000 --- a/conformance/opt-test.sh +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/bash -export CEL_CPP_ENABLE_CONSTANT_FOLDING=true -exec "$@" diff --git a/conformance/server.cc b/conformance/server.cc index ef55432fe..c5d362d3e 100644 --- a/conformance/server.cc +++ b/conformance/server.cc @@ -1,6 +1,8 @@ +#include + #include "google/api/expr/v1alpha1/syntax.pb.h" #include "google/api/expr/v1alpha1/checked.pb.h" -#include "google/api/expr/v1alpha1/conformance_service.grpc.pb.h" +#include "google/api/expr/v1alpha1/conformance_service.pb.h" #include "google/api/expr/v1alpha1/eval.pb.h" #include "google/api/expr/v1alpha1/syntax.pb.h" #include "google/api/expr/v1alpha1/value.pb.h" @@ -8,7 +10,8 @@ #include "google/protobuf/struct.pb.h" #include "google/protobuf/timestamp.pb.h" #include "google/rpc/code.pb.h" -#include "net/grpc/public/include/grpcpp/grpcpp.h" +#include "google/protobuf/util/json_util.h" +#include "absl/flags/parse.h" #include "absl/strings/str_split.h" #include "eval/public/builtin_func_registrar.h" #include "eval/public/cel_expr_builder_factory.h" @@ -21,17 +24,20 @@ #include "proto/test/v1/proto3/test_all_types.pb.h" -using ::grpc::Status; -using ::grpc::StatusCode; +using absl::Status; +using absl::StatusCode; using ::google::protobuf::Arena; +using ::google::protobuf::util::JsonStringToMessage; +using ::google::protobuf::util::MessageToJsonString; + +ABSL_FLAG(bool, opt, false, "Enable optimizations (constant folding)"); namespace google { namespace api { namespace expr { namespace runtime { -class ConformanceServiceImpl final - : public v1alpha1::grpc_gen::ConformanceService::Service { +class ConformanceServiceImpl { public: ConformanceServiceImpl(std::unique_ptr builder) : builder_(std::move(builder)), @@ -40,11 +46,10 @@ class ConformanceServiceImpl final proto3Tests_(&google::api::expr::test::v1::proto3::TestAllTypes:: default_instance()) {} - Status Parse(grpc::ServerContext* context, - const v1alpha1::ParseRequest* request, - v1alpha1::ParseResponse* response) override { + Status Parse(const v1alpha1::ParseRequest* request, + v1alpha1::ParseResponse* response) { if (request->cel_source().empty()) { - return Status(StatusCode::INVALID_ARGUMENT, "No source code."); + return Status(StatusCode::kInvalidArgument, "No source code."); } auto parse_status = parser::Parse(request->cel_source(), ""); if (!parse_status.ok()) { @@ -56,18 +61,16 @@ class ConformanceServiceImpl final (out).MergeFrom(parse_status.value()); response->mutable_parsed_expr()->CopyFrom(out); } - return Status::OK; + return absl::OkStatus(); } - Status Check(grpc::ServerContext* context, - const v1alpha1::CheckRequest* request, - v1alpha1::CheckResponse* response) override { - return Status(StatusCode::UNIMPLEMENTED, "Check is not supported"); + Status Check(const v1alpha1::CheckRequest* request, + v1alpha1::CheckResponse* response) { + return Status(StatusCode::kUnimplemented, "Check is not supported"); } - Status Eval(grpc::ServerContext* context, - const v1alpha1::EvalRequest* request, - v1alpha1::EvalResponse* response) override { + Status Eval(const v1alpha1::EvalRequest* request, + v1alpha1::EvalResponse* response) { const v1alpha1::Expr* expr = nullptr; if (request->has_parsed_expr()) { expr = &request->parsed_expr().expr(); @@ -82,7 +85,7 @@ class ConformanceServiceImpl final auto cel_expression_status = builder_->CreateExpression(&out, &source_info); if (!cel_expression_status.ok()) { - return Status(StatusCode::INTERNAL, + return Status(StatusCode::kInternal, std::string(cel_expression_status.status().message())); } @@ -95,14 +98,14 @@ class ConformanceServiceImpl final (*import_value).MergeFrom(pair.second.value()); auto import_status = ValueToCelValue(*import_value, &arena); if (!import_status.ok()) { - return Status(StatusCode::INTERNAL, import_status.status().ToString()); + return Status(StatusCode::kInternal, import_status.status().ToString()); } activation.InsertValue(pair.first, import_status.value()); } auto eval_status = cel_expression->Evaluate(activation, &arena); if (!eval_status.ok()) { - return Status(StatusCode::INTERNAL, + return Status(StatusCode::kInternal, std::string(eval_status.status().message())); } @@ -116,12 +119,12 @@ class ConformanceServiceImpl final google::api::expr::v1alpha1::Value export_value; auto export_status = CelValueToValue(result, &export_value); if (!export_status.ok()) { - return Status(StatusCode::INTERNAL, export_status.ToString()); + return Status(StatusCode::kInternal, export_status.ToString()); } auto* result_value = response->mutable_result()->mutable_value(); (*result_value).MergeFrom(export_value); } - return Status::OK; + return absl::OkStatus(); } private: @@ -130,13 +133,12 @@ class ConformanceServiceImpl final const google::api::expr::test::v1::proto3::TestAllTypes* proto3Tests_; }; -int RunServer(std::string server_address) { +int RunServer(bool optimize) { google::protobuf::Arena arena; InterpreterOptions options; - const char* enable_constant_folding = - getenv("CEL_CPP_ENABLE_CONSTANT_FOLDING"); - if (enable_constant_folding != nullptr) { + if (optimize) { + std::cerr << "Enabling optimizations" << std::endl; options.constant_folding = true; options.constant_arena = &arena; } @@ -153,19 +155,42 @@ int RunServer(std::string server_address) { NestedEnum_descriptor()); auto register_status = RegisterBuiltinFunctions(builder->GetRegistry()); if (!register_status.ok()) { + std::cerr << "Failed to initialize: " << register_status.ToString() + << std::endl; return 1; } ConformanceServiceImpl service(std::move(builder)); - grpc::ServerBuilder grpc_builder; - int port; - grpc_builder.AddListeningPort(server_address, - grpc::InsecureServerCredentials(), &port); - grpc_builder.RegisterService(&service); - std::unique_ptr server(grpc_builder.BuildAndStart()); - std::cout << "Listening on [::1]:" << port << std::endl; - fflush(stdout); - server->Wait(); + + // Implementation of a simple pipe protocol: + // INPUT LINE 1: parse/check/eval + // INPUT LINE 2: JSON of the corresponding request protobuf + // OUTPUT LINE 1: JSON of the coressponding response protobuf + while (true) { + std::string cmd, input, output; + std::getline(std::cin, cmd); + std::getline(std::cin, input); + if (cmd == "parse") { + v1alpha1::ParseRequest request; + v1alpha1::ParseResponse response; + CHECK_OK(JsonStringToMessage(input, &request)); + CHECK_OK(service.Parse(&request, &response)); + CHECK_OK(MessageToJsonString(response, &output)); + } else if (cmd == "eval") { + v1alpha1::EvalRequest request; + v1alpha1::EvalResponse response; + CHECK_OK(JsonStringToMessage(input, &request)); + CHECK_OK(service.Eval(&request, &response)); + CHECK_OK(MessageToJsonString(response, &output)); + } else if (cmd.empty()) { + return 0; + } else { + std::cerr << "Unexpected command: " << cmd << std::endl; + return 2; + } + std::cout << output << std::endl; + } + return 0; } @@ -175,6 +200,6 @@ int RunServer(std::string server_address) { } // namespace google int main(int argc, char** argv) { - std::string server_address = "[::1]:0"; - return google::api::expr::runtime::RunServer(server_address); + absl::ParseCommandLine(argc, argv); + return google::api::expr::runtime::RunServer(absl::GetFlag(FLAGS_opt)); } diff --git a/conformance/test.sh b/conformance/test.sh deleted file mode 100755 index 9d8ad4d3c..000000000 --- a/conformance/test.sh +++ /dev/null @@ -1,2 +0,0 @@ -#!/bin/bash -exec "$@" From fc02992a44eef7acecbcce354d4f4a39245ac949 Mon Sep 17 00:00:00 2001 From: kuat Date: Tue, 24 Nov 2020 20:06:34 -0500 Subject: [PATCH 19/19] Re-organize OSS build for easier maintenance. Minor fixes for OSS-compatible build. PiperOrigin-RevId: 344157853 --- BUILD.bazel | 1 + Dockerfile | 15 ++ WORKSPACE | 144 +----------------- base/status_macros.h | 4 + bazel/deps.bzl | 116 ++++++++++++++ bazel/deps_extra.bzl | 54 +++++++ cloudbuild.yaml | 2 +- conformance/BUILD | 35 +++++ conformance/server.cc | 9 +- eval/compiler/flat_expr_builder.cc | 2 + eval/compiler/flat_expr_builder_test.cc | 5 +- eval/compiler/qualified_reference_resolver.cc | 1 + eval/public/builtin_func_test.cc | 4 +- eval/public/testing/debug_string.cc | 4 +- eval/tests/BUILD | 1 + eval/tests/benchmark_test.cc | 73 ++++----- protoutil/type_registry.h | 2 +- 17 files changed, 287 insertions(+), 185 deletions(-) create mode 100644 BUILD.bazel create mode 100644 Dockerfile create mode 100644 bazel/deps.bzl create mode 100644 bazel/deps_extra.bzl diff --git a/BUILD.bazel b/BUILD.bazel new file mode 100644 index 000000000..ffd0fb0cd --- /dev/null +++ b/BUILD.bazel @@ -0,0 +1 @@ +package(default_visibility = ["//visibility:public"]) diff --git a/Dockerfile b/Dockerfile new file mode 100644 index 000000000..2561f3a82 --- /dev/null +++ b/Dockerfile @@ -0,0 +1,15 @@ +FROM ubuntu:bionic + +ENV DEBIAN_FRONTEND=noninteractive + +RUN rm -rf /var/lib/apt/lists/* \ + && apt-get update --fix-missing -qq \ + && apt-get install -qqy --no-install-recommends ca-certificates tzdata wget git clang-10 patch \ + && apt-get clean && rm -rf /var/lib/apt/lists/* + +RUN wget https://github.com/bazelbuild/bazelisk/releases/download/v1.5.0/bazelisk-linux-amd64 && chmod +x bazelisk-linux-amd64 && mv bazelisk-linux-amd64 /bin/bazel + +ENV CC=clang-10 +ENV CXX=clang++-10 + +ENTRYPOINT ["/bin/bazel"] diff --git a/WORKSPACE b/WORKSPACE index a910efe12..48ca50b27 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -1,143 +1,9 @@ -load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") -load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository") +workspace(name = "com_google_cel_cpp") -CEL_SPEC_GIT_SHA = "02a12e7cffe452a611b0e6ef47872963bbd87028" # 4/17/2020 +load("//bazel:deps.bzl", "cel_cpp_deps") -CEL_SPEC_SHA = "757cfdb00dc76fd0d12dadbae982c22a9218711d5e4cf30c94cfe6c05b1cdf2b" +cel_cpp_deps() -http_archive( - name = "com_google_cel_spec", - sha256 = CEL_SPEC_SHA, - strip_prefix = "cel-spec-" + CEL_SPEC_GIT_SHA, - urls = ["https://github.com/google/cel-spec/archive/" + CEL_SPEC_GIT_SHA + ".zip"], -) +load("//bazel:deps_extra.bzl", "cel_cpp_deps_extra") -http_archive( - name = "com_google_absl", - strip_prefix = "abseil-cpp-master", - urls = ["https://github.com/abseil/abseil-cpp/archive/master.zip"], -) - -# Google RE2 (Regular Expression) C++ Library -http_archive( - name = "com_googlesource_code_re2", - strip_prefix = "re2-master", - urls = ["https://github.com/google/re2/archive/master.zip"], -) - -# gRPC dependencies: -http_archive( - name = "com_github_grpc_grpc", - sha256 = "1236514199d3deb111a6dd7f6092f67617cd2b147f7eda7adbafccea95de7381", - strip_prefix = "grpc-1.31.0", - urls = ["https://github.com/grpc/grpc/archive/v1.31.0.tar.gz"], -) - -load("@com_github_grpc_grpc//bazel:grpc_deps.bzl", "grpc_deps") - -grpc_deps() - -GOOGLEAPIS_GIT_SHA = "be480e391cc88a75cf2a81960ef79c80d5012068" # Jul 24, 2019 - -GOOGLEAPIS_SHA = "c1969e5b72eab6d9b6cfcff748e45ba57294aeea1d96fd04cd081995de0605c2" - -http_archive( - name = "com_google_googleapis", - sha256 = GOOGLEAPIS_SHA, - strip_prefix = "googleapis-" + GOOGLEAPIS_GIT_SHA, - urls = ["https://github.com/googleapis/googleapis/archive/" + GOOGLEAPIS_GIT_SHA + ".tar.gz"], -) - -load("@com_google_googleapis//:repository_rules.bzl", "switched_rules_by_language") - -switched_rules_by_language( - name = "com_google_googleapis_imports", - cc = True, - go = True, - grpc = True, -) - -http_archive( - name = "io_bazel_rules_go", - sha256 = "f04d2373bcaf8aa09bccb08a98a57e721306c8f6043a2a0ee610fd6853dcde3d", - urls = ["https://github.com/bazelbuild/rules_go/releases/download/0.18.6/rules_go-0.18.6.tar.gz"], -) - -load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") - -# cel-go dependencies: -http_archive( - name = "bazel_gazelle", - sha256 = "3c681998538231a2d24d0c07ed5a7658cb72bfb5fd4bf9911157c0e9ac6a2687", - urls = ["https://github.com/bazelbuild/bazel-gazelle/releases/download/0.17.0/bazel-gazelle-0.17.0.tar.gz"], -) - -load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies", "go_repository") - -git_repository( - name = "com_google_cel_go", - remote = "https://github.com/google/cel-go.git", - tag = "v0.5.1", -) - -go_repository( - name = "org_golang_google_genproto", - build_file_proto_mode = "disable", - commit = "bd91e49a0898e27abb88c339b432fa53d7497ac0", - importpath = "google.golang.org/genproto", -) - -go_repository( - name = "com_github_antlr", - commit = "621b933c7a7f01c67ae9de15103151fa0f9d6d90", - importpath = "github.com/antlr/antlr4", -) - -go_rules_dependencies() - -go_register_toolchains() - -gazelle_dependencies() - -# Parser dependencies -http_archive( - name = "rules_antlr", - sha256 = "7249d1569293d9b239e23c65f6b4c81a07da921738bde0dfeb231ed98be40429", - strip_prefix = "rules_antlr-3cc2f9502a54ceb7b79b37383316b23c4da66f9a", - urls = ["https://github.com/marcohu/rules_antlr/archive/3cc2f9502a54ceb7b79b37383316b23c4da66f9a.tar.gz"], -) - -load("@rules_antlr//antlr:deps.bzl", "antlr_dependencies") - -antlr_dependencies(472) - -http_archive( - name = "antlr4_runtimes", - build_file_content = """ -package(default_visibility = ["//visibility:public"]) -cc_library( - name = "cpp", - srcs = glob(["runtime/Cpp/runtime/src/**/*.cpp"]), - hdrs = glob(["runtime/Cpp/runtime/src/**/*.h"]), - includes = ["runtime/Cpp/runtime/src"], -) -""", - sha256 = "46f5e1af5f4bd28ade55cb632f9a069656b31fc8c2408f9aa045f9b5f5caad64", - strip_prefix = "antlr4-4.7.2", - urls = ["https://github.com/antlr/antlr4/archive/4.7.2.tar.gz"], -) - -# tools/flatbuffers dependencies -FLAT_BUFFERS_SHA = "a83caf5910644ba1c421c002ef68e42f21c15f9f" - -http_archive( - name = "com_github_google_flatbuffers", - sha256 = "b8efbc25721e76780752bad775a97c3f77a0250271e2db37fc747b20e8b0f24a", - strip_prefix = "flatbuffers-" + FLAT_BUFFERS_SHA, - url = "https://github.com/google/flatbuffers/archive/" + FLAT_BUFFERS_SHA + ".tar.gz", -) - -# Needed by gRPC build rules (but not used). Should be after genproto. -load("@com_github_grpc_grpc//bazel:grpc_extra_deps.bzl", "grpc_extra_deps") - -grpc_extra_deps() +cel_cpp_deps_extra() diff --git a/base/status_macros.h b/base/status_macros.h index 4e7da02e1..cac4e8e54 100644 --- a/base/status_macros.h +++ b/base/status_macros.h @@ -67,4 +67,8 @@ inline To down_cast(From* f) { // so we only accept pointers #define EXPECT_OK(expression) EXPECT_TRUE(expression.ok()) #endif +#if !defined(CHECK_OK) +#define CHECK_OK(expression) assert(expression.ok()) +#endif + #endif // THIRD_PARTY_CEL_CPP_BASE_STATUS_MACROS_H_ diff --git a/bazel/deps.bzl b/bazel/deps.bzl new file mode 100644 index 000000000..95e9c3608 --- /dev/null +++ b/bazel/deps.bzl @@ -0,0 +1,116 @@ +""" +Main dependencies of cel-cpp. +""" + +load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") + +def base_deps(): + """Base evaluator and test dependencies.""" + http_archive( + name = "com_google_absl", + strip_prefix = "abseil-cpp-master", + urls = ["https://github.com/abseil/abseil-cpp/archive/master.zip"], + ) + + http_archive( + name = "com_google_googletest", + urls = ["https://github.com/google/googletest/archive/master.zip"], + strip_prefix = "googletest-master", + ) + + http_archive( + name = "com_github_google_benchmark", + urls = ["https://github.com/google/benchmark/archive/master.zip"], + strip_prefix = "benchmark-master", + ) + + http_archive( + name = "com_googlesource_code_re2", + strip_prefix = "re2-master", + urls = ["https://github.com/google/re2/archive/master.zip"], + ) + + PROTOBUF_VERSION = "3.14.0" + http_archive( + name = "com_google_protobuf", + strip_prefix = "protobuf-" + PROTOBUF_VERSION, + urls = ["https://github.com/protocolbuffers/protobuf/archive/v" + PROTOBUF_VERSION + ".tar.gz"], + ) + + GOOGLEAPIS_GIT_SHA = "be480e391cc88a75cf2a81960ef79c80d5012068" # Jul 24, 2019 + GOOGLEAPIS_SHA = "c1969e5b72eab6d9b6cfcff748e45ba57294aeea1d96fd04cd081995de0605c2" + http_archive( + name = "com_google_googleapis", + sha256 = GOOGLEAPIS_SHA, + strip_prefix = "googleapis-" + GOOGLEAPIS_GIT_SHA, + urls = ["https://github.com/googleapis/googleapis/archive/" + GOOGLEAPIS_GIT_SHA + ".tar.gz"], + ) + +def parser_deps(): + """ANTLR dependency for the parser.""" + http_archive( + name = "rules_antlr", + sha256 = "7249d1569293d9b239e23c65f6b4c81a07da921738bde0dfeb231ed98be40429", + strip_prefix = "rules_antlr-3cc2f9502a54ceb7b79b37383316b23c4da66f9a", + urls = ["https://github.com/marcohu/rules_antlr/archive/3cc2f9502a54ceb7b79b37383316b23c4da66f9a.tar.gz"], + ) + + http_archive( + name = "antlr4_runtimes", + build_file_content = """ +package(default_visibility = ["//visibility:public"]) +cc_library( + name = "cpp", + srcs = glob(["runtime/Cpp/runtime/src/**/*.cpp"]), + hdrs = glob(["runtime/Cpp/runtime/src/**/*.h"]), + includes = ["runtime/Cpp/runtime/src"], +) + """, + sha256 = "46f5e1af5f4bd28ade55cb632f9a069656b31fc8c2408f9aa045f9b5f5caad64", + strip_prefix = "antlr4-4.7.2", + urls = ["https://github.com/antlr/antlr4/archive/4.7.2.tar.gz"], + ) + +def flatbuffers_deps(): + """FlatBuffers support.""" + FLAT_BUFFERS_SHA = "a83caf5910644ba1c421c002ef68e42f21c15f9f" + http_archive( + name = "com_github_google_flatbuffers", + sha256 = "b8efbc25721e76780752bad775a97c3f77a0250271e2db37fc747b20e8b0f24a", + strip_prefix = "flatbuffers-" + FLAT_BUFFERS_SHA, + url = "https://github.com/google/flatbuffers/archive/" + FLAT_BUFFERS_SHA + ".tar.gz", + ) + +def cel_spec_deps(): + """CEL Spec conformance testing.""" + http_archive( + name = "io_bazel_rules_go", + sha256 = "207fad3e6689135c5d8713e5a17ba9d1290238f47b9ba545b63d9303406209c6", + urls = [ + "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.24.7/rules_go-v0.24.7.tar.gz", + "https://github.com/bazelbuild/rules_go/releases/download/v0.24.7/rules_go-v0.24.7.tar.gz", + ], + ) + + http_archive( + name = "bazel_gazelle", + sha256 = "b85f48fa105c4403326e9525ad2b2cc437babaa6e15a3fc0b1dbab0ab064bc7c", + urls = [ + "https://mirror.bazel.build/github.com/bazelbuild/bazel-gazelle/releases/download/v0.22.2/bazel-gazelle-v0.22.2.tar.gz", + "https://github.com/bazelbuild/bazel-gazelle/releases/download/v0.21.2/bazel-gazelle-v0.22.2.tar.gz", + ], + ) + + CEL_SPEC_GIT_SHA = "95fe21a64063d63482a4b1b3159c07b5b7b64d77" # 11/23/2020 + http_archive( + name = "com_google_cel_spec", + strip_prefix = "cel-spec-" + CEL_SPEC_GIT_SHA, + urls = ["https://github.com/google/cel-spec/archive/" + CEL_SPEC_GIT_SHA + ".zip"], + ) + +def cel_cpp_deps(): + """All core dependencies of cel-cpp.""" + base_deps() + parser_deps() + flatbuffers_deps() + cel_spec_deps() diff --git a/bazel/deps_extra.bzl b/bazel/deps_extra.bzl new file mode 100644 index 000000000..06060244f --- /dev/null +++ b/bazel/deps_extra.bzl @@ -0,0 +1,54 @@ +""" +Transitive dependencies. +""" + +load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps") +load("@com_google_googleapis//:repository_rules.bzl", "switched_rules_by_language") +load("@rules_antlr//antlr:deps.bzl", "antlr_dependencies") +load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies") +load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies", "go_repository") + +def cel_spec_deps_extra(): + """CEL Spec dependencies.""" + go_repository( + name = "org_golang_google_genproto", + build_file_proto_mode = "disable_global", + commit = "62d171c70ae133bd47722027b62f8820407cf744", + importpath = "google.golang.org/genproto", + ) + + go_repository( + name = "org_golang_google_grpc", + build_file_proto_mode = "disable_global", + importpath = "google.golang.org/grpc", + tag = "v1.33.2", + ) + + go_repository( + name = "org_golang_x_net", + importpath = "golang.org/x/net", + sum = "h1:oWX7TPOiFAMXLq8o0ikBYfCJVlRHBcsciT5bXOrH628=", + version = "v0.0.0-20190311183353-d8887717615a", + ) + + go_repository( + name = "org_golang_x_text", + importpath = "golang.org/x/text", + sum = "h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=", + version = "v0.3.0", + ) + + go_rules_dependencies() + go_register_toolchains() + gazelle_dependencies() + +def cel_cpp_deps_extra(): + """All transitive dependencies.""" + protobuf_deps() + switched_rules_by_language( + name = "com_google_googleapis_imports", + cc = True, + go = True, # cel-spec requirement + ) + antlr_dependencies(472) + cel_spec_deps_extra() diff --git a/cloudbuild.yaml b/cloudbuild.yaml index de8dee01f..5845145b7 100644 --- a/cloudbuild.yaml +++ b/cloudbuild.yaml @@ -1,5 +1,5 @@ steps: -- name: 'gcr.io/cel-analysis/bazel:3.0.0' +- name: 'gcr.io/cel-analysis/bazel:bionic-3.0.0' entrypoint: bazel args: ['test', '--test_output=errors', '...'] id: bazel-test diff --git a/conformance/BUILD b/conformance/BUILD index 7464686b7..ce8ee59e7 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -53,6 +53,7 @@ cc_binary( testonly = 1, srcs = ["server.cc"], deps = [ + "//base:status_macros", "//eval/public:builtin_func_registrar", "//eval/public:cel_expr_builder_factory", "//eval/public:transform_utility", @@ -60,6 +61,7 @@ cc_binary( "//eval/public/containers:container_backed_map_impl", "//internal:proto_util", "//parser", + "@com_google_absl//absl/flags:flag", "@com_google_absl//absl/flags:parse", "@com_google_absl//absl/strings", "@com_google_cel_spec//proto/test/v1/proto2:test_all_types_cc_proto", @@ -133,6 +135,39 @@ cc_binary( "--skip_test=fields/qualified_identifier_resolution/qualified_ident", "--skip_test=fields/qualified_identifier_resolution/map_field_select", "--skip_test=fields/qualified_identifier_resolution/ident_with_longest_prefix_check", + # New conformance tests awaiting synchronization. + "--skip_test=basic/self_eval_nonzeroish/self_eval_int_hex", + "--skip_test=basic/self_eval_nonzeroish/self_eval_int_hex_negative", + "--skip_test=basic/self_eval_nonzeroish/self_eval_uint_hex", + "--skip_test=basic/functions/unbound", + "--skip_test=basic/functions/unbound_is_runtime_error", + "--skip_test=comparisons/eq_literal/not_eq_list_false_vs_types", + "--skip_test=comparisons/eq_literal/not_eq_map_false_vs_types", + "--skip_test=dynamic/int32", + "--skip_test=dynamic/int64", + "--skip_test=dynamic/uint32", + "--skip_test=dynamic/uint64", + "--skip_test=dynamic/float", + "--skip_test=dynamic/double", + "--skip_test=dynamic/string", + "--skip_test=dynamic/bytes", + "--skip_test=dynamic/bool", + "--skip_test=dynamic/list", + "--skip_test=dynamic/struct", + "--skip_test=dynamic/value_null", + "--skip_test=dynamic/value_number", + "--skip_test=dynamic/value_string", + "--skip_test=dynamic/value_bool", + "--skip_test=dynamic/value_struct", + "--skip_test=dynamic/value_list", + "--skip_test=dynamic/any", + "--skip_test=dynamic/complex", + "--skip_test=enums/legacy_proto2", + "--skip_test=enums/legacy_proto3", + "--skip_test=enums/strong_proto2", + "--skip_test=enums/strong_proto3", + "--skip_test=timestamps/timestamp_range", + "--skip_test=timestamps/duration_range", ] + ["$(location " + test + ")" for test in ALL_TESTS], data = [ ":server", diff --git a/conformance/server.cc b/conformance/server.cc index c5d362d3e..fac49b3df 100644 --- a/conformance/server.cc +++ b/conformance/server.cc @@ -11,6 +11,7 @@ #include "google/protobuf/timestamp.pb.h" #include "google/rpc/code.pb.h" #include "google/protobuf/util/json_util.h" +#include "absl/flags/flag.h" #include "absl/flags/parse.h" #include "absl/strings/str_split.h" #include "eval/public/builtin_func_registrar.h" @@ -22,6 +23,7 @@ #include "parser/parser.h" #include "proto/test/v1/proto2/test_all_types.pb.h" #include "proto/test/v1/proto3/test_all_types.pb.h" +#include "base/status_macros.h" using absl::Status; @@ -105,8 +107,11 @@ class ConformanceServiceImpl { auto eval_status = cel_expression->Evaluate(activation, &arena); if (!eval_status.ok()) { - return Status(StatusCode::kInternal, - std::string(eval_status.status().message())); + *response->mutable_result() + ->mutable_error() + ->add_errors() + ->mutable_message() = eval_status.status().ToString(); + return absl::OkStatus(); } CelValue result = eval_status.value(); diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index 95dee4528..eeb6ccfa7 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -34,8 +34,10 @@ namespace runtime { namespace { +using google::api::expr::v1alpha1::CheckedExpr; using google::api::expr::v1alpha1::Constant; using google::api::expr::v1alpha1::Expr; +using google::api::expr::v1alpha1::Reference; using google::api::expr::v1alpha1::SourceInfo; using Ident = google::api::expr::v1alpha1::Expr::Ident; using Select = google::api::expr::v1alpha1::Expr::Select; diff --git a/eval/compiler/flat_expr_builder_test.cc b/eval/compiler/flat_expr_builder_test.cc index d3d73da55..2e05bbb10 100644 --- a/eval/compiler/flat_expr_builder_test.cc +++ b/eval/compiler/flat_expr_builder_test.cc @@ -33,6 +33,7 @@ namespace runtime { namespace { +using google::api::expr::v1alpha1::CheckedExpr; using google::api::expr::v1alpha1::Expr; using google::api::expr::v1alpha1::SourceInfo; @@ -593,7 +594,7 @@ TEST(FlatExprBuilderTest, CheckedExprActivationMissesReferences) { google::protobuf::Arena arena; auto result_or = cel_expr->Evaluate(activation, &arena); ASSERT_OK(result_or); - CelValue result = result_or.ValueOrDie(); + CelValue result = result_or.value(); ASSERT_TRUE(result.IsError()); EXPECT_THAT( *result.ErrorOrDie(), @@ -608,7 +609,7 @@ TEST(FlatExprBuilderTest, CheckedExprActivationMissesReferences) { activation.InsertValue("bar", CelValue::CreateMap(map_value.get())); result_or = cel_expr->Evaluate(activation, &arena); ASSERT_OK(result_or); - result = result_or.ValueOrDie(); + result = result_or.value(); ASSERT_TRUE(result.IsBool()); EXPECT_FALSE(result.BoolOrDie()); } diff --git a/eval/compiler/qualified_reference_resolver.cc b/eval/compiler/qualified_reference_resolver.cc index d92f9f292..819238226 100644 --- a/eval/compiler/qualified_reference_resolver.cc +++ b/eval/compiler/qualified_reference_resolver.cc @@ -21,6 +21,7 @@ namespace runtime { namespace { +using google::api::expr::v1alpha1::Constant; using google::api::expr::v1alpha1::Expr; using google::api::expr::v1alpha1::Reference; diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index 6d3bdd652..a9e148b97 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -1615,8 +1615,8 @@ TEST_F(BuiltinsTest, TypeComparisons) { paired_values.push_back( {CelValue::CreateMap(&cel_map1), CelValue::CreateMap(&cel_map2)}); - for (int i = 0; i < paired_values.size(); i++) { - for (int j = 0; j < paired_values.size(); j++) { + for (size_t i = 0; i < paired_values.size(); i++) { + for (size_t j = 0; j < paired_values.size(); j++) { CelValue result1; CelValue result2; diff --git a/eval/public/testing/debug_string.cc b/eval/public/testing/debug_string.cc index bf550eccb..5496e11c0 100644 --- a/eval/public/testing/debug_string.cc +++ b/eval/public/testing/debug_string.cc @@ -119,12 +119,12 @@ std::string FunctionCallString(const UnknownFunctionResult* fn) { absl::StrAppend(&call, DebugString(fn->arguments()[0]), "."); } absl::StrAppend(&call, fn->descriptor().name()); - for (int i = 1; i < fn->arguments().size(); i++) { + for (size_t i = 1; i < fn->arguments().size(); i++) { args.push_back(DebugString(fn->arguments()[i])); } } else { absl::StrAppend(&call, fn->descriptor().name()); - for (int i = 0; i < fn->arguments().size(); i++) { + for (size_t i = 0; i < fn->arguments().size(); i++) { args.push_back(DebugString(fn->arguments()[i])); } } diff --git a/eval/tests/BUILD b/eval/tests/BUILD index f53726bca..10f0fb3c6 100644 --- a/eval/tests/BUILD +++ b/eval/tests/BUILD @@ -17,6 +17,7 @@ cc_test( tags = ["manual"], deps = [ ":request_context_cc_proto", + "//base:status_macros", "//eval/public:activation", "//eval/public:builtin_func_registrar", "//eval/public:cel_expr_builder_factory", diff --git a/eval/tests/benchmark_test.cc b/eval/tests/benchmark_test.cc index a7a35f5fc..70db54653 100644 --- a/eval/tests/benchmark_test.cc +++ b/eval/tests/benchmark_test.cc @@ -15,6 +15,7 @@ #include "eval/public/containers/container_backed_list_impl.h" #include "eval/public/structs/cel_proto_wrapper.h" #include "eval/tests/request_context.pb.h" +#include "base/status_macros.h" namespace google { namespace api { @@ -32,7 +33,7 @@ using google::api::expr::v1alpha1::SourceInfo; static void BM_Eval(benchmark::State& state) { auto builder = CreateCelExpressionBuilder(); auto reg_status = RegisterBuiltinFunctions(builder->GetRegistry()); - CHECK_OK(reg_status); + ASSERT_OK(reg_status); int len = state.range(0); @@ -50,7 +51,7 @@ static void BM_Eval(benchmark::State& state) { SourceInfo source_info; auto cel_expr_status = builder->CreateExpression(&root_expr, &source_info); - CHECK_OK(cel_expr_status.status()); + ASSERT_OK(cel_expr_status.status()); std::unique_ptr cel_expr = std::move(cel_expr_status.value()); @@ -58,11 +59,11 @@ static void BM_Eval(benchmark::State& state) { google::protobuf::Arena arena; Activation activation; auto eval_result = cel_expr->Evaluate(activation, &arena); - CHECK_OK(eval_result.status()); + ASSERT_OK(eval_result.status()); CelValue result = eval_result.value(); - GOOGLE_CHECK(result.IsInt64()); - GOOGLE_CHECK(result.Int64OrDie() == len + 1); + ASSERT_TRUE(result.IsInt64()); + ASSERT_TRUE(result.Int64OrDie() == len + 1); } } @@ -74,7 +75,7 @@ BENCHMARK(BM_Eval)->Range(1, 32768); static void BM_EvalString(benchmark::State& state) { auto builder = CreateCelExpressionBuilder(); auto reg_status = RegisterBuiltinFunctions(builder->GetRegistry()); - CHECK_OK(reg_status); + ASSERT_OK(reg_status); int len = state.range(0); @@ -92,7 +93,7 @@ static void BM_EvalString(benchmark::State& state) { SourceInfo source_info; auto cel_expr_status = builder->CreateExpression(&root_expr, &source_info); - CHECK_OK(cel_expr_status.status()); + ASSERT_OK(cel_expr_status.status()); std::unique_ptr cel_expr = std::move(cel_expr_status.value()); @@ -100,11 +101,11 @@ static void BM_EvalString(benchmark::State& state) { google::protobuf::Arena arena; Activation activation; auto eval_result = cel_expr->Evaluate(activation, &arena); - CHECK_OK(eval_result.status()); + ASSERT_OK(eval_result.status()); CelValue result = eval_result.value(); - GOOGLE_CHECK(result.IsString()); - GOOGLE_CHECK(result.StringOrDie().value().size() == len + 1); + ASSERT_TRUE(result.IsString()); + ASSERT_TRUE(result.StringOrDie().value().size() == len + 1); } } @@ -622,7 +623,7 @@ void BM_PolicyNative(benchmark::State& state) { {"ip", kIP}, {"token", kToken}, {"path", kPath}}; for (auto _ : state) { auto result = NativeCheck(attributes, denylists, allowlists); - GOOGLE_CHECK(result); + ASSERT_TRUE(result); } } @@ -649,11 +650,11 @@ void BM_PolicySymbolic(benchmark::State& state) { options.constant_arena = &arena; auto builder = CreateCelExpressionBuilder(options); - CHECK_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); SourceInfo source_info; auto cel_expression_status = builder->CreateExpression(&expr, &source_info); - CHECK_OK(cel_expression_status.status()); + ASSERT_OK(cel_expression_status.status()); auto cel_expression = std::move(cel_expression_status.value()); Activation activation; @@ -663,8 +664,8 @@ void BM_PolicySymbolic(benchmark::State& state) { for (auto _ : state) { auto eval_result = cel_expression->Evaluate(activation, &arena); - CHECK_OK(eval_result.status()); - GOOGLE_CHECK(eval_result.value().BoolOrDie()); + ASSERT_OK(eval_result.status()); + ASSERT_TRUE(eval_result.value().BoolOrDie()); } } @@ -697,11 +698,11 @@ void BM_PolicySymbolicMap(benchmark::State& state) { google::protobuf::TextFormat::ParseFromString(CELAst(), &expr); auto builder = CreateCelExpressionBuilder(); - CHECK_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); SourceInfo source_info; auto cel_expression_status = builder->CreateExpression(&expr, &source_info); - CHECK_OK(cel_expression_status.status()); + ASSERT_OK(cel_expression_status.status()); auto cel_expression = std::move(cel_expression_status.value()); Activation activation; @@ -710,8 +711,8 @@ void BM_PolicySymbolicMap(benchmark::State& state) { for (auto _ : state) { auto eval_result = cel_expression->Evaluate(activation, &arena); - CHECK_OK(eval_result.status()); - GOOGLE_CHECK(eval_result.value().BoolOrDie()); + ASSERT_OK(eval_result.status()); + ASSERT_TRUE(eval_result.value().BoolOrDie()); } } @@ -724,11 +725,11 @@ void BM_PolicySymbolicProto(benchmark::State& state) { google::protobuf::TextFormat::ParseFromString(CELAst(), &expr); auto builder = CreateCelExpressionBuilder(); - CHECK_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); SourceInfo source_info; auto cel_expression_status = builder->CreateExpression(&expr, &source_info); - CHECK_OK(cel_expression_status.status()); + ASSERT_OK(cel_expression_status.status()); auto cel_expression = std::move(cel_expression_status.value()); Activation activation; @@ -741,8 +742,8 @@ void BM_PolicySymbolicProto(benchmark::State& state) { for (auto _ : state) { auto eval_result = cel_expression->Evaluate(activation, &arena); - CHECK_OK(eval_result.status()); - GOOGLE_CHECK(eval_result.value().BoolOrDie()); + ASSERT_OK(eval_result.status()); + ASSERT_TRUE(eval_result.value().BoolOrDie()); } } @@ -801,7 +802,7 @@ void BM_Comprehension(benchmark::State& state) { google::protobuf::Arena arena; Expr expr; Activation activation; - GOOGLE_CHECK(google::protobuf::TextFormat::ParseFromString(kListSum, &expr)); + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kListSum, &expr)); int len = state.range(0); std::vector list; @@ -813,14 +814,14 @@ void BM_Comprehension(benchmark::State& state) { ContainerBackedListImpl cel_list(std::move(list)); activation.InsertValue("list", CelValue::CreateList(&cel_list)); auto builder = CreateCelExpressionBuilder(); - CHECK_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); auto expr_plan = builder->CreateExpression(&expr, nullptr); - CHECK_OK(expr_plan.status()); + ASSERT_OK(expr_plan.status()); for (auto _ : state) { auto result = expr_plan.value()->Evaluate(activation, &arena); - CHECK_OK(result.status()); - GOOGLE_CHECK(result->IsInt64()); - CHECK_EQ(result->Int64OrDie(), len); + ASSERT_OK(result.status()); + ASSERT_TRUE(result->IsInt64()); + ASSERT_EQ(result->Int64OrDie(), len); } } @@ -923,7 +924,7 @@ void BM_NestedComprehension(benchmark::State& state) { google::protobuf::Arena arena; Expr expr; Activation activation; - GOOGLE_CHECK(google::protobuf::TextFormat::ParseFromString(kNestedListSum, &expr)); + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kNestedListSum, &expr)); int len = state.range(0); std::vector list; @@ -935,14 +936,14 @@ void BM_NestedComprehension(benchmark::State& state) { ContainerBackedListImpl cel_list(std::move(list)); activation.InsertValue("list", CelValue::CreateList(&cel_list)); auto builder = CreateCelExpressionBuilder(); - CHECK_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); auto expr_plan = builder->CreateExpression(&expr, nullptr); - CHECK_OK(expr_plan.status()); + ASSERT_OK(expr_plan.status()); for (auto _ : state) { auto result = expr_plan.value()->Evaluate(activation, &arena); - CHECK_OK(result.status()); - GOOGLE_CHECK(result->IsInt64()); - CHECK_EQ(result->Int64OrDie(), len * len); + ASSERT_OK(result.status()); + ASSERT_TRUE(result->IsInt64()); + ASSERT_EQ(result->Int64OrDie(), len * len); } } @@ -969,7 +970,7 @@ void BM_ComprehensionCpp(benchmark::State& state) { }; for (auto _ : state) { int result = op(); - CHECK_EQ(result, len); + ASSERT_EQ(result, len); } } diff --git a/protoutil/type_registry.h b/protoutil/type_registry.h index 9b5a96da4..158ff78bf 100644 --- a/protoutil/type_registry.h +++ b/protoutil/type_registry.h @@ -131,7 +131,7 @@ class TypeRegistry { * callable it is. * * @tparam ProtoType the concrete protobuf message class. - * @tparam C the constructor type. Templitzed to allow inline of lambda + * @tparam C the constructor type. Templatized to allow inline of lambda * invocation. * @returns false if a constructor was previously registered. */