From 74e5700ec9e03ffb85ba6b40fc550a467119992f Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Fri, 18 Sep 2026 11:34:26 -0700 Subject: [PATCH] Special case boolean logic built-ins. PiperOrigin-RevId: 983973588 --- eval/compiler/BUILD | 3 - .../cel_expression_builder_flat_impl.cc | 7 +- eval/compiler/constant_folding.cc | 6 +- eval/compiler/constant_folding_test.cc | 130 ++++---- eval/compiler/flat_expr_builder.cc | 149 +++++---- eval/compiler/flat_expr_builder_extensions.cc | 37 ++- eval/compiler/flat_expr_builder_extensions.h | 32 +- .../flat_expr_builder_extensions_test.cc | 73 +++-- eval/compiler/instrumentation.cc | 3 +- .../regex_precompilation_optimization.cc | 17 +- eval/eval/BUILD | 69 ++--- eval/eval/cel_expression_flat_impl.cc | 9 +- eval/eval/cel_expression_flat_impl.h | 2 +- eval/eval/compiler_constant_step.cc | 8 +- eval/eval/compiler_constant_step.h | 24 -- eval/eval/compiler_constant_step_test.cc | 38 +-- eval/eval/comprehension_slots.h | 7 +- eval/eval/comprehension_step.cc | 10 +- eval/eval/comprehension_step.h | 16 +- eval/eval/comprehension_step_test.cc | 46 ++- eval/eval/const_value_step.h | 8 - eval/eval/container_access_step.cc | 11 +- eval/eval/container_access_step.h | 5 +- eval/eval/container_access_step_test.cc | 27 +- eval/eval/create_list_step.cc | 25 +- eval/eval/create_list_step.h | 6 +- eval/eval/create_list_step_test.cc | 48 ++- eval/eval/create_map_step.cc | 22 +- eval/eval/create_map_step.h | 9 +- eval/eval/create_map_step_test.cc | 22 +- eval/eval/create_struct_step.cc | 12 +- eval/eval/create_struct_step.h | 4 +- eval/eval/create_struct_step_test.cc | 26 +- eval/eval/direct_expression_step.cc | 16 +- eval/eval/direct_expression_step.h | 21 +- eval/eval/equality_steps.cc | 14 +- eval/eval/equality_steps.h | 5 +- eval/eval/equality_steps_test.cc | 51 ++-- eval/eval/evaluator_core.cc | 214 ++++++++++++- eval/eval/evaluator_core.h | 289 ++++++++++++++++-- eval/eval/evaluator_core_test.cc | 137 +++++++-- eval/eval/expression_step_base.h | 9 +- eval/eval/function_step.cc | 10 +- eval/eval/function_step.h | 4 +- eval/eval/function_step_test.cc | 63 ++-- eval/eval/ident_step.cc | 19 +- eval/eval/ident_step.h | 7 +- eval/eval/ident_step_test.cc | 45 +-- eval/eval/jump_step.cc | 33 +- eval/eval/jump_step.h | 14 +- eval/eval/lazy_init_step.cc | 127 ++------ eval/eval/lazy_init_step.h | 31 +- eval/eval/lazy_init_step_test.cc | 45 ++- eval/eval/logic_step.cc | 261 ++++++---------- eval/eval/logic_step.h | 31 +- eval/eval/logic_step_test.cc | 54 +++- eval/eval/optional_or_step.cc | 17 +- eval/eval/optional_or_step.h | 6 +- eval/eval/regex_match_step.cc | 11 +- eval/eval/regex_match_step.h | 5 +- eval/eval/select_step.cc | 33 +- eval/eval/select_step.h | 10 +- eval/eval/select_step_test.cc | 85 +++--- eval/eval/shadowable_value_step.cc | 10 +- eval/eval/shadowable_value_step.h | 4 +- eval/eval/shadowable_value_step_test.cc | 21 +- eval/eval/ternary_step.cc | 7 +- eval/eval/ternary_step.h | 3 +- eval/eval/ternary_step_test.cc | 79 ++--- eval/public/cel_expression.h | 7 + extensions/select_optimization.cc | 6 +- runtime/internal/runtime_impl.cc | 10 +- runtime/runtime.h | 7 + 73 files changed, 1550 insertions(+), 1182 deletions(-) diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index dbd8d9c8b..032a12c74 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -123,8 +123,6 @@ cc_library( "//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", @@ -360,7 +358,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/cel_expression_builder_flat_impl.cc b/eval/compiler/cel_expression_builder_flat_impl.cc index 98ecc6aae..830b5ff1d 100644 --- a/eval/compiler/cel_expression_builder_flat_impl.cc +++ b/eval/compiler/cel_expression_builder_flat_impl.cc @@ -100,7 +100,12 @@ CelExpressionBuilderFlatImpl::CreateExpressionImpl( !impl.subexpressions().empty() && // mainline expression is exactly one recursive step. impl.subexpressions().front().size() == 1 && - impl.subexpressions().front().front()->GetNativeTypeId() == + impl.subexpressions().front().front().IsGenericStep() && + impl.subexpressions() + .front() + .front() + .GetGenericStep() + ->GetNativeTypeId() == cel::NativeTypeId::For()) { return CelExpressionRecursiveImpl::Create(env_, std::move(impl)); } diff --git a/eval/compiler/constant_folding.cc b/eval/compiler/constant_folding.cc index fa12a8e6d..071f16186 100644 --- a/eval/compiler/constant_folding.cc +++ b/eval/compiler/constant_folding.cc @@ -50,11 +50,11 @@ 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; using ::google::api::expr::runtime::ExecutionPathView; +using ::google::api::expr::runtime::ExpressionStep; using ::google::api::expr::runtime::FlatExpressionEvaluatorState; using ::google::api::expr::runtime::PlannerContext; using ::google::api::expr::runtime::ProgramOptimizer; @@ -242,9 +242,7 @@ absl::Status ConstantFoldingExtension::OnPostVisit(PlannerContext& context, // Otherwise make a stack machine plan. ExecutionPath new_plan; - CEL_ASSIGN_OR_RETURN( - new_plan.emplace_back(), - CreateConstValueStep(std::move(value), node.id(), false)); + 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 d1c0c31e0..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,10 +57,10 @@ 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; +using ::google::api::expr::runtime::ExpressionStep; using ::google::api::expr::runtime::PlannerContext; using ::google::api::expr::runtime::ProgramBuilder; using ::google::api::expr::runtime::ProgramOptimizer; @@ -115,26 +114,25 @@ TEST_F(UpdatedConstantFoldingTest, SkipsTernary) { program_builder.EnterSubexpression(&call); // condition program_builder.EnterSubexpression(&condition); - ASSERT_OK_AND_ASSIGN(auto step, - CreateConstValueStep(cel::BoolValue(true), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), condition.id())); program_builder.ExitSubexpression(&condition); // true program_builder.EnterSubexpression(&true_branch); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::BoolValue(true), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), true_branch.id())); program_builder.ExitSubexpression(&true_branch); // false program_builder.EnterSubexpression(&false_branch); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::BoolValue(true), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), false_branch.id())); program_builder.ExitSubexpression(&false_branch); // ternary. - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::NullValue(), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::NullValue(), call.id())); program_builder.ExitSubexpression(&call); std::shared_ptr arena; @@ -179,21 +177,20 @@ TEST_F(UpdatedConstantFoldingTest, SkipsOr) { // left program_builder.EnterSubexpression(&left_condition); - ASSERT_OK_AND_ASSIGN(auto step, - CreateConstValueStep(cel::BoolValue(false), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(false), left_condition.id())); program_builder.ExitSubexpression(&left_condition); // right program_builder.EnterSubexpression(&right_condition); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::BoolValue(true), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), right_condition.id())); program_builder.ExitSubexpression(&right_condition); // op // Just a placeholder. - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::NullValue(), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::NullValue(), call.id())); program_builder.ExitSubexpression(&call); std::shared_ptr arena; @@ -235,21 +232,20 @@ TEST_F(UpdatedConstantFoldingTest, SkipsAnd) { // left program_builder.EnterSubexpression(&left_condition); - ASSERT_OK_AND_ASSIGN(auto step, - CreateConstValueStep(cel::BoolValue(true), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), left_condition.id())); program_builder.ExitSubexpression(&left_condition); // right program_builder.EnterSubexpression(&right_condition); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::BoolValue(false), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep(ExpressionStep::MakeConstant(cel::BoolValue(false), + right_condition.id())); program_builder.ExitSubexpression(&right_condition); // op // Just a placeholder. - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::NullValue(), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::NullValue(), call.id())); program_builder.ExitSubexpression(&call); std::shared_ptr arena; @@ -291,19 +287,21 @@ TEST_F(UpdatedConstantFoldingTest, CreatesList) { // elem one program_builder.EnterSubexpression(&elem_one); - ASSERT_OK_AND_ASSIGN(auto step, CreateConstValueStep(cel::IntValue(1L), 1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(1L), elem_one.id())); program_builder.ExitSubexpression(&elem_one); // elem two program_builder.EnterSubexpression(&elem_two); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::IntValue(2L), 2)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(2L), elem_two.id())); program_builder.ExitSubexpression(&elem_two); // createlist - ASSERT_OK_AND_ASSIGN(step, CreateCreateListStep(create_list.list_expr(), 3)); - program_builder.AddStep(std::move(step)); + ASSERT_OK_AND_ASSIGN(auto step, + CreateCreateListStep(create_list.list_expr())); + program_builder.AddStep( + ExpressionStep::MakeGenericStep(std::move(step), create_list.id())); program_builder.ExitSubexpression(&create_list); std::shared_ptr arena; @@ -349,37 +347,39 @@ TEST_F(UpdatedConstantFoldingTest, CreatesLargeList) { // 0 ASSERT_TRUE(program_builder.EnterSubexpression(&elem0) != nullptr); - ASSERT_OK_AND_ASSIGN(auto step, CreateConstValueStep(cel::IntValue(1L), 1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(1L), elem0.id())); program_builder.ExitSubexpression(&elem0); // 1 ASSERT_TRUE(program_builder.EnterSubexpression(&elem1)); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::IntValue(2L), 2)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(2L), elem1.id())); program_builder.ExitSubexpression(&elem1); // 2 ASSERT_TRUE(program_builder.EnterSubexpression(&elem2) != nullptr); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::IntValue(3L), 3)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(3L), elem2.id())); program_builder.ExitSubexpression(&elem2); // 3 ASSERT_TRUE(program_builder.EnterSubexpression(&elem3) != nullptr); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::IntValue(4L), 4)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(4L), elem3.id())); program_builder.ExitSubexpression(&elem3); // 4 ASSERT_TRUE(program_builder.EnterSubexpression(&elem4) != nullptr); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::IntValue(5L), 5)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(5L), elem4.id())); program_builder.ExitSubexpression(&elem4); // createlist - ASSERT_OK_AND_ASSIGN(step, CreateCreateListStep(create_list.list_expr(), 6)); - program_builder.AddStep(std::move(step)); + ASSERT_OK_AND_ASSIGN(auto step_large, + CreateCreateListStep(create_list.list_expr())); + program_builder.AddStep( + ExpressionStep::MakeGenericStep(std::move(step_large), create_list.id())); program_builder.ExitSubexpression(&create_list); std::shared_ptr arena; @@ -426,21 +426,22 @@ TEST_F(UpdatedConstantFoldingTest, CreatesMap) { // key program_builder.EnterSubexpression(&key); - ASSERT_OK_AND_ASSIGN(auto step, CreateConstValueStep(cel::IntValue(1L), 1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(1L), key.id())); program_builder.ExitSubexpression(&key); // value program_builder.EnterSubexpression(&value); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::IntValue(2L), 2)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(2L), value.id())); program_builder.ExitSubexpression(&value); // create map ASSERT_OK_AND_ASSIGN( - step, CreateCreateStructStepForMap(create_map.map_expr().entries().size(), - {}, 3)); - program_builder.AddStep(std::move(step)); + auto step_map, + CreateCreateStructStepForMap(create_map.map_expr().entries().size(), {})); + program_builder.AddStep( + ExpressionStep::MakeGenericStep(std::move(step_map), create_map.id())); program_builder.ExitSubexpression(&create_map); std::shared_ptr arena; @@ -481,22 +482,22 @@ TEST_F(UpdatedConstantFoldingTest, CreatesInvalidMap) { // key program_builder.EnterSubexpression(&key); - ASSERT_OK_AND_ASSIGN(auto step, - CreateConstValueStep(cel::DoubleValue(1.0), 1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::DoubleValue(1.0), key.id())); program_builder.ExitSubexpression(&key); // value program_builder.EnterSubexpression(&value); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::IntValue(2L), 2)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::IntValue(2L), value.id())); program_builder.ExitSubexpression(&value); // create map ASSERT_OK_AND_ASSIGN( - step, CreateCreateStructStepForMap(create_map.map_expr().entries().size(), - {}, 3)); - program_builder.AddStep(std::move(step)); + auto step_invalid_map, + CreateCreateStructStepForMap(create_map.map_expr().entries().size(), {})); + program_builder.AddStep(ExpressionStep::MakeGenericStep( + std::move(step_invalid_map), create_map.id())); program_builder.ExitSubexpression(&create_map); std::shared_ptr arena; @@ -536,21 +537,20 @@ TEST_F(UpdatedConstantFoldingTest, ErrorsOnUnexpectedOrder) { program_builder.EnterSubexpression(&call); // left program_builder.EnterSubexpression(&left_condition); - ASSERT_OK_AND_ASSIGN(auto step, - CreateConstValueStep(cel::BoolValue(true), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep( + ExpressionStep::MakeConstant(cel::BoolValue(true), left_condition.id())); program_builder.ExitSubexpression(&left_condition); // right program_builder.EnterSubexpression(&right_condition); - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::BoolValue(false), -1)); - program_builder.AddStep(std::move(step)); + program_builder.AddStep(ExpressionStep::MakeConstant(cel::BoolValue(false), + right_condition.id())); program_builder.ExitSubexpression(&right_condition); // op // Just a placeholder. - ASSERT_OK_AND_ASSIGN(step, CreateConstValueStep(cel::NullValue(), -1)); - program_builder.AddStep(std::move(step)); + 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 53f7cf0c4..0581ebcd3 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -730,8 +730,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 +855,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 +865,8 @@ 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(CreateIdentStepForSlot(ident_expr.name(), slot.slot), + expr.id()); } return; } @@ -918,8 +918,8 @@ class FlatExprVisitor : public cel::AstVisitor { 1); return; } - AddStep(CreateShadowableValueStep(name, std::move(const_value).value(), - select_root_id)); + AddStep(CreateShadowableValueStep(name, std::move(const_value).value()), + select_root_id); return; } @@ -927,7 +927,7 @@ class FlatExprVisitor : public cel::AstVisitor { if (options_.max_recursion_depth != 0) { SetRecursiveStep(CreateDirectIdentStep(ident_name, expr.id()), 1); } else { - AddStep(CreateIdentStep(ident_name, expr.id())); + AddStep(CreateIdentStep(ident_name), expr.id()); } } @@ -1032,15 +1032,18 @@ class FlatExprVisitor : public cel::AstVisitor { } if (field_type.has_value()) { - AddStep(CreateTypedSelectStep( - std::move(field), *struct_type, *std::move(field_type), - select_expr.test_only(), expr.id(), - options_.enable_empty_wrapper_null_unboxing, enable_optional_types_)); + AddStep( + CreateTypedSelectStep(std::move(field), *struct_type, + *std::move(field_type), select_expr.test_only(), + options_.enable_empty_wrapper_null_unboxing, + enable_optional_types_), + expr.id()); return; } - AddStep(CreateSelectStep( - std::move(field), select_expr.test_only(), expr.id(), - options_.enable_empty_wrapper_null_unboxing, enable_optional_types_)); + AddStep(CreateSelectStep(std::move(field), select_expr.test_only(), + options_.enable_empty_wrapper_null_unboxing, + enable_optional_types_), + expr.id()); } // Call node handler group. @@ -1577,7 +1580,7 @@ class FlatExprVisitor : public cel::AstVisitor { SetRecursiveStep(CreateDirectMutableListStep(expr.id()), 1); return; } - AddStep(CreateMutableListStep(expr.id())); + AddStep(CreateMutableListStep(), expr.id()); return; } if (GetOptimizableListAppendOperand(comprehension.comprehension) == @@ -1598,7 +1601,7 @@ class FlatExprVisitor : public cel::AstVisitor { SetRecursiveStep(std::move(step), *depth + 1); return; } - AddStep(CreateCreateListStep(list_expr, expr.id())); + AddStep(CreateCreateListStep(list_expr), expr.id()); } // CreateStruct node handler. @@ -1635,8 +1638,8 @@ class FlatExprVisitor : public cel::AstVisitor { } AddStep(CreateCreateStructStep(std::move(resolved_name), std::move(fields), - MakeOptionalIndicesSet(struct_expr), - expr.id())); + MakeOptionalIndicesSet(struct_expr)), + expr.id()); } void PostVisitMap(const cel::Expr& expr, @@ -1655,7 +1658,7 @@ class FlatExprVisitor : public cel::AstVisitor { SetRecursiveStep(CreateDirectMutableMapStep(expr.id()), 1); return; } - AddStep(CreateMutableMapStep(expr.id())); + AddStep(CreateMutableMapStep(), expr.id()); return; } } @@ -1674,8 +1677,8 @@ class FlatExprVisitor : public cel::AstVisitor { return; } AddStep(CreateCreateStructStepForMap(map_expr.entries().size(), - MakeOptionalIndicesSet(map_expr), - expr.id())); + MakeOptionalIndicesSet(map_expr)), + expr.id()); } absl::Status progress_status() const { return progress_status_; } @@ -1708,8 +1711,9 @@ class FlatExprVisitor : public cel::AstVisitor { *depth + 1); return; } - AddStep(CreateFunctionStep(*call_expr, expr->id(), - std::move(lazy_overloads))); + AddStep( + CreateFunctionStep(*call_expr, expr->id(), std::move(lazy_overloads)), + expr->id()); return; } @@ -1743,7 +1747,8 @@ class FlatExprVisitor : public cel::AstVisitor { *recursion_depth + 1); return; } - AddStep(CreateFunctionStep(*call_expr, expr->id(), std::move(overloads))); + AddStep(CreateFunctionStep(*call_expr, expr->id(), std::move(overloads)), + expr->id()); } // Add a step to the program, taking ownership. If successful, returns the @@ -1752,21 +1757,32 @@ class FlatExprVisitor : public cel::AstVisitor { // Note: the pointer is only guaranteed to stay valid until the parent // subexpression is finalized. Optimizers may modify the program plan which // may free the step at that point. - ExpressionStep* AddStep( - absl::StatusOr> step) { + template + std::enable_if_t, T*> AddStep( + std::unique_ptr step, int64_t expr_id = -1) { + if (progress_status_.ok() && !PlanningSuppressed()) { + T* ptr = step.get(); + program_builder_.AddStep( + ExpressionStep::MakeGenericStep(std::move(step), expr_id)); + return ptr; + } + return nullptr; + } + + template + std::enable_if_t, T*> AddStep( + absl::StatusOr> step, int64_t expr_id = -1) { if (step.ok()) { - return AddStep(*std::move(step)); + return AddStep(*std::move(step), expr_id); } else { SetProgressStatusIfError(step.status()); } return nullptr; } - template - std::enable_if_t, T*> AddStep( - std::unique_ptr step) { + ExpressionStep* AddStep(ExpressionStep step) { if (progress_status_.ok() && !PlanningSuppressed()) { - return static_cast(program_builder_.AddStep(std::move(step))); + return program_builder_.AddStep(std::move(step)); } return nullptr; } @@ -2026,8 +2042,8 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleIndex( *depth + 1); return CallHandlerResult::kIntercepted; } - AddStep( - CreateContainerAccessStep(call_expr, expr.id(), enable_optional_types_)); + AddStep(CreateContainerAccessStep(call_expr, enable_optional_types_), + expr.id()); return CallHandlerResult::kIntercepted; } @@ -2051,7 +2067,7 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleNot( *depth + 1); return CallHandlerResult::kIntercepted; } - AddStep(CreateNotStep(expr.id())); + AddStep(ExpressionStep::MakeBooleanNotStep(expr.id())); return CallHandlerResult::kIntercepted; } @@ -2076,7 +2092,7 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleNotStrictlyFalse( *depth + 1); return CallHandlerResult::kIntercepted; } - AddStep(CreateNotStrictlyFalseStep(expr.id())); + AddStep(ExpressionStep::MakeNotStrictlyFalseStep(expr.id())); return CallHandlerResult::kIntercepted; } @@ -2118,7 +2134,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; @@ -2180,7 +2197,7 @@ FlatExprVisitor::CallHandlerResult FlatExprVisitor::HandleHeterogeneousEquality( *depth + 1); return CallHandlerResult::kIntercepted; } - AddStep(CreateEqualityStep(inequality, expr.id())); + AddStep(CreateEqualityStep(inequality), expr.id()); return CallHandlerResult::kIntercepted; } @@ -2205,7 +2222,7 @@ FlatExprVisitor::HandleHeterogeneousEqualityIn(const cel::Expr& expr, return CallHandlerResult::kIntercepted; } - AddStep(CreateInStep(expr.id())); + AddStep(CreateInStep(), expr.id()); return CallHandlerResult::kIntercepted; } @@ -2240,9 +2257,11 @@ 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_) { @@ -2254,10 +2273,9 @@ void LogicalCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { if (short_circuiting_ && arg_num < last_arg_index) { std::unique_ptr jump_step = is_or_ - ? CreateCondJumpStep(true, {}, /*expected_stack_size=*/arg_num + 1, - expr->id()) - : CreateCondJumpStep(false, {}, /*expected_stack_size=*/arg_num + 1, - expr->id()); + ? 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) { @@ -2289,7 +2307,7 @@ void OptionalOrCondVisitor::PostVisitTarget(const cel::Expr* expr) { // 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_, expr->id()); + CreateOptionalHasValueJumpStep(is_or_value_); ProgramStepIndex index = visitor_->GetCurrentIndex(); if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step)); jump_step_ptr) { @@ -2304,7 +2322,7 @@ void OptionalOrCondVisitor::PostVisit(const cel::Expr* expr) { return; } - visitor_->AddStep(CreateOptionalOrStep(is_or_value_, expr->id())); + visitor_->AddStep(CreateOptionalOrStep(is_or_value_), expr->id()); if (short_circuiting_) { for (auto& jump : jump_steps_) { visitor_->SetProgressStatusIfError( @@ -2338,8 +2356,7 @@ void TernaryCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { if (arg_num == 0) { // Jump in case of error or non-bool ProgramStepIndex error_jump_pos = visitor_->GetCurrentIndex(); - auto* error_jump = - visitor_->AddStep(CreateBoolCheckJumpStep({}, expr->id())); + auto* error_jump = visitor_->AddStep(CreateBoolCheckJumpStep()); if (error_jump) { error_jump_ = Jump(error_jump_pos, error_jump); } @@ -2347,8 +2364,7 @@ void TernaryCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { // 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({}, expr->id())); + auto* jump_to_second = visitor_->AddStep(CreateTernaryCondJumpStep()); if (jump_to_second) { jump_to_second_ = Jump(cond_jump_pos, static_cast(jump_to_second)); @@ -2357,7 +2373,7 @@ void TernaryCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { // 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({}, expr->id())); + auto* jump_after_first = visitor_->AddStep(CreateJumpStep()); if (!jump_after_first) { return; } @@ -2406,7 +2422,7 @@ void ExhaustiveTernaryCondVisitor::PostVisit(const cel::Expr* expr) { visitor_->MakeTernaryRecursive(expr); return; } - visitor_->AddStep(CreateTernaryStep(expr->id())); + visitor_->AddStep(CreateTernaryStep(), expr->id()); } void ComprehensionVisitor::PreVisit(const cel::Expr* expr) { @@ -2425,25 +2441,24 @@ absl::Status ComprehensionVisitor::PostVisitArgDefault( switch (arg_num) { case cel::ITER_RANGE: { init_step_pos_ = visitor_->GetCurrentIndex(); - init_step_ = visitor_->AddStep( - std::make_unique(expr->id())); + init_step_ = visitor_->AddStep(std::make_unique()); break; } case cel::ACCU_INIT: { next_step_pos_ = visitor_->GetCurrentIndex(); next_step_ = visitor_->AddStep(std::make_unique( - iter_slot_, iter2_slot_, accu_slot_, expr->id())); + iter_slot_, iter2_slot_, accu_slot_)); 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_, expr->id())); + iter_slot_, iter2_slot_, accu_slot_, short_circuiting_)); break; } case cel::LOOP_STEP: { ProgramStepIndex index = visitor_->GetCurrentIndex(); - auto* jump_to_next = visitor_->AddStep(CreateJumpStep({}, expr->id())); + auto* jump_to_next = visitor_->AddStep(CreateJumpStep()); if (!jump_to_next) { break; } @@ -2473,7 +2488,7 @@ absl::Status ComprehensionVisitor::PostVisitArgDefault( // Encountered an error earlier. Can't determine where to jump. break; } - visitor_->AddStep(CreateComprehensionFinishStep(accu_slot_, expr->id())); + visitor_->AddStep(CreateComprehensionFinishStep(accu_slot_), expr->id()); // Set offsets jumping past the result step in case of errors. CEL_ASSIGN_OR_RETURN( int jump_from_init, @@ -2506,7 +2521,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; } @@ -2517,7 +2532,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; } } @@ -2645,6 +2661,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.cc b/eval/compiler/flat_expr_builder_extensions.cc index ee106ff4a..7e1417eb7 100644 --- a/eval/compiler/flat_expr_builder_extensions.cc +++ b/eval/compiler/flat_expr_builder_extensions.cc @@ -205,7 +205,7 @@ void Subexpression::Flatten() { return; } - std::vector> flat; + ExecutionPath flat; std::vector flatten_stack; @@ -221,8 +221,10 @@ void Subexpression::Flatten() { elements.clear(); continue; } else if (subexpr->IsRecursive()) { - flat.push_back(std::make_unique( - std::move(subexpr->ExtractRecursiveProgram().step), + flat.push_back(ExpressionStep::MakeGenericStep( + std::make_unique( + std::move(subexpr->ExtractRecursiveProgram().step), + subexpr->self_->id()), subexpr->self_->id())); continue; } @@ -237,8 +239,7 @@ void Subexpression::Flatten() { flatten_stack.push_back({subexpr, i + 1}); flatten_stack.push_back({*child, 0}); break; - } else if (auto* step = - absl::get_if>(&element); + } else if (auto* step = absl::get_if(&element); step != nullptr) { flat.push_back(std::move(*step)); } else { @@ -259,8 +260,7 @@ Subexpression::RecursiveProgram Subexpression::ExtractRecursiveProgram() { return result; } -bool Subexpression::ExtractTo( - std::vector>& out) { +bool Subexpression::ExtractTo(ExecutionPath& out) { if (!IsFlattened()) { return false; } @@ -272,9 +272,8 @@ bool Subexpression::ExtractTo( return true; } -std::vector> -ProgramBuilder::FlattenSubexpression(Subexpression* expr) { - std::vector> out; +ExecutionPath ProgramBuilder::FlattenSubexpression(Subexpression* expr) { + ExecutionPath out; if (!expr) { return out; @@ -347,13 +346,19 @@ Subexpression* absl_nullable ProgramBuilder::GetSubexpression( return it->second.get(); } -ExpressionStep* absl_nullable ProgramBuilder::AddStep( - std::unique_ptr step) { +ExpressionStep* absl_nullable ProgramBuilder::AddStep(ExpressionStep step) { if (current_ == nullptr) { return nullptr; } - auto* step_ptr = step.get(); - return current_->AddStep(std::move(step)) ? step_ptr : nullptr; + if (current_->IsRecursive()) { + return nullptr; + } + if (current_->IsFlattened()) { + current_->flattened_elements().push_back(std::move(step)); + return ¤t_->flattened_elements().back(); + } + auto& elem = current_->elements().emplace_back(std::move(step)); + return absl::get_if(&elem); } int ProgramBuilder::ExtractSubexpression(const cel::Expr* expr) { @@ -457,8 +462,8 @@ absl::Status PlannerContext::ReplaceSubplan( return absl::OkStatus(); } -absl::Status PlannerContext::AddSubplanStep( - const cel::Expr& node, std::unique_ptr step) { +absl::Status PlannerContext::AddSubplanStep(const cel::Expr& node, + ExpressionStep step) { auto* subexpression = program_builder_.GetSubexpression(&node); if (subexpression == nullptr) { diff --git a/eval/compiler/flat_expr_builder_extensions.h b/eval/compiler/flat_expr_builder_extensions.h index 21e37b2a8..2ce70a166 100644 --- a/eval/compiler/flat_expr_builder_extensions.h +++ b/eval/compiler/flat_expr_builder_extensions.h @@ -23,6 +23,7 @@ #define THIRD_PARTY_CEL_CPP_EVAL_COMPILER_FLAT_EXPR_BUILDER_EXTENSIONS_H_ #include +#include #include #include #include @@ -84,11 +85,10 @@ class ProgramBuilder { // Must be tied to a ProgramBuilder to coordinate relationships. class Subexpression { private: - using Element = absl::variant, - Subexpression* absl_nonnull>; + using Element = absl::variant; using TreePlan = std::vector; - using FlattenedPlan = std::vector>; + using FlattenedPlan = ExecutionPath; public: struct RecursiveProgram { @@ -105,7 +105,7 @@ class ProgramBuilder { Subexpression& operator=(Subexpression&&) = delete; // Add a program step at the current end of the subexpression. - bool AddStep(std::unique_ptr step) { + bool AddStep(ExpressionStep step) { if (IsRecursive()) { return false; } @@ -141,13 +141,12 @@ class ProgramBuilder { // Accessor for program steps. // // Value is undefined if in the expression has not yet been flattened. - std::vector>& flattened_elements() { + ExecutionPath& flattened_elements() { ABSL_DCHECK(IsFlattened()); return absl::get(program_); } - const std::vector>& - flattened_elements() const { + const ExecutionPath& flattened_elements() const { ABSL_DCHECK(IsFlattened()); return absl::get(program_); } @@ -202,7 +201,7 @@ class ProgramBuilder { // ownership of the given steps. // // Returns false if the subexpression is not currently flattened. - bool ExtractTo(std::vector>& out); + bool ExtractTo(ExecutionPath& out); private: Subexpression(const cel::Expr* self, ProgramBuilder* owner); @@ -234,6 +233,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. // @@ -287,13 +291,12 @@ class ProgramBuilder { // 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. - ExpressionStep* absl_nullable AddStep(std::unique_ptr step); + ExpressionStep* absl_nullable AddStep(ExpressionStep step); void Reset(); private: - static std::vector> - FlattenSubexpression(Subexpression* absl_nonnull expr); + static ExecutionPath FlattenSubexpression(Subexpression* absl_nonnull expr); Subexpression* absl_nullable MakeSubexpression(const cel::Expr* expr); @@ -385,8 +388,13 @@ class PlannerContext { int depth); // Extend the current subplan with the given expression step. + absl::Status AddSubplanStep(const cel::Expr& node, ExpressionStep step); absl::Status AddSubplanStep(const cel::Expr& node, - std::unique_ptr step); + std::unique_ptr step, + int64_t expr_id = -1) { + return AddSubplanStep( + node, ExpressionStep::MakeGenericStep(std::move(step), expr_id)); + } const Resolver& resolver() const { return resolver_; } const cel::TypeReflector& type_reflector() const { return type_reflector_; } diff --git a/eval/compiler/flat_expr_builder_extensions_test.cc b/eval/compiler/flat_expr_builder_extensions_test.cc index 45913e61b..db13f6ec6 100644 --- a/eval/compiler/flat_expr_builder_extensions_test.cc +++ b/eval/compiler/flat_expr_builder_extensions_test.cc @@ -55,6 +55,17 @@ using ::testing::ElementsAre; using ::testing::IsEmpty; using ::testing::Optional; +class TestStepLogic : public ExpressionStepLogic { + public: + absl::Status Evaluate(ExecutionFrame* frame) const override { + return absl::OkStatus(); + } +}; + +std::unique_ptr MakeTestStepLogic() { + return std::make_unique(); +} + using Subexpression = ProgramBuilder::Subexpression; class PlannerContextTest : public testing::Test { @@ -78,13 +89,13 @@ class PlannerContextTest : public testing::Test { MATCHER_P(UniquePtrHolds, ptr, "") { const auto& got = arg; - return ptr == got.get(); + return got.IsGenericStep() && ptr == got.GetGenericStep(); } struct SimpleTreeSteps { - const ExpressionStep* a; - const ExpressionStep* b; - const ExpressionStep* c; + const ExpressionStepLogic* a; + const ExpressionStepLogic* b; + const ExpressionStepLogic* c; }; // simulate a program of: @@ -94,20 +105,23 @@ struct SimpleTreeSteps { absl::StatusOr InitSimpleTree( const Expr& a, const Expr& b, const Expr& c, ProgramBuilder& program_builder) { - CEL_ASSIGN_OR_RETURN(auto a_step, CreateConstValueStep(cel::NullValue(), -1)); - CEL_ASSIGN_OR_RETURN(auto b_step, CreateConstValueStep(cel::NullValue(), -1)); - CEL_ASSIGN_OR_RETURN(auto c_step, CreateConstValueStep(cel::NullValue(), -1)); + auto a_step = MakeTestStepLogic(); + auto b_step = MakeTestStepLogic(); + auto c_step = MakeTestStepLogic(); SimpleTreeSteps result{a_step.get(), b_step.get(), c_step.get()}; program_builder.EnterSubexpression(&a); program_builder.EnterSubexpression(&b); - program_builder.AddStep(std::move(b_step)); + program_builder.AddStep( + ExpressionStep::MakeGenericStep(std::move(b_step), -1)); program_builder.ExitSubexpression(&b); program_builder.EnterSubexpression(&c); - program_builder.AddStep(std::move(c_step)); + program_builder.AddStep( + ExpressionStep::MakeGenericStep(std::move(c_step), -1)); program_builder.ExitSubexpression(&c); - program_builder.AddStep(std::move(a_step)); + program_builder.AddStep( + ExpressionStep::MakeGenericStep(std::move(a_step), -1)); program_builder.ExitSubexpression(&a); return result; @@ -160,10 +174,9 @@ TEST_F(PlannerContextTest, ReplacePlan) { ExecutionPath new_a; - ASSERT_OK_AND_ASSIGN(auto new_a_step, - CreateConstValueStep(cel::NullValue(), -1)); - const ExpressionStep* new_a_step_ptr = new_a_step.get(); - new_a.push_back(std::move(new_a_step)); + 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)); ASSERT_THAT(context.ReplaceSubplan(a, std::move(new_a)), IsOk()); @@ -251,14 +264,12 @@ TEST_F(PlannerContextTest, ReplacePlanUpdatesSibling) { ExecutionPath new_b; - ASSERT_OK_AND_ASSIGN(auto b1_step, - CreateConstValueStep(cel::NullValue(), -1)); - const ExpressionStep* b1_step_ptr = b1_step.get(); - new_b.push_back(std::move(b1_step)); - ASSERT_OK_AND_ASSIGN(auto b2_step, - CreateConstValueStep(cel::NullValue(), -1)); - const ExpressionStep* b2_step_ptr = b2_step.get(); - new_b.push_back(std::move(b2_step)); + 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 = MakeTestStepLogic(); + const ExpressionStepLogic* b2_step_ptr = b2_step.get(); + new_b.push_back(ExpressionStep::MakeGenericStep(std::move(b2_step), -1)); ASSERT_THAT(context.ReplaceSubplan(b, std::move(new_b)), IsOk()); @@ -302,10 +313,9 @@ TEST_F(PlannerContextTest, AddSubplanStep) { ASSERT_OK_AND_ASSIGN(auto plan_steps, InitSimpleTree(a, b, c, program_builder)); - ASSERT_OK_AND_ASSIGN(auto b2_step, - CreateConstValueStep(cel::NullValue(), -1)); + auto b2_step = MakeTestStepLogic(); - const ExpressionStep* b2_step_ptr = b2_step.get(); + const ExpressionStepLogic* b2_step_ptr = b2_step.get(); std::shared_ptr arena; PlannerContext context(env_, resolver_, options_, @@ -332,8 +342,7 @@ TEST_F(PlannerContextTest, AddSubplanStepFailsOnUnknownNode) { ASSERT_THAT(InitSimpleTree(a, b, c, program_builder).status(), IsOk()); - ASSERT_OK_AND_ASSIGN(auto b2_step, - CreateConstValueStep(cel::NullValue(), -1)); + auto b2_step = MakeTestStepLogic(); std::shared_ptr arena; PlannerContext context(env_, resolver_, options_, @@ -480,8 +489,9 @@ TEST_F(ProgramBuilderTest, ExtractWorks) { program_builder.EnterSubexpression(&b); program_builder.ExitSubexpression(&b); - ASSERT_OK_AND_ASSIGN(auto a_step, CreateConstValueStep(cel::NullValue(), -1)); - program_builder.AddStep(std::move(a_step)); + auto a_step = MakeTestStepLogic(); + program_builder.AddStep( + ExpressionStep::MakeGenericStep(std::move(a_step), -1)); program_builder.EnterSubexpression(&c); program_builder.ExitSubexpression(&c); program_builder.ExitSubexpression(&a); @@ -563,8 +573,9 @@ TEST_F(ProgramBuilderTest, Recursive) { auto path = program_builder.FlattenMain(); ASSERT_THAT(path, testing::SizeIs(1)); - EXPECT_TRUE(path[0]->GetNativeTypeId() == - cel::NativeTypeId::For()); + EXPECT_TRUE(path[0].IsGenericStep() && + path[0].GetGenericStep()->GetNativeTypeId() == + cel::NativeTypeId::For()); } } // namespace diff --git a/eval/compiler/instrumentation.cc b/eval/compiler/instrumentation.cc index 3e37bdb45..ea3e7ddbb 100644 --- a/eval/compiler/instrumentation.cc +++ b/eval/compiler/instrumentation.cc @@ -69,7 +69,8 @@ class InstrumentOptimizer : public ProgramOptimizer { } return context.AddSubplanStep( - node, std::make_unique(node.id(), instrumentation_)); + node, std::make_unique(node.id(), instrumentation_), + node.id()); } private: diff --git a/eval/compiler/regex_precompilation_optimization.cc b/eval/compiler/regex_precompilation_optimization.cc index 38ef842b9..fa0d289f2 100644 --- a/eval/compiler/regex_precompilation_optimization.cc +++ b/eval/compiler/regex_precompilation_optimization.cc @@ -194,11 +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]->GetNativeTypeId() == - NativeTypeId::For()) { - constant = - down_cast(re_plan[0].get())->value(); + if (re_plan.size() == 1) { + cel::Value val; + if (GetIfConstant(re_plan[0], val)) { + constant = std::move(val); + } } } @@ -251,9 +251,10 @@ class RegexPrecompilationOptimization : public ProgramOptimizer { CEL_ASSIGN_OR_RETURN(ExecutionPath new_plan, context.ExtractSubplan(subject)); - CEL_ASSIGN_OR_RETURN( - new_plan.emplace_back(), - CreateRegexMatchStep(std::move(regex_program), call.id())); + CEL_ASSIGN_OR_RETURN(auto step, + CreateRegexMatchStep(std::move(regex_program))); + new_plan.push_back( + ExpressionStep::MakeGenericStep(std::move(step), call.id())); return context.ReplaceSubplan(call, std::move(new_plan)); } diff --git a/eval/eval/BUILD b/eval/eval/BUILD index f6ce6e221..99953d6b9 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -36,22 +36,34 @@ cc_library( name = "evaluator_core", srcs = [ "evaluator_core.cc", + "lazy_init_step.cc", + "logic_step.cc", ], hdrs = [ "evaluator_core.h", + "lazy_init_step.h", + "logic_step.h", ], deps = [ + ":attribute_trail", ":attribute_utility", ":comprehension_slots", + ":direct_expression_step", ":evaluator_stack", ":iterator_stack", + "//base:builtins", "//base:data", + "//common:casting", "//common:native_type", "//common:value", + "//common:value_kind", + "//eval/internal:errors", + "//internal:status_macros", "//runtime", "//runtime:activation_interface", "//runtime:runtime_options", "//runtime/internal:activation_attribute_matcher_access", + "//runtime/internal:errors", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/log:absl_check", @@ -59,7 +71,9 @@ cc_library( "@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", ], ) @@ -182,7 +196,6 @@ cc_library( deps = [ ":compiler_constant_step", ":direct_expression_step", - ":evaluator_core", "//common:value", "@com_google_absl//absl/status:statusor", ], @@ -425,33 +438,6 @@ cc_library( ], ) -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 = [ @@ -754,7 +740,6 @@ cc_test( ":direct_expression_step", ":evaluator_core", ":ident_step", - ":logic_step", "//base:attributes", "//base:data", "//common:casting", @@ -1137,8 +1122,6 @@ cc_library( deps = [ ":attribute_trail", ":direct_expression_step", - ":evaluator_core", - ":expression_step_base", "//common:native_type", "//common:value", "@com_google_absl//absl/status", @@ -1149,7 +1132,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", @@ -1163,31 +1148,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", @@ -1206,10 +1173,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", ], diff --git a/eval/eval/cel_expression_flat_impl.cc b/eval/eval/cel_expression_flat_impl.cc index 9e35b41ad..8c78d21ce 100644 --- a/eval/eval/cel_expression_flat_impl.cc +++ b/eval/eval/cel_expression_flat_impl.cc @@ -108,9 +108,12 @@ absl::StatusOr> CelExpressionRecursiveImpl::Create( absl_nonnull std::shared_ptr env, FlatExpression flat_expr) { - if (flat_expr.path().empty() || - flat_expr.path().front()->GetNativeTypeId() != - cel::NativeTypeId::For()) { + const ExpressionStepLogic* logic = nullptr; + if (!flat_expr.path().empty() && flat_expr.path()[0].IsGenericStep()) { + logic = flat_expr.path()[0].GetGenericStep(); + } + if (logic != nullptr && + logic->GetNativeTypeId() != cel::NativeTypeId::For()) { return absl::InvalidArgumentError(absl::StrCat( "Expected a recursive program step", flat_expr.path().size())); } diff --git a/eval/eval/cel_expression_flat_impl.h b/eval/eval/cel_expression_flat_impl.h index 7faf6856a..3590dc788 100644 --- a/eval/eval/cel_expression_flat_impl.h +++ b/eval/eval/cel_expression_flat_impl.h @@ -162,7 +162,7 @@ class CelExpressionRecursiveImpl : public CelExpression { : env_(std::move(env)), flat_expression_(std::move(flat_expression)), root_(cel::internal::down_cast( - flat_expression_.path()[0].get()) + flat_expression_.path()[0].GetGenericStep()) ->wrapped()) {} absl_nonnull std::shared_ptr env_; diff --git a/eval/eval/compiler_constant_step.cc b/eval/eval/compiler_constant_step.cc index 44a03cecd..b8ebe84f4 100644 --- a/eval/eval/compiler_constant_step.cc +++ b/eval/eval/compiler_constant_step.cc @@ -16,7 +16,7 @@ #include "absl/status/status.h" #include "common/value.h" #include "eval/eval/attribute_trail.h" -#include "eval/eval/evaluator_core.h" +#include "eval/eval/direct_expression_step.h" namespace google::api::expr::runtime { @@ -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 bd514a036..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,28 +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: - CompilerConstantStep(cel::Value value, int64_t expr_id, bool comes_from_ast) - : ExpressionStepBase(expr_id, comes_from_ast), 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 856ca30e0..25cde561d 100644 --- a/eval/eval/compiler_constant_step_test.cc +++ b/eval/eval/compiler_constant_step_test.cc @@ -17,6 +17,8 @@ #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 +32,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( - std::make_unique(cel::IntValue(42), -1, false)); +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; - ExecutionFrame frame(path, empty_activation_, options_, state_); - - 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), -1, false); +TEST_F(DirectCompilerConstantStepTest, TypeId) { + DirectCompilerConstantStep step(cel::IntValue(42), -1); - ExpressionStep& 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), -1, false); +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 5e741d805..1915c7352 100644 --- a/eval/eval/comprehension_step.cc +++ b/eval/eval/comprehension_step.cc @@ -60,8 +60,8 @@ AttributeQualifier AttributeQualifierFromValue(const Value& v) { class ComprehensionFinishStep final : public ExpressionStepBase { public: - ComprehensionFinishStep(size_t accu_slot, int64_t expr_id) - : ExpressionStepBase(expr_id), accu_slot_(accu_slot) {} + explicit ComprehensionFinishStep(size_t accu_slot) + : ExpressionStepBase(), accu_slot_(accu_slot) {} absl::Status Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(2)) { @@ -677,9 +677,9 @@ std::unique_ptr CreateDirectComprehensionStep( shortcircuiting, expr_id); } -std::unique_ptr CreateComprehensionFinishStep(size_t accu_slot, - int64_t expr_id) { - return std::make_unique(accu_slot, expr_id); +std::unique_ptr CreateComprehensionFinishStep( + size_t accu_slot) { + return std::make_unique(accu_slot); } } // namespace google::api::expr::runtime diff --git a/eval/eval/comprehension_step.h b/eval/eval/comprehension_step.h index 34a6afc19..f16b62e13 100644 --- a/eval/eval/comprehension_step.h +++ b/eval/eval/comprehension_step.h @@ -27,8 +27,7 @@ namespace google::api::expr::runtime { class ComprehensionInitStep final : public ExpressionStepBase { public: - explicit ComprehensionInitStep(int64_t expr_id) - : ExpressionStepBase(expr_id, /*comes_from_ast=*/false) {} + ComprehensionInitStep() : ExpressionStepBase() {} void set_error_jump_offset(int offset) { error_jump_offset_ = offset; } @@ -40,9 +39,8 @@ class ComprehensionInitStep final : public ExpressionStepBase { class ComprehensionNextStep final : public ExpressionStepBase { public: - ComprehensionNextStep(size_t iter_slot, size_t iter2_slot, size_t accu_slot, - int64_t expr_id) - : ExpressionStepBase(expr_id, /*comes_from_ast=*/false), + 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) {} @@ -70,8 +68,8 @@ class ComprehensionNextStep final : public ExpressionStepBase { class ComprehensionCondStep final : public ExpressionStepBase { public: ComprehensionCondStep(size_t iter_slot, size_t iter2_slot, size_t accu_slot, - bool shortcircuiting, int64_t expr_id) - : ExpressionStepBase(expr_id, /*comes_from_ast=*/false), + bool shortcircuiting) + : ExpressionStepBase(), iter_slot_(iter_slot), iter2_slot_(iter2_slot), accu_slot_(accu_slot), @@ -111,8 +109,8 @@ std::unique_ptr CreateDirectComprehensionStep( // Creates 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, - int64_t expr_id); +std::unique_ptr CreateComprehensionFinishStep( + size_t accu_slot); } // namespace google::api::expr::runtime diff --git a/eval/eval/comprehension_step_test.cc b/eval/eval/comprehension_step_test.cc index 681f8af4f..ef01b034d 100644 --- a/eval/eval/comprehension_step_test.cc +++ b/eval/eval/comprehension_step_test.cc @@ -40,6 +40,7 @@ namespace google::api::expr::runtime { namespace { +using ::absl_testing::IsOk; using ::absl_testing::StatusIs; using ::cel::BoolValue; using ::cel::Expr; @@ -86,7 +87,7 @@ class ListKeysStepTest : public testing::Test { class GetListKeysResultStep : public ExpressionStepBase { public: - GetListKeysResultStep() : ExpressionStepBase(-1, false) {} + GetListKeysResultStep() : ExpressionStepBase() {} absl::Status Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Pop(1); @@ -102,13 +103,12 @@ MATCHER_P(CelStringValue, val, "") { TEST_F(ListKeysStepTest, MapPartiallyUnknown) { ExecutionPath path; - auto result = CreateIdentStep("var", 0); - ASSERT_OK(result); - path.push_back(*std::move(result)); - ComprehensionInitStep* init_step = new ComprehensionInitStep(1); + path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("var"))); + auto init_step = std::make_unique(); init_step->set_error_jump_offset(1); - path.push_back(absl::WrapUnique(init_step)); - path.push_back(std::make_unique()); + path.push_back(ExpressionStep::MakeGenericStep(std::move(init_step))); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique())); auto expression = MakeExpression(std::move(path), /*unknown_attributes=*/true); @@ -129,7 +129,7 @@ TEST_F(ListKeysStepTest, MapPartiallyUnknown) { auto eval_result = expression->Evaluate(activation, &arena); - ASSERT_OK(eval_result); + ASSERT_THAT(eval_result, IsOk()); ASSERT_TRUE(eval_result->IsUnknownSet()); const auto& attrs = eval_result->UnknownSetOrDie()->unknown_attributes(); @@ -140,13 +140,12 @@ TEST_F(ListKeysStepTest, MapPartiallyUnknown) { TEST_F(ListKeysStepTest, ErrorPassedThrough) { ExecutionPath path; - auto result = CreateIdentStep("var", 0); - ASSERT_OK(result); - path.push_back(*std::move(result)); - ComprehensionInitStep* init_step = new ComprehensionInitStep(1); + path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("var"))); + auto init_step = std::make_unique(); init_step->set_error_jump_offset(1); - path.push_back(absl::WrapUnique(init_step)); - path.push_back(std::make_unique()); + path.push_back(ExpressionStep::MakeGenericStep(std::move(init_step))); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique())); auto expression = MakeExpression(std::move(path)); @@ -156,7 +155,7 @@ TEST_F(ListKeysStepTest, ErrorPassedThrough) { // Var not in activation, turns into cel error at eval time. auto eval_result = expression->Evaluate(activation, &arena); - ASSERT_OK(eval_result); + ASSERT_THAT(eval_result, IsOk()); ASSERT_TRUE(eval_result->IsError()); EXPECT_THAT(eval_result->ErrorOrDie()->message(), testing::HasSubstr("\"var\"")); @@ -165,13 +164,12 @@ TEST_F(ListKeysStepTest, ErrorPassedThrough) { TEST_F(ListKeysStepTest, UnknownSetPassedThrough) { ExecutionPath path; - auto result = CreateIdentStep("var", 0); - ASSERT_OK(result); - path.push_back(*std::move(result)); - ComprehensionInitStep* init_step = new ComprehensionInitStep(1); + path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("var"))); + auto init_step = std::make_unique(); init_step->set_error_jump_offset(1); - path.push_back(absl::WrapUnique(init_step)); - path.push_back(std::make_unique()); + path.push_back(ExpressionStep::MakeGenericStep(std::move(init_step))); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique())); auto expression = MakeExpression(std::move(path), /*unknown_attributes=*/true); @@ -183,7 +181,7 @@ TEST_F(ListKeysStepTest, UnknownSetPassedThrough) { auto eval_result = expression->Evaluate(activation, &arena); - ASSERT_OK(eval_result); + ASSERT_THAT(eval_result, IsOk()); ASSERT_TRUE(eval_result->IsUnknownSet()); EXPECT_THAT(eval_result->UnknownSetOrDie()->unknown_attributes(), SizeIs(1)); } @@ -412,7 +410,7 @@ TEST_F(DirectComprehensionTest, Shortcircuit) { Value result; AttributeTrail trail; - ASSERT_OK(compre_step->Evaluate(frame, result, trail)); + ASSERT_THAT(compre_step->Evaluate(frame, result, trail), IsOk()); EXPECT_THAT(result, BoolValueIs(false)); } @@ -484,7 +482,7 @@ TEST_F(DirectComprehensionTest, Exhaustive) { Value result; AttributeTrail trail; - ASSERT_OK(compre_step->Evaluate(frame, result, trail)); + ASSERT_THAT(compre_step->Evaluate(frame, result, trail), IsOk()); EXPECT_THAT(result, BoolValueIs(false)); } diff --git a/eval/eval/const_value_step.h b/eval/eval/const_value_step.h index c3cf6a424..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,13 +18,6 @@ inline std::unique_ptr CreateConstValueDirectStep( return std::make_unique(std::move(value), id); } -// Factory method for Constant AST node expression step. -inline absl::StatusOr> CreateConstValueStep( - cel::Value value, int64_t expr_id, bool comes_from_ast = true) { - return std::make_unique(std::move(value), expr_id, - comes_from_ast); -} - } // 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 4cf4ebf4d..19c534d99 100644 --- a/eval/eval/container_access_step.cc +++ b/eval/eval/container_access_step.cc @@ -276,9 +276,8 @@ void PerformLookup(ExecutionFrameBase& frame, const Value& container, // message. class ContainerAccessStep : public ExpressionStepBase { public: - ContainerAccessStep(int64_t expr_id, bool enable_optional_types) - : ExpressionStepBase(expr_id), - enable_optional_types_(enable_optional_types) {} + explicit ContainerAccessStep(bool enable_optional_types) + : ExpressionStepBase(), enable_optional_types_(enable_optional_types) {} absl::Status Evaluate(ExecutionFrame* frame) const override; @@ -357,14 +356,14 @@ std::unique_ptr CreateDirectContainerAccessStep( } // Factory method for Select - based Execution step -absl::StatusOr> CreateContainerAccessStep( - const cel::CallExpr& call, int64_t expr_id, bool enable_optional_types) { +absl::StatusOr> CreateContainerAccessStep( + const cel::CallExpr& call, bool enable_optional_types) { int arg_count = call.args().size() + (call.has_target() ? 1 : 0); if (arg_count != kNumContainerAccessArguments) { return absl::InvalidArgumentError(absl::StrCat( "Invalid argument count for index operation: ", arg_count)); } - return std::make_unique(expr_id, enable_optional_types); + return std::make_unique(enable_optional_types); } } // namespace google::api::expr::runtime diff --git a/eval/eval/container_access_step.h b/eval/eval/container_access_step.h index b7af5e895..5d5fdde81 100644 --- a/eval/eval/container_access_step.h +++ b/eval/eval/container_access_step.h @@ -17,9 +17,8 @@ std::unique_ptr CreateDirectContainerAccessStep( int64_t expr_id); // Factory method for Select - based Execution step -absl::StatusOr> CreateContainerAccessStep( - const cel::CallExpr& call, int64_t expr_id, - bool enable_optional_types = false); +absl::StatusOr> CreateContainerAccessStep( + const cel::CallExpr& call, bool enable_optional_types = false); } // namespace google::api::expr::runtime diff --git a/eval/eval/container_access_step_test.cc b/eval/eval/container_access_step_test.cc index 25bf72223..a0a69b2ef 100644 --- a/eval/eval/container_access_step_test.cc +++ b/eval/eval/container_access_step_test.cc @@ -39,6 +39,7 @@ namespace google::api::expr::runtime { namespace { +using ::absl_testing::IsOk; using ::absl_testing::StatusIs; using ::cel::Expr; using ::cel::SourceInfo; @@ -75,15 +76,18 @@ CelValue EvaluateAttributeHelper( key_expr.mutable_ident_expr().set_name("key"); if (use_recursive_impl) { - path.push_back(std::make_unique( - CreateDirectContainerAccessStep(CreateDirectIdentStep("container", 1), - CreateDirectIdentStep("key", 2), - /*enable_optional_types=*/false, 3), + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(CreateDirectContainerAccessStep( + CreateDirectIdentStep("container", 1), + CreateDirectIdentStep("key", 2), + /*enable_optional_types=*/false, 3)), 3)); } else { - path.push_back(std::move(CreateIdentStep("container", 1).value())); - path.push_back(std::move(CreateIdentStep("key", 2).value())); - path.push_back(std::move(CreateContainerAccessStep(call, 3).value())); + path.push_back( + ExpressionStep::MakeGenericStep(CreateIdentStep("container"), 1)); + path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("key"), 2)); + path.push_back(ExpressionStep::MakeGenericStep( + std::move(CreateContainerAccessStep(call).value()), 3)); } cel::RuntimeOptions options; @@ -227,8 +231,9 @@ TEST_P(ContainerAccessStepUniformityTest, TestMapKeyAccess) { TEST_P(ContainerAccessStepUniformityTest, TestBoolKeyType) { CelMapBuilder cel_map; - ASSERT_OK(cel_map.Add(CelValue::CreateBool(true), - CelValue::CreateStringView("value_true"))); + ASSERT_THAT(cel_map.Add(CelValue::CreateBool(true), + CelValue::CreateStringView("value_true")), + IsOk()); CelValue result = EvaluateAttribute(CelValue::CreateMap(&cel_map), CelValue::CreateBool(true), @@ -267,7 +272,7 @@ TEST_F(ContainerAccessStepTest, TestInvalidReceiverCreateContainerAccessStep) { Expr& extra_arg = call.mutable_args().emplace_back(); extra_arg.mutable_const_expr().set_bool_value(true); - EXPECT_THAT(CreateContainerAccessStep(call, 0).status(), + EXPECT_THAT(CreateContainerAccessStep(call).status(), StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("Invalid argument count"))); } @@ -285,7 +290,7 @@ TEST_F(ContainerAccessStepTest, TestInvalidGlobalCreateContainerAccessStep) { Expr& extra_arg = call.mutable_args().emplace_back(); extra_arg.mutable_const_expr().set_bool_value(true); - EXPECT_THAT(CreateContainerAccessStep(call, 0).status(), + EXPECT_THAT(CreateContainerAccessStep(call).status(), StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("Invalid argument count"))); } diff --git a/eval/eval/create_list_step.cc b/eval/eval/create_list_step.cc index bb977ce94..54c9ee8ba 100644 --- a/eval/eval/create_list_step.cc +++ b/eval/eval/create_list_step.cc @@ -35,11 +35,8 @@ using ::cel::common_internal::NewListValueBuilder; class CreateListStep : public ExpressionStepBase { public: - CreateListStep(int64_t expr_id, int list_size, - absl::flat_hash_set optional_indices) - : ExpressionStepBase(expr_id), - list_size_(list_size), - optional_indices_(std::move(optional_indices)) {} + 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; @@ -226,7 +223,7 @@ class CreateListDirectStep : public DirectExpressionStep { class MutableListStep : public ExpressionStepBase { public: - explicit MutableListStep(int64_t expr_id) : ExpressionStepBase(expr_id) {} + MutableListStep() = default; absl::Status Evaluate(ExecutionFrame* frame) const override; }; @@ -247,9 +244,9 @@ class DirectMutableListStep : public DirectExpressionStep { AttributeTrail& attribute) const override; }; -absl::Status DirectMutableListStep::Evaluate( - ExecutionFrameBase& frame, Value& result, - AttributeTrail& attribute_trail) const { +absl::Status DirectMutableListStep::Evaluate(ExecutionFrameBase& frame, + Value& result, + AttributeTrail& attribute) const { result = cel::CustomListValue( cel::common_internal::NewMutableListValue(frame.arena()), frame.arena()); return absl::OkStatus(); @@ -264,15 +261,15 @@ std::unique_ptr CreateDirectListStep( std::move(deps), std::move(optional_indices), expr_id); } -absl::StatusOr> CreateCreateListStep( - const cel::ListExpr& create_list_expr, int64_t expr_id) { +absl::StatusOr> CreateCreateListStep( + const cel::ListExpr& create_list_expr) { return std::make_unique( - expr_id, create_list_expr.elements().size(), + create_list_expr.elements().size(), MakeOptionalIndicesSet(create_list_expr)); } -std::unique_ptr CreateMutableListStep(int64_t expr_id) { - return std::make_unique(expr_id); +std::unique_ptr CreateMutableListStep() { + return std::make_unique(); } std::unique_ptr CreateDirectMutableListStep( diff --git a/eval/eval/create_list_step.h b/eval/eval/create_list_step.h index b60a5e9c8..c377338c5 100644 --- a/eval/eval/create_list_step.h +++ b/eval/eval/create_list_step.h @@ -19,14 +19,14 @@ std::unique_ptr CreateDirectListStep( absl::flat_hash_set optional_indices, int64_t expr_id); // Factory method for CreateList which constructs an immutable list. -absl::StatusOr> CreateCreateListStep( - const cel::ListExpr& create_list_expr, int64_t expr_id); +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(int64_t expr_id); +std::unique_ptr CreateMutableListStep(); // Factory method for CreateList which constructs a mutable list. // diff --git a/eval/eval/create_list_step_test.cc b/eval/eval/create_list_step_test.cc index dfc9557a6..2faf1f11e 100644 --- a/eval/eval/create_list_step_test.cc +++ b/eval/eval/create_list_step_test.cc @@ -80,16 +80,13 @@ 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); - CEL_ASSIGN_OR_RETURN( - auto const_step, - CreateConstValueStep(cel::interop_internal::CreateIntValue(value), - /*expr_id=*/-1)); - path.push_back(std::move(const_step)); + path.push_back(ExpressionStep::MakeConstant( + cel::interop_internal::CreateIntValue(value))); } - CEL_ASSIGN_OR_RETURN(auto step, - CreateCreateListStep(create_list, dummy_expr.id())); - path.push_back(std::move(step)); + CEL_ASSIGN_OR_RETURN(auto step, CreateCreateListStep(create_list)); + path.push_back( + ExpressionStep::MakeGenericStep(std::move(step), dummy_expr.id())); cel::RuntimeOptions options; if (enable_unknowns) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; @@ -122,15 +119,13 @@ absl::StatusOr RunExpressionWithCelValues( expr0.set_id(ind); expr0.mutable_ident_expr().set_name(var_name); - CEL_ASSIGN_OR_RETURN(auto ident_step, - CreateIdentStep(var_name, /*expr_id=*/-1)); - path.push_back(std::move(ident_step)); + path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep(var_name))); activation.InsertValue(var_name, value); } - CEL_ASSIGN_OR_RETURN(auto step0, - CreateCreateListStep(create_list, dummy_expr.id())); - path.push_back(std::move(step0)); + CEL_ASSIGN_OR_RETURN(auto step0, CreateCreateListStep(create_list)); + path.push_back( + ExpressionStep::MakeGenericStep(std::move(step0), dummy_expr.id())); cel::RuntimeOptions options; if (enable_unknowns) { @@ -164,9 +159,9 @@ TEST(CreateListStepTest, TestCreateListStackUnderflow) { auto& expr0 = create_list.mutable_elements().emplace_back().mutable_expr(); expr0.mutable_const_expr().set_int64_value(1); - ASSERT_OK_AND_ASSIGN(auto step0, - CreateCreateListStep(create_list, dummy_expr.id())); - path.push_back(std::move(step0)); + ASSERT_OK_AND_ASSIGN(auto step0, CreateCreateListStep(create_list)); + path.push_back( + ExpressionStep::MakeGenericStep(std::move(step0), dummy_expr.id())); auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl cel_expr( @@ -292,7 +287,7 @@ TEST(CreateDirectListStep, Basic) { cel::Value result; AttributeTrail attr; - ASSERT_OK(step->Evaluate(frame, result, attr)); + ASSERT_THAT(step->Evaluate(frame, result, attr), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).Size(), IsOkAndHolds(2)); @@ -320,7 +315,7 @@ TEST(CreateDirectListStep, ForwardFirstError) { cel::Value result; AttributeTrail attr; - ASSERT_OK(step->Evaluate(frame, result, attr)); + ASSERT_THAT(step->Evaluate(frame, result, attr), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).NativeValue(), @@ -332,9 +327,10 @@ std::vector UnknownAttrNames(const UnknownValue& v) { names.reserve(v.ToAttributeSet().size()); for (const auto& attr : v.ToAttributeSet()) { - EXPECT_OK(attr.AsString().status()); + EXPECT_THAT(attr.AsString().status(), IsOk()); names.push_back(attr.AsString().value_or("")); } + return names; } @@ -368,7 +364,7 @@ TEST(CreateDirectListStep, MergeUnknowns) { cel::Value result; AttributeTrail attr; - ASSERT_OK(step->Evaluate(frame, result, attr)); + ASSERT_THAT(step->Evaluate(frame, result, attr), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(UnknownAttrNames(Cast(result)), @@ -399,7 +395,7 @@ TEST(CreateDirectListStep, ErrorBeforeUnknown) { cel::Value result; AttributeTrail attr; - ASSERT_OK(step->Evaluate(frame, result, attr)); + ASSERT_THAT(step->Evaluate(frame, result, attr), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).NativeValue(), @@ -447,7 +443,7 @@ TEST(CreateDirectListStep, MissingAttribute) { cel::Value result; AttributeTrail attr; - ASSERT_OK(step->Evaluate(frame, result, attr)); + ASSERT_THAT(step->Evaluate(frame, result, attr), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT( @@ -476,7 +472,7 @@ TEST(CreateDirectListStep, OptionalPresentSet) { cel::Value result; AttributeTrail attr; - ASSERT_OK(step->Evaluate(frame, result, attr)); + ASSERT_THAT(step->Evaluate(frame, result, attr), IsOk()); ASSERT_TRUE(InstanceOf(result)); auto list = Cast(result); @@ -509,7 +505,7 @@ TEST(CreateDirectListStep, OptionalAbsentNotSet) { cel::Value result; AttributeTrail attr; - ASSERT_OK(step->Evaluate(frame, result, attr)); + ASSERT_THAT(step->Evaluate(frame, result, attr), IsOk()); ASSERT_TRUE(InstanceOf(result)); auto list = Cast(result); @@ -542,7 +538,7 @@ TEST(CreateDirectListStep, PartialUnknown) { cel::Value result; AttributeTrail attr; - ASSERT_OK(step->Evaluate(frame, result, attr)); + ASSERT_THAT(step->Evaluate(frame, result, attr), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(UnknownAttrNames(Cast(result)), diff --git a/eval/eval/create_map_step.cc b/eval/eval/create_map_step.cc index 451181e75..e90de1fc7 100644 --- a/eval/eval/create_map_step.cc +++ b/eval/eval/create_map_step.cc @@ -52,10 +52,9 @@ using ::cel::common_internal::NewMutableMapValue; // `CreateStruct` implementation for map. class CreateStructStepForMap final : public ExpressionStepBase { public: - CreateStructStepForMap(int64_t expr_id, size_t entry_count, + CreateStructStepForMap(size_t entry_count, absl::flat_hash_set optional_indices) - : ExpressionStepBase(expr_id), - entry_count_(entry_count), + : entry_count_(entry_count), optional_indices_(std::move(optional_indices)) {} absl::Status Evaluate(ExecutionFrame* frame) const override; @@ -235,9 +234,9 @@ absl::Status DirectCreateMapStep::Evaluate( return absl::OkStatus(); } -class MutableMapStep final : public ExpressionStep { +class MutableMapStep final : public ExpressionStepBase { public: - explicit MutableMapStep(int64_t expr_id) : ExpressionStep(expr_id) {} + MutableMapStep() = default; absl::Status Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Push(cel::CustomMapValue( @@ -268,17 +267,16 @@ std::unique_ptr CreateDirectCreateMapStep( std::move(deps), std::move(optional_indices), expr_id); } -absl::StatusOr> CreateCreateStructStepForMap( - size_t entry_count, absl::flat_hash_set optional_indices, - int64_t expr_id) { +absl::StatusOr> +CreateCreateStructStepForMap(size_t entry_count, + absl::flat_hash_set optional_indices) { // Make map-creating step. - return std::make_unique(expr_id, entry_count, + return std::make_unique(entry_count, std::move(optional_indices)); } -absl::StatusOr> CreateMutableMapStep( - int64_t expr_id) { - return std::make_unique(expr_id); +std::unique_ptr CreateMutableMapStep() { + return std::make_unique(); } std::unique_ptr CreateDirectMutableMapStep( diff --git a/eval/eval/create_map_step.h b/eval/eval/create_map_step.h index cf5e94644..599a48534 100644 --- a/eval/eval/create_map_step.h +++ b/eval/eval/create_map_step.h @@ -36,16 +36,15 @@ std::unique_ptr CreateDirectCreateMapStep( absl::flat_hash_set optional_indices, int64_t expr_id); // Creates an `ExpressionStep` which performs `CreateStruct` for a map. -absl::StatusOr> CreateCreateStructStepForMap( - size_t entry_count, absl::flat_hash_set optional_indices, - int64_t expr_id); +absl::StatusOr> +CreateCreateStructStepForMap(size_t entry_count, + absl::flat_hash_set optional_indices); // Factory method for CreateMap which constructs a mutable map. // // This is intended for the map construction step is generated for a // map-building comprehension (rather than a user authored expression). -absl::StatusOr> CreateMutableMapStep( - int64_t expr_id); +std::unique_ptr CreateMutableMapStep(); // Factory method for CreateMap which constructs a mutable map. // diff --git a/eval/eval/create_map_step_test.cc b/eval/eval/create_map_step_test.cc index dbc9adb5a..36a6eebd4 100644 --- a/eval/eval/create_map_step_test.cc +++ b/eval/eval/create_map_step_test.cc @@ -71,14 +71,11 @@ absl::StatusOr CreateStackMachineProgram( std::string key_name = absl::StrCat("key", index); std::string value_name = absl::StrCat("value", index); - CEL_ASSIGN_OR_RETURN(auto step_key, - CreateIdentStep(key_name, /*expr_id=*/-1)); + auto step_key = CreateIdentStep(key_name); + auto step_value = CreateIdentStep(value_name); - CEL_ASSIGN_OR_RETURN(auto step_value, - CreateIdentStep(value_name, /*expr _id=*/-1)); - - path.push_back(std::move(step_key)); - path.push_back(std::move(step_value)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step_key))); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step_value))); activation.InsertValue(key_name, item.first); activation.InsertValue(value_name, item.second); @@ -87,9 +84,9 @@ absl::StatusOr CreateStackMachineProgram( index++; } - CEL_ASSIGN_OR_RETURN( - auto step1, CreateCreateStructStepForMap(values.size(), {}, expr1.id())); - path.push_back(std::move(step1)); + CEL_ASSIGN_OR_RETURN(auto step1, + CreateCreateStructStepForMap(values.size(), {})); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step1), expr1.id())); return path; } @@ -113,8 +110,9 @@ absl::StatusOr CreateRecursiveProgram( index++; } - path.push_back(std::make_unique( - CreateDirectCreateMapStep(std::move(deps), {}, -1), -1)); + path.push_back( + ExpressionStep::MakeGenericStep(std::make_unique( + CreateDirectCreateMapStep(std::move(deps), {}, -1)))); return path; } diff --git a/eval/eval/create_struct_step.cc b/eval/eval/create_struct_step.cc index 5d042baf5..9c0936ac6 100644 --- a/eval/eval/create_struct_step.cc +++ b/eval/eval/create_struct_step.cc @@ -48,10 +48,9 @@ using ::cel::Value; // `CreateStruct` implementation for message/struct. class CreateStructStepForStruct final : public ExpressionStepBase { public: - CreateStructStepForStruct(int64_t expr_id, std::string name, - std::vector entries, + CreateStructStepForStruct(std::string name, std::vector entries, absl::flat_hash_set optional_indices) - : ExpressionStepBase(expr_id), + : ExpressionStepBase(), name_(std::move(name)), entries_(std::move(entries)), optional_indices_(std::move(optional_indices)) {} @@ -259,12 +258,11 @@ std::unique_ptr CreateDirectCreateStructStep( std::move(optional_indices)); } -std::unique_ptr CreateCreateStructStep( +std::unique_ptr CreateCreateStructStep( std::string name, std::vector field_keys, - absl::flat_hash_set optional_indices, int64_t expr_id) { + absl::flat_hash_set optional_indices) { // MakeOptionalIndicesSet(create_struct_expr) return std::make_unique( - expr_id, std::move(name), std::move(field_keys), - std::move(optional_indices)); + std::move(name), std::move(field_keys), std::move(optional_indices)); } } // namespace google::api::expr::runtime diff --git a/eval/eval/create_struct_step.h b/eval/eval/create_struct_step.h index eb80634f8..11880493a 100644 --- a/eval/eval/create_struct_step.h +++ b/eval/eval/create_struct_step.h @@ -35,9 +35,9 @@ std::unique_ptr CreateDirectCreateStructStep( // Creates an `ExpressionStep` which performs `CreateStruct` for a // message/struct. -std::unique_ptr CreateCreateStructStep( +std::unique_ptr CreateCreateStructStep( std::string name, std::vector field_keys, - absl::flat_hash_set optional_indices, int64_t expr_id); + absl::flat_hash_set optional_indices); } // namespace google::api::expr::runtime diff --git a/eval/eval/create_struct_step_test.cc b/eval/eval/create_struct_step_test.cc index 666dddcdb..056b10267 100644 --- a/eval/eval/create_struct_step_test.cc +++ b/eval/eval/create_struct_step_test.cc @@ -72,16 +72,14 @@ using ::testing::Pointwise; absl::StatusOr MakeStackMachinePath(absl::string_view field) { ExecutionPath path; - CEL_ASSIGN_OR_RETURN(auto step0, CreateIdentStep("message", /*expr_id=*/-1)); + auto step0 = CreateIdentStep("message"); auto step1 = CreateCreateStructStep("google.api.expr.runtime.TestMessage", {std::string(field)}, - /*optional_indices=*/{}, + /*optional_indices=*/{}); - /*id=*/-1); - - path.push_back(std::move(step0)); - path.push_back(std::move(step1)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step0))); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); return path; } @@ -99,7 +97,8 @@ absl::StatusOr MakeRecursivePath(absl::string_view field) { /*id=*/-1); - path.push_back(std::make_unique(std::move(step1), -1)); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(std::move(step1)))); return path; } @@ -212,14 +211,13 @@ TEST_P(CreateCreateStructStepTest, TestEmptyMessageCreation) { /*deps=*/{}, /*optional_indices=*/{}, /*id=*/-1); - path.push_back( - std::make_unique(std::move(step), /*id=*/-1)); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(std::move(step)))); } else { auto step = CreateCreateStructStep("google.api.expr.runtime.TestMessage", /*fields=*/{}, - /*optional_indices=*/{}, - /*id=*/-1); - path.push_back(std::move(step)); + /*optional_indices=*/{}); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step))); } cel::RuntimeOptions options; @@ -279,7 +277,7 @@ TEST(CreateCreateStructStepTest, TestMessageCreateWithUnknown) { auto eval_status = RunExpression(env, "bool_value", CelValue::CreateUnknownSet(&unknown_set), &arena, true, /*enable_recursive_planning=*/false); - ASSERT_OK(eval_status); + ASSERT_THAT(eval_status, IsOk()); ASSERT_TRUE(eval_status->IsUnknownSet()); } @@ -293,7 +291,7 @@ TEST(CreateCreateStructStepTest, TestMessageCreateWithUnknownRecursive) { auto eval_status = RunExpression(env, "bool_value", CelValue::CreateUnknownSet(&unknown_set), &arena, true, /*enable_recursive_planning=*/true); - ASSERT_OK(eval_status); + ASSERT_THAT(eval_status, IsOk()); ASSERT_TRUE(eval_status->IsUnknownSet()) << eval_status->DebugString(); } 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 f11479065..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,24 +77,6 @@ class DirectExpressionStep { int64_t expr_id_; }; -// Wrapper for direct steps to work with the stack machine impl. -class WrappedDirectStep : public ExpressionStep { - public: - WrappedDirectStep(std::unique_ptr impl, int64_t expr_id) - : ExpressionStep(expr_id, false), 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 d720302e4..bd1d4649a 100644 --- a/eval/eval/equality_steps.cc +++ b/eval/eval/equality_steps.cc @@ -107,8 +107,7 @@ class DirectEqualityStep : public DirectExpressionStep { class IterativeEqualityStep : public ExpressionStepBase { public: - explicit IterativeEqualityStep(bool negation, int64_t expr_id) - : ExpressionStepBase(expr_id), negation_(negation) {} + explicit IterativeEqualityStep(bool negation) : negation_(negation) {} absl::Status Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(2)) { @@ -244,7 +243,7 @@ class DirectInStep : public DirectExpressionStep { class IterativeInStep : public ExpressionStepBase { public: - explicit IterativeInStep(int64_t expr_id) : ExpressionStepBase(expr_id) {} + IterativeInStep() = default; absl::Status Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(2)) { @@ -272,9 +271,8 @@ std::unique_ptr CreateDirectEqualityStep( } // Factory method for iterative _==_ and _!=_ Execution step -std::unique_ptr CreateEqualityStep(bool negation, - int64_t expr_id) { - return std::make_unique(negation, expr_id); +std::unique_ptr CreateEqualityStep(bool negation) { + return std::make_unique(negation); } // Factory method for recursive @in Execution step @@ -286,8 +284,8 @@ std::unique_ptr CreateDirectInStep( } // Factory method for iterative @in Execution step -std::unique_ptr CreateInStep(int64_t expr_id) { - return std::make_unique(expr_id); +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 eb3bec4ca..7058f9c56 100644 --- a/eval/eval/equality_steps.h +++ b/eval/eval/equality_steps.h @@ -29,8 +29,7 @@ std::unique_ptr CreateDirectEqualityStep( std::unique_ptr rhs, bool negation, int64_t expr_id); // Factory method for iterative _==_/_!=_ Execution step -std::unique_ptr CreateEqualityStep(bool negation, - int64_t expr_id); +std::unique_ptr CreateEqualityStep(bool negation); // Factory method for recursive @in Execution step std::unique_ptr CreateDirectInStep( @@ -38,7 +37,7 @@ std::unique_ptr CreateDirectInStep( std::unique_ptr container, int64_t expr_id); // Factory method for iterative @in Execution step -std::unique_ptr CreateInStep(int64_t expr_id); +std::unique_ptr CreateInStep(); } // namespace google::api::expr::runtime diff --git a/eval/eval/equality_steps_test.cc b/eval/eval/equality_steps_test.cc index 168ce7603..3b7e93626 100644 --- a/eval/eval/equality_steps_test.cc +++ b/eval/eval/equality_steps_test.cc @@ -51,15 +51,15 @@ using ::cel::ValueKind; using ::cel::test::BoolValueIs; using ::cel::test::ValueKindIs; -class ValueStep : public ExpressionStep, public DirectExpressionStep { +class ValueStep : public ExpressionStepLogic, public DirectExpressionStep { public: ValueStep(Value value, Attribute attr) - : ExpressionStep(-1), + : ExpressionStepLogic(), DirectExpressionStep(-1), value_(std::move(value)), attr_(std::move(attr)) {} explicit ValueStep(Value value) - : ExpressionStep(-1), + : ExpressionStepLogic(), DirectExpressionStep(-1), value_(std::move(value)), attr_() {} @@ -148,11 +148,12 @@ TEST(IterativeTest, PartialAttrUnknown) { cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); - std::vector> steps; - steps.push_back( - std::make_unique(IntValue(1), cel::Attribute("foo"))); - steps.push_back(std::make_unique(IntValue(2))); - steps.push_back(CreateEqualityStep(false, -1)); + ExecutionPath steps; + steps.push_back(ExpressionStep::MakeGenericStep( + 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))); activation.SetUnknownPatterns({cel::AttributePattern( "foo", {cel::AttributeQualifierPattern::OfString("bar")})}); @@ -178,11 +179,12 @@ TEST(IterativeTest, PartialAttrUnknownDisabled) { cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); - std::vector> steps; - steps.push_back( - std::make_unique(IntValue(1), cel::Attribute("foo"))); - steps.push_back(std::make_unique(IntValue(2))); - steps.push_back(CreateEqualityStep(false, -1)); + ExecutionPath steps; + steps.push_back(ExpressionStep::MakeGenericStep( + 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))); activation.SetUnknownPatterns({cel::AttributePattern( "foo", {cel::AttributeQualifierPattern::OfString("bar")})}); @@ -284,12 +286,13 @@ TEST_P(EqualsTest, Iterative) { cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); - std::vector> steps; - steps.push_back( - std::make_unique(MakeValue(test_case.lhs, &arena))); + ExecutionPath steps; + steps.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(MakeValue(test_case.lhs, &arena)))); + steps.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(MakeValue(test_case.rhs, &arena)))); steps.push_back( - std::make_unique(MakeValue(test_case.rhs, &arena))); - steps.push_back(CreateEqualityStep(test_case.negation, -1)); + ExpressionStep::MakeGenericStep(CreateEqualityStep(test_case.negation))); ExecutionFrame frame(steps, activation, opts, state); @@ -465,12 +468,12 @@ TEST_P(InTest, Iterative) { cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); - std::vector> steps; - steps.push_back( - std::make_unique(MakeValue(test_case.lhs, &arena))); - steps.push_back( - std::make_unique(MakeValue(test_case.rhs, &arena))); - steps.push_back(CreateInStep(-1)); + ExecutionPath steps; + steps.push_back(ExpressionStep::MakeGenericStep( + 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())); ExecutionFrame frame(steps, activation, opts, state); diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index 05dbed854..a87ec2c9b 100644 --- a/eval/eval/evaluator_core.cc +++ b/eval/eval/evaluator_core.cc @@ -15,6 +15,8 @@ #include "eval/eval/evaluator_core.h" #include +#include +#include #include #include @@ -26,6 +28,11 @@ #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "common/value.h" +#include "common/value_kind.h" +#include "eval/eval/attribute_trail.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 "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" @@ -44,7 +51,7 @@ const ExpressionStep* ExecutionFrame::Next() { const size_t end_pos = execution_path_.size(); if (ABSL_PREDICT_TRUE(pc_ < end_pos)) { - const auto* step = execution_path_[pc_++].get(); + const auto* step = &execution_path_[pc_++]; ABSL_ASSUME(step != nullptr); return step; } @@ -103,6 +110,62 @@ class EvaluationStatus final { } // namespace +void ExpressionStep::Evaluate(ExecutionFrame* context) const { + switch (header_.kind) { + case ExpressionStepKind::kGenericLogic: { + EvaluationStatus s(u_.logic->Evaluate(context)); + if (!s.ok()) { + context->Abort(std::move(s).Consume()); + } + break; + } + case ExpressionStepKind::kIntConstant: + context->value_stack().Push(cel::IntValue(u_.int_val)); + break; + case ExpressionStepKind::kBoolConstant: + context->value_stack().Push(cel::BoolValue(u_.bool_val)); + break; + case ExpressionStepKind::kDoubleConstant: + context->value_stack().Push(cel::DoubleValue(u_.double_val)); + break; + case ExpressionStepKind::kNullConstant: + context->value_stack().Push(cel::NullValue()); + break; + case ExpressionStepKind::kUintConstant: + context->value_stack().Push(cel::UintValue(u_.uint_val)); + break; + case ExpressionStepKind::kOtherConstant: + context->value_stack().Push(*u_.other_val); + break; + case ExpressionStepKind::kLazyInit: + EvaluateLazyInitStep(u_.lazy_init, *context); + break; + case ExpressionStepKind::kAssignSlotAndPop: + EvaluateAssignSlotAndPop(u_.slot_index, *context); + break; + case ExpressionStepKind::kClearSlots: + EvaluateClearSlotStep(u_.clear_slots, *context); + break; + case ExpressionStepKind::kBooleanNot: + EvaluateNotStep(*context); + break; + case ExpressionStepKind::kNotStrictlyFalse: + EvaluateNotStrictlyFalseStep(*context); + break; + case ExpressionStepKind::kBooleanOr: + EvaluateBoolLogicStep(BoolLogicKind::kOr, u_.arg_count, *context); + break; + case ExpressionStepKind::kBooleanAnd: + EvaluateBoolLogicStep(BoolLogicKind::kAnd, u_.arg_count, *context); + break; + case ExpressionStepKind::kMovedFrom: + default: + context->Abort( + absl::InternalError("ExpressionStep::Evaluate called on moved-from " + "object")); + } +} + absl::StatusOr ExecutionFrame::Evaluate( EvaluationListener& listener) { const size_t initial_stack_size = value_stack().size(); @@ -110,18 +173,13 @@ absl::StatusOr ExecutionFrame::Evaluate( 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(); - } + expr->Evaluate(this); } } 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()) { + expr->Evaluate(this); + if (pc_ == 0 || !expr->comes_from_ast() || !abort_status().ok()) { // Skip if we just started a Call or if the step doesn't map to an // AST id. continue; @@ -141,6 +199,10 @@ absl::StatusOr ExecutionFrame::Evaluate( } } + if (!abort_status().ok()) { + return std::move(abort_status()); + } + const size_t final_stack_size = value_stack().size(); if (ABSL_PREDICT_FALSE(final_stack_size != initial_stack_size + 1 || final_stack_size == 0)) { @@ -175,4 +237,138 @@ absl::StatusOr FlatExpression::EvaluateWithCallback( return frame.Evaluate(frame.callback()); } +void ExpressionStep::SwapToEmpty(ExpressionStep& step, + ExpressionStep& empty_step) { + ABSL_DCHECK(empty_step.header_.kind == ExpressionStepKind::kMovedFrom); + using std::swap; + swap(step.header_, empty_step.header_); + switch (empty_step.header_.kind) { + case ExpressionStepKind::kGenericLogic: + empty_step.u_.logic = std::move(step.u_.logic); + break; + case ExpressionStepKind::kBoolConstant: + empty_step.u_.bool_val = step.u_.bool_val; + break; + case ExpressionStepKind::kIntConstant: + empty_step.u_.int_val = step.u_.int_val; + break; + case ExpressionStepKind::kUintConstant: + empty_step.u_.uint_val = step.u_.uint_val; + break; + case ExpressionStepKind::kDoubleConstant: + empty_step.u_.double_val = step.u_.double_val; + break; + case ExpressionStepKind::kNullConstant: + break; + case ExpressionStepKind::kOtherConstant: + empty_step.u_.other_val = std::move(step.u_.other_val); + break; + case ExpressionStepKind::kLazyInit: + empty_step.u_.lazy_init = step.u_.lazy_init; + break; + case ExpressionStepKind::kAssignSlotAndPop: + empty_step.u_.slot_index = step.u_.slot_index; + break; + case ExpressionStepKind::kClearSlots: + empty_step.u_.clear_slots = step.u_.clear_slots; + break; + case ExpressionStepKind::kBooleanNot: + case ExpressionStepKind::kNotStrictlyFalse: + break; + case ExpressionStepKind::kBooleanOr: + case ExpressionStepKind::kBooleanAnd: + empty_step.u_.arg_count = step.u_.arg_count; + break; + case ExpressionStepKind::kMovedFrom: + break; + default: + ABSL_UNREACHABLE(); + } + step.u_.empty = nullptr; +} + +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 = std::make_unique(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; + } +} + +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(); +} + } // namespace google::api::expr::runtime diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index 575abfa05..108ce1e26 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -17,11 +17,13 @@ #include #include +#include #include #include #include #include "absl/base/nullability.h" +#include "absl/base/optimization.h" #include "absl/log/absl_check.h" #include "absl/status/status.h" #include "absl/status/statusor.h" @@ -33,8 +35,11 @@ #include "common/value.h" #include "eval/eval/attribute_utility.h" #include "eval/eval/comprehension_slots.h" +#include "eval/eval/direct_expression_step.h" #include "eval/eval/evaluator_stack.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/runtime.h" @@ -54,25 +59,36 @@ class ExecutionFrame; using EvaluationListener = cel::TraceableProgram::EvaluationListener; -// Class Expression represents single execution step. +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, + kLazyInit = 8, + kAssignSlotAndPop = 9, + kClearSlots = 10, + kBooleanNot = 11, + kNotStrictlyFalse = 12, + kBooleanOr = 13, + kBooleanAnd = 14, +}; + class ExpressionStep { public: - explicit ExpressionStep(int64_t id, bool comes_from_ast = true) - : id_(id), comes_from_ast_(comes_from_ast) {} - + // Move-only. ExpressionStep(const ExpressionStep&) = delete; ExpressionStep& operator=(const ExpressionStep&) = delete; + ExpressionStep(ExpressionStep&&); + ExpressionStep& operator=(ExpressionStep&&); - virtual ~ExpressionStep() = 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; + ~ExpressionStep(); // Returns corresponding expression object ID. // Requires that the input expression has IDs assigned to sub-expressions, @@ -80,10 +96,149 @@ class ExpressionStep { // expression associated (e.g. a jump step), or if there is no ID assigned to // the corresponding expression. Useful for error scenarios where information // from Expr object is needed to create CelError. - int64_t id() const { return id_; } + int64_t id() const { + return header_.id >= 0 ? static_cast(header_.id) : -1; + } // Returns if the execution step comes from AST. - bool comes_from_ast() const { return comes_from_ast_; } + bool comes_from_ast() const { return header_.id >= 0; } + + void Evaluate(ExecutionFrame* context) const; + + const ExpressionStepLogic* GetGenericStep() const; + bool IsGenericStep() const; + + static ExpressionStep MakeGenericStep( + std::unique_ptr logic, int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kGenericLogic, id); + step.u_.logic = std::move(logic); + 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_LT(slot_index, std::numeric_limits::max()); + ABSL_DCHECK_LT(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_LT(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_LT(slot_index, std::numeric_limits::max()); + ABSL_DCHECK_LT(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; + } + + private: + struct Header { + ExpressionStepKind kind; + uint16_t reserved; + int32_t id; + }; + + 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) + : ExpressionStep(kind, id) { + u_.logic = std::move(logic); + } + + static void SwapToEmpty(ExpressionStep& step, ExpressionStep& empty_step); + + friend void swap(ExpressionStep& lhs, ExpressionStep& rhs) { + ExpressionStep tmp; + SwapToEmpty(lhs, tmp); + SwapToEmpty(rhs, lhs); + SwapToEmpty(tmp, rhs); + } + + friend bool GetIfConstant(const ExpressionStep& step, cel::Value& out); + friend bool IsConstant(const ExpressionStep& step); + + Header header_; + union Data { + std::nullptr_t empty; + std::unique_ptr logic; + int64_t int_val; + uint64_t uint_val; + double double_val; + bool bool_val; + std::unique_ptr other_val; + LazyInitStepInfo lazy_init; + size_t slot_index; + ClearSlotStepInfo clear_slots; + size_t arg_count; + + Data() : empty(nullptr) {} + ~Data() {} + } u_; +}; + +static_assert(sizeof(ExpressionStep) == 16); + +// Class Expression represents single execution step. +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). + 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 @@ -91,15 +246,29 @@ class ExpressionStep { virtual cel::NativeTypeId GetNativeTypeId() const { return cel::NativeTypeId(); } +}; + +// 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: - const int64_t id_; - const bool comes_from_ast_; + std::unique_ptr impl_; }; -using ExecutionPath = std::vector>; -using ExecutionPathView = - absl::Span>; +using ExecutionPath = std::vector; +using ExecutionPathView = absl::Span; // Class that wraps the state that needs to be allocated for expression // evaluation. This can be reused to save on allocations. @@ -281,6 +450,8 @@ class ExecutionFrameBase { return absl::OkStatus(); } + absl::Status& abort_status() { return abort_status_; } + protected: const cel::ActivationInterface* absl_nonnull activation_; EvaluationListener callback_; @@ -294,6 +465,7 @@ class ExecutionFrameBase { ComprehensionSlots* absl_nonnull slots_; const int max_iterations_; int iterations_; + absl::Status abort_status_; }; // ExecutionFrame manages the context needed for expression evaluation. @@ -318,7 +490,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, @@ -375,7 +547,7 @@ class ExecutionFrame : public ExecutionFrameBase { void Call(size_t slot_index, size_t subexpression_index) { ABSL_DCHECK_LT(subexpression_index, subexpressions_.size()); ExecutionPathView subexpression = subexpressions_[subexpression_index]; - ABSL_DCHECK(subexpression != execution_path_); + ABSL_DCHECK(subexpression.data() != execution_path_.data()); size_t return_pc = pc_; // return pc == size() is supported (a tail call). ABSL_DCHECK_LE(return_pc, execution_path_.size()); @@ -418,6 +590,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; @@ -511,6 +692,68 @@ class FlatExpression { absl_nullable std::shared_ptr arena_; }; +// Helper functions for checking ExpressionStep kinds. Used for program +// optimization. + +// 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); + +// Implementation details. + +inline ExpressionStep::~ExpressionStep() { + switch (header_.kind) { + case ExpressionStepKind::kGenericLogic: + u_.logic.reset(); + break; + case ExpressionStepKind::kOtherConstant: + u_.other_val.reset(); + 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: + break; + default: + 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.get(); +} + +inline bool ExpressionStep::IsGenericStep() const { + return header_.kind == ExpressionStepKind::kGenericLogic; +} + +inline ExpressionStep::ExpressionStep(ExpressionStep&& other) + : ExpressionStep() { + SwapToEmpty(other, *this); +} + +inline ExpressionStep& ExpressionStep::operator=(ExpressionStep&& other) { + ExpressionStep temp; + SwapToEmpty(*this, temp); + SwapToEmpty(other, *this); + return *this; +} + } // namespace google::api::expr::runtime #endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_EVALUATOR_CORE_H_ diff --git a/eval/eval/evaluator_core_test.cc b/eval/eval/evaluator_core_test.cc index 8d61c4659..dc449564c 100644 --- a/eval/eval/evaluator_core_test.cc +++ b/eval/eval/evaluator_core_test.cc @@ -1,8 +1,10 @@ #include "eval/eval/evaluator_core.h" #include +#include #include #include +#include #include "cel/expr/syntax.pb.h" #include "absl/status/status.h" @@ -25,6 +27,7 @@ namespace google::api::expr::runtime { +using ::absl_testing::IsOk; using ::cel::IntValue; using ::cel::TypeProvider; using ::cel::interop_internal::CreateIntValue; @@ -32,13 +35,14 @@ using ::cel::runtime_internal::NewTestingRuntimeEnv; using ::cel::expr::Expr; using ::google::api::expr::runtime::RegisterBuiltinFunctions; using ::testing::_; +using ::testing::ElementsAre; using ::testing::Eq; // Fake expression implementation // Pushes int64(0) on top of value stack. -class FakeConstExpressionStep : public ExpressionStep { +class FakeConstExpressionStep : public ExpressionStepLogic { public: - FakeConstExpressionStep() : ExpressionStep(0, true) {} + FakeConstExpressionStep() = default; absl::Status Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Push(CreateIntValue(0)); @@ -48,9 +52,9 @@ class FakeConstExpressionStep : public ExpressionStep { // Fake expression implementation // Increments argument on top of the stack. -class FakeIncrementExpressionStep : public ExpressionStep { +class FakeIncrementExpressionStep : public ExpressionStepLogic { public: - FakeIncrementExpressionStep() : ExpressionStep(0, true) {} + FakeIncrementExpressionStep() = default; absl::Status Evaluate(ExecutionFrame* frame) const override { auto value = frame->value_stack().Peek(); @@ -67,15 +71,13 @@ TEST(EvaluatorCoreTest, ExecutionFrameNext) { google::protobuf::Arena arena; cel::runtime_internal::RuntimeTypeProvider type_provider( cel::internal::GetTestingDescriptorPool()); - auto const_step = std::make_unique(); - auto incr_step1 = std::make_unique(); - auto incr_step2 = std::make_unique(); - path.push_back(std::move(const_step)); - path.push_back(std::move(incr_step1)); - path.push_back(std::move(incr_step2)); - - auto dummy_expr = std::make_unique(); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique())); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique())); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique())); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kDisabled; @@ -87,21 +89,20 @@ TEST(EvaluatorCoreTest, ExecutionFrameNext) { cel::internal::GetTestingMessageFactory(), &arena); ExecutionFrame frame(path, activation, options, state); - EXPECT_THAT(frame.Next(), Eq(path[0].get())); - EXPECT_THAT(frame.Next(), Eq(path[1].get())); - EXPECT_THAT(frame.Next(), Eq(path[2].get())); + EXPECT_THAT(frame.Next(), Eq(&path[0])); + EXPECT_THAT(frame.Next(), Eq(&path[1])); + EXPECT_THAT(frame.Next(), Eq(&path[2])); EXPECT_THAT(frame.Next(), Eq(nullptr)); } TEST(EvaluatorCoreTest, SimpleEvaluatorTest) { ExecutionPath path; - auto const_step = std::make_unique(); - auto incr_step1 = std::make_unique(); - auto incr_step2 = std::make_unique(); - - path.push_back(std::move(const_step)); - path.push_back(std::move(incr_step1)); - path.push_back(std::move(incr_step2)); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique())); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique())); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique())); auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( @@ -113,19 +114,105 @@ TEST(EvaluatorCoreTest, SimpleEvaluatorTest) { google::protobuf::Arena arena; auto status = impl.Evaluate(activation, &arena); - EXPECT_OK(status); + ASSERT_THAT(status, IsOk()); auto value = status.value(); EXPECT_TRUE(value.IsInt64()); 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("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, (int64_t expr_id, const CelValue& value, google::protobuf::Arena*)); }; +TEST(EvaluatorCoreTest, TraceFilterById) { + ExecutionPath path; + // Step with ID 0 should trigger trace callback. + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(), /*id=*/0)); + // Steps with large IDs (> int32_t max) should not trigger trace callback. + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(), + /*id=*/static_cast(std::numeric_limits::max()) + 1)); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(), + /*id=*/std::numeric_limits::max())); + // Steps with negative IDs should not trigger trace callback. + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(), /*id=*/-1)); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(), /*id=*/-100)); + + auto env = NewTestingRuntimeEnv(); + CelExpressionFlatImpl impl( + env, FlatExpression(std::move(path), 0, + env->type_registry.GetComposedTypeProvider(), + cel::RuntimeOptions{})); + + Activation activation; + google::protobuf::Arena arena; + + std::vector traced_ids; + auto eval_status = impl.Trace( + activation, &arena, + [&](int64_t expr_id, const CelValue& value, google::protobuf::Arena* arena) { + traced_ids.push_back(expr_id); + return absl::OkStatus(); + }); + ASSERT_THAT(eval_status, IsOk()); + EXPECT_THAT(traced_ids, ElementsAre(0)); +} + TEST(EvaluatorCoreTest, TraceTest) { Expr expr; cel::expr::SourceInfo source_info; @@ -186,7 +273,7 @@ TEST(EvaluatorCoreTest, TraceTest) { cel::RuntimeOptions options; options.short_circuiting = false; CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv(), options); - ASSERT_OK(RegisterBuiltinFunctions(builder.GetRegistry())); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry()), IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression(&expr, &source_info)); @@ -218,7 +305,7 @@ TEST(EvaluatorCoreTest, TraceTest) { callback.Call(expr_id, value, arena); return absl::OkStatus(); }); - ASSERT_OK(eval_status); + ASSERT_THAT(eval_status, IsOk()); } } // namespace google::api::expr::runtime diff --git a/eval/eval/expression_step_base.h b/eval/eval/expression_step_base.h index 5b2f72f8e..565dc5fa5 100644 --- a/eval/eval/expression_step_base.h +++ b/eval/eval/expression_step_base.h @@ -1,11 +1,18 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_EXPRESSION_STEP_BASE_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_EXPRESSION_STEP_BASE_H_ +#include + #include "eval/eval/evaluator_core.h" namespace google::api::expr::runtime { -using ExpressionStepBase = ExpressionStep; +class ExpressionStepBase : public ExpressionStepLogic { + public: + ExpressionStepBase() = default; + explicit ExpressionStepBase(int64_t /*expr_id*/, + bool /*comes_from_ast*/ = true) {} +}; } // namespace google::api::expr::runtime diff --git a/eval/eval/function_step.cc b/eval/eval/function_step.cc index 12c5af8a7..7e1efea28 100644 --- a/eval/eval/function_step.cc +++ b/eval/eval/function_step.cc @@ -157,7 +157,8 @@ class AbstractFunctionStep : public ExpressionStepBase { : ExpressionStepBase(expr_id), name_(name), num_arguments_(num_arguments), - receiver_style_(receiver_style) {} + receiver_style_(receiver_style), + expr_id_(expr_id) {} absl::Status Evaluate(ExecutionFrame* frame) const override; @@ -177,6 +178,7 @@ class AbstractFunctionStep : public ExpressionStepBase { std::string name_; size_t num_arguments_; bool receiver_style_; + int64_t expr_id_; }; inline absl::StatusOr Invoke( @@ -262,7 +264,7 @@ absl::StatusOr AbstractFunctionStep::DoEvaluate( // Overload found and is allowed to consume the arguments. if (matched_function.has_value() && ShouldAcceptOverload(matched_function->descriptor, input_args)) { - return Invoke(*matched_function, id(), input_args, *frame); + return Invoke(*matched_function, expr_id_, input_args, *frame); } return NoOverloadResult(name_, input_args, receiver_style_, *frame); @@ -506,7 +508,7 @@ std::unique_ptr CreateDirectLazyFunctionStep( LazyResolver(std::move(providers), call.function(), call.has_target())); } -absl::StatusOr> CreateFunctionStep( +absl::StatusOr> CreateFunctionStep( const cel::CallExpr& call_expr, int64_t expr_id, std::vector lazy_overloads) { bool receiver_style = call_expr.has_target(); @@ -516,7 +518,7 @@ absl::StatusOr> CreateFunctionStep( std::move(lazy_overloads), expr_id); } -absl::StatusOr> CreateFunctionStep( +absl::StatusOr> CreateFunctionStep( const cel::CallExpr& call_expr, int64_t expr_id, std::vector overloads) { bool receiver_style = call_expr.has_target(); diff --git a/eval/eval/function_step.h b/eval/eval/function_step.h index 9f664dc09..94307609e 100644 --- a/eval/eval/function_step.h +++ b/eval/eval/function_step.h @@ -32,14 +32,14 @@ std::unique_ptr CreateDirectLazyFunctionStep( // Factory method for Call-based execution step where the function will be // resolved at runtime (lazily) from an input Activation. -absl::StatusOr> CreateFunctionStep( +absl::StatusOr> CreateFunctionStep( 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( +absl::StatusOr> CreateFunctionStep( const cel::CallExpr& call, int64_t expr_id, std::vector overloads); diff --git a/eval/eval/function_step_test.cc b/eval/eval/function_step_test.cc index 3d3bae34d..102fdfaf0 100644 --- a/eval/eval/function_step_test.cc +++ b/eval/eval/function_step_test.cc @@ -216,7 +216,8 @@ std::unique_ptr CreateExpressionImpl( const cel::RuntimeOptions& options, std::unique_ptr expr) { ExecutionPath path; - path.push_back(std::make_unique(std::move(expr), -1)); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(std::move(expr)))); auto env = NewTestingRuntimeEnv(); return std::make_unique( @@ -225,17 +226,21 @@ std::unique_ptr CreateExpressionImpl( env->type_registry.GetComposedTypeProvider(), options)); } -absl::StatusOr> MakeTestFunctionStep( +absl::StatusOr MakeTestFunctionStep( const CallExpr& call, const CelFunctionRegistry& registry) { auto argument_matcher = ArgumentMatcher(call); auto lazy_overloads = registry.ModernFindLazyOverloads( call.function(), call.has_target(), argument_matcher); + int id = GetExprId(); if (!lazy_overloads.empty()) { - return CreateFunctionStep(call, GetExprId(), lazy_overloads); + CEL_ASSIGN_OR_RETURN(auto logic, + CreateFunctionStep(call, id, lazy_overloads)); + return ExpressionStep::MakeGenericStep(std::move(logic), id); } auto overloads = registry.FindStaticOverloads( call.function(), call.has_target(), argument_matcher); - return CreateFunctionStep(call, GetExprId(), overloads); + CEL_ASSIGN_OR_RETURN(auto logic, CreateFunctionStep(call, id, overloads)); + return ExpressionStep::MakeGenericStep(std::move(logic), id); } // Test common functions with varying levels of unknown support. @@ -397,7 +402,7 @@ TEST_P(FunctionStepTest, TestNoMatchingOverloadsUnexpectedArgCount) { ASSERT_OK_AND_ASSIGN(auto step2, MakeTestFunctionStep(call1, registry)); ASSERT_OK_AND_ASSIGN( - auto step3, + auto step3_logic, CreateFunctionStep(add_call, -1, registry.FindStaticOverloads( add_call.function(), false, @@ -406,7 +411,7 @@ TEST_P(FunctionStepTest, TestNoMatchingOverloadsUnexpectedArgCount) { path.push_back(std::move(step0)); path.push_back(std::move(step1)); path.push_back(std::move(step2)); - path.push_back(std::move(step3)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step3_logic))); std::unique_ptr impl = GetExpression(std::move(path)); @@ -535,13 +540,11 @@ TEST_P(FunctionStepTest, LazyFunctionOverloadingTest) { lt_call.mutable_args().emplace_back(); lt_call.set_function("_<_"); - ASSERT_OK_AND_ASSIGN( - auto step0, - CreateConstValueStep(cel::interop_internal::CreateIntValue(20), -1)); + auto step0 = + ExpressionStep::MakeConstant(cel::interop_internal::CreateIntValue(20)); ASSERT_OK_AND_ASSIGN(auto step1, MakeTestFunctionStep(call1, registry)); - ASSERT_OK_AND_ASSIGN( - auto step2, - CreateConstValueStep(cel::interop_internal::CreateDoubleValue(21.9), -1)); + 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)); @@ -675,8 +678,8 @@ TEST_P(FunctionStepTestUnknowns, PartialUnknownHandlingTest) { IdentExpr ident1; ident1.set_name("param"); CallExpr call1 = SinkFunction::MakeCall(); - - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("param", GetExprId())); + auto step0 = + ExpressionStep::MakeGenericStep(CreateIdentStep("param"), GetExprId()); ASSERT_OK_AND_ASSIGN(auto step1, MakeTestFunctionStep(call1, registry)); path.push_back(std::move(step0)); @@ -987,17 +990,17 @@ TEST(FunctionStepStrictnessTest, IfFunctionStrictAndGivenUnknownSkipsInvocation) { UnknownSet unknown_set; CelFunctionRegistry registry; - ASSERT_OK(registry.Register(std::make_unique( - CelValue::CreateUnknownSet(&unknown_set), "ConstUnknown"))); - ASSERT_OK(registry.Register(std::make_unique( - CelValue::Type::kUnknownSet, /*is_strict=*/true))); + ASSERT_THAT(registry.Register(std::make_unique( + CelValue::CreateUnknownSet(&unknown_set), "ConstUnknown")), + IsOk()); + ASSERT_THAT(registry.Register(std::make_unique( + CelValue::Type::kUnknownSet, /*is_strict=*/true)), + IsOk()); ExecutionPath path; CallExpr call0 = ConstFunction::MakeCall("ConstUnknown"); CallExpr call1 = SinkFunction::MakeCall(); - ASSERT_OK_AND_ASSIGN(std::unique_ptr step0, - MakeTestFunctionStep(call0, registry)); - ASSERT_OK_AND_ASSIGN(std::unique_ptr step1, - MakeTestFunctionStep(call1, registry)); + ASSERT_OK_AND_ASSIGN(auto step0, MakeTestFunctionStep(call0, registry)); + ASSERT_OK_AND_ASSIGN(auto step1, MakeTestFunctionStep(call1, registry)); path.push_back(std::move(step0)); path.push_back(std::move(step1)); cel::RuntimeOptions options; @@ -1018,17 +1021,17 @@ TEST(FunctionStepStrictnessTest, TEST(FunctionStepStrictnessTest, IfFunctionNonStrictAndGivenUnknownInvokesIt) { UnknownSet unknown_set; CelFunctionRegistry registry; - ASSERT_OK(registry.Register(std::make_unique( - CelValue::CreateUnknownSet(&unknown_set), "ConstUnknown"))); - ASSERT_OK(registry.Register(std::make_unique( - CelValue::Type::kUnknownSet, /*is_strict=*/false))); + ASSERT_THAT(registry.Register(std::make_unique( + CelValue::CreateUnknownSet(&unknown_set), "ConstUnknown")), + IsOk()); + ASSERT_THAT(registry.Register(std::make_unique( + CelValue::Type::kUnknownSet, /*is_strict=*/false)), + IsOk()); ExecutionPath path; CallExpr call0 = ConstFunction::MakeCall("ConstUnknown"); CallExpr call1 = SinkFunction::MakeCall(); - ASSERT_OK_AND_ASSIGN(std::unique_ptr step0, - MakeTestFunctionStep(call0, registry)); - ASSERT_OK_AND_ASSIGN(std::unique_ptr step1, - MakeTestFunctionStep(call1, registry)); + ASSERT_OK_AND_ASSIGN(auto step0, MakeTestFunctionStep(call0, registry)); + ASSERT_OK_AND_ASSIGN(auto step1, MakeTestFunctionStep(call1, registry)); path.push_back(std::move(step0)); path.push_back(std::move(step1)); Expr placeholder_expr; diff --git a/eval/eval/ident_step.cc b/eval/eval/ident_step.cc index 7ec1a3031..6852af065 100644 --- a/eval/eval/ident_step.cc +++ b/eval/eval/ident_step.cc @@ -29,8 +29,7 @@ using ::cel::runtime_internal::CreateError; class IdentStep : public ExpressionStepBase { public: - IdentStep(absl::string_view name, int64_t expr_id) - : ExpressionStepBase(expr_id), name_(name) {} + explicit IdentStep(absl::string_view name) : name_(name) {} absl::Status Evaluate(ExecutionFrame* frame) const override; @@ -95,8 +94,8 @@ absl::StatusOr LookupSlot( class SlotStep : public ExpressionStepBase { public: - SlotStep(absl::string_view name, size_t slot_index, int64_t expr_id) - : ExpressionStepBase(expr_id), name_(name), slot_index_(slot_index) {} + 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, @@ -161,14 +160,14 @@ std::unique_ptr CreateDirectSlotIdentStep( return std::make_unique(identifier, slot_index, expr_id); } -absl::StatusOr> CreateIdentStep( - const absl::string_view name, int64_t expr_id) { - return std::make_unique(name, expr_id); +std::unique_ptr CreateIdentStep( + const absl::string_view name) { + return std::make_unique(name); } -absl::StatusOr> CreateIdentStepForSlot( - const absl::string_view name, size_t slot_index, int64_t expr_id) { - return std::make_unique(name, slot_index, expr_id); +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 d1bdde388..ad65478bd 100644 --- a/eval/eval/ident_step.h +++ b/eval/eval/ident_step.h @@ -19,12 +19,11 @@ std::unique_ptr CreateDirectSlotIdentStep( absl::string_view identifier, size_t slot_index, int64_t expr_id); // Factory method for Ident - based Execution step -absl::StatusOr> CreateIdentStep( - absl::string_view name, int64_t expr_id); +std::unique_ptr CreateIdentStep(absl::string_view name); // Factory method for identifier that has been assigned to a slot. -absl::StatusOr> CreateIdentStepForSlot( - absl::string_view name, size_t slot_index, int64_t expr_id); +std::unique_ptr CreateIdentStepForSlot( + absl::string_view name, size_t slot_index); } // namespace google::api::expr::runtime diff --git a/eval/eval/ident_step_test.cc b/eval/eval/ident_step_test.cc index da5c0dc84..7be2a32dc 100644 --- a/eval/eval/ident_step_test.cc +++ b/eval/eval/ident_step_test.cc @@ -29,6 +29,7 @@ namespace google::api::expr::runtime { namespace { +using ::absl_testing::IsOk; using ::absl_testing::StatusIs; using ::cel::Cast; using ::cel::ErrorValue; @@ -46,10 +47,10 @@ using ::testing::HasSubstr; using ::testing::SizeIs; TEST(IdentStepTest, TestIdentStep) { - ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep("name0", /*id=*/-1)); + auto step = CreateIdentStep("name0"); ExecutionPath path; - path.push_back(std::move(step)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step))); auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( @@ -63,7 +64,7 @@ TEST(IdentStepTest, TestIdentStep) { activation.InsertValue("name0", CelValue::CreateString(&value)); auto status0 = impl.Evaluate(activation, &arena); - ASSERT_OK(status0); + ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -72,10 +73,10 @@ TEST(IdentStepTest, TestIdentStep) { } TEST(IdentStepTest, TestIdentStepNameNotFound) { - ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep("name0", /*id=*/-1)); + auto step = CreateIdentStep("name0"); ExecutionPath path; - path.push_back(std::move(step)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step))); auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( @@ -88,17 +89,17 @@ TEST(IdentStepTest, TestIdentStepNameNotFound) { std::string value("test"); auto status0 = impl.Evaluate(activation, &arena); - ASSERT_OK(status0); + ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); ASSERT_TRUE(result.IsError()); } TEST(IdentStepTest, DisableMissingAttributeErrorsOK) { - ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep("name0", /*id=*/-1)); + auto step = CreateIdentStep("name0"); ExecutionPath path; - path.push_back(std::move(step)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step))); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kDisabled; auto env = NewTestingRuntimeEnv(); @@ -114,7 +115,7 @@ TEST(IdentStepTest, DisableMissingAttributeErrorsOK) { activation.InsertValue("name0", CelValue::CreateString(&value)); auto status0 = impl.Evaluate(activation, &arena); - ASSERT_OK(status0); + ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -125,16 +126,16 @@ TEST(IdentStepTest, DisableMissingAttributeErrorsOK) { activation.set_missing_attribute_patterns({pattern}); status0 = impl.Evaluate(activation, &arena); - ASSERT_OK(status0); + ASSERT_THAT(status0, IsOk()); EXPECT_THAT(status0->StringOrDie().value(), Eq("test")); } TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { - ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep("name0", /*expr_id=*/1)); + auto step = CreateIdentStep("name0"); ExecutionPath path; - path.push_back(std::move(step)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step))); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kDisabled; @@ -153,7 +154,7 @@ TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { activation.InsertValue("name0", CelValue::CreateString(&value)); auto status0 = impl.Evaluate(activation, &arena); - ASSERT_OK(status0); + ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -164,17 +165,17 @@ TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { activation.set_missing_attribute_patterns({pattern}); status0 = impl.Evaluate(activation, &arena); - ASSERT_OK(status0); + ASSERT_THAT(status0, IsOk()); EXPECT_EQ(status0->ErrorOrDie()->code(), absl::StatusCode::kInvalidArgument); EXPECT_EQ(status0->ErrorOrDie()->message(), "MissingAttributeError: name0"); } TEST(IdentStepTest, TestIdentStepUnknownAttribute) { - ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep("name0", /*expr_id=*/1)); + auto step = CreateIdentStep("name0"); ExecutionPath path; - path.push_back(std::move(step)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step))); // Expression with unknowns enabled. cel::RuntimeOptions options; @@ -196,7 +197,7 @@ TEST(IdentStepTest, TestIdentStepUnknownAttribute) { activation.set_unknown_attribute_patterns(unknown_patterns); auto status0 = impl.Evaluate(activation, &arena); - ASSERT_OK(status0); + ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -207,7 +208,7 @@ TEST(IdentStepTest, TestIdentStepUnknownAttribute) { activation.set_unknown_attribute_patterns(unknown_patterns); status0 = impl.Evaluate(activation, &arena); - ASSERT_OK(status0); + ASSERT_THAT(status0, IsOk()); result = status0.value(); @@ -231,7 +232,7 @@ TEST(DirectIdentStepTest, Basic) { auto step = CreateDirectIdentStep("var1", -1); - ASSERT_OK(step->Evaluate(frame, result, trail)); + ASSERT_THAT(step->Evaluate(frame, result, trail), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).NativeValue(), Eq(42)); @@ -256,7 +257,7 @@ TEST(DirectIdentStepTest, UnknownAttribute) { auto step = CreateDirectIdentStep("var1", -1); - ASSERT_OK(step->Evaluate(frame, result, trail)); + ASSERT_THAT(step->Evaluate(frame, result, trail), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).ToAttributeSet(), SizeIs(1)); @@ -281,7 +282,7 @@ TEST(DirectIdentStepTest, MissingAttribute) { auto step = CreateDirectIdentStep("var1", -1); - ASSERT_OK(step->Evaluate(frame, result, trail)); + ASSERT_THAT(step->Evaluate(frame, result, trail), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).NativeValue(), @@ -303,7 +304,7 @@ TEST(DirectIdentStepTest, NotFound) { auto step = CreateDirectIdentStep("var1", -1); - ASSERT_OK(step->Evaluate(frame, result, trail)); + ASSERT_THAT(step->Evaluate(frame, result, trail), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).NativeValue(), diff --git a/eval/eval/jump_step.cc b/eval/eval/jump_step.cc index 243a02e8a..2aaf6bb09 100644 --- a/eval/eval/jump_step.cc +++ b/eval/eval/jump_step.cc @@ -38,8 +38,8 @@ using ::cel::runtime_internal::CreateNoMatchingOverloadError; class JumpStep : public JumpStepBase { public: // Constructs FunctionStep that uses overloads specified. - JumpStep(absl::optional jump_offset, int64_t expr_id) - : JumpStepBase(jump_offset, expr_id) {} + explicit JumpStep(absl::optional jump_offset) + : JumpStepBase(jump_offset) {} absl::Status Evaluate(ExecutionFrame* frame) const override { return Jump(frame); @@ -49,8 +49,8 @@ class JumpStep : public JumpStepBase { class CondJumpStep : public JumpStepBase { public: CondJumpStep(bool jump_condition, absl::optional jump_offset, - size_t stack_size, int64_t expr_id) - : JumpStepBase(jump_offset, expr_id), + size_t stack_size) + : JumpStepBase(jump_offset), jump_condition_(jump_condition), stack_size_(stack_size) {} @@ -79,8 +79,8 @@ class CondJumpStep : public JumpStepBase { class TernaryCondJumpStep : public JumpStepBase { public: - TernaryCondJumpStep(absl::optional jump_offset, int64_t expr_id) - : JumpStepBase(jump_offset, expr_id) {} + explicit TernaryCondJumpStep(absl::optional jump_offset) + : JumpStepBase(jump_offset) {} absl::Status Evaluate(ExecutionFrame* frame) const override { // Peek the top value @@ -109,8 +109,8 @@ class BoolCheckJumpStep : public JumpStepBase { // - 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 - BoolCheckJumpStep(absl::optional jump_offset, int64_t expr_id) - : JumpStepBase(jump_offset, expr_id) {} + explicit BoolCheckJumpStep(absl::optional jump_offset) + : JumpStepBase(jump_offset) {} absl::Status Evaluate(ExecutionFrame* frame) const override { // Peek the top value @@ -144,29 +144,28 @@ class BoolCheckJumpStep : public JumpStepBase { // Factory method for Conditional Jump step. std::unique_ptr CreateCondJumpStep( bool jump_condition, absl::optional jump_offset, - size_t expected_stack_size, int64_t expr_id) { + size_t expected_stack_size) { return std::make_unique(jump_condition, jump_offset, - expected_stack_size, expr_id); + expected_stack_size); } // Factory method for Ternary Conditional Jump step. std::unique_ptr CreateTernaryCondJumpStep( - absl::optional jump_offset, int64_t expr_id) { - return std::make_unique(jump_offset, expr_id); + absl::optional jump_offset) { + return std::make_unique(jump_offset); } // Factory method for Jump step. -std::unique_ptr CreateJumpStep(absl::optional jump_offset, - int64_t expr_id) { - return std::make_unique(jump_offset, expr_id); +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, int64_t expr_id) { - return std::make_unique(jump_offset, expr_id); + 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 index d8555ae10..5cef97329 100644 --- a/eval/eval/jump_step.h +++ b/eval/eval/jump_step.h @@ -28,8 +28,8 @@ namespace google::api::expr::runtime { class JumpStepBase : public ExpressionStepBase { public: - JumpStepBase(absl::optional jump_offset, int64_t expr_id) - : ExpressionStepBase(expr_id, false), jump_offset_(jump_offset) {} + explicit JumpStepBase(absl::optional jump_offset = absl::nullopt) + : ExpressionStepBase(), jump_offset_(jump_offset) {} void set_jump_offset(int offset) { jump_offset_ = offset; } @@ -45,8 +45,8 @@ class JumpStepBase : public ExpressionStepBase { }; // Factory method for Jump step. -std::unique_ptr CreateJumpStep(absl::optional jump_offset, - int64_t expr_id); +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. @@ -54,20 +54,20 @@ std::unique_ptr CreateJumpStep(absl::optional jump_offset, // 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, int64_t expr_id); + 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, int64_t expr_id); + 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, int64_t expr_id); + absl::optional jump_offset = absl::nullopt); } // namespace google::api::expr::runtime diff --git a/eval/eval/lazy_init_step.cc b/eval/eval/lazy_init_step.cc index eb9be7796..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, int64_t expr_id) - : ExpressionStepBase(expr_id), - 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, int64_t expr_id) - : ExpressionStepBase(expr_id), 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: - explicit ClearSlotsStep(size_t slot_index, size_t slot_count, int64_t expr_id) - : ExpressionStepBase(expr_id), - 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,27 +158,4 @@ std::unique_ptr CreateDirectLazyInitStep( expr_id); } -std::unique_ptr CreateLazyInitStep(size_t slot_index, - size_t subexpression_index, - int64_t expr_id) { - return std::make_unique(slot_index, subexpression_index, - expr_id); -} - -std::unique_ptr CreateAssignSlotAndPopStep(size_t slot_index) { - return std::make_unique(slot_index); -} - -std::unique_ptr CreateClearSlotStep(size_t slot_index, - int64_t expr_id) { - return std::make_unique(slot_index, expr_id); -} - -std::unique_ptr CreateClearSlotsStep(size_t slot_index, - size_t slot_count, - int64_t expr_id) { - ABSL_DCHECK_GT(slot_count, 0); - return std::make_unique(slot_index, slot_count, expr_id); -} - } // namespace google::api::expr::runtime diff --git a/eval/eval/lazy_init_step.h b/eval/eval/lazy_init_step.h index 714308dfd..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,24 +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, - int64_t expr_id); +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, - int64_t expr_id); +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, - int64_t expr_id); +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 b9bef90a1..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,92 +58,85 @@ class LazyInitStepTest : public testing::Test { Activation activation_; }; -TEST_F(LazyInitStepTest, CreateCheckInitStepDoesInit) { +TEST_F(LazyInitStepTest, MakeLazyInitDoesInit) { ExecutionPath path; ExecutionPath subpath; - path.push_back(CreateLazyInitStep(/*slot_index=*/0, - /*subexpression_index=*/1, -1)); + path.push_back(ExpressionStep::MakeLazyInitStep( + /*slot_index=*/0, /*subexpression_index=*/1)); - ASSERT_OK_AND_ASSIGN(subpath.emplace_back(), - CreateConstValueStep(cel::IntValue(42), -1, false)); + 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(CreateLazyInitStep(/*slot_index=*/0, -1, -1)); + path.push_back(ExpressionStep::MakeLazyInitStep( + /*slot_index=*/0, /*subexpression_index=*/2)); - ASSERT_OK_AND_ASSIGN(subpath.emplace_back(), - CreateConstValueStep(cel::IntValue(42), -1, false)); + 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(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(CreateClearSlotStep(0, -1)); + 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(CreateClearSlotsStep(0, 2, -1)); + 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 ed0a95275..9a9fa0a24 100644 --- a/eval/eval/logic_step.cc +++ b/eval/eval/logic_step.cc @@ -8,6 +8,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "absl/types/span.h" #include "base/builtins.h" @@ -17,7 +18,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" @@ -186,87 +186,6 @@ 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, int64_t expr_id) - : ExpressionStepBase(expr_id), - 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(CreateNoMatchingOverloadError( - (op_type_ == OpType::kOr) ? cel::builtin::kOr : cel::builtin::kAnd)); - } - } - - 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, @@ -322,47 +241,6 @@ absl::Status DirectNotStep::Evaluate(ExecutionFrameBase& frame, Value& result, return absl::OkStatus(); } -class IterativeNotStep : public ExpressionStepBase { - public: - explicit IterativeNotStep(int64_t expr_id) : ExpressionStepBase(expr_id) {} - - 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(CreateNoMatchingOverloadError(cel::builtin::kNot))); - break; - } - - return absl::OkStatus(); -} class DirectNotStrictlyFalseStep : public DirectExpressionStep { public: @@ -398,41 +276,121 @@ absl::Status DirectNotStrictlyFalseStep::Evaluate( return absl::OkStatus(); } -class IterativeNotStrictlyFalseStep : public ExpressionStepBase { - public: - explicit IterativeNotStrictlyFalseStep(int64_t expr_id) - : ExpressionStepBase(expr_id) {} +} // 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")); + } + 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( + frame.value_stack().PopAndPush( cel::ErrorValue(CreateNoMatchingOverloadError(cel::builtin::kNot))); break; } +} + +void EvaluateNotStrictlyFalseStep(ExecutionFrame& frame) { + if (!frame.value_stack().HasEnough(1)) { + frame.Abort(absl::InternalError("Value stack underflow")); + } + const Value& operand = frame.value_stack().Peek(); - return absl::OkStatus(); + switch (operand.kind()) { + case ValueKind::kBool: + // just forward. + break; + case ValueKind::kUnknown: + case ValueKind::kError: + frame.value_stack().PopAndPush(BoolValue(true)); + break; + // just forward. + break; + default: + frame.value_stack().PopAndPush( + cel::ErrorValue(CreateNoMatchingOverloadError(cel::builtin::kNot))); + 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")); + } + + 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? + absl::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)); +} -// Factory method for "And" Execution step std::unique_ptr CreateDirectAndStep( std::unique_ptr lhs, std::unique_ptr rhs, int64_t expr_id, @@ -441,7 +399,6 @@ std::unique_ptr CreateDirectAndStep( OpType::kAnd, shortcircuiting); } -// Factory method for "Or" Execution step std::unique_ptr CreateDirectOrStep( std::unique_ptr lhs, std::unique_ptr rhs, int64_t expr_id, @@ -450,29 +407,12 @@ std::unique_ptr CreateDirectOrStep( OpType::kOr, shortcircuiting); } -// Factory method for "And" Execution step -absl::StatusOr> CreateAndStep(size_t num_args, - int64_t expr_id) { - return std::make_unique(OpType::kAnd, num_args, expr_id); -} - -// Factory method for "Or" Execution step -absl::StatusOr> CreateOrStep(size_t num_args, - int64_t expr_id) { - return std::make_unique(OpType::kOr, num_args, expr_id); -} - // Factory method for recursive logical not "!" Execution step std::unique_ptr CreateDirectNotStep( std::unique_ptr operand, int64_t expr_id) { return std::make_unique(std::move(operand), expr_id); } -// Factory method for iterative logical not "!" Execution step -std::unique_ptr CreateNotStep(int64_t expr_id) { - return std::make_unique(expr_id); -} - // Factory method for recursive logical "@not_strictly_false" Execution step. std::unique_ptr CreateDirectNotStrictlyFalseStep( std::unique_ptr operand, int64_t expr_id) { @@ -480,9 +420,4 @@ std::unique_ptr CreateDirectNotStrictlyFalseStep( expr_id); } -// Factory method for iterative logical "@not_strictly_false" Execution step. -std::unique_ptr CreateNotStrictlyFalseStep(int64_t expr_id) { - return std::make_unique(expr_id); -} - } // namespace google::api::expr::runtime diff --git a/eval/eval/logic_step.h b/eval/eval/logic_step.h index 4f5be2615..68e8bf74f 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 : uint8_t { + kAnd = 0, + kOr = 1, +}; + +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,28 +35,14 @@ std::unique_ptr CreateDirectOrStep( std::unique_ptr rhs, int64_t expr_id, bool shortcircuiting); -// Factory method for "And" Execution step -absl::StatusOr> CreateAndStep(size_t num_args, - int64_t expr_id); - -// Factory method for "Or" Execution step -absl::StatusOr> CreateOrStep(size_t num_args, - int64_t expr_id); - // 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(int64_t expr_id); - // 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(int64_t expr_id); - } // 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 29725377a..93f2fb888 100644 --- a/eval/eval/logic_step_test.cc +++ b/eval/eval/logic_step_test.cc @@ -67,16 +67,11 @@ class LogicStepTest : public testing::TestWithParam { absl::Status EvaluateLogic(CelValue arg0, CelValue arg1, bool is_or, CelValue* result, bool enable_unknown) { ExecutionPath path; - CEL_ASSIGN_OR_RETURN(auto step, CreateIdentStep("name0", /*expr_id=*/-1)); - path.push_back(std::move(step)); - - CEL_ASSIGN_OR_RETURN(step, CreateIdentStep("name1", /*expr_id=*/-1)); - path.push_back(std::move(step)); - - CEL_ASSIGN_OR_RETURN( - step, (is_or) ? CreateOrStep(/*num_args=*/2, /*expr_id=*/2) - : CreateAndStep(/*num_args=*/2, /*expr_id=*/2)); - path.push_back(std::move(step)); + path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("name0"))); + path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("name1"))); + 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; @@ -647,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 1c52d91b6..99614c3d8 100644 --- a/eval/eval/optional_or_step.cc +++ b/eval/eval/optional_or_step.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include "absl/base/optimization.h" @@ -79,8 +80,8 @@ ErrorValue MakeNoOverloadError(OptionalOrKind kind) { // getting the result of optional.value()) class OptionalHasValueJumpStep final : public JumpStepBase { public: - OptionalHasValueJumpStep(int64_t expr_id, OptionalOrKind kind) - : JumpStepBase({}, expr_id), kind_(kind) {} + explicit OptionalHasValueJumpStep(OptionalOrKind kind) + : JumpStepBase(std::nullopt), kind_(kind) {} absl::Status Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(1)) { @@ -110,8 +111,8 @@ class OptionalHasValueJumpStep final : public JumpStepBase { class OptionalOrStep : public ExpressionStepBase { public: - explicit OptionalOrStep(int64_t expr_id, OptionalOrKind kind) - : ExpressionStepBase(expr_id), kind_(kind) {} + explicit OptionalOrStep(OptionalOrKind kind) + : ExpressionStepBase(), kind_(kind) {} absl::Status Evaluate(ExecutionFrame* frame) const override; @@ -273,17 +274,13 @@ absl::Status DirectOptionalOrStep::Evaluate(ExecutionFrameBase& frame, } // namespace -std::unique_ptr CreateOptionalHasValueJumpStep(bool or_value, - int64_t expr_id) { +std::unique_ptr CreateOptionalHasValueJumpStep(bool or_value) { return std::make_unique( - expr_id, or_value ? OptionalOrKind::kOrValue : OptionalOrKind::kOrOptional); } -std::unique_ptr CreateOptionalOrStep(bool is_or_value, - int64_t expr_id) { +std::unique_ptr CreateOptionalOrStep(bool is_or_value) { return std::make_unique( - expr_id, is_or_value ? OptionalOrKind::kOrValue : OptionalOrKind::kOrOptional); } diff --git a/eval/eval/optional_or_step.h b/eval/eval/optional_or_step.h index 59977c857..3f149e44b 100644 --- a/eval/eval/optional_or_step.h +++ b/eval/eval/optional_or_step.h @@ -31,13 +31,11 @@ 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, - int64_t expr_id); +std::unique_ptr CreateOptionalHasValueJumpStep(bool or_value); // Factory method for OptionalOr step, used to implement optional.or and // optional.orValue. -std::unique_ptr CreateOptionalOrStep(bool is_or_value, - int64_t expr_id); +std::unique_ptr CreateOptionalOrStep(bool is_or_value); // Creates a step implementing the short-circuiting optional.or or // optional.orValue step. diff --git a/eval/eval/regex_match_step.cc b/eval/eval/regex_match_step.cc index 2a06de1b8..abc301225 100644 --- a/eval/eval/regex_match_step.cc +++ b/eval/eval/regex_match_step.cc @@ -59,9 +59,8 @@ struct MatchesVisitor final { class RegexMatchStep final : public ExpressionStepBase { public: - RegexMatchStep(int64_t expr_id, std::shared_ptr re2) - : ExpressionStepBase(expr_id, /*comes_from_ast=*/true), - re2_(std::move(re2)) {} + explicit RegexMatchStep(std::shared_ptr re2) + : ExpressionStepBase(), re2_(std::move(re2)) {} absl::Status Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(kNumRegexMatchArguments)) { @@ -127,9 +126,9 @@ std::unique_ptr CreateDirectRegexMatchStep( std::move(re2)); } -absl::StatusOr> CreateRegexMatchStep( - std::shared_ptr re2, int64_t expr_id) { - return std::make_unique(expr_id, std::move(re2)); +absl::StatusOr> CreateRegexMatchStep( + std::shared_ptr re2) { + return std::make_unique(std::move(re2)); } } // namespace google::api::expr::runtime diff --git a/eval/eval/regex_match_step.h b/eval/eval/regex_match_step.h index 1d8a09118..bcbff7bdc 100644 --- a/eval/eval/regex_match_step.h +++ b/eval/eval/regex_match_step.h @@ -29,9 +29,8 @@ std::unique_ptr CreateDirectRegexMatchStep( int64_t expr_id, std::unique_ptr subject, std::shared_ptr re2); -absl::StatusOr> CreateRegexMatchStep( - std::shared_ptr re2, int64_t expr_id); - +absl::StatusOr> CreateRegexMatchStep( + std::shared_ptr re2); } #endif // THIRD_PARTY_CEL_CPP_EVAL_EVAL_REGEX_MATCH_STEP_H_ diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index ec9e924aa..3d4bf071a 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -206,10 +206,9 @@ absl::Status PerformOptionalGet(const Value& target, absl::string_view field, // message. class SelectStep : public ExpressionStepBase { public: - SelectStep(StringValue value, bool test_field_presence, int64_t expr_id, + SelectStep(StringValue value, bool test_field_presence, bool enable_wrapper_type_null_unboxing, bool enable_optional_types) - : ExpressionStepBase(expr_id), - field_value_(std::move(value)), + : field_value_(std::move(value)), field_(field_value_.ToString()), unboxing_option_(enable_wrapper_type_null_unboxing ? ProtoWrapperTypeOptions::kUnsetNull @@ -460,12 +459,11 @@ bool SupportsCachedFieldDescriptor( class ProtoSelectStep : public SelectStep { public: - ProtoSelectStep(StringValue value, int64_t expr_id, - bool enable_wrapper_type_null_unboxing, + ProtoSelectStep(StringValue value, bool enable_wrapper_type_null_unboxing, bool enable_optional_types, const google::protobuf::Descriptor* descriptor, const google::protobuf::FieldDescriptor* field_descriptor) - : SelectStep(std::move(value), /*test_field_presence=*/false, expr_id, + : SelectStep(std::move(value), /*test_field_presence=*/false, enable_wrapper_type_null_unboxing, enable_optional_types), descriptor_(descriptor), field_descriptor_(field_descriptor) { @@ -540,11 +538,10 @@ absl::Status ProtoSelectStep::EvaluateMessageFieldGet( class ProtoHasStep : public SelectStep { public: - ProtoHasStep(StringValue value, int64_t expr_id, - bool enable_wrapper_type_null_unboxing, + ProtoHasStep(StringValue value, bool enable_wrapper_type_null_unboxing, bool enable_optional_types, const google::protobuf::Descriptor* descriptor, const google::protobuf::FieldDescriptor* field_descriptor) - : SelectStep(std::move(value), /*test_field_presence=*/true, expr_id, + : SelectStep(std::move(value), /*test_field_presence=*/true, enable_wrapper_type_null_unboxing, enable_optional_types), descriptor_(descriptor), field_descriptor_(field_descriptor) { @@ -610,24 +607,24 @@ std::unique_ptr CreateDirectSelectStep( } // Factory method for Select - based Execution step -absl::StatusOr> CreateSelectStep( - cel::StringValue field, bool test_only, int64_t expr_id, +absl::StatusOr> CreateSelectStep( + cel::StringValue field, bool test_only, bool enable_wrapper_type_null_unboxing, bool enable_optional_types) { - return std::make_unique(std::move(field), test_only, expr_id, + return std::make_unique(std::move(field), test_only, enable_wrapper_type_null_unboxing, enable_optional_types); } // Factory method for Select - based Execution step -absl::StatusOr> CreateTypedSelectStep( +absl::StatusOr> CreateTypedSelectStep( cel::StringValue field, cel::StructType resolved_operand_type, - cel::StructTypeField resolved_field, bool test_only, int64_t expr_id, + cel::StructTypeField resolved_field, bool test_only, bool enable_wrapper_type_null_unboxing, bool enable_optional_types) { if (!resolved_operand_type.IsMessage()) { // The specialization only supports messages. Fallback to the generic // implementation for other types. // TODO(uncreated-issue/89): support optional select and chaining. - return CreateSelectStep(std::move(field), test_only, expr_id, + return CreateSelectStep(std::move(field), test_only, enable_wrapper_type_null_unboxing, enable_optional_types); } @@ -647,19 +644,19 @@ absl::StatusOr> CreateTypedSelectStep( // crash. // // Fallback to the generic implementation. - return CreateSelectStep(std::move(field), test_only, expr_id, + return CreateSelectStep(std::move(field), test_only, enable_wrapper_type_null_unboxing, enable_optional_types); } if (test_only) { return std::make_unique( - std::move(field), expr_id, enable_wrapper_type_null_unboxing, + std::move(field), enable_wrapper_type_null_unboxing, enable_optional_types, descriptor, field_descriptor); } return std::make_unique( - std::move(field), expr_id, enable_wrapper_type_null_unboxing, + std::move(field), enable_wrapper_type_null_unboxing, enable_optional_types, descriptor, field_descriptor); } diff --git a/eval/eval/select_step.h b/eval/eval/select_step.h index c3f965a94..d20014027 100644 --- a/eval/eval/select_step.h +++ b/eval/eval/select_step.h @@ -19,13 +19,13 @@ std::unique_ptr CreateDirectSelectStep( bool enable_optional_types = false); // Factory method for Select stack machine based Execution step -absl::StatusOr> CreateSelectStep( - cel::StringValue field, bool test_only, int64_t expr_id, - bool enable_wrapper_type_null_unboxing, bool enable_optional_ytpes = false); +absl::StatusOr> CreateSelectStep( + cel::StringValue field, bool test_only, + bool enable_wrapper_type_null_unboxing, bool enable_optional_types = false); -absl::StatusOr> CreateTypedSelectStep( +absl::StatusOr> CreateTypedSelectStep( cel::StringValue field, cel::StructType resolved_operand_type, - cel::StructTypeField resolved_field, bool test_only, int64_t expr_id, + cel::StructTypeField resolved_field, bool test_only, bool enable_wrapper_type_null_unboxing, bool enable_optional_types); } // namespace google::api::expr::runtime diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index f19370cdc..7930a6675 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -104,14 +104,16 @@ class SelectStepTest : public testing::Test { auto& ident = expr0.mutable_ident_expr(); ident.set_name("target"); - CEL_ASSIGN_OR_RETURN(auto step0, CreateIdentStep(ident.name(), expr0.id())); + auto step0 = CreateIdentStep(ident.name()); CEL_ASSIGN_OR_RETURN( auto step1, CreateSelectStep(cel::StringValue(select.field()), select.test_only(), - expr.id(), options.enable_wrapper_type_null_unboxing)); + options.enable_wrapper_type_null_unboxing)); - path.push_back(std::move(step0)); - path.push_back(std::move(step1)); + path.push_back( + ExpressionStep::MakeGenericStep(std::move(step0), expr0.id())); + path.push_back( + ExpressionStep::MakeGenericStep(std::move(step1), expr.id())); cel::RuntimeOptions runtime_options; if (options.enable_unknowns) { @@ -286,23 +288,22 @@ TEST_F(SelectStepTest, MapPresenseIsErrorTest) { Expr& expr0 = select_map.mutable_operand(); auto& ident = expr0.mutable_ident_expr(); ident.set_name("target"); - - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); + auto step0 = CreateIdentStep(ident.name()); ASSERT_OK_AND_ASSIGN( auto step1, CreateSelectStep(cel::StringValue(select_map.field()), - select_map.test_only(), expr1.id(), + select_map.test_only(), /*enable_wrapper_type_null_unboxing=*/false)); ASSERT_OK_AND_ASSIGN( auto step2, CreateSelectStep(cel::StringValue(select.field()), select.test_only(), - select_expr.id(), /*enable_wrapper_type_null_unboxing=*/false)); ExecutionPath path; - 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(step0), expr0.id())); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step1), expr1.id())); + path.push_back( + ExpressionStep::MakeGenericStep(std::move(step2), select_expr.id())); CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env_->type_registry.GetComposedTypeProvider(), @@ -749,15 +750,15 @@ TEST_P(SelectStepConformanceTest, CelErrorAsArgument) { auto& ident = expr0.mutable_ident_expr(); ident.set_name("message"); - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); + auto step0 = CreateIdentStep(ident.name()); ASSERT_OK_AND_ASSIGN( auto step1, CreateSelectStep(cel::StringValue(select.field()), select.test_only(), - dummy_expr.id(), /*enable_wrapper_type_null_unboxing=*/false)); - path.push_back(std::move(step0)); - path.push_back(std::move(step1)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step0), expr0.id())); + path.push_back( + ExpressionStep::MakeGenericStep(std::move(step1), dummy_expr.id())); CelError error = absl::CancelledError(); @@ -791,15 +792,15 @@ TEST_F(SelectStepTest, DisableMissingAttributeOK) { auto& ident = expr0.mutable_ident_expr(); ident.set_name("message"); - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); + auto step0 = CreateIdentStep(ident.name()); ASSERT_OK_AND_ASSIGN( auto step1, CreateSelectStep(cel::StringValue(select.field()), select.test_only(), - dummy_expr.id(), /*enable_wrapper_type_null_unboxing=*/false)); - path.push_back(std::move(step0)); - path.push_back(std::move(step1)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step0), expr0.id())); + path.push_back( + ExpressionStep::MakeGenericStep(std::move(step1), dummy_expr.id())); CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, @@ -834,15 +835,15 @@ TEST_F(SelectStepTest, UnrecoverableUnknownValueProducesError) { auto& ident = expr0.mutable_ident_expr(); ident.set_name("message"); - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); + auto step0 = CreateIdentStep(ident.name()); ASSERT_OK_AND_ASSIGN( auto step1, CreateSelectStep(cel::StringValue(select.field()), select.test_only(), - dummy_expr.id(), /*enable_wrapper_type_null_unboxing=*/false)); - path.push_back(std::move(step0)); - path.push_back(std::move(step1)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step0), expr0.id())); + path.push_back( + ExpressionStep::MakeGenericStep(std::move(step1), dummy_expr.id())); cel::RuntimeOptions options; options.enable_missing_attribute_errors = true; @@ -883,16 +884,16 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { auto& ident = expr0.mutable_ident_expr(); ident.set_name("message"); - auto step0_status = CreateIdentStep(ident.name(), expr0.id()); - auto step1_status = CreateSelectStep( - cel::StringValue(select.field()), select.test_only(), dummy_expr.id(), - /*enable_wrapper_type_null_unboxing=*/false); + auto step0 = CreateIdentStep(ident.name()); + auto step1_status = + CreateSelectStep(cel::StringValue(select.field()), select.test_only(), + /*enable_wrapper_type_null_unboxing=*/false); - ASSERT_THAT(step0_status, IsOk()); ASSERT_THAT(step1_status, IsOk()); - path.push_back(*std::move(step0_status)); - path.push_back(*std::move(step1_status)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step0), expr0.id())); + path.push_back(ExpressionStep::MakeGenericStep(std::move(*step1_status), + dummy_expr.id())); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; @@ -977,7 +978,7 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { } TEST_P(SelectStepConformanceTest, TypedSelectStepTest) { - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("message", -1)); + auto step0 = CreateIdentStep("message"); cel::StructType resolved_operand_type( (cel::MessageType(TestAllTypes::descriptor()))); @@ -990,13 +991,13 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepTest) { auto step1, CreateTypedSelectStep(cel::StringValue("single_int64"), resolved_operand_type, resolved_field, - /*test_only=*/false, -1, + /*test_only=*/false, /*enable_wrapper_type_null_unboxing=*/false, /*enable_optional_types=*/false)); ExecutionPath path; - path.push_back(std::move(step0)); - path.push_back(std::move(step1)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step0))); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); cel::RuntimeOptions options; if (GetParam()) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; @@ -1018,7 +1019,7 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepTest) { } TEST_P(SelectStepConformanceTest, TypedSelectStepPropagatesUnknown) { - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("message", -1)); + auto step0 = CreateIdentStep("message"); cel::StructType resolved_operand_type( (cel::MessageType(TestAllTypes::descriptor()))); @@ -1031,13 +1032,13 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepPropagatesUnknown) { auto step1, CreateTypedSelectStep(cel::StringValue("single_int64"), resolved_operand_type, resolved_field, - /*test_only=*/false, -1, + /*test_only=*/false, /*enable_wrapper_type_null_unboxing=*/false, /*enable_optional_types=*/false)); ExecutionPath path; - path.push_back(std::move(step0)); - path.push_back(std::move(step1)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step0))); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); cel::RuntimeOptions options; if (GetParam()) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; @@ -1056,7 +1057,7 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepPropagatesUnknown) { } TEST_F(SelectStepTest, TypedSelectStepUnknownPatternResolvesToUnknown) { - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("message", -1)); + auto step0 = CreateIdentStep("message"); cel::StructType resolved_operand_type( (cel::MessageType(TestAllTypes::descriptor()))); @@ -1069,13 +1070,13 @@ TEST_F(SelectStepTest, TypedSelectStepUnknownPatternResolvesToUnknown) { auto step1, CreateTypedSelectStep(cel::StringValue("single_int64"), resolved_operand_type, resolved_field, - /*test_only=*/false, -1, + /*test_only=*/false, /*enable_wrapper_type_null_unboxing=*/false, /*enable_optional_types=*/false)); ExecutionPath path; - path.push_back(std::move(step0)); - path.push_back(std::move(step1)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step0))); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; CelExpressionFlatImpl cel_expr( diff --git a/eval/eval/shadowable_value_step.cc b/eval/eval/shadowable_value_step.cc index 0839d28cc..0b75f7cc2 100644 --- a/eval/eval/shadowable_value_step.cc +++ b/eval/eval/shadowable_value_step.cc @@ -24,8 +24,8 @@ using ::cel::Value; class ShadowableValueStep : public ExpressionStepBase { public: - ShadowableValueStep(std::string identifier, cel::Value value, int64_t expr_id) - : ExpressionStepBase(expr_id), + ShadowableValueStep(std::string identifier, cel::Value value) + : ExpressionStepBase(), identifier_(std::move(identifier)), value_(std::move(value)) {} @@ -83,10 +83,10 @@ absl::Status DirectShadowableValueStep::Evaluate( } // namespace -absl::StatusOr> CreateShadowableValueStep( - absl::string_view name, cel::Value value, int64_t expr_id) { +absl::StatusOr> CreateShadowableValueStep( + absl::string_view name, cel::Value value) { return std::make_unique(std::string(name), - std::move(value), expr_id); + std::move(value)); } std::unique_ptr CreateDirectShadowableValueStep( diff --git a/eval/eval/shadowable_value_step.h b/eval/eval/shadowable_value_step.h index 9c386f02d..fae90353b 100644 --- a/eval/eval/shadowable_value_step.h +++ b/eval/eval/shadowable_value_step.h @@ -15,8 +15,8 @@ namespace google::api::expr::runtime { // Create an identifier resolution step with a default value that may be // shadowed by an identifier of the same name within the runtime-provided // Activation. -absl::StatusOr> CreateShadowableValueStep( - absl::string_view name, cel::Value value, int64_t expr_id); +absl::StatusOr> CreateShadowableValueStep( + absl::string_view name, cel::Value value); std::unique_ptr CreateDirectShadowableValueStep( absl::string_view name, cel::Value value, int64_t expr_id); diff --git a/eval/eval/shadowable_value_step_test.cc b/eval/eval/shadowable_value_step_test.cc index 4a7cabea1..83ffd5857 100644 --- a/eval/eval/shadowable_value_step_test.cc +++ b/eval/eval/shadowable_value_step_test.cc @@ -34,11 +34,10 @@ absl::StatusOr RunShadowableExpression( const absl_nonnull std::shared_ptr& env, std::string identifier, cel::Value value, const Activation& activation, Arena* arena) { - CEL_ASSIGN_OR_RETURN( - auto step, - CreateShadowableValueStep(std::move(identifier), std::move(value), 1)); + CEL_ASSIGN_OR_RETURN(auto step, + CreateShadowableValueStep(identifier, std::move(value))); ExecutionPath path; - path.push_back(std::move(step)); + path.push_back(ExpressionStep::MakeGenericStep(std::move(step), 1)); CelExpressionFlatImpl impl( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, @@ -55,11 +54,10 @@ TEST(ShadowableValueStepTest, TestEvaluateNoShadowing) { Arena arena; auto type_value = CreateTypeValueFromView(&arena, type_name); - auto status = - RunShadowableExpression(env, type_name, type_value, activation, &arena); - ASSERT_OK(status); + ASSERT_OK_AND_ASSIGN( + auto value, + RunShadowableExpression(env, type_name, type_value, activation, &arena)); - auto value = status.value(); ASSERT_TRUE(value.IsCelType()); EXPECT_THAT(value.CelTypeOrDie().value(), Eq(type_name)); } @@ -74,11 +72,10 @@ TEST(ShadowableValueStepTest, TestEvaluateShadowedIdentifier) { Arena arena; auto type_value = CreateTypeValueFromView(&arena, type_name); - auto status = - RunShadowableExpression(env, type_name, type_value, activation, &arena); - ASSERT_OK(status); + ASSERT_OK_AND_ASSIGN( + auto value, + RunShadowableExpression(env, type_name, type_value, activation, &arena)); - auto value = status.value(); ASSERT_TRUE(value.IsInt64()); EXPECT_THAT(value.Int64OrDie(), Eq(1024L)); } diff --git a/eval/eval/ternary_step.cc b/eval/eval/ternary_step.cc index a12d6863e..6f289c7a5 100644 --- a/eval/eval/ternary_step.cc +++ b/eval/eval/ternary_step.cc @@ -124,7 +124,7 @@ class ShortcircuitingDirectTernaryStep : public DirectExpressionStep { class TernaryStep : public ExpressionStepBase { public: // Constructs FunctionStep that uses overloads specified. - explicit TernaryStep(int64_t expr_id) : ExpressionStepBase(expr_id) {} + TernaryStep() : ExpressionStepBase() {} absl::Status Evaluate(ExecutionFrame* frame) const override; }; @@ -186,9 +186,8 @@ std::unique_ptr CreateDirectTernaryStep( std::move(condition), std::move(left), std::move(right), expr_id); } -absl::StatusOr> CreateTernaryStep( - int64_t expr_id) { - return std::make_unique(expr_id); +std::unique_ptr CreateTernaryStep() { + return std::make_unique(); } } // namespace google::api::expr::runtime diff --git a/eval/eval/ternary_step.h b/eval/eval/ternary_step.h index 2b51e95ea..82491f589 100644 --- a/eval/eval/ternary_step.h +++ b/eval/eval/ternary_step.h @@ -18,8 +18,7 @@ std::unique_ptr CreateDirectTernaryStep( bool shortcircuiting = true); // Factory method for ternary (_?_:_) execution step -absl::StatusOr> CreateTernaryStep( - int64_t expr_id); +std::unique_ptr CreateTernaryStep(); } // namespace google::api::expr::runtime diff --git a/eval/eval/ternary_step_test.cc b/eval/eval/ternary_step_test.cc index 2c400b3e1..9b17b5356 100644 --- a/eval/eval/ternary_step_test.cc +++ b/eval/eval/ternary_step_test.cc @@ -38,6 +38,7 @@ namespace google::api::expr::runtime { namespace { +using ::absl_testing::IsOk; using ::absl_testing::StatusIs; using ::cel::BoolValue; using ::cel::Cast; @@ -64,17 +65,10 @@ class LogicStepTest : public testing::TestWithParam { CelValue* result, bool enable_unknown) { ExecutionPath path; - CEL_ASSIGN_OR_RETURN(auto step, CreateIdentStep("name0", /*expr_id=*/-1)); - path.push_back(std::move(step)); - - CEL_ASSIGN_OR_RETURN(step, CreateIdentStep("name1", /*expr_id=*/-1)); - path.push_back(std::move(step)); - - CEL_ASSIGN_OR_RETURN(step, CreateIdentStep("name2", /*expr_id=*/-1)); - path.push_back(std::move(step)); - - CEL_ASSIGN_OR_RETURN(step, CreateTernaryStep(4)); - path.push_back(std::move(step)); + path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("name0"))); + path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("name1"))); + path.push_back(ExpressionStep::MakeGenericStep(CreateIdentStep("name2"))); + path.push_back(ExpressionStep::MakeGenericStep(CreateTernaryStep(), 4)); cel::RuntimeOptions options; if (enable_unknown) { @@ -109,14 +103,14 @@ TEST_P(LogicStepTest, TestBoolCond) { absl::Status status = EvaluateLogic(CelValue::CreateBool(true), CelValue::CreateBool(true), CelValue::CreateBool(false), &result, GetParam()); - ASSERT_OK(status); + ASSERT_THAT(status, IsOk()); ASSERT_TRUE(result.IsBool()); ASSERT_TRUE(result.BoolOrDie()); status = EvaluateLogic(CelValue::CreateBool(false), CelValue::CreateBool(true), CelValue::CreateBool(false), &result, GetParam()); - ASSERT_OK(status); + ASSERT_THAT(status, IsOk()); ASSERT_TRUE(result.IsBool()); ASSERT_FALSE(result.BoolOrDie()); } @@ -125,16 +119,19 @@ TEST_P(LogicStepTest, TestErrorHandling) { CelValue result; CelError error = absl::CancelledError(); CelValue error_value = CelValue::CreateError(&error); - ASSERT_OK(EvaluateLogic(error_value, CelValue::CreateBool(true), - CelValue::CreateBool(false), &result, GetParam())); + ASSERT_THAT(EvaluateLogic(error_value, CelValue::CreateBool(true), + CelValue::CreateBool(false), &result, GetParam()), + IsOk()); ASSERT_TRUE(result.IsError()); - ASSERT_OK(EvaluateLogic(CelValue::CreateBool(true), error_value, - CelValue::CreateBool(false), &result, GetParam())); + ASSERT_THAT(EvaluateLogic(CelValue::CreateBool(true), error_value, + CelValue::CreateBool(false), &result, GetParam()), + IsOk()); ASSERT_TRUE(result.IsError()); - ASSERT_OK(EvaluateLogic(CelValue::CreateBool(false), error_value, - CelValue::CreateBool(false), &result, GetParam())); + ASSERT_THAT(EvaluateLogic(CelValue::CreateBool(false), error_value, + CelValue::CreateBool(false), &result, GetParam()), + IsOk()); ASSERT_TRUE(result.IsBool()); ASSERT_FALSE(result.BoolOrDie()); } @@ -145,25 +142,30 @@ TEST_F(LogicStepTest, TestUnknownHandling) { CelError cel_error = absl::CancelledError(); CelValue unknown_value = CelValue::CreateUnknownSet(&unknown_set); CelValue error_value = CelValue::CreateError(&cel_error); - ASSERT_OK(EvaluateLogic(unknown_value, CelValue::CreateBool(true), - CelValue::CreateBool(false), &result, true)); + ASSERT_THAT(EvaluateLogic(unknown_value, CelValue::CreateBool(true), + CelValue::CreateBool(false), &result, true), + IsOk()); ASSERT_TRUE(result.IsUnknownSet()); - ASSERT_OK(EvaluateLogic(CelValue::CreateBool(true), unknown_value, - CelValue::CreateBool(false), &result, true)); + ASSERT_THAT(EvaluateLogic(CelValue::CreateBool(true), unknown_value, + CelValue::CreateBool(false), &result, true), + IsOk()); ASSERT_TRUE(result.IsUnknownSet()); - ASSERT_OK(EvaluateLogic(CelValue::CreateBool(false), unknown_value, - CelValue::CreateBool(false), &result, true)); + ASSERT_THAT(EvaluateLogic(CelValue::CreateBool(false), unknown_value, + CelValue::CreateBool(false), &result, true), + IsOk()); ASSERT_TRUE(result.IsBool()); ASSERT_FALSE(result.BoolOrDie()); - ASSERT_OK(EvaluateLogic(error_value, unknown_value, - CelValue::CreateBool(false), &result, true)); + ASSERT_THAT(EvaluateLogic(error_value, unknown_value, + CelValue::CreateBool(false), &result, true), + IsOk()); ASSERT_TRUE(result.IsError()); - ASSERT_OK(EvaluateLogic(unknown_value, error_value, - CelValue::CreateBool(false), &result, true)); + ASSERT_THAT(EvaluateLogic(unknown_value, error_value, + CelValue::CreateBool(false), &result, true), + IsOk()); ASSERT_TRUE(result.IsUnknownSet()); Expr expr0; @@ -184,9 +186,10 @@ TEST_F(LogicStepTest, TestUnknownHandling) { EXPECT_THAT(unknown_attr_set0.size(), Eq(1)); EXPECT_THAT(unknown_attr_set1.size(), Eq(1)); - ASSERT_OK(EvaluateLogic(CelValue::CreateUnknownSet(&unknown_set0), - CelValue::CreateUnknownSet(&unknown_set1), - CelValue::CreateBool(false), &result, true)); + ASSERT_THAT(EvaluateLogic(CelValue::CreateUnknownSet(&unknown_set0), + CelValue::CreateUnknownSet(&unknown_set1), + CelValue::CreateBool(false), &result, true), + IsOk()); ASSERT_TRUE(result.IsUnknownSet()); const auto& attrs = result.UnknownSetOrDie()->unknown_attributes(); ASSERT_THAT(attrs, testing::SizeIs(1)); @@ -222,7 +225,7 @@ TEST_P(TernaryStepDirectTest, ReturnLhs) { cel::Value result; AttributeTrail attr_unused; - ASSERT_OK(step->Evaluate(frame, result, attr_unused)); + ASSERT_THAT(step->Evaluate(frame, result, attr_unused), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_EQ(Cast(result).NativeValue(), 1); @@ -243,7 +246,7 @@ TEST_P(TernaryStepDirectTest, ReturnRhs) { cel::Value result; AttributeTrail attr_unused; - ASSERT_OK(step->Evaluate(frame, result, attr_unused)); + ASSERT_THAT(step->Evaluate(frame, result, attr_unused), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_EQ(Cast(result).NativeValue(), 2); @@ -266,7 +269,7 @@ TEST_P(TernaryStepDirectTest, ForwardError) { cel::Value result; AttributeTrail attr_unused; - ASSERT_OK(step->Evaluate(frame, result, attr_unused)); + ASSERT_THAT(step->Evaluate(frame, result, attr_unused), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).NativeValue(), @@ -294,7 +297,7 @@ TEST_P(TernaryStepDirectTest, ForwardUnknown) { cel::Value result; AttributeTrail attr_unused; - ASSERT_OK(step->Evaluate(frame, result, attr_unused)); + ASSERT_THAT(step->Evaluate(frame, result, attr_unused), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).ToAttributeSet(), ElementsAre(Truly([](const cel::Attribute& attr) { @@ -317,7 +320,7 @@ TEST_P(TernaryStepDirectTest, UnexpectedCondtionKind) { cel::Value result; AttributeTrail attr_unused; - ASSERT_OK(step->Evaluate(frame, result, attr_unused)); + ASSERT_THAT(step->Evaluate(frame, result, attr_unused), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).NativeValue(), @@ -358,7 +361,7 @@ TEST_P(TernaryStepDirectTest, Shortcircuiting) { cel::Value result; AttributeTrail attr_unused; - ASSERT_OK(step->Evaluate(frame, result, attr_unused)); + ASSERT_THAT(step->Evaluate(frame, result, attr_unused), IsOk()); ASSERT_TRUE(InstanceOf(result)); EXPECT_THAT(Cast(result).NativeValue(), Eq(1)); diff --git a/eval/public/cel_expression.h b/eval/public/cel_expression.h index 4cf029e89..af28e2ae6 100644 --- a/eval/public/cel_expression.h +++ b/eval/public/cel_expression.h @@ -24,6 +24,13 @@ namespace google::api::expr::runtime { // then the order of the callback invocations is guaranteed to correspond // the order of variable sub-elements (e.g. the order of elements returned // by Comprehension.iter_range). +// +// Expression IDs outside of the range [0, INT32_MAX] are not supported and +// will not invoke the listener. While the AST allows any int64, supported +// parser implementations should use a dense range starting at 1. In practice, +// no AST should contain more than ~ 1e9 nodes. +// +// ID 0 should not be considered valid, but is supported for legacy reasons. using CelEvaluationListener = std::function; diff --git a/extensions/select_optimization.cc b/extensions/select_optimization.cc index 4dcd7d594..5bc7f8748 100644 --- a/extensions/select_optimization.cc +++ b/extensions/select_optimization.cc @@ -84,6 +84,7 @@ using ::google::api::expr::runtime::CelValue; using ::google::api::expr::runtime::DirectExpressionStep; using ::google::api::expr::runtime::ExecutionFrame; using ::google::api::expr::runtime::ExecutionFrameBase; +using ::google::api::expr::runtime::ExpressionStep; using ::google::api::expr::runtime::ExpressionStepBase; using ::google::api::expr::runtime::GetGenericProtoTypeInfoInstance; using ::google::api::expr::runtime::PlannerContext; @@ -969,8 +970,9 @@ absl::Status SelectOptimizer::OnPostVisit(PlannerContext& context, CEL_ASSIGN_OR_RETURN(auto operand_subplan, context.ExtractSubplan(operand)); absl::c_move(operand_subplan, std::back_inserter(path)); - path.push_back( - std::make_unique(node.id(), std::move(impl))); + path.push_back(ExpressionStep::MakeGenericStep( + std::make_unique(node.id(), std::move(impl)), + node.id())); return context.ReplaceSubplan(node, std::move(path)); } diff --git a/runtime/internal/runtime_impl.cc b/runtime/internal/runtime_impl.cc index 92d097b2c..3e8eef59a 100644 --- a/runtime/internal/runtime_impl.cc +++ b/runtime/internal/runtime_impl.cc @@ -139,11 +139,15 @@ RuntimeImpl::CreateTraceableProgram( !flat_expr.subexpressions().empty() && // mainline expression is exactly one recursive step. flat_expr.subexpressions().front().size() == 1 && - flat_expr.subexpressions().front().front()->GetNativeTypeId() == - NativeTypeId::For()) { + flat_expr.subexpressions().front().front().IsGenericStep() && + flat_expr.subexpressions() + .front() + .front() + .GetGenericStep() + ->GetNativeTypeId() == NativeTypeId::For()) { const DirectExpressionStep* root = internal::down_cast( - flat_expr.subexpressions().front().front().get()) + flat_expr.subexpressions().front().front().GetGenericStep()) ->wrapped(); return std::make_unique(environment_, std::move(flat_expr), root); diff --git a/runtime/runtime.h b/runtime/runtime.h index 8c76236dd..02ad9fd06 100644 --- a/runtime/runtime.h +++ b/runtime/runtime.h @@ -130,6 +130,13 @@ class TraceableProgram : public Program { // to an AST expression node. The value provided is the top of the value // stack, corresponding to the result of evaluating the given sub expression. // + // Expression IDs outside of the range [0, INT32_MAX] are not supported and + // will not invoke the listener. While the AST allows any int64, supported + // parser implementations should use a dense range starting at 1. In practice, + // no AST should contain more than ~ 1e9 nodes. + // + // ID 0 should not be considered valid, but is supported for legacy reasons. + // // A returning a non-ok status stops evaluation and forwards the error. using EvaluationListener = absl::AnyInvocable