Skip to content

Commit 6108479

Browse files
jnthntatumcopybara-github
jnthntatum
authored and
copybara-github
committed
Generalize cleanup and jump steps for and/or.
PiperOrigin-RevId: 945241913
1 parent 753fb7e commit 6108479

6 files changed

Lines changed: 130 additions & 157 deletions

File tree

‎eval/compiler/BUILD‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -501,16 +501,18 @@ cc_test(
501501
],
502502
deps = [
503503
":cel_expression_builder_flat_impl",
504-
"//base:builtins",
505504
"//eval/public:activation",
506505
"//eval/public:cel_attribute",
507506
"//eval/public:cel_expression",
508507
"//eval/public:cel_value",
509508
"//eval/public:unknown_attribute_set",
510509
"//eval/public:unknown_set",
511510
"//internal:testing",
511+
"//parser",
512+
"//parser:options",
512513
"//runtime:runtime_options",
513514
"//runtime/internal:runtime_env_testing",
515+
"@com_google_absl//absl/log:absl_check",
514516
"@com_google_absl//absl/status",
515517
"@com_google_absl//absl/strings",
516518
"@com_google_protobuf//:protobuf",

‎eval/compiler/flat_expr_builder.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2193,10 +2193,10 @@ void BinaryCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) {
21932193
std::unique_ptr<JumpStepBase> jump_step;
21942194
switch (cond_) {
21952195
case BinaryCond::kAnd:
2196-
jump_step = CreateCondJumpStep(false, true, {}, expr->id());
2196+
jump_step = CreateCondJumpStep(false, {}, expr->id());
21972197
break;
21982198
case BinaryCond::kOr:
2199-
jump_step = CreateCondJumpStep(true, true, {}, expr->id());
2199+
jump_step = CreateCondJumpStep(true, {}, expr->id());
22002200
break;
22012201
default:
22022202
ABSL_UNREACHABLE();
@@ -2321,7 +2321,7 @@ void TernaryCondVisitor::PostVisitArg(int arg_num, const cel::Expr* expr) {
23212321
// Value is to be removed from the stack.
23222322
ProgramStepIndex cond_jump_pos = visitor_->GetCurrentIndex();
23232323
auto* jump_to_second =
2324-
visitor_->AddStep(CreateCondJumpStep(false, false, {}, expr->id()));
2324+
visitor_->AddStep(CreateTernaryCondJumpStep({}, expr->id()));
23252325
if (jump_to_second) {
23262326
jump_to_second_ =
23272327
Jump(cond_jump_pos, static_cast<JumpStepBase*>(jump_to_second));

‎eval/compiler/flat_expr_builder_short_circuiting_conformance_test.cc‎

Lines changed: 39 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
// A collection of tests that confirm that short-circuit and non-short-circuit
22
// produce expressions with the same outputs.
33
#include <memory>
4+
#include <string>
5+
#include <tuple>
46

7+
#include "absl/log/absl_check.h"
58
#include "absl/status/status.h"
9+
#include "absl/strings/str_cat.h"
610
#include "absl/strings/string_view.h"
7-
#include "absl/strings/substitute.h"
8-
#include "base/builtins.h"
911
#include "eval/compiler/cel_expression_builder_flat_impl.h"
1012
#include "eval/public/activation.h"
1113
#include "eval/public/cel_attribute.h"
@@ -14,75 +16,22 @@
1416
#include "eval/public/unknown_attribute_set.h"
1517
#include "eval/public/unknown_set.h"
1618
#include "internal/testing.h"
19+
#include "parser/options.h"
20+
#include "parser/parser.h"
1721
#include "runtime/internal/runtime_env_testing.h"
1822
#include "runtime/runtime_options.h"
1923
#include "google/protobuf/arena.h"
20-
#include "google/protobuf/text_format.h"
2124

2225
namespace google::api::expr::runtime {
2326

2427
namespace {
2528

2629
using ::cel::runtime_internal::NewTestingRuntimeEnv;
2730
using ::cel::expr::Expr;
31+
using ::google::api::expr::parser::Parse;
2832
using ::testing::Eq;
2933
using ::testing::SizeIs;
3034

31-
constexpr char kTwoLogicalOp[] = R"cel(
32-
id: 1
33-
call_expr {
34-
function: "$0"
35-
args {
36-
id: 2
37-
ident_expr {
38-
name: "var1",
39-
}
40-
}
41-
args {
42-
id: 3
43-
call_expr {
44-
function: "$0"
45-
args {
46-
id: 4
47-
ident_expr {
48-
name: "var2"
49-
}
50-
}
51-
args {
52-
id: 5
53-
ident_expr {
54-
name: "var3"
55-
}
56-
}
57-
}
58-
}
59-
}
60-
)cel";
61-
62-
constexpr char kTernaryExpr[] = R"cel(
63-
id: 1
64-
call_expr {
65-
function: "_?_:_"
66-
args {
67-
id: 2
68-
ident_expr {
69-
name: "cond"
70-
}
71-
}
72-
args {
73-
id: 3
74-
ident_expr {
75-
name: "arg1"
76-
}
77-
}
78-
args {
79-
id: 4
80-
ident_expr {
81-
name: "arg2"
82-
}
83-
}
84-
})cel";
85-
8635
void BuildAndEval(CelExpressionBuilder* builder, const Expr& expr,
8736
const Activation& activation, google::protobuf::Arena* arena,
8837
CelValue* result) {
@@ -95,12 +44,16 @@ void BuildAndEval(CelExpressionBuilder* builder, const Expr& expr,
9544
*result = *value;
9645
}
9746

98-
class ShortCircuitingTest : public testing::TestWithParam<bool> {
47+
class ShortCircuitingTest
48+
: public testing::TestWithParam<std::tuple<bool, bool>> {
9949
public:
50+
bool short_circuiting() const { return std::get<0>(GetParam()); }
51+
bool enable_variadic() const { return std::get<1>(GetParam()); }
52+
10053
std::unique_ptr<CelExpressionBuilder> GetBuilder(
10154
bool enable_unknowns = false) {
10255
cel::RuntimeOptions options;
103-
options.short_circuiting = GetParam();
56+
options.short_circuiting = short_circuiting();
10457
if (enable_unknowns) {
10558
options.unknown_processing =
10659
cel::UnknownProcessingOptions::kAttributeAndFunction;
@@ -109,14 +62,20 @@ class ShortCircuitingTest : public testing::TestWithParam<bool> {
10962
NewTestingRuntimeEnv(), options);
11063
return result;
11164
}
65+
66+
Expr ParseExpr(absl::string_view expression) {
67+
cel::ParserOptions options;
68+
options.enable_variadic_logical_operators = enable_variadic();
69+
auto parsed_expr = Parse(expression, "<input>", options);
70+
ABSL_CHECK_OK(parsed_expr.status());
71+
return parsed_expr->expr();
72+
}
11273
};
11374

11475
TEST_P(ShortCircuitingTest, BasicAnd) {
115-
Expr expr;
76+
Expr expr = ParseExpr("var1 && var2 && var3");
11677
Activation activation;
11778
google::protobuf::Arena arena;
118-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(
119-
absl::Substitute(kTwoLogicalOp, ::cel::builtin::kAnd), &expr));
12079
auto builder = GetBuilder();
12180

12281
activation.InsertValue("var1", CelValue::CreateBool(true));
@@ -140,11 +99,9 @@ TEST_P(ShortCircuitingTest, BasicAnd) {
14099
}
141100

142101
TEST_P(ShortCircuitingTest, BasicOr) {
143-
Expr expr;
102+
Expr expr = ParseExpr("var1 || var2 || var3");
144103
Activation activation;
145104
google::protobuf::Arena arena;
146-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(
147-
absl::Substitute(kTwoLogicalOp, ::cel::builtin::kOr), &expr));
148105
auto builder = GetBuilder();
149106

150107
activation.InsertValue("var1", CelValue::CreateBool(false));
@@ -168,11 +125,9 @@ TEST_P(ShortCircuitingTest, BasicOr) {
168125
}
169126

170127
TEST_P(ShortCircuitingTest, ErrorAnd) {
171-
Expr expr;
128+
Expr expr = ParseExpr("var1 && var2 && var3");
172129
Activation activation;
173130
google::protobuf::Arena arena;
174-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(
175-
absl::Substitute(kTwoLogicalOp, ::cel::builtin::kAnd), &expr));
176131
auto builder = GetBuilder();
177132
absl::Status error = absl::InternalError("error");
178133

@@ -198,11 +153,9 @@ TEST_P(ShortCircuitingTest, ErrorAnd) {
198153
}
199154

200155
TEST_P(ShortCircuitingTest, ErrorOr) {
201-
Expr expr;
156+
Expr expr = ParseExpr("var1 || var2 || var3");
202157
Activation activation;
203158
google::protobuf::Arena arena;
204-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(
205-
absl::Substitute(kTwoLogicalOp, ::cel::builtin::kOr), &expr));
206159
auto builder = GetBuilder();
207160
absl::Status error = absl::InternalError("error");
208161

@@ -228,11 +181,9 @@ TEST_P(ShortCircuitingTest, ErrorOr) {
228181
}
229182

230183
TEST_P(ShortCircuitingTest, UnknownAnd) {
231-
Expr expr;
184+
Expr expr = ParseExpr("var1 && var2 && var3");
232185
Activation activation;
233186
google::protobuf::Arena arena;
234-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(
235-
absl::Substitute(kTwoLogicalOp, ::cel::builtin::kAnd), &expr));
236187
auto builder = GetBuilder(/* enable_unknowns=*/true);
237188
absl::Status error = absl::InternalError("error");
238189

@@ -260,11 +211,9 @@ TEST_P(ShortCircuitingTest, UnknownAnd) {
260211
}
261212

262213
TEST_P(ShortCircuitingTest, UnknownOr) {
263-
Expr expr;
214+
Expr expr = ParseExpr("var1 || var2 || var3");
264215
Activation activation;
265216
google::protobuf::Arena arena;
266-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(
267-
absl::Substitute(kTwoLogicalOp, ::cel::builtin::kOr), &expr));
268217
auto builder = GetBuilder(/* enable_unknowns=*/true);
269218
absl::Status error = absl::InternalError("error");
270219

@@ -292,10 +241,9 @@ TEST_P(ShortCircuitingTest, UnknownOr) {
292241
}
293242

294243
TEST_P(ShortCircuitingTest, BasicTernary) {
295-
Expr expr;
244+
Expr expr = ParseExpr("cond ? arg1 : arg2");
296245
Activation activation;
297246
google::protobuf::Arena arena;
298-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kTernaryExpr, &expr));
299247
auto builder = GetBuilder();
300248

301249
activation.InsertValue("cond", CelValue::CreateBool(true));
@@ -319,10 +267,9 @@ TEST_P(ShortCircuitingTest, BasicTernary) {
319267
}
320268

321269
TEST_P(ShortCircuitingTest, TernaryErrorHandling) {
322-
Expr expr;
270+
Expr expr = ParseExpr("cond ? arg1 : arg2");
323271
Activation activation;
324272
google::protobuf::Arena arena;
325-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kTernaryExpr, &expr));
326273
auto builder = GetBuilder();
327274

328275
absl::Status error1 = absl::InternalError("error1");
@@ -349,10 +296,9 @@ TEST_P(ShortCircuitingTest, TernaryErrorHandling) {
349296
}
350297

351298
TEST_P(ShortCircuitingTest, TernaryUnknownCondHandling) {
352-
Expr expr;
299+
Expr expr = ParseExpr("cond ? arg1 : arg2");
353300
Activation activation;
354301
google::protobuf::Arena arena;
355-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kTernaryExpr, &expr));
356302
auto builder = GetBuilder(/*enable_unknowns=*/true);
357303

358304
absl::Status error = absl::InternalError("error1");
@@ -386,10 +332,9 @@ TEST_P(ShortCircuitingTest, TernaryUnknownCondHandling) {
386332
}
387333

388334
TEST_P(ShortCircuitingTest, TernaryUnknownArgsHandling) {
389-
Expr expr;
335+
Expr expr = ParseExpr("cond ? arg1 : arg2");
390336
Activation activation;
391337
google::protobuf::Arena arena;
392-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kTernaryExpr, &expr));
393338
auto builder = GetBuilder(/*enable_unknowns=*/true);
394339

395340
absl::Status error = absl::InternalError("error1");
@@ -421,10 +366,9 @@ TEST_P(ShortCircuitingTest, TernaryUnknownArgsHandling) {
421366
}
422367

423368
TEST_P(ShortCircuitingTest, TernaryUnknownAndErrorHandling) {
424-
Expr expr;
369+
Expr expr = ParseExpr("cond ? arg1 : arg2");
425370
Activation activation;
426371
google::protobuf::Arena arena;
427-
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kTernaryExpr, &expr));
428372
auto builder = GetBuilder(/*enable_unknowns=*/true);
429373

430374
absl::Status error = absl::InternalError("error1");
@@ -457,16 +401,16 @@ TEST_P(ShortCircuitingTest, TernaryUnknownAndErrorHandling) {
457401
EXPECT_EQ(attrs.begin()->variable_name(), "cond");
458402
}
459403

460-
const char* TestName(testing::TestParamInfo<bool> info) {
461-
if (info.param) {
462-
return "short_circuit_enabled";
463-
} else {
464-
return "short_circuit_disabled";
465-
}
404+
std::string TestName(testing::TestParamInfo<std::tuple<bool, bool>> info) {
405+
return absl::StrCat(
406+
std::get<0>(info.param) ? "short_circuit_enabled"
407+
: "short_circuit_disabled",
408+
"_", std::get<1>(info.param) ? "variadic_enabled" : "variadic_disabled");
466409
}
467410

468411
INSTANTIATE_TEST_SUITE_P(Test, ShortCircuitingTest,
469-
testing::Values(false, true), &TestName);
412+
testing::Combine(testing::Bool(), testing::Bool()),
413+
&TestName);
470414

471415
} // namespace
472416

0 commit comments

Comments
 (0)