From 610847962ae24b034e094179e4cd08a6f9e0aef0 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Thu, 9 Jul 2026 11:58:50 -0700 Subject: [PATCH] Generalize cleanup and jump steps for and/or. PiperOrigin-RevId: 945241913 --- eval/compiler/BUILD | 4 +- eval/compiler/flat_expr_builder.cc | 6 +- ...ilder_short_circuiting_conformance_test.cc | 134 +++++------------- eval/eval/jump_step.cc | 62 +++++--- eval/eval/jump_step.h | 15 +- eval/eval/logic_step.cc | 66 +++++---- 6 files changed, 130 insertions(+), 157 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..9cb9fbb01 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -2193,10 +2193,10 @@ void BinaryCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) { std::unique_ptr jump_step; switch (cond_) { case BinaryCond::kAnd: - jump_step = CreateCondJumpStep(false, true, {}, expr->id()); + jump_step = CreateCondJumpStep(false, {}, expr->id()); break; case BinaryCond::kOr: - jump_step = CreateCondJumpStep(true, true, {}, expr->id()); + jump_step = CreateCondJumpStep(true, {}, expr->id()); break; default: ABSL_UNREACHABLE(); @@ -2321,7 +2321,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/jump_step.cc b/eval/eval/jump_step.cc index a65789841..60c8c039e 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,16 @@ 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, int64_t expr_id) { + return std::make_unique(jump_condition, jump_offset, 1, + 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..27deba352 100644 --- a/eval/eval/jump_step.h +++ b/eval/eval/jump_step.h @@ -48,14 +48,19 @@ 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, 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..6d5c8bbb3 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)); @@ -453,12 +451,12 @@ 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); + return std::make_unique(OpType::kAnd, 2, expr_id); } // Factory method for "Or" Execution step absl::StatusOr> CreateOrStep(int64_t expr_id) { - return std::make_unique(OpType::kOr, expr_id); + return std::make_unique(OpType::kOr, 2, expr_id); } // Factory method for recursive logical not "!" Execution step