From 840daaa041d9becce386214f99e4b2141c9bcdf1 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Thu, 4 Apr 2024 16:44:19 -0700 Subject: [PATCH] Remove attribute checking from slot accesses. Move responsibility for checking exact match to the step setting the iter_var slot. PiperOrigin-RevId: 622008215 --- eval/eval/BUILD | 3 +- eval/eval/comprehension_step.cc | 62 +++++++++++++++----------- eval/eval/ident_step.cc | 22 +-------- eval/tests/BUILD | 7 ++- eval/tests/unknowns_end_to_end_test.cc | 59 +++++++++++++++++++++--- 5 files changed, 96 insertions(+), 57 deletions(-) diff --git a/eval/eval/BUILD b/eval/eval/BUILD index 81ea1c980..36b6e0e35 100644 --- a/eval/eval/BUILD +++ b/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", ], ) diff --git a/eval/eval/comprehension_step.cc b/eval/eval/comprehension_step.cc index d1cacd3b6..f93eedde9 100644 --- a/eval/eval/comprehension_step.cc +++ b/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 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()) { if (iter_range->Is() || iter_range->Is()) { @@ -196,46 +201,53 @@ absl::Status ComprehensionNextStep::Evaluate(ExecutionFrame* frame) const { } return frame->JumpTo(error_jump_offset_); } - auto iter_range_list = iter_range.As(); + ListValue iter_range_list = Cast(iter_range); // Get the current index off the stack. const auto& current_index_value = state[POS_CURRENT_INDEX]; - if (!current_index_value->Is()) { + if (!InstanceOf(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().NativeValue(); + int64_t next_index = Cast(current_index_value).NativeValue() + 1; + + frame->comprehension_slots().Set(accu_slot_, state[POS_LOOP_STEP_ACCU]); + + if (next_index >= static_cast(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(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(next_index))); } - current_index += 1; - - CEL_ASSIGN_OR_RETURN(auto current_value, - iter_range_list.Get(frame->value_factory(), - static_cast(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(); diff --git a/eval/eval/ident_step.cc b/eval/eval/ident_step.cc index 1665458de..aab002b82 100644 --- a/eval/eval/ident_step.cc +++ b/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(); } diff --git a/eval/tests/BUILD b/eval/tests/BUILD index 5f4aacf66..8f4bb1536 100644 --- a/eval/tests/BUILD +++ b/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", ], ) diff --git a/eval/tests/unknowns_end_to_end_test.cc b/eval/tests/unknowns_end_to_end_test.cc index 9e6300690..776d68e52 100644 --- a/eval/tests/unknowns_end_to_end_test.cc +++ b/eval/tests/unknowns_end_to_end_test.cc @@ -4,18 +4,17 @@ // the unknowns is particular to the runtime. #include +#include #include #include +#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 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;