Add recursive eval support for shadowable values.

eval/compiler/cel_expression_builder_flat_impl_test.cc

@@ -127,6 +127,7 @@ TEST_P(RecursivePlanTest, ParsedExprRecursiveImpl) {
       std::make_unique<ProtobufDescriptorProvider>(
           google::protobuf::DescriptorPool::generated_pool(),
           google::protobuf::MessageFactory::generated_factory()));
+  builder.GetTypeRegistry()->RegisterEnum("TestEnum", {{"FOO", 1}, {"BAR", 2}});
   ASSERT_OK(RegisterBuiltinFunctions(builder.GetRegistry()));
 
   ASSERT_OK_AND_ASSIGN(std::unique_ptr<CelExpression> plan,
@@ -148,6 +149,7 @@ TEST_P(RecursivePlanTest, ParsedExprRecursiveImpl) {
   msg.mutable_child()->mutable_payload()->set_single_int64(42);
   activation.InsertValue("struct_var",
                          CelProtoWrapper::CreateMessage(&msg, &arena));
+  activation.InsertValue("TestEnum.BAR", CelValue::CreateInt64(-1));
 
   ASSERT_OK_AND_ASSIGN(CelValue result, plan->Evaluate(activation, &arena));
   EXPECT_THAT(result, test_case.matcher);
@@ -167,6 +169,7 @@ TEST_P(RecursivePlanTest, ParsedExprRecursiveOptimizedImpl) {
       std::make_unique<ProtobufDescriptorProvider>(
           google::protobuf::DescriptorPool::generated_pool(),
           google::protobuf::MessageFactory::generated_factory()));
+  builder.GetTypeRegistry()->RegisterEnum("TestEnum", {{"FOO", 1}, {"BAR", 2}});
   ASSERT_OK(RegisterBuiltinFunctions(builder.GetRegistry()));
 
   builder.flat_expr_builder().AddProgramOptimizer(
@@ -192,6 +195,7 @@ TEST_P(RecursivePlanTest, ParsedExprRecursiveOptimizedImpl) {
   msg.mutable_child()->mutable_payload()->set_single_int64(42);
   activation.InsertValue("struct_var",
                          CelProtoWrapper::CreateMessage(&msg, &arena));
+  activation.InsertValue("TestEnum.BAR", CelValue::CreateInt64(-1));
 
   ASSERT_OK_AND_ASSIGN(CelValue result, plan->Evaluate(activation, &arena));
   EXPECT_THAT(result, test_case.matcher);
@@ -214,6 +218,7 @@ TEST_P(RecursivePlanTest, ParsedExprRecursiveTraceSupport) {
       std::make_unique<ProtobufDescriptorProvider>(
           google::protobuf::DescriptorPool::generated_pool(),
           google::protobuf::MessageFactory::generated_factory()));
+  builder.GetTypeRegistry()->RegisterEnum("TestEnum", {{"FOO", 1}, {"BAR", 2}});
   ASSERT_OK(RegisterBuiltinFunctions(builder.GetRegistry()));
 
   ASSERT_OK_AND_ASSIGN(std::unique_ptr<CelExpression> plan,
@@ -235,6 +240,7 @@ TEST_P(RecursivePlanTest, ParsedExprRecursiveTraceSupport) {
   msg.mutable_child()->mutable_payload()->set_single_int64(42);
   activation.InsertValue("struct_var",
                          CelProtoWrapper::CreateMessage(&msg, &arena));
+  activation.InsertValue("TestEnum.BAR", CelValue::CreateInt64(-1));
 
   ASSERT_OK_AND_ASSIGN(CelValue result, plan->Trace(activation, &arena, cb));
   EXPECT_THAT(result, test_case.matcher);
@@ -255,6 +261,7 @@ TEST_P(RecursivePlanTest, Disabled) {
       std::make_unique<ProtobufDescriptorProvider>(
           google::protobuf::DescriptorPool::generated_pool(),
           google::protobuf::MessageFactory::generated_factory()));
+  builder.GetTypeRegistry()->RegisterEnum("TestEnum", {{"FOO", 1}, {"BAR", 2}});
   ASSERT_OK(RegisterBuiltinFunctions(builder.GetRegistry()));
 
   ASSERT_OK_AND_ASSIGN(std::unique_ptr<CelExpression> plan,
@@ -276,6 +283,7 @@ TEST_P(RecursivePlanTest, Disabled) {
   msg.mutable_child()->mutable_payload()->set_single_int64(42);
   activation.InsertValue("struct_var",
                          CelProtoWrapper::CreateMessage(&msg, &arena));
+  activation.InsertValue("TestEnum.BAR", CelValue::CreateInt64(-1));
 
   ASSERT_OK_AND_ASSIGN(CelValue result, plan->Evaluate(activation, &arena));
   EXPECT_THAT(result, test_case.matcher);
@@ -321,8 +329,11 @@ INSTANTIATE_TEST_SUITE_P(
          test::IsCelInt64(50)},
         {"bind_with_comprehensions",
          R"(cel.bind(x, [1, 2], cel.bind(y, x.map(z, z * 2), y.exists(z, z == 4))))",
+         test::IsCelBool(true)},
+        {"shadowable_value_default", R"(TestEnum.FOO == 1)",
+         test::IsCelBool(true)},
+        {"shadowable_value_shadowed", R"(TestEnum.BAR == -1)",
          test::IsCelBool(true)}}),
-
     [](const testing::TestParamInfo<RecursiveTestCase>& info) -> std::string {
       return info.param.test_name;
     });

eval/compiler/flat_expr_builder.cc

@@ -533,7 +533,7 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor {
     if (!progress_status_.ok()) {
       return;
     }
-    const std::string& path = ident_expr->name();
+    std::string path = ident_expr->name();
     if (!ValidateOrError(
             !path.empty(),
             "Invalid expression: identifier 'name' must not be empty")) {
@@ -542,6 +542,9 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor {
 
     // Attempt to resolve a select expression as a namespaced identifier for an
     // enum or type constant value.
+    absl::optional<cel::Value> const_value;
+    int64_t select_root_id = -1;
+
     while (!namespace_stack_.empty()) {
       const auto& select_node = namespace_stack_.front();
       // Generate path in format "<ident>.<field 0>.<field 1>...".
@@ -552,23 +555,34 @@ class FlatExprVisitor : public cel::ast_internal::AstVisitor {
       // qualified path present in the expression. Whether the identifier
       // can be resolved to a type instance depends on whether the option to
       // 'enable_qualified_type_identifiers' is set to true.
-      auto const_value =
-          resolver_.FindConstant(qualified_path, select_expr->id());
+      const_value = resolver_.FindConstant(qualified_path, select_expr->id());
       if (const_value) {
-        AddStep(CreateShadowableValueStep(
-            qualified_path, std::move(*const_value), select_expr->id()));
         resolved_select_expr_ = select_expr;
+        select_root_id = select_expr->id();
+        path = qualified_path;
         namespace_stack_.clear();
-        return;
+        break;
       }
       namespace_stack_.pop_front();
     }
 
-    // Attempt to resolve a simple identifier as an enum or type constant value.
-    auto const_value = resolver_.FindConstant(path, expr->id());
+    if (!const_value) {
+      // Attempt to resolve a simple identifier as an enum or type constant
+      // value.
+      const_value = resolver_.FindConstant(path, expr->id());
+      select_root_id = expr->id();
+    }
+
     if (const_value) {
-      AddStep(
-          CreateShadowableValueStep(path, std::move(*const_value), expr->id()));
+      if (options_.max_recursion_depth != 0) {
+        SetRecursiveStep(CreateDirectShadowableValueStep(
+                             std::move(path), std::move(const_value).value(),
+                             select_root_id),
+                         1);
+        return;
+      }
+      AddStep(CreateShadowableValueStep(
+          std::move(path), std::move(const_value).value(), select_root_id));
       return;
     }
 

eval/compiler/flat_expr_builder_extensions.cc

@@ -34,8 +34,37 @@
 
 namespace google::api::expr::runtime {
 
+namespace {
+
 using Subexpression = google::api::expr::runtime::ProgramBuilder::Subexpression;
 
+// Remap a recursive program to its parent if the parent is a transparent
+// wrapper.
+void MaybeReassignChildRecursiveProgram(Subexpression* parent) {
+  if (parent->IsFlattened() || parent->IsRecursive()) {
+    return;
+  }
+  if (parent->elements().size() != 1) {
+    return;
+  }
+  auto* child_alternative =
+      absl::get_if<std::unique_ptr<Subexpression>>(&parent->elements()[0]);
+  if (child_alternative == nullptr) {
+    return;
+  }
+
+  auto& child_subexpression = *child_alternative;
+  if (!child_subexpression->IsRecursive()) {
+    return;
+  }
+
+  auto child_program = child_subexpression->ExtractRecursiveProgram();
+  parent->set_recursive_program(std::move(child_program.step),
+                                child_program.depth);
+}
+
+}  // namespace
+
 Subexpression::Subexpression(const cel::ast_internal::Expr* self,
                              ProgramBuilder* owner)
     : self_(self), parent_(nullptr), subprogram_map_(owner->subprogram_map_) {}
@@ -302,6 +331,8 @@ absl::Nullable<Subexpression*> ProgramBuilder::ExitSubexpression(
   ABSL_DCHECK(expr == current_->self_);
   ABSL_DCHECK(GetSubexpression(expr) == current_);
 
+  MaybeReassignChildRecursiveProgram(current_);
+
   Subexpression* result = GetSubexpression(current_->parent_);
   ABSL_DCHECK(result != nullptr || current_ == root_.get());
   current_ = result;

eval/eval/BUILD

@@ -1008,10 +1008,14 @@ cc_library(
     srcs = ["shadowable_value_step.cc"],
     hdrs = ["shadowable_value_step.h"],
     deps = [
+        ":attribute_trail",
+        ":direct_expression_step",
         ":evaluator_core",
         ":expression_step_base",
         "//common:value",
         "//internal:status_macros",
+        "@com_google_absl//absl/memory",
+        "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
     ],
 )

eval/eval/shadowable_value_step.cc

@@ -5,14 +5,22 @@
 #include <string>
 #include <utility>
 
+#include "absl/memory/memory.h"
+#include "absl/status/status.h"
 #include "absl/status/statusor.h"
+#include "common/value.h"
+#include "eval/eval/attribute_trail.h"
+#include "eval/eval/direct_expression_step.h"
+#include "eval/eval/evaluator_core.h"
 #include "eval/eval/expression_step_base.h"
 #include "internal/status_macros.h"
 
 namespace google::api::expr::runtime {
 
 namespace {
 
+using ::cel::Value;
+
 class ShadowableValueStep : public ExpressionStepBase {
  public:
   ShadowableValueStep(std::string identifier, cel::Value value, int64_t expr_id)
@@ -24,7 +32,7 @@ class ShadowableValueStep : public ExpressionStepBase {
 
  private:
   std::string identifier_;
-  cel::Value value_;
+  Value value_;
 };
 
 absl::Status ShadowableValueStep::Evaluate(ExecutionFrame* frame) const {
@@ -40,6 +48,39 @@ absl::Status ShadowableValueStep::Evaluate(ExecutionFrame* frame) const {
   return absl::OkStatus();
 }
 
+class DirectShadowableValueStep : public DirectExpressionStep {
+ public:
+  DirectShadowableValueStep(std::string identifier, cel::Value value,
+                            int64_t expr_id)
+      : DirectExpressionStep(expr_id),
+        identifier_(std::move(identifier)),
+        value_(std::move(value)) {}
+
+  absl::Status Evaluate(ExecutionFrameBase& frame, Value& result,
+                        AttributeTrail& attribute) const override;
+
+ private:
+  std::string identifier_;
+  Value value_;
+};
+
+// TODO(uncreated-issue/67): Attribute tracking is skipped for the shadowed case. May
+// cause problems for users with unknown tracking and variables named like
+// 'list' etc, but follows the current behavior of the stack machine version.
+absl::Status DirectShadowableValueStep::Evaluate(
+    ExecutionFrameBase& frame, Value& result, AttributeTrail& attribute) const {
+  cel::Value scratch;
+  CEL_ASSIGN_OR_RETURN(auto var,
+                       frame.activation().FindVariable(frame.value_manager(),
+                                                       identifier_, scratch));
+  if (var.has_value()) {
+    result = *var;
+  } else {
+    result = value_;
+  }
+  return absl::OkStatus();
+}
+
 }  // namespace
 
 absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateShadowableValueStep(
@@ -48,4 +89,10 @@ absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateShadowableValueStep(
                                                 std::move(value), expr_id);
 }
 
+std::unique_ptr<DirectExpressionStep> CreateDirectShadowableValueStep(
+    std::string identifier, cel::Value value, int64_t expr_id) {
+  return std::make_unique<DirectShadowableValueStep>(std::move(identifier),
+                                                     std::move(value), expr_id);
+}
+
 }  // namespace google::api::expr::runtime

eval/eval/shadowable_value_step.h

@@ -7,6 +7,7 @@
 
 #include "absl/status/statusor.h"
 #include "common/value.h"
+#include "eval/eval/direct_expression_step.h"
 #include "eval/eval/evaluator_core.h"
 
 namespace google::api::expr::runtime {
@@ -17,6 +18,9 @@ namespace google::api::expr::runtime {
 absl::StatusOr<std::unique_ptr<ExpressionStep>> CreateShadowableValueStep(
     std::string identifier, cel::Value value, int64_t expr_id);
 
+std::unique_ptr<DirectExpressionStep> CreateDirectShadowableValueStep(
+    std::string identifier, cel::Value value, int64_t expr_id);
+
 }  // namespace google::api::expr::runtime
 
 #endif  // THIRD_PARTY_CEL_CPP_EVAL_EVAL_SHADOWABLE_VALUE_STEP_H_