diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index dbd8d9c8b..8df9912bf 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -36,6 +36,7 @@ cc_library( "//common:value", "//eval/eval:direct_expression_step", "//eval/eval:evaluator_core", + "//eval/eval:expression_step_logic", "//eval/eval:trace_step", "//internal:casts", "//runtime:runtime_options", @@ -111,7 +112,6 @@ cc_library( "//common:type", "//common:type_spec_resolver", "//common:value", - "//eval/eval:comprehension_step", "//eval/eval:const_value_step", "//eval/eval:container_access_step", "//eval/eval:create_list_step", @@ -120,11 +120,9 @@ cc_library( "//eval/eval:direct_expression_step", "//eval/eval:equality_steps", "//eval/eval:evaluator_core", + "//eval/eval:expression_step_logic", "//eval/eval:function_step", "//eval/eval:ident_step", - "//eval/eval:jump_step", - "//eval/eval:lazy_init_step", - "//eval/eval:logic_step", "//eval/eval:optional_or_step", "//eval/eval:select_step", "//eval/eval:shadowable_value_step", @@ -139,6 +137,7 @@ cc_library( "//runtime/internal:issue_collector", "//runtime/internal:runtime_env", "@com_google_absl//absl/algorithm:container", + "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/container:flat_hash_set", @@ -360,7 +359,6 @@ cc_test( "//base:ast", "//common:expr", "//common:value", - "//eval/eval:const_value_step", "//eval/eval:create_list_step", "//eval/eval:create_map_step", "//eval/eval:evaluator_core", diff --git a/eval/compiler/constant_folding.cc b/eval/compiler/constant_folding.cc index 93cb35e65..071f16186 100644 --- a/eval/compiler/constant_folding.cc +++ b/eval/compiler/constant_folding.cc @@ -50,7 +50,6 @@ using ::cel::builtin::kOr; using ::cel::builtin::kTernary; using ::cel::runtime_internal::ConvertConstant; using ::google::api::expr::runtime::CreateConstValueDirectStep; -using ::google::api::expr::runtime::CreateConstValueStep; using ::google::api::expr::runtime::EvaluationListener; using ::google::api::expr::runtime::ExecutionFrame; using ::google::api::expr::runtime::ExecutionPath; @@ -243,8 +242,7 @@ absl::Status ConstantFoldingExtension::OnPostVisit(PlannerContext& context, // Otherwise make a stack machine plan. ExecutionPath new_plan; - new_plan.push_back(ExpressionStep::MakeGenericStep( - CreateConstValueStep(std::move(value)), node.id())); + new_plan.push_back(ExpressionStep::MakeConstant(value, node.id())); return context.ReplaceSubplan(node, std::move(new_plan)); } diff --git a/eval/compiler/constant_folding_test.cc b/eval/compiler/constant_folding_test.cc index 1528f5a5d..8c6fcf54c 100644 --- a/eval/compiler/constant_folding_test.cc +++ b/eval/compiler/constant_folding_test.cc @@ -29,7 +29,6 @@ #include "common/value.h" #include "eval/compiler/flat_expr_builder_extensions.h" #include "eval/compiler/resolver.h" -#include "eval/eval/const_value_step.h" #include "eval/eval/create_list_step.h" #include "eval/eval/create_map_step.h" #include "eval/eval/evaluator_core.h" @@ -58,7 +57,6 @@ using ::cel::runtime_internal::IssueCollector; using ::cel::runtime_internal::NewTestingRuntimeEnv; using ::cel::expr::ParsedExpr; using ::google::api::expr::parser::Parse; -using ::google::api::expr::runtime::CreateConstValueStep; using ::google::api::expr::runtime::CreateCreateListStep; using ::google::api::expr::runtime::CreateCreateStructStepForMap; using ::google::api::expr::runtime::ExecutionPath; @@ -116,25 +114,25 @@ TEST_F(UpdatedConstantFoldingTest, SkipsTernary) { program_builder.EnterSubexpression(&call); // condition program_builder.EnterSubexpression(&condition); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::BoolValue(true)), condition.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), condition.id())); program_builder.ExitSubexpression(&condition); // true program_builder.EnterSubexpression(&true_branch); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::BoolValue(true)), true_branch.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), true_branch.id())); program_builder.ExitSubexpression(&true_branch); // false program_builder.EnterSubexpression(&false_branch); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::BoolValue(true)), false_branch.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), false_branch.id())); program_builder.ExitSubexpression(&false_branch); // ternary. - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::NullValue()), call.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::NullValue(), call.id())); program_builder.ExitSubexpression(&call); std::shared_ptr arena; @@ -179,20 +177,20 @@ TEST_F(UpdatedConstantFoldingTest, SkipsOr) { // left program_builder.EnterSubexpression(&left_condition); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::BoolValue(false)), left_condition.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(false), left_condition.id())); program_builder.ExitSubexpression(&left_condition); // right program_builder.EnterSubexpression(&right_condition); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::BoolValue(true)), right_condition.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), right_condition.id())); program_builder.ExitSubexpression(&right_condition); // op // Just a placeholder. - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::NullValue()), call.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::NullValue(), call.id())); program_builder.ExitSubexpression(&call); std::shared_ptr arena; @@ -234,20 +232,20 @@ TEST_F(UpdatedConstantFoldingTest, SkipsAnd) { // left program_builder.EnterSubexpression(&left_condition); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::BoolValue(true)), left_condition.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), left_condition.id())); program_builder.ExitSubexpression(&left_condition); // right program_builder.EnterSubexpression(&right_condition); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::BoolValue(false)), right_condition.id())); + program_builder.AddStep(ExpressionStep::MakeConstant(cel::BoolValue(false), + right_condition.id())); program_builder.ExitSubexpression(&right_condition); // op // Just a placeholder. - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::NullValue()), call.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::NullValue(), call.id())); program_builder.ExitSubexpression(&call); std::shared_ptr arena; @@ -289,14 +287,14 @@ TEST_F(UpdatedConstantFoldingTest, CreatesList) { // elem one program_builder.EnterSubexpression(&elem_one); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::IntValue(1L)), elem_one.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(1L), elem_one.id())); program_builder.ExitSubexpression(&elem_one); // elem two program_builder.EnterSubexpression(&elem_two); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::IntValue(2L)), elem_two.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(2L), elem_two.id())); program_builder.ExitSubexpression(&elem_two); // createlist @@ -349,32 +347,32 @@ TEST_F(UpdatedConstantFoldingTest, CreatesLargeList) { // 0 ASSERT_TRUE(program_builder.EnterSubexpression(&elem0) != nullptr); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::IntValue(1L)), elem0.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(1L), elem0.id())); program_builder.ExitSubexpression(&elem0); // 1 ASSERT_TRUE(program_builder.EnterSubexpression(&elem1)); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::IntValue(2L)), elem1.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(2L), elem1.id())); program_builder.ExitSubexpression(&elem1); // 2 ASSERT_TRUE(program_builder.EnterSubexpression(&elem2) != nullptr); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::IntValue(3L)), elem2.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(3L), elem2.id())); program_builder.ExitSubexpression(&elem2); // 3 ASSERT_TRUE(program_builder.EnterSubexpression(&elem3) != nullptr); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::IntValue(4L)), elem3.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(4L), elem3.id())); program_builder.ExitSubexpression(&elem3); // 4 ASSERT_TRUE(program_builder.EnterSubexpression(&elem4) != nullptr); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::IntValue(5L)), elem4.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(5L), elem4.id())); program_builder.ExitSubexpression(&elem4); // createlist @@ -428,14 +426,14 @@ TEST_F(UpdatedConstantFoldingTest, CreatesMap) { // key program_builder.EnterSubexpression(&key); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::IntValue(1L)), key.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(1L), key.id())); program_builder.ExitSubexpression(&key); // value program_builder.EnterSubexpression(&value); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::IntValue(2L)), value.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(2L), value.id())); program_builder.ExitSubexpression(&value); // create map @@ -484,14 +482,14 @@ TEST_F(UpdatedConstantFoldingTest, CreatesInvalidMap) { // key program_builder.EnterSubexpression(&key); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::DoubleValue(1.0)), key.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::DoubleValue(1.0), key.id())); program_builder.ExitSubexpression(&key); // value program_builder.EnterSubexpression(&value); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::IntValue(2L)), value.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(2L), value.id())); program_builder.ExitSubexpression(&value); // create map @@ -539,20 +537,20 @@ TEST_F(UpdatedConstantFoldingTest, ErrorsOnUnexpectedOrder) { program_builder.EnterSubexpression(&call); // left program_builder.EnterSubexpression(&left_condition); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::BoolValue(true)), left_condition.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), left_condition.id())); program_builder.ExitSubexpression(&left_condition); // right program_builder.EnterSubexpression(&right_condition); - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::BoolValue(false)), right_condition.id())); + program_builder.AddStep(ExpressionStep::MakeConstant(cel::BoolValue(false), + right_condition.id())); program_builder.ExitSubexpression(&right_condition); // op // Just a placeholder. - program_builder.AddStep(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::NullValue()), call.id())); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::NullValue(), call.id())); program_builder.ExitSubexpression(&call); std::shared_ptr arena; diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index 677918c7c..7b9f6dabf 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -31,6 +31,7 @@ #include #include "absl/algorithm/container.h" +#include "absl/base/nullability.h" #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" #include "absl/container/node_hash_map.h" @@ -71,9 +72,9 @@ #include "eval/eval/direct_expression_step.h" #include "eval/eval/equality_steps.h" #include "eval/eval/evaluator_core.h" +#include "eval/eval/expression_step_logic.h" #include "eval/eval/function_step.h" #include "eval/eval/ident_step.h" -#include "eval/eval/jump_step.h" #include "eval/eval/lazy_init_step.h" #include "eval/eval/logic_step.h" #include "eval/eval/optional_or_step.h" @@ -155,13 +156,6 @@ struct ProgramStepIndex { // A convenience wrapper for offset-calculating logic. class Jump { public: - // Default constructor for empty jump. - // - // Users must check that jump is non-empty before calling member functions. - explicit Jump() : self_index_{-1, nullptr}, jump_step_(nullptr) {} - Jump(ProgramStepIndex self_index, JumpStepBase* jump_step) - : self_index_(self_index), jump_step_(jump_step) {} - static absl::StatusOr CalculateOffset(ProgramStepIndex base, ProgramStepIndex target) { if (target.subexpression != base.subexpression) { @@ -171,21 +165,19 @@ class Jump { } int offset = base.subexpression->CalculateOffset(base.index, target.index); - return offset; - } - absl::Status set_target(ProgramStepIndex target) { - CEL_ASSIGN_OR_RETURN(int offset, CalculateOffset(self_index_, target)); + // The offset may be packed into a 31-bit int field. Technically, the offset + // could be larger for a well-formed expression with more than 2**30 steps, + // but such a case is unlikely to exist at this time. + constexpr int kMaxJumpOffset = (1 << 30) - 1; + constexpr int kMinJumpOffset = -(1 << 30); - jump_step_->set_jump_offset(offset); - return absl::OkStatus(); + if (offset < kMinJumpOffset || offset > kMaxJumpOffset) { + return absl::InternalError("Jump offset exceeds supported range: " + + absl::StrCat(offset)); + } + return offset; } - - bool exists() { return jump_step_ != nullptr; } - - private: - ProgramStepIndex self_index_; - JumpStepBase* jump_step_; }; class CondVisitor { @@ -223,7 +215,7 @@ class LogicalCondVisitor : public CondVisitor { private: FlatExprVisitor* visitor_; const bool is_or_; - std::vector jump_steps_; + std::vector jump_steps_; bool short_circuiting_; }; @@ -245,7 +237,8 @@ class OptionalOrCondVisitor : public CondVisitor { private: FlatExprVisitor* visitor_; const bool is_or_value_; - std::vector jump_steps_; + OptionalHasValueJumpStep* jump_step_ = nullptr; + ProgramStepIndex jump_step_pos_{-1, nullptr}; bool short_circuiting_; }; @@ -259,9 +252,8 @@ class TernaryCondVisitor : public CondVisitor { private: FlatExprVisitor* visitor_; - Jump jump_to_second_; - Jump error_jump_; - Jump jump_after_first_; + std::optional cond_jump_pos_; + std::optional jump_after_first_pos_; }; class ExhaustiveTernaryCondVisitor : public CondVisitor { @@ -427,13 +419,11 @@ bool IsBlock(const cel::CallExpr* call) { return call->function() == kBlock; } // Visitor for Comprehension expressions. class ComprehensionVisitor { public: - explicit ComprehensionVisitor(FlatExprVisitor* visitor, bool short_circuiting, - bool is_trivial, size_t iter_slot, - size_t iter2_slot, size_t accu_slot) + explicit ComprehensionVisitor(FlatExprVisitor* visitor, bool is_trivial, + size_t iter_slot, size_t iter2_slot, + size_t accu_slot) : visitor_(visitor), - next_step_(nullptr), - cond_step_(nullptr), - short_circuiting_(short_circuiting), + init_step_(nullptr), is_trivial_(is_trivial), accu_init_extracted_(false), iter_slot_(iter_slot), @@ -461,14 +451,14 @@ class ComprehensionVisitor { absl::Status PostVisitArgDefault(cel::ComprehensionArg arg_num, const cel::Expr* comprehension_expr); + ComprehensionCondStep* absl_nullable GetCondStep(); + ComprehensionNextStep* absl_nullable GetNextStep(); + FlatExprVisitor* visitor_; ComprehensionInitStep* init_step_; - ComprehensionNextStep* next_step_; - ComprehensionCondStep* cond_step_; ProgramStepIndex init_step_pos_; - ProgramStepIndex next_step_pos_; - ProgramStepIndex cond_step_pos_; - bool short_circuiting_; + std::optional next_step_pos_; + std::optional cond_step_pos_; bool is_trivial_; bool accu_init_extracted_; size_t iter_slot_; @@ -730,8 +720,8 @@ class FlatExprVisitor : public cel::AstVisitor { return; } - AddStep(CreateConstValueStep(std::move(converted_value).value()), - expr.id()); + AddStep(ExpressionStep::MakeConstant(std::move(converted_value).value(), + expr.id())); } struct SlotLookupResult { @@ -855,8 +845,8 @@ class FlatExprVisitor : public cel::AstVisitor { program.depth + 1); } else { // Off by one since mainline expression will be index 0. - AddStep(CreateLazyInitStep(slot.slot, slot.subexpression + 1), - expr.id()); + AddStep(ExpressionStep::MakeLazyInitStep( + slot.slot, slot.subexpression + 1, expr.id())); } return; } else if (slot.slot >= 0) { @@ -865,8 +855,7 @@ class FlatExprVisitor : public cel::AstVisitor { CreateDirectSlotIdentStep(ident_expr.name(), slot.slot, expr.id()), 1); } else { - AddStep(CreateIdentStepForSlot(ident_expr.name(), slot.slot), - expr.id()); + AddStep(ExpressionStep::MakeReadSlotStep(slot.slot, expr.id())); } return; } @@ -1439,9 +1428,8 @@ class FlatExprVisitor : public cel::AstVisitor { /*.iter_var2_in_scope=*/false, /*.accu_var_in_scope=*/false, /*.in_accu_init=*/false, - std::make_unique(this, options_.short_circuiting, - is_bind, iter_slot, iter2_slot, - accu_slot)}); + std::make_unique(this, is_bind, iter_slot, + iter2_slot, accu_slot)}); comprehension_stack_.back().visitor->PreVisit(&expr); } @@ -1580,11 +1568,12 @@ class FlatExprVisitor : public cel::AstVisitor { SetRecursiveStep(CreateDirectMutableListStep(expr.id()), 1); return; } - AddStep(CreateMutableListStep(), expr.id()); + AddStep(ExpressionStep::MakeNewMutableListStep(expr.id())); return; } if (GetOptimizableListAppendOperand(comprehension.comprehension) == &expr) { + // Avoid wrapping the appended element in a create list step. return; } } @@ -1711,9 +1700,10 @@ class FlatExprVisitor : public cel::AstVisitor { *depth + 1); return; } - AddStep( - CreateFunctionStep(*call_expr, expr->id(), std::move(lazy_overloads)), - expr->id()); + AddStep(ExpressionStep::MakeLazyFunctionStep( + CreateLazyFunctionStep(*call_expr, expr->id(), + std::move(lazy_overloads)), + expr->id())); return; } @@ -1747,8 +1737,9 @@ class FlatExprVisitor : public cel::AstVisitor { *recursion_depth + 1); return; } - AddStep(CreateFunctionStep(*call_expr, expr->id(), std::move(overloads)), - expr->id()); + AddStep(ExpressionStep::MakeEagerFunctionStep( + CreateFunctionStep(*call_expr, expr->id(), std::move(overloads)), + expr->id())); } // Add a step to the program, taking ownership. If successful, returns the @@ -2067,7 +2058,7 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleNot( *depth + 1); return CallHandlerResult::kIntercepted; } - AddStep(CreateNotStep(), expr.id()); + AddStep(ExpressionStep::MakeBooleanNotStep(expr.id())); return CallHandlerResult::kIntercepted; } @@ -2092,7 +2083,7 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleNotStrictlyFalse( *depth + 1); return CallHandlerResult::kIntercepted; } - AddStep(CreateNotStrictlyFalseStep(), expr.id()); + AddStep(ExpressionStep::MakeNotStrictlyFalseStep(expr.id())); return CallHandlerResult::kIntercepted; } @@ -2134,7 +2125,8 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleBlock( // Otherwise, iterative plan. if (block.slot_count > 0) { - AddStep(CreateClearSlotsStep(block.index, block.slot_count), expr.id()); + AddStep(ExpressionStep::MakeClearSlotsStep(block.index, block.slot_count, + expr.id())); } return CallHandlerResult::kIntercepted; @@ -2153,21 +2145,25 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleListAppend( const cel::ComprehensionExpr* comprehension = comprehension_stack_.back().comprehension; const cel::Expr& loop_step = comprehension->loop_step(); + // Macro loop_step for a map() will contain a list concat operation: // accu_var + [elem] - if (&loop_step == &expr) { - AddResolvedFunctionStep(&call_expr, &expr, - cel::builtin::kRuntimeListAppend); - return CallHandlerResult::kIntercepted; - } + const bool is_map_loop_step = &loop_step == &expr; + // Macro loop_step for a filter() will contain a ternary: // filter ? accu_var + [elem] : accu_var - if (loop_step.has_call_expr() && + const bool is_filter_loop_step = + loop_step.has_call_expr() && loop_step.call_expr().function() == cel::builtin::kTernary && loop_step.call_expr().args().size() == 3 && - &(loop_step.call_expr().args()[1]) == &expr) { - AddResolvedFunctionStep(&call_expr, &expr, - cel::builtin::kRuntimeListAppend); + &(loop_step.call_expr().args()[1]) == &expr; + if (is_map_loop_step || is_filter_loop_step) { + if (RecursionEligible().has_value()) { + AddResolvedFunctionStep(&call_expr, &expr, + cel::builtin::kRuntimeListAppend); + } else { + AddStep(ExpressionStep::MakeMutableListAppendStep(expr.id())); + } return CallHandlerResult::kIntercepted; } } @@ -2196,7 +2192,8 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleHeterogeneousEquality( *depth + 1); return CallHandlerResult::kIntercepted; } - AddStep(CreateEqualityStep(inequality), expr.id()); + AddStep(inequality ? ExpressionStep::MakeFastNotEqualStep(expr.id()) + : ExpressionStep::MakeFastEqualStep(expr.id())); return CallHandlerResult::kIntercepted; } @@ -2221,7 +2218,7 @@ FlatExprVisitor::HandleHeterogeneousEqualityIn(const cel::Expr& expr, return CallHandlerResult::kIntercepted; } - AddStep(CreateInStep(), expr.id()); + AddStep(ExpressionStep::MakeFastInStep(expr.id())); return CallHandlerResult::kIntercepted; } @@ -2256,28 +2253,37 @@ void LogicalCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { const size_t num_args = expr->call_expr().args().size(); if (arg_num == last_arg_index) { if (is_or_) { - visitor_->AddStep(CreateOrStep(num_args), expr->id()); + visitor_->AddStep( + ExpressionStep::MakeBooleanOrStep(num_args, expr->id())); } else { - visitor_->AddStep(CreateAndStep(num_args), expr->id()); + visitor_->AddStep( + ExpressionStep::MakeBooleanAndStep(num_args, expr->id())); } if (short_circuiting_ && !jump_steps_.empty()) { - for (auto& jump : jump_steps_) { - visitor_->SetProgressStatusIfError( - jump.set_target(visitor_->GetCurrentIndex())); + ProgramStepIndex target = visitor_->GetCurrentIndex(); + for (const ProgramStepIndex& jump_pos : jump_steps_) { + absl::StatusOr offset = Jump::CalculateOffset(jump_pos, target); + if (!offset.ok()) { + visitor_->SetProgressStatusIfError(offset.status()); + continue; + } + ExpressionStep* step = + jump_pos.subexpression->GetIfExpressionStep(jump_pos.index); + BoolJumpStepInfo* jump_info = GetIfBoolJumpStep(step); + if (jump_info != nullptr) { + jump_info->set = true; + jump_info->offset = *offset; + } } } } if (short_circuiting_ && arg_num < last_arg_index) { - std::unique_ptr jump_step = - is_or_ - ? CreateCondJumpStep(true, {}, /*expected_stack_size=*/arg_num + 1) - : CreateCondJumpStep(false, {}, - /*expected_stack_size=*/arg_num + 1); ProgramStepIndex index = visitor_->GetCurrentIndex(); - if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step)); - jump_step_ptr) { - jump_steps_.push_back(Jump(index, jump_step_ptr)); - } + visitor_->AddStep(is_or_ ? ExpressionStep::MakeBooleanOrJumpStep( + /*arg_count=*/arg_num + 1) + : ExpressionStep::MakeBooleanAndJumpStep( + /*arg_count=*/arg_num + 1)); + jump_steps_.push_back(index); } } @@ -2297,20 +2303,14 @@ void OptionalOrCondVisitor::PostVisitTarget(const cel::Expr* expr) { if (visitor_->PlanRecursiveProgram()) { return; } - if (short_circuiting_) { - // If first branch evaluation result is enough to determine output, - // jump over the second branch and provide result of the first argument as - // final output. - // Retain a pointer to the jump step so we can update the target after - // planning the second argument. - std::unique_ptr jump_step = - CreateOptionalHasValueJumpStep(is_or_value_); - ProgramStepIndex index = visitor_->GetCurrentIndex(); - if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step)); - jump_step_ptr) { - jump_steps_.push_back(Jump(index, jump_step_ptr)); - } + if (!short_circuiting_) { + return; } + + // Keep a pointer to the jump step so we can update the target after + // planning the second argument. + jump_step_pos_ = visitor_->GetCurrentIndex(); + jump_step_ = visitor_->AddStep(CreateOptionalHasValueJumpStep(is_or_value_)); } void OptionalOrCondVisitor::PostVisit(const cel::Expr* expr) { @@ -2320,12 +2320,20 @@ void OptionalOrCondVisitor::PostVisit(const cel::Expr* expr) { } visitor_->AddStep(CreateOptionalOrStep(is_or_value_), expr->id()); - if (short_circuiting_) { - for (auto& jump : jump_steps_) { - visitor_->SetProgressStatusIfError( - jump.set_target(visitor_->GetCurrentIndex())); - } + if (!short_circuiting_) { + return; + } + + ABSL_DCHECK(jump_step_ != nullptr) + << "OptionalOrCondVisitor::PostVisit: jump_step_ is null"; + absl::StatusOr offset = + Jump::CalculateOffset(jump_step_pos_, visitor_->GetCurrentIndex()); + if (!offset.ok()) { + visitor_->SetProgressStatusIfError(offset.status()); + return; } + + jump_step_->set_jump_offset(*offset); } void TernaryCondVisitor::PreVisit(const cel::Expr* expr) { @@ -2351,37 +2359,35 @@ void TernaryCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { // condition argument for ternary operator if (arg_num == 0) { - // Jump in case of error or non-bool - ProgramStepIndex error_jump_pos = visitor_->GetCurrentIndex(); - auto* error_jump = visitor_->AddStep(CreateBoolCheckJumpStep()); - if (error_jump) { - error_jump_ = Jump(error_jump_pos, error_jump); - } - - // Jump to the second branch of execution - // Value is to be removed from the stack. - ProgramStepIndex cond_jump_pos = visitor_->GetCurrentIndex(); - auto* jump_to_second = visitor_->AddStep(CreateTernaryCondJumpStep()); - if (jump_to_second) { - jump_to_second_ = - Jump(cond_jump_pos, static_cast(jump_to_second)); - } + // Jump in case of error or non-bool, or jump to the second branch of + // execution if false. + cond_jump_pos_ = visitor_->GetCurrentIndex(); + visitor_->AddStep(ExpressionStep::MakeTernaryJumpStep()); } else if (arg_num == 1) { // Jump after the first and over the second branch of execution. - // Value is to be removed from the stack. - ProgramStepIndex jump_pos = visitor_->GetCurrentIndex(); - auto* jump_after_first = visitor_->AddStep(CreateJumpStep()); - if (!jump_after_first) { + jump_after_first_pos_ = visitor_->GetCurrentIndex(); + visitor_->AddStep(ExpressionStep::MakeFixedJumpStep()); + + ExpressionStep* cond_step = + cond_jump_pos_.has_value() + ? cond_jump_pos_->subexpression->GetIfExpressionStep( + cond_jump_pos_->index) + : nullptr; + TernaryJumpStepInfo* ternary_info = GetIfTernaryJumpStep(cond_step); + if (!visitor_->ValidateOrError( + ternary_info != nullptr, + "Error configuring ternary operator: jump_to_second_ is null")) { return; } - jump_after_first_ = Jump(jump_pos, jump_after_first); - if (visitor_->ValidateOrError( - jump_to_second_.exists(), - "Error configuring ternary operator: jump_to_second_ is null")) { - visitor_->SetProgressStatusIfError( - jump_to_second_.set_target(visitor_->GetCurrentIndex())); + absl::StatusOr offset = + Jump::CalculateOffset(*cond_jump_pos_, visitor_->GetCurrentIndex()); + if (!offset.ok()) { + visitor_->SetProgressStatusIfError(offset.status()); + return; } + + ternary_info->jump_to_second_offset = *offset; } // Code executed after traversing the final branch of execution // (arg_num == 2) is placed in PostVisitCall, to make this method less @@ -2394,17 +2400,41 @@ void TernaryCondVisitor::PostVisit(const cel::Expr* expr) { return; } // Determine and set jump offset in jump instruction. + ExpressionStep* cond_step = + cond_jump_pos_.has_value() + ? cond_jump_pos_->subexpression->GetIfExpressionStep( + cond_jump_pos_->index) + : nullptr; + TernaryJumpStepInfo* ternary_info = GetIfTernaryJumpStep(cond_step); if (visitor_->ValidateOrError( - error_jump_.exists(), + ternary_info != nullptr, "Error configuring ternary operator: error_jump_ is null")) { - visitor_->SetProgressStatusIfError( - error_jump_.set_target(visitor_->GetCurrentIndex())); + absl::StatusOr offset = + Jump::CalculateOffset(*cond_jump_pos_, visitor_->GetCurrentIndex()); + if (!offset.ok()) { + visitor_->SetProgressStatusIfError(offset.status()); + } else { + ternary_info->set = true; + ternary_info->error_offset = *offset; + } } + ExpressionStep* jump_after_first_step = + jump_after_first_pos_.has_value() + ? jump_after_first_pos_->subexpression->GetIfExpressionStep( + jump_after_first_pos_->index) + : nullptr; + FixedJumpStepInfo* fixed_info = GetIfFixedJumpStep(jump_after_first_step); if (visitor_->ValidateOrError( - jump_after_first_.exists(), + fixed_info != nullptr, "Error configuring ternary operator: jump_after_first_ is null")) { - visitor_->SetProgressStatusIfError( - jump_after_first_.set_target(visitor_->GetCurrentIndex())); + absl::StatusOr offset = Jump::CalculateOffset( + *jump_after_first_pos_, visitor_->GetCurrentIndex()); + if (!offset.ok()) { + visitor_->SetProgressStatusIfError(offset.status()); + } else { + fixed_info->set = true; + fixed_info->offset = *offset; + } } } @@ -2430,6 +2460,30 @@ void ComprehensionVisitor::PreVisit(const cel::Expr* expr) { } } +ComprehensionCondStep* absl_nullable ComprehensionVisitor::GetCondStep() { + if (!cond_step_pos_) { + return nullptr; + } + ExpressionStep* step = + cond_step_pos_->subexpression->GetIfExpressionStep(cond_step_pos_->index); + if (!step) { + return nullptr; + } + return GetIfComprehensionCondStep(*step); +} + +ComprehensionNextStep* absl_nullable ComprehensionVisitor::GetNextStep() { + if (!next_step_pos_) { + return nullptr; + } + ExpressionStep* step = + next_step_pos_->subexpression->GetIfExpressionStep(next_step_pos_->index); + if (!step) { + return nullptr; + } + return GetIfComprehensionNextStep(*step); +} + absl::Status ComprehensionVisitor::PostVisitArgDefault( cel::ComprehensionArg arg_num, const cel::Expr* expr) { if (visitor_->PlanRecursiveProgram()) { @@ -2438,69 +2492,98 @@ absl::Status ComprehensionVisitor::PostVisitArgDefault( switch (arg_num) { case cel::ITER_RANGE: { init_step_pos_ = visitor_->GetCurrentIndex(); - init_step_ = visitor_->AddStep(std::make_unique()); + if (iter_slot_ != iter2_slot_) { + init_step_ = visitor_->AddStep(std::make_unique( + iter_slot_, iter2_slot_, accu_slot_)); + } else { + init_step_ = visitor_->AddStep( + std::make_unique(iter_slot_, accu_slot_)); + } break; } case cel::ACCU_INIT: { next_step_pos_ = visitor_->GetCurrentIndex(); - next_step_ = visitor_->AddStep(std::make_unique( - iter_slot_, iter2_slot_, accu_slot_)); + if (iter_slot_ != iter2_slot_) { + visitor_->AddStep(ExpressionStep::MakeComprehensionNext2Step()); + } else { + visitor_->AddStep(ExpressionStep::MakeComprehensionNextStep()); + } break; } case cel::LOOP_CONDITION: { cond_step_pos_ = visitor_->GetCurrentIndex(); - cond_step_ = visitor_->AddStep(std::make_unique( - iter_slot_, iter2_slot_, accu_slot_, short_circuiting_)); + if (iter_slot_ != iter2_slot_) { + visitor_->AddStep(ExpressionStep::MakeComprehensionCond2Step()); + } else { + visitor_->AddStep(ExpressionStep::MakeComprehensionCondStep()); + } break; } case cel::LOOP_STEP: { ProgramStepIndex index = visitor_->GetCurrentIndex(); - auto* jump_to_next = visitor_->AddStep(CreateJumpStep()); - if (!jump_to_next) { + ExpressionStep* jump_step = + visitor_->AddStep(ExpressionStep::MakeFixedJumpStep()); + FixedJumpStepInfo* fixed_info = GetIfFixedJumpStep(jump_step); + if (fixed_info == nullptr) { + // either an error occurred earlier or planning is suppressed. break; } - Jump jump_helper(index, jump_to_next); - visitor_->SetProgressStatusIfError( - jump_helper.set_target(next_step_pos_)); + CEL_ASSIGN_OR_RETURN(int offset, + Jump::CalculateOffset(index, *next_step_pos_)); + fixed_info->set = true; + fixed_info->offset = offset; // Set offsets jumping to the result step. - if (cond_step_) { - CEL_ASSIGN_OR_RETURN( - int jump_from_cond, - Jump::CalculateOffset(cond_step_pos_, visitor_->GetCurrentIndex())); - cond_step_->set_jump_offset(jump_from_cond); + if (auto* cond_step = GetCondStep(); cond_step != nullptr) { + CEL_ASSIGN_OR_RETURN(int jump_from_cond, + Jump::CalculateOffset( + *cond_step_pos_, visitor_->GetCurrentIndex())); + cond_step->set_jump_offset(jump_from_cond); + } else { + return absl::InvalidArgumentError("Comprehension is malformed"); } - if (next_step_) { - CEL_ASSIGN_OR_RETURN( - int jump_from_next, - Jump::CalculateOffset(next_step_pos_, visitor_->GetCurrentIndex())); + if (auto* next_step = GetNextStep(); next_step != nullptr) { + CEL_ASSIGN_OR_RETURN(int jump_from_next, + Jump::CalculateOffset( + *next_step_pos_, visitor_->GetCurrentIndex())); - next_step_->set_jump_offset(jump_from_next); + next_step->set_jump_offset(jump_from_next); + } else { + return absl::InvalidArgumentError("Comprehension is malformed"); } break; } case cel::RESULT: { - if (!init_step_ || !next_step_ || !cond_step_) { + if (!init_step_ || !next_step_pos_ || !cond_step_pos_) { // Encountered an error earlier. Can't determine where to jump. break; } - visitor_->AddStep(CreateComprehensionFinishStep(accu_slot_), expr->id()); + visitor_->AddStep( + ExpressionStep::MakeComprehensionFinishStep(accu_slot_, expr->id())); // Set offsets jumping past the result step in case of errors. CEL_ASSIGN_OR_RETURN( int jump_from_init, Jump::CalculateOffset(init_step_pos_, visitor_->GetCurrentIndex())); init_step_->set_error_jump_offset(jump_from_init); - CEL_ASSIGN_OR_RETURN( - int jump_from_next, - Jump::CalculateOffset(next_step_pos_, visitor_->GetCurrentIndex())); - next_step_->set_error_jump_offset(jump_from_next); + if (auto* next_step = GetNextStep(); next_step != nullptr) { + CEL_ASSIGN_OR_RETURN(int jump_from_next, + Jump::CalculateOffset( + *next_step_pos_, visitor_->GetCurrentIndex())); + next_step->set_error_jump_offset(jump_from_next); + } else { + return absl::InvalidArgumentError("Comprehension is malformed"); + } - CEL_ASSIGN_OR_RETURN( - int jump_from_cond, - Jump::CalculateOffset(cond_step_pos_, visitor_->GetCurrentIndex())); - cond_step_->set_error_jump_offset(jump_from_cond); + if (auto* cond_step = GetCondStep(); cond_step != nullptr) { + CEL_ASSIGN_OR_RETURN(int jump_from_cond, + Jump::CalculateOffset( + *cond_step_pos_, visitor_->GetCurrentIndex())); + cond_step->set_error_jump_offset(jump_from_cond); + } else { + return absl::InvalidArgumentError("Comprehension is malformed"); + } break; } } @@ -2518,7 +2601,7 @@ void ComprehensionVisitor::PostVisitArgTrivial(cel::ComprehensionArg arg_num, } case cel::ACCU_INIT: { if (!accu_init_extracted_) { - visitor_->AddStep(CreateAssignSlotAndPopStep(accu_slot_)); + visitor_->AddStep(ExpressionStep::MakeAssignSlotAndPopStep(accu_slot_)); } break; } @@ -2529,7 +2612,8 @@ void ComprehensionVisitor::PostVisitArgTrivial(cel::ComprehensionArg arg_num, break; } case cel::RESULT: { - visitor_->AddStep(CreateClearSlotStep(accu_slot_), expr->id()); + visitor_->AddStep( + ExpressionStep::MakeClearSlotStep(accu_slot_, expr->id())); break; } } @@ -2657,6 +2741,15 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( return visitor.progress_status(); } + if (visitor.slot_count() > std::numeric_limits::max() || + program_builder.ExtractedSubexpressionCount() > + std::numeric_limits::max()) { + // Impractical to trigger (we'd run out of memory first), but assuming this + // allows us to pack references to slots and subexpressions. + return absl::InternalError( + "Expression too large to be executed, exceeds uint32_t limits."); + } + if (issues != nullptr) { (*issues) = issue_collector.ExtractIssues(); } diff --git a/eval/compiler/flat_expr_builder_extensions.h b/eval/compiler/flat_expr_builder_extensions.h index 0112356e4..e85f337f9 100644 --- a/eval/compiler/flat_expr_builder_extensions.h +++ b/eval/compiler/flat_expr_builder_extensions.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include "absl/base/attributes.h" @@ -45,6 +46,7 @@ #include "eval/compiler/resolver.h" #include "eval/eval/direct_expression_step.h" #include "eval/eval/evaluator_core.h" +#include "eval/eval/expression_step_logic.h" #include "eval/eval/trace_step.h" #include "internal/casts.h" #include "runtime/internal/issue_collector.h" @@ -125,6 +127,13 @@ class ProgramBuilder { elements().push_back(expr); } + ExpressionStep* absl_nullable GetIfExpressionStep(size_t index) { + if (index >= elements().size()) { + return nullptr; + } + return std::get_if(&elements()[index]); + } + // Accessor for elements (either simple steps or subexpressions). // // Value is undefined if in the expression has already been flattened. @@ -233,6 +242,11 @@ class ProgramBuilder { // programs table to starting state. std::vector FlattenSubexpressions(); + // Return the current number of subexpressions in the program builder. + size_t ExtractedSubexpressionCount() const { + return extracted_subexpressions_.size(); + } + // Returns the current subexpression where steps and new subexpressions are // added. // @@ -283,9 +297,8 @@ class ProgramBuilder { // Add a program step to the current subexpression. // If successful, returns the step pointer. // - // Note: If successful, the pointer should remain valid until the parent - // expression is finalized. Optimizers may modify the program plan which may - // free the step at that point. + // Note: If successful, the pointer should remain valid until a further call + // to AddStep, AddSubexpression, or Flatten. ExpressionStep* absl_nullable AddStep(ExpressionStep step); void Reset(); diff --git a/eval/compiler/flat_expr_builder_extensions_test.cc b/eval/compiler/flat_expr_builder_extensions_test.cc index a1e8ecd4b..2eae196af 100644 --- a/eval/compiler/flat_expr_builder_extensions_test.cc +++ b/eval/compiler/flat_expr_builder_extensions_test.cc @@ -55,6 +55,15 @@ using ::testing::ElementsAre; using ::testing::IsEmpty; using ::testing::Optional; +class TestStepLogic : public ExpressionStepLogic { + public: + void Evaluate(ExecutionFrame* frame) const override {} +}; + +std::unique_ptr MakeTestStepLogic() { + return std::make_unique(); +} + using Subexpression = ProgramBuilder::Subexpression; class PlannerContextTest : public testing::Test { @@ -94,9 +103,9 @@ struct SimpleTreeSteps { absl::StatusOr InitSimpleTree( const Expr& a, const Expr& b, const Expr& c, ProgramBuilder& program_builder) { - auto a_step = CreateConstValueStep(cel::NullValue()); - auto b_step = CreateConstValueStep(cel::NullValue()); - auto c_step = CreateConstValueStep(cel::NullValue()); + auto a_step = MakeTestStepLogic(); + auto b_step = MakeTestStepLogic(); + auto c_step = MakeTestStepLogic(); SimpleTreeSteps result{a_step.get(), b_step.get(), c_step.get()}; @@ -163,7 +172,7 @@ TEST_F(PlannerContextTest, ReplacePlan) { ExecutionPath new_a; - auto new_a_step = CreateConstValueStep(cel::NullValue()); + auto new_a_step = MakeTestStepLogic(); const ExpressionStepLogic* new_a_step_ptr = new_a_step.get(); new_a.push_back(ExpressionStep::MakeGenericStep(std::move(new_a_step), -1)); @@ -253,10 +262,10 @@ TEST_F(PlannerContextTest, ReplacePlanUpdatesSibling) { ExecutionPath new_b; - auto b1_step = CreateConstValueStep(cel::NullValue()); + auto b1_step = MakeTestStepLogic(); const ExpressionStepLogic* b1_step_ptr = b1_step.get(); new_b.push_back(ExpressionStep::MakeGenericStep(std::move(b1_step), -1)); - auto b2_step = CreateConstValueStep(cel::NullValue()); + auto b2_step = MakeTestStepLogic(); const ExpressionStepLogic* b2_step_ptr = b2_step.get(); new_b.push_back(ExpressionStep::MakeGenericStep(std::move(b2_step), -1)); @@ -302,7 +311,7 @@ TEST_F(PlannerContextTest, AddSubplanStep) { ASSERT_OK_AND_ASSIGN(auto plan_steps, InitSimpleTree(a, b, c, program_builder)); - auto b2_step = CreateConstValueStep(cel::NullValue()); + auto b2_step = MakeTestStepLogic(); const ExpressionStepLogic* b2_step_ptr = b2_step.get(); @@ -331,7 +340,7 @@ TEST_F(PlannerContextTest, AddSubplanStepFailsOnUnknownNode) { ASSERT_THAT(InitSimpleTree(a, b, c, program_builder).status(), IsOk()); - auto b2_step = CreateConstValueStep(cel::NullValue()); + auto b2_step = MakeTestStepLogic(); std::shared_ptr arena; PlannerContext context(env_, resolver_, options_, @@ -478,7 +487,7 @@ TEST_F(ProgramBuilderTest, ExtractWorks) { program_builder.EnterSubexpression(&b); program_builder.ExitSubexpression(&b); - auto a_step = CreateConstValueStep(cel::NullValue()); + auto a_step = MakeTestStepLogic(); program_builder.AddStep( ExpressionStep::MakeGenericStep(std::move(a_step), -1)); program_builder.EnterSubexpression(&c); diff --git a/eval/compiler/instrumentation.cc b/eval/compiler/instrumentation.cc index ea3e7ddbb..7f522c739 100644 --- a/eval/compiler/instrumentation.cc +++ b/eval/compiler/instrumentation.cc @@ -37,14 +37,17 @@ class InstrumentStep : public ExpressionStepBase { expr_id_(expr_id), instrumentation_(std::move(instrumentation)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(1)) { - return absl::InternalError("stack underflow in instrument step."); + frame->Abort(absl::InternalError("stack underflow in instrument step.")); + return; } - return instrumentation_(expr_id_, frame->value_stack().Peek()); - - return absl::OkStatus(); + if (absl::Status status = + instrumentation_(expr_id_, frame->value_stack().Peek()); + !status.ok()) { + frame->Abort(std::move(status)); + } } private: diff --git a/eval/compiler/regex_precompilation_optimization.cc b/eval/compiler/regex_precompilation_optimization.cc index 46c8817af..fa0d289f2 100644 --- a/eval/compiler/regex_precompilation_optimization.cc +++ b/eval/compiler/regex_precompilation_optimization.cc @@ -194,12 +194,11 @@ class RegexPrecompilationOptimization : public ProgramOptimizer { } else { // otherwise stack-machine program. ExecutionPathView re_plan = context.GetSubplan(re_expr); - if (re_plan.size() == 1 && re_plan[0].IsGenericStep() && - re_plan[0].GetGenericStep()->GetNativeTypeId() == - NativeTypeId::For()) { - constant = - down_cast(re_plan[0].GetGenericStep()) - ->value(); + if (re_plan.size() == 1) { + cel::Value val; + if (GetIfConstant(re_plan[0], val)) { + constant = std::move(val); + } } } diff --git a/eval/eval/BUILD b/eval/eval/BUILD index 1b4de3631..26b581ef4 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -35,31 +35,64 @@ package_group( cc_library( name = "evaluator_core", srcs = [ + "comprehension_step.cc", + "equality_steps.cc", "evaluator_core.cc", + "function_step.cc", + "lazy_init_step.cc", + "logic_step.cc", ], hdrs = [ + "comprehension_step.h", + "equality_steps.h", "evaluator_core.h", + "function_step.h", + "lazy_init_step.h", + "logic_step.h", ], deps = [ + ":attribute_trail", ":attribute_utility", ":comprehension_slots", + ":direct_expression_step", ":evaluator_stack", + ":expression_step_logic", ":iterator_stack", + "//base:attributes", + "//base:builtins", "//base:data", + "//common:casting", + "//common:expr", + "//common:function_descriptor", + "//common:kind", "//common:native_type", "//common:value", + "//common:value_kind", + "//eval/internal:errors", + "//internal:number", + "//internal:status_macros", "//runtime", "//runtime:activation_interface", + "//runtime:function", + "//runtime:function_overload_reference", + "//runtime:function_provider", + "//runtime:function_registry", "//runtime:runtime_options", "//runtime/internal:activation_attribute_matcher_access", + "//runtime/internal:errors", + "//runtime/standard:equality_functions", + "@com_google_absl//absl/base", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/base:nullability", + "@com_google_absl//absl/container:inlined_vector", "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/log:absl_log", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", + "@com_google_absl//absl/types:optional", "@com_google_absl//absl/types:span", + "@com_google_cel_spec//proto/cel/expr:value_cc_proto", "@com_google_protobuf//:protobuf", ], ) @@ -166,12 +199,26 @@ cc_test( ], ) +cc_library( + name = "expression_step_logic", + hdrs = [ + "expression_step_logic.h", + ], + deps = [ + "//common:native_type", + "@com_google_absl//absl/status", + ], +) + cc_library( name = "expression_step_base", hdrs = [ "expression_step_base.h", ], - deps = [":evaluator_core"], + deps = [ + ":evaluator_core", + ":expression_step_logic", + ], ) cc_library( @@ -182,7 +229,6 @@ cc_library( deps = [ ":compiler_constant_step", ":direct_expression_step", - ":evaluator_core", "//common:value", "@com_google_absl//absl/status:statusor", ], @@ -254,6 +300,7 @@ cc_library( ":direct_expression_step", ":evaluator_core", ":expression_step_base", + ":expression_step_logic", "//common:value", "//eval/internal:errors", "//internal:status_macros", @@ -266,37 +313,8 @@ cc_library( cc_library( name = "function_step", - srcs = [ - "function_step.cc", - ], - hdrs = [ - "function_step.h", - ], deps = [ - ":attribute_trail", - ":direct_expression_step", ":evaluator_core", - ":expression_step_base", - "//common:casting", - "//common:expr", - "//common:function_descriptor", - "//common:kind", - "//common:value", - "//common:value_kind", - "//eval/internal:errors", - "//internal:status_macros", - "//runtime:activation_interface", - "//runtime:function", - "//runtime:function_overload_reference", - "//runtime:function_provider", - "//runtime:function_registry", - "//runtime/internal:errors", - "@com_google_absl//absl/container:inlined_vector", - "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", - "@com_google_absl//absl/strings", - "@com_google_absl//absl/types:optional", - "@com_google_absl//absl/types:span", ], ) @@ -405,74 +423,10 @@ cc_library( ], ) -cc_library( - name = "jump_step", - srcs = [ - "jump_step.cc", - ], - hdrs = [ - "jump_step.h", - ], - deps = [ - ":evaluator_core", - ":expression_step_base", - "//common:value", - "//eval/internal:errors", - "@com_google_absl//absl/status", - "@com_google_absl//absl/types:optional", - "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", - ], -) - -cc_library( - name = "logic_step", - srcs = [ - "logic_step.cc", - ], - hdrs = [ - "logic_step.h", - ], - deps = [ - ":attribute_trail", - ":direct_expression_step", - ":evaluator_core", - ":expression_step_base", - "//base:builtins", - "//common:casting", - "//common:value", - "//common:value_kind", - "//eval/internal:errors", - "//internal:status_macros", - "//runtime/internal:errors", - "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", - "@com_google_absl//absl/types:optional", - "@com_google_absl//absl/types:span", - ], -) - cc_library( name = "equality_steps", - srcs = [ - "equality_steps.cc", - ], - hdrs = [ - "equality_steps.h", - ], deps = [ - ":attribute_trail", - ":direct_expression_step", ":evaluator_core", - ":expression_step_base", - "//base:builtins", - "//common:value", - "//common:value_kind", - "//internal:number", - "//internal:status_macros", - "//runtime/internal:errors", - "//runtime/standard:equality_functions", - "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", ], ) @@ -504,35 +458,6 @@ cc_test( ], ) -cc_library( - name = "comprehension_step", - srcs = [ - "comprehension_step.cc", - ], - hdrs = [ - "comprehension_step.h", - ], - deps = [ - ":attribute_trail", - ":comprehension_slots", - ":direct_expression_step", - ":evaluator_core", - ":expression_step_base", - "//base:attributes", - "//common:casting", - "//common:value", - "//common:value_kind", - "//eval/internal:errors", - "//internal:status_macros", - "@com_google_absl//absl/base", - "@com_google_absl//absl/base:core_headers", - "@com_google_absl//absl/base:nullability", - "@com_google_absl//absl/log:absl_check", - "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", - ], -) - cc_test( name = "comprehension_step_test", size = "small", @@ -543,7 +468,6 @@ cc_test( ":attribute_trail", ":cel_expression_flat_impl", ":comprehension_slots", - ":comprehension_step", ":const_value_step", ":direct_expression_step", ":evaluator_core", @@ -732,6 +656,7 @@ cc_test( "//runtime:standard_functions", "//runtime/internal:runtime_env_testing", "@com_google_absl//absl/status", + "@com_google_absl//absl/status:status_macros", "@com_google_absl//absl/status:status_matchers", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", @@ -753,7 +678,6 @@ cc_test( ":direct_expression_step", ":evaluator_core", ":ident_step", - ":logic_step", "//base:attributes", "//base:data", "//common:casting", @@ -1137,7 +1061,6 @@ cc_library( ":attribute_trail", ":direct_expression_step", ":evaluator_core", - ":expression_step_base", "//common:native_type", "//common:value", "@com_google_absl//absl/status", @@ -1148,7 +1071,9 @@ cc_test( name = "compiler_constant_step_test", srcs = ["compiler_constant_step_test.cc"], deps = [ + ":attribute_trail", ":compiler_constant_step", + ":direct_expression_step", ":evaluator_core", "//common:native_type", "//common:value", @@ -1162,31 +1087,13 @@ cc_test( ], ) -cc_library( - name = "lazy_init_step", - srcs = ["lazy_init_step.cc"], - hdrs = ["lazy_init_step.h"], - deps = [ - ":attribute_trail", - ":comprehension_slots", - ":direct_expression_step", - ":evaluator_core", - ":expression_step_base", - "//common:value", - "//internal:status_macros", - "@com_google_absl//absl/base:nullability", - "@com_google_absl//absl/status", - "@com_google_cel_spec//proto/cel/expr:value_cc_proto", - ], -) - cc_test( name = "lazy_init_step_test", srcs = ["lazy_init_step_test.cc"], deps = [ + ":comprehension_slots", ":const_value_step", ":evaluator_core", - ":lazy_init_step", "//base:data", "//common:value", "//internal:testing", @@ -1205,10 +1112,8 @@ cc_library( hdrs = ["direct_expression_step.h"], deps = [ ":attribute_trail", - ":evaluator_core", "//common:native_type", "//common:value", - "//internal:status_macros", "@com_google_absl//absl/status", "@com_google_absl//absl/types:optional", ], @@ -1238,15 +1143,14 @@ cc_library( ":direct_expression_step", ":evaluator_core", ":expression_step_base", - ":jump_step", + ":expression_step_logic", "//common:casting", + "//common:optional_ref", "//common:value", "//internal:status_macros", "//runtime/internal:errors", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", - "@com_google_absl//absl/types:optional", "@com_google_absl//absl/types:span", "@com_google_protobuf//:protobuf", ], diff --git a/eval/eval/compiler_constant_step.cc b/eval/eval/compiler_constant_step.cc index 44a03cecd..c1b46b92e 100644 --- a/eval/eval/compiler_constant_step.cc +++ b/eval/eval/compiler_constant_step.cc @@ -28,10 +28,4 @@ absl::Status DirectCompilerConstantStep::Evaluate( return absl::OkStatus(); } -absl::Status CompilerConstantStep::Evaluate(ExecutionFrame* frame) const { - frame->value_stack().Push(value_); - - return absl::OkStatus(); -} - } // namespace google::api::expr::runtime diff --git a/eval/eval/compiler_constant_step.h b/eval/eval/compiler_constant_step.h index 967f6512f..6708e02de 100644 --- a/eval/eval/compiler_constant_step.h +++ b/eval/eval/compiler_constant_step.h @@ -22,8 +22,6 @@ #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" namespace google::api::expr::runtime { @@ -49,27 +47,6 @@ class DirectCompilerConstantStep : public DirectExpressionStep { cel::Value value_; }; -// ExpressionStep implementation that simply pushes a constant value on the -// stack. -// -// Overrides NativeTypeId ()o allow the FlatExprBuilder and extensions to -// inspect the underlying value. -class CompilerConstantStep : public ExpressionStepBase { - public: - explicit CompilerConstantStep(cel::Value value) : value_(std::move(value)) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override; - - cel::NativeTypeId GetNativeTypeId() const override { - return cel::NativeTypeId::For(); - } - - const cel::Value& value() const { return value_; } - - private: - cel::Value value_; -}; - } // namespace google::api::expr::runtime #endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_COMPILER_CONSTANT_STEP_H_ diff --git a/eval/eval/compiler_constant_step_test.cc b/eval/eval/compiler_constant_step_test.cc index 8fb226813..181728479 100644 --- a/eval/eval/compiler_constant_step_test.cc +++ b/eval/eval/compiler_constant_step_test.cc @@ -13,10 +13,10 @@ // limitations under the License. #include "eval/eval/compiler_constant_step.h" -#include - #include "common/native_type.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 "internal/testing.h" #include "internal/testing_descriptor_pool.h" @@ -30,43 +30,41 @@ namespace google::api::expr::runtime { namespace { -class CompilerConstantStepTest : public testing::Test { +class DirectCompilerConstantStepTest : public testing::Test { public: - CompilerConstantStepTest() - : type_provider_(cel::internal::GetTestingDescriptorPool()), - state_(2, 0, type_provider_, cel::internal::GetTestingDescriptorPool(), - cel::internal::GetTestingMessageFactory(), &arena_) {} + DirectCompilerConstantStepTest() + : type_provider_(cel::internal::GetTestingDescriptorPool()) {} protected: google::protobuf::Arena arena_; cel::runtime_internal::RuntimeTypeProvider type_provider_; - FlatExpressionEvaluatorState state_; cel::Activation empty_activation_; cel::RuntimeOptions options_; }; -TEST_F(CompilerConstantStepTest, Evaluate) { - ExecutionPath path; - path.push_back(ExpressionStep::MakeGenericStep( - std::make_unique(cel::IntValue(42)))); - - ExecutionFrame frame(path, empty_activation_, options_, state_); +TEST_F(DirectCompilerConstantStepTest, Evaluate) { + ExecutionFrameBase frame(empty_activation_, options_, type_provider_, + cel::internal::GetTestingDescriptorPool(), + cel::internal::GetTestingMessageFactory(), &arena_); + DirectCompilerConstantStep step(cel::IntValue(42), -1); + cel::Value result; + AttributeTrail attr; - ASSERT_OK_AND_ASSIGN(cel::Value result, frame.Evaluate()); + ASSERT_THAT(step.Evaluate(frame, result, attr), absl_testing::IsOk()); EXPECT_EQ(result.GetInt().NativeValue(), 42); } -TEST_F(CompilerConstantStepTest, TypeId) { - CompilerConstantStep step(cel::IntValue(42)); +TEST_F(DirectCompilerConstantStepTest, TypeId) { + DirectCompilerConstantStep step(cel::IntValue(42), -1); - ExpressionStepLogic& abstract_step = step; + const DirectExpressionStep& abstract_step = step; EXPECT_EQ(abstract_step.GetNativeTypeId(), - cel::NativeTypeId::For()); + cel::NativeTypeId::For()); } -TEST_F(CompilerConstantStepTest, Value) { - CompilerConstantStep step(cel::IntValue(42)); +TEST_F(DirectCompilerConstantStepTest, Value) { + DirectCompilerConstantStep step(cel::IntValue(42), -1); EXPECT_EQ(step.value().GetInt().NativeValue(), 42); } diff --git a/eval/eval/comprehension_slots.h b/eval/eval/comprehension_slots.h index 795cca7f7..5a7ce21cc 100644 --- a/eval/eval/comprehension_slots.h +++ b/eval/eval/comprehension_slots.h @@ -16,6 +16,8 @@ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_COMPREHENSION_SLOTS_H_ #include +#include +#include #include #include "absl/base/attributes.h" @@ -110,7 +112,10 @@ class ComprehensionSlots final { return *instance; } - explicit ComprehensionSlots(size_t size) : slots_(size) {} + explicit ComprehensionSlots(size_t size) : slots_(size) { + ABSL_DCHECK_LT(size, + static_cast(std::numeric_limits::max())); + } ComprehensionSlots(const ComprehensionSlots&) = delete; ComprehensionSlots& operator=(const ComprehensionSlots&) = delete; diff --git a/eval/eval/comprehension_step.cc b/eval/eval/comprehension_step.cc index 7f6d5b4e2..6293f97f4 100644 --- a/eval/eval/comprehension_step.cc +++ b/eval/eval/comprehension_step.cc @@ -20,7 +20,7 @@ #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/iterator_stack.h" #include "eval/internal/errors.h" #include "internal/status_macros.h" @@ -41,6 +41,7 @@ using ::cel::ValueIterator; using ::cel::ValueIteratorPtr; using ::cel::ValueKind; using ::cel::runtime_internal::CreateNoMatchingOverloadError; +using ::cel::runtime_internal::IteratorStack; AttributeQualifier AttributeQualifierFromValue(const Value& v) { switch (v.kind()) { @@ -58,25 +59,6 @@ AttributeQualifier AttributeQualifierFromValue(const Value& v) { } } -class ComprehensionFinishStep final : public ExpressionStepBase { - public: - explicit ComprehensionFinishStep(size_t accu_slot) - : ExpressionStepBase(), accu_slot_(accu_slot) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - if (!frame->value_stack().HasEnough(2)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); - } - frame->value_stack().SwapAndPop(2, 1); - frame->comprehension_slots().ClearSlot(accu_slot_); - frame->iterator_stack().Pop(); - return absl::OkStatus(); - } - - private: - const size_t accu_slot_; -}; - class ComprehensionDirectStep final : public DirectExpressionStep { public: explicit ComprehensionDirectStep( @@ -446,14 +428,16 @@ absl::Status ComprehensionDirectStep::Evaluate2(ExecutionFrameBase& frame, } // namespace -absl::Status ComprehensionInitStep::Evaluate(ExecutionFrame* frame) const { +void ComprehensionInitStep::Evaluate(ExecutionFrame* frame) const { if (!frame->value_stack().HasEnough(1)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); + frame->Abort(absl::InternalError("Value stack underflow")); + return; } const Value& top = frame->value_stack().Peek(); if (top.IsError() || top.IsUnknown()) { - return frame->JumpTo(error_jump_offset_); + frame->JumpToOrAbort(error_jump_offset_); + return; } if (frame->enable_unknowns() && top.IsMap()) { @@ -461,45 +445,62 @@ absl::Status ComprehensionInitStep::Evaluate(ExecutionFrame* frame) const { if (frame->attribute_utility().CheckForUnknownPartial(top_attr)) { frame->value_stack().PopAndPush( frame->attribute_utility().CreateUnknownSet(top_attr.attribute())); - return frame->JumpTo(error_jump_offset_); + frame->JumpToOrAbort(error_jump_offset_); + return; } } + absl::StatusOr iterator; switch (top.kind()) { - case ValueKind::kList: { - CEL_ASSIGN_OR_RETURN(auto iterator, top.GetList().NewIterator()); - frame->iterator_stack().Push(std::move(iterator)); - } break; - case ValueKind::kMap: { - CEL_ASSIGN_OR_RETURN(auto iterator, top.GetMap().NewIterator()); - frame->iterator_stack().Push(std::move(iterator)); - } break; + case ValueKind::kList: + iterator = top.GetList().NewIterator(); + break; + case ValueKind::kMap: + iterator = top.GetMap().NewIterator(); + break; default: // Replace with an error and jump past // ComprehensionFinishStep. frame->value_stack().PopAndPush(cel::ErrorValue::From( CreateNoMatchingOverloadError(""), frame->arena())); - return frame->JumpTo(error_jump_offset_); + frame->JumpToOrAbort(error_jump_offset_); + return; } - return absl::OkStatus(); + if (!iterator.ok()) { + frame->Abort(std::move(iterator).status()); + return; + } + if (has_iter2_) { + frame->iterator_stack().Push(*std::move(iterator), iter_slot_, iter2_slot_, + accu_slot_); + } else { + frame->iterator_stack().Push(*std::move(iterator), iter_slot_, accu_slot_); + } } -absl::Status ComprehensionNextStep::Evaluate1(ExecutionFrame* frame) const { +void ComprehensionNextStep::Evaluate1(ExecutionFrame* frame) const { + if (frame->iterator_stack().empty()) { + frame->Abort(absl::InternalError("Iterator stack underflow")); + return; + } + const IteratorStack::Entry& entry = *frame->iterator_stack().Peek(); if (!frame->value_stack().HasEnough(2)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); + frame->Abort( + absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + return; } { Value& accu_var = frame->value_stack().Peek(); AttributeTrail& accu_var_attr = frame->value_stack().PeekAttribute(); - frame->comprehension_slots().Set(accu_slot_, std::move(accu_var), + frame->comprehension_slots().Set(entry.accu_slot, std::move(accu_var), std::move(accu_var_attr)); frame->value_stack().Pop(1); } ComprehensionSlots::Slot* iter_slot = - frame->comprehension_slots().Get(iter_slot_); + frame->comprehension_slots().Get(entry.iter_slot); ABSL_DCHECK(iter_slot != nullptr); iter_slot->Set(); @@ -519,15 +520,23 @@ absl::Status ComprehensionNextStep::Evaluate1(ExecutionFrame* frame) const { default: ABSL_UNREACHABLE(); } - CEL_ASSIGN_OR_RETURN(bool ok, - frame->iterator_stack().Peek()->Next2( - frame->descriptor_pool(), frame->message_factory(), - frame->arena(), key, value)); - if (!ok) { + absl::StatusOr ok = entry.iterator->Next2(frame->descriptor_pool(), + frame->message_factory(), + frame->arena(), key, value); + if (!ok.ok()) { + frame->Abort(std::move(ok).status()); + return; + } + if (!*ok) { iter_slot->Clear(); - return frame->JumpTo(jump_offset_); + frame->JumpToOrAbort(jump_offset_); + return; + } + absl::Status inc_status = frame->IncrementIterations(); + if (!inc_status.ok()) { + frame->Abort(std::move(inc_status)); + return; } - CEL_RETURN_IF_ERROR(frame->IncrementIterations()); *iter_slot->mutable_attribute() = frame->value_stack().PeekAttribute().Step( AttributeQualifierFromValue(*key)); if (frame->attribute_utility().CheckForUnknownExact( @@ -536,53 +545,74 @@ absl::Status ComprehensionNextStep::Evaluate1(ExecutionFrame* frame) const { iter_slot->attribute().attribute()); } } else { - CEL_ASSIGN_OR_RETURN(bool ok, - frame->iterator_stack().Peek()->Next1( - frame->descriptor_pool(), frame->message_factory(), - frame->arena(), iter_slot->mutable_value())); - if (!ok) { + absl::StatusOr ok = entry.iterator->Next1( + frame->descriptor_pool(), frame->message_factory(), frame->arena(), + iter_slot->mutable_value()); + if (!ok.ok()) { + frame->Abort(std::move(ok).status()); + return; + } + if (!*ok) { iter_slot->Clear(); - return frame->JumpTo(jump_offset_); + frame->JumpToOrAbort(jump_offset_); + return; + } + absl::Status inc_status = frame->IncrementIterations(); + if (!inc_status.ok()) { + frame->Abort(std::move(inc_status)); + return; } - CEL_RETURN_IF_ERROR(frame->IncrementIterations()); } - return absl::OkStatus(); } -absl::Status ComprehensionNextStep::Evaluate2(ExecutionFrame* frame) const { +void ComprehensionNextStep::Evaluate2(ExecutionFrame* frame) const { + if (frame->iterator_stack().empty()) { + frame->Abort(absl::InternalError("Iterator stack underflow")); + return; + } + const IteratorStack::Entry& entry = *frame->iterator_stack().Peek(); if (!frame->value_stack().HasEnough(2)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); + frame->Abort( + absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + return; } { Value& accu_var = frame->value_stack().Peek(); AttributeTrail& accu_var_attr = frame->value_stack().PeekAttribute(); - frame->comprehension_slots().Set(accu_slot_, std::move(accu_var), + frame->comprehension_slots().Set(entry.accu_slot, std::move(accu_var), std::move(accu_var_attr)); frame->value_stack().Pop(1); } ComprehensionSlots::Slot* iter_slot = - frame->comprehension_slots().Get(iter_slot_); + frame->comprehension_slots().Get(entry.iter_slot); ABSL_DCHECK(iter_slot != nullptr); iter_slot->Set(); ComprehensionSlots::Slot* iter2_slot = - frame->comprehension_slots().Get(iter2_slot_); + frame->comprehension_slots().Get(entry.iter2_slot); ABSL_DCHECK(iter2_slot != nullptr); iter2_slot->Set(); - CEL_ASSIGN_OR_RETURN( - bool ok, - frame->iterator_stack().Peek()->Next2( - frame->descriptor_pool(), frame->message_factory(), frame->arena(), - iter_slot->mutable_value(), iter2_slot->mutable_value())); - if (!ok) { + absl::StatusOr ok = entry.iterator->Next2( + frame->descriptor_pool(), frame->message_factory(), frame->arena(), + iter_slot->mutable_value(), iter2_slot->mutable_value()); + if (!ok.ok()) { + frame->Abort(std::move(ok).status()); + return; + } + if (!*ok) { iter_slot->Clear(); iter2_slot->Clear(); - return frame->JumpTo(jump_offset_); + frame->JumpToOrAbort(jump_offset_); + return; + } + absl::Status inc_status = frame->IncrementIterations(); + if (!inc_status.ok()) { + frame->Abort(std::move(inc_status)); + return; } - CEL_RETURN_IF_ERROR(frame->IncrementIterations()); if (frame->enable_unknowns()) { *iter_slot->mutable_attribute() = *iter2_slot->mutable_attribute() = frame->value_stack().PeekAttribute().Step( @@ -594,12 +624,18 @@ absl::Status ComprehensionNextStep::Evaluate2(ExecutionFrame* frame) const { iter2_slot->attribute().attribute()); } } - return absl::OkStatus(); } -absl::Status ComprehensionCondStep::Evaluate1(ExecutionFrame* frame) const { +void ComprehensionCondStep::Evaluate1(ExecutionFrame* frame) const { + if (frame->iterator_stack().empty()) { + frame->Abort(absl::InternalError("Iterator stack underflow")); + return; + } + const IteratorStack::Entry& entry = *frame->iterator_stack().Peek(); if (!frame->value_stack().HasEnough(2)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); + frame->Abort( + absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + return; } const Value& top = frame->value_stack().Peek(); switch (top.kind()) { @@ -607,33 +643,45 @@ absl::Status ComprehensionCondStep::Evaluate1(ExecutionFrame* frame) const { break; case ValueKind::kError: ABSL_FALLTHROUGH_INTENDED; - case ValueKind::kUnknown: + case ValueKind::kUnknown: { frame->value_stack().SwapAndPop(2, 1); - frame->comprehension_slots().ClearSlot(iter_slot_); - frame->comprehension_slots().ClearSlot(accu_slot_); + frame->comprehension_slots().ClearSlot(entry.iter_slot); + frame->comprehension_slots().ClearSlot(entry.accu_slot); frame->iterator_stack().Pop(); - return frame->JumpTo(error_jump_offset_); - default: + frame->JumpToOrAbort(error_jump_offset_); + return; + } + default: { frame->value_stack().PopAndPush( 2, cel::ErrorValue::From( CreateNoMatchingOverloadError(""), frame->arena())); - frame->comprehension_slots().ClearSlot(iter_slot_); - frame->comprehension_slots().ClearSlot(accu_slot_); + frame->comprehension_slots().ClearSlot(entry.iter_slot); + frame->comprehension_slots().ClearSlot(entry.accu_slot); frame->iterator_stack().Pop(); - return frame->JumpTo(error_jump_offset_); + frame->JumpToOrAbort(error_jump_offset_); + return; + } } const bool loop_condition = absl::implicit_cast(top.GetBool()); + const bool short_circuiting = frame->options().short_circuiting; frame->value_stack().Pop(1); // loop_condition - if (!loop_condition && shortcircuiting_) { - return frame->JumpTo(jump_offset_); + if (!loop_condition && short_circuiting) { + frame->JumpToOrAbort(jump_offset_); + return; } - return absl::OkStatus(); } -absl::Status ComprehensionCondStep::Evaluate2(ExecutionFrame* frame) const { +void ComprehensionCondStep::Evaluate2(ExecutionFrame* frame) const { + if (frame->iterator_stack().empty()) { + frame->Abort(absl::InternalError("Iterator stack underflow")); + return; + } + const IteratorStack::Entry& entry = *frame->iterator_stack().Peek(); if (!frame->value_stack().HasEnough(2)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); + frame->Abort( + absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + return; } const Value& top = frame->value_stack().Peek(); switch (top.kind()) { @@ -641,30 +689,35 @@ absl::Status ComprehensionCondStep::Evaluate2(ExecutionFrame* frame) const { break; case ValueKind::kError: ABSL_FALLTHROUGH_INTENDED; - case ValueKind::kUnknown: + case ValueKind::kUnknown: { frame->value_stack().SwapAndPop(2, 1); - frame->comprehension_slots().ClearSlot(iter_slot_); - frame->comprehension_slots().ClearSlot(iter2_slot_); - frame->comprehension_slots().ClearSlot(accu_slot_); + frame->comprehension_slots().ClearSlot(entry.iter_slot); + frame->comprehension_slots().ClearSlot(entry.iter2_slot); + frame->comprehension_slots().ClearSlot(entry.accu_slot); frame->iterator_stack().Pop(); - return frame->JumpTo(error_jump_offset_); - default: + frame->JumpToOrAbort(error_jump_offset_); + return; + } + default: { frame->value_stack().PopAndPush( 2, cel::ErrorValue::From( CreateNoMatchingOverloadError(""), frame->arena())); - frame->comprehension_slots().ClearSlot(iter_slot_); - frame->comprehension_slots().ClearSlot(iter2_slot_); - frame->comprehension_slots().ClearSlot(accu_slot_); + frame->comprehension_slots().ClearSlot(entry.iter_slot); + frame->comprehension_slots().ClearSlot(entry.iter2_slot); + frame->comprehension_slots().ClearSlot(entry.accu_slot); frame->iterator_stack().Pop(); - return frame->JumpTo(error_jump_offset_); + frame->JumpToOrAbort(error_jump_offset_); + return; + } } const bool loop_condition = absl::implicit_cast(top.GetBool()); + const bool short_circuiting = frame->options().short_circuiting; frame->value_stack().Pop(1); // loop_condition - if (!loop_condition && shortcircuiting_) { - return frame->JumpTo(jump_offset_); + if (!loop_condition && short_circuiting) { + frame->JumpToOrAbort(jump_offset_); + return; } - return absl::OkStatus(); } std::unique_ptr CreateDirectComprehensionStep( @@ -681,9 +734,15 @@ std::unique_ptr CreateDirectComprehensionStep( shortcircuiting, expr_id); } -std::unique_ptr CreateComprehensionFinishStep( - size_t accu_slot) { - return std::make_unique(accu_slot); +void EvaluateComprehensionFinishStep(size_t accu_slot, ExecutionFrame& frame) { + if (!frame.value_stack().HasEnough(2)) { + frame.Abort( + absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + return; + } + frame.value_stack().SwapAndPop(2, 1); + frame.comprehension_slots().ClearSlot(accu_slot); + frame.iterator_stack().Pop(); } } // namespace google::api::expr::runtime diff --git a/eval/eval/comprehension_step.h b/eval/eval/comprehension_step.h index f16b62e13..8bac8dab3 100644 --- a/eval/eval/comprehension_step.h +++ b/eval/eval/comprehension_step.h @@ -8,8 +8,8 @@ #include "absl/status/status.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/eval/iterator_stack.h" namespace google::api::expr::runtime { @@ -25,75 +25,63 @@ namespace google::api::expr::runtime { // 8: 1 -> 2 // 9: ComprehensionFinishStep 2 -> 1 -class ComprehensionInitStep final : public ExpressionStepBase { +class ComprehensionInitStep final : public ExpressionStepLogic { public: - ComprehensionInitStep() : ExpressionStepBase() {} + ComprehensionInitStep(size_t iter_slot, size_t iter2_slot, size_t accu_slot) + : iter_slot_(iter_slot), + iter2_slot_(iter2_slot), + accu_slot_(accu_slot), + has_iter2_(true) {} + ComprehensionInitStep(size_t iter_slot, size_t accu_slot) + : iter_slot_(iter_slot), + iter2_slot_(0), + accu_slot_(accu_slot), + has_iter2_(false) {} void set_error_jump_offset(int offset) { error_jump_offset_ = offset; } - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: + const size_t iter_slot_; + const size_t iter2_slot_; + const size_t accu_slot_; + bool has_iter2_ = false; int error_jump_offset_ = std::numeric_limits::max(); }; -class ComprehensionNextStep final : public ExpressionStepBase { +class ComprehensionNextStep final { public: - ComprehensionNextStep(size_t iter_slot, size_t iter2_slot, size_t accu_slot) - : ExpressionStepBase(), - iter_slot_(iter_slot), - iter2_slot_(iter2_slot), - accu_slot_(accu_slot) {} + ComprehensionNextStep() = default; void set_jump_offset(int offset) { jump_offset_ = offset; } void set_error_jump_offset(int offset) { error_jump_offset_ = offset; } - absl::Status Evaluate(ExecutionFrame* frame) const override { - return iter_slot_ == iter2_slot_ ? Evaluate1(frame) : Evaluate2(frame); - } - - private: - absl::Status Evaluate1(ExecutionFrame* frame) const; + void Evaluate1(ExecutionFrame* frame) const; - absl::Status Evaluate2(ExecutionFrame* frame) const; + void Evaluate2(ExecutionFrame* frame) const; - const size_t iter_slot_; - const size_t iter2_slot_; - const size_t accu_slot_; - int jump_offset_ = std::numeric_limits::max(); - int error_jump_offset_ = std::numeric_limits::max(); + private: + int32_t jump_offset_ = std::numeric_limits::max(); + int32_t error_jump_offset_ = std::numeric_limits::max(); }; -class ComprehensionCondStep final : public ExpressionStepBase { +class ComprehensionCondStep final { public: - ComprehensionCondStep(size_t iter_slot, size_t iter2_slot, size_t accu_slot, - bool shortcircuiting) - : ExpressionStepBase(), - iter_slot_(iter_slot), - iter2_slot_(iter2_slot), - accu_slot_(accu_slot), - shortcircuiting_(shortcircuiting) {} + ComprehensionCondStep() = default; void set_jump_offset(int offset) { jump_offset_ = offset; } void set_error_jump_offset(int offset) { error_jump_offset_ = offset; } - absl::Status Evaluate(ExecutionFrame* frame) const override { - return iter_slot_ == iter2_slot_ ? Evaluate1(frame) : Evaluate2(frame); - } - - private: - absl::Status Evaluate1(ExecutionFrame* frame) const; + void Evaluate1(ExecutionFrame* frame) const; - absl::Status Evaluate2(ExecutionFrame* frame) const; + void Evaluate2(ExecutionFrame* frame) const; - const size_t iter_slot_; - const size_t iter2_slot_; - const size_t accu_slot_; - int jump_offset_ = std::numeric_limits::max(); - int error_jump_offset_ = std::numeric_limits::max(); - const bool shortcircuiting_; + private: + int32_t jump_offset_ = std::numeric_limits::max(); + int32_t error_jump_offset_ = std::numeric_limits::max(); }; // Creates a step for executing a comprehension. @@ -106,11 +94,10 @@ std::unique_ptr CreateDirectComprehensionStep( std::unique_ptr result_step, bool shortcircuiting, int64_t expr_id); -// Creates a cleanup step for the comprehension. +// Runs a cleanup step for the comprehension. // Removes the comprehension context then pushes the 'result' sub expression to // the top of the stack. -std::unique_ptr CreateComprehensionFinishStep( - size_t accu_slot); +void EvaluateComprehensionFinishStep(size_t accu_slot, ExecutionFrame& frame); } // namespace google::api::expr::runtime diff --git a/eval/eval/comprehension_step_test.cc b/eval/eval/comprehension_step_test.cc index ef01b034d..2ffbafb1b 100644 --- a/eval/eval/comprehension_step_test.cc +++ b/eval/eval/comprehension_step_test.cc @@ -7,7 +7,6 @@ #include "cel/expr/syntax.pb.h" #include "google/protobuf/struct.pb.h" -#include "absl/memory/memory.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" @@ -44,7 +43,6 @@ using ::absl_testing::IsOk; using ::absl_testing::StatusIs; using ::cel::BoolValue; using ::cel::Expr; -using ::cel::IdentExpr; using ::cel::IntValue; using ::cel::TypeProvider; using ::cel::Value; @@ -57,12 +55,6 @@ using ::testing::Eq; using ::testing::Return; using ::testing::SizeIs; -IdentExpr CreateIdent(const std::string& var) { - IdentExpr expr; - expr.set_name(var); - return expr; -} - class ListKeysStepTest : public testing::Test { public: ListKeysStepTest() = default; @@ -89,9 +81,8 @@ class GetListKeysResultStep : public ExpressionStepBase { public: GetListKeysResultStep() : ExpressionStepBase() {} - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Pop(1); - return absl::OkStatus(); } }; @@ -104,7 +95,8 @@ MATCHER_P(CelStringValue, val, "") { TEST_F(ListKeysStepTest, MapPartiallyUnknown) { ExecutionPath path; path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("var"))); - auto init_step = std::make_unique(); + auto init_step = + std::make_unique(/*iter_slot=*/0, /*accu_slot=*/0); init_step->set_error_jump_offset(1); path.push_back(ExpressionStep::MakeGenericStep(std::move(init_step))); path.push_back(ExpressionStep::MakeGenericStep( @@ -141,7 +133,8 @@ TEST_F(ListKeysStepTest, MapPartiallyUnknown) { TEST_F(ListKeysStepTest, ErrorPassedThrough) { ExecutionPath path; path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("var"))); - auto init_step = std::make_unique(); + auto init_step = + std::make_unique(/*iter_slot=*/0, /*accu_slot=*/0); init_step->set_error_jump_offset(1); path.push_back(ExpressionStep::MakeGenericStep(std::move(init_step))); path.push_back(ExpressionStep::MakeGenericStep( @@ -165,7 +158,8 @@ TEST_F(ListKeysStepTest, ErrorPassedThrough) { TEST_F(ListKeysStepTest, UnknownSetPassedThrough) { ExecutionPath path; path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("var"))); - auto init_step = std::make_unique(); + auto init_step = + std::make_unique(/*iter_slot=*/0, /*accu_slot=*/0); init_step->set_error_jump_offset(1); path.push_back(ExpressionStep::MakeGenericStep(std::move(init_step))); path.push_back(ExpressionStep::MakeGenericStep( diff --git a/eval/eval/const_value_step.h b/eval/eval/const_value_step.h index 8a08b580a..55425ceab 100644 --- a/eval/eval/const_value_step.h +++ b/eval/eval/const_value_step.h @@ -9,7 +9,6 @@ #include "common/value.h" #include "eval/eval/compiler_constant_step.h" #include "eval/eval/direct_expression_step.h" -#include "eval/eval/evaluator_core.h" namespace google::api::expr::runtime { @@ -19,12 +18,6 @@ inline std::unique_ptr CreateConstValueDirectStep( return std::make_unique(std::move(value), id); } -// Factory method for Constant AST node expression step. -inline std::unique_ptr CreateConstValueStep( - cel::Value value) { - return std::make_unique(std::move(value)); -} - } // namespace google::api::expr::runtime #endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_CONST_VALUE_STEP_H_ diff --git a/eval/eval/container_access_step.cc b/eval/eval/container_access_step.cc index 8ce87ef3f..f4a33b217 100644 --- a/eval/eval/container_access_step.cc +++ b/eval/eval/container_access_step.cc @@ -289,17 +289,17 @@ class ContainerAccessStep : public ExpressionStepBase { explicit ContainerAccessStep(bool enable_optional_types) : ExpressionStepBase(), enable_optional_types_(enable_optional_types) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: bool enable_optional_types_; }; -absl::Status ContainerAccessStep::Evaluate(ExecutionFrame* frame) const { +void ContainerAccessStep::Evaluate(ExecutionFrame* frame) const { if (!frame->value_stack().HasEnough(kNumContainerAccessArguments)) { - return absl::Status( - absl::StatusCode::kInternal, - "Insufficient arguments supplied for ContainerAccess-type expression"); + frame->Abort(absl::InternalError( + "Insufficient arguments supplied for ContainerAccess-type expression")); + return; } Value result; @@ -312,8 +312,6 @@ absl::Status ContainerAccessStep::Evaluate(ExecutionFrame* frame) const { enable_optional_types_, result, result_trail); frame->value_stack().PopAndPush(kNumContainerAccessArguments, std::move(result), std::move(result_trail)); - - return absl::OkStatus(); } class DirectContainerAccessStep : public DirectExpressionStep { diff --git a/eval/eval/create_list_step.cc b/eval/eval/create_list_step.cc index 54c9ee8ba..65636f347 100644 --- a/eval/eval/create_list_step.cc +++ b/eval/eval/create_list_step.cc @@ -38,7 +38,7 @@ class CreateListStep : public ExpressionStepBase { CreateListStep(int list_size, absl::flat_hash_set optional_indices) : list_size_(list_size), optional_indices_(std::move(optional_indices)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: absl::Status DoEvaluate(ExecutionFrame* frame, Value* result) const; @@ -47,22 +47,24 @@ class CreateListStep : public ExpressionStepBase { absl::flat_hash_set optional_indices_; }; -absl::Status CreateListStep::Evaluate(ExecutionFrame* frame) const { +void CreateListStep::Evaluate(ExecutionFrame* frame) const { if (list_size_ < 0) { - return absl::Status(absl::StatusCode::kInternal, - "CreateListStep: list size is <0"); + frame->Abort(absl::InternalError("CreateListStep: list size is <0")); + return; } if (!frame->value_stack().HasEnough(list_size_)) { - return absl::Status(absl::StatusCode::kInternal, - "CreateListStep: stack underflow"); + frame->Abort(absl::InternalError("CreateListStep: stack underflow")); + return; } Value result; - CEL_RETURN_IF_ERROR(DoEvaluate(frame, &result)); + if (absl::Status status = DoEvaluate(frame, &result); !status.ok()) { + frame->Abort(std::move(status)); + return; + } frame->value_stack().PopAndPush(list_size_, std::move(result)); - return absl::OkStatus(); } absl::Status CreateListStep::DoEvaluate(ExecutionFrame* frame, @@ -221,20 +223,6 @@ class CreateListDirectStep : public DirectExpressionStep { absl::flat_hash_set optional_indices_; }; -class MutableListStep : public ExpressionStepBase { - public: - MutableListStep() = default; - - absl::Status Evaluate(ExecutionFrame* frame) const override; -}; - -absl::Status MutableListStep::Evaluate(ExecutionFrame* frame) const { - frame->value_stack().Push(cel::CustomListValue( - cel::common_internal::NewMutableListValue(frame->arena()), - frame->arena())); - return absl::OkStatus(); -} - class DirectMutableListStep : public DirectExpressionStep { public: explicit DirectMutableListStep(int64_t expr_id) @@ -268,10 +256,6 @@ absl::StatusOr> CreateCreateListStep( MakeOptionalIndicesSet(create_list_expr)); } -std::unique_ptr CreateMutableListStep() { - return std::make_unique(); -} - std::unique_ptr CreateDirectMutableListStep( int64_t expr_id) { return std::make_unique(expr_id); diff --git a/eval/eval/create_list_step.h b/eval/eval/create_list_step.h index c377338c5..3423febe5 100644 --- a/eval/eval/create_list_step.h +++ b/eval/eval/create_list_step.h @@ -22,12 +22,6 @@ std::unique_ptr CreateDirectListStep( absl::StatusOr> CreateCreateListStep( const cel::ListExpr& create_list_expr); -// Factory method for CreateList which constructs a mutable list. -// -// This is intended for the list construction step is generated for a -// list-building comprehension (rather than a user authored expression). -std::unique_ptr CreateMutableListStep(); - // Factory method for CreateList which constructs a mutable list. // // This is intended for the list construction step is generated for a diff --git a/eval/eval/create_list_step_test.cc b/eval/eval/create_list_step_test.cc index ebb1ecb41..2faf1f11e 100644 --- a/eval/eval/create_list_step_test.cc +++ b/eval/eval/create_list_step_test.cc @@ -80,8 +80,8 @@ absl::StatusOr RunExpression( for (auto value : values) { auto& expr0 = create_list.mutable_elements().emplace_back().mutable_expr(); expr0.mutable_const_expr().set_int64_value(value); - path.push_back(ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::interop_internal::CreateIntValue(value)))); + path.push_back(ExpressionStep::MakeConstant( + cel::interop_internal::CreateIntValue(value))); } CEL_ASSIGN_OR_RETURN(auto step, CreateCreateListStep(create_list)); diff --git a/eval/eval/create_map_step.cc b/eval/eval/create_map_step.cc index e90de1fc7..4b27e5e30 100644 --- a/eval/eval/create_map_step.cc +++ b/eval/eval/create_map_step.cc @@ -57,7 +57,7 @@ class CreateStructStepForMap final : public ExpressionStepBase { : entry_count_(entry_count), optional_indices_(std::move(optional_indices)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: absl::StatusOr DoEvaluate(ExecutionFrame* frame) const; @@ -118,16 +118,20 @@ absl::StatusOr CreateStructStepForMap::DoEvaluate( return std::move(*builder).Build(); } -absl::Status CreateStructStepForMap::Evaluate(ExecutionFrame* frame) const { +void CreateStructStepForMap::Evaluate(ExecutionFrame* frame) const { if (frame->value_stack().size() < 2 * entry_count_) { - return absl::InternalError("CreateStructStepForMap: stack underflow"); + frame->Abort( + absl::InternalError("CreateStructStepForMap: stack underflow")); + return; } - CEL_ASSIGN_OR_RETURN(auto result, DoEvaluate(frame)); - - frame->value_stack().PopAndPush(2 * entry_count_, std::move(result)); + absl::StatusOr result = DoEvaluate(frame); + if (!result.ok()) { + frame->Abort(std::move(result).status()); + return; + } - return absl::OkStatus(); + frame->value_stack().PopAndPush(2 * entry_count_, *std::move(result)); } class DirectCreateMapStep : public DirectExpressionStep { @@ -238,10 +242,9 @@ class MutableMapStep final : public ExpressionStepBase { public: MutableMapStep() = default; - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Push(cel::CustomMapValue( NewMutableMapValue(frame->arena()), frame->arena())); - return absl::OkStatus(); } }; diff --git a/eval/eval/create_struct_step.cc b/eval/eval/create_struct_step.cc index a4467a736..bce2a3ea9 100644 --- a/eval/eval/create_struct_step.cc +++ b/eval/eval/create_struct_step.cc @@ -55,7 +55,7 @@ class CreateStructStepForStruct final : public ExpressionStepBase { entries_(std::move(entries)), optional_indices_(std::move(optional_indices)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: absl::StatusOr DoEvaluate(ExecutionFrame* frame) const; @@ -131,14 +131,18 @@ absl::StatusOr CreateStructStepForStruct::DoEvaluate( return std::move(*builder).Build(); } -absl::Status CreateStructStepForStruct::Evaluate(ExecutionFrame* frame) const { +void CreateStructStepForStruct::Evaluate(ExecutionFrame* frame) const { if (frame->value_stack().size() < entries_.size()) { - return absl::InternalError("CreateStructStepForStruct: stack underflow"); + frame->Abort( + absl::InternalError("CreateStructStepForStruct: stack underflow")); + return; } - CEL_ASSIGN_OR_RETURN(Value result, DoEvaluate(frame)); - frame->value_stack().PopAndPush(entries_.size(), std::move(result)); - - return absl::OkStatus(); + absl::StatusOr result = DoEvaluate(frame); + if (!result.ok()) { + frame->Abort(std::move(result).status()); + return; + } + frame->value_stack().PopAndPush(entries_.size(), *std::move(result)); } class DirectCreateStructStep : public DirectExpressionStep { diff --git a/eval/eval/direct_expression_step.cc b/eval/eval/direct_expression_step.cc index 2d7fc6fc0..36b5ed1f5 100644 --- a/eval/eval/direct_expression_step.cc +++ b/eval/eval/direct_expression_step.cc @@ -13,22 +13,8 @@ // limitations under the License. #include "eval/eval/direct_expression_step.h" -#include - -#include "absl/status/status.h" -#include "common/value.h" -#include "eval/eval/attribute_trail.h" -#include "eval/eval/evaluator_core.h" -#include "internal/status_macros.h" - namespace google::api::expr::runtime { -absl::Status WrappedDirectStep::Evaluate(ExecutionFrame* frame) const { - cel::Value result; - AttributeTrail attribute_trail; - CEL_RETURN_IF_ERROR(impl_->Evaluate(*frame, result, attribute_trail)); - frame->value_stack().Push(std::move(result), std::move(attribute_trail)); - return absl::OkStatus(); -} +// Intentionally empty. } // namespace google::api::expr::runtime diff --git a/eval/eval/direct_expression_step.h b/eval/eval/direct_expression_step.h index ac783d2d7..547adad41 100644 --- a/eval/eval/direct_expression_step.h +++ b/eval/eval/direct_expression_step.h @@ -24,10 +24,11 @@ #include "common/native_type.h" #include "common/value.h" #include "eval/eval/attribute_trail.h" -#include "eval/eval/evaluator_core.h" namespace google::api::expr::runtime { +class ExecutionFrameBase; + // Represents a directly evaluated CEL expression. // // Subexpressions assign to values on the C++ program stack and call their @@ -76,25 +77,6 @@ class DirectExpressionStep { int64_t expr_id_; }; -// Wrapper for direct steps to work with the stack machine impl. -class WrappedDirectStep : public ExpressionStepLogic { - public: - explicit WrappedDirectStep(std::unique_ptr impl, - int64_t expr_id = -1) - : impl_(std::move(impl)) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override; - - cel::NativeTypeId GetNativeTypeId() const override { - return cel::NativeTypeId::For(); - } - - const DirectExpressionStep* wrapped() const { return impl_.get(); } - - private: - std::unique_ptr impl_; -}; - } // namespace google::api::expr::runtime #endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_DIRECT_EXPRESSION_STEP_H_ diff --git a/eval/eval/equality_steps.cc b/eval/eval/equality_steps.cc index 0069aef1e..483b91468 100644 --- a/eval/eval/equality_steps.cc +++ b/eval/eval/equality_steps.cc @@ -25,7 +25,6 @@ #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/number.h" #include "internal/status_macros.h" #include "runtime/internal/errors.h" @@ -107,29 +106,6 @@ class DirectEqualityStep : public DirectExpressionStep { bool negation_; }; -class IterativeEqualityStep : public ExpressionStepBase { - public: - explicit IterativeEqualityStep(bool negation) : negation_(negation) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - if (!frame->value_stack().HasEnough(2)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); - } - auto args = frame->value_stack().GetSpan(2); - auto attrs = frame->value_stack().GetAttributeSpan(2); - - CEL_ASSIGN_OR_RETURN(Value result, - EvaluateEquality(*frame, args[0], attrs[0], args[1], - attrs[1], negation_)); - - frame->value_stack().PopAndPush(2, std::move(result)); - return absl::OkStatus(); - } - - private: - bool negation_; -}; - absl::StatusOr EvaluateInMap(ExecutionFrameBase& frame, const Value& item, const MapValue& container) { @@ -245,26 +221,45 @@ class DirectInStep : public DirectExpressionStep { std::unique_ptr container_; }; -class IterativeInStep : public ExpressionStepBase { - public: - IterativeInStep() = default; - - absl::Status Evaluate(ExecutionFrame* frame) const override { - if (!frame->value_stack().HasEnough(2)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); - } - - auto args = frame->value_stack().GetSpan(2); - auto attrs = frame->value_stack().GetAttributeSpan(2); +} // namespace - CEL_ASSIGN_OR_RETURN( - Value result, EvaluateIn(*frame, args[0], attrs[0], args[1], attrs[1])); - frame->value_stack().PopAndPush(2, std::move(result)); - return absl::OkStatus(); +void EvaluateFastEqualStep(bool negation, ExecutionFrame& frame) { + if (!frame.value_stack().HasEnough(2)) { + frame.Abort( + absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + return; } -}; + absl::Span args = frame.value_stack().GetSpan(2); + absl::Span attrs = + frame.value_stack().GetAttributeSpan(2); + + absl::StatusOr result = + EvaluateEquality(frame, args[0], attrs[0], args[1], attrs[1], negation); + if (!result.ok()) { + frame.Abort(std::move(result).status()); + return; + } + frame.value_stack().PopAndPush(2, *std::move(result)); +} -} // namespace +void EvaluateFastInStep(ExecutionFrame& frame) { + if (!frame.value_stack().HasEnough(2)) { + frame.Abort( + absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + return; + } + absl::Span args = frame.value_stack().GetSpan(2); + absl::Span attrs = + frame.value_stack().GetAttributeSpan(2); + + absl::StatusOr result = + EvaluateIn(frame, args[0], attrs[0], args[1], attrs[1]); + if (!result.ok()) { + frame.Abort(std::move(result).status()); + return; + } + frame.value_stack().PopAndPush(2, *std::move(result)); +} // Factory method for recursive _==_ and _!=_ Execution step std::unique_ptr CreateDirectEqualityStep( @@ -274,11 +269,6 @@ std::unique_ptr CreateDirectEqualityStep( negation, expr_id); } -// Factory method for iterative _==_ and _!=_ Execution step -std::unique_ptr CreateEqualityStep(bool negation) { - return std::make_unique(negation); -} - // Factory method for recursive @in Execution step std::unique_ptr CreateDirectInStep( std::unique_ptr item, @@ -287,9 +277,4 @@ std::unique_ptr CreateDirectInStep( expr_id); } -// Factory method for iterative @in Execution step -std::unique_ptr CreateInStep() { - return std::make_unique(); -} - } // namespace google::api::expr::runtime diff --git a/eval/eval/equality_steps.h b/eval/eval/equality_steps.h index 7058f9c56..141e571d1 100644 --- a/eval/eval/equality_steps.h +++ b/eval/eval/equality_steps.h @@ -19,25 +19,24 @@ #include #include "eval/eval/direct_expression_step.h" -#include "eval/eval/evaluator_core.h" namespace google::api::expr::runtime { +class ExecutionFrame; + // Factory method for recursive _==_/_!=_ Execution step std::unique_ptr CreateDirectEqualityStep( std::unique_ptr lhs, std::unique_ptr rhs, bool negation, int64_t expr_id); -// Factory method for iterative _==_/_!=_ Execution step -std::unique_ptr CreateEqualityStep(bool negation); +void EvaluateFastEqualStep(bool negation, ExecutionFrame& frame); // Factory method for recursive @in Execution step std::unique_ptr CreateDirectInStep( std::unique_ptr item, std::unique_ptr container, int64_t expr_id); -// Factory method for iterative @in Execution step -std::unique_ptr CreateInStep(); +void EvaluateFastInStep(ExecutionFrame& frame); } // namespace google::api::expr::runtime diff --git a/eval/eval/equality_steps_test.cc b/eval/eval/equality_steps_test.cc index 3b7e93626..76031e169 100644 --- a/eval/eval/equality_steps_test.cc +++ b/eval/eval/equality_steps_test.cc @@ -64,9 +64,8 @@ class ValueStep : public ExpressionStepLogic, public DirectExpressionStep { value_(std::move(value)), attr_() {} - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Push(value_, attr_); - return absl::OkStatus(); } absl::Status Evaluate(ExecutionFrameBase& frame, Value& result, @@ -153,7 +152,7 @@ TEST(IterativeTest, PartialAttrUnknown) { std::make_unique(IntValue(1), cel::Attribute("foo")))); steps.push_back(ExpressionStep::MakeGenericStep( std::make_unique(IntValue(2)))); - steps.push_back(ExpressionStep::MakeGenericStep(CreateEqualityStep(false))); + steps.push_back(ExpressionStep::MakeFastEqualStep()); activation.SetUnknownPatterns({cel::AttributePattern( "foo", {cel::AttributeQualifierPattern::OfString("bar")})}); @@ -184,7 +183,7 @@ TEST(IterativeTest, PartialAttrUnknownDisabled) { std::make_unique(IntValue(1), cel::Attribute("foo")))); steps.push_back(ExpressionStep::MakeGenericStep( std::make_unique(IntValue(2)))); - steps.push_back(ExpressionStep::MakeGenericStep(CreateEqualityStep(false))); + steps.push_back(ExpressionStep::MakeFastEqualStep()); activation.SetUnknownPatterns({cel::AttributePattern( "foo", {cel::AttributeQualifierPattern::OfString("bar")})}); @@ -291,8 +290,8 @@ TEST_P(EqualsTest, Iterative) { std::make_unique(MakeValue(test_case.lhs, &arena)))); steps.push_back(ExpressionStep::MakeGenericStep( std::make_unique(MakeValue(test_case.rhs, &arena)))); - steps.push_back( - ExpressionStep::MakeGenericStep(CreateEqualityStep(test_case.negation))); + steps.push_back(test_case.negation ? ExpressionStep::MakeFastNotEqualStep() + : ExpressionStep::MakeFastEqualStep()); ExecutionFrame frame(steps, activation, opts, state); @@ -473,7 +472,7 @@ TEST_P(InTest, Iterative) { std::make_unique(MakeValue(test_case.lhs, &arena)))); steps.push_back(ExpressionStep::MakeGenericStep( std::make_unique(MakeValue(test_case.rhs, &arena)))); - steps.push_back(ExpressionStep::MakeGenericStep(CreateInStep())); + steps.push_back(ExpressionStep::MakeFastInStep()); ExecutionFrame frame(steps, activation, opts, state); diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index 0aa6000a4..b34c09c7c 100644 --- a/eval/eval/evaluator_core.cc +++ b/eval/eval/evaluator_core.cc @@ -15,8 +15,13 @@ #include "eval/eval/evaluator_core.h" #include +#include +#include +#include +#include #include +#include "absl/base/attributes.h" #include "absl/base/nullability.h" #include "absl/base/optimization.h" #include "absl/log/absl_check.h" @@ -24,8 +29,19 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" +#include "absl/types/span.h" #include "common/value.h" +#include "common/value_kind.h" +#include "common/values/list_value_builder.h" +#include "eval/eval/attribute_trail.h" +#include "eval/eval/comprehension_slots.h" +#include "eval/eval/comprehension_step.h" +#include "eval/eval/equality_steps.h" +#include "eval/eval/lazy_init_step.h" +#include "eval/eval/logic_step.h" +#include "internal/status_macros.h" #include "runtime/activation_interface.h" +#include "runtime/internal/errors.h" #include "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/message.h" @@ -38,7 +54,7 @@ void FlatExpressionEvaluatorState::Reset() { comprehension_slots_.Reset(); } -const ExpressionStep* ExecutionFrame::Next() { +const ExpressionStep* ExecutionFrame::NextSlow() { while (true) { const size_t end_pos = execution_path_.size(); @@ -65,6 +81,13 @@ const ExpressionStep* ExecutionFrame::Next() { } } +void ExecutionFrame::AbortJumpOutOfRange(int offset) { + Abort(absl::Status( + absl::StatusCode::kInternal, + absl::StrCat("Jump address out of range: position: ", pc_, + ", offset: ", offset, ", range: ", execution_path_.size()))); +} + namespace { // This class abuses the fact that `absl::Status` is trivially destructible when @@ -102,42 +125,88 @@ class EvaluationStatus final { } // namespace +void ExpressionStep::EvaluateMutableListAppendStep(ExecutionFrame& frame) { + if (!frame.value_stack().HasEnough(2)) { + frame.Abort( + absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + return; + } + absl::Span args = frame.value_stack().GetSpan(2); + if (args[0].IsError()) { + frame.value_stack().Pop(1); + return; + } + if (args[1].IsError()) { + frame.value_stack().SwapAndPop(2, 1); + return; + } + if (frame.unknown_processing_enabled()) { + std::optional unknown_set = + frame.attribute_utility().IdentifyAndMergeUnknowns( + args, frame.value_stack().GetAttributeSpan(2), + /*use_partial=*/true); + if (unknown_set.has_value()) { + frame.value_stack().PopAndPush(2, std::move(*unknown_set)); + return; + } + } + if (const cel::common_internal::MutableListValue* mutable_list_value = + cel::common_internal::AsMutableListValue(args[0]); + mutable_list_value != nullptr) { + absl::Status status = mutable_list_value->Append(args[1]); + if (!status.ok()) { + frame.Abort(std::move(status)); + return; + } + frame.value_stack().Pop(1); + return; + } + frame.Abort( + absl::InvalidArgumentError("Unexpected call to runtime list append.")); +} + +void ExecutionFrame::Interpret() { + for (const ExpressionStep* expr = Next(); expr != nullptr; expr = Next()) { + expr->Evaluate(*this); + } +} + +void ExecutionFrame::InterpretWithCallback(EvaluationListener& listener) { + for (const ExpressionStep* expr = Next(); expr != nullptr; expr = Next()) { + expr->Evaluate(*this); + if (pc_ == 0 || !expr->comes_from_ast() || !abort_status().ok()) { + // Skip if we just started a Call, if the step doesn't map to an AST + // id, or if evaluation was aborted. + continue; + } + + if (ABSL_PREDICT_FALSE(value_stack().empty())) { + ABSL_LOG(ERROR) << "Stack is empty after a ExpressionStep.Evaluate. " + "Try to disable short-circuiting."; + continue; + } + if (EvaluationStatus status(listener(expr->id(), value_stack().Peek(), + descriptor_pool(), message_factory(), + arena())); + !status.ok()) { + Abort(std::move(status).Consume()); + return; + } + } +} + absl::StatusOr ExecutionFrame::Evaluate( EvaluationListener& listener) { const size_t initial_stack_size = value_stack().size(); if (!listener) { - for (const ExpressionStep* expr = Next(); - ABSL_PREDICT_TRUE(expr != nullptr); expr = Next()) { - if (EvaluationStatus status(expr->Evaluate(this)); !status.ok()) { - return std::move(status).Consume(); - } - } + Interpret(); } else { - for (const ExpressionStep* expr = Next(); - ABSL_PREDICT_TRUE(expr != nullptr); expr = Next()) { - if (EvaluationStatus status(expr->Evaluate(this)); !status.ok()) { - return std::move(status).Consume(); - } - - if (pc_ == 0 || !expr->comes_from_ast()) { - // Skip if we just started a Call or if the step doesn't map to an - // AST id. - continue; - } + InterpretWithCallback(listener); + } - if (ABSL_PREDICT_FALSE(value_stack().empty())) { - ABSL_LOG(ERROR) << "Stack is empty after a ExpressionStep.Evaluate. " - "Try to disable short-circuiting."; - continue; - } - if (EvaluationStatus status(listener(expr->id(), value_stack().Peek(), - descriptor_pool(), message_factory(), - arena())); - !status.ok()) { - return std::move(status).Consume(); - } - } + if (!abort_status().ok()) { + return std::move(abort_status()); } const size_t final_stack_size = value_stack().size(); @@ -174,4 +243,129 @@ absl::StatusOr FlatExpression::EvaluateWithCallback( return frame.Evaluate(frame.callback()); } +ExpressionStep ExpressionStep::MakeConstant(const cel::Value& value, + int64_t id) { + if (id < 0 || id > std::numeric_limits::max()) { + id = -1; + } + int32_t id32 = static_cast(id); + switch (value.kind()) { + case cel::ValueKind::kBool: { + ExpressionStep step(ExpressionStepKind::kBoolConstant, id32); + step.u_.bool_val = value.GetBool().NativeValue(); + return step; + } + case cel::ValueKind::kInt: { + ExpressionStep step(ExpressionStepKind::kIntConstant, id32); + step.u_.int_val = value.GetInt().NativeValue(); + return step; + } + case cel::ValueKind::kUint: { + ExpressionStep step(ExpressionStepKind::kUintConstant, id32); + step.u_.uint_val = value.GetUint().NativeValue(); + return step; + } + case cel::ValueKind::kDouble: { + ExpressionStep step(ExpressionStepKind::kDoubleConstant, id32); + step.u_.double_val = value.GetDouble().NativeValue(); + return step; + } + case cel::ValueKind::kNull: + return ExpressionStep(ExpressionStepKind::kNullConstant, id32); + default: { + ExpressionStep step(ExpressionStepKind::kOtherConstant, id32); + step.u_.other_val = new cel::Value(value); + return step; + } + } +} + +bool GetIfConstant(const ExpressionStep& step, cel::Value& out) { + switch (step.header_.kind) { + case ExpressionStepKind::kIntConstant: + out = cel::IntValue(step.u_.int_val); + return true; + case ExpressionStepKind::kBoolConstant: + out = cel::BoolValue(step.u_.bool_val); + return true; + case ExpressionStepKind::kDoubleConstant: + out = cel::DoubleValue(step.u_.double_val); + return true; + case ExpressionStepKind::kNullConstant: + out = cel::NullValue(); + return true; + case ExpressionStepKind::kUintConstant: + out = cel::UintValue(step.u_.uint_val); + return true; + case ExpressionStepKind::kOtherConstant: + out = *step.u_.other_val; + return true; + default: + return false; + } +} + +bool IsConstant(const ExpressionStep& step) { + switch (step.header_.kind) { + case ExpressionStepKind::kIntConstant: + case ExpressionStepKind::kBoolConstant: + case ExpressionStepKind::kDoubleConstant: + case ExpressionStepKind::kNullConstant: + case ExpressionStepKind::kUintConstant: + case ExpressionStepKind::kOtherConstant: + return true; + default: + return false; + } +} + +ComprehensionCondStep* GetIfComprehensionCondStep(ExpressionStep& step) { + if (step.header_.kind == ExpressionStepKind::kComprehensionCond || + step.header_.kind == ExpressionStepKind::kComprehensionCond2) { + return &step.u_.cond_step; + } + return nullptr; +} + +ComprehensionNextStep* GetIfComprehensionNextStep(ExpressionStep& step) { + if (step.header_.kind == ExpressionStepKind::kComprehensionNext || + step.header_.kind == ExpressionStepKind::kComprehensionNext2) { + return &step.u_.next_step; + } + return nullptr; +} + +BoolJumpStepInfo* GetIfBoolJumpStep(ExpressionStep& step) { + if (step.header_.kind == ExpressionStepKind::kBooleanOrJump || + step.header_.kind == ExpressionStepKind::kBooleanAndJump) { + return &step.u_.bool_jump_step; + } + return nullptr; +} + +TernaryJumpStepInfo* GetIfTernaryJumpStep(ExpressionStep& step) { + if (step.header_.kind == ExpressionStepKind::kTernaryJump) { + return &step.u_.ternary_jump_step; + } + return nullptr; +} + +FixedJumpStepInfo* GetIfFixedJumpStep(ExpressionStep& step) { + if (step.header_.kind == ExpressionStepKind::kFixedJump) { + return &step.u_.fixed_jump_step; + } + return nullptr; +} + +void WrappedDirectStep::Evaluate(ExecutionFrame* frame) const { + cel::Value result; + AttributeTrail attribute_trail; + if (absl::Status status = impl_->Evaluate(*frame, result, attribute_trail); + !status.ok()) { + frame->Abort(std::move(status)); + return; + } + frame->value_stack().Push(std::move(result), std::move(attribute_trail)); +} + } // namespace google::api::expr::runtime diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index f25be8448..9cdda9e39 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -19,10 +19,10 @@ #include #include #include -#include #include #include +#include "absl/base/attributes.h" #include "absl/base/nullability.h" #include "absl/base/optimization.h" #include "absl/log/absl_check.h" @@ -34,12 +34,22 @@ #include "base/type_provider.h" #include "common/native_type.h" #include "common/value.h" +#include "common/value_kind.h" +#include "common/values/list_value_builder.h" #include "eval/eval/attribute_utility.h" #include "eval/eval/comprehension_slots.h" +#include "eval/eval/comprehension_step.h" +#include "eval/eval/direct_expression_step.h" +#include "eval/eval/equality_steps.h" #include "eval/eval/evaluator_stack.h" +#include "eval/eval/expression_step_logic.h" +#include "eval/eval/function_step.h" #include "eval/eval/iterator_stack.h" +#include "eval/eval/lazy_init_step.h" +#include "eval/eval/logic_step.h" #include "runtime/activation_interface.h" #include "runtime/internal/activation_attribute_matcher_access.h" +#include "runtime/internal/errors.h" #include "runtime/runtime.h" #include "runtime/runtime_options.h" #include "google/protobuf/arena.h" @@ -57,11 +67,67 @@ class ExecutionFrame; using EvaluationListener = cel::TraceableProgram::EvaluationListener; -class ExpressionStepLogic; - enum class ExpressionStepKind : uint16_t { kMovedFrom = 0, kGenericLogic = 1, + kIntConstant = 2, + kBoolConstant = 3, + kDoubleConstant = 4, + kNullConstant = 5, + kUintConstant = 6, + // Any constant that can't be inlined. + kOtherConstant = 7, + // Lazy subexpressions. + kLazyInit = 8, + kAssignSlotAndPop = 9, + kClearSlots = 10, + // Core boolean logic. + kBooleanNot = 11, + kNotStrictlyFalse = 12, + kBooleanOr = 13, + kBooleanAnd = 14, + // Comprehension steps. + // Init step doesn't fit inline and is slow anyway, so it uses a generic step. + kComprehensionFinish = 15, + kComprehensionNext = 16, + kComprehensionCond = 17, + kComprehensionNext2 = 18, + kComprehensionCond2 = 19, + kReadSlot = 20, + // Jump steps. + kBooleanOrJump = 21, + kBooleanAndJump = 22, + kTernaryJump = 23, + kFixedJump = 24, + // Functions calls. + kEagerFunction = 25, + kLazyFunction = 26, + // 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, + // Special built-in steps for mutable lists implementing map/filter. + kNewMutableList = 30, + kMutableListAppend = 31, +}; + +struct BoolJumpStepInfo { + size_t arg_count : 32; + bool set : 1; + int offset : 31; +}; + +struct TernaryJumpStepInfo { + bool set : 1; + int error_offset : 31; + int jump_to_second_offset : 32; +}; + +struct FixedJumpStepInfo { + bool set : 1; + int32_t reserved : 31; + int offset : 32; }; class ExpressionStep { @@ -72,6 +138,8 @@ class ExpressionStep { ExpressionStep(ExpressionStep&&); ExpressionStep& operator=(ExpressionStep&&); + ~ExpressionStep(); + // Returns corresponding expression object ID. // Requires that the input expression has IDs assigned to sub-expressions, // e.g. via a checker. The default value 0 is returned if there is no @@ -85,64 +153,276 @@ class ExpressionStep { // Returns if the execution step comes from AST. bool comes_from_ast() const { return header_.id >= 0; } - absl::Status Evaluate(ExecutionFrame* context) const; - - template - T Get() const; - template - bool Is() const; + // Evaluates this step on the given execution frame. + ABSL_ATTRIBUTE_ALWAYS_INLINE inline void Evaluate( + ExecutionFrame& frame) const; const ExpressionStepLogic* GetGenericStep() const; bool IsGenericStep() const; static ExpressionStep MakeGenericStep( std::unique_ptr logic, int64_t id = -1) { - if (id < 0 || id > std::numeric_limits::max()) { - id = -1; - } - return ExpressionStep(ExpressionStepKind::kGenericLogic, id, - std::move(logic)); + ExpressionStep step(ExpressionStepKind::kGenericLogic, id); + step.u_.logic = logic.release(); + return step; + } + + static ExpressionStep MakeConstant(const cel::Value& value, int64_t id = -1); + + static ExpressionStep MakeLazyInitStep(size_t slot_index, + size_t subexpression_index, + int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kLazyInit, id); + ABSL_DCHECK_LE(slot_index, std::numeric_limits::max()); + ABSL_DCHECK_LE(subexpression_index, std::numeric_limits::max()); + step.u_.lazy_init = LazyInitStepInfo{slot_index, subexpression_index}; + return step; + } + + static ExpressionStep MakeAssignSlotAndPopStep(size_t slot_index, + int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kAssignSlotAndPop, id); + ABSL_DCHECK_LE(slot_index, std::numeric_limits::max()); + step.u_.slot_index = slot_index; + return step; + } + + static ExpressionStep MakeClearSlotStep(size_t slot_index, int64_t id = -1) { + return MakeClearSlotsStep(slot_index, 1, id); + } + + static ExpressionStep MakeClearSlotsStep(size_t slot_index, size_t slot_count, + int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kClearSlots, id); + ABSL_DCHECK_LE(slot_index, std::numeric_limits::max()); + ABSL_DCHECK_LE(slot_count, std::numeric_limits::max()); + step.u_.clear_slots = ClearSlotStepInfo{slot_index, slot_count}; + return step; + } + + static ExpressionStep MakeBooleanNotStep(int64_t id = -1) { + return ExpressionStep(ExpressionStepKind::kBooleanNot, id); + } + + static ExpressionStep MakeNotStrictlyFalseStep(int64_t id = -1) { + return ExpressionStep(ExpressionStepKind::kNotStrictlyFalse, id); + } + + static ExpressionStep MakeBooleanOrStep(size_t num_args, int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kBooleanOr, id); + ABSL_DCHECK_LT(num_args, std::numeric_limits::max()); + step.u_.arg_count = num_args; + return step; + } + + static ExpressionStep MakeBooleanAndStep(size_t num_args, int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kBooleanAnd, id); + ABSL_DCHECK_LT(num_args, std::numeric_limits::max()); + step.u_.arg_count = num_args; + return step; + } + + static ExpressionStep MakeComprehensionFinishStep(size_t accu_slot, + int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kComprehensionFinish, id); + step.u_.slot_index = accu_slot; + return step; + } + + static ExpressionStep MakeComprehensionNextStep(int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kComprehensionNext, id); + step.u_.next_step = ComprehensionNextStep(); + return step; + } + + static ExpressionStep MakeComprehensionNext2Step(int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kComprehensionNext2, id); + step.u_.next_step = ComprehensionNextStep(); + return step; + } + + static ExpressionStep MakeComprehensionCondStep(int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kComprehensionCond, id); + step.u_.cond_step = ComprehensionCondStep(); + return step; + } + + static ExpressionStep MakeComprehensionCond2Step(int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kComprehensionCond2, id); + step.u_.cond_step = ComprehensionCondStep(); + return step; + } + + static ExpressionStep MakeReadSlotStep(size_t slot_index, int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kReadSlot, id); + ABSL_DCHECK_LT(slot_index, std::numeric_limits::max()); + step.u_.slot_index = slot_index; + return step; + } + + static ExpressionStep MakeBooleanOrJumpStep(size_t arg_count, + int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kBooleanOrJump, id); + ABSL_DCHECK_LT(arg_count, std::numeric_limits::max()); + step.u_.bool_jump_step = BoolJumpStepInfo{arg_count, false, 0}; + return step; + } + + static ExpressionStep MakeBooleanAndJumpStep(size_t arg_count, + int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kBooleanAndJump, id); + ABSL_DCHECK_LT(arg_count, std::numeric_limits::max()); + step.u_.bool_jump_step = BoolJumpStepInfo{arg_count, false, 0}; + return step; + } + + static ExpressionStep MakeTernaryJumpStep(int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kTernaryJump, id); + step.u_.ternary_jump_step = TernaryJumpStepInfo{false, 0, 0}; + return step; + } + + static ExpressionStep MakeFixedJumpStep(int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kFixedJump, id); + step.u_.fixed_jump_step = FixedJumpStepInfo{false, 0, 0}; + return step; + } + + static ExpressionStep MakeFastInStep(int64_t id = -1) { + return ExpressionStep(ExpressionStepKind::kFastIn, id); + } + + static ExpressionStep MakeFastEqualStep(int64_t id = -1) { + return ExpressionStep(ExpressionStepKind::kFastEqual, id); + } + + static ExpressionStep MakeFastNotEqualStep(int64_t id = -1) { + return ExpressionStep(ExpressionStepKind::kFastNotEqual, id); + } + + static ExpressionStep MakeNewMutableListStep(int64_t id = -1) { + return ExpressionStep(ExpressionStepKind::kNewMutableList, id); + } + + static ExpressionStep MakeMutableListAppendStep(int64_t id = -1) { + return ExpressionStep(ExpressionStepKind::kMutableListAppend, id); + } + + static ExpressionStep MakeEagerFunctionStep( + std::unique_ptr step_impl, int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kEagerFunction, id); + step.u_.eager_function_step = step_impl.release(); + return step; + } + + static ExpressionStep MakeLazyFunctionStep( + std::unique_ptr step_impl, int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kLazyFunction, id); + step.u_.lazy_function_step = step_impl.release(); + return step; } private: + static ABSL_ATTRIBUTE_ALWAYS_INLINE inline void EvaluateReadSlotStep( + size_t slot_index, ExecutionFrame& frame); + static ABSL_ATTRIBUTE_ALWAYS_INLINE inline void EvaluateBoolJumpStep( + const BoolJumpStepInfo& step, bool target, ExecutionFrame& frame); + static ABSL_ATTRIBUTE_ALWAYS_INLINE inline void EvaluateTernaryJumpStep( + const TernaryJumpStepInfo& step, ExecutionFrame& frame); + static void EvaluateMutableListAppendStep(ExecutionFrame& frame); + struct Header { ExpressionStepKind kind; uint16_t reserved; int32_t id; }; - friend void swap(ExpressionStep& lhs, ExpressionStep& rhs); - - ExpressionStep() = default; - + ExpressionStep() : header_{ExpressionStepKind::kMovedFrom, 0, -1} {} + ExpressionStep(ExpressionStepKind kind, int32_t id) : header_{kind, 0, id} { + header_ = {kind, 0, id}; + } + ExpressionStep(ExpressionStepKind kind, int64_t id) : ExpressionStep() { + if (id < 0 || id > std::numeric_limits::max()) { + id = -1; + } + header_ = {kind, 0, static_cast(id)}; + } ExpressionStep(ExpressionStepKind kind, int32_t id, std::unique_ptr logic) - : header_{kind, 0, id}, logic_(std::move(logic)) {} + : ExpressionStep(kind, id) { + u_.logic = logic.release(); + } + + friend void swap(ExpressionStep& lhs, ExpressionStep& rhs) { + using std::swap; + swap(lhs.header_, rhs.header_); + swap(lhs.u_, rhs.u_); + } + + friend bool GetIfConstant(const ExpressionStep& step, cel::Value& out); + friend bool IsConstant(const ExpressionStep& step); + friend ComprehensionCondStep* GetIfComprehensionCondStep( + ExpressionStep& step); + friend ComprehensionNextStep* GetIfComprehensionNextStep( + ExpressionStep& step); + friend BoolJumpStepInfo* GetIfBoolJumpStep(ExpressionStep& step); + friend TernaryJumpStepInfo* GetIfTernaryJumpStep(ExpressionStep& step); + friend FixedJumpStepInfo* GetIfFixedJumpStep(ExpressionStep& step); Header header_; - std::unique_ptr logic_; + // Note: ptr members are 'owned' by the step. + // ~ExpressionStep() is responsible for freeing. + union Data { + std::nullptr_t empty; + ExpressionStepLogic* logic; + int64_t int_val; + uint64_t uint_val; + double double_val; + bool bool_val; + cel::Value* other_val; + LazyInitStepInfo lazy_init; + size_t slot_index; + ClearSlotStepInfo clear_slots; + size_t arg_count; + ComprehensionCondStep cond_step; + ComprehensionNextStep next_step; + BoolJumpStepInfo bool_jump_step; + TernaryJumpStepInfo ternary_jump_step; + FixedJumpStepInfo fixed_jump_step; + EagerFunctionStep* eager_function_step; + LazyFunctionStep* lazy_function_step; + + Data() : empty(nullptr) {} + ~Data() {} + } u_; }; -// Class Expression represents single execution step. -class ExpressionStepLogic { +#ifndef _MSC_VER +// Keep the core instruction size small to get better memory locality for the +// main program. +// +// MSVC does not support some of the bit-field packing used here so it will be +// larger. +static_assert(sizeof(ExpressionStep) == 16); +#endif + +// Wrapper for direct steps to work with the stack machine impl. +class WrappedDirectStep : public ExpressionStepLogic { public: - virtual ~ExpressionStepLogic() = default; - - // Performs actual evaluation. - // Values are passed between Expression objects via EvaluatorStack, which is - // supplied with context. - // Also, Expression gets values supplied by caller though Activation - // interface. - // ExpressionStep instances can in specific cases - // modify execution order(perform jumps). - virtual absl::Status Evaluate(ExecutionFrame* context) const = 0; - - // Return the type of the underlying expression step for special handling in - // the planning phase. This should only be overridden by special cases, and - // callers must not make any assumptions about the default case. - virtual cel::NativeTypeId GetNativeTypeId() const { - return cel::NativeTypeId(); + explicit WrappedDirectStep(std::unique_ptr impl, + int64_t expr_id = -1) + : impl_(std::move(impl)) {} + + void Evaluate(ExecutionFrame* frame) const override; + + cel::NativeTypeId GetNativeTypeId() const override { + return cel::NativeTypeId::For(); } + + const DirectExpressionStep* wrapped() const { return impl_.get(); } + + private: + std::unique_ptr impl_; }; using ExecutionPath = std::vector; @@ -346,6 +626,8 @@ class ExecutionFrameBase { return absl::OkStatus(); } + absl::Status& abort_status() { return abort_status_; } + protected: const cel::ActivationInterface* absl_nonnull activation_; EvaluationListener callback_; @@ -359,6 +641,7 @@ class ExecutionFrameBase { ComprehensionSlots* absl_nonnull slots_; const int max_iterations_; int iterations_; + absl::Status abort_status_; const bool attribute_tracking_enabled_; const bool missing_attribute_errors_enabled_; const bool unknown_processing_enabled_; @@ -387,7 +670,7 @@ class ExecutionFrame : public ExecutionFrameBase { execution_path_(flat), value_stack_(&state.value_stack()), iterator_stack_(&state.iterator_stack()), - subexpressions_() {} + subexpressions_(&execution_path_, 1) {} ExecutionFrame( absl::Span subexpressions, @@ -408,7 +691,14 @@ class ExecutionFrame : public ExecutionFrameBase { } // Returns next expression to evaluate. - const ExpressionStep* Next(); + ABSL_ATTRIBUTE_ALWAYS_INLINE const ExpressionStep* Next() { + if (ABSL_PREDICT_TRUE(pc_ < execution_path_.size())) { + const ExpressionStep* step = &execution_path_[pc_++]; + ABSL_ASSUME(step != nullptr); + return step; + } + return NextSlow(); + } // Evaluate the execution frame to completion. absl::StatusOr Evaluate(EvaluationListener& listener); @@ -434,6 +724,19 @@ class ExecutionFrame : public ExecutionFrameBase { return absl::OkStatus(); } + ABSL_ATTRIBUTE_ALWAYS_INLINE void JumpToOrAbort(int offset) { + ABSL_DCHECK_LE(offset, static_cast(execution_path_.size())); + ABSL_DCHECK_GE(offset, -static_cast(pc_)); + + int new_pc = static_cast(pc_) + offset; + if (ABSL_PREDICT_FALSE(new_pc < 0 || + new_pc > static_cast(execution_path_.size()))) { + AbortJumpOutOfRange(offset); + return; + } + pc_ = static_cast(new_pc); + } + // Move pc to a subexpression. // // Unlike a `Call` in a programming language, the subexpression is evaluated @@ -487,6 +790,15 @@ class ExecutionFrame : public ExecutionFrameBase { return *activation_; } + void Abort(absl::Status status) { + ABSL_DCHECK(!subexpressions_.empty()); + ABSL_DCHECK(!status.ok()); + abort_status_.Update(std::move(status)); + call_stack_.clear(); + execution_path_ = subexpressions_[0]; + pc_ = execution_path_.size(); + } + private: struct SubFrame { size_t return_pc; @@ -495,6 +807,21 @@ class ExecutionFrame : public ExecutionFrameBase { size_t expected_stack_size; }; + // Runs steps until the program completes or is aborted. + void Interpret(); + + // Same as Interpret(), but also calls `listener` with the result of each + // step that maps to an AST node. + void InterpretWithCallback(EvaluationListener& listener); + + // Slow path of Next(): returns from a completed subexpression, or returns + // nullptr at the end of the program. + ABSL_ATTRIBUTE_NOINLINE const ExpressionStep* NextSlow(); + + // Error path of JumpToOrAbort(). + ABSL_ATTRIBUTE_NOINLINE ABSL_ATTRIBUTE_COLD void AbortJumpOutOfRange( + int offset); + size_t pc_; // pc_ - Program Counter. Current position on execution path. ExecutionPathView execution_path_; EvaluatorStack* absl_nonnull const value_stack_; @@ -580,36 +907,98 @@ class FlatExpression { absl_nullable std::shared_ptr arena_; }; -// Implementation details. +// Helper functions for checking ExpressionStep kinds. Used for program +// optimization. -inline absl::Status ExpressionStep::Evaluate(ExecutionFrame* context) const { - ABSL_DCHECK_EQ(header_.kind, ExpressionStepKind::kGenericLogic); - return logic_->Evaluate(context); +// Checks if the step is a constant and if so, writes the value into `out`. +// Returns true if the step is a constant, false otherwise. +bool GetIfConstant(const ExpressionStep& step, cel::Value& out); + +// Checks if the step is a constant. +bool IsConstant(const ExpressionStep& step); + +ComprehensionCondStep* GetIfComprehensionCondStep(ExpressionStep& step); +ComprehensionNextStep* GetIfComprehensionNextStep(ExpressionStep& step); + +BoolJumpStepInfo* GetIfBoolJumpStep(ExpressionStep& step); +inline BoolJumpStepInfo* GetIfBoolJumpStep(ExpressionStep* step) { + if (step == nullptr) { + return nullptr; + } + return GetIfBoolJumpStep(*step); } -template -inline T ExpressionStep::Get() const { - if constexpr (std::is_same_v) { - return GetGenericStep(); - } else { - static_assert(sizeof(T) == 0, "unsupported type"); +TernaryJumpStepInfo* GetIfTernaryJumpStep(ExpressionStep& step); +inline TernaryJumpStepInfo* GetIfTernaryJumpStep(ExpressionStep* step) { + if (step == nullptr) { + return nullptr; } - ABSL_UNREACHABLE(); + return GetIfTernaryJumpStep(*step); } -inline const ExpressionStepLogic* ExpressionStep::GetGenericStep() const { - ABSL_DCHECK_EQ(header_.kind, ExpressionStepKind::kGenericLogic); - return logic_.get(); +FixedJumpStepInfo* GetIfFixedJumpStep(ExpressionStep& step); +inline FixedJumpStepInfo* GetIfFixedJumpStep(ExpressionStep* step) { + if (step == nullptr) { + return nullptr; + } + return GetIfFixedJumpStep(*step); } -template -inline bool ExpressionStep::Is() const { - if constexpr (std::is_same_v) { - return header_.kind == ExpressionStepKind::kGenericLogic; - } else { - static_assert(sizeof(T) == 0, "unsupported type"); +// Implementation details. + +inline ExpressionStep::~ExpressionStep() { + switch (header_.kind) { + case ExpressionStepKind::kGenericLogic: + delete u_.logic; + break; + case ExpressionStepKind::kOtherConstant: + delete u_.other_val; + break; + case ExpressionStepKind::kEagerFunction: + delete u_.eager_function_step; + break; + case ExpressionStepKind::kLazyFunction: + delete u_.lazy_function_step; + break; + case ExpressionStepKind::kMovedFrom: + case ExpressionStepKind::kIntConstant: + case ExpressionStepKind::kBoolConstant: + case ExpressionStepKind::kDoubleConstant: + case ExpressionStepKind::kNullConstant: + case ExpressionStepKind::kUintConstant: + case ExpressionStepKind::kLazyInit: + case ExpressionStepKind::kAssignSlotAndPop: + case ExpressionStepKind::kClearSlots: + case ExpressionStepKind::kBooleanNot: + case ExpressionStepKind::kNotStrictlyFalse: + case ExpressionStepKind::kBooleanOr: + case ExpressionStepKind::kBooleanAnd: + case ExpressionStepKind::kComprehensionFinish: + case ExpressionStepKind::kComprehensionCond: + case ExpressionStepKind::kComprehensionNext: + case ExpressionStepKind::kComprehensionCond2: + case ExpressionStepKind::kComprehensionNext2: + case ExpressionStepKind::kReadSlot: + case ExpressionStepKind::kBooleanOrJump: + case ExpressionStepKind::kBooleanAndJump: + case ExpressionStepKind::kTernaryJump: + case ExpressionStepKind::kFixedJump: + case ExpressionStepKind::kFastIn: + case ExpressionStepKind::kFastEqual: + case ExpressionStepKind::kFastNotEqual: + case ExpressionStepKind::kNewMutableList: + case ExpressionStepKind::kMutableListAppend: + break; + default: + ABSL_UNREACHABLE(); } - ABSL_UNREACHABLE(); + header_.kind = ExpressionStepKind::kMovedFrom; + u_.empty = nullptr; +} + +inline const ExpressionStepLogic* ExpressionStep::GetGenericStep() const { + ABSL_DCHECK_EQ(header_.kind, ExpressionStepKind::kGenericLogic); + return u_.logic; } inline bool ExpressionStep::IsGenericStep() const { @@ -619,20 +1008,181 @@ inline bool ExpressionStep::IsGenericStep() const { inline ExpressionStep::ExpressionStep(ExpressionStep&& other) : ExpressionStep() { using std::swap; - swap(*this, other); + swap(other, *this); } inline ExpressionStep& ExpressionStep::operator=(ExpressionStep&& other) { using std::swap; - swap(*this, other); + ExpressionStep tmp; + swap(*this, tmp); + swap(other, *this); return *this; } -inline void swap(ExpressionStep& lhs, ExpressionStep& rhs) { - using std::swap; +inline void ExpressionStep::EvaluateReadSlotStep(size_t slot_index, + ExecutionFrame& frame) { + const ComprehensionSlots::Slot* slot = + frame.comprehension_slots().Get(slot_index); + if (!slot->Has()) { + frame.Abort(absl::InternalError(absl::StrCat( + "Comprehension variable read out of scope: ", slot_index))); + return; + } + frame.value_stack().Push(slot->value(), slot->attribute()); +} + +inline void ExpressionStep::EvaluateBoolJumpStep(const BoolJumpStepInfo& step, + bool target, + ExecutionFrame& frame) { + ABSL_DCHECK(step.set) << "BoolJumpStep did not have a value set."; + if (!frame.value_stack().HasEnough(step.arg_count)) { + frame.Abort( + absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + return; + } + const cel::Value& value = frame.value_stack().Peek(); + if (value.IsBool() && value.GetBool().NativeValue() == target) { + frame.value_stack().SwapAndPop(step.arg_count, step.arg_count - 1); + frame.JumpToOrAbort(step.offset); + } + // No-op if the value is not a bool or the value is not the target. + // Cleanup will happen if we hit a later jump or we fall-through. +} + +inline void ExpressionStep::EvaluateTernaryJumpStep( + const TernaryJumpStepInfo& step, ExecutionFrame& frame) { + ABSL_DCHECK(step.set) << "TernaryJumpStep did not have a value set."; + if (!frame.value_stack().HasEnough(1)) { + frame.Abort(absl::InternalError("TernaryJumpStep: value stack underflow")); + return; + } + const cel::Value& condition = frame.value_stack().Peek(); + switch (condition.kind()) { + case cel::ValueKind::kBool: + if (!condition.GetBool().NativeValue()) { + frame.JumpToOrAbort(step.jump_to_second_offset); + } + frame.value_stack().Pop(); + break; + default: + frame.value_stack().PopAndPush( + cel::ErrorValue(cel::runtime_internal::CreateNoMatchingOverloadError( + ""))); + ABSL_FALLTHROUGH_INTENDED; + case cel::ValueKind::kError: + case cel::ValueKind::kUnknown: + // Propagate the error or unknown set. + frame.JumpToOrAbort(step.error_offset); + break; + } +} - swap(lhs.header_, rhs.header_); - swap(lhs.logic_, rhs.logic_); +inline void ExpressionStep::Evaluate(ExecutionFrame& frame) const { + switch (header_.kind) { + case ExpressionStepKind::kGenericLogic: + u_.logic->Evaluate(&frame); + break; + case ExpressionStepKind::kIntConstant: + frame.value_stack().Push(cel::IntValue(u_.int_val)); + break; + case ExpressionStepKind::kBoolConstant: + frame.value_stack().Push(cel::BoolValue(u_.bool_val)); + break; + case ExpressionStepKind::kDoubleConstant: + frame.value_stack().Push(cel::DoubleValue(u_.double_val)); + break; + case ExpressionStepKind::kNullConstant: + frame.value_stack().Push(cel::NullValue()); + break; + case ExpressionStepKind::kUintConstant: + frame.value_stack().Push(cel::UintValue(u_.uint_val)); + break; + case ExpressionStepKind::kOtherConstant: + frame.value_stack().Push(*u_.other_val); + break; + case ExpressionStepKind::kLazyInit: + EvaluateLazyInitStep(u_.lazy_init, frame); + break; + case ExpressionStepKind::kAssignSlotAndPop: + EvaluateAssignSlotAndPop(u_.slot_index, frame); + break; + case ExpressionStepKind::kClearSlots: + EvaluateClearSlotStep(u_.clear_slots, frame); + break; + case ExpressionStepKind::kBooleanNot: + EvaluateNotStep(frame); + break; + case ExpressionStepKind::kNotStrictlyFalse: + EvaluateNotStrictlyFalseStep(frame); + break; + case ExpressionStepKind::kBooleanOr: + EvaluateBoolLogicStep(BoolLogicKind::kOr, u_.arg_count, frame); + break; + case ExpressionStepKind::kBooleanAnd: + EvaluateBoolLogicStep(BoolLogicKind::kAnd, u_.arg_count, frame); + break; + case ExpressionStepKind::kComprehensionFinish: + EvaluateComprehensionFinishStep(u_.slot_index, frame); + break; + case ExpressionStepKind::kComprehensionNext: + u_.next_step.Evaluate1(&frame); + break; + case ExpressionStepKind::kComprehensionNext2: + u_.next_step.Evaluate2(&frame); + break; + case ExpressionStepKind::kComprehensionCond: + u_.cond_step.Evaluate1(&frame); + break; + case ExpressionStepKind::kComprehensionCond2: + u_.cond_step.Evaluate2(&frame); + break; + case ExpressionStepKind::kReadSlot: + EvaluateReadSlotStep(u_.slot_index, frame); + break; + case ExpressionStepKind::kBooleanOrJump: + EvaluateBoolJumpStep(u_.bool_jump_step, true, frame); + break; + case ExpressionStepKind::kBooleanAndJump: + EvaluateBoolJumpStep(u_.bool_jump_step, false, frame); + break; + case ExpressionStepKind::kTernaryJump: + EvaluateTernaryJumpStep(u_.ternary_jump_step, frame); + break; + case ExpressionStepKind::kFixedJump: + ABSL_DCHECK(u_.fixed_jump_step.set) + << "FixedJumpStep did not have a value set."; + frame.JumpToOrAbort(u_.fixed_jump_step.offset); + break; + case ExpressionStepKind::kEagerFunction: + u_.eager_function_step->Evaluate(frame); + break; + case ExpressionStepKind::kLazyFunction: + u_.lazy_function_step->Evaluate(frame); + break; + case ExpressionStepKind::kFastIn: + EvaluateFastInStep(frame); + break; + case ExpressionStepKind::kFastEqual: + EvaluateFastEqualStep(/*negation=*/false, frame); + break; + case ExpressionStepKind::kFastNotEqual: + EvaluateFastEqualStep(/*negation=*/true, frame); + break; + case ExpressionStepKind::kNewMutableList: + frame.value_stack().Push(cel::CustomListValue( + cel::common_internal::NewMutableListValue(frame.arena()), + frame.arena())); + break; + case ExpressionStepKind::kMutableListAppend: + EvaluateMutableListAppendStep(frame); + break; + case ExpressionStepKind::kMovedFrom: + frame.Abort(absl::InternalError( + "ExpressionStep::Evaluate called on moved-from step")); + break; + default: + ABSL_UNREACHABLE(); + } } } // namespace google::api::expr::runtime diff --git a/eval/eval/evaluator_core_test.cc b/eval/eval/evaluator_core_test.cc index 873bc7365..c67dc8b58 100644 --- a/eval/eval/evaluator_core_test.cc +++ b/eval/eval/evaluator_core_test.cc @@ -28,6 +28,7 @@ namespace google::api::expr::runtime { using ::absl_testing::IsOk; +using ::absl_testing::StatusIs; using ::cel::IntValue; using ::cel::TypeProvider; using ::cel::interop_internal::CreateIntValue; @@ -37,6 +38,7 @@ using ::google::api::expr::runtime::RegisterBuiltinFunctions; using ::testing::_; using ::testing::ElementsAre; using ::testing::Eq; +using ::testing::HasSubstr; // Fake expression implementation // Pushes int64(0) on top of value stack. @@ -44,9 +46,8 @@ class FakeConstExpressionStep : public ExpressionStepLogic { public: FakeConstExpressionStep() = default; - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Push(CreateIntValue(0)); - return absl::OkStatus(); } }; @@ -56,13 +57,12 @@ class FakeIncrementExpressionStep : public ExpressionStepLogic { public: FakeIncrementExpressionStep() = default; - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { auto value = frame->value_stack().Peek(); frame->value_stack().Pop(1); EXPECT_TRUE(value->Is()); int64_t val = value.GetInt().NativeValue(); frame->value_stack().Push(CreateIntValue(val + 1)); - return absl::OkStatus(); } }; @@ -95,6 +95,88 @@ TEST(EvaluatorCoreTest, ExecutionFrameNext) { EXPECT_THAT(frame.Next(), Eq(nullptr)); } +TEST(EvaluatorCoreTest, JumpOutOfRangeAborts) { + ExecutionPath path; + google::protobuf::Arena arena; + cel::runtime_internal::RuntimeTypeProvider type_provider( + cel::internal::GetTestingDescriptorPool()); + + path.push_back(ExpressionStep::MakeFixedJumpStep()); + FixedJumpStepInfo* jump_info = GetIfFixedJumpStep(path.back()); + ASSERT_NE(jump_info, nullptr); + jump_info->set = true; + jump_info->offset = 1; + + cel::RuntimeOptions options; + cel::Activation activation; + FlatExpressionEvaluatorState state( + path.size(), + /*comprehension_slots_size=*/0, type_provider, + cel::internal::GetTestingDescriptorPool(), + cel::internal::GetTestingMessageFactory(), &arena); + ExecutionFrame frame(path, activation, options, state); + + EXPECT_THAT(frame.Evaluate(), + StatusIs(absl::StatusCode::kInternal, + HasSubstr("Jump address out of range"))); +} + +TEST(EvaluatorCoreTest, TestStepDataAccessors) { + ExpressionStep fixed_jump = ExpressionStep::MakeFixedJumpStep(); + EXPECT_NE(GetIfFixedJumpStep(fixed_jump), nullptr); + EXPECT_NE(GetIfFixedJumpStep(&fixed_jump), nullptr); + EXPECT_EQ(GetIfFixedJumpStep(static_cast(nullptr)), nullptr); + + ExpressionStep bool_or_jump = ExpressionStep::MakeBooleanOrJumpStep(2); + EXPECT_NE(GetIfBoolJumpStep(bool_or_jump), nullptr); + EXPECT_NE(GetIfBoolJumpStep(&bool_or_jump), nullptr); + EXPECT_EQ(GetIfBoolJumpStep(static_cast(nullptr)), nullptr); + + ExpressionStep bool_and_jump = ExpressionStep::MakeBooleanAndJumpStep(2); + EXPECT_NE(GetIfBoolJumpStep(bool_and_jump), nullptr); + EXPECT_NE(GetIfBoolJumpStep(&bool_and_jump), nullptr); + + ExpressionStep ternary_jump = ExpressionStep::MakeTernaryJumpStep(); + EXPECT_NE(GetIfTernaryJumpStep(ternary_jump), nullptr); + EXPECT_NE(GetIfTernaryJumpStep(&ternary_jump), nullptr); + EXPECT_EQ(GetIfTernaryJumpStep(static_cast(nullptr)), + nullptr); + + ExpressionStep comp_cond = ExpressionStep::MakeComprehensionCondStep(); + EXPECT_NE(GetIfComprehensionCondStep(comp_cond), nullptr); + + ExpressionStep comp_cond2 = ExpressionStep::MakeComprehensionCond2Step(); + EXPECT_NE(GetIfComprehensionCondStep(comp_cond2), nullptr); + + ExpressionStep comp_next = ExpressionStep::MakeComprehensionNextStep(); + EXPECT_NE(GetIfComprehensionNextStep(comp_next), nullptr); + + ExpressionStep comp_next2 = ExpressionStep::MakeComprehensionNext2Step(); + EXPECT_NE(GetIfComprehensionNextStep(comp_next2), nullptr); + + ExpressionStep const_step = ExpressionStep::MakeConstant(cel::IntValue(42)); + EXPECT_TRUE(IsConstant(const_step)); + cel::Value value; + EXPECT_TRUE(GetIfConstant(const_step, value)); + EXPECT_TRUE(value.IsInt()); + EXPECT_EQ(value.GetInt().NativeValue(), 42); + + ExpressionStep generic_step = ExpressionStep::MakeGenericStep( + std::make_unique()); + EXPECT_TRUE(generic_step.IsGenericStep()); + EXPECT_NE(generic_step.GetGenericStep(), nullptr); + + // Negative checks for mismatched step kinds. + EXPECT_EQ(GetIfFixedJumpStep(bool_or_jump), nullptr); + EXPECT_EQ(GetIfBoolJumpStep(fixed_jump), nullptr); + EXPECT_EQ(GetIfTernaryJumpStep(fixed_jump), nullptr); + EXPECT_EQ(GetIfComprehensionCondStep(fixed_jump), nullptr); + EXPECT_EQ(GetIfComprehensionNextStep(fixed_jump), nullptr); + EXPECT_FALSE(IsConstant(fixed_jump)); + EXPECT_FALSE(GetIfConstant(fixed_jump, value)); + EXPECT_FALSE(fixed_jump.IsGenericStep()); +} + TEST(EvaluatorCoreTest, SimpleEvaluatorTest) { ExecutionPath path; path.push_back(ExpressionStep::MakeGenericStep( @@ -121,6 +203,54 @@ TEST(EvaluatorCoreTest, SimpleEvaluatorTest) { EXPECT_THAT(value.Int64OrDie(), Eq(2)); } +TEST(EvaluatorCoreTest, MakeConstant) { + cel::runtime_internal::RuntimeTypeProvider type_provider( + cel::internal::GetTestingDescriptorPool()); + google::protobuf::Arena arena; + cel::Activation activation; + cel::RuntimeOptions options; + + auto evaluate_constant = + [&](const cel::Value& value) -> absl::StatusOr { + ExecutionPath path; + path.push_back(ExpressionStep::MakeConstant(value)); + FlatExpression expr(std::move(path), 0, type_provider, options); + auto state = expr.MakeEvaluatorState( + cel::internal::GetTestingDescriptorPool(), + cel::internal::GetTestingMessageFactory(), &arena); + return expr.EvaluateWithCallback(activation, nullptr, nullptr, state); + }; + + ASSERT_OK_AND_ASSIGN(auto bool_val, evaluate_constant(cel::BoolValue(true))); + EXPECT_TRUE(bool_val.IsBool()); + EXPECT_TRUE(bool_val.GetBool().NativeValue()); + + ASSERT_OK_AND_ASSIGN(auto int_val, evaluate_constant(cel::IntValue(42))); + EXPECT_TRUE(int_val.IsInt()); + EXPECT_EQ(int_val.GetInt().NativeValue(), 42); + + ASSERT_OK_AND_ASSIGN(auto uint_val, evaluate_constant(cel::UintValue(100))); + EXPECT_TRUE(uint_val.IsUint()); + EXPECT_EQ(uint_val.GetUint().NativeValue(), 100); + + ASSERT_OK_AND_ASSIGN(auto double_val, + evaluate_constant(cel::DoubleValue(3.14))); + EXPECT_TRUE(double_val.IsDouble()); + EXPECT_DOUBLE_EQ(double_val.GetDouble().NativeValue(), 3.14); + + ASSERT_OK_AND_ASSIGN(auto null_val, evaluate_constant(cel::NullValue())); + EXPECT_TRUE(null_val.IsNull()); + + ASSERT_OK_AND_ASSIGN(auto str_val, + evaluate_constant(cel::StringValue::Literal("hello"))); + EXPECT_TRUE(str_val.IsString()); + EXPECT_EQ(str_val.GetString().ToString(), "hello"); + + auto step_with_id = ExpressionStep::MakeConstant(cel::IntValue(1), 123); + EXPECT_EQ(step_with_id.id(), 123); + EXPECT_TRUE(step_with_id.comes_from_ast()); +} + class MockTraceCallback { public: MOCK_METHOD(void, Call, diff --git a/eval/eval/evaluator_stack.h b/eval/eval/evaluator_stack.h index b6abd1f76..e318f00ac 100644 --- a/eval/eval/evaluator_stack.h +++ b/eval/eval/evaluator_stack.h @@ -278,9 +278,9 @@ class EvaluatorStack { PopAndPush(n, std::forward(value), absl::nullopt); } - // Swaps the `n - i` element (from the top of the stack) with the `n` element, - // and pops `n - 1` elements. This results in the `n - i` element being at the - // top of the stack. + // Given the top `n` the elements of the stack, swap the `i`th element with + // the 0th, then pop n - 1 elements (leaving the `i`th element as the new + // top). void SwapAndPop(size_t n, size_t i) { ABSL_DCHECK_GT(n, 0); ABSL_DCHECK_LT(i, n); diff --git a/eval/eval/expression_step_base.h b/eval/eval/expression_step_base.h index 565dc5fa5..7f4425098 100644 --- a/eval/eval/expression_step_base.h +++ b/eval/eval/expression_step_base.h @@ -3,7 +3,7 @@ #include -#include "eval/eval/evaluator_core.h" +#include "eval/eval/expression_step_logic.h" namespace google::api::expr::runtime { diff --git a/eval/eval/expression_step_logic.h b/eval/eval/expression_step_logic.h new file mode 100644 index 000000000..2b07af527 --- /dev/null +++ b/eval/eval/expression_step_logic.h @@ -0,0 +1,51 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_EXPRESSION_STEP_LOGIC_H_ +#define THIRD_PARTY_CEL_CPP_EVAL_EVAL_EXPRESSION_STEP_LOGIC_H_ + +#include "common/native_type.h" + +namespace google::api::expr::runtime { + +class ExecutionFrame; + +// ExpressionStepLogic is the base class for generic expression steps that are +// not implemented directly in the evaluator core. +class ExpressionStepLogic { + public: + virtual ~ExpressionStepLogic() = default; + + // Performs actual evaluation. + // Values are passed between Expression objects via EvaluatorStack, which is + // supplied with context. + // Also, Expression gets values supplied by caller though Activation + // interface. + // ExpressionStep instances can in specific cases + // modify execution order(perform jumps). + // + // Unrecoverable errors are reported via ExecutionFrame::Abort. + virtual void Evaluate(ExecutionFrame* frame) const = 0; + + // Return the type of the underlying expression step for special handling in + // the planning phase. This should only be overridden by special cases, and + // callers must not make any assumptions about the default case. + virtual cel::NativeTypeId GetNativeTypeId() const { + return cel::NativeTypeId(); + } +}; + +} // namespace google::api::expr::runtime + +#endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_EXPRESSION_STEP_LOGIC_H_ diff --git a/eval/eval/function_step.cc b/eval/eval/function_step.cc index ca63392c7..fe06b8eca 100644 --- a/eval/eval/function_step.cc +++ b/eval/eval/function_step.cc @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -24,7 +25,7 @@ #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 "eval/eval/expression_step_logic.h" #include "eval/internal/errors.h" #include "internal/status_macros.h" #include "runtime/activation_interface.h" @@ -146,40 +147,6 @@ bool IsUnknownFunctionResultError(const Value& result) { // resolve to a single function implementation and a descriptor or none. using ResolveResult = absl::optional; -// Implementation of ExpressionStep that finds suitable CelFunction overload and -// invokes it. Abstract base class standardizes behavior between lazy and eager -// function bindings. Derived classes provide ResolveFunction behavior. -class AbstractFunctionStep : public ExpressionStepBase { - public: - // Constructs FunctionStep that uses overloads specified. - AbstractFunctionStep(const std::string& name, size_t num_arguments, - bool receiver_style, int64_t expr_id) - : ExpressionStepBase(expr_id), - name_(name), - num_arguments_(num_arguments), - receiver_style_(receiver_style), - expr_id_(expr_id) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override; - - // Handles overload resolution and updating result appropriately. - // Shouldn't update frame state. - // - // A non-ok result is an unrecoverable error, either from an illegal - // evaluation state or forwarded from an extension function. Errors where - // evaluation can reasonably condition are returned in the result as a - // cel::ErrorValue. - absl::StatusOr DoEvaluate(ExecutionFrame* frame) const; - - virtual absl::StatusOr ResolveFunction( - absl::Span args, const ExecutionFrame* frame) const = 0; - - protected: - std::string name_; - size_t num_arguments_; - bool receiver_style_; - int64_t expr_id_; -}; inline absl::StatusOr Invoke( const cel::FunctionOverloadReference& overload, int64_t expr_id, @@ -248,49 +215,7 @@ Value NoOverloadResult(absl::string_view name, frame.arena()); } -absl::StatusOr AbstractFunctionStep::DoEvaluate( - ExecutionFrame* frame) const { - // Create Span object that contains input arguments to the function. - auto input_args = frame->value_stack().GetSpan(num_arguments_); - - std::vector unknowns_args; - // Preprocess args. If an argument is partially unknown, convert it to an - // unknown attribute set. - if (frame->enable_unknowns()) { - auto input_attrs = frame->value_stack().GetAttributeSpan(num_arguments_); - unknowns_args = CheckForPartialUnknowns(frame, input_args, input_attrs); - input_args = absl::MakeConstSpan(unknowns_args); - } - - // Derived class resolves to a single function overload or none. - CEL_ASSIGN_OR_RETURN(ResolveResult matched_function, - ResolveFunction(input_args, frame)); - - // Overload found and is allowed to consume the arguments. - if (matched_function.has_value() && - ShouldAcceptOverload(matched_function->descriptor, input_args)) { - return Invoke(*matched_function, expr_id_, input_args, *frame); - } - - return NoOverloadResult(name_, input_args, receiver_style_, *frame); -} - -absl::Status AbstractFunctionStep::Evaluate(ExecutionFrame* frame) const { - if (!frame->value_stack().HasEnough(num_arguments_)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); - } - - // DoEvaluate may return a status for non-recoverable errors (e.g. - // unexpected typing, illegal expression state). Application errors that can - // reasonably be handled as a cel error will appear in the result value. - CEL_ASSIGN_OR_RETURN(auto result, DoEvaluate(frame)); - - frame->value_stack().PopAndPush(num_arguments_, std::move(result)); - - return absl::OkStatus(); -} - -absl::StatusOr ResolveStatic( +ResolveResult ResolveStatic( absl::Span input_args, absl::Span overloads) { for (const auto& overload : overloads) { @@ -341,48 +266,7 @@ absl::StatusOr ResolveLazy( return result; } -class EagerFunctionStep : public AbstractFunctionStep { - public: - EagerFunctionStep(std::vector overloads, - const std::string& name, size_t num_args, - bool receiver_style, int64_t expr_id) - : AbstractFunctionStep(name, num_args, receiver_style, expr_id), - overloads_(std::move(overloads)) {} - - absl::StatusOr ResolveFunction( - absl::Span input_args, - const ExecutionFrame* frame) const override { - return ResolveStatic(input_args, overloads_); - } - - private: - std::vector overloads_; -}; - -class LazyFunctionStep : public AbstractFunctionStep { - public: - // Constructs LazyFunctionStep that attempts to lookup function implementation - // at runtime. - LazyFunctionStep(const std::string& name, size_t num_args, - bool receiver_style, - std::vector providers, - int64_t expr_id) - : AbstractFunctionStep(name, num_args, receiver_style, expr_id), - providers_(std::move(providers)) {} - - absl::StatusOr ResolveFunction( - absl::Span input_args, - const ExecutionFrame* frame) const override; - - private: - std::vector providers_; -}; -absl::StatusOr LazyFunctionStep::ResolveFunction( - absl::Span input_args, - const ExecutionFrame* frame) const { - return ResolveLazy(input_args, name_, receiver_style_, providers_, *frame); -} class StaticResolver { public: @@ -495,6 +379,74 @@ class DirectFunctionStepImpl : public DirectExpressionStep { } // namespace +template +void EvaluateFunctionStep(const Step* step, ExecutionFrame& frame) { + if (!frame.value_stack().HasEnough(step->num_arguments_)) { + frame.Abort( + absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + return; + } + + // Create Span object that contains input arguments to the function. + absl::Span input_args = + frame.value_stack().GetSpan(step->num_arguments_); + + std::vector unknowns_args; + // Preprocess args. If an argument is partially unknown, convert it to an + // unknown attribute set. + if (frame.enable_unknowns()) { + absl::Span input_attrs = + frame.value_stack().GetAttributeSpan(step->num_arguments_); + unknowns_args = CheckForPartialUnknowns(&frame, input_args, input_attrs); + input_args = absl::MakeConstSpan(unknowns_args); + } + + // Derived class resolves to a single function overload or none. + absl::StatusOr matched_function = + step->ResolveFunction(input_args, frame); + if (!matched_function.ok()) { + frame.Abort(std::move(matched_function).status()); + return; + } + + // Overload found and is allowed to consume the arguments. + if (matched_function->has_value() && + ShouldAcceptOverload((*matched_function)->descriptor, input_args)) { + absl::StatusOr result = + Invoke(**matched_function, step->expr_id_, input_args, frame); + if (!result.ok()) { + frame.Abort(std::move(result).status()); + return; + } + frame.value_stack().PopAndPush(step->num_arguments_, *std::move(result)); + return; + } + + frame.value_stack().PopAndPush( + step->num_arguments_, + NoOverloadResult(step->name_, input_args, step->receiver_style_, frame)); +} + +void EagerFunctionStep::Evaluate(ExecutionFrame& frame) const { + EvaluateFunctionStep(this, frame); +} + +void LazyFunctionStep::Evaluate(ExecutionFrame& frame) const { + EvaluateFunctionStep(this, frame); +} + +ResolveResult EagerFunctionStep::ResolveFunction( + absl::Span input_args, + const ExecutionFrameBase& frame) const { + return ResolveStatic(input_args, overloads_); +} + +absl::StatusOr LazyFunctionStep::ResolveFunction( + absl::Span input_args, + const ExecutionFrameBase& frame) const { + return ResolveLazy(input_args, name_, receiver_style_, providers_, frame); +} + std::unique_ptr CreateDirectFunctionStep( int64_t expr_id, const cel::CallExpr& call, std::vector> deps, @@ -513,24 +465,24 @@ std::unique_ptr CreateDirectLazyFunctionStep( LazyResolver(std::move(providers), call.function(), call.has_target())); } -absl::StatusOr> CreateFunctionStep( +std::unique_ptr CreateLazyFunctionStep( const cel::CallExpr& call_expr, int64_t expr_id, std::vector lazy_overloads) { bool receiver_style = call_expr.has_target(); size_t num_args = call_expr.args().size() + (receiver_style ? 1 : 0); - const std::string& name = call_expr.function(); - return std::make_unique(name, num_args, receiver_style, - std::move(lazy_overloads), expr_id); + return std::make_unique(std::move(lazy_overloads), + call_expr.function(), num_args, + receiver_style, expr_id); } -absl::StatusOr> CreateFunctionStep( +std::unique_ptr CreateFunctionStep( const cel::CallExpr& call_expr, int64_t expr_id, std::vector overloads) { bool receiver_style = call_expr.has_target(); size_t num_args = call_expr.args().size() + (receiver_style ? 1 : 0); - const std::string& name = call_expr.function(); - return std::make_unique(std::move(overloads), name, - num_args, receiver_style, expr_id); + return std::make_unique(std::move(overloads), + call_expr.function(), num_args, + receiver_style, expr_id); } } // namespace google::api::expr::runtime diff --git a/eval/eval/function_step.h b/eval/eval/function_step.h index 94307609e..e0373d670 100644 --- a/eval/eval/function_step.h +++ b/eval/eval/function_step.h @@ -1,14 +1,21 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_FUNCTION_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_FUNCTION_STEP_H_ +#include #include #include +#include +#include +#include #include +#include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/types/span.h" #include "common/expr.h" +#include "common/value.h" #include "eval/eval/direct_expression_step.h" -#include "eval/eval/evaluator_core.h" +#include "eval/eval/expression_step_logic.h" #include "runtime/function_overload_reference.h" #include "runtime/function_registry.h" @@ -30,19 +37,88 @@ std::unique_ptr CreateDirectLazyFunctionStep( std::vector> deps, std::vector providers); +class LazyFunctionStep; +class EagerFunctionStep; +class ExecutionFrameBase; +class ExecutionFrame; + // Factory method for Call-based execution step where the function will be // resolved at runtime (lazily) from an input Activation. -absl::StatusOr> CreateFunctionStep( +std::unique_ptr CreateLazyFunctionStep( const cel::CallExpr& call, int64_t expr_id, std::vector lazy_overloads); // Factory method for Call-based execution step where the function has been // statically resolved from a set of eagerly functions configured in the // CelFunctionRegistry. -absl::StatusOr> CreateFunctionStep( +std::unique_ptr CreateFunctionStep( const cel::CallExpr& call, int64_t expr_id, std::vector overloads); +// Common base class for EagerFunctionStep and LazyFunctionStep. +class FunctionStepBase { + private: + friend class EagerFunctionStep; + friend class LazyFunctionStep; + template + friend void EvaluateFunctionStep(const Step* step, ExecutionFrame& frame); + + // Constructs FunctionStep that uses overloads specified. + FunctionStepBase(const std::string& name, size_t num_arguments, + bool receiver_style, int64_t expr_id) + : name_(name), + num_arguments_(num_arguments), + receiver_style_(receiver_style), + expr_id_(expr_id) {} + + std::string name_; + size_t num_arguments_; + bool receiver_style_; + int64_t expr_id_; +}; + +class EagerFunctionStep : public FunctionStepBase { + public: + EagerFunctionStep(std::vector overloads, + const std::string& name, size_t num_args, + bool receiver_style, int64_t expr_id) + : FunctionStepBase(name, num_args, receiver_style, expr_id), + overloads_(std::move(overloads)) {} + + void Evaluate(ExecutionFrame& frame) const; + + private: + template + friend void EvaluateFunctionStep(const Step* step, ExecutionFrame& frame); + + std::optional ResolveFunction( + absl::Span input_args, + const ExecutionFrameBase& frame) const; + + std::vector overloads_; +}; + +class LazyFunctionStep : public FunctionStepBase { + public: + LazyFunctionStep(std::vector providers, + const std::string& name, size_t num_args, + bool receiver_style, int64_t expr_id) + : FunctionStepBase(name, num_args, receiver_style, expr_id), + providers_(std::move(providers)) {} + + void Evaluate(ExecutionFrame& frame) const; + + private: + template + friend void EvaluateFunctionStep(const Step* step, ExecutionFrame& frame); + + absl::StatusOr> ResolveFunction( + absl::Span input_args, + const ExecutionFrameBase& frame) const; + + std::vector providers_; +}; + } // namespace google::api::expr::runtime #endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_FUNCTION_STEP_H_ diff --git a/eval/eval/function_step_test.cc b/eval/eval/function_step_test.cc index 8adbda76c..0480c1446 100644 --- a/eval/eval/function_step_test.cc +++ b/eval/eval/function_step_test.cc @@ -8,6 +8,7 @@ #include #include "absl/status/status.h" +#include "absl/status/status_macros.h" #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" @@ -233,14 +234,13 @@ absl::StatusOr MakeTestFunctionStep( call.function(), call.has_target(), argument_matcher); int id = GetExprId(); if (!lazy_overloads.empty()) { - CEL_ASSIGN_OR_RETURN(auto logic, - CreateFunctionStep(call, id, lazy_overloads)); - return ExpressionStep::MakeGenericStep(std::move(logic), id); + return ExpressionStep::MakeLazyFunctionStep( + CreateLazyFunctionStep(call, id, std::move(lazy_overloads)), id); } auto overloads = registry.FindStaticOverloads( call.function(), call.has_target(), argument_matcher); - CEL_ASSIGN_OR_RETURN(auto logic, CreateFunctionStep(call, id, overloads)); - return ExpressionStep::MakeGenericStep(std::move(logic), id); + return ExpressionStep::MakeEagerFunctionStep( + CreateFunctionStep(call, id, std::move(overloads)), id); } // Test common functions with varying levels of unknown support. @@ -401,17 +401,13 @@ TEST_P(FunctionStepTest, TestNoMatchingOverloadsUnexpectedArgCount) { ASSERT_OK_AND_ASSIGN(auto step1, MakeTestFunctionStep(call1, registry)); ASSERT_OK_AND_ASSIGN(auto step2, MakeTestFunctionStep(call1, registry)); - ASSERT_OK_AND_ASSIGN( - auto step3_logic, - CreateFunctionStep(add_call, -1, - registry.FindStaticOverloads( - add_call.function(), false, - {cel::Kind::kInt64, cel::Kind::kInt64}))); - path.push_back(std::move(step0)); path.push_back(std::move(step1)); path.push_back(std::move(step2)); - path.push_back(ExpressionStep::MakeGenericStep(std::move(step3_logic))); + path.push_back(ExpressionStep::MakeEagerFunctionStep(CreateFunctionStep( + add_call, -1, + registry.FindStaticOverloads(add_call.function(), false, + {cel::Kind::kInt64, cel::Kind::kInt64})))); std::unique_ptr impl = GetExpression(std::move(path)); @@ -540,11 +536,11 @@ TEST_P(FunctionStepTest, LazyFunctionOverloadingTest) { lt_call.mutable_args().emplace_back(); lt_call.set_function("_<_"); - auto step0 = ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::interop_internal::CreateIntValue(20))); + auto step0 = + ExpressionStep::MakeConstant(cel::interop_internal::CreateIntValue(20)); ASSERT_OK_AND_ASSIGN(auto step1, MakeTestFunctionStep(call1, registry)); - auto step2 = ExpressionStep::MakeGenericStep( - CreateConstValueStep(cel::interop_internal::CreateDoubleValue(21.9))); + auto step2 = ExpressionStep::MakeConstant( + cel::interop_internal::CreateDoubleValue(21.9)); ASSERT_OK_AND_ASSIGN(auto step3, MakeTestFunctionStep(call2, registry)); ASSERT_OK_AND_ASSIGN(auto step4, MakeTestFunctionStep(lt_call, registry)); diff --git a/eval/eval/ident_step.cc b/eval/eval/ident_step.cc index 9d6edef72..e3985d183 100644 --- a/eval/eval/ident_step.cc +++ b/eval/eval/ident_step.cc @@ -17,6 +17,7 @@ #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" @@ -31,7 +32,7 @@ class IdentStep : public ExpressionStepBase { public: explicit IdentStep(absl::string_view name) : name_(name) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: std::string name_; @@ -73,15 +74,17 @@ absl::Status LookupIdent(absl::string_view name, ExecutionFrameBase& frame, return absl::OkStatus(); } -absl::Status IdentStep::Evaluate(ExecutionFrame* frame) const { +void IdentStep::Evaluate(ExecutionFrame* frame) const { Value value; AttributeTrail attribute; - CEL_RETURN_IF_ERROR(LookupIdent(name_, *frame, value, 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)); - - return absl::OkStatus(); } absl::StatusOr LookupSlot( @@ -94,24 +97,6 @@ absl::StatusOr LookupSlot( return slot; } -class SlotStep : public ExpressionStepBase { - public: - SlotStep(absl::string_view name, size_t slot_index) - : name_(name), slot_index_(slot_index) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - CEL_ASSIGN_OR_RETURN(const ComprehensionSlots::Slot* slot, - LookupSlot(name_, slot_index_, *frame)); - frame->value_stack().Push(slot->value(), slot->attribute()); - return absl::OkStatus(); - } - - private: - std::string name_; - - size_t slot_index_; -}; - class DirectIdentStep : public DirectExpressionStep { public: DirectIdentStep(absl::string_view name, int64_t expr_id) @@ -167,9 +152,4 @@ std::unique_ptr CreateIdentStep( return std::make_unique(name); } -std::unique_ptr CreateIdentStepForSlot( - const absl::string_view name, size_t slot_index) { - return std::make_unique(name, slot_index); -} - } // namespace google::api::expr::runtime diff --git a/eval/eval/ident_step.h b/eval/eval/ident_step.h index ad65478bd..508056827 100644 --- a/eval/eval/ident_step.h +++ b/eval/eval/ident_step.h @@ -5,10 +5,9 @@ #include #include -#include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "eval/eval/direct_expression_step.h" -#include "eval/eval/evaluator_core.h" +#include "eval/eval/expression_step_logic.h" namespace google::api::expr::runtime { @@ -21,10 +20,6 @@ std::unique_ptr CreateDirectSlotIdentStep( // Factory method for Ident - based Execution step std::unique_ptr CreateIdentStep(absl::string_view name); -// Factory method for identifier that has been assigned to a slot. -std::unique_ptr CreateIdentStepForSlot( - absl::string_view name, size_t slot_index); - } // namespace google::api::expr::runtime #endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_IDENT_STEP_H_ diff --git a/eval/eval/iterator_stack.h b/eval/eval/iterator_stack.h index 9b5daa889..0b0de14aa 100644 --- a/eval/eval/iterator_stack.h +++ b/eval/eval/iterator_stack.h @@ -27,8 +27,15 @@ namespace cel::runtime_internal { class IteratorStack final { public: + struct Entry { + absl_nonnull ValueIteratorPtr iterator; + size_t iter_slot; + size_t iter2_slot; + size_t accu_slot; + }; + explicit IteratorStack(size_t max_size) : max_size_(max_size) { - iterators_.reserve(max_size_); + entries_.reserve(max_size_); } IteratorStack(const IteratorStack&) = delete; @@ -37,38 +44,55 @@ class IteratorStack final { IteratorStack& operator=(const IteratorStack&) = delete; IteratorStack& operator=(IteratorStack&&) = delete; - size_t size() const { return iterators_.size(); } + size_t size() const { return entries_.size(); } - bool empty() const { return iterators_.empty(); } + bool empty() const { return entries_.empty(); } - bool full() const { return iterators_.size() == max_size_; } + bool full() const { return entries_.size() == max_size_; } size_t max_size() const { return max_size_; } - void Clear() { iterators_.clear(); } + void Clear() { entries_.clear(); } + + void Push(absl_nonnull ValueIteratorPtr iterator, size_t iter_slot, + size_t iter2_slot, size_t accu_slot) { + ABSL_DCHECK(!full()); + ABSL_DCHECK(iterator != nullptr); + + entries_.push_back( + Entry{std::move(iterator), iter_slot, iter2_slot, accu_slot}); + } - void Push(absl_nonnull ValueIteratorPtr iterator) { + void Push(absl_nonnull ValueIteratorPtr iterator, size_t iter_slot, + size_t accu_slot) { ABSL_DCHECK(!full()); ABSL_DCHECK(iterator != nullptr); - iterators_.push_back(std::move(iterator)); + entries_.push_back(Entry{std::move(iterator), iter_slot, 0, accu_slot}); + } + + ValueIterator* absl_nonnull PeekIterator() { + ABSL_DCHECK(!empty()); + + return entries_.back().iterator.get(); } - ValueIterator* absl_nonnull Peek() { + // Returns a pointer to the top entry in the stack. + // Invalidated by Pop() and Push(). + Entry* absl_nonnull Peek() { ABSL_DCHECK(!empty()); - ABSL_DCHECK(iterators_.back() != nullptr); - return iterators_.back().get(); + return &entries_.back(); } void Pop() { ABSL_DCHECK(!empty()); - iterators_.pop_back(); + entries_.pop_back(); } private: - std::vector iterators_; + std::vector entries_; size_t max_size_; }; diff --git a/eval/eval/jump_step.cc b/eval/eval/jump_step.cc deleted file mode 100644 index ff5d864f2..000000000 --- a/eval/eval/jump_step.cc +++ /dev/null @@ -1,171 +0,0 @@ -// Copyright 2017 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include "eval/eval/jump_step.h" - -#include -#include -#include -#include - -#include "absl/status/status.h" -#include "absl/types/optional.h" -#include "common/value.h" -#include "eval/eval/evaluator_core.h" -#include "eval/internal/errors.h" - -namespace google::api::expr::runtime { - -namespace { - -using ::cel::BoolValue; -using ::cel::ErrorValue; -using ::cel::UnknownValue; -using ::cel::Value; -using ::cel::runtime_internal::CreateNoMatchingOverloadError; - -class JumpStep : public JumpStepBase { - public: - // Constructs FunctionStep that uses overloads specified. - explicit JumpStep(absl::optional jump_offset) - : JumpStepBase(jump_offset) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - return Jump(frame); - } -}; - -class CondJumpStep : public JumpStepBase { - public: - CondJumpStep(bool jump_condition, absl::optional jump_offset, - size_t stack_size) - : JumpStepBase(jump_offset), - jump_condition_(jump_condition), - stack_size_(stack_size) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - // Peek the top value - if (!frame->value_stack().HasEnough(stack_size_)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); - } - - const auto& value = frame->value_stack().Peek(); - const auto should_jump = - value.IsBool() && jump_condition_ == value.GetBool().NativeValue(); - - if (should_jump) { - frame->value_stack().SwapAndPop(stack_size_, stack_size_ - 1); - return Jump(frame); - } - - return absl::OkStatus(); - } - - private: - const bool jump_condition_; - const size_t stack_size_; -}; - -class TernaryCondJumpStep : public JumpStepBase { - public: - explicit TernaryCondJumpStep(absl::optional jump_offset) - : JumpStepBase(jump_offset) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - // Peek the top value - if (!frame->value_stack().HasEnough(1)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); - } - - const auto& value = frame->value_stack().Peek(); - const auto should_jump = value.IsBool() && !value.GetBool().NativeValue(); - - frame->value_stack().Pop(1); - - if (should_jump) { - return Jump(frame); - } - - return absl::OkStatus(); - } -}; - -class BoolCheckJumpStep : public JumpStepBase { - public: - // Checks if the top value is a boolean: - // - no-op if it is a boolean - // - jump to the label if it is an error value - // - jump to the label if it is unknown value - // - jump to the label if it is neither an error nor a boolean, pops it and - // pushes "no matching overload" error - explicit BoolCheckJumpStep(absl::optional jump_offset) - : JumpStepBase(jump_offset) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - // Peek the top value - if (!frame->value_stack().HasEnough(1)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); - } - - const Value& value = frame->value_stack().Peek(); - - if (value->Is()) { - return absl::OkStatus(); - } - - if (value->Is() || value->Is()) { - return Jump(frame); - } - - // Neither bool, error, nor unknown set. - Value error_value = cel::ErrorValue::From( - CreateNoMatchingOverloadError(""), frame->arena()); - - frame->value_stack().PopAndPush(std::move(error_value)); - return Jump(frame); - - return absl::OkStatus(); - } -}; - -} // namespace - -// Factory method for Conditional Jump step. -std::unique_ptr CreateCondJumpStep( - bool jump_condition, absl::optional jump_offset, - size_t expected_stack_size) { - return std::make_unique(jump_condition, jump_offset, - expected_stack_size); -} - -// Factory method for Ternary Conditional Jump step. -std::unique_ptr CreateTernaryCondJumpStep( - absl::optional jump_offset) { - return std::make_unique(jump_offset); -} - -// Factory method for Jump step. -std::unique_ptr CreateJumpStep(absl::optional jump_offset) { - return std::make_unique(jump_offset); -} - -// Factory method for Conditional Jump step. -// Conditional Jump requires a value to sit on the stack. -// If this value is an error or unknown, a jump is performed. -std::unique_ptr CreateBoolCheckJumpStep( - absl::optional jump_offset) { - return std::make_unique(jump_offset); -} - -} // namespace google::api::expr::runtime diff --git a/eval/eval/jump_step.h b/eval/eval/jump_step.h deleted file mode 100644 index 5cef97329..000000000 --- a/eval/eval/jump_step.h +++ /dev/null @@ -1,74 +0,0 @@ -// Copyright 2017 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_JUMP_STEP_H_ -#define THIRD_PARTY_CEL_CPP_EVAL_EVAL_JUMP_STEP_H_ - -#include -#include - -#include "cel/expr/syntax.pb.h" -#include "absl/status/status.h" -#include "absl/types/optional.h" -#include "eval/eval/evaluator_core.h" -#include "eval/eval/expression_step_base.h" - -namespace google::api::expr::runtime { - -class JumpStepBase : public ExpressionStepBase { - public: - explicit JumpStepBase(absl::optional jump_offset = absl::nullopt) - : ExpressionStepBase(), jump_offset_(jump_offset) {} - - void set_jump_offset(int offset) { jump_offset_ = offset; } - - absl::Status Jump(ExecutionFrame* frame) const { - if (!jump_offset_.has_value()) { - return absl::Status(absl::StatusCode::kInternal, "Jump offset not set"); - } - return frame->JumpTo(jump_offset_.value()); - } - - private: - absl::optional jump_offset_; -}; - -// Factory method for Jump step. -std::unique_ptr CreateJumpStep( - absl::optional jump_offset = absl::nullopt); - -// Factory method for Conditional Jump step (used for and/or shortcircuiting). -// Conditional Jump requires a boolean value to sit on the stack. -// It is compared to jump_condition, and if matched, jump is performed. -// The boolean value is left on top of the stack. -std::unique_ptr CreateCondJumpStep( - bool jump_condition, absl::optional jump_offset, - size_t expected_stack_size); - -// Factory method for Ternary Conditional Jump step. -// Requires a boolean condition value on top of the stack. -// If the boolean value is false, a jump is performed to the second branch. -// The condition value is popped from the stack before jumping or continuing. -std::unique_ptr CreateTernaryCondJumpStep( - absl::optional jump_offset = absl::nullopt); - -// Factory method for ErrorJump step. -// This step performs a Jump when an Error is on the top of the stack. -// Value is left on stack if it is a bool or an error. -std::unique_ptr CreateBoolCheckJumpStep( - absl::optional jump_offset = absl::nullopt); - -} // namespace google::api::expr::runtime - -#endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_JUMP_STEP_H_ diff --git a/eval/eval/lazy_init_step.cc b/eval/eval/lazy_init_step.cc index 7d5fa538a..cdb1f9666 100644 --- a/eval/eval/lazy_init_step.cc +++ b/eval/eval/lazy_init_step.cc @@ -27,7 +27,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 "internal/status_macros.h" namespace google::api::expr::runtime { @@ -36,28 +35,6 @@ namespace { using ::cel::Value; -class LazyInitStep final : public ExpressionStepBase { - public: - LazyInitStep(size_t slot_index, size_t subexpression_index) - : ExpressionStepBase(), - slot_index_(slot_index), - subexpression_index_(subexpression_index) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - ComprehensionSlot* slot = frame->comprehension_slots().Get(slot_index_); - if (slot->Has()) { - frame->value_stack().Push(slot->value(), slot->attribute()); - } else { - frame->Call(slot_index_, subexpression_index_); - } - return absl::OkStatus(); - } - - private: - const size_t slot_index_; - const size_t subexpression_index_; -}; - class DirectLazyInitStep final : public DirectExpressionStep { public: DirectLazyInitStep(size_t slot_index, @@ -106,61 +83,6 @@ class BindStep : public DirectExpressionStep { std::unique_ptr subexpression_; }; -class AssignSlotAndPopStepStep final : public ExpressionStepBase { - public: - explicit AssignSlotAndPopStepStep(size_t slot_index) - : ExpressionStepBase(/*expr_id=*/-1, /*comes_from_ast=*/false), - slot_index_(slot_index) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - if (!frame->value_stack().HasEnough(1)) { - return absl::InternalError("Stack underflow assigning lazy value"); - } - - frame->comprehension_slots().Set(slot_index_, frame->value_stack().Peek(), - frame->value_stack().PeekAttribute()); - frame->value_stack().Pop(1); - - return absl::OkStatus(); - } - - private: - const size_t slot_index_; -}; - -class ClearSlotStep : public ExpressionStepBase { - public: - explicit ClearSlotStep(size_t slot_index) - : ExpressionStepBase(), slot_index_(slot_index) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - frame->comprehension_slots().ClearSlot(slot_index_); - return absl::OkStatus(); - } - - private: - size_t slot_index_; -}; - -class ClearSlotsStep final : public ExpressionStepBase { - public: - ClearSlotsStep(size_t slot_index, size_t slot_count) - : ExpressionStepBase(), - slot_index_(slot_index), - slot_count_(slot_count) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - for (size_t i = 0; i < slot_count_; ++i) { - frame->comprehension_slots().ClearSlot(slot_index_ + i); - } - return absl::OkStatus(); - } - - private: - const size_t slot_index_; - const size_t slot_count_; -}; - class BlockStep : public DirectExpressionStep { public: BlockStep(size_t slot_index, size_t slot_count, @@ -190,6 +112,32 @@ class BlockStep : public DirectExpressionStep { } // namespace +void EvaluateLazyInitStep(const LazyInitStepInfo& step, ExecutionFrame& frame) { + ComprehensionSlot* slot = frame.comprehension_slots().Get(step.slot_index); + if (slot->Has()) { + frame.value_stack().Push(slot->value(), slot->attribute()); + } else { + frame.Call(step.slot_index, step.subexpression_index); + } +} + +void EvaluateAssignSlotAndPop(size_t slot_index, ExecutionFrame& frame) { + if (!frame.value_stack().HasEnough(1)) { + frame.Abort(absl::InternalError("Stack underflow assigning lazy value")); + return; + } + ComprehensionSlot* slot = frame.comprehension_slots().Get(slot_index); + slot->Set(frame.value_stack().Peek(), frame.value_stack().PeekAttribute()); + frame.value_stack().Pop(1); +} + +void EvaluateClearSlotStep(const ClearSlotStepInfo& step, + ExecutionFrame& frame) { + for (size_t i = 0; i < step.slot_count; ++i) { + frame.comprehension_slots().ClearSlot(step.slot_index + i); + } +} + std::unique_ptr CreateDirectBindStep( size_t slot_index, std::unique_ptr expression, int64_t expr_id) { @@ -210,24 +158,4 @@ std::unique_ptr CreateDirectLazyInitStep( expr_id); } -std::unique_ptr CreateLazyInitStep( - size_t slot_index, size_t subexpression_index) { - return std::make_unique(slot_index, subexpression_index); -} - -std::unique_ptr CreateAssignSlotAndPopStep( - size_t slot_index) { - return std::make_unique(slot_index); -} - -std::unique_ptr CreateClearSlotStep(size_t slot_index) { - return std::make_unique(slot_index); -} - -std::unique_ptr CreateClearSlotsStep(size_t slot_index, - size_t slot_count) { - ABSL_DCHECK_GT(slot_count, 0); - return std::make_unique(slot_index, slot_count); -} - } // namespace google::api::expr::runtime diff --git a/eval/eval/lazy_init_step.h b/eval/eval/lazy_init_step.h index e1ef59c93..c73c80ee1 100644 --- a/eval/eval/lazy_init_step.h +++ b/eval/eval/lazy_init_step.h @@ -43,10 +43,11 @@ #include "absl/base/nullability.h" #include "eval/eval/direct_expression_step.h" -#include "eval/eval/evaluator_core.h" namespace google::api::expr::runtime { +class ExecutionFrame; + // Creates a step representing a Bind expression. std::unique_ptr CreateDirectBindStep( size_t slot_index, std::unique_ptr expression, @@ -63,22 +64,22 @@ std::unique_ptr CreateDirectLazyInitStep( size_t slot_index, const DirectExpressionStep* absl_nonnull subexpression, int64_t expr_id); -// Creates a step representing accessing a lazily evaluated alias from -// a bind or block. -std::unique_ptr CreateLazyInitStep( - size_t slot_index, size_t subexpression_index); +struct LazyInitStepInfo { + size_t slot_index : 32; + size_t subexpression_index : 32; +}; + +void EvaluateLazyInitStep(const LazyInitStepInfo& step, ExecutionFrame& frame); -// Helper step to assign a slot value from the top of stack on initialization. -std::unique_ptr CreateAssignSlotAndPopStep( - size_t slot_index); +void EvaluateAssignSlotAndPop(size_t slot_index, ExecutionFrame& frame); -// Helper step to clear a slot. -// Slots may be reused in different contexts so need to be cleared after a -// context is done. -std::unique_ptr CreateClearSlotStep(size_t slot_index); +struct ClearSlotStepInfo { + size_t slot_index : 32; + size_t slot_count : 32 = 1; +}; -std::unique_ptr CreateClearSlotsStep(size_t slot_index, - size_t slot_count); +void EvaluateClearSlotStep(const ClearSlotStepInfo& step, + ExecutionFrame& frame); } // namespace google::api::expr::runtime diff --git a/eval/eval/lazy_init_step_test.cc b/eval/eval/lazy_init_step_test.cc index 00490dbe1..3b5282d21 100644 --- a/eval/eval/lazy_init_step_test.cc +++ b/eval/eval/lazy_init_step_test.cc @@ -19,7 +19,7 @@ #include "base/type_provider.h" #include "common/value.h" -#include "eval/eval/const_value_step.h" +#include "eval/eval/comprehension_slots.h" #include "eval/eval/evaluator_core.h" #include "internal/testing.h" #include "internal/testing_descriptor_pool.h" @@ -58,94 +58,85 @@ class LazyInitStepTest : public testing::Test { Activation activation_; }; -TEST_F(LazyInitStepTest, CreateCheckInitStepDoesInit) { +TEST_F(LazyInitStepTest, MakeLazyInitDoesInit) { ExecutionPath path; ExecutionPath subpath; - path.push_back(ExpressionStep::MakeGenericStep(CreateLazyInitStep( - /*slot_index=*/0, /*subexpression_index=*/1))); + path.push_back(ExpressionStep::MakeLazyInitStep( + /*slot_index=*/0, /*subexpression_index=*/1)); - subpath.push_back( - ExpressionStep::MakeGenericStep(CreateConstValueStep(cel::IntValue(42)))); + subpath.push_back(ExpressionStep::MakeConstant(cel::IntValue(42))); std::vector expression_table{path, subpath}; ExecutionFrame frame(expression_table, activation_, runtime_options_, evaluator_state_); - ASSERT_OK_AND_ASSIGN(auto value, frame.Evaluate()); + ASSERT_OK_AND_ASSIGN(cel::Value value, frame.Evaluate()); EXPECT_TRUE(value->Is() && value.GetInt().NativeValue() == 42); } -TEST_F(LazyInitStepTest, CreateCheckInitStepSkipInit) { +TEST_F(LazyInitStepTest, MakeLazyInitSkipInit) { ExecutionPath path; ExecutionPath subpath; - // This is the expected usage, but in this test we are just depending on the - // fact that these don't change the stack and fit the program layout - // requirements. - path.push_back(ExpressionStep::MakeGenericStep(CreateLazyInitStep( - /*slot_index=*/0, /*subexpression_index=*/-1))); + path.push_back(ExpressionStep::MakeLazyInitStep( + /*slot_index=*/0, /*subexpression_index=*/2)); - subpath.push_back( - ExpressionStep::MakeGenericStep(CreateConstValueStep(cel::IntValue(42)))); + subpath.push_back(ExpressionStep::MakeConstant(cel::IntValue(42))); std::vector expression_table{path, subpath}; ExecutionFrame frame(expression_table, activation_, runtime_options_, evaluator_state_); frame.comprehension_slots().Set(0, cel::IntValue(42)); - ASSERT_OK_AND_ASSIGN(auto value, frame.Evaluate()); + ASSERT_OK_AND_ASSIGN(cel::Value value, frame.Evaluate()); EXPECT_TRUE(value->Is() && value.GetInt().NativeValue() == 42); } -TEST_F(LazyInitStepTest, CreateAssignSlotAndPopStepBasic) { +TEST_F(LazyInitStepTest, MakeAssignSlotAndPopBasic) { ExecutionPath path; - path.push_back( - ExpressionStep::MakeGenericStep(CreateAssignSlotAndPopStep(0))); + path.push_back(ExpressionStep::MakeAssignSlotAndPopStep(0)); ExecutionFrame frame(path, activation_, runtime_options_, evaluator_state_); frame.comprehension_slots().ClearSlot(0); frame.value_stack().Push(cel::IntValue(42)); - // This will error because no return value, step will still evaluate. frame.Evaluate().IgnoreError(); - auto* slot = frame.comprehension_slots().Get(0); + ComprehensionSlots::Slot* slot = frame.comprehension_slots().Get(0); ASSERT_TRUE(slot->Has()); EXPECT_TRUE(slot->value()->Is() && slot->value().GetInt().NativeValue() == 42); EXPECT_TRUE(frame.value_stack().empty()); } -TEST_F(LazyInitStepTest, CreateClearSlotStepBasic) { +TEST_F(LazyInitStepTest, MakeClearSlotBasic) { ExecutionPath path; - path.push_back(ExpressionStep::MakeGenericStep(CreateClearSlotStep(0))); + path.push_back(ExpressionStep::MakeClearSlotStep(0)); ExecutionFrame frame(path, activation_, runtime_options_, evaluator_state_); frame.comprehension_slots().Set(0, cel::IntValue(42)); - // This will error because no return value, step will still evaluate. frame.Evaluate().IgnoreError(); - auto* slot = frame.comprehension_slots().Get(0); + ComprehensionSlot* slot = frame.comprehension_slots().Get(0); ASSERT_FALSE(slot->Has()); } -TEST_F(LazyInitStepTest, CreateClearSlotsStepBasic) { +TEST_F(LazyInitStepTest, MakeClearSlotsBasic) { ExecutionPath path; - path.push_back(ExpressionStep::MakeGenericStep(CreateClearSlotsStep(0, 2))); + path.push_back(ExpressionStep::MakeClearSlotsStep(0, 2)); ExecutionFrame frame(path, activation_, runtime_options_, evaluator_state_); frame.comprehension_slots().Set(0, cel::IntValue(42)); frame.comprehension_slots().Set(1, cel::IntValue(42)); - // This will error because no return value, step will still evaluate. frame.Evaluate().IgnoreError(); EXPECT_FALSE(frame.comprehension_slots().Get(0)->Has()); diff --git a/eval/eval/logic_step.cc b/eval/eval/logic_step.cc index 9eaa94f4b..a7528687b 100644 --- a/eval/eval/logic_step.cc +++ b/eval/eval/logic_step.cc @@ -7,8 +7,7 @@ #include #include "absl/status/status.h" -#include "absl/status/statusor.h" -#include "absl/types/optional.h" +#include "absl/strings/string_view.h" #include "absl/types/span.h" #include "base/builtins.h" #include "common/casting.h" @@ -17,7 +16,6 @@ #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 "eval/internal/errors.h" #include "internal/status_macros.h" #include "runtime/internal/errors.h" @@ -35,11 +33,9 @@ using ::cel::Value; using ::cel::ValueKind; using ::cel::runtime_internal::CreateNoMatchingOverloadError; -enum class OpType { kAnd, kOr }; - // Shared logic for the fall through case (we didn't see the shortcircuit // value). -absl::Status ReturnLogicResult(ExecutionFrameBase& frame, OpType op_type, +absl::Status ReturnLogicResult(ExecutionFrameBase& frame, BoolLogicKind op_type, Value& lhs_result, Value& rhs_result, AttributeTrail& attribute_trail, AttributeTrail& rhs_attr) { @@ -78,8 +74,9 @@ absl::Status ReturnLogicResult(ExecutionFrameBase& frame, OpType op_type, // Otherwise, add a no overload error. attribute_trail = AttributeTrail(); lhs_result = cel::ErrorValue::From( - CreateNoMatchingOverloadError( - op_type == OpType::kOr ? cel::builtin::kOr : cel::builtin::kAnd), + CreateNoMatchingOverloadError(op_type == BoolLogicKind::kOr + ? cel::builtin::kOr + : cel::builtin::kAnd), frame.arena()); return absl::OkStatus(); } @@ -88,7 +85,7 @@ class ExhaustiveDirectLogicStep : public DirectExpressionStep { public: explicit ExhaustiveDirectLogicStep(std::unique_ptr lhs, std::unique_ptr rhs, - OpType op_type, int64_t expr_id) + BoolLogicKind op_type, int64_t expr_id) : DirectExpressionStep(expr_id), lhs_(std::move(lhs)), rhs_(std::move(rhs)), @@ -100,7 +97,7 @@ class ExhaustiveDirectLogicStep : public DirectExpressionStep { private: std::unique_ptr lhs_; std::unique_ptr rhs_; - OpType op_type_; + BoolLogicKind op_type_; }; absl::Status ExhaustiveDirectLogicStep::Evaluate( @@ -116,16 +113,16 @@ absl::Status ExhaustiveDirectLogicStep::Evaluate( ValueKind rhs_kind = rhs_result.kind(); if (lhs_kind == ValueKind::kBool) { bool lhs_bool = Cast(result).NativeValue(); - if ((op_type_ == OpType::kOr && lhs_bool) || - (op_type_ == OpType::kAnd && !lhs_bool)) { + if ((op_type_ == BoolLogicKind::kOr && lhs_bool) || + (op_type_ == BoolLogicKind::kAnd && !lhs_bool)) { return absl::OkStatus(); } } if (rhs_kind == ValueKind::kBool) { bool rhs_bool = Cast(rhs_result).NativeValue(); - if ((op_type_ == OpType::kOr && rhs_bool) || - (op_type_ == OpType::kAnd && !rhs_bool)) { + if ((op_type_ == BoolLogicKind::kOr && rhs_bool) || + (op_type_ == BoolLogicKind::kAnd && !rhs_bool)) { result = std::move(rhs_result); attribute_trail = std::move(rhs_attr); return absl::OkStatus(); @@ -140,7 +137,7 @@ class DirectLogicStep : public DirectExpressionStep { public: explicit DirectLogicStep(std::unique_ptr lhs, std::unique_ptr rhs, - OpType op_type, int64_t expr_id) + BoolLogicKind op_type, int64_t expr_id) : DirectExpressionStep(expr_id), lhs_(std::move(lhs)), rhs_(std::move(rhs)), @@ -152,7 +149,7 @@ class DirectLogicStep : public DirectExpressionStep { private: std::unique_ptr lhs_; std::unique_ptr rhs_; - OpType op_type_; + BoolLogicKind op_type_; }; absl::Status DirectLogicStep::Evaluate(ExecutionFrameBase& frame, Value& result, @@ -161,8 +158,8 @@ absl::Status DirectLogicStep::Evaluate(ExecutionFrameBase& frame, Value& result, ValueKind lhs_kind = result.kind(); if (lhs_kind == ValueKind::kBool) { bool lhs_bool = Cast(result).NativeValue(); - if ((op_type_ == OpType::kOr && lhs_bool) || - (op_type_ == OpType::kAnd && !lhs_bool)) { + if ((op_type_ == BoolLogicKind::kOr && lhs_bool) || + (op_type_ == BoolLogicKind::kAnd && !lhs_bool)) { return absl::OkStatus(); } } @@ -176,8 +173,8 @@ absl::Status DirectLogicStep::Evaluate(ExecutionFrameBase& frame, Value& result, if (rhs_kind == ValueKind::kBool) { bool rhs_bool = Cast(rhs_result).NativeValue(); - if ((op_type_ == OpType::kOr && rhs_bool) || - (op_type_ == OpType::kAnd && !rhs_bool)) { + if ((op_type_ == BoolLogicKind::kOr && rhs_bool) || + (op_type_ == BoolLogicKind::kAnd && !rhs_bool)) { result = std::move(rhs_result); attribute_trail = std::move(rhs_attr); return absl::OkStatus(); @@ -188,94 +185,10 @@ absl::Status DirectLogicStep::Evaluate(ExecutionFrameBase& frame, Value& result, rhs_attr); } -class LogicalOpStep : public ExpressionStepBase { - public: - // Constructs FunctionStep that uses overloads specified. - LogicalOpStep(OpType op_type, size_t count) - : shortcircuit_(op_type == OpType::kOr), - op_type_(op_type), - count_(count) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override; - - private: - void Calculate(ExecutionFrame* frame, absl::Span args, - Value& result) const { - std::optional error_pos; - - for (size_t i = 0; i < args.size(); i++) { - const Value& arg = args[i]; - switch (arg.kind()) { - case ValueKind::kBool: - if (arg.GetBool() == shortcircuit_) { - result = arg; - return; - } - break; - case ValueKind::kUnknown: - break; - case ValueKind::kError: - default: - if (!error_pos.has_value()) { - error_pos = i; - } - break; - } - } - - // As opposed to regular function, logical operation treat Unknowns with - // higher precedence than error. This is due to the fact that after Unknown - // is resolved to actual value, it may short-circuit and thus hide the - // error. - if (frame->enable_unknowns()) { - // Check if unknown? - absl::optional unknown_set = - frame->attribute_utility().MergeUnknowns(args); - if (unknown_set.has_value()) { - result = std::move(*unknown_set); - return; - } - } - - if (!error_pos.has_value()) { - result = cel::BoolValue(!shortcircuit_); - return; - } - - result = args[error_pos.value()]; - if (!result.IsError()) { - result = cel::ErrorValue::From( - CreateNoMatchingOverloadError((op_type_ == OpType::kOr) - ? cel::builtin::kOr - : cel::builtin::kAnd), - frame->arena()); - } - } - - bool shortcircuit_; - const OpType op_type_; - size_t count_; -}; - -absl::Status LogicalOpStep::Evaluate(ExecutionFrame* frame) const { - // Must have 2 or more values on the stack. - if (!frame->value_stack().HasEnough(count_)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); - } - - // Create Span object that contains input arguments to the function. - auto args = frame->value_stack().GetSpan(count_); - Value result; - Calculate(frame, args, result); - frame->value_stack().PopAndPush(args.size(), std::move(result)); - - return absl::OkStatus(); -} - std::unique_ptr CreateDirectLogicStep( std::unique_ptr lhs, - std::unique_ptr rhs, int64_t expr_id, OpType op_type, - bool shortcircuiting) { + std::unique_ptr rhs, int64_t expr_id, + BoolLogicKind op_type, bool shortcircuiting) { if (shortcircuiting) { return std::make_unique(std::move(lhs), std::move(rhs), op_type, expr_id); @@ -326,47 +239,6 @@ absl::Status DirectNotStep::Evaluate(ExecutionFrameBase& frame, Value& result, return absl::OkStatus(); } -class IterativeNotStep : public ExpressionStepBase { - public: - IterativeNotStep() = default; - - absl::Status Evaluate(ExecutionFrame* frame) const override; -}; - -absl::Status IterativeNotStep::Evaluate(ExecutionFrame* frame) const { - if (!frame->value_stack().HasEnough(1)) { - return absl::InternalError("Value stack underflow"); - } - const Value& operand = frame->value_stack().Peek(); - - if (frame->unknown_processing_enabled()) { - const AttributeTrail& attribute_trail = - frame->value_stack().PeekAttribute(); - if (frame->attribute_utility().CheckForUnknownPartial(attribute_trail)) { - frame->value_stack().PopAndPush( - frame->attribute_utility().CreateUnknownSet( - attribute_trail.attribute())); - return absl::OkStatus(); - } - } - - switch (operand.kind()) { - case ValueKind::kBool: - frame->value_stack().PopAndPush( - BoolValue{!operand.GetBool().NativeValue()}); - break; - case ValueKind::kUnknown: - case ValueKind::kError: - // just forward. - break; - default: - frame->value_stack().PopAndPush(cel::ErrorValue::From( - CreateNoMatchingOverloadError(cel::builtin::kNot), frame->arena())); - break; - } - - return absl::OkStatus(); -} class DirectNotStrictlyFalseStep : public DirectExpressionStep { public: @@ -402,45 +274,128 @@ absl::Status DirectNotStrictlyFalseStep::Evaluate( return absl::OkStatus(); } -class IterativeNotStrictlyFalseStep : public ExpressionStepBase { - public: - IterativeNotStrictlyFalseStep() = default; +} // namespace - absl::Status Evaluate(ExecutionFrame* frame) const override; -}; +void EvaluateNotStep(ExecutionFrame& frame) { + if (!frame.value_stack().HasEnough(1)) { + frame.Abort(absl::InternalError("Value stack underflow")); + return; + } + const Value& operand = frame.value_stack().Peek(); -absl::Status IterativeNotStrictlyFalseStep::Evaluate( - ExecutionFrame* frame) const { - if (!frame->value_stack().HasEnough(1)) { - return absl::InternalError("Value stack underflow"); + if (frame.unknown_processing_enabled()) { + const AttributeTrail& attribute_trail = frame.value_stack().PeekAttribute(); + if (frame.attribute_utility().CheckForUnknownPartial(attribute_trail)) { + frame.value_stack().PopAndPush(frame.attribute_utility().CreateUnknownSet( + attribute_trail.attribute())); + return; + } } - const Value& operand = frame->value_stack().Peek(); switch (operand.kind()) { case ValueKind::kBool: - // just forward. + frame.value_stack().PopAndPush( + BoolValue{!operand.GetBool().NativeValue()}); break; case ValueKind::kUnknown: case ValueKind::kError: - frame->value_stack().PopAndPush(BoolValue(true)); + // just forward. break; default: - frame->value_stack().PopAndPush(cel::ErrorValue::From( - CreateNoMatchingOverloadError(cel::builtin::kNot), frame->arena())); + frame.value_stack().PopAndPush(cel::ErrorValue::From( + CreateNoMatchingOverloadError(cel::builtin::kNot), frame.arena())); break; } +} - return absl::OkStatus(); +void EvaluateNotStrictlyFalseStep(ExecutionFrame& frame) { + if (!frame.value_stack().HasEnough(1)) { + frame.Abort(absl::InternalError("Value stack underflow")); + return; + } + const Value& operand = frame.value_stack().Peek(); + + switch (operand.kind()) { + case ValueKind::kBool: + // just forward. + break; + case ValueKind::kUnknown: + case ValueKind::kError: + frame.value_stack().PopAndPush(BoolValue(true)); + break; + default: + frame.value_stack().PopAndPush(cel::ErrorValue::From( + CreateNoMatchingOverloadError(cel::builtin::kNot), frame.arena())); + break; + } } -} // namespace +void EvaluateBoolLogicStep(BoolLogicKind kind, size_t num_args, + ExecutionFrame& frame) { + if (!frame.value_stack().HasEnough(num_args)) { + frame.Abort(absl::InternalError("Value stack underflow")); + return; + } + + const bool shortcircuit = kind == BoolLogicKind::kOr; + const absl::string_view op_name = + kind == BoolLogicKind::kOr ? cel::builtin::kOr : cel::builtin::kAnd; + absl::Span args = frame.value_stack().GetSpan(num_args); + std::optional error_pos; + + for (size_t i = 0; i < args.size(); i++) { + const Value& arg = args[i]; + switch (arg.kind()) { + case ValueKind::kBool: + if (arg.GetBool() == shortcircuit) { + frame.value_stack().PopAndPush(num_args, + cel::BoolValue(shortcircuit)); + return; + } + break; + case ValueKind::kUnknown: + break; + case ValueKind::kError: + default: + if (!error_pos.has_value()) { + error_pos = i; + } + break; + } + } + + // As opposed to regular function, logical operation treat Unknowns with + // higher precedence than error. This is due to the fact that after Unknown + // is resolved to actual value, it may short-circuit and thus hide the + // error. + if (frame.enable_unknowns()) { + // Check if unknown? + std::optional unknown_set = + frame.attribute_utility().MergeUnknowns(args); + if (unknown_set.has_value()) { + frame.value_stack().PopAndPush(num_args, *std::move(unknown_set)); + return; + } + } + + if (!error_pos.has_value()) { + frame.value_stack().PopAndPush(num_args, cel::BoolValue(!shortcircuit)); + return; + } + + cel::Value result = args[error_pos.value()]; + if (!result.IsError()) { + result = cel::ErrorValue(CreateNoMatchingOverloadError(op_name)); + } + frame.value_stack().PopAndPush(num_args, std::move(result)); +} std::unique_ptr CreateDirectAndStep( std::unique_ptr lhs, std::unique_ptr rhs, int64_t expr_id, bool shortcircuiting) { return CreateDirectLogicStep(std::move(lhs), std::move(rhs), expr_id, - OpType::kAnd, shortcircuiting); + BoolLogicKind::kAnd, shortcircuiting); } std::unique_ptr CreateDirectOrStep( @@ -448,17 +403,7 @@ std::unique_ptr CreateDirectOrStep( std::unique_ptr rhs, int64_t expr_id, bool shortcircuiting) { return CreateDirectLogicStep(std::move(lhs), std::move(rhs), expr_id, - OpType::kOr, shortcircuiting); -} - -// Factory method for "And" Execution step -std::unique_ptr CreateAndStep(size_t num_args) { - return std::make_unique(OpType::kAnd, num_args); -} - -// Factory method for "Or" Execution step -std::unique_ptr CreateOrStep(size_t num_args) { - return std::make_unique(OpType::kOr, num_args); + BoolLogicKind::kOr, shortcircuiting); } // Factory method for recursive logical not "!" Execution step @@ -467,11 +412,6 @@ std::unique_ptr CreateDirectNotStep( return std::make_unique(std::move(operand), expr_id); } -// Factory method for iterative logical not "!" Execution step -std::unique_ptr CreateNotStep() { - return std::make_unique(); -} - // Factory method for recursive logical "@not_strictly_false" Execution step. std::unique_ptr CreateDirectNotStrictlyFalseStep( std::unique_ptr operand, int64_t expr_id) { @@ -479,9 +419,4 @@ std::unique_ptr CreateDirectNotStrictlyFalseStep( expr_id); } -// Factory method for iterative logical "@not_strictly_false" Execution step. -std::unique_ptr CreateNotStrictlyFalseStep() { - return std::make_unique(); -} - } // namespace google::api::expr::runtime diff --git a/eval/eval/logic_step.h b/eval/eval/logic_step.h index 95f58b9db..401eaf2ca 100644 --- a/eval/eval/logic_step.h +++ b/eval/eval/logic_step.h @@ -1,15 +1,28 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_LOGIC_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_LOGIC_STEP_H_ +#include #include #include -#include "absl/status/statusor.h" #include "eval/eval/direct_expression_step.h" -#include "eval/eval/evaluator_core.h" namespace google::api::expr::runtime { +class ExecutionFrame; + +void EvaluateNotStep(ExecutionFrame& frame); + +void EvaluateNotStrictlyFalseStep(ExecutionFrame& frame); + +enum class BoolLogicKind { + kAnd, + kOr, +}; + +void EvaluateBoolLogicStep(BoolLogicKind kind, size_t num_args, + ExecutionFrame& frame); + // Factory method for "And" Execution step std::unique_ptr CreateDirectAndStep( std::unique_ptr lhs, @@ -22,26 +35,14 @@ std::unique_ptr CreateDirectOrStep( std::unique_ptr rhs, int64_t expr_id, bool shortcircuiting); -// Factory method for "And" Execution step -std::unique_ptr CreateAndStep(size_t num_args); - -// Factory method for "Or" Execution step -std::unique_ptr CreateOrStep(size_t num_args); - // Factory method for recursive logical not "!" Execution step std::unique_ptr CreateDirectNotStep( std::unique_ptr operand, int64_t expr_id); -// Factory method for iterative logical not "!" Execution step -std::unique_ptr CreateNotStep(); - // Factory method for recursive logical "@not_strictly_false" Execution step. std::unique_ptr CreateDirectNotStrictlyFalseStep( std::unique_ptr operand, int64_t expr_id); -// Factory method for iterative logical "@not_strictly_false" Execution step. -std::unique_ptr CreateNotStrictlyFalseStep(); - } // namespace google::api::expr::runtime #endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_LOGIC_STEP_H_ diff --git a/eval/eval/logic_step_test.cc b/eval/eval/logic_step_test.cc index bdb6ceed9..93f2fb888 100644 --- a/eval/eval/logic_step_test.cc +++ b/eval/eval/logic_step_test.cc @@ -69,9 +69,9 @@ class LogicStepTest : public testing::TestWithParam { ExecutionPath path; path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("name0"))); path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("name1"))); - path.push_back(ExpressionStep::MakeGenericStep( - (is_or) ? CreateOrStep(/*num_args=*/2) : CreateAndStep(/*num_args=*/2), - /*expr_id=*/2)); + path.push_back( + (is_or) ? ExpressionStep::MakeBooleanOrStep(/*num_args=*/2, /*id=*/2) + : ExpressionStep::MakeBooleanAndStep(/*num_args=*/2, /*id=*/2)); auto dummy_expr = std::make_unique(); cel::RuntimeOptions options; @@ -642,6 +642,45 @@ INSTANTIATE_TEST_SUITE_P( [](const testing::TestParamInfo& info) -> std::string { return info.param.name; }); +TEST(UnaryLogicStepTest, BooleanNot) { + ExecutionPath path; + path.push_back(ExpressionStep::MakeConstant(cel::BoolValue(true))); + path.push_back(ExpressionStep::MakeBooleanNotStep()); + + google::protobuf::Arena arena; + cel::runtime_internal::RuntimeTypeProvider type_provider( + cel::internal::GetTestingDescriptorPool()); + FlatExpressionEvaluatorState state( + 2, 0, type_provider, cel::internal::GetTestingDescriptorPool(), + cel::internal::GetTestingMessageFactory(), &arena); + cel::Activation activation; + cel::RuntimeOptions options; + ExecutionFrame frame(path, activation, options, state); + ASSERT_OK_AND_ASSIGN(cel::Value value, frame.Evaluate()); + ASSERT_TRUE(value.IsBool()); + EXPECT_FALSE(value.GetBool().NativeValue()); +} + +TEST(UnaryLogicStepTest, NotStrictlyFalse) { + ExecutionPath path; + path.push_back(ExpressionStep::MakeConstant( + cel::ErrorValue(absl::InternalError("error")))); + path.push_back(ExpressionStep::MakeNotStrictlyFalseStep()); + + google::protobuf::Arena arena; + cel::runtime_internal::RuntimeTypeProvider type_provider( + cel::internal::GetTestingDescriptorPool()); + FlatExpressionEvaluatorState state( + 2, 0, type_provider, cel::internal::GetTestingDescriptorPool(), + cel::internal::GetTestingMessageFactory(), &arena); + cel::Activation activation; + cel::RuntimeOptions options; + ExecutionFrame frame(path, activation, options, state); + ASSERT_OK_AND_ASSIGN(cel::Value value, frame.Evaluate()); + ASSERT_TRUE(value.IsBool()); + EXPECT_TRUE(value.GetBool().NativeValue()); +} + } // namespace } // namespace google::api::expr::runtime diff --git a/eval/eval/optional_or_step.cc b/eval/eval/optional_or_step.cc index e98c6ff16..5ac1e8400 100644 --- a/eval/eval/optional_or_step.cc +++ b/eval/eval/optional_or_step.cc @@ -21,16 +21,15 @@ #include "absl/base/optimization.h" #include "absl/status/status.h" -#include "absl/status/statusor.h" -#include "absl/types/optional.h" #include "absl/types/span.h" #include "common/casting.h" +#include "common/optional_ref.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 "eval/eval/jump_step.h" +#include "eval/eval/expression_step_logic.h" #include "internal/status_macros.h" #include "runtime/internal/errors.h" #include "google/protobuf/arena.h" @@ -60,62 +59,12 @@ ErrorValue MakeNoOverloadError(OptionalOrKind kind, google::protobuf::Arena* are ABSL_UNREACHABLE(); } -// Implements short-circuiting for optional.or. -// Expected layout if short-circuiting enabled: -// -// +--------+-----------------------+-------------------------------+ -// | idx | Step | Stack After | -// +--------+-----------------------+-------------------------------+ -// | 1 | | OptionalValue | -// +--------+-----------------------+-------------------------------+ -// | 2 | Jump to 5 if present | OptionalValue | -// +--------+-----------------------+-------------------------------+ -// | 3 | | OptionalValue, OptionalValue | -// +--------+-----------------------+-------------------------------+ -// | 4 | optional.or | OptionalValue | -// +--------+-----------------------+-------------------------------+ -// | 5 | | ... | -// +--------------------------------+-------------------------------+ -// -// If implementing the orValue variant, the jump step handles unwrapping ( -// getting the result of optional.value()) -class OptionalHasValueJumpStep final : public JumpStepBase { - public: - explicit OptionalHasValueJumpStep(OptionalOrKind kind) - : JumpStepBase(std::nullopt), kind_(kind) {} - - absl::Status Evaluate(ExecutionFrame* frame) const override { - if (!frame->value_stack().HasEnough(1)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); - } - const auto& value = frame->value_stack().Peek(); - auto optional_value = As(value); - // We jump if the receiver is `optional_type` which has a value or the - // receiver is an error/unknown. Unlike `_||_` we are not commutative. If - // we run into an error/unknown, we skip the `else` branch. - const bool should_jump = - (optional_value.has_value() && optional_value->HasValue()) || - (!optional_value.has_value() && (cel::InstanceOf(value) || - cel::InstanceOf(value))); - if (should_jump) { - if (kind_ == OptionalOrKind::kOrValue && optional_value.has_value()) { - frame->value_stack().PopAndPush(optional_value->Value()); - } - return Jump(frame); - } - return absl::OkStatus(); - } - - private: - const OptionalOrKind kind_; -}; - class OptionalOrStep : public ExpressionStepBase { public: explicit OptionalOrStep(OptionalOrKind kind) : ExpressionStepBase(), kind_(kind) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: const OptionalOrKind kind_; @@ -164,9 +113,10 @@ absl::Status EvalOptionalOr(OptionalOrKind kind, const Value& lhs, return absl::OkStatus(); } -absl::Status OptionalOrStep::Evaluate(ExecutionFrame* frame) const { +void OptionalOrStep::Evaluate(ExecutionFrame* frame) const { if (!frame->value_stack().HasEnough(2)) { - return absl::InternalError("Value stack underflow"); + frame->Abort(absl::InternalError("Value stack underflow")); + return; } absl::Span args = frame->value_stack().GetSpan(2); @@ -175,12 +125,15 @@ absl::Status OptionalOrStep::Evaluate(ExecutionFrame* frame) const { Value result; AttributeTrail result_attr; - CEL_RETURN_IF_ERROR(EvalOptionalOr(kind_, args[0], args[1], args_attr[0], - args_attr[1], result, result_attr, - frame->arena())); + if (absl::Status status = + EvalOptionalOr(kind_, args[0], args[1], args_attr[0], args_attr[1], + result, result_attr, frame->arena()); + !status.ok()) { + frame->Abort(std::move(status)); + return; + } frame->value_stack().PopAndPush(2, std::move(result), std::move(result_attr)); - return absl::OkStatus(); } class ExhaustiveDirectOptionalOrStep : public DirectExpressionStep { @@ -276,9 +229,36 @@ absl::Status DirectOptionalOrStep::Evaluate(ExecutionFrameBase& frame, } // namespace -std::unique_ptr CreateOptionalHasValueJumpStep(bool or_value) { - return std::make_unique( - or_value ? OptionalOrKind::kOrValue : OptionalOrKind::kOrOptional); +void OptionalHasValueJumpStep::Evaluate(ExecutionFrame* frame) const { + if (!frame->value_stack().HasEnough(1)) { + frame->Abort(absl::InternalError("Value stack underflow")); + return; + } + const Value& value = frame->value_stack().Peek(); + cel::optional_ref optional_value = + As(value); + // We jump if the receiver is `optional_type` which has a value or the + // receiver is an error/unknown. Unlike `_||_` we are not commutative. If + // we run into an error/unknown, we skip the `else` branch. + const bool should_jump = + (optional_value.has_value() && optional_value->HasValue()) || + (!optional_value.has_value() && (cel::InstanceOf(value) || + cel::InstanceOf(value))); + if (should_jump) { + if (is_or_value_ && optional_value.has_value()) { + frame->value_stack().PopAndPush(optional_value->Value()); + } + if (!jump_offset_.has_value()) { + frame->Abort(absl::InternalError("Jump offset not set")); + return; + } + frame->JumpToOrAbort(*jump_offset_); + } +} + +std::unique_ptr CreateOptionalHasValueJumpStep( + bool or_value) { + return std::make_unique(or_value); } std::unique_ptr CreateOptionalOrStep(bool is_or_value) { diff --git a/eval/eval/optional_or_step.h b/eval/eval/optional_or_step.h index 3f149e44b..f54057fa9 100644 --- a/eval/eval/optional_or_step.h +++ b/eval/eval/optional_or_step.h @@ -17,13 +17,29 @@ #include #include +#include +#include "absl/status/status.h" #include "eval/eval/direct_expression_step.h" #include "eval/eval/evaluator_core.h" -#include "eval/eval/jump_step.h" +#include "eval/eval/expression_step_base.h" namespace google::api::expr::runtime { +class OptionalHasValueJumpStep final : public ExpressionStepBase { + public: + explicit OptionalHasValueJumpStep(bool is_or_value) + : is_or_value_(is_or_value) {} + + void set_jump_offset(int offset) { jump_offset_ = offset; } + + void Evaluate(ExecutionFrame* frame) const override; + + private: + const bool is_or_value_; + std::optional jump_offset_; +}; + // Factory method for OptionalHasValueJump step, used to implement // short-circuiting optional.or and optional.orValue. // @@ -31,7 +47,8 @@ namespace google::api::expr::runtime { // true, performs a jump. If `or_value` is true and we are jumping, // `optional.value` is called and the result replaces the optional at the top of // the stack. -std::unique_ptr CreateOptionalHasValueJumpStep(bool or_value); +std::unique_ptr CreateOptionalHasValueJumpStep( + bool or_value); // Factory method for OptionalOr step, used to implement optional.or and // optional.orValue. diff --git a/eval/eval/regex_match_step.cc b/eval/eval/regex_match_step.cc index abc301225..d13e4f651 100644 --- a/eval/eval/regex_match_step.cc +++ b/eval/eval/regex_match_step.cc @@ -62,23 +62,22 @@ class RegexMatchStep final : public ExpressionStepBase { explicit RegexMatchStep(std::shared_ptr re2) : ExpressionStepBase(), re2_(std::move(re2)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(kNumRegexMatchArguments)) { - return absl::Status(absl::StatusCode::kInternal, - "Insufficient arguments supplied for regular " - "expression match"); + frame->Abort(absl::InternalError( + "Insufficient arguments supplied for regular expression match")); + return; } auto input_args = frame->value_stack().GetSpan(kNumRegexMatchArguments); const auto& subject = input_args[kRegexMatchStepSubject]; if (!subject->Is()) { - return absl::Status(absl::StatusCode::kInternal, - "First argument for regular " - "expression match must be a string"); + frame->Abort(absl::InternalError( + "First argument for regular expression match must be a string")); + return; } bool match = subject.GetString().NativeValue(MatchesVisitor{*re2_}); frame->value_stack().Pop(kNumRegexMatchArguments); frame->value_stack().Push(cel::BoolValue(match)); - return absl::OkStatus(); } private: diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index 528f12d0f..1d6c337d6 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -215,7 +215,7 @@ class SelectStep : public ExpressionStepBase { test_field_presence_(test_field_presence), enable_optional_types_(enable_optional_types) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; protected: std::string field_; @@ -224,10 +224,11 @@ class SelectStep : public ExpressionStepBase { bool enable_optional_types_; }; -absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { +void SelectStep::Evaluate(ExecutionFrame* frame) const { if (!frame->value_stack().HasEnough(1)) { - return absl::Status(absl::StatusCode::kInternal, - "No arguments supplied for Select-type expression"); + frame->Abort(absl::InternalError( + "No arguments supplied for Select-type expression")); + return; } const Value& arg = frame->value_stack().Peek(); @@ -235,7 +236,7 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { if (arg.IsUnknown() || arg.IsError()) { // Bubble up unknowns and errors. - return absl::OkStatus(); + return; } AttributeTrail result_trail; @@ -255,7 +256,7 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { frame->value_stack().PopAndPush( cel::ErrorValue::From(InvalidSelectTargetError(), frame->arena()), std::move(result_trail)); - return absl::OkStatus(); + return; } absl::optional marked_attribute_check = @@ -263,7 +264,7 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { if (marked_attribute_check.has_value()) { frame->value_stack().PopAndPush(std::move(marked_attribute_check).value(), std::move(result_trail)); - return absl::OkStatus(); + return; } Value result; @@ -273,28 +274,32 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { if (!optional_arg->HasValue()) { frame->value_stack().PopAndPush(cel::BoolValue{false}, std::move(result_trail)); - return absl::OkStatus(); + return; } optional_arg->Value(&result); target = &result; } - CEL_RETURN_IF_ERROR( - PerformHas(*target, field_, cel::StringValue::WrapUnsafe(field_), - frame->descriptor_pool(), frame->message_factory(), - frame->arena(), result)); + if (absl::Status status = + PerformHas(*target, field_, cel::StringValue::WrapUnsafe(field_), + frame->descriptor_pool(), frame->message_factory(), + frame->arena(), result); + !status.ok()) { + frame->Abort(std::move(status)); + return; + } frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); - return absl::OkStatus(); + return; } if (optional_arg) { if (!optional_arg->HasValue()) { frame->value_stack().PopAndPush(OptionalValue::None(), std::move(result_trail)); - return absl::OkStatus(); + return; } Value value; optional_arg->Value(&value); - auto status = PerformOptionalGet( + absl::Status status = PerformOptionalGet( value, field_, cel::StringValue::WrapUnsafe(field_), unboxing_option_, frame->descriptor_pool(), frame->message_factory(), frame->arena(), frame->options().enable_use_new_field_select_implementation, result); @@ -302,15 +307,18 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { result = ErrorValue::From(std::move(status), frame->arena()); } frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); - return absl::OkStatus(); + return; } - CEL_RETURN_IF_ERROR(PerformGet( - arg, field_, cel::StringValue::WrapUnsafe(field_), unboxing_option_, - frame->descriptor_pool(), frame->message_factory(), frame->arena(), - frame->options().enable_use_new_field_select_implementation, result)); + if (absl::Status status = PerformGet( + arg, field_, cel::StringValue::WrapUnsafe(field_), unboxing_option_, + frame->descriptor_pool(), frame->message_factory(), frame->arena(), + frame->options().enable_use_new_field_select_implementation, result); + !status.ok()) { + frame->Abort(std::move(status)); + return; + } frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); - return absl::OkStatus(); } class DirectSelectStep : public DirectExpressionStep { @@ -473,10 +481,11 @@ class ProtoSelectStep : public SelectStep { ABSL_DCHECK(field_descriptor_ != nullptr); } - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(1)) { - return absl::InternalError( - "No arguments supplied for Select-type expression"); + frame->Abort(absl::InternalError( + "No arguments supplied for Select-type expression")); + return; } const Value& arg = frame->value_stack().Peek(); @@ -484,7 +493,8 @@ class ProtoSelectStep : public SelectStep { unwrapped.has_value() && SupportsCachedFieldDescriptor(*unwrapped, descriptor_, field_descriptor_)) { - return EvaluateMessageFieldGet(frame, *unwrapped); + EvaluateMessageFieldGet(frame, *unwrapped); + return; } else if (const google::protobuf::Message* legacy_message = cel::interop_internal::GetLegacyMessage(arg); frame->options().enable_use_new_field_select_implementation && @@ -494,19 +504,20 @@ class ProtoSelectStep : public SelectStep { // can minimize back and forth interop conversions. if (SupportsCachedFieldDescriptor(parsed_message, descriptor_, field_descriptor_)) { - return EvaluateMessageFieldGet(frame, legacy_message); + EvaluateMessageFieldGet(frame, legacy_message); + return; } } // If we get an unexpected value type, fall back to the generic // implementation. - return SelectStep::Evaluate(frame); + SelectStep::Evaluate(frame); } private: - absl::Status EvaluateMessageFieldGet( + void EvaluateMessageFieldGet( ExecutionFrame* frame, const cel::ParsedMessageValue& parsed_message) const; - absl::Status EvaluateMessageFieldGet( + void EvaluateMessageFieldGet( ExecutionFrame* frame, const google::protobuf::Message* absl_nonnull legacy_message) const; @@ -514,28 +525,35 @@ class ProtoSelectStep : public SelectStep { const google::protobuf::FieldDescriptor* field_descriptor_; }; -absl::Status ProtoSelectStep::EvaluateMessageFieldGet( +void ProtoSelectStep::EvaluateMessageFieldGet( ExecutionFrame* frame, const cel::ParsedMessageValue& parsed_message) const { if (CheckAttributeTrail(field_, frame)) { - return absl::OkStatus(); + return; + } + if (absl::Status status = parsed_message.GetField( + field_descriptor_, unboxing_option_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), + &frame->value_stack().Peek()); + !status.ok()) { + frame->Abort(std::move(status)); } - return parsed_message.GetField( - field_descriptor_, unboxing_option_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &frame->value_stack().Peek()); } -absl::Status ProtoSelectStep::EvaluateMessageFieldGet( +void ProtoSelectStep::EvaluateMessageFieldGet( ExecutionFrame* frame, const google::protobuf::Message* absl_nonnull legacy_message) const { ABSL_DCHECK(legacy_message != nullptr); if (CheckAttributeTrail(field_, frame)) { - return absl::OkStatus(); + return; + } + if (absl::Status status = cel::interop_internal::WrapLegacyMessageField( + legacy_message, field_descriptor_, unboxing_option_, + frame->descriptor_pool(), frame->message_factory(), frame->arena(), + &frame->value_stack().Peek()); + !status.ok()) { + frame->Abort(std::move(status)); } - return cel::interop_internal::WrapLegacyMessageField( - legacy_message, field_descriptor_, unboxing_option_, - frame->descriptor_pool(), frame->message_factory(), frame->arena(), - &frame->value_stack().Peek()); } class ProtoHasStep : public SelectStep { @@ -551,10 +569,11 @@ class ProtoHasStep : public SelectStep { ABSL_DCHECK(field_descriptor_ != nullptr); } - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(1)) { - return absl::InternalError( - "No arguments supplied for Select-type expression"); + frame->Abort(absl::InternalError( + "No arguments supplied for Select-type expression")); + return; } const Value& arg = frame->value_stack().Peek(); @@ -562,7 +581,8 @@ class ProtoHasStep : public SelectStep { unwrapped.has_value() && SupportsCachedFieldDescriptor(*unwrapped, descriptor_, field_descriptor_)) { - return EvaluateHas(frame, *unwrapped); + EvaluateHas(frame, *unwrapped); + return; } else if (const google::protobuf::Message* legacy_message = cel::interop_internal::GetLegacyMessage(arg); legacy_message != nullptr) { @@ -570,31 +590,31 @@ class ProtoHasStep : public SelectStep { cel::UnsafeParsedMessageValue(legacy_message); if (SupportsCachedFieldDescriptor(parsed_message, descriptor_, field_descriptor_)) { - return EvaluateHas(frame, parsed_message); + EvaluateHas(frame, parsed_message); + return; } } // If we get an unexpected value type, fall back to the generic // implementation. - return SelectStep::Evaluate(frame); + SelectStep::Evaluate(frame); } private: - absl::Status EvaluateHas(ExecutionFrame* frame, - const cel::ParsedMessageValue& parsed_message) const; + void EvaluateHas(ExecutionFrame* frame, + const cel::ParsedMessageValue& parsed_message) const; const google::protobuf::Descriptor* descriptor_; const google::protobuf::FieldDescriptor* field_descriptor_; }; -absl::Status ProtoHasStep::EvaluateHas( +void ProtoHasStep::EvaluateHas( ExecutionFrame* frame, const cel::ParsedMessageValue& parsed_message) const { if (CheckAttributeTrail(field_, frame)) { - return absl::OkStatus(); + return; } frame->value_stack().Peek() = BoolValue{parsed_message.HasField(field_descriptor_)}; - return absl::OkStatus(); } } // namespace diff --git a/eval/eval/shadowable_value_step.cc b/eval/eval/shadowable_value_step.cc index 0b75f7cc2..b6199ed7e 100644 --- a/eval/eval/shadowable_value_step.cc +++ b/eval/eval/shadowable_value_step.cc @@ -29,25 +29,27 @@ class ShadowableValueStep : public ExpressionStepBase { identifier_(std::move(identifier)), value_(std::move(value)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: std::string identifier_; Value value_; }; -absl::Status ShadowableValueStep::Evaluate(ExecutionFrame* frame) const { +void ShadowableValueStep::Evaluate(ExecutionFrame* frame) const { cel::Value result; - CEL_ASSIGN_OR_RETURN(auto found, - frame->modern_activation().FindVariable( - identifier_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result)); - if (found) { + absl::StatusOr found = frame->modern_activation().FindVariable( + identifier_, frame->descriptor_pool(), frame->message_factory(), + frame->arena(), &result); + if (!found.ok()) { + frame->Abort(std::move(found).status()); + return; + } + if (*found) { frame->value_stack().Push(std::move(result)); } else { frame->value_stack().Push(value_); } - return absl::OkStatus(); } class DirectShadowableValueStep : public DirectExpressionStep { diff --git a/eval/eval/ternary_step.cc b/eval/eval/ternary_step.cc index 97b064d22..0924b915f 100644 --- a/eval/eval/ternary_step.cc +++ b/eval/eval/ternary_step.cc @@ -128,13 +128,14 @@ class TernaryStep : public ExpressionStepBase { // Constructs FunctionStep that uses overloads specified. TernaryStep() : ExpressionStepBase() {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; }; -absl::Status TernaryStep::Evaluate(ExecutionFrame* frame) const { +void TernaryStep::Evaluate(ExecutionFrame* frame) const { // Must have 3 or more values on the stack. if (!frame->value_stack().HasEnough(3)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); + frame->Abort(absl::InternalError("Value stack underflow")); + return; } // Create Span object that contains input arguments to the function. @@ -148,13 +149,13 @@ absl::Status TernaryStep::Evaluate(ExecutionFrame* frame) const { // Check if unknown? if (condition.IsUnknown()) { frame->value_stack().Pop(2); - return absl::OkStatus(); + return; } } if (condition.IsError()) { frame->value_stack().Pop(2); - return absl::OkStatus(); + return; } cel::Value result; @@ -168,8 +169,6 @@ absl::Status TernaryStep::Evaluate(ExecutionFrame* frame) const { } frame->value_stack().PopAndPush(args.size(), std::move(result)); - - return absl::OkStatus(); } } // namespace diff --git a/extensions/select_optimization.cc b/extensions/select_optimization.cc index 60d414de3..7bca6414c 100644 --- a/extensions/select_optimization.cc +++ b/extensions/select_optimization.cc @@ -755,7 +755,7 @@ class StackMachineImpl : public ExpressionStepBase { StackMachineImpl(int expr_id, OptimizedSelectImpl impl) : ExpressionStepBase(expr_id), impl_(std::move(impl)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: // Get the effective attribute for the optimized select expression. @@ -772,7 +772,7 @@ AttributeTrail StackMachineImpl::GetAttributeTrail( return impl_.GetAttributeTrail(attr); } -absl::Status StackMachineImpl::Evaluate(ExecutionFrame* frame) const { +void StackMachineImpl::Evaluate(ExecutionFrame* frame) const { // Default empty. AttributeTrail attribute_trail; // TODO(uncreated-issue/51): add support for variable qualifiers and string literal @@ -784,7 +784,7 @@ absl::Status StackMachineImpl::Evaluate(ExecutionFrame* frame) const { if (operand->Is() || operand->Is()) { // Just forward the error which is already top of stack. - return absl::OkStatus(); + return; } if (frame->enable_attribute_tracking()) { @@ -793,27 +793,34 @@ absl::Status StackMachineImpl::Evaluate(ExecutionFrame* frame) const { // select arguments. // TODO(uncreated-issue/51): add support variable qualifiers attribute_trail = GetAttributeTrail(frame); - CEL_ASSIGN_OR_RETURN(std::optional value, - CheckForMarkedAttributes(*frame, attribute_trail)); - if (value.has_value()) { + absl::StatusOr> value = + CheckForMarkedAttributes(*frame, attribute_trail); + if (!value.ok()) { + frame->Abort(std::move(value).status()); + return; + } + if (value->has_value()) { frame->value_stack().Pop(kStackInputs); - frame->value_stack().Push(std::move(value).value(), + frame->value_stack().Push(std::move(*value).value(), std::move(attribute_trail)); - return absl::OkStatus(); + return; } } if (!operand->Is()) { - return absl::InvalidArgumentError( - "Expected struct type for select optimization."); + frame->Abort(absl::InvalidArgumentError( + "Expected struct type for select optimization.")); + return; } - CEL_ASSIGN_OR_RETURN(Value result, - impl_.ApplySelect(*frame, operand.GetStruct())); + absl::StatusOr result = impl_.ApplySelect(*frame, operand.GetStruct()); + if (!result.ok()) { + frame->Abort(std::move(result).status()); + return; + } frame->value_stack().Pop(kStackInputs); - frame->value_stack().Push(std::move(result), std::move(attribute_trail)); - return absl::OkStatus(); + frame->value_stack().Push(*std::move(result), std::move(attribute_trail)); } class RecursiveImpl : public DirectExpressionStep {