From b623e12e0533119f5da887d56c79545939b00321 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 8 Jul 2026 14:20:29 -0700 Subject: [PATCH] Simplify program layout for variadic and/or. PiperOrigin-RevId: 944697041 --- eval/compiler/BUILD | 4 +- eval/compiler/flat_expr_builder.cc | 207 +++++++----------- ...ilder_short_circuiting_conformance_test.cc | 134 ++++-------- eval/eval/BUILD | 1 - eval/eval/jump_step.cc | 63 ++++-- eval/eval/jump_step.h | 16 +- eval/eval/logic_step.cc | 72 +++--- eval/eval/logic_step.h | 6 +- eval/eval/logic_step_test.cc | 4 +- 9 files changed, 223 insertions(+), 284 deletions(-) diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index f7300cb58..ea4dd46b3 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -501,7 +501,6 @@ cc_test( ], deps = [ ":cel_expression_builder_flat_impl", - "//base:builtins", "//eval/public:activation", "//eval/public:cel_attribute", "//eval/public:cel_expression", @@ -509,8 +508,11 @@ cc_test( "//eval/public:unknown_attribute_set", "//eval/public:unknown_set", "//internal:testing", + "//parser", + "//parser:options", "//runtime:runtime_options", "//runtime/internal:runtime_env_testing", + "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_protobuf//:protobuf", diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index aa9a8858c..fa3c5d4ec 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -198,14 +198,7 @@ class CondVisitor { virtual void PostVisitTarget(const cel::Expr* expr) {} }; -enum class BinaryCond { - kAnd = 0, - kOr, - kOptionalOr, - kOptionalOrValue, -}; - -// Visitor managing the "&&" and "||" operatiions. +// Visitor managing the "&&" and "||" (boolean logic) operations. // Implements short-circuiting if enabled. // // With short-circuiting enabled, generates a program like: @@ -218,20 +211,41 @@ enum class BinaryCond { // | i + 3 | BooleanOperator | Op(arg1, arg2) | // | i + 4 | | arg1 | Op(arg1, arg2) | // +-------------+------------------------+------------------------+ -class BinaryCondVisitor : public CondVisitor { +class LogicalCondVisitor : public CondVisitor { public: - explicit BinaryCondVisitor(FlatExprVisitor* visitor, BinaryCond cond, - bool short_circuiting) - : visitor_(visitor), cond_(cond), short_circuiting_(short_circuiting) {} + explicit LogicalCondVisitor(FlatExprVisitor* visitor, bool is_or, + bool short_circuiting) + : visitor_(visitor), is_or_(is_or), short_circuiting_(short_circuiting) {} void PreVisit(const cel::Expr* expr) override; void PostVisitArg(int arg_num, const cel::Expr* expr) override; void PostVisit(const cel::Expr* expr) override; + + private: + FlatExprVisitor* visitor_; + const bool is_or_; + std::vector jump_steps_; + bool short_circuiting_; +}; + +// Visitor managing optional "or" and "orValue" operations. +// Implements short-circuiting if enabled. +class OptionalOrCondVisitor : public CondVisitor { + public: + explicit OptionalOrCondVisitor(FlatExprVisitor* visitor, bool is_or_value, + bool short_circuiting) + : visitor_(visitor), + is_or_value_(is_or_value), + short_circuiting_(short_circuiting) {} + + void PreVisit(const cel::Expr* expr) override; + void PostVisitArg(int arg_num, const cel::Expr* expr) override {} void PostVisitTarget(const cel::Expr* expr) override; + void PostVisit(const cel::Expr* expr) override; private: FlatExprVisitor* visitor_; - const BinaryCond cond_; + const bool is_or_value_; std::vector jump_steps_; bool short_circuiting_; }; @@ -997,11 +1011,11 @@ class FlatExprVisitor : public cel::AstVisitor { std::unique_ptr cond_visitor; if (call_expr.function() == cel::builtin::kAnd) { - cond_visitor = std::make_unique( - this, BinaryCond::kAnd, options_.short_circuiting); + cond_visitor = std::make_unique( + this, /*is_or=*/false, options_.short_circuiting); } else if (call_expr.function() == cel::builtin::kOr) { - cond_visitor = std::make_unique( - this, BinaryCond::kOr, options_.short_circuiting); + cond_visitor = std::make_unique( + this, /*is_or=*/true, options_.short_circuiting); } else if (call_expr.function() == cel::builtin::kTernary) { if (options_.short_circuiting) { cond_visitor = std::make_unique(this); @@ -1011,13 +1025,13 @@ class FlatExprVisitor : public cel::AstVisitor { } else if (enable_optional_types_ && call_expr.function() == kOptionalOrFn && call_expr.has_target() && call_expr.args().size() == 1) { - cond_visitor = std::make_unique( - this, BinaryCond::kOptionalOr, options_.short_circuiting); + cond_visitor = std::make_unique( + this, /*is_or_value=*/false, options_.short_circuiting); } else if (enable_optional_types_ && call_expr.function() == kOptionalOrValueFn && call_expr.has_target() && call_expr.args().size() == 1) { - cond_visitor = std::make_unique( - this, BinaryCond::kOptionalOrValue, options_.short_circuiting); + cond_visitor = std::make_unique( + this, /*is_or_value=*/true, options_.short_circuiting); } else if (IsBlock(&call_expr)) { // cel.@block if (block_.has_value()) { @@ -2147,91 +2161,67 @@ FlatExprVisitor::HandleHeterogeneousEqualityIn(const cel::Expr& expr, return CallHandlerResult::kIntercepted; } -void BinaryCondVisitor::PreVisit(const cel::Expr* expr) { - switch (cond_) { - case BinaryCond::kAnd: - ABSL_FALLTHROUGH_INTENDED; - case BinaryCond::kOr: - visitor_->ValidateOrError( - !expr->call_expr().has_target() && - expr->call_expr().args().size() >= 2, - "Invalid argument count for a binary function call."); - break; - case BinaryCond::kOptionalOr: - ABSL_FALLTHROUGH_INTENDED; - case BinaryCond::kOptionalOrValue: - visitor_->ValidateOrError(expr->call_expr().has_target() && - expr->call_expr().args().size() == 1, - "Invalid argument count for or/orValue call."); - break; - } +void LogicalCondVisitor::PreVisit(const cel::Expr* expr) { + visitor_->ValidateOrError( + !expr->call_expr().has_target() && expr->call_expr().args().size() >= 2, + "Invalid argument count for a binary function call."); } -void BinaryCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { +void LogicalCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { if (visitor_->PlanRecursiveProgram()) { return; } const int last_arg_index = expr->call_expr().args().size() - 1; - if (cond_ == BinaryCond::kAnd || cond_ == BinaryCond::kOr) { - if (arg_num > 0) { - switch (cond_) { - case BinaryCond::kAnd: - visitor_->AddStep(CreateAndStep(expr->id())); - break; - case BinaryCond::kOr: - visitor_->AddStep(CreateOrStep(expr->id())); - break; - default: - break; - } - if (short_circuiting_ && !jump_steps_.empty()) { + const size_t num_args = expr->call_expr().args().size(); + if (arg_num == last_arg_index) { + if (is_or_) { + visitor_->AddStep(CreateOrStep(num_args, expr->id())); + } else { + visitor_->AddStep(CreateAndStep(num_args, expr->id())); + } + if (short_circuiting_ && !jump_steps_.empty()) { + for (auto& jump : jump_steps_) { visitor_->SetProgressStatusIfError( - jump_steps_.back().set_target(visitor_->GetCurrentIndex())); + jump.set_target(visitor_->GetCurrentIndex())); } } - if (short_circuiting_ && arg_num < last_arg_index) { - std::unique_ptr jump_step; - switch (cond_) { - case BinaryCond::kAnd: - jump_step = CreateCondJumpStep(false, true, {}, expr->id()); - break; - case BinaryCond::kOr: - jump_step = CreateCondJumpStep(true, true, {}, expr->id()); - break; - default: - ABSL_UNREACHABLE(); - } - ProgramStepIndex index = visitor_->GetCurrentIndex(); - if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step)); - jump_step_ptr) { - jump_steps_.push_back(Jump(index, jump_step_ptr)); - } + } + if (short_circuiting_ && arg_num < last_arg_index) { + std::unique_ptr jump_step = + is_or_ ? CreateCondJumpStep(true, {}, arg_num + 1, expr->id()) + : CreateCondJumpStep(false, {}, arg_num + 1, expr->id()); + ProgramStepIndex index = visitor_->GetCurrentIndex(); + if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step)); + jump_step_ptr) { + jump_steps_.push_back(Jump(index, jump_step_ptr)); } } } -void BinaryCondVisitor::PostVisitTarget(const cel::Expr* expr) { +void LogicalCondVisitor::PostVisit(const cel::Expr* expr) { + if (visitor_->PlanRecursiveProgram()) { + visitor_->MakeShortcircuitRecursive(expr, is_or_); + } +} + +void OptionalOrCondVisitor::PreVisit(const cel::Expr* expr) { + visitor_->ValidateOrError( + expr->call_expr().has_target() && expr->call_expr().args().size() == 1, + "Invalid argument count for or/orValue call."); +} + +void OptionalOrCondVisitor::PostVisitTarget(const cel::Expr* expr) { if (visitor_->PlanRecursiveProgram()) { return; } - if (short_circuiting_ && (cond_ == BinaryCond::kOptionalOr || - cond_ == BinaryCond::kOptionalOrValue)) { + if (short_circuiting_) { // If first branch evaluation result is enough to determine output, // jump over the second branch and provide result of the first argument as // final output. // Retain a pointer to the jump step so we can update the target after // planning the second argument. - std::unique_ptr jump_step; - switch (cond_) { - case BinaryCond::kOptionalOr: - jump_step = CreateOptionalHasValueJumpStep(false, expr->id()); - break; - case BinaryCond::kOptionalOrValue: - jump_step = CreateOptionalHasValueJumpStep(true, expr->id()); - break; - default: - ABSL_UNREACHABLE(); - } + std::unique_ptr jump_step = + CreateOptionalHasValueJumpStep(is_or_value_, expr->id()); ProgramStepIndex index = visitor_->GetCurrentIndex(); if (JumpStepBase* jump_step_ptr = visitor_->AddStep(std::move(jump_step)); jump_step_ptr) { @@ -2240,48 +2230,17 @@ void BinaryCondVisitor::PostVisitTarget(const cel::Expr* expr) { } } -void BinaryCondVisitor::PostVisit(const cel::Expr* expr) { +void OptionalOrCondVisitor::PostVisit(const cel::Expr* expr) { if (visitor_->PlanRecursiveProgram()) { - switch (cond_) { - case BinaryCond::kAnd: - visitor_->MakeShortcircuitRecursive(expr, /*is_or=*/false); - break; - case BinaryCond::kOr: - visitor_->MakeShortcircuitRecursive(expr, /*is_or=*/true); - break; - case BinaryCond::kOptionalOr: - visitor_->MakeOptionalShortcircuit(expr, - /*is_or_value=*/false); - break; - case BinaryCond::kOptionalOrValue: - visitor_->MakeOptionalShortcircuit(expr, - /*is_or_value=*/true); - break; - default: - ABSL_UNREACHABLE(); - } + visitor_->MakeOptionalShortcircuit(expr, is_or_value_); return; } - if (cond_ == BinaryCond::kOptionalOr || - cond_ == BinaryCond::kOptionalOrValue) { - switch (cond_) { - case BinaryCond::kOptionalOr: - visitor_->AddStep( - CreateOptionalOrStep(/*is_or_value=*/false, expr->id())); - break; - case BinaryCond::kOptionalOrValue: - visitor_->AddStep( - CreateOptionalOrStep(/*is_or_value=*/true, expr->id())); - break; - default: - ABSL_UNREACHABLE(); - } - if (short_circuiting_) { - for (auto& jump : jump_steps_) { - visitor_->SetProgressStatusIfError( - jump.set_target(visitor_->GetCurrentIndex())); - } + visitor_->AddStep(CreateOptionalOrStep(is_or_value_, expr->id())); + if (short_circuiting_) { + for (auto& jump : jump_steps_) { + visitor_->SetProgressStatusIfError( + jump.set_target(visitor_->GetCurrentIndex())); } } } @@ -2321,7 +2280,7 @@ void TernaryCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { // Value is to be removed from the stack. ProgramStepIndex cond_jump_pos = visitor_->GetCurrentIndex(); auto* jump_to_second = - visitor_->AddStep(CreateCondJumpStep(false, false, {}, expr->id())); + visitor_->AddStep(CreateTernaryCondJumpStep({}, expr->id())); if (jump_to_second) { jump_to_second_ = Jump(cond_jump_pos, static_cast(jump_to_second)); diff --git a/eval/compiler/flat_expr_builder_short_circuiting_conformance_test.cc b/eval/compiler/flat_expr_builder_short_circuiting_conformance_test.cc index afe7c5f9f..c00b66824 100644 --- a/eval/compiler/flat_expr_builder_short_circuiting_conformance_test.cc +++ b/eval/compiler/flat_expr_builder_short_circuiting_conformance_test.cc @@ -1,11 +1,13 @@ // A collection of tests that confirm that short-circuit and non-short-circuit // produce expressions with the same outputs. #include +#include +#include +#include "absl/log/absl_check.h" #include "absl/status/status.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" -#include "absl/strings/substitute.h" -#include "base/builtins.h" #include "eval/compiler/cel_expression_builder_flat_impl.h" #include "eval/public/activation.h" #include "eval/public/cel_attribute.h" @@ -14,10 +16,11 @@ #include "eval/public/unknown_attribute_set.h" #include "eval/public/unknown_set.h" #include "internal/testing.h" +#include "parser/options.h" +#include "parser/parser.h" #include "runtime/internal/runtime_env_testing.h" #include "runtime/runtime_options.h" #include "google/protobuf/arena.h" -#include "google/protobuf/text_format.h" namespace google::api::expr::runtime { @@ -25,64 +28,10 @@ namespace { using ::cel::runtime_internal::NewTestingRuntimeEnv; using ::cel::expr::Expr; +using ::google::api::expr::parser::Parse; using ::testing::Eq; using ::testing::SizeIs; -constexpr char kTwoLogicalOp[] = R"cel( -id: 1 -call_expr { - function: "$0" - args { - id: 2 - ident_expr { - name: "var1", - } - } - args { - id: 3 - call_expr { - function: "$0" - args { - id: 4 - ident_expr { - name: "var2" - } - } - args { - id: 5 - ident_expr { - name: "var3" - } - } - } - } -} -)cel"; - -constexpr char kTernaryExpr[] = R"cel( -id: 1 -call_expr { - function: "_?_:_" - args { - id: 2 - ident_expr { - name: "cond" - } - } - args { - id: 3 - ident_expr { - name: "arg1" - } - } - args { - id: 4 - ident_expr { - name: "arg2" - } - } -})cel"; - void BuildAndEval(CelExpressionBuilder* builder, const Expr& expr, const Activation& activation, google::protobuf::Arena* arena, CelValue* result) { @@ -95,12 +44,16 @@ void BuildAndEval(CelExpressionBuilder* builder, const Expr& expr, *result = *value; } -class ShortCircuitingTest : public testing::TestWithParam { +class ShortCircuitingTest + : public testing::TestWithParam> { public: + bool short_circuiting() const { return std::get<0>(GetParam()); } + bool enable_variadic() const { return std::get<1>(GetParam()); } + std::unique_ptr GetBuilder( bool enable_unknowns = false) { cel::RuntimeOptions options; - options.short_circuiting = GetParam(); + options.short_circuiting = short_circuiting(); if (enable_unknowns) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeAndFunction; @@ -109,14 +62,20 @@ class ShortCircuitingTest : public testing::TestWithParam { NewTestingRuntimeEnv(), options); return result; } + + Expr ParseExpr(absl::string_view expression) { + cel::ParserOptions options; + options.enable_variadic_logical_operators = enable_variadic(); + auto parsed_expr = Parse(expression, "", options); + ABSL_CHECK_OK(parsed_expr.status()); + return parsed_expr->expr(); + } }; TEST_P(ShortCircuitingTest, BasicAnd) { - Expr expr; + Expr expr = ParseExpr("var1 && var2 && var3"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( - absl::Substitute(kTwoLogicalOp, ::cel::builtin::kAnd), &expr)); auto builder = GetBuilder(); activation.InsertValue("var1", CelValue::CreateBool(true)); @@ -140,11 +99,9 @@ TEST_P(ShortCircuitingTest, BasicAnd) { } TEST_P(ShortCircuitingTest, BasicOr) { - Expr expr; + Expr expr = ParseExpr("var1 || var2 || var3"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( - absl::Substitute(kTwoLogicalOp, ::cel::builtin::kOr), &expr)); auto builder = GetBuilder(); activation.InsertValue("var1", CelValue::CreateBool(false)); @@ -168,11 +125,9 @@ TEST_P(ShortCircuitingTest, BasicOr) { } TEST_P(ShortCircuitingTest, ErrorAnd) { - Expr expr; + Expr expr = ParseExpr("var1 && var2 && var3"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( - absl::Substitute(kTwoLogicalOp, ::cel::builtin::kAnd), &expr)); auto builder = GetBuilder(); absl::Status error = absl::InternalError("error"); @@ -198,11 +153,9 @@ TEST_P(ShortCircuitingTest, ErrorAnd) { } TEST_P(ShortCircuitingTest, ErrorOr) { - Expr expr; + Expr expr = ParseExpr("var1 || var2 || var3"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( - absl::Substitute(kTwoLogicalOp, ::cel::builtin::kOr), &expr)); auto builder = GetBuilder(); absl::Status error = absl::InternalError("error"); @@ -228,11 +181,9 @@ TEST_P(ShortCircuitingTest, ErrorOr) { } TEST_P(ShortCircuitingTest, UnknownAnd) { - Expr expr; + Expr expr = ParseExpr("var1 && var2 && var3"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( - absl::Substitute(kTwoLogicalOp, ::cel::builtin::kAnd), &expr)); auto builder = GetBuilder(/* enable_unknowns=*/true); absl::Status error = absl::InternalError("error"); @@ -260,11 +211,9 @@ TEST_P(ShortCircuitingTest, UnknownAnd) { } TEST_P(ShortCircuitingTest, UnknownOr) { - Expr expr; + Expr expr = ParseExpr("var1 || var2 || var3"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( - absl::Substitute(kTwoLogicalOp, ::cel::builtin::kOr), &expr)); auto builder = GetBuilder(/* enable_unknowns=*/true); absl::Status error = absl::InternalError("error"); @@ -292,10 +241,9 @@ TEST_P(ShortCircuitingTest, UnknownOr) { } TEST_P(ShortCircuitingTest, BasicTernary) { - Expr expr; + Expr expr = ParseExpr("cond ? arg1 : arg2"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kTernaryExpr, &expr)); auto builder = GetBuilder(); activation.InsertValue("cond", CelValue::CreateBool(true)); @@ -319,10 +267,9 @@ TEST_P(ShortCircuitingTest, BasicTernary) { } TEST_P(ShortCircuitingTest, TernaryErrorHandling) { - Expr expr; + Expr expr = ParseExpr("cond ? arg1 : arg2"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kTernaryExpr, &expr)); auto builder = GetBuilder(); absl::Status error1 = absl::InternalError("error1"); @@ -349,10 +296,9 @@ TEST_P(ShortCircuitingTest, TernaryErrorHandling) { } TEST_P(ShortCircuitingTest, TernaryUnknownCondHandling) { - Expr expr; + Expr expr = ParseExpr("cond ? arg1 : arg2"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kTernaryExpr, &expr)); auto builder = GetBuilder(/*enable_unknowns=*/true); absl::Status error = absl::InternalError("error1"); @@ -386,10 +332,9 @@ TEST_P(ShortCircuitingTest, TernaryUnknownCondHandling) { } TEST_P(ShortCircuitingTest, TernaryUnknownArgsHandling) { - Expr expr; + Expr expr = ParseExpr("cond ? arg1 : arg2"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kTernaryExpr, &expr)); auto builder = GetBuilder(/*enable_unknowns=*/true); absl::Status error = absl::InternalError("error1"); @@ -421,10 +366,9 @@ TEST_P(ShortCircuitingTest, TernaryUnknownArgsHandling) { } TEST_P(ShortCircuitingTest, TernaryUnknownAndErrorHandling) { - Expr expr; + Expr expr = ParseExpr("cond ? arg1 : arg2"); Activation activation; google::protobuf::Arena arena; - ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kTernaryExpr, &expr)); auto builder = GetBuilder(/*enable_unknowns=*/true); absl::Status error = absl::InternalError("error1"); @@ -457,16 +401,16 @@ TEST_P(ShortCircuitingTest, TernaryUnknownAndErrorHandling) { EXPECT_EQ(attrs.begin()->variable_name(), "cond"); } -const char* TestName(testing::TestParamInfo info) { - if (info.param) { - return "short_circuit_enabled"; - } else { - return "short_circuit_disabled"; - } +std::string TestName(testing::TestParamInfo> info) { + return absl::StrCat( + std::get<0>(info.param) ? "short_circuit_enabled" + : "short_circuit_disabled", + "_", std::get<1>(info.param) ? "variadic_enabled" : "variadic_disabled"); } INSTANTIATE_TEST_SUITE_P(Test, ShortCircuitingTest, - testing::Values(false, true), &TestName); + testing::Combine(testing::Bool(), testing::Bool()), + &TestName); } // namespace diff --git a/eval/eval/BUILD b/eval/eval/BUILD index 44c7ded79..c0f544405 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -418,7 +418,6 @@ cc_library( "//common:value", "//eval/internal:errors", "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/types:optional", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", ], diff --git a/eval/eval/jump_step.cc b/eval/eval/jump_step.cc index a65789841..243a02e8a 100644 --- a/eval/eval/jump_step.cc +++ b/eval/eval/jump_step.cc @@ -14,14 +14,15 @@ #include "eval/eval/jump_step.h" +#include #include #include #include #include "absl/status/status.h" -#include "absl/status/statusor.h" #include "absl/types/optional.h" #include "common/value.h" +#include "eval/eval/evaluator_core.h" #include "eval/internal/errors.h" namespace google::api::expr::runtime { @@ -47,28 +48,24 @@ class JumpStep : public JumpStepBase { class CondJumpStep : public JumpStepBase { public: - // Constructs FunctionStep that uses overloads specified. - CondJumpStep(bool jump_condition, bool leave_on_stack, - absl::optional jump_offset, int64_t expr_id) + CondJumpStep(bool jump_condition, absl::optional jump_offset, + size_t stack_size, int64_t expr_id) : JumpStepBase(jump_offset, expr_id), jump_condition_(jump_condition), - leave_on_stack_(leave_on_stack) {} + stack_size_(stack_size) {} absl::Status Evaluate(ExecutionFrame* frame) const override { // Peek the top value - if (!frame->value_stack().HasEnough(1)) { + if (!frame->value_stack().HasEnough(stack_size_)) { return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); } const auto& value = frame->value_stack().Peek(); - const auto should_jump = value.Is() && - jump_condition_ == value.GetBool().NativeValue(); - - if (!leave_on_stack_) { - frame->value_stack().Pop(1); - } + const auto should_jump = + value.IsBool() && jump_condition_ == value.GetBool().NativeValue(); if (should_jump) { + frame->value_stack().SwapAndPop(stack_size_, stack_size_ - 1); return Jump(frame); } @@ -77,7 +74,31 @@ class CondJumpStep : public JumpStepBase { private: const bool jump_condition_; - const bool leave_on_stack_; + const size_t stack_size_; +}; + +class TernaryCondJumpStep : public JumpStepBase { + public: + TernaryCondJumpStep(absl::optional jump_offset, int64_t expr_id) + : JumpStepBase(jump_offset, expr_id) {} + + absl::Status Evaluate(ExecutionFrame* frame) const override { + // Peek the top value + if (!frame->value_stack().HasEnough(1)) { + return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); + } + + const auto& value = frame->value_stack().Peek(); + const auto should_jump = value.IsBool() && !value.GetBool().NativeValue(); + + frame->value_stack().Pop(1); + + if (should_jump) { + return Jump(frame); + } + + return absl::OkStatus(); + } }; class BoolCheckJumpStep : public JumpStepBase { @@ -121,13 +142,17 @@ class BoolCheckJumpStep : public JumpStepBase { } // namespace // Factory method for Conditional Jump step. -// Conditional Jump requires a boolean value to sit on the stack. -// It is compared to jump_condition, and if matched, jump is performed. std::unique_ptr CreateCondJumpStep( - bool jump_condition, bool leave_on_stack, absl::optional jump_offset, - int64_t expr_id) { - return std::make_unique(jump_condition, leave_on_stack, - jump_offset, expr_id); + bool jump_condition, absl::optional jump_offset, + size_t expected_stack_size, int64_t expr_id) { + return std::make_unique(jump_condition, jump_offset, + expected_stack_size, expr_id); +} + +// Factory method for Ternary Conditional Jump step. +std::unique_ptr CreateTernaryCondJumpStep( + absl::optional jump_offset, int64_t expr_id) { + return std::make_unique(jump_offset, expr_id); } // Factory method for Jump step. diff --git a/eval/eval/jump_step.h b/eval/eval/jump_step.h index 55147da5f..d8555ae10 100644 --- a/eval/eval/jump_step.h +++ b/eval/eval/jump_step.h @@ -48,14 +48,20 @@ class JumpStepBase : public ExpressionStepBase { std::unique_ptr CreateJumpStep(absl::optional jump_offset, int64_t expr_id); -// Factory method for Conditional Jump step. +// Factory method for Conditional Jump step (used for and/or shortcircuiting). // Conditional Jump requires a boolean value to sit on the stack. // It is compared to jump_condition, and if matched, jump is performed. -// leave on stack indicates whether value should be kept on top of the stack or -// removed. +// The boolean value is left on top of the stack. std::unique_ptr CreateCondJumpStep( - bool jump_condition, bool leave_on_stack, absl::optional jump_offset, - int64_t expr_id); + bool jump_condition, absl::optional jump_offset, + size_t expected_stack_size, int64_t expr_id); + +// Factory method for Ternary Conditional Jump step. +// Requires a boolean condition value on top of the stack. +// If the boolean value is false, a jump is performed to the second branch. +// The condition value is popped from the stack before jumping or continuing. +std::unique_ptr CreateTernaryCondJumpStep( + absl::optional jump_offset, int64_t expr_id); // Factory method for ErrorJump step. // This step performs a Jump when an Error is on the top of the stack. diff --git a/eval/eval/logic_step.cc b/eval/eval/logic_step.cc index f844d8c05..9e65db8f0 100644 --- a/eval/eval/logic_step.cc +++ b/eval/eval/logic_step.cc @@ -3,6 +3,7 @@ #include #include #include +#include #include #include "absl/status/status.h" @@ -188,8 +189,8 @@ absl::Status DirectLogicStep::Evaluate(ExecutionFrameBase& frame, Value& result, class LogicalOpStep : public ExpressionStepBase { public: // Constructs FunctionStep that uses overloads specified. - LogicalOpStep(OpType op_type, int64_t expr_id) - : ExpressionStepBase(expr_id), op_type_(op_type) { + LogicalOpStep(OpType op_type, size_t count, int64_t expr_id) + : ExpressionStepBase(expr_id), op_type_(op_type), count_(count) { shortcircuit_ = (op_type_ == OpType::kOr); } @@ -198,28 +199,25 @@ class LogicalOpStep : public ExpressionStepBase { private: void Calculate(ExecutionFrame* frame, absl::Span args, Value& result) const { - bool bool_args[2]; - bool has_bool_args[2]; + std::optional error_pos; for (size_t i = 0; i < args.size(); i++) { - has_bool_args[i] = args[i]->Is(); - if (has_bool_args[i]) { - bool_args[i] = args[i].GetBool().NativeValue(); - if (bool_args[i] == shortcircuit_) { - result = BoolValue{bool_args[i]}; - return; - } - } - } - - if (has_bool_args[0] && has_bool_args[1]) { - switch (op_type_) { - case OpType::kAnd: - result = BoolValue{bool_args[0] && bool_args[1]}; - return; - case OpType::kOr: - result = BoolValue{bool_args[0] || bool_args[1]}; - return; + const Value& arg = args[i]; + switch (arg.kind()) { + case ValueKind::kBool: + if (arg.GetBool() == shortcircuit_) { + result = arg; + return; + } + break; + case ValueKind::kUnknown: + break; + case ValueKind::kError: + default: + if (!error_pos.has_value()) { + error_pos = i; + } + break; } } @@ -237,31 +235,31 @@ class LogicalOpStep : public ExpressionStepBase { } } - if (args[0]->Is()) { - result = args[0]; - return; - } else if (args[1]->Is()) { - result = args[1]; + if (!error_pos.has_value()) { + result = cel::BoolValue(!shortcircuit_); return; } - // Fallback. - result = cel::ErrorValue(CreateNoMatchingOverloadError( - (op_type_ == OpType::kOr) ? cel::builtin::kOr : cel::builtin::kAnd)); + result = args[error_pos.value()]; + if (!result.IsError()) { + result = cel::ErrorValue(CreateNoMatchingOverloadError( + (op_type_ == OpType::kOr) ? cel::builtin::kOr : cel::builtin::kAnd)); + } } const OpType op_type_; + size_t count_; bool shortcircuit_; }; absl::Status LogicalOpStep::Evaluate(ExecutionFrame* frame) const { // Must have 2 or more values on the stack. - if (!frame->value_stack().HasEnough(2)) { + if (!frame->value_stack().HasEnough(count_)) { return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); } // Create Span object that contains input arguments to the function. - auto args = frame->value_stack().GetSpan(2); + auto args = frame->value_stack().GetSpan(count_); Value result; Calculate(frame, args, result); frame->value_stack().PopAndPush(args.size(), std::move(result)); @@ -452,13 +450,15 @@ std::unique_ptr CreateDirectOrStep( } // Factory method for "And" Execution step -absl::StatusOr> CreateAndStep(int64_t expr_id) { - return std::make_unique(OpType::kAnd, expr_id); +absl::StatusOr> CreateAndStep(size_t num_args, + int64_t expr_id) { + return std::make_unique(OpType::kAnd, num_args, expr_id); } // Factory method for "Or" Execution step -absl::StatusOr> CreateOrStep(int64_t expr_id) { - return std::make_unique(OpType::kOr, expr_id); +absl::StatusOr> CreateOrStep(size_t num_args, + int64_t expr_id) { + return std::make_unique(OpType::kOr, num_args, expr_id); } // Factory method for recursive logical not "!" Execution step diff --git a/eval/eval/logic_step.h b/eval/eval/logic_step.h index d75ed3715..4f5be2615 100644 --- a/eval/eval/logic_step.h +++ b/eval/eval/logic_step.h @@ -23,10 +23,12 @@ std::unique_ptr CreateDirectOrStep( bool shortcircuiting); // Factory method for "And" Execution step -absl::StatusOr> CreateAndStep(int64_t expr_id); +absl::StatusOr> CreateAndStep(size_t num_args, + int64_t expr_id); // Factory method for "Or" Execution step -absl::StatusOr> CreateOrStep(int64_t expr_id); +absl::StatusOr> CreateOrStep(size_t num_args, + int64_t expr_id); // Factory method for recursive logical not "!" Execution step std::unique_ptr CreateDirectNotStep( diff --git a/eval/eval/logic_step_test.cc b/eval/eval/logic_step_test.cc index 17ca8ba0d..2bdcbc8ea 100644 --- a/eval/eval/logic_step_test.cc +++ b/eval/eval/logic_step_test.cc @@ -73,7 +73,9 @@ class LogicStepTest : public testing::TestWithParam { CEL_ASSIGN_OR_RETURN(step, CreateIdentStep("name1", /*expr_id=*/-1)); path.push_back(std::move(step)); - CEL_ASSIGN_OR_RETURN(step, (is_or) ? CreateOrStep(2) : CreateAndStep(2)); + CEL_ASSIGN_OR_RETURN( + step, (is_or) ? CreateOrStep(/*num_args=*/2, /*expr_id=*/2) + : CreateAndStep(/*num_args=*/2, /*expr_id=*/2)); path.push_back(std::move(step)); auto dummy_expr = std::make_unique();