diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index a8fe79b56..e929ee3f4 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -149,8 +149,11 @@ cc_test( "//common:function_descriptor", "//common:kind", "//common:value", + "//eval/eval:cel_expression_flat_impl", + "//eval/eval:evaluator_core", "//eval/public:activation", "//eval/public:ast_rewrite", + "//eval/public:base_activation", "//eval/public:builtin_func_registrar", "//eval/public:cel_attribute", "//eval/public:cel_expr_builder_factory", diff --git a/eval/compiler/constant_folding.cc b/eval/compiler/constant_folding.cc index 4a8d55c31..8e854a867 100644 --- a/eval/compiler/constant_folding.cc +++ b/eval/compiler/constant_folding.cc @@ -201,18 +201,18 @@ absl::Status ConstantFoldingExtension::OnPostVisit(PlannerContext& context, // This subexpression is already optimized out or suppressed. return absl::OkStatus(); } - // copy string to managed handle if backed by the original program. + // Ensure that string constants are copied to the constant folding arena Value value; if (node.has_const_expr()) { CEL_ASSIGN_OR_RETURN(value, ConvertConstant(node.const_expr(), state_.arena())); } else { - ExecutionFrame frame(subplan, empty_, context.options(), state_); state_.Reset(); // Update stack size to accommodate sub expression. // This only results in a vector resize if the new maxsize is greater than // the current capacity. state_.value_stack().SetMaxSize(subplan.size()); + ExecutionFrame frame(subplan, empty_, context.options(), state_); auto result = frame.Evaluate(); // If this would be a runtime error, then don't adjust the program plan, but diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index 8228764a4..c76355d07 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -70,6 +70,7 @@ #include "eval/eval/evaluator_core.h" #include "eval/eval/expression_step_logic.h" #include "eval/eval/function_step.h" +#include "eval/eval/ident_step.h" #include "eval/eval/optional_or_step.h" #include "eval/eval/select_step.h" #include "eval/eval/shadowable_value_step.h" @@ -892,8 +893,12 @@ class FlatExprVisitor : public cel::AstVisitor { return; } + if (PlanningSuppressed()) { + return; + } absl::string_view ident_name = absl::StripPrefix(ident_expr.name(), "."); - AddStep(ExpressionStep::MakeIdentifierStep(ident_name, expr.id())); + size_t ident_index = AddOrUpdateIdentifier(ident_name); + AddStep(ExpressionStep::MakeIdentifierStep(ident_index, expr.id())); } void PreVisitSelect(const cel::Expr& expr, @@ -1547,6 +1552,15 @@ class FlatExprVisitor : public cel::AstVisitor { return index_manager_.IterStackRequirement(); } + IdentifierTable BuildIdentifierTable(size_t& total_slot_count) && { + IdentifierTable table; + table.reserve(identifiers_.size()); + for (std::string& name : identifiers_) { + table.push_back(Identifier{std::move(name), total_slot_count++}); + } + return table; + } + void AddOptimizer(std::unique_ptr optimizer) { program_optimizers_.push_back(std::move(optimizer)); } @@ -1698,6 +1712,8 @@ class FlatExprVisitor : public cel::AstVisitor { void MaybeResolveType(const cel::Expr& expr); + size_t AddOrUpdateIdentifier(absl::string_view name); + const Resolver& resolver_; const cel::TypeProvider& type_provider_; absl::Status progress_status_; @@ -1726,6 +1742,8 @@ class FlatExprVisitor : public cel::AstVisitor { ProgramBuilder& program_builder_; PlannerContext& extension_context_; IndexManager index_manager_; + std::vector identifiers_; + absl::flat_hash_map identifier_map_; bool enable_optional_types_; std::optional block_; @@ -1872,6 +1890,16 @@ void FlatExprVisitor::MaybeResolveType(const cel::Expr& expr) { SetResolvedType(expr, *type); } +size_t FlatExprVisitor::AddOrUpdateIdentifier(absl::string_view name) { + auto it = identifier_map_.find(name); + if (it == identifier_map_.end()) { + size_t index = identifiers_.size(); + it = identifier_map_.emplace(name, index).first; + identifiers_.push_back(std::string(name)); + } + return it->second; +} + void LogicalCondVisitor::PreVisit(const cel::Expr* expr) { visitor_->ValidateOrError( !expr->call_expr().has_target() && expr->call_expr().args().size() >= 2, @@ -2329,7 +2357,12 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( return visitor.progress_status(); } - if (visitor.slot_count() > std::numeric_limits::max() || + size_t total_slot_count = visitor.slot_count(); + size_t iterator_stack_size = visitor.iter_stack_count(); + IdentifierTable identifiers = + std::move(visitor).BuildIdentifierTable(total_slot_count); + + if (total_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 @@ -2345,10 +2378,21 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( ExecutionPath execution_path; std::vector subexpressions = FlattenExpressionTable(program_builder, execution_path); + // Iterate over execution path and update identifier steps with resolved slot. + // TODO(b/572083203): Should invert this so the local slots are stored + // after the global ones. + for (ExpressionStep& step : execution_path) { + if (IdentStepInfo* ident_step = GetIfIdentStep(step); + ident_step != nullptr) { + ident_step->slot_index = static_cast( + identifiers[ident_step->ident_index].slot_index); + } + } return FlatExpression(std::move(execution_path), std::move(subexpressions), - visitor.slot_count(), visitor.iter_stack_count(), - GetTypeProvider(), options_, std::move(arena)); + total_slot_count, iterator_stack_size, + GetTypeProvider(), options_, std::move(arena), + std::move(identifiers)); } const cel::TypeProvider& FlatExprBuilder::GetTypeProvider() const { return use_legacy_type_provider_ diff --git a/eval/compiler/flat_expr_builder_test.cc b/eval/compiler/flat_expr_builder_test.cc index 5ce27a603..db9d6d30c 100644 --- a/eval/compiler/flat_expr_builder_test.cc +++ b/eval/compiler/flat_expr_builder_test.cc @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -41,8 +42,12 @@ #include "eval/compiler/cel_expression_builder_flat_impl.h" #include "eval/compiler/constant_folding.h" #include "eval/compiler/qualified_reference_resolver.h" +#include "eval/eval/cel_expression_flat_impl.h" +#include "eval/eval/evaluator_core.h" +#include "eval/eval/ident_step.h" #include "eval/public/activation.h" #include "eval/public/ast_rewrite.h" +#include "eval/public/base_activation.h" #include "eval/public/builtin_func_registrar.h" #include "eval/public/cel_attribute.h" #include "eval/public/cel_expr_builder_factory.h" @@ -3148,6 +3153,107 @@ TEST(FlatExprBuilderTest, ListAppendComprehensionWithTargetFailsPlanning) { StatusIs(absl::StatusCode::kInvalidArgument)); } +class CountingActivation : public BaseActivation { + public: + void InsertInt(absl::string_view name, int64_t val) { + values_[name] = CelValue::CreateInt64(val); + } + + void InsertBool(absl::string_view name, bool val) { + values_[name] = CelValue::CreateBool(val); + } + + std::vector FindFunctionOverloads( + absl::string_view) const override { + return {}; + } + + std::optional FindValue(absl::string_view name, + google::protobuf::Arena*) const override { + lookup_counts_[name]++; + auto it = values_.find(name); + if (it != values_.end()) { + return it->second; + } + return std::nullopt; + } + + int LookupCount(absl::string_view name) const { + auto it = lookup_counts_.find(name); + return it != lookup_counts_.end() ? it->second : 0; + } + + private: + absl::flat_hash_map values_; + mutable absl::flat_hash_map lookup_counts_; +}; + +TEST(FlatExprBuilderTest, IdentifierTableRepeatedRootIdentifierReceivesSlot) { + ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse("x + y + x")); + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv()); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry()), IsOk()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr cel_expr, + builder.CreateExpression(&parsed_expr.expr(), + &parsed_expr.source_info())); + + const FlatExpression& flat_expr = + static_cast(cel_expr.get()) + ->flat_expression(); + ASSERT_THAT(flat_expr.identifiers(), SizeIs(2)); + EXPECT_EQ(flat_expr.identifiers()[0].name, "x"); + EXPECT_EQ(flat_expr.identifiers()[0].slot_index, 0); + EXPECT_EQ(flat_expr.identifiers()[1].name, "y"); + EXPECT_EQ(flat_expr.identifiers()[1].slot_index, 1); + EXPECT_EQ(flat_expr.comprehension_slots_size(), 2); + EXPECT_EQ(flat_expr.iterator_stack_size(), 0); + + CountingActivation activation; + activation.InsertInt("x", 10); + activation.InsertInt("y", 20); + google::protobuf::Arena arena; + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); + EXPECT_THAT(result, test::IsCelInt64(40)); + EXPECT_EQ(activation.LookupCount("x"), 1); + EXPECT_EQ(activation.LookupCount("y"), 1); +} + +TEST(FlatExprBuilderTest, + IdentifierTableInComprehensionReceivesSlotAboveComprehensionSlots) { + ASSERT_OK_AND_ASSIGN( + ParsedExpr parsed_expr, + parser::Parse("[1, 2, 3].all(i, i < limit) && single_var")); + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv()); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry()), IsOk()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr cel_expr, + builder.CreateExpression(&parsed_expr.expr(), + &parsed_expr.source_info())); + + const FlatExpression& flat_expr = + static_cast(cel_expr.get()) + ->flat_expression(); + // Local comprehension variables (i, @result) are not in the root identifier + // table; only "limit" and "single_var" are present. + ASSERT_THAT(flat_expr.identifiers(), SizeIs(2)); + EXPECT_EQ(flat_expr.identifiers()[0].name, "limit"); + // Comprehension uses 2 slots (accu_var at 0, iter_var at 1), so "limit" gets + // slot 2 and "single_var" gets slot 3. + EXPECT_EQ(flat_expr.identifiers()[0].slot_index, 2); + EXPECT_EQ(flat_expr.identifiers()[1].name, "single_var"); + EXPECT_EQ(flat_expr.identifiers()[1].slot_index, 3); + EXPECT_EQ(flat_expr.comprehension_slots_size(), 4); + EXPECT_EQ(flat_expr.iterator_stack_size(), 1); + + CountingActivation activation; + activation.InsertInt("limit", 5); + activation.InsertBool("single_var", true); + google::protobuf::Arena arena; + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); + EXPECT_THAT(result, test::IsCelBool(true)); + EXPECT_EQ(activation.LookupCount("limit"), 1); + EXPECT_EQ(activation.LookupCount("single_var"), 1); + EXPECT_EQ(activation.LookupCount("i"), 0); +} + } // namespace } // namespace google::api::expr::runtime diff --git a/eval/eval/BUILD b/eval/eval/BUILD index b477b35f7..98d3cfc07 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -85,6 +85,7 @@ cc_library( "//runtime/standard:equality_functions", "@com_google_absl//absl/base", "@com_google_absl//absl/base:core_headers", + "@com_google_absl//absl/base:no_destructor", "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/log:absl_check", @@ -448,6 +449,7 @@ cc_test( ":cel_expression_flat_impl", ":container_access_step", ":evaluator_core", + ":expression_step_logic", "//base:builtins", "//base:data", "//common:ast", @@ -471,6 +473,7 @@ cc_test( "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", "@com_google_protobuf//:protobuf", "@com_google_protobuf//:struct_cc_proto", @@ -510,7 +513,9 @@ cc_test( "//base:data", "//common:memory", "//eval/public:activation", + "//eval/public:base_activation", "//eval/public:cel_attribute", + "//eval/public:cel_function", "//eval/public:cel_value", "//internal:testing", "//runtime:runtime_options", diff --git a/eval/eval/comprehension_slots.h b/eval/eval/comprehension_slots.h index 5a7ce21cc..fe0fbd615 100644 --- a/eval/eval/comprehension_slots.h +++ b/eval/eval/comprehension_slots.h @@ -150,7 +150,7 @@ class ComprehensionSlots final { size_t size() const { return slots_.size(); } private: - absl::FixedArray slots_; + absl::FixedArray slots_; }; } // namespace google::api::expr::runtime diff --git a/eval/eval/comprehension_step_test.cc b/eval/eval/comprehension_step_test.cc index 5964421ed..ee4882816 100644 --- a/eval/eval/comprehension_step_test.cc +++ b/eval/eval/comprehension_step_test.cc @@ -50,8 +50,9 @@ class ListKeysStepTest : public testing::Test { auto env = NewTestingRuntimeEnv(); return std::make_unique( env, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + FlatExpression(std::move(path), /*comprehension_slot_count=*/1, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"var", 0}})); } private: @@ -75,7 +76,7 @@ MATCHER_P(CelStringValue, val, "") { TEST_F(ListKeysStepTest, MapPartiallyUnknown) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("var")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); auto init_step = std::make_unique(/*iter_slot=*/0, /*accu_slot=*/0); init_step->set_error_jump_offset(1); @@ -115,7 +116,7 @@ TEST_F(ListKeysStepTest, MapPartiallyUnknown) { TEST_F(ListKeysStepTest, ErrorPassedThrough) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("var")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); auto init_step = std::make_unique(/*iter_slot=*/0, /*accu_slot=*/0); init_step->set_error_jump_offset(1); @@ -140,7 +141,7 @@ TEST_F(ListKeysStepTest, ErrorPassedThrough) { TEST_F(ListKeysStepTest, UnknownSetPassedThrough) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("var")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); auto init_step = std::make_unique(/*iter_slot=*/0, /*accu_slot=*/0); init_step->set_error_jump_offset(1); diff --git a/eval/eval/container_access_step_test.cc b/eval/eval/container_access_step_test.cc index a298c28e3..fb60b7189 100644 --- a/eval/eval/container_access_step_test.cc +++ b/eval/eval/container_access_step_test.cc @@ -11,12 +11,15 @@ #include "absl/base/nullability.h" #include "absl/log/absl_check.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "base/builtins.h" #include "base/type_provider.h" #include "common/ast.h" #include "common/expr.h" #include "eval/eval/cel_expression_flat_impl.h" #include "eval/eval/evaluator_core.h" +#include "eval/eval/expression_step_logic.h" +#include "eval/eval/ident_step.h" #include "eval/public/activation.h" #include "eval/public/cel_attribute.h" #include "eval/public/cel_expr_builder_factory.h" @@ -75,18 +78,21 @@ CelValue EvaluateAttributeHelper( container_expr.mutable_ident_expr().set_name("container"); key_expr.mutable_ident_expr().set_name("key"); - path.push_back(ExpressionStep::MakeIdentifierStep("container", 1)); - path.push_back(ExpressionStep::MakeIdentifierStep("key", 2)); - path.push_back(ExpressionStep::MakeGenericStep( - std::move(CreateContainerAccessStep(call).value()), 3)); + path.push_back(ExpressionStep::MakeIdentifierStep(0, 1)); + path.push_back(ExpressionStep::MakeIdentifierStep(1, 2)); + absl::StatusOr> access_step = + CreateContainerAccessStep(call); + ABSL_CHECK_OK(access_step); // Crash OK + path.push_back(ExpressionStep::MakeGenericStep(*std::move(access_step), 3)); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; options.enable_heterogeneous_equality = false; CelExpressionFlatImpl cel_expr( - env, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), /*comprehension_slot_count=*/2, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, + IdentifierTable{{"container", 0}, {"key", 1}})); Activation activation; activation.InsertValue("container", container); diff --git a/eval/eval/create_list_step_test.cc b/eval/eval/create_list_step_test.cc index 56ac204b5..370482017 100644 --- a/eval/eval/create_list_step_test.cc +++ b/eval/eval/create_list_step_test.cc @@ -19,6 +19,7 @@ #include "common/value.h" #include "eval/eval/cel_expression_flat_impl.h" #include "eval/eval/evaluator_core.h" +#include "eval/eval/ident_step.h" #include "eval/internal/interop.h" #include "eval/public/activation.h" #include "eval/public/cel_attribute.h" @@ -72,7 +73,8 @@ absl::StatusOr RunExpression( FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; return cel_expr.Evaluate(activation, arena); @@ -87,6 +89,7 @@ absl::StatusOr RunExpressionWithCelValues( Expr dummy_expr; Activation activation; + IdentifierTable identifiers; auto& create_list = dummy_expr.mutable_list_expr(); int ind = 0; for (auto value : values) { @@ -95,7 +98,8 @@ absl::StatusOr RunExpressionWithCelValues( expr0.set_id(ind); expr0.mutable_ident_expr().set_name(var_name); - path.push_back(ExpressionStep::MakeIdentifierStep(var_name)); + path.push_back(ExpressionStep::MakeIdentifierStep(identifiers.size())); + identifiers.push_back(Identifier{var_name, identifiers.size()}); activation.InsertValue(var_name, value); } @@ -106,10 +110,12 @@ absl::StatusOr RunExpressionWithCelValues( options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } + const size_t slot_count = identifiers.size(); CelExpressionFlatImpl cel_expr( - env, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/slot_count, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, std::move(identifiers))); return cel_expr.Evaluate(activation, arena); } @@ -139,7 +145,7 @@ TEST(CreateListStepTest, TestCreateListStackUnderflow) { CelExpressionFlatImpl cel_expr( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; diff --git a/eval/eval/create_map_step_test.cc b/eval/eval/create_map_step_test.cc index f9f11516c..ff2148627 100644 --- a/eval/eval/create_map_step_test.cc +++ b/eval/eval/create_map_step_test.cc @@ -30,6 +30,7 @@ #include "common/expr.h" #include "eval/eval/cel_expression_flat_impl.h" #include "eval/eval/evaluator_core.h" +#include "eval/eval/ident_step.h" #include "eval/public/activation.h" #include "eval/public/cel_value.h" #include "eval/public/unknown_set.h" @@ -54,7 +55,7 @@ using ::google::protobuf::Arena; absl::StatusOr CreateStackMachineProgram( const std::vector>& values, - Activation& activation) { + Activation& activation, IdentifierTable& identifiers) { ExecutionPath path; Expr expr1; @@ -69,8 +70,10 @@ absl::StatusOr CreateStackMachineProgram( std::string key_name = absl::StrCat("key", index); std::string value_name = absl::StrCat("value", index); - path.push_back(ExpressionStep::MakeIdentifierStep(key_name)); - path.push_back(ExpressionStep::MakeIdentifierStep(value_name)); + path.push_back(ExpressionStep::MakeIdentifierStep(identifiers.size())); + identifiers.push_back(Identifier{key_name, identifiers.size()}); + path.push_back(ExpressionStep::MakeIdentifierStep(identifiers.size())); + identifiers.push_back(Identifier{value_name, identifiers.size()}); activation.InsertValue(key_name, item.first); activation.InsertValue(value_name, item.second); @@ -93,18 +96,22 @@ absl::StatusOr RunCreateMapExpression( const std::vector>& values, google::protobuf::Arena* arena, bool enable_unknowns, bool enable_recursive_program) { Activation activation; + IdentifierTable identifiers; ExecutionPath path; - CEL_ASSIGN_OR_RETURN(path, CreateStackMachineProgram(values, activation)); + CEL_ASSIGN_OR_RETURN( + path, CreateStackMachineProgram(values, activation, identifiers)); cel::RuntimeOptions options; if (enable_unknowns) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } + const size_t slot_count = identifiers.size(); CelExpressionFlatImpl cel_expr( - env, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/slot_count, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, std::move(identifiers))); return cel_expr.Evaluate(activation, arena); } diff --git a/eval/eval/create_struct_step_test.cc b/eval/eval/create_struct_step_test.cc index c6fa6f036..52cd06e6a 100644 --- a/eval/eval/create_struct_step_test.cc +++ b/eval/eval/create_struct_step_test.cc @@ -73,7 +73,7 @@ absl::StatusOr MakeStackMachinePath(absl::string_view field) { {std::string(field)}, /*optional_indices=*/{}); - path.push_back(ExpressionStep::MakeIdentifierStep("message")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); return path; @@ -103,9 +103,9 @@ absl::StatusOr RunExpression( CEL_ASSIGN_OR_RETURN(path, MakeStackMachinePath(field)); CelExpressionFlatImpl cel_expr( - env, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), /*comprehension_slot_count=*/1, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"message", 0}})); Activation activation; activation.InsertValue("message", value); @@ -189,7 +189,8 @@ TEST_P(CreateCreateStructStepTest, TestEmptyMessageCreation) { CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr.Evaluate(activation, &arena_)); diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index 2fc6eda93..c707cc22c 100644 --- a/eval/eval/evaluator_core.cc +++ b/eval/eval/evaluator_core.cc @@ -44,9 +44,13 @@ namespace google::api::expr::runtime { void FlatExpressionEvaluatorState::Reset() { + if (!dirty_) { + return; + } value_stack_.Clear(); iterator_stack_.Clear(); comprehension_slots_.Reset(); + dirty_ = false; } const ExpressionStep* ExecutionFrame::NextSlow() { @@ -239,9 +243,8 @@ absl::StatusOr FlatExpression::EvaluateWithCallback( const cel::EmbedderContext* absl_nullable embedder_context, EvaluationListener listener, FlatExpressionEvaluatorState& state) const { state.Reset(); - ExecutionFrame frame(subexpressions_, activation, options_, state, - std::move(listener), embedder_context); + std::move(listener), embedder_context, identifiers_); return frame.Evaluate(frame.callback()); } @@ -360,4 +363,11 @@ FixedJumpStepInfo* GetIfFixedJumpStep(ExpressionStep& step) { return nullptr; } +IdentStepInfo* GetIfIdentStep(ExpressionStep& step) { + if (step.header_.kind == ExpressionStepKind::kIdentifier) { + return &step.u_.ident_step; + } + return nullptr; +} + } // namespace google::api::expr::runtime diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index b389c38a6..c401d9475 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -19,11 +19,11 @@ #include #include #include -#include #include #include #include "absl/base/attributes.h" +#include "absl/base/no_destructor.h" #include "absl/base/nullability.h" #include "absl/base/optimization.h" #include "absl/log/absl_check.h" @@ -32,6 +32,7 @@ #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "absl/types/span.h" +#include "base/attribute.h" #include "base/type_provider.h" #include "common/value.h" #include "common/value_kind.h" @@ -294,16 +295,23 @@ class ExpressionStep { return step; } - static ExpressionStep MakeIdentifierStep(absl::string_view identifier, - int64_t id = -1) { + static ExpressionStep MakeIdentifierStep(size_t ident_index, + size_t slot_index, int64_t id) { ExpressionStep step(ExpressionStepKind::kIdentifier, id); - step.u_.identifier = new std::string(identifier); + ABSL_DCHECK_LE(ident_index, std::numeric_limits::max()); + ABSL_DCHECK_LE(slot_index, std::numeric_limits::max()); + step.u_.ident_step = IdentStepInfo{static_cast(ident_index), + static_cast(slot_index)}; return step; } - static ExpressionStep MakeIdentStep(absl::string_view identifier, - int64_t id = -1) { - return MakeIdentifierStep(identifier, id); + static ExpressionStep MakeIdentifierStep(size_t ident_index, + int64_t id = -1) { + return MakeIdentifierStep(ident_index, ident_index, id); + } + + static ExpressionStep MakeIdentStep(size_t ident_index, int64_t id = -1) { + return MakeIdentifierStep(ident_index, id); } static ExpressionStep MakeFastInStep(int64_t id = -1) { @@ -400,6 +408,7 @@ class ExpressionStep { friend BoolJumpStepInfo* GetIfBoolJumpStep(ExpressionStep& step); friend TernaryJumpStepInfo* GetIfTernaryJumpStep(ExpressionStep& step); friend FixedJumpStepInfo* GetIfFixedJumpStep(ExpressionStep& step); + friend IdentStepInfo* GetIfIdentStep(ExpressionStep& step); Header header_; // Note: ptr members are 'owned' by the step. @@ -423,7 +432,7 @@ class ExpressionStep { FixedJumpStepInfo fixed_jump_step; EagerFunctionStep* eager_function_step; LazyFunctionStep* lazy_function_step; - std::string* identifier; + IdentStepInfo ident_step; ListStepInfo* create_list_step; SmallListStepInfo create_small_list_step; @@ -475,6 +484,7 @@ class FlatExpressionEvaluatorState { arena_(arena) {} void Reset(); + void MarkDirty() { dirty_ = true; } EvaluatorStack& value_stack() { return value_stack_; } @@ -514,6 +524,7 @@ class FlatExpressionEvaluatorState { const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool_; google::protobuf::MessageFactory* absl_nullability_unknown message_factory_ = nullptr; google::protobuf::Arena* absl_nullability_unknown arena_ = nullptr; + bool dirty_ = false; }; // Context needed for evaluation. This is sufficient for supporting @@ -706,7 +717,8 @@ class ExecutionFrame : public ExecutionFrameBase { ExecutionPathView flat, const cel::ActivationInterface& activation, const cel::RuntimeOptions& options, FlatExpressionEvaluatorState& state, EvaluationListener callback = EvaluationListener(), - const cel::EmbedderContext* absl_nullable embedder_context = nullptr) + const cel::EmbedderContext* absl_nullable embedder_context = nullptr, + const IdentifierTable& identifiers = EmptyIdentifierTable()) : ExecutionFrameBase(activation, std::move(callback), options, state.type_provider(), state.descriptor_pool(), state.message_factory(), state.arena(), @@ -715,14 +727,18 @@ class ExecutionFrame : public ExecutionFrameBase { execution_path_(flat), value_stack_(&state.value_stack()), iterator_stack_(&state.iterator_stack()), - subexpressions_(&execution_path_, 1) {} + subexpressions_(&execution_path_, 1), + identifiers_(&identifiers) { + state.MarkDirty(); + } ExecutionFrame( absl::Span subexpressions, const cel::ActivationInterface& activation, const cel::RuntimeOptions& options, FlatExpressionEvaluatorState& state, EvaluationListener callback = EvaluationListener(), - const cel::EmbedderContext* absl_nullable embedder_context = nullptr) + const cel::EmbedderContext* absl_nullable embedder_context = nullptr, + const IdentifierTable& identifiers = EmptyIdentifierTable()) : ExecutionFrameBase(activation, std::move(callback), options, state.type_provider(), state.descriptor_pool(), state.message_factory(), state.arena(), @@ -731,8 +747,10 @@ class ExecutionFrame : public ExecutionFrameBase { execution_path_(subexpressions[0]), value_stack_(&state.value_stack()), iterator_stack_(&state.iterator_stack()), - subexpressions_(subexpressions) { + subexpressions_(subexpressions), + identifiers_(&identifiers) { ABSL_DCHECK(!subexpressions.empty()); + state.MarkDirty(); } // Returns next expression to evaluate. @@ -793,6 +811,8 @@ class ExecutionFrame : public ExecutionFrameBase { return *iterator_stack_; } + const IdentifierTable& identifiers() const { return *identifiers_; } + bool enable_attribute_tracking() const { return attribute_tracking_enabled(); } @@ -830,6 +850,11 @@ class ExecutionFrame : public ExecutionFrameBase { } private: + static const IdentifierTable& EmptyIdentifierTable() { + static const absl::NoDestructor kEmpty; + return *kEmpty; + } + struct SubFrame { size_t return_pc; size_t slot_index; @@ -857,6 +882,7 @@ class ExecutionFrame : public ExecutionFrameBase { EvaluatorStack* absl_nonnull const value_stack_; cel::runtime_internal::IteratorStack* absl_nonnull const iterator_stack_; absl::Span subexpressions_; + const IdentifierTable* absl_nonnull identifiers_; std::vector call_stack_; }; @@ -869,28 +895,32 @@ class FlatExpression { FlatExpression(ExecutionPath path, size_t comprehension_slots_size, const cel::TypeProvider& type_provider, const cel::RuntimeOptions& options, - absl_nullable std::shared_ptr arena = nullptr) + absl_nullable std::shared_ptr arena, + IdentifierTable identifiers) : path_(std::move(path)), subexpressions_({path_}), comprehension_slots_size_(comprehension_slots_size), iterator_stack_size_(comprehension_slots_size), type_provider_(type_provider), options_(options), - arena_(std::move(arena)) {} + arena_(std::move(arena)), + identifiers_(std::move(identifiers)) {} FlatExpression(ExecutionPath path, std::vector subexpressions, size_t comprehension_slots_size, size_t iterator_stack_size, const cel::TypeProvider& type_provider, const cel::RuntimeOptions& options, - absl_nullable std::shared_ptr arena = nullptr) + absl_nullable std::shared_ptr arena, + IdentifierTable identifiers) : path_(std::move(path)), subexpressions_(std::move(subexpressions)), comprehension_slots_size_(comprehension_slots_size), iterator_stack_size_(iterator_stack_size), type_provider_(type_provider), options_(options), - arena_(std::move(arena)) {} + arena_(std::move(arena)), + identifiers_(std::move(identifiers)) {} // Move-only FlatExpression(FlatExpression&&) = default; @@ -926,6 +956,8 @@ class FlatExpression { return subexpressions_; } + const IdentifierTable& identifiers() const { return identifiers_; } + const cel::RuntimeOptions& options() const { return options_; } size_t comprehension_slots_size() const { return comprehension_slots_size_; } @@ -944,6 +976,7 @@ class FlatExpression { // Arena used during planning phase, may hold constant values so should be // kept alive. absl_nullable std::shared_ptr arena_; + IdentifierTable identifiers_; }; // Helper functions for checking ExpressionStep kinds. Used for program @@ -983,6 +1016,14 @@ inline FixedJumpStepInfo* GetIfFixedJumpStep(ExpressionStep* step) { return GetIfFixedJumpStep(*step); } +IdentStepInfo* GetIfIdentStep(ExpressionStep& step); +inline IdentStepInfo* GetIfIdentStep(ExpressionStep* step) { + if (step == nullptr) { + return nullptr; + } + return GetIfIdentStep(*step); +} + // Implementation details. inline ExpressionStep::~ExpressionStep() { @@ -993,9 +1034,6 @@ inline ExpressionStep::~ExpressionStep() { case ExpressionStepKind::kOtherConstant: delete u_.other_val; break; - case ExpressionStepKind::kIdentifier: - delete u_.identifier; - break; case ExpressionStepKind::kEagerFunction: delete u_.eager_function_step; break; @@ -1028,6 +1066,7 @@ inline ExpressionStep::~ExpressionStep() { case ExpressionStepKind::kBooleanAndJump: case ExpressionStepKind::kTernaryJump: case ExpressionStepKind::kFixedJump: + case ExpressionStepKind::kIdentifier: case ExpressionStepKind::kFastIn: case ExpressionStepKind::kFastEqual: case ExpressionStepKind::kFastNotEqual: @@ -1201,7 +1240,7 @@ inline void ExpressionStep::Evaluate(ExecutionFrame& frame) const { frame.JumpToOrAbort(u_.fixed_jump_step.offset); break; case ExpressionStepKind::kIdentifier: - EvaluateIdentifierStep(*u_.identifier, frame); + EvaluateIdentifierStep(u_.ident_step, frame); break; case ExpressionStepKind::kEagerFunction: u_.eager_function_step->Evaluate(frame); diff --git a/eval/eval/evaluator_core_test.cc b/eval/eval/evaluator_core_test.cc index 1f43f3f35..44ad09659 100644 --- a/eval/eval/evaluator_core_test.cc +++ b/eval/eval/evaluator_core_test.cc @@ -142,6 +142,13 @@ TEST(EvaluatorCoreTest, TestStepDataAccessors) { EXPECT_EQ(GetIfTernaryJumpStep(static_cast(nullptr)), nullptr); + ExpressionStep ident_step = ExpressionStep::MakeIdentifierStep(1, 2, 3); + ASSERT_NE(GetIfIdentStep(ident_step), nullptr); + EXPECT_EQ(GetIfIdentStep(ident_step)->ident_index, 1); + EXPECT_EQ(GetIfIdentStep(ident_step)->slot_index, 2); + EXPECT_NE(GetIfIdentStep(&ident_step), nullptr); + EXPECT_EQ(GetIfIdentStep(static_cast(nullptr)), nullptr); + ExpressionStep comp_cond = ExpressionStep::MakeComprehensionCondStep(); EXPECT_NE(GetIfComprehensionCondStep(comp_cond), nullptr); @@ -190,7 +197,7 @@ TEST(EvaluatorCoreTest, SimpleEvaluatorTest) { CelExpressionFlatImpl impl( env, FlatExpression(std::move(path), 0, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; @@ -214,7 +221,8 @@ TEST(EvaluatorCoreTest, MakeConstant) { [&](const cel::Value& value) -> absl::StatusOr { ExecutionPath path; path.push_back(ExpressionStep::MakeConstant(value)); - FlatExpression expr(std::move(path), 0, type_provider, options); + FlatExpression expr(std::move(path), 0, type_provider, options, + /*arena=*/nullptr, {}); auto state = expr.MakeEvaluatorState( cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); @@ -279,7 +287,7 @@ TEST(EvaluatorCoreTest, TraceFilterById) { CelExpressionFlatImpl impl( env, FlatExpression(std::move(path), 0, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; diff --git a/eval/eval/function_step_test.cc b/eval/eval/function_step_test.cc index 9b9ce6a36..c42bf0604 100644 --- a/eval/eval/function_step_test.cc +++ b/eval/eval/function_step_test.cc @@ -236,7 +236,8 @@ class FunctionStepTest return std::make_unique( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); } }; @@ -604,15 +605,18 @@ INSTANTIATE_TEST_SUITE_P( class FunctionStepTestUnknowns : public testing::TestWithParam { public: - std::unique_ptr GetExpression(ExecutionPath&& path) { + std::unique_ptr GetExpression( + ExecutionPath&& path, IdentifierTable identifiers = {}) { cel::RuntimeOptions options; options.unknown_processing = GetParam(); auto env = NewTestingRuntimeEnv(); + const size_t slot_count = identifiers.size(); return std::make_unique( env, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + FlatExpression(std::move(path), /*comprehension_slot_count=*/slot_count, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, std::move(identifiers))); } }; @@ -654,13 +658,14 @@ TEST_P(FunctionStepTestUnknowns, PartialUnknownHandlingTest) { IdentExpr ident1; ident1.set_name("param"); CallExpr call1 = SinkFunction::MakeCall(); - auto step0 = ExpressionStep::MakeIdentifierStep("param", GetExprId()); + ExpressionStep step0 = ExpressionStep::MakeIdentifierStep(0, GetExprId()); ASSERT_OK_AND_ASSIGN(auto step1, MakeTestFunctionStep(call1, registry)); path.push_back(std::move(step0)); path.push_back(std::move(step1)); - std::unique_ptr impl = GetExpression(std::move(path)); + std::unique_ptr impl = + GetExpression(std::move(path), IdentifierTable{{"param", 0}}); Activation activation; TestMessage msg; @@ -748,10 +753,10 @@ TEST(FunctionStepTestUnknownFunctionResults, CaptureArgs) { cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; @@ -798,10 +803,10 @@ TEST(FunctionStepTestUnknownFunctionResults, MergeDownCaptureArgs) { cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; @@ -848,10 +853,10 @@ TEST(FunctionStepTestUnknownFunctionResults, MergeCaptureArgs) { cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; @@ -893,10 +898,10 @@ TEST(FunctionStepTestUnknownFunctionResults, UnknownVsErrorPrecedenceTest) { cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; @@ -983,10 +988,10 @@ TEST(FunctionStepStrictnessTest, cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; ASSERT_OK_AND_ASSIGN(CelValue value, impl.Evaluate(activation, &arena)); @@ -1015,10 +1020,10 @@ TEST(FunctionStepStrictnessTest, IfFunctionNonStrictAndGivenUnknownInvokesIt) { cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; ASSERT_OK_AND_ASSIGN(CelValue value, impl.Evaluate(activation, &arena)); diff --git a/eval/eval/ident_step.cc b/eval/eval/ident_step.cc index db1173101..7bce4c382 100644 --- a/eval/eval/ident_step.cc +++ b/eval/eval/ident_step.cc @@ -1,15 +1,17 @@ #include "eval/eval/ident_step.h" +#include #include #include +#include "absl/base/optimization.h" #include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "common/value.h" #include "eval/eval/attribute_trail.h" +#include "eval/eval/comprehension_slots.h" #include "eval/eval/evaluator_core.h" -#include "eval/eval/expression_step_logic.h" #include "eval/internal/errors.h" #include "internal/status_macros.h" @@ -40,7 +42,7 @@ absl::Status LookupIdent(absl::string_view name, ExecutionFrameBase& frame, } CEL_ASSIGN_OR_RETURN( - auto found, frame.activation().FindVariable(name, frame.descriptor_pool(), + bool found, frame.activation().FindVariable(name, frame.descriptor_pool(), frame.message_factory(), frame.arena(), &result)); @@ -58,16 +60,27 @@ absl::Status LookupIdent(absl::string_view name, ExecutionFrameBase& frame, } // namespace -void EvaluateIdentifierStep(absl::string_view identifier, - ExecutionFrame& frame) { +void EvaluateIdentifierStep(const IdentStepInfo& step, ExecutionFrame& frame) { + ComprehensionSlot* slot = frame.comprehension_slots().Get(step.slot_index); + if (slot->Has()) { + frame.value_stack().Push(slot->value(), slot->attribute()); + return; + } + if (ABSL_PREDICT_FALSE(step.ident_index >= frame.identifiers().size())) { + frame.Abort(absl::InternalError( + absl::StrCat("Identifier index out of range: ", step.ident_index))); + return; + } + const Identifier& ident = frame.identifiers()[step.ident_index]; frame.value_stack().Push(cel::NullValue()); if (absl::Status status = - LookupIdent(identifier, frame, frame.value_stack().Peek(), + LookupIdent(ident.name, frame, frame.value_stack().Peek(), frame.value_stack().PeekAttribute()); !status.ok()) { frame.Abort(std::move(status)); return; } + slot->Set(frame.value_stack().Peek(), frame.value_stack().PeekAttribute()); } } // namespace google::api::expr::runtime diff --git a/eval/eval/ident_step.h b/eval/eval/ident_step.h index 2ce50c58e..fa35ad6d7 100644 --- a/eval/eval/ident_step.h +++ b/eval/eval/ident_step.h @@ -1,14 +1,28 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_IDENT_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_IDENT_STEP_H_ -#include "absl/strings/string_view.h" +#include +#include +#include +#include namespace google::api::expr::runtime { class ExecutionFrame; -void EvaluateIdentifierStep(absl::string_view identifier, - ExecutionFrame& frame); +struct Identifier { + std::string name; + size_t slot_index = 0; +}; + +using IdentifierTable = std::vector; + +struct IdentStepInfo { + uint32_t ident_index; + uint32_t slot_index; +}; + +void EvaluateIdentifierStep(const IdentStepInfo& step, ExecutionFrame& frame); } // namespace google::api::expr::runtime diff --git a/eval/eval/ident_step_test.cc b/eval/eval/ident_step_test.cc index 49ef870d6..f651ec439 100644 --- a/eval/eval/ident_step_test.cc +++ b/eval/eval/ident_step_test.cc @@ -1,6 +1,7 @@ #include "eval/eval/ident_step.h" #include +#include #include #include #include @@ -11,7 +12,9 @@ #include "eval/eval/cel_expression_flat_impl.h" #include "eval/eval/evaluator_core.h" #include "eval/public/activation.h" +#include "eval/public/base_activation.h" #include "eval/public/cel_attribute.h" +#include "eval/public/cel_function.h" #include "eval/public/cel_value.h" #include "internal/testing.h" #include "runtime/internal/runtime_env_testing.h" @@ -32,20 +35,21 @@ using ::testing::Eq; TEST(IdentStepTest, TestIdentStep) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env, FlatExpression(std::move(path), /*comprehension_slot_count=*/1, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, + IdentifierTable{{"name0", 0}})); Activation activation; Arena arena; std::string value("test"); activation.InsertValue("name0", CelValue::CreateString(&value)); - auto status0 = impl.Evaluate(activation, &arena); + absl::StatusOr status0 = impl.Evaluate(activation, &arena); ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -54,21 +58,74 @@ TEST(IdentStepTest, TestIdentStep) { EXPECT_THAT(result.StringOrDie().value(), Eq("test")); } +TEST(IdentStepTest, TestIdentStepWithSlotCachesLookup) { + ExecutionPath path; + // Evaluate "name0" twice and check equality (or return the second after + // popping the first viaComprehensionFinish / ReadSlot). Here we read the + // identifier step twice and then read slot 1 directly. + path.push_back(ExpressionStep::MakeIdentifierStep( + /*ident_index=*/0, /*slot_index=*/1, /*id=*/-1)); + path.push_back(ExpressionStep::MakeAssignSlotAndPopStep(0)); + path.push_back(ExpressionStep::MakeIdentifierStep( + /*ident_index=*/0, /*slot_index=*/1, /*id=*/-1)); + + auto env = NewTestingRuntimeEnv(); + CelExpressionFlatImpl impl( + env, FlatExpression(std::move(path), /*comprehension_slot_count=*/2, + env->type_registry.GetComposedTypeProvider(), + cel::RuntimeOptions{}, /*arena=*/nullptr, + IdentifierTable{{"name0", /*slot_index=*/1}})); + + class CountingActivation : public BaseActivation { + public: + CountingActivation(int* counter, const std::string* val) + : counter_(counter), val_(val) {} + std::vector FindFunctionOverloads( + absl::string_view) const override { + return {}; + } + std::optional FindValue(absl::string_view name, + Arena*) const override { + if (name == "name0") { + (*counter_)++; + return CelValue::CreateString(val_); + } + return std::nullopt; + } + + private: + int* counter_; + const std::string* val_; + }; + + Arena arena; + int lookup_calls = 0; + std::string value("cached_value"); + CountingActivation activation(&lookup_calls, &value); + + absl::StatusOr status0 = impl.Evaluate(activation, &arena); + ASSERT_THAT(status0, IsOk()); + ASSERT_TRUE(status0->IsString()); + EXPECT_THAT(status0->StringOrDie().value(), Eq("cached_value")); + EXPECT_EQ(lookup_calls, 1); +} + TEST(IdentStepTest, TestIdentStepNameNotFound) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env, FlatExpression(std::move(path), /*comprehension_slot_count=*/1, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, + IdentifierTable{{"name0", 0}})); Activation activation; Arena arena; std::string value("test"); - auto status0 = impl.Evaluate(activation, &arena); + absl::StatusOr status0 = impl.Evaluate(activation, &arena); ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -77,22 +134,22 @@ TEST(IdentStepTest, TestIdentStepNameNotFound) { TEST(IdentStepTest, DisableMissingAttributeErrorsOK) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kDisabled; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/1, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"name0", 0}})); Activation activation; Arena arena; std::string value("test"); activation.InsertValue("name0", CelValue::CreateString(&value)); - auto status0 = impl.Evaluate(activation, &arena); + absl::StatusOr status0 = impl.Evaluate(activation, &arena); ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -111,7 +168,7 @@ TEST(IdentStepTest, DisableMissingAttributeErrorsOK) { TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kDisabled; @@ -119,17 +176,17 @@ TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/1, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"name0", 0}})); Activation activation; Arena arena; std::string value("test"); activation.InsertValue("name0", CelValue::CreateString(&value)); - auto status0 = impl.Evaluate(activation, &arena); + absl::StatusOr status0 = impl.Evaluate(activation, &arena); ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -149,17 +206,17 @@ TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { TEST(IdentStepTest, TestIdentStepUnknownAttribute) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); // Expression with unknowns enabled. cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/1, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"name0", 0}})); Activation activation; Arena arena; @@ -170,7 +227,7 @@ TEST(IdentStepTest, TestIdentStepUnknownAttribute) { unknown_patterns.push_back(CelAttributePattern("name_bad", {})); ASSERT_THAT(activation.SetUnknownAttributePatterns(unknown_patterns), IsOk()); - auto status0 = impl.Evaluate(activation, &arena); + absl::StatusOr status0 = impl.Evaluate(activation, &arena); ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); diff --git a/eval/eval/logic_step_test.cc b/eval/eval/logic_step_test.cc index 59888b20a..44a0a2a56 100644 --- a/eval/eval/logic_step_test.cc +++ b/eval/eval/logic_step_test.cc @@ -39,8 +39,8 @@ class LogicStepTest : public testing::TestWithParam { absl::Status EvaluateLogic(CelValue arg0, CelValue arg1, bool is_or, CelValue* result, bool enable_unknown) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); - path.push_back(ExpressionStep::MakeIdentifierStep("name1")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); + path.push_back(ExpressionStep::MakeIdentifierStep(1)); path.push_back( (is_or) ? ExpressionStep::MakeBooleanOrStep(/*num_args=*/2, /*id=*/2) : ExpressionStep::MakeBooleanAndStep(/*num_args=*/2, /*id=*/2)); @@ -53,8 +53,10 @@ class LogicStepTest : public testing::TestWithParam { } CelExpressionFlatImpl impl( env_, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + FlatExpression(std::move(path), /*comprehension_slot_count=*/2, + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, + IdentifierTable{{"name0", 0}, {"name1", 1}})); Activation activation; activation.InsertValue("name0", arg0); diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index 4963550b4..e2adb9d2c 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -85,8 +85,7 @@ class SelectStepTest : public testing::Test { CreateSelectStep(select.field(), select.test_only(), options.enable_wrapper_type_null_unboxing)); - path.push_back( - ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, expr0.id())); path.push_back( ExpressionStep::MakeGenericStep(std::move(step1), expr.id())); @@ -98,9 +97,10 @@ class SelectStepTest : public testing::Test { // Force the creation of a message factory at the env level. static_cast(env_->MutableMessageFactory()); CelExpressionFlatImpl cel_expr( - env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/1, env_->type_registry.GetComposedTypeProvider(), - runtime_options)); + runtime_options, /*arena=*/nullptr, + IdentifierTable{{ident.name(), 0}})); Activation activation; activation.InsertValue("target", target); @@ -273,14 +273,15 @@ TEST_F(SelectStepTest, MapPresenseIsErrorTest) { /*enable_wrapper_type_null_unboxing=*/false)); ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, 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_, FlatExpression(std::move(path), /*comprehension_slot_count=*/1, env_->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, + IdentifierTable{{ident.name(), 0}})); Activation activation; activation.InsertValue("target", CelProtoWrapper::CreateMessage(&message, &arena_)); @@ -708,7 +709,7 @@ TEST_P(SelectStepConformanceTest, CelErrorAsArgument) { CreateSelectStep(select.field(), select.test_only(), /*enable_wrapper_type_null_unboxing=*/false)); - path.push_back(ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, expr0.id())); path.push_back( ExpressionStep::MakeGenericStep(std::move(step1), dummy_expr.id())); @@ -720,8 +721,9 @@ TEST_P(SelectStepConformanceTest, CelErrorAsArgument) { } CelExpressionFlatImpl cel_expr( env_, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + FlatExpression(std::move(path), /*comprehension_slot_count=*/1, + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{ident.name(), 0}})); Activation activation; activation.InsertValue("message", CelValue::CreateError(&error)); @@ -749,14 +751,15 @@ TEST_F(SelectStepTest, DisableMissingAttributeOK) { CreateSelectStep(select.field(), select.test_only(), /*enable_wrapper_type_null_unboxing=*/false)); - path.push_back(ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, 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, + env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/1, env_->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, + IdentifierTable{{ident.name(), 0}})); Activation activation; activation.InsertValue("message", CelProtoWrapper::CreateMessage(&message, &arena_)); @@ -791,7 +794,7 @@ TEST_F(SelectStepTest, UnrecoverableUnknownValueProducesError) { CreateSelectStep(select.field(), select.test_only(), /*enable_wrapper_type_null_unboxing=*/false)); - path.push_back(ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, expr0.id())); path.push_back( ExpressionStep::MakeGenericStep(std::move(step1), dummy_expr.id())); @@ -799,8 +802,9 @@ TEST_F(SelectStepTest, UnrecoverableUnknownValueProducesError) { options.enable_missing_attribute_errors = true; CelExpressionFlatImpl cel_expr( env_, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + FlatExpression(std::move(path), /*comprehension_slot_count=*/1, + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{ident.name(), 0}})); Activation activation; activation.InsertValue("message", CelProtoWrapper::CreateMessage(&message, &arena_)); @@ -840,7 +844,7 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { ASSERT_THAT(step1_status, IsOk()); - path.push_back(ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, expr0.id())); path.push_back(ExpressionStep::MakeGenericStep(std::move(*step1_status), dummy_expr.id())); @@ -848,8 +852,9 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; CelExpressionFlatImpl cel_expr( env_, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + FlatExpression(std::move(path), /*comprehension_slot_count=*/1, + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{ident.name(), 0}})); { std::vector unknown_patterns; @@ -947,7 +952,7 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepTest) { /*enable_optional_types=*/false)); ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("message")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); cel::RuntimeOptions options; if (GetParam()) { @@ -955,8 +960,9 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepTest) { } CelExpressionFlatImpl cel_expr( env_, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + FlatExpression(std::move(path), /*comprehension_slot_count=*/1, + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"message", 0}})); TestAllTypes message; message.set_single_int64(42); @@ -985,7 +991,7 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepPropagatesUnknown) { /*enable_optional_types=*/false)); ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("message")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); cel::RuntimeOptions options; if (GetParam()) { @@ -993,8 +999,9 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepPropagatesUnknown) { } CelExpressionFlatImpl cel_expr( env_, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + FlatExpression(std::move(path), /*comprehension_slot_count=*/1, + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"message", 0}})); UnknownSet unknown_set(UnknownAttributeSet({CelAttribute("message", {})})); Activation activation; @@ -1020,14 +1027,15 @@ TEST_F(SelectStepTest, TypedSelectStepUnknownPatternResolvesToUnknown) { /*enable_optional_types=*/false)); ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("message")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; CelExpressionFlatImpl cel_expr( env_, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + FlatExpression(std::move(path), /*comprehension_slot_count=*/1, + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"message", 0}})); TestAllTypes message; message.set_single_int64(42); diff --git a/eval/eval/shadowable_value_step_test.cc b/eval/eval/shadowable_value_step_test.cc index 42234b515..46721f100 100644 --- a/eval/eval/shadowable_value_step_test.cc +++ b/eval/eval/shadowable_value_step_test.cc @@ -44,7 +44,7 @@ absl::StatusOr RunShadowableExpression( CelExpressionFlatImpl impl( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, {})); return impl.Evaluate(activation, arena); } diff --git a/eval/eval/ternary_step_test.cc b/eval/eval/ternary_step_test.cc index d1011f247..e1d9e6cbe 100644 --- a/eval/eval/ternary_step_test.cc +++ b/eval/eval/ternary_step_test.cc @@ -9,6 +9,7 @@ #include "base/type_provider.h" #include "eval/eval/cel_expression_flat_impl.h" #include "eval/eval/evaluator_core.h" +#include "eval/eval/ident_step.h" #include "eval/public/activation.h" #include "eval/public/cel_value.h" #include "internal/testing.h" @@ -36,9 +37,9 @@ class LogicStepTest : public testing::TestWithParam { CelValue* result, bool enable_unknown) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); - path.push_back(ExpressionStep::MakeIdentifierStep("name1")); - path.push_back(ExpressionStep::MakeIdentifierStep("name2")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); + path.push_back(ExpressionStep::MakeIdentifierStep(1)); + path.push_back(ExpressionStep::MakeIdentifierStep(2)); path.push_back(ExpressionStep::MakeGenericStep(CreateTernaryStep(), 4)); cel::RuntimeOptions options; @@ -47,9 +48,11 @@ class LogicStepTest : public testing::TestWithParam { cel::UnknownProcessingOptions::kAttributeOnly; } CelExpressionFlatImpl impl( - env_, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + env_, FlatExpression( + std::move(path), /*comprehension_slot_count=*/3, + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, + IdentifierTable{{"name0", 0}, {"name1", 1}, {"name2", 2}})); Activation activation; std::string value("test");