Add special case for identifier step.

eval/compiler/flat_expr_builder.cc

@@ -916,7 +916,7 @@ class FlatExprVisitor : public cel::AstVisitor {
     if (options_.max_recursion_depth != 0) {
       SetRecursiveStep(CreateDirectIdentStep(ident_name, expr.id()), 1);
     } else {
-      AddStep(CreateIdentStep(ident_name), expr.id());
+      AddStep(ExpressionStep::MakeIdentifierStep(ident_name, expr.id()));
     }
   }
 

eval/eval/BUILD

@@ -39,6 +39,7 @@ cc_library(
         "equality_steps.cc",
         "evaluator_core.cc",
         "function_step.cc",
+        "ident_step.cc",
         "lazy_init_step.cc",
         "logic_step.cc",
     ],
@@ -47,6 +48,7 @@ cc_library(
         "equality_steps.h",
         "evaluator_core.h",
         "function_step.h",
+        "ident_step.h",
         "lazy_init_step.h",
         "logic_step.h",
     ],
@@ -288,26 +290,8 @@ cc_library(
 
 cc_library(
     name = "ident_step",
-    srcs = [
-        "ident_step.cc",
-    ],
-    hdrs = [
-        "ident_step.h",
-    ],
     deps = [
-        ":attribute_trail",
-        ":comprehension_slots",
-        ":direct_expression_step",
         ":evaluator_core",
-        ":expression_step_base",
-        ":expression_step_logic",
-        "//common:value",
-        "//eval/internal:errors",
-        "//internal:status_macros",
-        "@com_google_absl//absl/base:nullability",
-        "@com_google_absl//absl/status",
-        "@com_google_absl//absl/status:statusor",
-        "@com_google_absl//absl/strings",
     ],
 )
 

eval/eval/comprehension_step_test.cc

@@ -94,7 +94,7 @@ MATCHER_P(CelStringValue, val, "") {
 
 TEST_F(ListKeysStepTest, MapPartiallyUnknown) {
   ExecutionPath path;
-  path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("var")));
+  path.push_back(ExpressionStep::MakeIdentifierStep("var"));
   auto init_step =
       std::make_unique<ComprehensionInitStep>(/*iter_slot=*/0, /*accu_slot=*/0);
   init_step->set_error_jump_offset(1);
@@ -132,7 +132,7 @@ TEST_F(ListKeysStepTest, MapPartiallyUnknown) {
 
 TEST_F(ListKeysStepTest, ErrorPassedThrough) {
   ExecutionPath path;
-  path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("var")));
+  path.push_back(ExpressionStep::MakeIdentifierStep("var"));
   auto init_step =
       std::make_unique<ComprehensionInitStep>(/*iter_slot=*/0, /*accu_slot=*/0);
   init_step->set_error_jump_offset(1);
@@ -157,7 +157,7 @@ TEST_F(ListKeysStepTest, ErrorPassedThrough) {
 
 TEST_F(ListKeysStepTest, UnknownSetPassedThrough) {
   ExecutionPath path;
-  path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("var")));
+  path.push_back(ExpressionStep::MakeIdentifierStep("var"));
   auto init_step =
       std::make_unique<ComprehensionInitStep>(/*iter_slot=*/0, /*accu_slot=*/0);
   init_step->set_error_jump_offset(1);

eval/eval/container_access_step_test.cc

@@ -83,9 +83,8 @@ CelValue EvaluateAttributeHelper(
             /*enable_optional_types=*/false, 3)),
         3));
   } else {
-    path.push_back(
-        ExpressionStep::MakeGenericStep(CreateIdentStep("container"), 1));
-    path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("key"), 2));
+    path.push_back(ExpressionStep::MakeIdentifierStep("container", 1));
+    path.push_back(ExpressionStep::MakeIdentifierStep("key", 2));
     path.push_back(ExpressionStep::MakeGenericStep(
         std::move(CreateContainerAccessStep(call).value()), 3));
   }

eval/eval/create_list_step_test.cc

@@ -119,7 +119,7 @@ absl::StatusOr<CelValue> RunExpressionWithCelValues(
     expr0.set_id(ind);
     expr0.mutable_ident_expr().set_name(var_name);
 
-    path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep(var_name)));
+    path.push_back(ExpressionStep::MakeIdentifierStep(var_name));
     activation.InsertValue(var_name, value);
   }
 

eval/eval/create_map_step_test.cc

@@ -71,11 +71,8 @@ absl::StatusOr<ExecutionPath> CreateStackMachineProgram(
     std::string key_name = absl::StrCat("key", index);
     std::string value_name = absl::StrCat("value", index);
 
-    auto step_key = CreateIdentStep(key_name);
-    auto step_value = CreateIdentStep(value_name);
-
-    path.push_back(ExpressionStep::MakeGenericStep(std::move(step_key)));
-    path.push_back(ExpressionStep::MakeGenericStep(std::move(step_value)));
+    path.push_back(ExpressionStep::MakeIdentifierStep(key_name));
+    path.push_back(ExpressionStep::MakeIdentifierStep(value_name));
 
     activation.InsertValue(key_name, item.first);
     activation.InsertValue(value_name, item.second);

eval/eval/create_struct_step_test.cc

@@ -72,13 +72,11 @@ using ::testing::Pointwise;
 absl::StatusOr<ExecutionPath> MakeStackMachinePath(absl::string_view field) {
   ExecutionPath path;
 
-  auto step0 = CreateIdentStep("message");
-
   auto step1 = CreateCreateStructStep("google.api.expr.runtime.TestMessage",
                                       {std::string(field)},
                                       /*optional_indices=*/{});
 
-  path.push_back(ExpressionStep::MakeGenericStep(std::move(step0)));
+  path.push_back(ExpressionStep::MakeIdentifierStep("message"));
   path.push_back(ExpressionStep::MakeGenericStep(std::move(step1)));
 
   return path;

eval/eval/evaluator_core.h

@@ -19,6 +19,7 @@
 #include <cstdint>
 #include <limits>
 #include <memory>
+#include <string>
 #include <utility>
 #include <vector>
 
@@ -44,6 +45,7 @@
 #include "eval/eval/evaluator_stack.h"
 #include "eval/eval/expression_step_logic.h"
 #include "eval/eval/function_step.h"
+#include "eval/eval/ident_step.h"
 #include "eval/eval/iterator_stack.h"
 #include "eval/eval/lazy_init_step.h"
 #include "eval/eval/logic_step.h"
@@ -99,17 +101,19 @@ enum class ExpressionStepKind : uint16_t {
   kBooleanAndJump = 22,
   kTernaryJump = 23,
   kFixedJump = 24,
+  // Identifier
+  kIdentifier = 25,
   // Functions calls.
-  kEagerFunction = 25,
-  kLazyFunction = 26,
+  kEagerFunction = 26,
+  kLazyFunction = 27,
   // fast built-ins. These are used if we know they haven't been extended.
   // otherwise we use normal function call steps.
-  kFastIn = 27,
-  kFastEqual = 28,
-  kFastNotEqual = 29,
+  kFastIn = 28,
+  kFastEqual = 29,
+  kFastNotEqual = 30,
   // Special built-in steps for mutable lists implementing map/filter.
-  kNewMutableList = 30,
-  kMutableListAppend = 31,
+  kNewMutableList = 31,
+  kMutableListAppend = 32,
 };
 
 struct BoolJumpStepInfo {
@@ -288,6 +292,18 @@ class ExpressionStep {
     return step;
   }
 
+  static ExpressionStep MakeIdentifierStep(absl::string_view identifier,
+                                           int64_t id = -1) {
+    ExpressionStep step(ExpressionStepKind::kIdentifier, id);
+    step.u_.identifier = new std::string(identifier);
+    return step;
+  }
+
+  static ExpressionStep MakeIdentStep(absl::string_view identifier,
+                                      int64_t id = -1) {
+    return MakeIdentifierStep(identifier, id);
+  }
+
   static ExpressionStep MakeFastInStep(int64_t id = -1) {
     return ExpressionStep(ExpressionStepKind::kFastIn, id);
   }
@@ -391,6 +407,7 @@ class ExpressionStep {
     FixedJumpStepInfo fixed_jump_step;
     EagerFunctionStep* eager_function_step;
     LazyFunctionStep* lazy_function_step;
+    std::string* identifier;
 
     Data() : empty(nullptr) {}
     ~Data() {}
@@ -949,6 +966,9 @@ inline ExpressionStep::~ExpressionStep() {
     case ExpressionStepKind::kOtherConstant:
       delete u_.other_val;
       break;
+    case ExpressionStepKind::kIdentifier:
+      delete u_.identifier;
+      break;
     case ExpressionStepKind::kEagerFunction:
       delete u_.eager_function_step;
       break;
@@ -1149,6 +1169,9 @@ inline void ExpressionStep::Evaluate(ExecutionFrame& frame) const {
           << "FixedJumpStep did not have a value set.";
       frame.JumpToOrAbort(u_.fixed_jump_step.offset);
       break;
+    case ExpressionStepKind::kIdentifier:
+      EvaluateIdentifierStep(*u_.identifier, frame);
+      break;
     case ExpressionStepKind::kEagerFunction:
       u_.eager_function_step->Evaluate(frame);
       break;

eval/eval/function_step_test.cc

@@ -674,8 +674,7 @@ TEST_P(FunctionStepTestUnknowns, PartialUnknownHandlingTest) {
   IdentExpr ident1;
   ident1.set_name("param");
   CallExpr call1 = SinkFunction::MakeCall();
-  auto step0 =
-      ExpressionStep::MakeGenericStep(CreateIdentStep("param"), GetExprId());
+  auto step0 = ExpressionStep::MakeIdentifierStep("param", GetExprId());
   ASSERT_OK_AND_ASSIGN(auto step1, MakeTestFunctionStep(call1, registry));
 
   path.push_back(std::move(step0));

eval/eval/ident_step.cc

@@ -16,7 +16,6 @@
 #include "eval/eval/comprehension_slots.h"
 #include "eval/eval/direct_expression_step.h"
 #include "eval/eval/evaluator_core.h"
-#include "eval/eval/expression_step_base.h"
 #include "eval/eval/expression_step_logic.h"
 #include "eval/internal/errors.h"
 #include "internal/status_macros.h"
@@ -28,16 +27,6 @@ namespace {
 using ::cel::Value;
 using ::cel::runtime_internal::CreateError;
 
-class IdentStep : public ExpressionStepBase {
- public:
-  explicit IdentStep(absl::string_view name) : name_(name) {}
-
-  void Evaluate(ExecutionFrame* frame) const override;
-
- private:
-  std::string name_;
-};
-
 absl::Status LookupIdent(absl::string_view name, ExecutionFrameBase& frame,
                          Value& result, AttributeTrail& attribute) {
   if (frame.attribute_tracking_enabled()) {
@@ -74,19 +63,6 @@ absl::Status LookupIdent(absl::string_view name, ExecutionFrameBase& frame,
   return absl::OkStatus();
 }
 
-void IdentStep::Evaluate(ExecutionFrame* frame) const {
-  Value value;
-  AttributeTrail attribute;
-
-  if (absl::Status status = LookupIdent(name_, *frame, value, attribute);
-      !status.ok()) {
-    frame->Abort(std::move(status));
-    return;
-  }
-
-  frame->value_stack().Push(std::move(value), std::move(attribute));
-}
-
 absl::StatusOr<ComprehensionSlots::Slot* absl_nonnull> LookupSlot(
     absl::string_view name, size_t slot_index, ExecutionFrameBase& frame) {
   ComprehensionSlots::Slot* slot = frame.comprehension_slots().Get(slot_index);
@@ -137,6 +113,18 @@ class DirectSlotStep : public DirectExpressionStep {
 
 }  // namespace
 
+void EvaluateIdentifierStep(absl::string_view identifier,
+                            ExecutionFrame& frame) {
+  frame.value_stack().Push(cel::NullValue());
+  if (absl::Status status =
+          LookupIdent(identifier, frame, frame.value_stack().Peek(),
+                      frame.value_stack().PeekAttribute());
+      !status.ok()) {
+    frame.Abort(std::move(status));
+    return;
+  }
+}
+
 std::unique_ptr<DirectExpressionStep> CreateDirectIdentStep(
     absl::string_view identifier, int64_t expr_id) {
   return std::make_unique<DirectIdentStep>(identifier, expr_id);
@@ -147,9 +135,4 @@ std::unique_ptr<DirectExpressionStep> CreateDirectSlotIdentStep(
   return std::make_unique<DirectSlotStep>(identifier, slot_index, expr_id);
 }
 
-std::unique_ptr<ExpressionStepLogic> CreateIdentStep(
-    const absl::string_view name) {
-  return std::make_unique<IdentStep>(name);
-}
-
 }  // namespace google::api::expr::runtime

eval/eval/ident_step.h

@@ -11,14 +11,16 @@
 
 namespace google::api::expr::runtime {
 
+class ExecutionFrame;
+
 std::unique_ptr<DirectExpressionStep> CreateDirectIdentStep(
     absl::string_view identifier, int64_t expr_id);
 
 std::unique_ptr<DirectExpressionStep> CreateDirectSlotIdentStep(
     absl::string_view identifier, size_t slot_index, int64_t expr_id);
 
-// Factory method for Ident - based Execution step
-std::unique_ptr<ExpressionStepLogic> CreateIdentStep(absl::string_view name);
+void EvaluateIdentifierStep(absl::string_view identifier,
+                            ExecutionFrame& frame);
 
 }  // namespace google::api::expr::runtime
 

eval/eval/ident_step_test.cc

@@ -47,10 +47,8 @@ using ::testing::HasSubstr;
 using ::testing::SizeIs;
 
 TEST(IdentStepTest, TestIdentStep) {
-  auto step = CreateIdentStep("name0");
-
   ExecutionPath path;
-  path.push_back(ExpressionStep::MakeGenericStep(std::move(step)));
+  path.push_back(ExpressionStep::MakeIdentifierStep("name0"));
 
   auto env = NewTestingRuntimeEnv();
   CelExpressionFlatImpl impl(
@@ -73,10 +71,8 @@ TEST(IdentStepTest, TestIdentStep) {
 }
 
 TEST(IdentStepTest, TestIdentStepNameNotFound) {
-  auto step = CreateIdentStep("name0");
-
   ExecutionPath path;
-  path.push_back(ExpressionStep::MakeGenericStep(std::move(step)));
+  path.push_back(ExpressionStep::MakeIdentifierStep("name0"));
 
   auto env = NewTestingRuntimeEnv();
   CelExpressionFlatImpl impl(
@@ -96,10 +92,8 @@ TEST(IdentStepTest, TestIdentStepNameNotFound) {
 }
 
 TEST(IdentStepTest, DisableMissingAttributeErrorsOK) {
-  auto step = CreateIdentStep("name0");
-
   ExecutionPath path;
-  path.push_back(ExpressionStep::MakeGenericStep(std::move(step)));
+  path.push_back(ExpressionStep::MakeIdentifierStep("name0"));
   cel::RuntimeOptions options;
   options.unknown_processing = cel::UnknownProcessingOptions::kDisabled;
   auto env = NewTestingRuntimeEnv();
@@ -132,10 +126,8 @@ TEST(IdentStepTest, DisableMissingAttributeErrorsOK) {
 }
 
 TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) {
-  auto step = CreateIdentStep("name0");
-
   ExecutionPath path;
-  path.push_back(ExpressionStep::MakeGenericStep(std::move(step)));
+  path.push_back(ExpressionStep::MakeIdentifierStep("name0"));
 
   cel::RuntimeOptions options;
   options.unknown_processing = cel::UnknownProcessingOptions::kDisabled;
@@ -172,10 +164,8 @@ TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) {
 }
 
 TEST(IdentStepTest, TestIdentStepUnknownAttribute) {
-  auto step = CreateIdentStep("name0");
-
   ExecutionPath path;
-  path.push_back(ExpressionStep::MakeGenericStep(std::move(step)));
+  path.push_back(ExpressionStep::MakeIdentifierStep("name0"));
 
   // Expression with unknowns enabled.
   cel::RuntimeOptions options;

eval/eval/logic_step_test.cc

@@ -67,8 +67,8 @@ class LogicStepTest : public testing::TestWithParam<bool> {
   absl::Status EvaluateLogic(CelValue arg0, CelValue arg1, bool is_or,
                              CelValue* result, bool enable_unknown) {
     ExecutionPath path;
-    path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("name0")));
-    path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("name1")));
+    path.push_back(ExpressionStep::MakeIdentifierStep("name0"));
+    path.push_back(ExpressionStep::MakeIdentifierStep("name1"));
     path.push_back(
         (is_or) ? ExpressionStep::MakeBooleanOrStep(/*num_args=*/2, /*id=*/2)
                 : ExpressionStep::MakeBooleanAndStep(/*num_args=*/2, /*id=*/2));