OSS Export

conformance/BUILD

@@ -85,8 +85,6 @@ cc_binary(
             # 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",

eval/compiler/flat_expr_builder_test.cc

@@ -604,7 +604,7 @@ TEST(FlatExprBuilderTest, CheckedExprActivationMissesReferences) {
       {CelValue::CreateStringView("var2"), CelValue::CreateBool(false)}};
 
   std::unique_ptr<CelMap> map_value =
-      CreateContainerBackedMap(absl::MakeSpan(map_pairs));
+      CreateContainerBackedMap(absl::MakeSpan(map_pairs)).value();
   activation.InsertValue("bar", CelValue::CreateMap(map_value.get()));
   result_or = cel_expr->Evaluate(activation, &arena);
   ASSERT_OK(result_or);

eval/eval/BUILD

@@ -17,6 +17,7 @@ cc_library(
     deps = [
         ":attribute_trail",
         ":attribute_utility",
+        ":evaluator_stack",
         "//base:status_macros",
         "//eval/public:activation",
         "//eval/public:cel_attribute",
@@ -33,6 +34,31 @@ cc_library(
     ],
 )
 
+cc_library(
+    name = "evaluator_stack",
+    srcs = [
+        "evaluator_stack.cc",
+    ],
+    hdrs = [
+        "evaluator_stack.h",
+    ],
+    deps = [
+        ":attribute_trail",
+        "//eval/public:cel_value",
+    ],
+)
+
+cc_test(
+    name = "evaluator_stack_test",
+    srcs = [
+        "evaluator_stack_test.cc",
+    ],
+    deps = [
+        ":evaluator_stack",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 cc_library(
     name = "expression_step_base",
     hdrs = [
@@ -419,6 +445,7 @@ cc_test(
         "//eval/public/structs:cel_proto_wrapper",
         "//eval/testutil:test_message_cc_proto",
         "//testutil:util",
+        "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
         "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto",
         "@com_google_googletest//:gtest_main",

eval/eval/create_struct_step.cc

@@ -1,6 +1,7 @@
 #include "eval/eval/create_struct_step.h"
 
 #include <memory>
+#include <utility>
 
 #include "google/api/expr/v1alpha1/syntax.pb.h"
 #include "absl/status/status.h"
@@ -233,16 +234,17 @@ absl::Status CreateStructStepForMap::DoEvaluate(ExecutionFrame* frame,
     map_entries.push_back({args[2 * i], args[2 * i + 1]});
   }
 
-  auto cel_map =
+  auto status_or_cel_map =
       CreateContainerBackedMap(absl::Span<std::pair<CelValue, CelValue>>(
           map_entries.data(), map_entries.size()));
-
-  if (cel_map == nullptr) {
-    *result = CreateErrorValue(frame->arena(), "Failed to create map");
-
+  if (!status_or_cel_map.ok()) {
+    *result =
+        CreateErrorValue(frame->arena(), status_or_cel_map.status().message());
     return absl::OkStatus();
   }
 
+  auto cel_map = std::move(*status_or_cel_map);
+
   *result = CelValue::CreateMap(cel_map.get());
 
   // Pass object ownership to Arena.

eval/eval/create_struct_step_test.cc

@@ -593,7 +593,8 @@ TEST_P(CreateCreateStructStepTest, TestSetStringMapField) {
 
   auto cel_map =
       CreateContainerBackedMap(absl::Span<std::pair<CelValue, CelValue>>(
-          entries.data(), entries.size()));
+                                   entries.data(), entries.size()))
+          .value();
 
   ASSERT_NO_FATAL_FAILURE(RunExpressionAndGetMessage(
       "string_int32_map", CelValue::CreateMap(cel_map.get()), &arena, &test_msg,
@@ -620,7 +621,8 @@ TEST_P(CreateCreateStructStepTest, TestSetInt64MapField) {
 
   auto cel_map =
       CreateContainerBackedMap(absl::Span<std::pair<CelValue, CelValue>>(
-          entries.data(), entries.size()));
+                                   entries.data(), entries.size()))
+          .value();
 
   ASSERT_NO_FATAL_FAILURE(RunExpressionAndGetMessage(
       "int64_int32_map", CelValue::CreateMap(cel_map.get()), &arena, &test_msg,
@@ -647,7 +649,8 @@ TEST_P(CreateCreateStructStepTest, TestSetUInt64MapField) {
 
   auto cel_map =
       CreateContainerBackedMap(absl::Span<std::pair<CelValue, CelValue>>(
-          entries.data(), entries.size()));
+                                   entries.data(), entries.size()))
+          .value();
 
   ASSERT_NO_FATAL_FAILURE(RunExpressionAndGetMessage(
       "uint64_int32_map", CelValue::CreateMap(cel_map.get()), &arena, &test_msg,

eval/eval/evaluator_core.cc

@@ -34,17 +34,6 @@ absl::Status CheckIterAccess(CelExpressionFlatEvaluationState* state,
 
 }  // namespace
 
-void ValueStack::Clear() {
-  for (auto& v : stack_) {
-    v = CelValue();
-  }
-  for (auto& attr : attribute_stack_) {
-    attr = AttributeTrail();
-  }
-
-  current_size_ = 0;
-}
-
 CelExpressionFlatEvaluationState::CelExpressionFlatEvaluationState(
     size_t value_stack_size, const std::set<std::string>& iter_variable_names,
     google::protobuf::Arena* arena)
@@ -171,7 +160,7 @@ absl::StatusOr<CelValue> CelExpressionFlatImpl::Trace(
                        enable_unknowns_, enable_unknown_function_results_,
                        enable_missing_attribute_errors_);
 
-  ValueStack* stack = &frame.value_stack();
+  EvaluatorStack* stack = &frame.value_stack();
   size_t initial_stack_size = stack->size();
   const ExpressionStep* expr;
   while ((expr = frame.Next()) != nullptr) {

eval/eval/evaluator_core.h

@@ -20,6 +20,7 @@
 #include "absl/types/span.h"
 #include "eval/eval/attribute_trail.h"
 #include "eval/eval/attribute_utility.h"
+#include "eval/eval/evaluator_stack.h"
 #include "eval/public/activation.h"
 #include "eval/public/cel_attribute.h"
 #include "eval/public/cel_expression.h"
@@ -42,7 +43,7 @@ class ExpressionStep {
   virtual ~ExpressionStep() {}
 
   // Performs actual evaluation.
-  // Values are passed between Expression objects via ValueStack, which is
+  // Values are passed between Expression objects via EvaluatorStack, which is
   // supplied with context.
   // Also, Expression gets values supplied by caller though Activation
   // interface.
@@ -64,122 +65,6 @@ class ExpressionStep {
 
 using ExecutionPath = std::vector<std::unique_ptr<const ExpressionStep>>;
 
-// CelValue stack.
-// Implementation is based on vector to allow passing parameters from
-// stack as Span<>.
-class ValueStack {
- public:
-  explicit ValueStack(size_t max_size) : current_size_(0) {
-    stack_.resize(max_size);
-    attribute_stack_.resize(max_size);
-  }
-
-  // Return the current stack size.
-  size_t size() const { return current_size_; }
-
-  // Return the maximum size of the stack.
-  size_t max_size() const { return stack_.size(); }
-
-  // Returns true if stack is empty.
-  bool empty() const { return current_size_ == 0; }
-
-  // Attributes stack size.
-  size_t attribute_size() const { return current_size_; }
-
-  // Check that stack has enough elements.
-  bool HasEnough(size_t size) const { return current_size_ >= size; }
-
-  // Dumps the entire stack state as is.
-  void Clear();
-
-  // Gets the last size elements of the stack.
-  // Checking that stack has enough elements is caller's responsibility.
-  // Please note that calls to Push may invalidate returned Span object.
-  absl::Span<const CelValue> GetSpan(size_t size) const {
-    if (!HasEnough(size)) {
-      GOOGLE_LOG(ERROR) << "Requested span size (" << size
-                 << ") exceeds current stack size: " << current_size_;
-    }
-    return absl::Span<const CelValue>(stack_.data() + current_size_ - size,
-                                      size);
-  }
-
-  // Gets the last size attribute trails of the stack.
-  // Checking that stack has enough elements is caller's responsibility.
-  // Please note that calls to Push may invalidate returned Span object.
-  absl::Span<const AttributeTrail> GetAttributeSpan(size_t size) const {
-    return absl::Span<const AttributeTrail>(
-        attribute_stack_.data() + current_size_ - size, size);
-  }
-
-  // Peeks the last element of the stack.
-  // Checking that stack is not empty is caller's responsibility.
-  const CelValue& Peek() const {
-    if (empty()) {
-      GOOGLE_LOG(ERROR) << "Peeking on empty ValueStack";
-    }
-    return stack_[current_size_ - 1];
-  }
-
-  // Peeks the last element of the attribute stack.
-  // Checking that stack is not empty is caller's responsibility.
-  const AttributeTrail& PeekAttribute() const {
-    if (empty()) {
-      GOOGLE_LOG(ERROR) << "Peeking on empty ValueStack";
-    }
-    return attribute_stack_[current_size_ - 1];
-  }
-
-  // Clears the last size elements of the stack.
-  // Checking that stack has enough elements is caller's responsibility.
-  void Pop(size_t size) {
-    if (!HasEnough(size)) {
-      GOOGLE_LOG(ERROR) << "Trying to pop more elements (" << size
-                 << ") than the current stack size: " << current_size_;
-    }
-    current_size_ -= size;
-  }
-
-  // Put element on the top of the stack.
-  void Push(const CelValue& value) { Push(value, AttributeTrail()); }
-
-  void Push(const CelValue& value, AttributeTrail attribute) {
-    if (current_size_ >= stack_.size()) {
-      GOOGLE_LOG(ERROR) << "No room to push more elements on to ValueStack";
-    }
-    stack_[current_size_] = value;
-    attribute_stack_[current_size_] = attribute;
-    current_size_++;
-  }
-
-  // Replace element on the top of the stack.
-  // Checking that stack is not empty is caller's responsibility.
-  void PopAndPush(const CelValue& value) {
-    PopAndPush(value, AttributeTrail());
-  }
-
-  // Replace element on the top of the stack.
-  // Checking that stack is not empty is caller's responsibility.
-  void PopAndPush(const CelValue& value, AttributeTrail attribute) {
-    if (empty()) {
-      GOOGLE_LOG(ERROR) << "Cannot PopAndPush on empty stack.";
-    }
-    stack_[current_size_ - 1] = value;
-    attribute_stack_[current_size_ - 1] = attribute;
-  }
-
-  // Preallocate stack.
-  void Reserve(size_t size) {
-    stack_.reserve(size);
-    attribute_stack_.reserve(size);
-  }
-
- private:
-  std::vector<CelValue> stack_;
-  std::vector<AttributeTrail> attribute_stack_;
-  size_t current_size_;
-};
-
 class CelExpressionFlatEvaluationState : public CelEvaluationState {
  public:
   CelExpressionFlatEvaluationState(
@@ -196,7 +81,7 @@ class CelExpressionFlatEvaluationState : public CelEvaluationState {
 
   void Reset();
 
-  ValueStack& value_stack() { return value_stack_; }
+  EvaluatorStack& value_stack() { return value_stack_; }
 
   std::vector<IterVarFrame>& iter_stack() { return iter_stack_; }
 
@@ -207,7 +92,7 @@ class CelExpressionFlatEvaluationState : public CelEvaluationState {
   google::protobuf::Arena* arena() { return arena_; }
 
  private:
-  ValueStack value_stack_;
+  EvaluatorStack value_stack_;
   std::set<std::string> iter_variable_names_;
   std::vector<IterVarFrame> iter_stack_;
   google::protobuf::Arena* arena_;
@@ -254,7 +139,7 @@ class ExecutionFrame {
     return absl::OkStatus();
   }
 
-  ValueStack& value_stack() { return state_->value_stack(); }
+  EvaluatorStack& value_stack() { return state_->value_stack(); }
   bool enable_unknowns() const { return enable_unknowns_; }
   bool enable_unknown_function_results() const {
     return enable_unknown_function_results_;

eval/eval/evaluator_core_test.cc

@@ -53,52 +53,6 @@ class FakeIncrementExpressionStep : public ExpressionStep {
   bool ComesFromAst() const override { return true; }
 };
 
-// Test Value Stack Push/Pop operation
-TEST(EvaluatorCoreTest, ValueStackPushPop) {
-  google::protobuf::Arena arena;
-  google::api::expr::v1alpha1::Expr expr;
-  expr.mutable_ident_expr()->set_name("name");
-  CelAttribute attribute(expr, {});
-  ValueStack stack(10);
-  stack.Push(CelValue::CreateInt64(1));
-  stack.Push(CelValue::CreateInt64(2), AttributeTrail());
-  stack.Push(CelValue::CreateInt64(3), AttributeTrail(expr, &arena));
-
-  ASSERT_EQ(stack.Peek().Int64OrDie(), 3);
-  ASSERT_THAT(stack.PeekAttribute().attribute(), NotNull());
-  ASSERT_EQ(*stack.PeekAttribute().attribute(), attribute);
-
-  stack.Pop(1);
-
-  ASSERT_EQ(stack.Peek().Int64OrDie(), 2);
-  ASSERT_EQ(stack.PeekAttribute().attribute(), nullptr);
-
-  stack.Pop(1);
-
-  ASSERT_EQ(stack.Peek().Int64OrDie(), 1);
-  ASSERT_EQ(stack.PeekAttribute().attribute(), nullptr);
-}
-
-// Test that inner stacks within value stack retain the equality of their sizes.
-TEST(EvaluatorCoreTest, ValueStackBalanced) {
-  ValueStack stack(10);
-  ASSERT_EQ(stack.size(), stack.attribute_size());
-
-  stack.Push(CelValue::CreateInt64(1));
-  ASSERT_EQ(stack.size(), stack.attribute_size());
-  stack.Push(CelValue::CreateInt64(2), AttributeTrail());
-  stack.Push(CelValue::CreateInt64(3), AttributeTrail());
-  ASSERT_EQ(stack.size(), stack.attribute_size());
-
-  stack.PopAndPush(CelValue::CreateInt64(4), AttributeTrail());
-  ASSERT_EQ(stack.size(), stack.attribute_size());
-  stack.PopAndPush(CelValue::CreateInt64(5));
-  ASSERT_EQ(stack.size(), stack.attribute_size());
-
-  stack.Pop(3);
-  ASSERT_EQ(stack.size(), stack.attribute_size());
-}
-
 TEST(EvaluatorCoreTest, ExecutionFrameNext) {
   ExecutionPath path;
   auto const_step = absl::make_unique<const FakeConstExpressionStep>();

eval/eval/evaluator_stack.cc

@@ -0,0 +1,22 @@
+#include "eval/eval/evaluator_stack.h"
+
+namespace google {
+namespace api {
+namespace expr {
+namespace runtime {
+
+void EvaluatorStack::Clear() {
+  for (auto& v : stack_) {
+    v = CelValue();
+  }
+  for (auto& attr : attribute_stack_) {
+    attr = AttributeTrail();
+  }
+
+  current_size_ = 0;
+}
+
+}  // namespace runtime
+}  // namespace expr
+}  // namespace api
+}  // namespace google