From 1ff12a19b2f04e21b24774e7e14c0f81f0ea8410 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Mon, 12 Oct 2026 16:51:55 -0700 Subject: [PATCH] Map root identifiers to slots and memoize. PiperOrigin-RevId: 998236649 --- eval/compiler/BUILD | 13 ++ eval/compiler/constant_folding.cc | 9 +- eval/compiler/flat_expr_builder.cc | 100 +++++++++++++- .../flat_expr_builder_comprehensions_test.cc | 125 ++++++++++++++++++ eval/compiler/flat_expr_builder_test.cc | 106 +++++++++++++++ eval/eval/BUILD | 6 + eval/eval/comprehension_slots.h | 2 +- eval/eval/comprehension_step_test.cc | 11 +- eval/eval/container_access_step_test.cc | 20 ++- eval/eval/create_list_step_test.cc | 18 ++- eval/eval/create_map_step_test.cc | 21 ++- eval/eval/create_struct_step_test.cc | 11 +- eval/eval/equality_steps_test.cc | 8 +- eval/eval/evaluator_core.cc | 22 ++- eval/eval/evaluator_core.h | 99 +++++++++----- eval/eval/evaluator_core_test.cc | 18 ++- eval/eval/function_step_test.cc | 65 ++++----- eval/eval/ident_step.cc | 23 +++- eval/eval/ident_step.h | 19 ++- eval/eval/ident_step_test.cc | 109 +++++++++++---- eval/eval/iterator_stack.h | 3 +- eval/eval/lazy_init_step_test.cc | 3 +- eval/eval/logic_step_test.cc | 10 +- eval/eval/select_step_test.cc | 64 +++++---- eval/eval/shadowable_value_step_test.cc | 2 +- eval/eval/ternary_step_test.cc | 15 ++- extensions/bindings_ext.cc | 4 + extensions/bindings_ext.h | 4 + testutil/BUILD | 1 + testutil/test_macros.cc | 16 ++- testutil/test_macros.h | 3 + 31 files changed, 738 insertions(+), 192 deletions(-) diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index 8d9a5aa3b..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", @@ -204,6 +207,11 @@ cc_test( ":cel_expression_builder_flat_impl", ":comprehension_vulnerability_check", ":flat_expr_builder", + "//base:ast", + "//common:source", + "//common:value", + "//common:value_testing", + "//eval/eval:evaluator_core", "//eval/public:activation", "//eval/public:builtin_func_registrar", "//eval/public:cel_attribute", @@ -213,10 +221,15 @@ cc_test( "//eval/public/containers:container_backed_list_impl", "//eval/public/testing:matchers", "//eval/testutil:test_message_cc_proto", + "//extensions:bindings_ext", "//internal:testing", "//parser", + "//parser:parser_interface", + "//runtime:activation", "//runtime:runtime_options", + "//runtime/internal:runtime_env", "//runtime/internal:runtime_env_testing", + "//testutil:test_macros", "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", diff --git a/eval/compiler/constant_folding.cc b/eval/compiler/constant_folding.cc index fb58d7d48..8e854a867 100644 --- a/eval/compiler/constant_folding.cc +++ b/eval/compiler/constant_folding.cc @@ -75,8 +75,9 @@ class ConstantFoldingExtension : public ProgramOptimizer { const TypeProvider& type_provider) : shared_arena_(std::move(shared_arena)), shared_message_factory_(std::move(shared_message_factory)), - state_(kDefaultStackLimit, kComprehensionSlotCount, type_provider, - descriptor_pool, message_factory, arena) {} + state_(kDefaultStackLimit, kComprehensionSlotCount, + /*iterator_stack_size=*/0, type_provider, descriptor_pool, + message_factory, arena) {} absl::Status OnPreVisit(google::api::expr::runtime::PlannerContext& context, const Expr& node) override; @@ -200,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 f0920bf18..7f6d6963f 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" @@ -105,7 +106,12 @@ class FlatExprVisitor; // Helper for bookkeeping variables mapped to indexes. class IndexManager { public: - IndexManager() : next_free_slot_(0), max_slot_count_(0) {} + IndexManager() + : next_free_slot_(0), + max_slot_count_(0), + iter_stack_count_(0), + max_iter_stack_count_(0), + lazy_iter_stack_count_(0) {} size_t ReserveSlots(size_t n) { size_t result = next_free_slot_; @@ -121,11 +127,30 @@ class IndexManager { return next_free_slot_; } + void ReserveIterStack() { + ++iter_stack_count_; + max_iter_stack_count_ = std::max(max_iter_stack_count_, iter_stack_count_); + } + void ReleaseIterStack() { --iter_stack_count_; } + void AddLazyIterStack() { ++lazy_iter_stack_count_; } + size_t max_slot_count() const { return max_slot_count_; } + // Returns the maximum number of iterator stack slots required to evaluate the + // expression by tracking the maximum comprehension nesting in the main + // expression and dedicating a slot for any comprehensions evaluated lazily. + // This is not as tight as possible, but is a correct upper bound estimate and + // is tight for small expressions. + size_t IterStackRequirement() const { + return max_iter_stack_count_ + lazy_iter_stack_count_; + } + private: size_t next_free_slot_; size_t max_slot_count_; + size_t iter_stack_count_; + size_t max_iter_stack_count_; + size_t lazy_iter_stack_count_; }; // Helper for computing jump offsets. @@ -868,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, @@ -1145,21 +1174,36 @@ class FlatExprVisitor : public cel::AstVisitor { slot_count = 0; } } + // If this is in the scope of an optimized bind accu-init, account the slots // to the outermost bind-init scope. // // The init expression is effectively inlined at the first usage in the // critical path (which is unknown at plan time), so the used slots need to // be dedicated for the entire scope of that bind. + bool in_bind_init = false; for (ComprehensionStackRecord& record : comprehension_stack_) { if (record.in_accu_init && record.is_optimizable_bind) { record.slot_count += slot_count; slot_count = 0; + in_bind_init = true; break; } // If no bind init subexpression, account normally. } + // Iterator frames are releasable if not in a bind (no iterator push), and + // not in a lazy subexpression (which may evaluate in different implicit + // scopes). + const bool is_stack_releasable = + !is_bind && !in_bind_init && (!block_.has_value() || !block_->in); + + if (is_stack_releasable) { + index_manager_.ReserveIterStack(); + } else if (!is_bind) { + index_manager_.AddLazyIterStack(); + } + comprehension_stack_.push_back( {&expr, &comprehension, iter_slot, iter2_slot, accu_slot, slot_count, /*subexpression=*/-1, @@ -1172,6 +1216,7 @@ class FlatExprVisitor : public cel::AstVisitor { options_.enable_comprehension_mutable_map, options_.short_circuiting), /*.is_optimizable_bind=*/is_bind, + /*.is_stack_releasable=*/is_stack_releasable, /*.iter_var_in_scope=*/false, /*.iter_var2_in_scope=*/false, /*.accu_var_in_scope=*/false, @@ -1198,6 +1243,10 @@ class FlatExprVisitor : public cel::AstVisitor { record.visitor->PostVisit(&expr); index_manager_.ReleaseSlots(record.slot_count); + if (record.is_stack_releasable) { + ABSL_DCHECK(!record.is_optimizable_bind); + index_manager_.ReleaseIterStack(); + } comprehension_stack_.pop_back(); } @@ -1499,6 +1548,19 @@ class FlatExprVisitor : public cel::AstVisitor { size_t slot_count() const { return index_manager_.max_slot_count(); } + size_t iter_stack_count() const { + 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)); } @@ -1530,6 +1592,7 @@ class FlatExprVisitor : public cel::AstVisitor { bool is_optimizable_list_append; bool is_optimizable_map_insert; bool is_optimizable_bind; + bool is_stack_releasable; bool iter_var_in_scope; bool iter_var2_in_scope; bool accu_var_in_scope; @@ -1649,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_; @@ -1677,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_; @@ -1823,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, @@ -2280,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 @@ -2296,10 +2378,18 @@ 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. + for (ExpressionStep& step : execution_path) { + if (IdentStepInfo* ident_step = GetIfIdentStep(step); + ident_step != nullptr) { + ident_step->slot_index = identifiers[ident_step->ident_index].slot_index; + } + } return FlatExpression(std::move(execution_path), std::move(subexpressions), - visitor.slot_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_comprehensions_test.cc b/eval/compiler/flat_expr_builder_comprehensions_test.cc index ed0f67912..df60a456d 100644 --- a/eval/compiler/flat_expr_builder_comprehensions_test.cc +++ b/eval/compiler/flat_expr_builder_comprehensions_test.cc @@ -14,15 +14,22 @@ * limitations under the License. */ +#include +#include #include #include "cel/expr/syntax.pb.h" #include "google/protobuf/field_mask.pb.h" #include "absl/status/status.h" #include "absl/strings/string_view.h" +#include "base/ast.h" +#include "common/source.h" +#include "common/value.h" +#include "common/value_testing.h" #include "eval/compiler/cel_expression_builder_flat_impl.h" #include "eval/compiler/comprehension_vulnerability_check.h" #include "eval/compiler/flat_expr_builder.h" +#include "eval/eval/evaluator_core.h" #include "eval/public/activation.h" #include "eval/public/builtin_func_registrar.h" #include "eval/public/cel_attribute.h" @@ -32,10 +39,15 @@ #include "eval/public/containers/container_backed_list_impl.h" #include "eval/public/testing/matchers.h" #include "eval/testutil/test_message.pb.h" +#include "extensions/bindings_ext.h" #include "internal/testing.h" #include "parser/parser.h" +#include "parser/parser_interface.h" +#include "runtime/activation.h" +#include "runtime/internal/runtime_env.h" #include "runtime/internal/runtime_env_testing.h" #include "runtime/runtime_options.h" +#include "testutil/test_macros.h" #include "google/protobuf/arena.h" #include "google/protobuf/text_format.h" @@ -623,6 +635,119 @@ TEST_F(CelExpressionBuilderFlatImplComprehensionsTest, HasSubstr("Unexpected iter_var access in trivial comprehension"))); } +TEST_F(CelExpressionBuilderFlatImplComprehensionsTest, + BindInsideComprehensionWithComprehensionInInitIterStackCount) { + std::unique_ptr parser_builder = cel::NewParserBuilder(); + ASSERT_THAT( + parser_builder->AddLibrary(cel::extensions::BindingsParserLibrary()), + IsOk()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr parser, + parser_builder->Build()); + + ASSERT_OK_AND_ASSIGN( + std::unique_ptr source, + parser->PrepareSource( + "[[1, 2], [3, 4]].map(l, cel.bind(filtered, l.filter(x, x > 1), " + "size(filtered) > 0 ? filtered[0] : 0)) == [2, 3]")); + ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, parser->Parse(*source)); + + cel::RuntimeOptions options = GetRuntimeOptions(); + std::shared_ptr env = + NewTestingRuntimeEnv(); + CelExpressionBuilderFlatImpl builder(env, options); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry()), IsOk()); + + ASSERT_OK_AND_ASSIGN( + FlatExpression flat_expr, + builder.flat_expr_builder().CreateExpressionImpl(std::move(ast), + /*issues=*/nullptr)); + EXPECT_EQ(flat_expr.iterator_stack_size(), 2); + + cel::Activation activation; + google::protobuf::Arena arena; + FlatExpressionEvaluatorState state = flat_expr.MakeEvaluatorState( + env->descriptor_pool.get(), env->MutableMessageFactory(), &arena); + ASSERT_OK_AND_ASSIGN( + cel::Value result, + flat_expr.EvaluateWithCallback(activation, /*embedder_context=*/nullptr, + EvaluationListener(), state)); + EXPECT_THAT(result, cel::test::BoolValueIs(true)); +} + +TEST_F(CelExpressionBuilderFlatImplComprehensionsTest, + BlockWithComprehensionInBindingAndBodyIterStackCount) { + std::unique_ptr parser_builder = cel::NewParserBuilder(); + ASSERT_THAT(parser_builder->AddLibrary(cel::test::TestMacrosParserLibrary()), + IsOk()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr parser, + parser_builder->Build()); + + ASSERT_OK_AND_ASSIGN( + std::unique_ptr source, + parser->PrepareSource( + "cel.block([[1, 2, 3].filter(x, x > 1), [1, 2, 3].filter(x, x < 3)], " + "[1, 2].map(y, y in cel.index(0) || y in cel.index(1))) == [true, " + "true]")); + ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, parser->Parse(*source)); + + cel::RuntimeOptions options = GetRuntimeOptions(); + std::shared_ptr env = + NewTestingRuntimeEnv(); + CelExpressionBuilderFlatImpl builder(env, options); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry()), IsOk()); + + ASSERT_OK_AND_ASSIGN( + FlatExpression flat_expr, + builder.flat_expr_builder().CreateExpressionImpl(std::move(ast), + /*issues=*/nullptr)); + EXPECT_EQ(flat_expr.iterator_stack_size(), 3); + + cel::Activation activation; + google::protobuf::Arena arena; + FlatExpressionEvaluatorState state = flat_expr.MakeEvaluatorState( + env->descriptor_pool.get(), env->MutableMessageFactory(), &arena); + ASSERT_OK_AND_ASSIGN( + cel::Value result, + flat_expr.EvaluateWithCallback(activation, /*embedder_context=*/nullptr, + EvaluationListener(), state)); + EXPECT_THAT(result, cel::test::BoolValueIs(true)); +} + +TEST_F(CelExpressionBuilderFlatImplComprehensionsTest, + NonNestedComprehensionsIterStackCount) { + std::unique_ptr parser_builder = cel::NewParserBuilder(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr parser, + parser_builder->Build()); + + ASSERT_OK_AND_ASSIGN( + std::unique_ptr source, + parser->PrepareSource( + "[1, 2].map(x, x + 1) + [3, 4].filter(y, y > 3) == [2, 3, 4]")); + ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, parser->Parse(*source)); + + cel::RuntimeOptions options = GetRuntimeOptions(); + std::shared_ptr env = + NewTestingRuntimeEnv(); + CelExpressionBuilderFlatImpl builder(env, options); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry()), IsOk()); + + ASSERT_OK_AND_ASSIGN( + FlatExpression flat_expr, + builder.flat_expr_builder().CreateExpressionImpl(std::move(ast), + /*issues=*/nullptr)); + EXPECT_EQ(flat_expr.iterator_stack_size(), 1); + + cel::Activation activation; + google::protobuf::Arena arena; + FlatExpressionEvaluatorState state = flat_expr.MakeEvaluatorState( + env->descriptor_pool.get(), env->MutableMessageFactory(), &arena); + ASSERT_OK_AND_ASSIGN( + cel::Value result, + flat_expr.EvaluateWithCallback(activation, /*embedder_context=*/nullptr, + EvaluationListener(), state)); + EXPECT_THAT(result, cel::test::BoolValueIs(true)); +} + } // namespace } // namespace google::api::expr::runtime 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 fa6c95a9b..a839d8b03 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", @@ -450,6 +451,7 @@ cc_test( ":cel_expression_flat_impl", ":container_access_step", ":evaluator_core", + ":expression_step_logic", "//base:builtins", "//base:data", "//common:ast", @@ -473,6 +475,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", @@ -512,7 +515,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", @@ -929,6 +934,7 @@ cc_library( deps = [ "//common:value", "@com_google_absl//absl/base:nullability", + "@com_google_absl//absl/container:inlined_vector", "@com_google_absl//absl/log:absl_check", ], ) 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/equality_steps_test.cc b/eval/eval/equality_steps_test.cc index 4ebfe10ef..50970ca44 100644 --- a/eval/eval/equality_steps_test.cc +++ b/eval/eval/equality_steps_test.cc @@ -81,7 +81,7 @@ TEST(IterativeTest, PartialAttrUnknown) { FlatExpressionEvaluatorState state( /*value_stack_size=*/5, - /*comprehension_slot_count=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); @@ -113,7 +113,7 @@ TEST(IterativeTest, PartialAttrUnknownDisabled) { FlatExpressionEvaluatorState state( /*value_stack_size=*/5, - /*comprehension_slot_count=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); @@ -183,7 +183,7 @@ TEST_P(EqualsTest, Iterative) { FlatExpressionEvaluatorState state( /*value_stack_size=*/5, - /*comprehension_slot_count=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); @@ -328,7 +328,7 @@ TEST_P(InTest, Iterative) { FlatExpressionEvaluatorState state( /*value_stack_size=*/5, - /*comprehension_slot_count=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index cbc47d136..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() { @@ -222,16 +226,16 @@ FlatExpressionEvaluatorState FlatExpression::MakeEvaluatorState( google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena) const { return FlatExpressionEvaluatorState(path_.size(), comprehension_slots_size_, - type_provider_, descriptor_pool, - message_factory, arena); + iterator_stack_size_, type_provider_, + descriptor_pool, message_factory, arena); } FlatExpressionEvaluatorState FlatExpression::MakeEvaluatorState( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nullable message_factory) const { return FlatExpressionEvaluatorState(path_.size(), comprehension_slots_size_, - type_provider_, descriptor_pool, - message_factory); + iterator_stack_size_, type_provider_, + descriptor_pool, message_factory); } absl::StatusOr FlatExpression::EvaluateWithCallback( @@ -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 e415487c3..112d1187f 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,22 @@ 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{ident_index, 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 +407,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 +431,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; @@ -450,14 +458,11 @@ class FlatExpressionEvaluatorState { public: FlatExpressionEvaluatorState( size_t value_stack_size, size_t comprehension_slot_count, - const cel::TypeProvider& type_provider, + size_t iterator_stack_size, const cel::TypeProvider& type_provider, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nullable message_factory = nullptr) : value_stack_(value_stack_size), - // We currently use comprehension_slot_count because it is less of an - // over estimate than value_stack_size. In future we should just - // calculate the correct capacity. - iterator_stack_(comprehension_slot_count), + iterator_stack_(iterator_stack_size), comprehension_slots_(comprehension_slot_count), type_provider_(type_provider), descriptor_pool_(descriptor_pool), @@ -465,15 +470,12 @@ class FlatExpressionEvaluatorState { FlatExpressionEvaluatorState( size_t value_stack_size, size_t comprehension_slot_count, - const cel::TypeProvider& type_provider, + size_t iterator_stack_size, const cel::TypeProvider& type_provider, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena) : value_stack_(value_stack_size), - // We currently use comprehension_slot_count because it is less of an - // over estimate than value_stack_size. In future we should just - // calculate the correct capacity. - iterator_stack_(comprehension_slot_count), + iterator_stack_(iterator_stack_size), comprehension_slots_(comprehension_slot_count), type_provider_(type_provider), descriptor_pool_(descriptor_pool), @@ -481,6 +483,7 @@ class FlatExpressionEvaluatorState { arena_(arena) {} void Reset(); + void MarkDirty() { dirty_ = true; } EvaluatorStack& value_stack() { return value_stack_; } @@ -520,6 +523,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 @@ -712,7 +716,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(), @@ -721,14 +726,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(), @@ -737,8 +746,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. @@ -799,6 +810,8 @@ class ExecutionFrame : public ExecutionFrameBase { return *iterator_stack_; } + const IdentifierTable& identifiers() const { return *identifiers_; } + bool enable_attribute_tracking() const { return attribute_tracking_enabled(); } @@ -836,6 +849,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; @@ -863,6 +881,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_; }; @@ -875,26 +894,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 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; @@ -930,21 +955,27 @@ 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_; } + size_t iterator_stack_size() const { return iterator_stack_size_; } + const cel::TypeProvider& type_provider() const { return type_provider_; } private: ExecutionPath path_; std::vector subexpressions_; size_t comprehension_slots_size_; + size_t iterator_stack_size_; const cel::TypeProvider& type_provider_; cel::RuntimeOptions options_; // 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 @@ -984,6 +1015,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() { @@ -994,9 +1033,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; @@ -1029,6 +1065,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: @@ -1202,7 +1239,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 c67dc8b58..44ad09659 100644 --- a/eval/eval/evaluator_core_test.cc +++ b/eval/eval/evaluator_core_test.cc @@ -84,7 +84,7 @@ TEST(EvaluatorCoreTest, ExecutionFrameNext) { cel::Activation activation; FlatExpressionEvaluatorState state( path.size(), - /*comprehension_slots_size=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); ExecutionFrame frame(path, activation, options, state); @@ -111,7 +111,7 @@ TEST(EvaluatorCoreTest, JumpOutOfRangeAborts) { cel::Activation activation; FlatExpressionEvaluatorState state( path.size(), - /*comprehension_slots_size=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); ExecutionFrame frame(path, activation, options, state); @@ -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..8199dbdd6 100644 --- a/eval/eval/ident_step.h +++ b/eval/eval/ident_step.h @@ -1,14 +1,27 @@ #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 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 { + size_t ident_index : 32; + size_t slot_index : 32; +}; + +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/iterator_stack.h b/eval/eval/iterator_stack.h index 0b0de14aa..ccb377dd3 100644 --- a/eval/eval/iterator_stack.h +++ b/eval/eval/iterator_stack.h @@ -20,6 +20,7 @@ #include #include "absl/base/nullability.h" +#include "absl/container/inlined_vector.h" #include "absl/log/absl_check.h" #include "common/value.h" @@ -92,7 +93,7 @@ class IteratorStack final { } private: - std::vector entries_; + absl::InlinedVector entries_; size_t max_size_; }; diff --git a/eval/eval/lazy_init_step_test.cc b/eval/eval/lazy_init_step_test.cc index 3b5282d21..d7211e597 100644 --- a/eval/eval/lazy_init_step_test.cc +++ b/eval/eval/lazy_init_step_test.cc @@ -46,7 +46,8 @@ class LazyInitStepTest : public testing::Test { public: LazyInitStepTest() : type_provider_(cel::internal::GetTestingDescriptorPool()), - evaluator_state_(kValueStack, kComprehensionSlotCount, type_provider_, + evaluator_state_(kValueStack, kComprehensionSlotCount, + /*iterator_stack_size=*/0, type_provider_, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena_) {} 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"); diff --git a/extensions/bindings_ext.cc b/extensions/bindings_ext.cc index 4823c077c..b9a80d493 100644 --- a/extensions/bindings_ext.cc +++ b/extensions/bindings_ext.cc @@ -100,4 +100,8 @@ CheckerLibrary BindingsCheckerLibrary(int version) { }}; } +ParserLibrary BindingsParserLibrary() { + return ParserLibrary{"cel.lib.ext.bindings", &ConfigureParser}; +} + } // namespace cel::extensions diff --git a/extensions/bindings_ext.h b/extensions/bindings_ext.h index 40b83a37f..5f54abf25 100644 --- a/extensions/bindings_ext.h +++ b/extensions/bindings_ext.h @@ -22,6 +22,7 @@ #include "parser/macro.h" #include "parser/macro_registry.h" #include "parser/options.h" +#include "parser/parser_interface.h" namespace cel::extensions { @@ -41,6 +42,9 @@ CompilerLibrary BindingsCompilerLibrary(int version = kBindingsVersionLatest); // Declarations for the bindings extension library. CheckerLibrary BindingsCheckerLibrary(int version = kBindingsVersionLatest); +// Macros for the bindings extension library. +ParserLibrary BindingsParserLibrary(); + } // namespace cel::extensions #endif // THIRD_PARTY_CEL_CPP_EXTENSIONS_BINDINGS_EXT_H_ diff --git a/testutil/BUILD b/testutil/BUILD index a8bc214c9..6574db5d5 100644 --- a/testutil/BUILD +++ b/testutil/BUILD @@ -42,6 +42,7 @@ cc_library( "//parser:macro", "//parser:macro_expr_factory", "//parser:macro_registry", + "//parser:parser_interface", "@com_google_absl//absl/base:no_destructor", "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/status", diff --git a/testutil/test_macros.cc b/testutil/test_macros.cc index 19e9a4844..30b94280d 100644 --- a/testutil/test_macros.cc +++ b/testutil/test_macros.cc @@ -15,19 +15,20 @@ #include "testutil/test_macros.h" #include +#include #include #include "absl/base/no_destructor.h" #include "absl/log/absl_check.h" #include "absl/status/status.h" #include "absl/strings/str_cat.h" -#include "absl/types/optional.h" #include "absl/types/span.h" #include "common/expr.h" #include "internal/status_macros.h" #include "parser/macro.h" #include "parser/macro_expr_factory.h" #include "parser/macro_registry.h" +#include "parser/parser_interface.h" namespace cel::test { @@ -170,4 +171,17 @@ absl::Status RegisterTestMacros(MacroRegistry& registry) { return absl::OkStatus(); } +ParserLibrary TestMacrosParserLibrary() { + return ParserLibrary{ + "cel.test.test_macros", + [](ParserBuilder& builder) -> absl::Status { + CEL_RETURN_IF_ERROR(builder.AddMacro(CelBlockMacro())); + CEL_RETURN_IF_ERROR(builder.AddMacro(CelIndexMacro())); + CEL_RETURN_IF_ERROR(builder.AddMacro(CelIterVarMacro())); + CEL_RETURN_IF_ERROR(builder.AddMacro(CelAccuVarMacro())); + return absl::OkStatus(); + }, + }; +} + } // namespace cel::test diff --git a/testutil/test_macros.h b/testutil/test_macros.h index cad897999..cc988ceb1 100644 --- a/testutil/test_macros.h +++ b/testutil/test_macros.h @@ -18,6 +18,7 @@ #include "absl/status/status.h" #include "parser/macro.h" #include "parser/macro_registry.h" +#include "parser/parser_interface.h" namespace cel::test { @@ -28,6 +29,8 @@ const Macro& CelAccuVarMacro(); absl::Status RegisterTestMacros(MacroRegistry& registry); +ParserLibrary TestMacrosParserLibrary(); + } // namespace cel::test #endif // THIRD_PARTY_CEL_CPP_TESTUTIL_TEST_MACROS_H_