Merge pull request #120 from google/cpp-sync

Latest sync from google3
google · Mar 31, 2021 · 3b7f706 · 3b7f706
2 parents bb03a5f + f3b9b46
commit 3b7f706
Show file tree

Hide file tree

Showing 69 changed files with 1,938 additions and 813 deletions.
diff --git a/common/BUILD b/common/BUILD
@@ -196,6 +196,7 @@ cc_test(
         "//internal:types",
         "//internal:value_internal",
         "//testutil:util",
+        "@com_google_absl//absl/meta:type_traits",
         "@com_google_absl//absl/strings",
         "@com_google_googleapis//google/rpc:status_cc_proto",
         "@com_google_googleapis//google/type:money_cc_proto",
@@ -218,6 +219,7 @@ cc_library(
         "//internal:list_impl",
         "//internal:map_impl",
         "//internal:types",
+        "@com_google_absl//absl/meta:type_traits",
     ],
 )
 

diff --git a/common/converters.h b/common/converters.h
@@ -5,6 +5,7 @@
 
 #include <memory>
 
+#include "absl/meta/type_traits.h"
 #include "common/parent_ref.h"
 #include "common/value.h"
 #include "internal/list_impl.h"
@@ -53,7 +54,7 @@ template <Value::Kind ValueKind, typename T>
 Value ValueFromList(T&& value) {
   static_assert(!std::is_pointer<T>::value, "use ValueForList");
   return Value::MakeList<
-      internal::ListWrapper<ValueKind, internal::remove_cvref_t<T>>>(
+      internal::ListWrapper<ValueKind, absl::remove_cvref_t<T>>>(
       std::forward<T>(value));
 }
 
@@ -92,7 +93,7 @@ template <Value::Kind KeyKind, Value::Kind ValueKind, typename T>
 Value ValueFromMap(T&& value) {
   static_assert(!std::is_pointer<T>::value, "use ValueForList");
   return Value::MakeMap<
-      internal::MapWrapper<KeyKind, ValueKind, internal::remove_cvref_t<T>>>(
+      internal::MapWrapper<KeyKind, ValueKind, absl::remove_cvref_t<T>>>(
       std::forward<T>(value));
 }
 

diff --git a/common/escaping.cc b/common/escaping.cc
@@ -241,7 +241,7 @@ inline std::tuple<std::string, absl::string_view, std::string> unescape_char(
   }
 
   if (value < 0x80 || !encode) {
-    char tmp[2] = {(char)value, '\0'};
+    char tmp[2] = {static_cast<char>(value), '\0'};
     return std::make_tuple(std::string(tmp), s, "");
   } else {
     char tmp[5];
@@ -294,7 +294,7 @@ absl::optional<std::string> 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 || (!absl::StrContains(value, '\\'))) {
     return value;
   }
 

diff --git a/common/value.cc b/common/value.cc
@@ -20,8 +20,6 @@ namespace api {
 namespace expr {
 namespace common {
 
-using ::google::api::expr::internal::NotFoundError;
-
 namespace {
 
 static constexpr const Value::Kind kIndexToKind[] = {

diff --git a/common/value_test.cc b/common/value_test.cc
@@ -10,6 +10,7 @@
 #include "google/type/money.pb.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include "absl/meta/type_traits.h"
 #include "absl/strings/str_cat.h"
 #include "common/custom_object.h"
 #include "internal/status_util.h"
@@ -709,15 +710,15 @@ class ValueVisitTest : public ::testing::Test {
 
   template <typename H>
   void TestGetPtrVisitor() {
-    using T = remove_reference_t<decltype(*inst_of<H&>())>;
+    using T = absl::remove_reference_t<decltype(*inst_of<H&>())>;
     ExpectSameType<const T*, GetPtrVisitorType<H, T>>();
     // Return by const ref.
     ExpectSameType<const T&, GetVisitorType<H, const T&, T>>();
   }
 
   template <typename H, typename U>
   void TestGetVisitor() {
-    using T = remove_reference_t<decltype(*inst_of<H&>())>;
+    using T = absl::remove_reference_t<decltype(*inst_of<H&>())>;
     // Return by optional.
     ExpectSameType<absl::optional<U>,
                    GetVisitorType<H, absl::optional<U>, T>>();
@@ -727,7 +728,7 @@ class ValueVisitTest : public ::testing::Test {
 
   template <typename H>
   void TestValueAdapter() {
-    using T = remove_reference_t<decltype(*inst_of<H&>())>;
+    using T = absl::remove_reference_t<decltype(*inst_of<H&>())>;
     ExpectSameType<T&, ValueAdapterType<H&>>();
     ExpectSameType<const T&, ValueAdapterType<const H&>>();
   }

diff --git a/conformance/BUILD b/conformance/BUILD
@@ -18,6 +18,7 @@ ALL_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/parse.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",
@@ -26,27 +27,6 @@ ALL_TESTS = [
     "@com_google_cel_spec//tests/simple:testdata/unknowns.textproto",
 ]
 
-DASHBOARD_TESTS = [
-    "@com_google_cel_spec//tests/simple:testdata/basic.textproto",
-    "@com_google_cel_spec//tests/simple:testdata/comparisons.textproto",
-    "@com_google_cel_spec//tests/simple:testdata/conversions.textproto",
-    "@com_google_cel_spec//tests/simple:testdata/dynamic.textproto",
-    "@com_google_cel_spec//tests/simple:testdata/enums.textproto",
-    "@com_google_cel_spec//tests/simple:testdata/fields.textproto",
-    "@com_google_cel_spec//tests/simple:testdata/fp_math.textproto",
-    "@com_google_cel_spec//tests/simple:testdata/integer_math.textproto",
-    "@com_google_cel_spec//tests/simple:testdata/lists.textproto",
-    "@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",
-    "@com_google_cel_spec//tests/simple:testdata/unknowns.textproto",
-]
-
 cc_binary(
     name = "server",
     testonly = 1,
@@ -81,71 +61,54 @@ cc_binary(
             "--server=\"$(location :server) " + arg + "\"",
             "--skip_check",
             "--pipe",
-            # TODO(issues/93): Inconsistent Duration.getMilliseconds() behavior.
+
+            # Tests which require spec changes.
+            # TODO(issues/93): Deprecate Duration.getMilliseconds.
             "--skip_test=timestamps/duration_converters/get_milliseconds",
-            # 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/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/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",
-            # Requires container support
-            "--skip_test=namespace/namespace/self_eval_container_lookup_unchecked",
+            # TODO(issues/110): Tune parse limits to mirror those for proto deserialization and C++ safety limits.
+            "--skip_test=parse/nest/list_index,message_literal,funcall,list_literal,map_literal;repeat/conditional,add_sub,mul_div,select,index,map_literal,message_literal",
+
+            # Broken test cases which should be supported.
+            # TODO(issues/111): Byte literal decoding of invalid UTF-8 results in incorrect bytes output.
             "--skip_test=basic/self_eval_nonzeroish/self_eval_bytes_invalid_utf8",
-            # Requires heteregenous equality spec clarification
-            "--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/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.
-            "--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",
-            # New conformance tests awaiting synchronization.
+            # TODO(issues/112): Unbound functions result in empty eval response.
             "--skip_test=basic/functions/unbound",
             "--skip_test=basic/functions/unbound_is_runtime_error",
+            # TODO(issues/113): Aggregate values must logically AND element equality results.
             "--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",
+            # TODO(issues/114): Ensure the 'in' operator is a logical OR of element equality results.
+            "--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",
+            # TODO(issues/115): The 'in' operator fails with maps containing boolean keys.
+            "--skip_test=fields/in/singleton",
+            # TODO(issues/97): Parse-only qualified variable lookup "x.y" wtih binding "x.y" or "y" within container "x" fails
+            "--skip_test=fields/qualified_identifier_resolution/qualified_ident,map_field_select,ident_with_longest_prefix_check,qualified_identifier_resolution_unchecked",
+            "--skip_test=namespace/qualified/self_eval_qualified_lookup",
+            "--skip_test=namespace/namespace/self_eval_container_lookup,self_eval_container_lookup_unchecked",
+            # TODO(issues/116): Debug why dynamic/list/var fails to JSON parse correctly.
+            "--skip_test=dynamic/list/var",
+            # TODO(issues/109): Ensure that unset wrapper fields return 'null' rather than the default value of the wrapper.
+            "--skip_test=dynamic/int32/field_read_proto2_unset,field_read_proto3_unset;uint32/field_read_proto2_unset;uint64/field_read_proto2_unset;float/field_read_proto2_unset,field_read_proto3_unset;double/field_read_proto2_unset,field_read_proto3_unset",
+            "--skip_test=proto2/empty_field/wkt",
+            "--skip_test=proto3/empty_field/wkt",
+            # TODO(issues/117): Integer overflow on enum assignments should error.
+            "--skip_test=enums/legacy_proto2/select_big,select_neg,assign_standalone_int_too_big,assign_standalone_int_too_neg",
+            "--skip_test=enums/legacy_proto3/assign_standalone_int_too_big,assign_standalone_int_too_neg",
+            # TODO(issues/118): Duration and timestamp range errors should result in evaluation errors.
+            "--skip_test=timestamps/timestamp_range",
+
+            # Future features for CEL 1.0
+            # TODO(issues/119): Strong typing support for enums, specified but not implemented.
             "--skip_test=enums/strong_proto2",
             "--skip_test=enums/strong_proto3",
-            "--skip_test=timestamps/timestamp_range",
-            "--skip_test=timestamps/duration_range",
+            # Bad tests, temporarily disable.
+            "--skip_test=dynamic/float/field_assign_proto2_range,field_assign_proto3_range",
         ] + ["$(location " + test + ")" for test in ALL_TESTS],
         data = [
             ":server",
@@ -165,12 +128,17 @@ sh_test(
         "$(location @com_google_cel_spec//tests/simple:simple_test)",
         "--server=$(location :server)",
         "--skip_check",
+        # TODO(issues/116): Debug why dynamic/list/var fails to JSON parse correctly.
+        "--skip_test=dynamic/list/var",
+        # TODO(issues/119): Strong typing support for enums, specified but not implemented.
+        "--skip_test=enums/strong_proto2",
+        "--skip_test=enums/strong_proto3",
         "--pipe",
-    ] + ["$(location " + test + ")" for test in DASHBOARD_TESTS],
+    ] + ["$(location " + test + ")" for test in ALL_TESTS],
     data = [
         ":server",
         "@com_google_cel_spec//tests/simple:simple_test",
-    ] + DASHBOARD_TESTS,
+    ] + ALL_TESTS,
     visibility = [
         "//:__subpackages__",
         "//third_party/cel:__pkg__",

diff --git a/conformance/server.cc b/conformance/server.cc
@@ -1,8 +1,6 @@
-#include <iostream>
-
+#include "google/api/expr/v1alpha1/conformance_service.pb.h"
 #include "google/api/expr/v1alpha1/syntax.pb.h"
 #include "google/api/expr/v1alpha1/checked.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"
@@ -151,6 +149,7 @@ int RunServer(bool optimize) {
   google::protobuf::Arena arena;
   InterpreterOptions options;
   options.enable_qualified_type_identifiers = true;
+  options.enable_string_size_as_unicode_codepoints = true;
 
   if (optimize) {
     std::cerr << "Enabling optimizations" << std::endl;
@@ -169,7 +168,8 @@ int RunServer(bool optimize) {
                               NestedEnum_descriptor());
   type_registry->Register(google::api::expr::test::v1::proto3::TestAllTypes::
                               NestedEnum_descriptor());
-  auto register_status = RegisterBuiltinFunctions(builder->GetRegistry());
+  auto register_status =
+      RegisterBuiltinFunctions(builder->GetRegistry(), options);
   if (!register_status.ok()) {
     std::cerr << "Failed to initialize: " << register_status.ToString()
               << std::endl;
@@ -181,7 +181,7 @@ int RunServer(bool optimize) {
   // 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
+  // OUTPUT LINE 1: JSON of the corresponding response protobuf
   while (true) {
     std::string cmd, input, output;
     std::getline(std::cin, cmd);

diff --git a/eval/eval/BUILD b/eval/eval/BUILD
@@ -173,13 +173,10 @@ cc_library(
     deps = [
         ":evaluator_core",
         ":expression_step_base",
-        "//eval/public:activation",
-        "//eval/public:cel_value",
         "//eval/public/containers:container_backed_list_impl",
+        "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
-        "@com_google_absl//absl/strings",
         "@com_google_absl//absl/types:span",
-        "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto",
     ],
 )
 

diff --git a/eval/eval/attribute_utility_test.cc b/eval/eval/attribute_utility_test.cc
@@ -15,7 +15,6 @@ namespace runtime {
 
 using google::api::expr::v1alpha1::Expr;
 using testing::Eq;
-using testing::IsNull;
 using testing::NotNull;
 using testing::SizeIs;
 using testing::UnorderedPointwise;

diff --git a/eval/eval/comprehension_step.cc b/eval/eval/comprehension_step.cc
@@ -209,7 +209,7 @@ absl::Status ComprehensionFinish::Evaluate(ExecutionFrame* frame) const {
 
 class ListKeysStep : public ExpressionStepBase {
  public:
-  ListKeysStep(int64_t expr_id) : ExpressionStepBase(expr_id, false) {}
+  explicit ListKeysStep(int64_t expr_id) : ExpressionStepBase(expr_id, false) {}
   absl::Status Evaluate(ExecutionFrame* frame) const override;
 
  private:

diff --git a/eval/eval/container_access_step.cc b/eval/eval/container_access_step.cc
@@ -22,7 +22,7 @@ constexpr int NUM_CONTAINER_ACCESS_ARGUMENTS = 2;
 // message.
 class ContainerAccessStep : public ExpressionStepBase {
  public:
-  ContainerAccessStep(int64_t expr_id) : ExpressionStepBase(expr_id) {}
+  explicit ContainerAccessStep(int64_t expr_id) : ExpressionStepBase(expr_id) {}
 
   absl::Status Evaluate(ExecutionFrame* frame) const override;
 
@@ -54,7 +54,7 @@ inline CelValue ContainerAccessStep::LookupInMap(const CelMap* cel_map,
       break;
     }
   }
-  return CreateNoSuchKeyError(arena, absl::StrCat("Key not found in map"));
+  return CreateNoSuchKeyError(arena, "Key not found in map");
 }
 
 inline CelValue ContainerAccessStep::LookupInList(const CelList* cel_list,