From 694dd58cbeef47e88d9f185ae7759e113449da37 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 15 Apr 2026 11:41:20 -0700 Subject: [PATCH] Refactor select step impl to behave consistently between iterative and recursive programs. PiperOrigin-RevId: 900271414 --- eval/eval/select_step.cc | 355 ++++++++++++++------------------------- 1 file changed, 125 insertions(+), 230 deletions(-) diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index 420f3ac31..844d10da9 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -5,7 +5,6 @@ #include #include -#include "absl/base/nullability.h" #include "absl/log/absl_check.h" #include "absl/log/absl_log.h" #include "absl/status/status.h" @@ -32,7 +31,6 @@ namespace { using ::cel::BoolValue; using ::cel::ErrorValue; using ::cel::MapValue; -using ::cel::NullValue; using ::cel::OptionalValue; using ::cel::ProtoWrapperTypeOptions; using ::cel::StringValue; @@ -75,35 +73,97 @@ absl::optional CheckForMarkedAttributes(const AttributeTrail& trail, return absl::nullopt; } -void TestOnlySelect(const StructValue& msg, const std::string& field, - const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, - google::protobuf::MessageFactory* absl_nonnull message_factory, - google::protobuf::Arena* absl_nonnull arena, - Value* absl_nonnull result) { - absl::StatusOr has_field = msg.HasFieldByName(field); +absl::Status PerformHas(const Value& target, absl::string_view field, + const StringValue& field_value, + const google::protobuf::DescriptorPool* descriptor_pool, + google::protobuf::MessageFactory* message_factory, + google::protobuf::Arena* arena, Value& result) { + switch (target.kind()) { + case ValueKind::kMap: { + CEL_RETURN_IF_ERROR(target.GetMap().Has(field_value, descriptor_pool, + message_factory, arena, &result)); + return absl::OkStatus(); + } + case ValueKind::kStruct: { + auto has_field = target.GetStruct().HasFieldByName(field); + if (!has_field.ok()) { + result = ErrorValue(std::move(has_field).status()); + } else { + result = BoolValue{*has_field}; + } + return absl::OkStatus(); + } + default: + return InvalidSelectTargetError(); + } +} - if (!has_field.ok()) { - *result = ErrorValue(std::move(has_field).status()); - return; +absl::Status PerformGet(const Value& target, absl::string_view field, + const StringValue& field_value, + ProtoWrapperTypeOptions unboxing_option, + const google::protobuf::DescriptorPool* descriptor_pool, + google::protobuf::MessageFactory* message_factory, + google::protobuf::Arena* arena, Value& result) { + switch (target.kind()) { + case ValueKind::kMap: { + auto status = target.GetMap().Get(field_value, descriptor_pool, + message_factory, arena, &result); + if (!status.ok()) { + result = ErrorValue(std::move(status)); + } + return absl::OkStatus(); + } + case ValueKind::kStruct: { + auto status = target.GetStruct().GetFieldByName( + field, unboxing_option, descriptor_pool, message_factory, arena, + &result); + if (!status.ok()) { + result = ErrorValue(std::move(status)); + } + return absl::OkStatus(); + } + default: + return InvalidSelectTargetError(); } - *result = BoolValue{*has_field}; } -void TestOnlySelect(const MapValue& map, const StringValue& field_name, - const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, - google::protobuf::MessageFactory* absl_nonnull message_factory, - google::protobuf::Arena* absl_nonnull arena, - Value* absl_nonnull result) { - // Field presence only supports string keys containing valid identifier - // characters. - absl::Status presence = - map.Has(field_name, descriptor_pool, message_factory, arena, result); - - if (!presence.ok()) { - *result = ErrorValue(std::move(presence)); - return; +absl::Status PerformOptionalGet(const Value& target, absl::string_view field, + const StringValue& field_value, + ProtoWrapperTypeOptions unboxing_option, + const google::protobuf::DescriptorPool* descriptor_pool, + google::protobuf::MessageFactory* message_factory, + google::protobuf::Arena* arena, Value& result) { + switch (target.kind()) { + case ValueKind::kMap: { + CEL_ASSIGN_OR_RETURN( + bool found, target.GetMap().Find(field_value, descriptor_pool, + message_factory, arena, &result)); + if (!found) { + result = OptionalValue::None(); + return absl::OkStatus(); + } + ABSL_DCHECK(!result.IsUnknown()); + result = OptionalValue::Of(std::move(result), arena); + return absl::OkStatus(); + } + case ValueKind::kStruct: { + CEL_ASSIGN_OR_RETURN(bool found, + target.GetStruct().HasFieldByName(field)); + if (!found) { + result = OptionalValue::None(); + return absl::OkStatus(); + } + CEL_RETURN_IF_ERROR(target.GetStruct().GetFieldByName( + field, unboxing_option, descriptor_pool, message_factory, arena, + &result)); + + ABSL_DCHECK(!result.IsUnknown()); + result = OptionalValue::Of(std::move(result), arena); + return absl::OkStatus(); + } + default: + return InvalidSelectTargetError(); } - ABSL_DCHECK(!result->IsUnknown()); } // SelectStep performs message field access specified by Expr::Select @@ -124,11 +184,6 @@ class SelectStep : public ExpressionStepBase { absl::Status Evaluate(ExecutionFrame* frame) const override; private: - absl::Status PerformTestOnlySelect(ExecutionFrame* frame, - const Value& arg) const; - absl::StatusOr PerformSelect(ExecutionFrame* frame, const Value& arg, - Value& result) const; - cel::StringValue field_value_; std::string field_; bool test_field_presence_; @@ -177,128 +232,48 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { return absl::OkStatus(); } - // Handle test only Select. + Value result; if (test_field_presence_) { + const Value* target = &arg; if (optional_arg) { if (!optional_arg->HasValue()) { - frame->value_stack().PopAndPush(cel::BoolValue{false}); + frame->value_stack().PopAndPush(cel::BoolValue{false}, + std::move(result_trail)); return absl::OkStatus(); } - Value value; - optional_arg->Value(&value); - return PerformTestOnlySelect(frame, value); + optional_arg->Value(&result); + target = &result; } - return PerformTestOnlySelect(frame, arg); + CEL_RETURN_IF_ERROR( + PerformHas(*target, field_, field_value_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), result)); + frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); + return absl::OkStatus(); } - // Normal select path. - // Select steps can be applied to either maps or messages if (optional_arg) { if (!optional_arg->HasValue()) { - // Leave optional_arg at the top of the stack. Its empty. + frame->value_stack().PopAndPush(OptionalValue::None(), + std::move(result_trail)); return absl::OkStatus(); } Value value; - Value result; - bool ok; optional_arg->Value(&value); - CEL_ASSIGN_OR_RETURN(ok, PerformSelect(frame, value, result)); - if (!ok) { - frame->value_stack().PopAndPush(cel::OptionalValue::None(), - std::move(result_trail)); - return absl::OkStatus(); + auto status = PerformOptionalGet( + value, field_, field_value_, unboxing_option_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), result); + if (!status.ok()) { + result = ErrorValue(std::move(status)); } - frame->value_stack().PopAndPush( - cel::OptionalValue::Of(std::move(result), frame->arena()), - std::move(result_trail)); + frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); return absl::OkStatus(); } - // Normal select path. - // Select steps can be applied to either maps or messages - switch (arg.kind()) { - case ValueKind::kStruct: { - Value result; - auto status = arg.GetStruct().GetFieldByName( - field_, unboxing_option_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result); - if (!status.ok()) { - result = ErrorValue(std::move(status)); - } - frame->value_stack().PopAndPush(std::move(result), - std::move(result_trail)); - return absl::OkStatus(); - } - case ValueKind::kMap: { - Value result; - auto status = - arg.GetMap().Get(field_value_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result); - if (!status.ok()) { - result = ErrorValue(std::move(status)); - } - frame->value_stack().PopAndPush(std::move(result), - std::move(result_trail)); - return absl::OkStatus(); - } - default: - // Control flow should have returned earlier. - return InvalidSelectTargetError(); - } -} - -absl::Status SelectStep::PerformTestOnlySelect(ExecutionFrame* frame, - const Value& arg) const { - switch (arg.kind()) { - case ValueKind::kMap: { - Value result; - TestOnlySelect(arg.GetMap(), field_value_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result); - frame->value_stack().PopAndPush(std::move(result)); - return absl::OkStatus(); - } - case ValueKind::kMessage: { - Value result; - TestOnlySelect(arg.GetStruct(), field_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result); - frame->value_stack().PopAndPush(std::move(result)); - return absl::OkStatus(); - } - default: - // Control flow should have returned earlier. - return InvalidSelectTargetError(); - } -} - -absl::StatusOr SelectStep::PerformSelect(ExecutionFrame* frame, - const Value& arg, - Value& result) const { - switch (arg->kind()) { - case ValueKind::kStruct: { - const auto& struct_value = arg.GetStruct(); - CEL_ASSIGN_OR_RETURN(auto ok, struct_value.HasFieldByName(field_)); - if (!ok) { - result = NullValue{}; - return false; - } - CEL_RETURN_IF_ERROR(struct_value.GetFieldByName( - field_, unboxing_option_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result)); - ABSL_DCHECK(!result.IsUnknown()); - return true; - } - case ValueKind::kMap: { - CEL_ASSIGN_OR_RETURN( - auto found, - arg.GetMap().Find(field_value_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result)); - ABSL_DCHECK(!found || !result.IsUnknown()); - return found; - } - default: - // Control flow should have returned earlier. - return InvalidSelectTargetError(); - } + CEL_RETURN_IF_ERROR(PerformGet( + arg, field_, field_value_, unboxing_option_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), result)); + frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); + return absl::OkStatus(); } class DirectSelectStep : public DirectExpressionStep { @@ -362,11 +337,11 @@ class DirectSelectStep : public DirectExpressionStep { } Value value; optional_arg->Value(&value); - PerformTestOnlySelect(frame, value, result); - return absl::OkStatus(); + return PerformHas(value, field_, field_value_, frame.descriptor_pool(), + frame.message_factory(), frame.arena(), result); } - PerformTestOnlySelect(frame, result, result); - return absl::OkStatus(); + return PerformHas(result, field_, field_value_, frame.descriptor_pool(), + frame.message_factory(), frame.arena(), result); } if (optional_arg) { @@ -376,26 +351,24 @@ class DirectSelectStep : public DirectExpressionStep { } Value value; optional_arg->Value(&value); - return PerformOptionalSelect(frame, value, result); + auto status = + PerformOptionalGet(value, field_, field_value_, unboxing_option_, + frame.descriptor_pool(), frame.message_factory(), + frame.arena(), result); + if (!status.ok()) { + result = ErrorValue(std::move(status)); + } + return absl::OkStatus(); } - auto status = PerformSelect(frame, result, result); - if (!status.ok()) { - result = ErrorValue(std::move(status)); - } - return absl::OkStatus(); + return PerformGet(result, field_, field_value_, unboxing_option_, + frame.descriptor_pool(), frame.message_factory(), + frame.arena(), result); } private: std::unique_ptr operand_; - void PerformTestOnlySelect(ExecutionFrameBase& frame, const Value& value, - Value& result) const; - absl::Status PerformOptionalSelect(ExecutionFrameBase& frame, - const Value& value, Value& result) const; - absl::Status PerformSelect(ExecutionFrameBase& frame, const Value& value, - Value& result) const; - // Field name in formats supported by each of the map and struct field access // APIs. // @@ -410,84 +383,6 @@ class DirectSelectStep : public DirectExpressionStep { bool enable_optional_types_; }; -void DirectSelectStep::PerformTestOnlySelect(ExecutionFrameBase& frame, - const cel::Value& value, - Value& result) const { - switch (value.kind()) { - case ValueKind::kMap: - TestOnlySelect(value.GetMap(), field_value_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result); - return; - case ValueKind::kMessage: - TestOnlySelect(value.GetStruct(), field_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result); - return; - default: - // Control flow should have returned earlier. - result = cel::ErrorValue(InvalidSelectTargetError()); - return; - } -} - -absl::Status DirectSelectStep::PerformOptionalSelect(ExecutionFrameBase& frame, - const Value& value, - Value& result) const { - switch (value.kind()) { - case ValueKind::kStruct: { - auto struct_value = value.GetStruct(); - CEL_ASSIGN_OR_RETURN(auto ok, struct_value.HasFieldByName(field_)); - if (!ok) { - result = OptionalValue::None(); - return absl::OkStatus(); - } - CEL_RETURN_IF_ERROR(struct_value.GetFieldByName( - field_, unboxing_option_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result)); - ABSL_DCHECK(!result.IsUnknown()); - result = OptionalValue::Of(std::move(result), frame.arena()); - return absl::OkStatus(); - } - case ValueKind::kMap: { - CEL_ASSIGN_OR_RETURN( - auto found, - value.GetMap().Find(field_value_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result)); - if (!found) { - result = OptionalValue::None(); - return absl::OkStatus(); - } - ABSL_DCHECK(!result.IsUnknown()); - result = OptionalValue::Of(std::move(result), frame.arena()); - return absl::OkStatus(); - } - default: - // Control flow should have returned earlier. - return InvalidSelectTargetError(); - } -} - -absl::Status DirectSelectStep::PerformSelect(ExecutionFrameBase& frame, - const cel::Value& value, - Value& result) const { - switch (value.kind()) { - case ValueKind::kStruct: - CEL_RETURN_IF_ERROR(value.GetStruct().GetFieldByName( - field_, unboxing_option_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result)); - ABSL_DCHECK(!result.IsUnknown()); - return absl::OkStatus(); - case ValueKind::kMap: - CEL_RETURN_IF_ERROR( - value.GetMap().Get(field_value_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result)); - ABSL_DCHECK(!result.IsUnknown()); - return absl::OkStatus(); - default: - // Control flow should have returned earlier. - return InvalidSelectTargetError(); - } -} - } // namespace std::unique_ptr CreateDirectSelectStep(