From 00d2a6dc86755604b206683bc307fe0a5af7848a Mon Sep 17 00:00:00 2001 From: kuat Date: Mon, 30 Nov 2020 16:29:32 -0500 Subject: [PATCH 01/12] parser: support hex integers PiperOrigin-RevId: 344877072 --- parser/visitor.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/parser/visitor.cc b/parser/visitor.cc index 037d6b4b6..8a4f1930e 100644 --- a/parser/visitor.cc +++ b/parser/visitor.cc @@ -4,10 +4,10 @@ #include "google/protobuf/struct.pb.h" #include "absl/memory/memory.h" +#include "absl/strings/match.h" #include "absl/strings/numbers.h" #include "absl/strings/str_format.h" #include "absl/strings/str_join.h" -#include "absl/strings/match.h" #include "common/escaping.h" #include "common/operators.h" #include "parser/balancer.h" @@ -421,7 +421,7 @@ antlrcpp::Any ParserVisitor::visitUint(CelParser::UintContext* ctx) { value.resize(value.size() - 1); } int base = 10; - if (absl::StartsWith(value, "0x")) { + if (absl::StartsWith(ctx->tok->getText(), "0x")) { base = 16; } uint64_t uint_value; From 7fe0ffc51a0ec55d2095d24d648bc2e8aa62fd3d Mon Sep 17 00:00:00 2001 From: kuat Date: Thu, 3 Dec 2020 16:17:50 -0500 Subject: [PATCH 02/12] Conformance: respond with errors instead of asserting them. PiperOrigin-RevId: 345520413 --- base/status_macros.h | 4 --- conformance/BUILD | 1 - conformance/server.cc | 62 ++++++++++++++++++++++++++++--------------- 3 files changed, 40 insertions(+), 27 deletions(-) diff --git a/base/status_macros.h b/base/status_macros.h index cac4e8e54..4e7da02e1 100644 --- a/base/status_macros.h +++ b/base/status_macros.h @@ -67,8 +67,4 @@ 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/conformance/BUILD b/conformance/BUILD index 5d8fe7e7a..09ccb5e7f 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -53,7 +53,6 @@ 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", diff --git a/conformance/server.cc b/conformance/server.cc index fac49b3df..40dbd692e 100644 --- a/conformance/server.cc +++ b/conformance/server.cc @@ -23,7 +23,6 @@ #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; @@ -48,10 +47,13 @@ class ConformanceServiceImpl { proto3Tests_(&google::api::expr::test::v1::proto3::TestAllTypes:: default_instance()) {} - Status Parse(const v1alpha1::ParseRequest* request, - v1alpha1::ParseResponse* response) { + void Parse(const v1alpha1::ParseRequest* request, + v1alpha1::ParseResponse* response) { if (request->cel_source().empty()) { - return Status(StatusCode::kInvalidArgument, "No source code."); + auto issue = response->add_issues(); + issue->set_message("No source code"); + issue->set_code(google::rpc::Code::INVALID_ARGUMENT); + return; } auto parse_status = parser::Parse(request->cel_source(), ""); if (!parse_status.ok()) { @@ -63,16 +65,17 @@ class ConformanceServiceImpl { (out).MergeFrom(parse_status.value()); response->mutable_parsed_expr()->CopyFrom(out); } - return absl::OkStatus(); } - Status Check(const v1alpha1::CheckRequest* request, - v1alpha1::CheckResponse* response) { - return Status(StatusCode::kUnimplemented, "Check is not supported"); + void Check(const v1alpha1::CheckRequest* request, + v1alpha1::CheckResponse* response) { + auto issue = response->add_issues(); + issue->set_message("Check is not supported"); + issue->set_code(google::rpc::Code::UNIMPLEMENTED); } - Status Eval(const v1alpha1::EvalRequest* request, - v1alpha1::EvalResponse* response) { + void Eval(const v1alpha1::EvalRequest* request, + v1alpha1::EvalResponse* response) { const v1alpha1::Expr* expr = nullptr; if (request->has_parsed_expr()) { expr = &request->parsed_expr().expr(); @@ -87,8 +90,10 @@ class ConformanceServiceImpl { auto cel_expression_status = builder_->CreateExpression(&out, &source_info); if (!cel_expression_status.ok()) { - return Status(StatusCode::kInternal, - std::string(cel_expression_status.status().message())); + auto issue = response->add_issues(); + issue->set_message(cel_expression_status.status().ToString()); + issue->set_code(google::rpc::Code::INTERNAL); + return; } auto cel_expression = std::move(cel_expression_status.value()); @@ -100,7 +105,10 @@ class ConformanceServiceImpl { (*import_value).MergeFrom(pair.second.value()); auto import_status = ValueToCelValue(*import_value, &arena); if (!import_status.ok()) { - return Status(StatusCode::kInternal, import_status.status().ToString()); + auto issue = response->add_issues(); + issue->set_message(import_status.status().ToString()); + issue->set_code(google::rpc::Code::INTERNAL); + return; } activation.InsertValue(pair.first, import_status.value()); } @@ -111,7 +119,7 @@ class ConformanceServiceImpl { ->mutable_error() ->add_errors() ->mutable_message() = eval_status.status().ToString(); - return absl::OkStatus(); + return; } CelValue result = eval_status.value(); @@ -124,12 +132,14 @@ class ConformanceServiceImpl { google::api::expr::v1alpha1::Value export_value; auto export_status = CelValueToValue(result, &export_value); if (!export_status.ok()) { - return Status(StatusCode::kInternal, export_status.ToString()); + auto issue = response->add_issues(); + issue->set_message(export_status.ToString()); + issue->set_code(google::rpc::Code::INTERNAL); + return; } auto* result_value = response->mutable_result()->mutable_value(); (*result_value).MergeFrom(export_value); } - return absl::OkStatus(); } private: @@ -178,15 +188,23 @@ int RunServer(bool optimize) { 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)); + if (!JsonStringToMessage(input, &request).ok()) { + std::cerr << "Failed to parse JSON" << std::endl; + } + service.Parse(&request, &response); + if (!MessageToJsonString(response, &output).ok()) { + std::cerr << "Failed to convert to JSON" << std::endl; + } } 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)); + if (!JsonStringToMessage(input, &request).ok()) { + std::cerr << "Failed to parse JSON" << std::endl; + } + service.Eval(&request, &response); + if (!MessageToJsonString(response, &output).ok()) { + std::cerr << "Failed to convert to JSON" << std::endl; + } } else if (cmd.empty()) { return 0; } else { From 151ba74fe554948c4bb3c3122ad00126a76c3894 Mon Sep 17 00:00:00 2001 From: kuat Date: Mon, 7 Dec 2020 13:49:34 -0500 Subject: [PATCH 03/12] Division by zero for doubles requires IEEE754 support. Add a static assert. PiperOrigin-RevId: 346129050 --- eval/public/builtin_func_registrar.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/eval/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc index 473d398d2..cbac5e33f 100644 --- a/eval/public/builtin_func_registrar.cc +++ b/eval/public/builtin_func_registrar.cc @@ -438,6 +438,9 @@ CelValue Div(Arena* arena, uint64_t v0, uint64_t v1) { template <> CelValue Div(Arena*, double v0, double v1) { + static_assert(std::numeric_limits::is_iec559, + "Division by zero for doubles must be supported"); + // For double, division will result in +/- inf return CelValue::CreateDouble(v0 / v1); } From bb7fd0456988e368cdefced58d398a2f09de2416 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Thu, 10 Dec 2020 01:25:16 -0500 Subject: [PATCH 04/12] Stabilize concatenation order of messages from errors in SourceFactory::errorMessage PiperOrigin-RevId: 346711382 --- parser/parser_test.cc | 15 +++++------ parser/source_factory.cc | 57 +++++++++++++++++++++++++++++++++------- 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/parser/parser_test.cc b/parser/parser_test.cc index d06899da2..81f8e0324 100644 --- a/parser/parser_test.cc +++ b/parser/parser_test.cc @@ -21,7 +21,6 @@ namespace parser { namespace { using ::google::api::expr::v1alpha1::Expr; -using ::google::api::expr::v1alpha1::ParsedExpr; using testing::Not; struct TestInfo { @@ -370,15 +369,15 @@ std::vector test_cases = { // Parse error tests {"*@a | b", "", - "ERROR: :1:2: Syntax error: token recognition error at: '@'\n" - " | *@a | b\n" - " | .^\n" "ERROR: :1:1: Syntax error: extraneous input '*' expecting {'[', " "'{', " "'(', '.', '-', '!', 'true', 'false', 'null', NUM_FLOAT, NUM_INT, " "NUM_UINT, STRING, BYTES, IDENTIFIER}\n" " | *@a | b\n" " | ^\n" + "ERROR: :1:2: Syntax error: token recognition error at: '@'\n" + " | *@a | b\n" + " | .^\n" "ERROR: :1:5: Syntax error: token recognition error at: '| '\n" " | *@a | b\n" " | ....^\n" @@ -800,15 +799,15 @@ std::vector test_cases = { "ERROR: :1:15: reserved identifier: var\n" " | [1, 2, 3].map(var, var * var)\n" " | ..............^\n" + "ERROR: :1:15: argument is not an identifier\n" + " | [1, 2, 3].map(var, var * var)\n" + " | ..............^\n" "ERROR: :1:20: reserved identifier: var\n" " | [1, 2, 3].map(var, var * var)\n" " | ...................^\n" "ERROR: :1:26: reserved identifier: var\n" " | [1, 2, 3].map(var, var * var)\n" - " | .........................^\n" - "ERROR: :1:15: argument is not an identifier\n" - " | [1, 2, 3].map(var, var * var)\n" - " | ..............^"}, + " | .........................^"}, {"[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[" "[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[" "[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[" diff --git a/parser/source_factory.cc b/parser/source_factory.cc index c8fea90ec..92990a4a8 100644 --- a/parser/source_factory.cc +++ b/parser/source_factory.cc @@ -1,5 +1,8 @@ #include "parser/source_factory.h" +#include +#include + #include "google/protobuf/struct.pb.h" #include "absl/memory/memory.h" #include "absl/strings/numbers.h" @@ -19,6 +22,10 @@ const int kMaxErrorsToReport = 100; using common::CelOperator; using google::api::expr::v1alpha1::Expr; +int32_t PositiveOrMax(int32_t value) { + return value >= 0 ? value : std::numeric_limits::max(); +} + } // namespace SourceFactory::SourceFactory(const std::string& expression) @@ -440,30 +447,60 @@ Expr SourceFactory::reportError(const SourceFactory::SourceLocation& loc, std::string SourceFactory::errorMessage(const std::string& description, const std::string& expression) const { + // Errors are collected as they are encountered, not by their location within + // the source. To have a more stable error message as implementation + // details change, we sort the collected errors by their source location + // first. + + // Use pointer arithmetic to avoid making unnecessary copies of Error when + // sorting. + std::vector errors_sorted; + errors_sorted.reserve(errors_truncated_.size()); + for (auto& error : errors_truncated_) { + errors_sorted.push_back(&error); + } + std::stable_sort(errors_sorted.begin(), errors_sorted.end(), + [](const Error* lhs, const Error* rhs) { + // SourceLocation::noLocation uses -1 and we ideally want + // those to be last. + auto lhs_line = PositiveOrMax(lhs->location.line); + auto lhs_col = PositiveOrMax(lhs->location.col); + auto rhs_line = PositiveOrMax(rhs->location.line); + auto rhs_col = PositiveOrMax(rhs->location.col); + + return lhs_line < rhs_line || + (lhs_line == rhs_line && lhs_col < rhs_col); + }); + + // Build the summary error message using the sorted errors. + bool errors_truncated = num_errors_ > kMaxErrorsToReport; std::vector messages; + messages.reserve( + errors_sorted.size() + + errors_truncated); // Reserve space for the transform and an + // additional element when truncation occurs. std::transform( - errors_truncated_.begin(), errors_truncated_.end(), - std::back_inserter(messages), - [this, description, expression](const SourceFactory::Error& error) { - std::string s = absl::StrFormat("ERROR: %s:%zu:%zu: %s", description, - error.location.line, - // add one to the 0-based column - error.location.col + 1, error.message); - std::string snippet = getSourceLine(error.location.line, expression); + errors_sorted.begin(), errors_sorted.end(), std::back_inserter(messages), + [this, &description, &expression](const SourceFactory::Error* error) { + std::string s = absl::StrFormat( + "ERROR: %s:%zu:%zu: %s", description, error->location.line, + // add one to the 0-based column + error->location.col + 1, error->message); + std::string snippet = getSourceLine(error->location.line, expression); std::string::size_type pos = 0; while ((pos = snippet.find("\t", pos)) != std::string::npos) { snippet.replace(pos, 1, " "); } std::string src_line = "\n | " + snippet; std::string ind_line = "\n | "; - for (int i = 0; i < error.location.col; ++i) { + for (int i = 0; i < error->location.col; ++i) { ind_line += "."; } ind_line += "^"; s += src_line + ind_line; return s; }); - if (num_errors_ > kMaxErrorsToReport) { + if (errors_truncated) { messages.emplace_back(absl::StrCat(num_errors_ - kMaxErrorsToReport, " more errors were truncated.")); } From ae1cecd66c96642fb9675c722fe4706c71e80097 Mon Sep 17 00:00:00 2001 From: tswadell Date: Thu, 10 Dec 2020 14:25:16 -0500 Subject: [PATCH 05/12] Implementation of the uint() conversion functions PiperOrigin-RevId: 346826656 --- conformance/BUILD | 8 +- conformance/server.cc | 2 +- eval/public/builtin_func_registrar.cc | 101 ++++++++++++++++++++------ eval/public/builtin_func_test.cc | 80 ++++++++++++++++---- eval/public/cel_builtins.h | 1 + 5 files changed, 150 insertions(+), 42 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index 09ccb5e7f..94d76c8cb 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -93,16 +93,14 @@ cc_binary( "--skip_test=conversions/double", # TODO(issues/80): Missing dyn() conversion functions "--skip_test=conversions/dyn/dyn_heterogeneous_list", - # TODO(issues/81): Conversion functions for int() which can be - # uncommented when the spec changes to truncation rather than - # rounding. + # TODO(issues/81): Conversion functions for int(), uint() which can be + # uncommented when the spec changes to truncation rather than rounding. "--skip_test=conversions/int/double_nearest,double_nearest_neg,double_half_away_neg,double_half_away_pos", + "--skip_test=conversions/uint/double_nearest,double_nearest_int,double_half_away", # TODO(issues/82): Unexpected behavior when converting invalid bytes to string. "--skip_test=conversions/string/bytes_invalid", # TODO(issues/83): Missing type() conversion functions "--skip_test=conversions/type", - # TODO(issues/84): Missing uint() conversion functions - "--skip_test=conversions/uint", # TODO(issues/96): Well-known type conversion support. "--skip_test=proto2/literal_wellknown", "--skip_test=proto3/literal_wellknown", diff --git a/conformance/server.cc b/conformance/server.cc index 40dbd692e..5022c668a 100644 --- a/conformance/server.cc +++ b/conformance/server.cc @@ -40,7 +40,7 @@ namespace runtime { class ConformanceServiceImpl { public: - ConformanceServiceImpl(std::unique_ptr builder) + explicit ConformanceServiceImpl(std::unique_ptr builder) : builder_(std::move(builder)), proto2Tests_(&google::api::expr::test::v1::proto2::TestAllTypes:: default_instance()), diff --git a/eval/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc index cbac5e33f..33109facb 100644 --- a/eval/public/builtin_func_registrar.cc +++ b/eval/public/builtin_func_registrar.cc @@ -67,63 +67,63 @@ bool GreaterThanOrEqual(Arena* arena, Type t1, Type t2) { // Duration comparison specializations template <> CelValue Inequal(Arena*, absl::Duration t1, absl::Duration t2) { - return CelValue::CreateBool(operator!=(t1, t2)); + return CelValue::CreateBool(absl::operator!=(t1, t2)); } template <> CelValue Equal(Arena*, absl::Duration t1, absl::Duration t2) { - return CelValue::CreateBool(operator==(t1, t2)); + return CelValue::CreateBool(absl::operator==(t1, t2)); } template <> bool LessThan(Arena*, absl::Duration t1, absl::Duration t2) { - return operator<(t1, t2); + return absl::operator<(t1, t2); } template <> bool LessThanOrEqual(Arena*, absl::Duration t1, absl::Duration t2) { - return operator<=(t1, t2); + return absl::operator<=(t1, t2); } template <> bool GreaterThan(Arena*, absl::Duration t1, absl::Duration t2) { - return operator>(t1, t2); + return absl::operator>(t1, t2); } template <> bool GreaterThanOrEqual(Arena*, absl::Duration t1, absl::Duration t2) { - return operator>=(t1, t2); + return absl::operator>=(t1, t2); } // Timestamp comparison specializations template <> CelValue Inequal(Arena*, absl::Time t1, absl::Time t2) { - return CelValue::CreateBool(operator!=(t1, t2)); + return CelValue::CreateBool(absl::operator!=(t1, t2)); } template <> CelValue Equal(Arena*, absl::Time t1, absl::Time t2) { - return CelValue::CreateBool(operator==(t1, t2)); + return CelValue::CreateBool(absl::operator==(t1, t2)); } template <> bool LessThan(Arena*, absl::Time t1, absl::Time t2) { - return operator<(t1, t2); + return absl::operator<(t1, t2); } template <> bool LessThanOrEqual(Arena*, absl::Time t1, absl::Time t2) { - return operator<=(t1, t2); + return absl::operator<=(t1, t2); } template <> bool GreaterThan(Arena*, absl::Time t1, absl::Time t2) { - return operator>(t1, t2); + return absl::operator>(t1, t2); } template <> bool GreaterThanOrEqual(Arena*, absl::Time t1, absl::Time t2) { - return operator>=(t1, t2); + return absl::operator>=(t1, t2); } // Message specializations @@ -593,7 +593,7 @@ const absl::Status FindTimeBreakdown(absl::Time timestamp, absl::string_view tz, } // Check to see whether the timezone is an IANA timezone. - if (absl::LoadTimeZone(std::string(tz), &time_zone)) { + if (absl::LoadTimeZone(tz, &time_zone)) { *breakdown = time_zone.At(timestamp); return absl::OkStatus(); } @@ -632,8 +632,7 @@ CelValue GetTimeBreakdownPart( CelValue CreateTimestampFromString(Arena* arena, CelValue::StringHolder time_str) { absl::Time ts; - if (!absl::ParseTime(absl::RFC3339_full, std::string(time_str.value()), &ts, - nullptr)) { + if (!absl::ParseTime(absl::RFC3339_full, time_str.value(), &ts, nullptr)) { return CreateErrorValue(arena, "String to Timestamp conversion failed", absl::StatusCode::kInvalidArgument); } @@ -725,7 +724,7 @@ CelValue GetMilliseconds(Arena* arena, absl::Time timestamp, CelValue CreateDurationFromString(Arena* arena, CelValue::StringHolder dur_str) { absl::Duration d; - if (!absl::ParseDuration(std::string(dur_str.value()), &d)) { + if (!absl::ParseDuration(dur_str.value(), &d)) { return CreateErrorValue(arena, "String to Duration conversion failed", absl::StatusCode::kInvalidArgument); } @@ -1421,39 +1420,47 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, // type conversion to int // TODO(issues/26): To return errors on loss of precision // (overflow/underflow) by returning StatusOr. + + // time -> int status = FunctionAdapter::CreateAndRegister( builtin::kInt, false, [](Arena*, absl::Time t) { return absl::ToUnixSeconds(t); }, registry); if (!status.ok()) return status; + // double -> int status = FunctionAdapter::CreateAndRegister( builtin::kInt, false, [](Arena* arena, double v) { - if ((v > (double)kIntMax) || (v < (double)kIntMin)) { + if ((v > static_cast(kIntMax)) || + (v < static_cast(kIntMin))) { return CreateErrorValue(arena, "double out of int range", absl::StatusCode::kInvalidArgument); } - return CelValue::CreateInt64((int64_t)v); + return CelValue::CreateInt64(static_cast(v)); }, registry); if (!status.ok()) return status; + // bool -> int status = FunctionAdapter::CreateAndRegister( - builtin::kInt, false, [](Arena*, bool v) { return (int64_t)v; }, registry); + builtin::kInt, false, + [](Arena*, bool v) { return static_cast(v); }, registry); if (!status.ok()) return status; + // uint -> int status = FunctionAdapter::CreateAndRegister( builtin::kInt, false, [](Arena* arena, uint64_t v) { - if (v > (uint64_t)kIntMax) { + if (v > static_cast(kIntMax)) { return CreateErrorValue(arena, "uint out of int range", absl::StatusCode::kInvalidArgument); } - return CelValue::CreateInt64((int64_t)v); + return CelValue::CreateInt64(static_cast(v)); }, registry); if (!status.ok()) return status; + // string -> int status = FunctionAdapter::CreateAndRegister( builtin::kInt, false, [](Arena* arena, CelValue::StringHolder s) { @@ -1468,6 +1475,58 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, registry); if (!status.ok()) return status; + // int -> int + status = FunctionAdapter::CreateAndRegister( + builtin::kInt, false, [](Arena*, int64_t v) { return v; }, registry); + if (!status.ok()) return status; + + // type conversion to uint + // double -> uint + status = FunctionAdapter::CreateAndRegister( + builtin::kUint, false, + [](Arena* arena, double v) { + if ((v > static_cast(kUintMax)) || (v < 0)) { + return CreateErrorValue(arena, "double out of uint range", + absl::StatusCode::kInvalidArgument); + } + return CelValue::CreateUint64(static_cast(v)); + }, + registry); + if (!status.ok()) return status; + + // int -> uint + status = FunctionAdapter::CreateAndRegister( + builtin::kUint, false, + [](Arena* arena, int64_t v) { + if (v < 0) { + return CreateErrorValue(arena, "int out of uint range", + absl::StatusCode::kInvalidArgument); + } + return CelValue::CreateUint64(static_cast(v)); + }, + registry); + if (!status.ok()) return status; + + // string -> uint + status = FunctionAdapter::CreateAndRegister( + builtin::kUint, false, + [](Arena* arena, CelValue::StringHolder s) { + uint64_t result; + if (absl::SimpleAtoi(s.value(), &result)) { + return CelValue::CreateUint64(result); + } else { + return CreateErrorValue(arena, "doesn't convert to a string", + absl::StatusCode::kInvalidArgument); + } + }, + registry); + if (!status.ok()) return status; + + // uint -> uint + status = FunctionAdapter::CreateAndRegister( + builtin::kUint, false, [](Arena*, uint64_t v) { return v; }, registry); + if (!status.ok()) return status; + // duration // duration() conversion from string.. diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index a9e148b97..162244c95 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -132,6 +132,17 @@ class BuiltinsTest : public ::testing::Test { << operation << " for " << CelValue::TypeName(ref.type()); } + void TestTypeConverts(absl::string_view operation, const CelValue& ref, + uint64_t result) { + CelValue result_value; + + ASSERT_NO_FATAL_FAILURE(PerformRun(operation, {}, {ref}, &result_value)); + + ASSERT_EQ(result_value.IsUint64(), true); + ASSERT_EQ(result_value.Uint64OrDie(), result) + << operation << " for " << CelValue::TypeName(ref.type()); + } + // Helper method. Attempts to perform a type conversion and expects an error // as the result. void TestTypeConversionError(absl::string_view operation, @@ -671,37 +682,85 @@ TEST_F(BuiltinsTest, TestTimestampFunctions) { 3L); } -TEST_F(BuiltinsTest, TestTypeConversions_Timestamp) { +TEST_F(BuiltinsTest, TestIntConversions_int) { + TestTypeConverts(builtin::kInt, CelValue::CreateInt64(100L), 100L); +} + +TEST_F(BuiltinsTest, TestIntConversions_Timestamp) { Timestamp ref; ref.set_seconds(100); TestTypeConverts(builtin::kInt, CelProtoWrapper::CreateTimestamp(&ref), 100L); } -TEST_F(BuiltinsTest, TestTypeConversions_double) { +TEST_F(BuiltinsTest, TestIntConversions_double) { double ref = 100.1; TestTypeConverts(builtin::kInt, CelValue::CreateDouble(ref), 100L); } -TEST_F(BuiltinsTest, TestTypeConversions_uint64) { +TEST_F(BuiltinsTest, TestIntConversions_string) { + std::string ref = "100"; + TestTypeConverts(builtin::kInt, CelValue::CreateString(&ref), 100L); +} + +TEST_F(BuiltinsTest, TestIntConversions_uint) { uint64_t ref = 100; TestTypeConverts(builtin::kInt, CelValue::CreateUint64(ref), 100L); } -TEST_F(BuiltinsTest, TestTypeConversionError_doubleNegRange) { +TEST_F(BuiltinsTest, TestIntConversionError_doubleNegRange) { double range = -1.0e99; TestTypeConversionError(builtin::kInt, CelValue::CreateDouble(range)); } -TEST_F(BuiltinsTest, TestTypeConversionError_doublePosRange) { +TEST_F(BuiltinsTest, TestIntConversionError_doublePosRange) { double range = 1.0e99; TestTypeConversionError(builtin::kInt, CelValue::CreateDouble(range)); } -TEST_F(BuiltinsTest, TestTypeConversionError_uint64Range) { +TEST_F(BuiltinsTest, TestIntConversionError_uintRange) { uint64_t range = 18446744073709551615UL; TestTypeConversionError(builtin::kInt, CelValue::CreateUint64(range)); } +TEST_F(BuiltinsTest, TestUintConversions_double) { + double ref = 100.1; + TestTypeConverts(builtin::kUint, CelValue::CreateDouble(ref), 100UL); +} + +TEST_F(BuiltinsTest, TestUintConversions_int) { + int64_t ref = 100L; + TestTypeConverts(builtin::kUint, CelValue::CreateInt64(ref), 100UL); +} + +TEST_F(BuiltinsTest, TestUintConversions_string) { + std::string ref = "100"; + TestTypeConverts(builtin::kUint, CelValue::CreateString(&ref), 100UL); +} + +TEST_F(BuiltinsTest, TestUintConversions_uint) { + TestTypeConverts(builtin::kUint, CelValue::CreateUint64(100UL), 100UL); +} + +TEST_F(BuiltinsTest, TestUintConversionError_doubleNegRange) { + double range = -1.0e99; + TestTypeConversionError(builtin::kUint, CelValue::CreateDouble(range)); +} + +TEST_F(BuiltinsTest, TestUintConversionError_doublePosRange) { + double range = 1.0e99; + TestTypeConversionError(builtin::kUint, CelValue::CreateDouble(range)); +} + +TEST_F(BuiltinsTest, TestUintConversionError_intRange) { + int64_t range = -1L; + TestTypeConversionError(builtin::kUint, CelValue::CreateInt64(range)); +} + +TEST_F(BuiltinsTest, TestUintConversionError_stringInvalid) { + string invalid = "-100"; + TestTypeConversionError(builtin::kUint, CelValue::CreateString(&invalid)); +} + TEST_F(BuiltinsTest, TestTimestampComparisons) { Timestamp ref; Timestamp lesser; @@ -1517,15 +1576,6 @@ TEST_F(BuiltinsTest, MatchesMaxSize) { EXPECT_TRUE(result_value.IsError()); } -TEST_F(BuiltinsTest, StringToInt) { - std::string target = "-42"; - std::vector args = {CelValue::CreateString(&target)}; - CelValue result_value; - ASSERT_NO_FATAL_FAILURE(PerformRun(builtin::kInt, {}, args, &result_value)); - ASSERT_TRUE(result_value.IsInt64()); - EXPECT_EQ(result_value.Int64OrDie(), -42); -} - TEST_F(BuiltinsTest, StringToIntNonInt) { std::string target = "not_a_number"; std::vector args = {CelValue::CreateString(&target)}; diff --git a/eval/public/cel_builtins.h b/eval/public/cel_builtins.h index bff10e819..9dd2dec31 100644 --- a/eval/public/cel_builtins.h +++ b/eval/public/cel_builtins.h @@ -74,6 +74,7 @@ constexpr char kMilliseconds[] = "getMilliseconds"; constexpr char kInt[] = "int"; constexpr char kString[] = "string"; constexpr char kType[] = "type"; +constexpr char kUint[] = "uint"; } // namespace builtin From f9988db43c49a3e87e5c68ca04e6060045937863 Mon Sep 17 00:00:00 2001 From: tswadell Date: Thu, 10 Dec 2020 15:26:43 -0500 Subject: [PATCH 06/12] Implementation of double() type conversion functions PiperOrigin-RevId: 346840645 --- conformance/BUILD | 6 +- eval/public/builtin_func_registrar.cc | 422 +++++++++++++++----------- eval/public/builtin_func_test.cc | 36 +++ eval/public/cel_builtins.h | 1 + 4 files changed, 285 insertions(+), 180 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index 94d76c8cb..e76545697 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -89,8 +89,6 @@ cc_binary( "--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 - "--skip_test=conversions/double", # TODO(issues/80): Missing dyn() conversion functions "--skip_test=conversions/dyn/dyn_heterogeneous_list", # TODO(issues/81): Conversion functions for int(), uint() which can be @@ -113,13 +111,11 @@ cc_binary( "--skip_test=namespace/namespace/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", - "--skip_test=comparisons/ne_literal/not_ne_bytes", "--skip_test=comparisons/in_list_literal/elem_in_mixed_type_list_error", "--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/int64_field_select_unsupported,list_field_select_unsupported,map_key_null,qualified_identifier_resolution_unchecked", + "--skip_test=fields/qualified_identifier_resolution/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/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc index 33109facb..ddf67b330 100644 --- a/eval/public/builtin_func_registrar.cc +++ b/eval/public/builtin_func_registrar.cc @@ -13,6 +13,7 @@ #include "eval/public/cel_builtins.h" #include "eval/public/cel_function_adapter.h" #include "eval/public/cel_function_registry.h" +#include "eval/public/cel_options.h" #include "eval/public/containers/container_backed_list_impl.h" #include "re2/re2.h" @@ -849,15 +850,7 @@ absl::Status RegisterStringFunctions(CelFunctionRegistry* registry, absl::Status RegisterTimestampFunctions(CelFunctionRegistry* registry, const InterpreterOptions& options) { - // Timestamp - // - // timestamp() conversion from string.. - auto status = - FunctionAdapter::CreateAndRegister( - builtin::kTimestamp, false, CreateTimestampFromString, registry); - if (!status.ok()) return status; - - status = FunctionAdapter:: + auto status = FunctionAdapter:: CreateAndRegister( builtin::kFullYear, true, [](Arena* arena, absl::Time ts, CelValue::StringHolder tz) @@ -1020,6 +1013,247 @@ absl::Status RegisterTimestampFunctions(CelFunctionRegistry* registry, return absl::OkStatus(); } +absl::Status RegisterDoubleConversionFunctions(CelFunctionRegistry* registry, + const InterpreterOptions&) { + // double -> double + auto status = FunctionAdapter::CreateAndRegister( + builtin::kDouble, false, [](Arena*, double v) { return v; }, registry); + if (!status.ok()) return status; + + // int -> double + status = FunctionAdapter::CreateAndRegister( + builtin::kDouble, false, + [](Arena*, int64_t v) { return static_cast(v); }, registry); + if (!status.ok()) return status; + + // string -> double + status = FunctionAdapter::CreateAndRegister( + builtin::kDouble, false, + [](Arena* arena, CelValue::StringHolder s) { + double result; + if (absl::SimpleAtod(s.value(), &result)) { + return CelValue::CreateDouble(result); + } else { + return CreateErrorValue(arena, "cannot convert string to double", + absl::StatusCode::kInvalidArgument); + } + }, + registry); + if (!status.ok()) return status; + + // uint -> double + status = FunctionAdapter::CreateAndRegister( + builtin::kDouble, false, + [](Arena*, uint64_t v) { return static_cast(v); }, registry); + if (!status.ok()) return status; + + return absl::OkStatus(); +} + +absl::Status RegisterIntConversionFunctions(CelFunctionRegistry* registry, + const InterpreterOptions&) { + // bool -> int + auto status = FunctionAdapter::CreateAndRegister( + builtin::kInt, false, + [](Arena*, bool v) { return static_cast(v); }, registry); + if (!status.ok()) return status; + + // double -> int + status = FunctionAdapter::CreateAndRegister( + builtin::kInt, false, + [](Arena* arena, double v) { + if ((v > static_cast(kIntMax)) || + (v < static_cast(kIntMin))) { + return CreateErrorValue(arena, "double out of int range", + absl::StatusCode::kInvalidArgument); + } + return CelValue::CreateInt64(static_cast(v)); + }, + registry); + if (!status.ok()) return status; + + // int -> int + status = FunctionAdapter::CreateAndRegister( + builtin::kInt, false, [](Arena*, int64_t v) { return v; }, registry); + if (!status.ok()) return status; + + // string -> int + status = FunctionAdapter::CreateAndRegister( + builtin::kInt, false, + [](Arena* arena, CelValue::StringHolder s) { + int64_t result; + if (absl::SimpleAtoi(s.value(), &result)) { + return CelValue::CreateInt64(result); + } else { + return CreateErrorValue(arena, "cannot convert string to int", + absl::StatusCode::kInvalidArgument); + } + }, + registry); + if (!status.ok()) return status; + + // time -> int + status = FunctionAdapter::CreateAndRegister( + builtin::kInt, false, + [](Arena*, absl::Time t) { return absl::ToUnixSeconds(t); }, registry); + if (!status.ok()) return status; + + // uint -> int + status = FunctionAdapter::CreateAndRegister( + builtin::kInt, false, + [](Arena* arena, uint64_t v) { + if (v > static_cast(kIntMax)) { + return CreateErrorValue(arena, "uint out of int range", + absl::StatusCode::kInvalidArgument); + } + return CelValue::CreateInt64(static_cast(v)); + }, + registry); + if (!status.ok()) return status; + + return absl::OkStatus(); +} + +absl::Status RegisterStringConversionFunctions( + CelFunctionRegistry* registry, const InterpreterOptions& options) { + // May be optionally disabled to reduce potential allocs. + if (!options.enable_string_conversion) { + return absl::OkStatus(); + } + + // bytes -> string + auto status = FunctionAdapter:: + CreateAndRegister( + builtin::kString, false, + [](Arena* arena, + CelValue::BytesHolder value) -> CelValue::StringHolder { + return CelValue::StringHolder( + Arena::Create(arena, std::string(value.value()))); + }, + registry); + if (!status.ok()) return status; + + // double -> string + status = FunctionAdapter::CreateAndRegister( + builtin::kString, false, + [](Arena* arena, double value) -> CelValue::StringHolder { + return CelValue::StringHolder( + Arena::Create(arena, absl::StrCat(value))); + }, + registry); + if (!status.ok()) return status; + + // int -> string + status = FunctionAdapter::CreateAndRegister( + builtin::kString, false, + [](Arena* arena, int64_t value) -> CelValue::StringHolder { + return CelValue::StringHolder( + Arena::Create(arena, absl::StrCat(value))); + }, + registry); + if (!status.ok()) return status; + + // string -> string + status = FunctionAdapter:: + CreateAndRegister( + builtin::kString, false, + [](Arena*, CelValue::StringHolder value) -> CelValue::StringHolder { + return value; + }, + registry); + if (!status.ok()) return status; + + // uint -> string + status = FunctionAdapter::CreateAndRegister( + builtin::kString, false, + [](Arena* arena, uint64_t value) -> CelValue::StringHolder { + return CelValue::StringHolder( + Arena::Create(arena, absl::StrCat(value))); + }, + registry); + if (!status.ok()) return status; + + return absl::OkStatus(); +} + +absl::Status RegisterUintConversionFunctions(CelFunctionRegistry* registry, + const InterpreterOptions&) { + // double -> uint + auto status = FunctionAdapter::CreateAndRegister( + builtin::kUint, false, + [](Arena* arena, double v) { + if ((v > static_cast(kUintMax)) || (v < 0)) { + return CreateErrorValue(arena, "double out of uint range", + absl::StatusCode::kInvalidArgument); + } + return CelValue::CreateUint64(static_cast(v)); + }, + registry); + if (!status.ok()) return status; + + // int -> uint + status = FunctionAdapter::CreateAndRegister( + builtin::kUint, false, + [](Arena* arena, int64_t v) { + if (v < 0) { + return CreateErrorValue(arena, "int out of uint range", + absl::StatusCode::kInvalidArgument); + } + return CelValue::CreateUint64(static_cast(v)); + }, + registry); + if (!status.ok()) return status; + + // string -> uint + status = FunctionAdapter::CreateAndRegister( + builtin::kUint, false, + [](Arena* arena, CelValue::StringHolder s) { + uint64_t result; + if (absl::SimpleAtoi(s.value(), &result)) { + return CelValue::CreateUint64(result); + } else { + return CreateErrorValue(arena, "doesn't convert to a string", + absl::StatusCode::kInvalidArgument); + } + }, + registry); + if (!status.ok()) return status; + + // uint -> uint + status = FunctionAdapter::CreateAndRegister( + builtin::kUint, false, [](Arena*, uint64_t v) { return v; }, registry); + if (!status.ok()) return status; + + return absl::OkStatus(); +} + +absl::Status RegisterConversionFunctions(CelFunctionRegistry* registry, + const InterpreterOptions& options) { + auto status = RegisterDoubleConversionFunctions(registry, options); + if (!status.ok()) return status; + + // duration() conversion from string. + status = FunctionAdapter::CreateAndRegister( + builtin::kDuration, false, CreateDurationFromString, registry); + if (!status.ok()) return status; + + status = RegisterIntConversionFunctions(registry, options); + if (!status.ok()) return status; + + status = RegisterStringConversionFunctions(registry, options); + if (!status.ok()) return status; + + // timestamp() conversion from string. + status = FunctionAdapter::CreateAndRegister( + builtin::kTimestamp, false, CreateTimestampFromString, registry); + if (!status.ok()) return status; + + status = RegisterUintConversionFunctions(registry, options); + if (!status.ok()) return status; + + return absl::OkStatus(); +} + } // namespace absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, @@ -1052,6 +1286,9 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, status = RegisterComparisonFunctions(registry, options); if (!status.ok()) return status; + status = RegisterConversionFunctions(registry, options); + if (!status.ok()) return status; + // Strictness status = FunctionAdapter::CreateAndRegister( builtin::kNotStrictlyFalse, false, @@ -1417,123 +1654,7 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, status = RegisterTimestampFunctions(registry, options); if (!status.ok()) return status; - // type conversion to int - // TODO(issues/26): To return errors on loss of precision - // (overflow/underflow) by returning StatusOr. - - // time -> int - status = FunctionAdapter::CreateAndRegister( - builtin::kInt, false, - [](Arena*, absl::Time t) { return absl::ToUnixSeconds(t); }, registry); - if (!status.ok()) return status; - - // double -> int - status = FunctionAdapter::CreateAndRegister( - builtin::kInt, false, - [](Arena* arena, double v) { - if ((v > static_cast(kIntMax)) || - (v < static_cast(kIntMin))) { - return CreateErrorValue(arena, "double out of int range", - absl::StatusCode::kInvalidArgument); - } - return CelValue::CreateInt64(static_cast(v)); - }, - registry); - if (!status.ok()) return status; - - // bool -> int - status = FunctionAdapter::CreateAndRegister( - builtin::kInt, false, - [](Arena*, bool v) { return static_cast(v); }, registry); - if (!status.ok()) return status; - - // uint -> int - status = FunctionAdapter::CreateAndRegister( - builtin::kInt, false, - [](Arena* arena, uint64_t v) { - if (v > static_cast(kIntMax)) { - return CreateErrorValue(arena, "uint out of int range", - absl::StatusCode::kInvalidArgument); - } - return CelValue::CreateInt64(static_cast(v)); - }, - registry); - if (!status.ok()) return status; - - // string -> int - status = FunctionAdapter::CreateAndRegister( - builtin::kInt, false, - [](Arena* arena, CelValue::StringHolder s) { - int64_t result; - if (absl::SimpleAtoi(s.value(), &result)) { - return CelValue::CreateInt64(result); - } else { - return CreateErrorValue(arena, "doesn't convert to a string", - absl::StatusCode::kInvalidArgument); - } - }, - registry); - if (!status.ok()) return status; - - // int -> int - status = FunctionAdapter::CreateAndRegister( - builtin::kInt, false, [](Arena*, int64_t v) { return v; }, registry); - if (!status.ok()) return status; - - // type conversion to uint - // double -> uint - status = FunctionAdapter::CreateAndRegister( - builtin::kUint, false, - [](Arena* arena, double v) { - if ((v > static_cast(kUintMax)) || (v < 0)) { - return CreateErrorValue(arena, "double out of uint range", - absl::StatusCode::kInvalidArgument); - } - return CelValue::CreateUint64(static_cast(v)); - }, - registry); - if (!status.ok()) return status; - - // int -> uint - status = FunctionAdapter::CreateAndRegister( - builtin::kUint, false, - [](Arena* arena, int64_t v) { - if (v < 0) { - return CreateErrorValue(arena, "int out of uint range", - absl::StatusCode::kInvalidArgument); - } - return CelValue::CreateUint64(static_cast(v)); - }, - registry); - if (!status.ok()) return status; - - // string -> uint - status = FunctionAdapter::CreateAndRegister( - builtin::kUint, false, - [](Arena* arena, CelValue::StringHolder s) { - uint64_t result; - if (absl::SimpleAtoi(s.value(), &result)) { - return CelValue::CreateUint64(result); - } else { - return CreateErrorValue(arena, "doesn't convert to a string", - absl::StatusCode::kInvalidArgument); - } - }, - registry); - if (!status.ok()) return status; - - // uint -> uint - status = FunctionAdapter::CreateAndRegister( - builtin::kUint, false, [](Arena*, uint64_t v) { return v; }, registry); - if (!status.ok()) return status; - - // duration - - // duration() conversion from string.. - status = FunctionAdapter::CreateAndRegister( - builtin::kDuration, false, CreateDurationFromString, registry); - if (!status.ok()) return status; - + // duration functions status = FunctionAdapter::CreateAndRegister( builtin::kHours, true, [](Arena* arena, absl::Duration d) -> CelValue { @@ -1566,55 +1687,6 @@ absl::Status RegisterBuiltinFunctions(CelFunctionRegistry* registry, registry); if (!status.ok()) return status; - if (options.enable_string_conversion) { - status = FunctionAdapter::CreateAndRegister( - builtin::kString, false, - [](Arena* arena, int64_t value) -> CelValue::StringHolder { - return CelValue::StringHolder( - Arena::Create(arena, absl::StrCat(value))); - }, - registry); - if (!status.ok()) return status; - - status = FunctionAdapter::CreateAndRegister( - builtin::kString, false, - [](Arena* arena, uint64_t value) -> CelValue::StringHolder { - return CelValue::StringHolder( - Arena::Create(arena, absl::StrCat(value))); - }, - registry); - if (!status.ok()) return status; - - status = FunctionAdapter::CreateAndRegister( - builtin::kString, false, - [](Arena* arena, double value) -> CelValue::StringHolder { - return CelValue::StringHolder( - Arena::Create(arena, absl::StrCat(value))); - }, - registry); - if (!status.ok()) return status; - - status = FunctionAdapter:: - CreateAndRegister( - builtin::kString, false, - [](Arena* arena, - CelValue::BytesHolder value) -> CelValue::StringHolder { - return CelValue::StringHolder(Arena::Create( - arena, std::string(value.value()))); - }, - registry); - if (!status.ok()) return status; - - status = FunctionAdapter:: - CreateAndRegister( - builtin::kString, false, - [](Arena*, CelValue::StringHolder value) -> CelValue::StringHolder { - return value; - }, - registry); - if (!status.ok()) return status; - } - status = FunctionAdapter::CreateAndRegister( builtin::kType, false, diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index 162244c95..5b4505cd7 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -121,6 +121,17 @@ class BuiltinsTest : public ::testing::Test { } // Helper method. Looks up in registry and tests Type conversions. + void TestTypeConverts(absl::string_view operation, const CelValue& ref, + double result) { + CelValue result_value; + + ASSERT_NO_FATAL_FAILURE(PerformRun(operation, {}, {ref}, &result_value)); + + ASSERT_EQ(result_value.IsDouble(), true); + ASSERT_EQ(result_value.DoubleOrDie(), result) + << operation << " for " << CelValue::TypeName(ref.type()); + } + void TestTypeConverts(absl::string_view operation, const CelValue& ref, int64_t result) { CelValue result_value; @@ -682,6 +693,31 @@ TEST_F(BuiltinsTest, TestTimestampFunctions) { 3L); } +TEST_F(BuiltinsTest, TestDoubleConversions_double) { + double ref = 100.1; + TestTypeConverts(builtin::kDouble, CelValue::CreateDouble(ref), 100.1); +} + +TEST_F(BuiltinsTest, TestDoubleConversions_int) { + int64_t ref = 100L; + TestTypeConverts(builtin::kDouble, CelValue::CreateInt64(ref), 100.0); +} + +TEST_F(BuiltinsTest, TestDoubleConversions_string) { + std::string ref = "-100.1"; + TestTypeConverts(builtin::kDouble, CelValue::CreateString(&ref), -100.1); +} + +TEST_F(BuiltinsTest, TestDoubleConversions_uint) { + uint64_t ref = 100UL; + TestTypeConverts(builtin::kDouble, CelValue::CreateUint64(ref), 100.0); +} + +TEST_F(BuiltinsTest, TestDoubleConversionError_stringInvalid) { + string invalid = "-100e-10.0"; + TestTypeConversionError(builtin::kDouble, CelValue::CreateString(&invalid)); +} + TEST_F(BuiltinsTest, TestIntConversions_int) { TestTypeConverts(builtin::kInt, CelValue::CreateInt64(100L), 100L); } diff --git a/eval/public/cel_builtins.h b/eval/public/cel_builtins.h index 9dd2dec31..c3a4217a3 100644 --- a/eval/public/cel_builtins.h +++ b/eval/public/cel_builtins.h @@ -71,6 +71,7 @@ constexpr char kMilliseconds[] = "getMilliseconds"; // Type conversions // TODO(issues/23): Add other type conversion methods. +constexpr char kDouble[] = "double"; constexpr char kInt[] = "int"; constexpr char kString[] = "string"; constexpr char kType[] = "type"; From 53e58a792e43ded60726a8e3c7e50874f4499a3e Mon Sep 17 00:00:00 2001 From: tswadell Date: Thu, 10 Dec 2020 15:55:07 -0500 Subject: [PATCH 07/12] Implement the dyn() identity function PiperOrigin-RevId: 346846292 --- conformance/BUILD | 2 -- eval/public/builtin_func_registrar.cc | 6 ++++++ eval/public/builtin_func_test.cc | 6 ++++++ eval/public/cel_builtins.h | 1 + 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index e76545697..8bd059c9c 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -89,8 +89,6 @@ cc_binary( "--skip_test=timestamps/duration_conversions/toType_duration,toString_duration", # TODO(issues/78): Missing bytes() conversion functions "--skip_test=conversions/bytes", - # TODO(issues/80): Missing dyn() conversion functions - "--skip_test=conversions/dyn/dyn_heterogeneous_list", # TODO(issues/81): Conversion functions for int(), uint() which can be # uncommented when the spec changes to truncation rather than rounding. "--skip_test=conversions/int/double_nearest,double_nearest_neg,double_half_away_neg,double_half_away_pos", diff --git a/eval/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc index ddf67b330..32bddaecb 100644 --- a/eval/public/builtin_func_registrar.cc +++ b/eval/public/builtin_func_registrar.cc @@ -1237,6 +1237,12 @@ absl::Status RegisterConversionFunctions(CelFunctionRegistry* registry, builtin::kDuration, false, CreateDurationFromString, registry); if (!status.ok()) return status; + // dyn() identity function. + // TODO(issues/102): strip dyn() function references at type-check time. + status = FunctionAdapter::CreateAndRegister( + builtin::kDyn, false, + [](Arena*, CelValue value) -> CelValue { return value; }, registry); + status = RegisterIntConversionFunctions(registry, options); if (!status.ok()) return status; diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index 5b4505cd7..b0ea37f2c 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -718,6 +718,12 @@ TEST_F(BuiltinsTest, TestDoubleConversionError_stringInvalid) { TestTypeConversionError(builtin::kDouble, CelValue::CreateString(&invalid)); } +TEST_F(BuiltinsTest, TestDynConversions) { + TestTypeConverts(builtin::kDyn, CelValue::CreateDouble(100.1), 100.1); + TestTypeConverts(builtin::kDyn, CelValue::CreateInt64(100L), 100L); + TestTypeConverts(builtin::kDyn, CelValue::CreateUint64(100UL), 100UL); +} + TEST_F(BuiltinsTest, TestIntConversions_int) { TestTypeConverts(builtin::kInt, CelValue::CreateInt64(100L), 100L); } diff --git a/eval/public/cel_builtins.h b/eval/public/cel_builtins.h index c3a4217a3..0cf6c14a7 100644 --- a/eval/public/cel_builtins.h +++ b/eval/public/cel_builtins.h @@ -72,6 +72,7 @@ constexpr char kMilliseconds[] = "getMilliseconds"; // Type conversions // TODO(issues/23): Add other type conversion methods. constexpr char kDouble[] = "double"; +constexpr char kDyn[] = "dyn"; constexpr char kInt[] = "int"; constexpr char kString[] = "string"; constexpr char kType[] = "type"; From 414f7309076949323b5737133bcb6ffea02a4109 Mon Sep 17 00:00:00 2001 From: tswadell Date: Thu, 10 Dec 2020 19:53:11 -0500 Subject: [PATCH 08/12] Implement bytes() conversion function PiperOrigin-RevId: 346894277 --- conformance/BUILD | 2 -- eval/public/builtin_func_registrar.cc | 31 ++++++++++++++++++++++++++- eval/public/builtin_func_test.cc | 23 ++++++++++++++++++++ eval/public/cel_builtins.h | 1 + 4 files changed, 54 insertions(+), 3 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index 8bd059c9c..6d50aaed4 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -87,8 +87,6 @@ cc_binary( # 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 - "--skip_test=conversions/bytes", # TODO(issues/81): Conversion functions for int(), uint() which can be # uncommented when the spec changes to truncation rather than rounding. "--skip_test=conversions/int/double_nearest,double_nearest_neg,double_half_away_neg,double_half_away_pos", diff --git a/eval/public/builtin_func_registrar.cc b/eval/public/builtin_func_registrar.cc index 32bddaecb..34554d5d4 100644 --- a/eval/public/builtin_func_registrar.cc +++ b/eval/public/builtin_func_registrar.cc @@ -1013,6 +1013,30 @@ absl::Status RegisterTimestampFunctions(CelFunctionRegistry* registry, return absl::OkStatus(); } +absl::Status RegisterBytesConversionFunctions(CelFunctionRegistry* registry, + const InterpreterOptions&) { + // bytes -> bytes + auto status = FunctionAdapter:: + CreateAndRegister( + builtin::kBytes, false, + [](Arena*, CelValue::BytesHolder value) -> CelValue::BytesHolder { + return value; + }, + registry); + if (!status.ok()) return status; + + // string -> bytes + status = FunctionAdapter::CreateAndRegister( + builtin::kBytes, false, + [](Arena* arena, CelValue::StringHolder value) -> CelValue { + return CelValue::CreateBytesView(value.value()); + }, + registry); + if (!status.ok()) return status; + + return absl::OkStatus(); +} + absl::Status RegisterDoubleConversionFunctions(CelFunctionRegistry* registry, const InterpreterOptions&) { // double -> double @@ -1121,6 +1145,8 @@ absl::Status RegisterStringConversionFunctions( return absl::OkStatus(); } + // TODO(issues/82): ensure the bytes conversion to string handles UTF-8 + // properly, and avoids unncessary allocations. // bytes -> string auto status = FunctionAdapter:: CreateAndRegister( @@ -1229,7 +1255,10 @@ absl::Status RegisterUintConversionFunctions(CelFunctionRegistry* registry, absl::Status RegisterConversionFunctions(CelFunctionRegistry* registry, const InterpreterOptions& options) { - auto status = RegisterDoubleConversionFunctions(registry, options); + auto status = RegisterBytesConversionFunctions(registry, options); + if (!status.ok()) return status; + + status = RegisterDoubleConversionFunctions(registry, options); if (!status.ok()) return status; // duration() conversion from string. diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index b0ea37f2c..f0a67ce5b 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -121,6 +121,17 @@ class BuiltinsTest : public ::testing::Test { } // Helper method. Looks up in registry and tests Type conversions. + void TestTypeConverts(absl::string_view operation, const CelValue& ref, + CelValue::BytesHolder result) { + CelValue result_value; + + ASSERT_NO_FATAL_FAILURE(PerformRun(operation, {}, {ref}, &result_value)); + + ASSERT_EQ(result_value.IsBytes(), true); + ASSERT_EQ(result_value.BytesOrDie(), result) + << operation << " for " << CelValue::TypeName(ref.type()); + } + void TestTypeConverts(absl::string_view operation, const CelValue& ref, double result) { CelValue result_value; @@ -693,6 +704,18 @@ TEST_F(BuiltinsTest, TestTimestampFunctions) { 3L); } +TEST_F(BuiltinsTest, TestBytesConversions_bytes) { + std::string txt = "hello"; + CelValue::BytesHolder result = CelValue::BytesHolder(&txt); + TestTypeConverts(builtin::kBytes, CelValue::CreateBytes(&txt), result); +} + +TEST_F(BuiltinsTest, TestBytesConversions_string) { + std::string txt = "hello"; + CelValue::BytesHolder result = CelValue::BytesHolder(&txt); + TestTypeConverts(builtin::kBytes, CelValue::CreateString(&txt), result); +} + TEST_F(BuiltinsTest, TestDoubleConversions_double) { double ref = 100.1; TestTypeConverts(builtin::kDouble, CelValue::CreateDouble(ref), 100.1); diff --git a/eval/public/cel_builtins.h b/eval/public/cel_builtins.h index 0cf6c14a7..c7a5f7f0b 100644 --- a/eval/public/cel_builtins.h +++ b/eval/public/cel_builtins.h @@ -71,6 +71,7 @@ constexpr char kMilliseconds[] = "getMilliseconds"; // Type conversions // TODO(issues/23): Add other type conversion methods. +constexpr char kBytes[] = "bytes"; constexpr char kDouble[] = "double"; constexpr char kDyn[] = "dyn"; constexpr char kInt[] = "int"; From 1f35b77daeb29be6f3fd0fe58abb24ecf3f0f008 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Mon, 14 Dec 2020 16:37:42 -0500 Subject: [PATCH 09/12] Internal change PiperOrigin-RevId: 347458787 --- eval/public/transform_utility.cc | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/eval/public/transform_utility.cc b/eval/public/transform_utility.cc index e10a27a99..4b4ebb8ad 100644 --- a/eval/public/transform_utility.cc +++ b/eval/public/transform_utility.cc @@ -114,7 +114,8 @@ absl::StatusOr ValueToCelValue(const Value& value, case Value::kBoolValue: return CelValue::CreateBool(value.bool_value()); case Value::kBytesValue: - return CelValue::CreateBytes(CelValue::BytesHolder(&value.bytes_value())); + return CelValue::CreateBytes(CelValue::BytesHolder( + arena->Create(arena, value.bytes_value()))); case Value::kDoubleValue: return CelValue::CreateDouble(value.double_value()); case Value::kEnumValue: @@ -146,14 +147,18 @@ absl::StatusOr ValueToCelValue(const Value& value, } case Value::kNullValue: return CelValue::CreateNull(); - case Value::kObjectValue: - return CelProtoWrapper::CreateMessage(&value.object_value(), arena); + case Value::kObjectValue: { + auto cel_value = + CelProtoWrapper::CreateMessage(&value.object_value(), arena); + if (cel_value.IsError()) return *cel_value.ErrorOrDie(); + return cel_value; + } case Value::kStringValue: - return CelValue::CreateString( - CelValue::StringHolder(&value.string_value())); + return CelValue::CreateString(CelValue::StringHolder( + arena->Create(arena, value.string_value()))); case Value::kTypeValue: - return CelValue::CreateCelType( - CelValue::CelTypeHolder(&value.type_value())); + return CelValue::CreateCelType(CelValue::CelTypeHolder( + arena->Create(arena, value.type_value()))); case Value::kUint64Value: return CelValue::CreateUint64(value.uint64_value()); case Value::KIND_NOT_SET: From d2ef27c9206302025cb4c5c54b53a85f88879c7a Mon Sep 17 00:00:00 2001 From: tswadell Date: Tue, 15 Dec 2020 14:13:31 -0500 Subject: [PATCH 10/12] Ensure the lifecycle of rewritten expressions is preserved in the output CelExpression PiperOrigin-RevId: 347652258 --- eval/compiler/flat_expr_builder.cc | 23 ++++++----- eval/compiler/flat_expr_builder_test.cc | 54 +++++++++++++++++++++++++ eval/eval/evaluator_core.h | 10 +++-- 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index eeb6ccfa7..78928580d 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -1,5 +1,7 @@ #include "eval/compiler/flat_expr_builder.h" +#include + #include "google/api/expr/v1alpha1/checked.pb.h" #include "stack" #include "absl/container/node_hash_map.h" @@ -658,8 +660,8 @@ void ComprehensionVisitor::PreVisit(const Expr*) { void ComprehensionVisitor::PostVisitArg(int arg_num, const Expr* expr) { const Comprehension* comprehension = &expr->comprehension_expr(); - const auto accu_var = comprehension->accu_var(); - const auto iter_var = comprehension->iter_var(); + const auto& accu_var = comprehension->accu_var(); + const auto& iter_var = comprehension->iter_var(); // TODO(issues/20): Consider refactoring the comprehension prologue step. switch (arg_num) { case ITER_RANGE: { @@ -732,7 +734,7 @@ FlatExprBuilder::CreateExpressionImpl( const Expr* effective_expr = expr; // transformed expression preserving expression IDs - Expr rewrite_buffer; + std::unique_ptr rewrite_buffer = nullptr; // 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 @@ -745,19 +747,19 @@ FlatExprBuilder::CreateExpressionImpl( return rewritten.status(); } if (rewritten.value().has_value()) { - rewrite_buffer = std::move(rewritten)->value(); - effective_expr = &rewrite_buffer; + rewrite_buffer = + std::make_unique(std::move(rewritten).value().value()); + effective_expr = rewrite_buffer.get(); } // TODO(issues/99): we could setup a check step here that confirms all of // references are defined before actually evaluating. } + Expr const_fold_buffer; if (constant_folding_) { - Expr buffer; FoldConstants(*effective_expr, *this->GetRegistry(), constant_arena_, - idents, &buffer); - rewrite_buffer = std::move(buffer); - effective_expr = &rewrite_buffer; + idents, &const_fold_buffer); + effective_expr = &const_fold_buffer; } std::set iter_variable_names; @@ -776,7 +778,8 @@ FlatExprBuilder::CreateExpressionImpl( absl::make_unique( expr, std::move(execution_path), comprehension_max_iterations_, std::move(iter_variable_names), enable_unknowns_, - enable_unknown_function_results_, enable_missing_attribute_errors_); + enable_unknown_function_results_, enable_missing_attribute_errors_, + std::move(rewrite_buffer)); if (warnings != nullptr) { *warnings = std::move(warnings_builder).warnings(); diff --git a/eval/compiler/flat_expr_builder_test.cc b/eval/compiler/flat_expr_builder_test.cc index 2e05bbb10..2a65d2bc3 100644 --- a/eval/compiler/flat_expr_builder_test.cc +++ b/eval/compiler/flat_expr_builder_test.cc @@ -614,6 +614,60 @@ TEST(FlatExprBuilderTest, CheckedExprActivationMissesReferences) { EXPECT_FALSE(result.BoolOrDie()); } +TEST(FlatExprBuilderTest, CheckedExprWithReferenceMapAndConstantFolding) { + CheckedExpr expr; + // {`var1`: 'hello'} + google::protobuf::TextFormat::ParseFromString(R"( + reference_map { + key: 3 + value { + name: "var1" + value { + int64_value: 1 + } + } + } + expr { + id: 1 + struct_expr { + entries { + id: 2 + map_key { + id: 3 + ident_expr { + name: "var1" + } + } + value { + id: 4 + const_expr { + string_value: "hello" + } + } + } + } + })", + &expr); + + FlatExprBuilder builder; + google::protobuf::Arena arena; + builder.set_constant_folding(true, &arena); + 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; + auto result_or = cel_expr->Evaluate(activation, &arena); + ASSERT_OK(result_or); + CelValue result = result_or.value(); + ASSERT_TRUE(result.IsMap()); + auto m = result.MapOrDie(); + auto v = (*m)[CelValue::CreateInt64(1L)]; + EXPECT_THAT(v.value().StringOrDie().value(), Eq("hello")); +} + TEST(FlatExprBuilderTest, ComprehensionWorksForError) { Expr expr; // {}[0].all(x, x) should evaluate OK but return an error value diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index 1ad1008c6..c92a00901 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -67,7 +67,7 @@ using ExecutionPath = std::vector>; // stack as Span<>. class ValueStack { public: - ValueStack(size_t max_size) : current_size_(0) { + explicit ValueStack(size_t max_size) : current_size_(0) { stack_.resize(max_size); attribute_stack_.resize(max_size); } @@ -336,8 +336,10 @@ class CelExpressionFlatImpl : public CelExpression { std::set iter_variable_names, bool enable_unknowns = false, bool enable_unknown_function_results = false, - bool enable_missing_attribute_errors = false) - : path_(std::move(path)), + bool enable_missing_attribute_errors = false, + std::unique_ptr rewritten_expr = nullptr) + : rewritten_expr_(std::move(rewritten_expr)), + path_(std::move(path)), max_iterations_(max_iterations), iter_variable_names_(std::move(iter_variable_names)), enable_unknowns_(enable_unknowns), @@ -372,6 +374,8 @@ class CelExpressionFlatImpl : public CelExpression { CelEvaluationListener callback) const override; private: + // Maintain lifecycle of a modified expression. + std::unique_ptr rewritten_expr_; const ExecutionPath path_; const int max_iterations_; const std::set iter_variable_names_; From 6dc9795f45ee91414c8d3b45ddb7bcb8535ae0e5 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Tue, 15 Dec 2020 14:37:35 -0500 Subject: [PATCH 11/12] Internal change. PiperOrigin-RevId: 347657817 --- common/escaping.cc | 2 +- parser/source_factory.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common/escaping.cc b/common/escaping.cc index 39aba6d0a..26b1367b8 100644 --- a/common/escaping.cc +++ b/common/escaping.cc @@ -294,7 +294,7 @@ absl::optional unescape(const std::string& s, bool is_bytes) { } value = value.substr(1, n - 2); // If there is nothing to escape, then return. - if (is_raw_literal || (value.find("\\") == std::string::npos)) { + if (is_raw_literal || (value.find('\\') == std::string::npos)) { return value; } diff --git a/parser/source_factory.cc b/parser/source_factory.cc index 92990a4a8..3573f5289 100644 --- a/parser/source_factory.cc +++ b/parser/source_factory.cc @@ -488,7 +488,7 @@ std::string SourceFactory::errorMessage(const std::string& description, error->location.col + 1, error->message); std::string snippet = getSourceLine(error->location.line, expression); std::string::size_type pos = 0; - while ((pos = snippet.find("\t", pos)) != std::string::npos) { + while ((pos = snippet.find('\t', pos)) != std::string::npos) { snippet.replace(pos, 1, " "); } std::string src_line = "\n | " + snippet; From 9cacfc6cdf8a172b8da24974c8d2f7f0b8cfe5e3 Mon Sep 17 00:00:00 2001 From: kuat Date: Thu, 17 Dec 2020 13:48:26 -0500 Subject: [PATCH 12/12] OSS export. PiperOrigin-RevId: 348051353 --- eval/eval/evaluator_core.h | 2 ++ eval/public/builtin_func_test.cc | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index c92a00901..b9d05c583 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -34,6 +34,8 @@ namespace runtime { // Forward declaration of ExecutionFrame, to resolve circular dependency. class ExecutionFrame; +using Expr = google::api::expr::v1alpha1::Expr; + // Class Expression represents single execution step. class ExpressionStep { public: diff --git a/eval/public/builtin_func_test.cc b/eval/public/builtin_func_test.cc index f0a67ce5b..5b3f7b267 100644 --- a/eval/public/builtin_func_test.cc +++ b/eval/public/builtin_func_test.cc @@ -737,7 +737,7 @@ TEST_F(BuiltinsTest, TestDoubleConversions_uint) { } TEST_F(BuiltinsTest, TestDoubleConversionError_stringInvalid) { - string invalid = "-100e-10.0"; + std::string invalid = "-100e-10.0"; TestTypeConversionError(builtin::kDouble, CelValue::CreateString(&invalid)); } @@ -822,7 +822,7 @@ TEST_F(BuiltinsTest, TestUintConversionError_intRange) { } TEST_F(BuiltinsTest, TestUintConversionError_stringInvalid) { - string invalid = "-100"; + std::string invalid = "-100"; TestTypeConversionError(builtin::kUint, CelValue::CreateString(&invalid)); }