Remove attribute checking from slot accesses.

eval/eval/BUILD

@@ -419,14 +419,15 @@ cc_library(
         ":expression_step_base",
         "//base:attributes",
         "//base:kind",
+        "//common:casting",
         "//common:value",
         "//eval/internal:errors",
-        "//internal:casts",
         "//internal:status_macros",
         "//runtime/internal:mutable_list_impl",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/types:optional",
+        "@com_google_absl//absl/types:span",
     ],
 )
 

eval/eval/comprehension_step.cc

@@ -8,21 +8,26 @@
 #include "absl/status/status.h"
 #include "absl/strings/str_cat.h"
 #include "absl/types/optional.h"
+#include "absl/types/span.h"
 #include "base/attribute.h"
 #include "base/kind.h"
+#include "common/casting.h"
 #include "common/value.h"
 #include "eval/eval/attribute_trail.h"
 #include "eval/eval/comprehension_slots.h"
 #include "eval/eval/evaluator_core.h"
 #include "eval/eval/expression_step_base.h"
 #include "eval/internal/errors.h"
-#include "internal/casts.h"
 #include "internal/status_macros.h"
 #include "runtime/internal/mutable_list_impl.h"
 
 namespace google::api::expr::runtime {
 namespace {
 
+using ::cel::Cast;
+using ::cel::InstanceOf;
+using ::cel::IntValue;
+using ::cel::ListValue;
 using ::cel::UnknownValue;
 using ::cel::Value;
 using ::cel::runtime_internal::CreateNoMatchingOverloadError;
@@ -181,10 +186,10 @@ absl::Status ComprehensionNextStep::Evaluate(ExecutionFrame* frame) const {
   if (!frame->value_stack().HasEnough(kStackSize)) {
     return absl::Status(absl::StatusCode::kInternal, "Value stack underflow");
   }
-  auto state = frame->value_stack().GetSpan(kStackSize);
+  absl::Span<const Value> state = frame->value_stack().GetSpan(kStackSize);
 
   // Get range from the stack.
-  auto& iter_range = state[POS_ITER_RANGE];
+  const cel::Value& iter_range = state[POS_ITER_RANGE];
   if (!iter_range->Is<cel::ListValue>()) {
     if (iter_range->Is<cel::ErrorValue>() ||
         iter_range->Is<cel::UnknownValue>()) {
@@ -196,46 +201,53 @@ absl::Status ComprehensionNextStep::Evaluate(ExecutionFrame* frame) const {
     }
     return frame->JumpTo(error_jump_offset_);
   }
-  auto iter_range_list = iter_range.As<cel::ListValue>();
+  ListValue iter_range_list = Cast<ListValue>(iter_range);
 
   // Get the current index off the stack.
   const auto& current_index_value = state[POS_CURRENT_INDEX];
-  if (!current_index_value->Is<cel::IntValue>()) {
+  if (!InstanceOf<IntValue>(current_index_value)) {
     return absl::InternalError(absl::StrCat(
         "ComprehensionNextStep: want int, got ",
         cel::KindToString(ValueKindToKind(current_index_value->kind()))));
   }
   CEL_RETURN_IF_ERROR(frame->IncrementIterations());
 
-  int64_t current_index = current_index_value.As<cel::IntValue>().NativeValue();
+  int64_t next_index = Cast<IntValue>(current_index_value).NativeValue() + 1;
+
+  frame->comprehension_slots().Set(accu_slot_, state[POS_LOOP_STEP_ACCU]);
+
+  if (next_index >= static_cast<int64_t>(iter_range_list.Size())) {
+    // Make sure the iter var is out of scope.
+    frame->comprehension_slots().ClearSlot(iter_slot_);
+    // pop loop step
+    frame->value_stack().Pop(1);
+    // jump to result production step
+    return frame->JumpTo(jump_offset_);
+  }
 
-  AttributeTrail iter_range_attr;
   AttributeTrail iter_trail;
   if (frame->enable_unknowns()) {
-    auto attr = frame->value_stack().GetAttributeSpan(kStackSize);
-    iter_range_attr = attr[POS_ITER_RANGE];
     iter_trail =
-        iter_range_attr.Step(cel::AttributeQualifier::OfInt(current_index + 1));
+        frame->value_stack().GetAttributeSpan(kStackSize)[POS_ITER_RANGE].Step(
+            cel::AttributeQualifier::OfInt(next_index));
   }
 
-  // Pop invalidates references to the stack on the following line so copy.
-  Value loop_step = std::move(state[POS_LOOP_STEP_ACCU]);
-  frame->value_stack().Pop(1);
-  frame->comprehension_slots().Set(accu_slot_, std::move(loop_step));
-
-  // Make sure the iter var is out of scope.
-  if (current_index >= static_cast<int64_t>(iter_range_list.Size()) - 1) {
-    frame->comprehension_slots().ClearSlot(iter_slot_);
-    return frame->JumpTo(jump_offset_);
+  Value current_value;
+  if (frame->enable_unknowns() && frame->attribute_utility().CheckForUnknown(
+                                      iter_trail, /*use_partial=*/false)) {
+    current_value =
+        frame->attribute_utility().CreateUnknownSet(iter_trail.attribute());
+  } else {
+    CEL_ASSIGN_OR_RETURN(current_value,
+                         iter_range_list.Get(frame->value_factory(),
+                                             static_cast<size_t>(next_index)));
   }
 
-  current_index += 1;
-
-  CEL_ASSIGN_OR_RETURN(auto current_value,
-                       iter_range_list.Get(frame->value_factory(),
-                                           static_cast<size_t>(current_index)));
+  // pop loop step
+  // pop old current_index
+  // push new current_index
   frame->value_stack().PopAndPush(
-      frame->value_factory().CreateIntValue(current_index));
+      2, frame->value_factory().CreateIntValue(next_index));
   frame->comprehension_slots().Set(iter_slot_, std::move(current_value),
                                    std::move(iter_trail));
   return absl::OkStatus();

eval/eval/ident_step.cc

@@ -115,27 +115,7 @@ absl::Status SlotStep::Evaluate(ExecutionFrame* frame) const {
         absl::StrCat("Comprehension variable accessed out of scope: ", name_));
   }
 
-  const auto& attribute_trail = slot->attribute;
-
-  if (frame->enable_missing_attribute_errors() &&
-      frame->attribute_utility().CheckForMissingAttribute(attribute_trail)) {
-    CEL_ASSIGN_OR_RETURN(std::string attribute,
-                         attribute_trail.attribute().AsString());
-    frame->value_stack().Push(frame->value_factory().CreateErrorValue(
-        CreateMissingAttributeError(std::move(attribute))));
-    return absl::OkStatus();
-  }
-
-  if (frame->enable_unknowns()) {
-    if (frame->attribute_utility().CheckForUnknown(attribute_trail, false)) {
-      auto unknown_set = frame->attribute_utility().CreateUnknownSet(
-          attribute_trail.attribute());
-      frame->value_stack().Push(std::move(unknown_set));
-      return absl::OkStatus();
-    }
-  }
-
-  frame->value_stack().Push(slot->value, attribute_trail);
+  frame->value_stack().Push(slot->value, slot->attribute);
   return absl::OkStatus();
 }
 

eval/tests/BUILD

@@ -181,7 +181,7 @@ cc_test(
     ],
     deps = [
         "//base:attributes",
-        "//eval/eval:evaluator_core",
+        "//base:function_result",
         "//eval/public:activation",
         "//eval/public:builtin_func_registrar",
         "//eval/public:cel_attribute",
@@ -191,15 +191,14 @@ cc_test(
         "//eval/public:cel_options",
         "//eval/public:cel_value",
         "//eval/public:unknown_set",
-        "//eval/public/containers:container_backed_list_impl",
         "//eval/public/containers:container_backed_map_impl",
         "//eval/public/structs:cel_proto_wrapper",
-        "//internal:status_macros",
         "//internal:testing",
-        "@com_google_absl//absl/container:btree",
+        "//parser",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/types:span",
+        "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto",
         "@com_google_protobuf//:protobuf",
     ],
 )

eval/tests/unknowns_end_to_end_test.cc

@@ -4,18 +4,17 @@
 // the unknowns is particular to the runtime.
 
 #include <memory>
+#include <string>
 #include <utility>
 #include <vector>
 
+#include "google/api/expr/v1alpha1/syntax.pb.h"
 #include "google/protobuf/struct.pb.h"
-#include "google/protobuf/arena.h"
-#include "google/protobuf/text_format.h"
-#include "absl/container/btree_map.h"
 #include "absl/status/status.h"
 #include "absl/strings/string_view.h"
 #include "absl/types/span.h"
 #include "base/attribute.h"
-#include "eval/eval/evaluator_core.h"
+#include "base/function_result.h"
 #include "eval/public/activation.h"
 #include "eval/public/builtin_func_registrar.h"
 #include "eval/public/cel_attribute.h"
@@ -24,12 +23,13 @@
 #include "eval/public/cel_function.h"
 #include "eval/public/cel_options.h"
 #include "eval/public/cel_value.h"
-#include "eval/public/containers/container_backed_list_impl.h"
 #include "eval/public/containers/container_backed_map_impl.h"
 #include "eval/public/structs/cel_proto_wrapper.h"
 #include "eval/public/unknown_set.h"
-#include "internal/status_macros.h"
 #include "internal/testing.h"
+#include "parser/parser.h"
+#include "google/protobuf/arena.h"
+#include "google/protobuf/text_format.h"
 
 namespace google {
 namespace api {
@@ -38,6 +38,8 @@ namespace runtime {
 namespace {
 
 using google::api::expr::v1alpha1::Expr;
+using google::api::expr::v1alpha1::ParsedExpr;
+using ::google::api::expr::parser::Parse;
 using ::google::protobuf::Arena;
 using testing::ElementsAre;
 
@@ -975,6 +977,51 @@ constexpr char kFilterElementsComp[] = R"pb(
     }
   })pb";
 
+TEST(UnknownsIterAttrTest, IterAttributeTrailExact) {
+  InterpreterOptions options;
+  Activation activation;
+  Arena arena;
+
+  ASSERT_OK_AND_ASSIGN(ParsedExpr expr, Parse("list_var.exists(x, x)"));
+
+  protobuf::Value element;
+  element.set_bool_value(false);
+  protobuf::ListValue list;
+  *list.add_values() = element;
+  *list.add_values() = element;
+  *list.add_values() = element;
+
+  (*list.mutable_values())[0].set_bool_value(true);
+
+  options.unknown_processing = UnknownProcessingOptions::kAttributeAndFunction;
+  auto builder = CreateCelExpressionBuilder(options);
+  ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry()));
+  activation.InsertValue("list_var",
+                         CelProtoWrapper::CreateMessage(&list, &arena));
+
+  // list_var[0]
+  std::vector<CelAttributePattern> unknown_attribute_patterns;
+  unknown_attribute_patterns.push_back(CelAttributePattern(
+      "list_var",
+      {CreateCelAttributeQualifierPattern(CelValue::CreateInt64(0))}));
+  activation.set_unknown_attribute_patterns(
+      std::move(unknown_attribute_patterns));
+
+  ASSERT_OK_AND_ASSIGN(
+      auto plan, builder->CreateExpression(&expr.expr(), &expr.source_info()));
+  CelValue response = plan->Evaluate(activation, &arena).value();
+
+  ASSERT_TRUE(response.IsUnknownSet()) << CelValue::TypeName(response.type());
+  ASSERT_EQ(response.UnknownSetOrDie()->unknown_attributes().size(), 1);
+
+  ASSERT_EQ(response.UnknownSetOrDie()
+                ->unknown_attributes()
+                .begin()
+                ->qualifier_path()
+                .size(),
+            1);
+}
+
 TEST(UnknownsIterAttrTest, IterAttributeTrailFilterValues) {
   InterpreterOptions options;
   Expr expr;