Skip to content

Commit 46900e5

Browse files
jnthntatumcopybara-github
jnthntatum
authored and
copybara-github
committed
Add special constant instruction in the main interpret loop.
Consolidate error reporting and move error check outside of Eval loop. PiperOrigin-RevId: 982722450
1 parent 2ff1fbd commit 46900e5

17 files changed

Lines changed: 379 additions & 197 deletions

‎eval/compiler/BUILD‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,6 @@ cc_test(
360360
"//base:ast",
361361
"//common:expr",
362362
"//common:value",
363-
"//eval/eval:const_value_step",
364363
"//eval/eval:create_list_step",
365364
"//eval/eval:create_map_step",
366365
"//eval/eval:evaluator_core",

‎eval/compiler/constant_folding.cc‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ using ::cel::builtin::kOr;
5050
using ::cel::builtin::kTernary;
5151
using ::cel::runtime_internal::ConvertConstant;
5252
using ::google::api::expr::runtime::CreateConstValueDirectStep;
53-
using ::google::api::expr::runtime::CreateConstValueStep;
5453
using ::google::api::expr::runtime::EvaluationListener;
5554
using ::google::api::expr::runtime::ExecutionFrame;
5655
using ::google::api::expr::runtime::ExecutionPath;
@@ -243,8 +242,7 @@ absl::Status ConstantFoldingExtension::OnPostVisit(PlannerContext& context,
243242

244243
// Otherwise make a stack machine plan.
245244
ExecutionPath new_plan;
246-
new_plan.push_back(ExpressionStep::MakeGenericStep(
247-
CreateConstValueStep(std::move(value)), node.id()));
245+
new_plan.push_back(ExpressionStep::MakeConstant(value, node.id()));
248246

249247
return context.ReplaceSubplan(node, std::move(new_plan));
250248
}

‎eval/compiler/constant_folding_test.cc‎

Lines changed: 48 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
#include "common/value.h"
3030
#include "eval/compiler/flat_expr_builder_extensions.h"
3131
#include "eval/compiler/resolver.h"
32-
#include "eval/eval/const_value_step.h"
3332
#include "eval/eval/create_list_step.h"
3433
#include "eval/eval/create_map_step.h"
3534
#include "eval/eval/evaluator_core.h"
@@ -58,7 +57,6 @@ using ::cel::runtime_internal::IssueCollector;
5857
using ::cel::runtime_internal::NewTestingRuntimeEnv;
5958
using ::cel::expr::ParsedExpr;
6059
using ::google::api::expr::parser::Parse;
61-
using ::google::api::expr::runtime::CreateConstValueStep;
6260
using ::google::api::expr::runtime::CreateCreateListStep;
6361
using ::google::api::expr::runtime::CreateCreateStructStepForMap;
6462
using ::google::api::expr::runtime::ExecutionPath;
@@ -116,25 +114,25 @@ TEST_F(UpdatedConstantFoldingTest, SkipsTernary) {
116114
program_builder.EnterSubexpression(&call);
117115
// condition
118116
program_builder.EnterSubexpression(&condition);
119-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
120-
CreateConstValueStep(cel::BoolValue(true)), condition.id()));
117+
program_builder.AddStep(
118+
ExpressionStep::MakeConstant(cel::BoolValue(true), condition.id()));
121119
program_builder.ExitSubexpression(&condition);
122120

123121
// true
124122
program_builder.EnterSubexpression(&true_branch);
125-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
126-
CreateConstValueStep(cel::BoolValue(true)), true_branch.id()));
123+
program_builder.AddStep(
124+
ExpressionStep::MakeConstant(cel::BoolValue(true), true_branch.id()));
127125
program_builder.ExitSubexpression(&true_branch);
128126

129127
// false
130128
program_builder.EnterSubexpression(&false_branch);
131-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
132-
CreateConstValueStep(cel::BoolValue(true)), false_branch.id()));
129+
program_builder.AddStep(
130+
ExpressionStep::MakeConstant(cel::BoolValue(true), false_branch.id()));
133131
program_builder.ExitSubexpression(&false_branch);
134132

135133
// ternary.
136-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
137-
CreateConstValueStep(cel::NullValue()), call.id()));
134+
program_builder.AddStep(
135+
ExpressionStep::MakeConstant(cel::NullValue(), call.id()));
138136
program_builder.ExitSubexpression(&call);
139137

140138
std::shared_ptr<google::protobuf::Arena> arena;
@@ -179,20 +177,20 @@ TEST_F(UpdatedConstantFoldingTest, SkipsOr) {
179177

180178
// left
181179
program_builder.EnterSubexpression(&left_condition);
182-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
183-
CreateConstValueStep(cel::BoolValue(false)), left_condition.id()));
180+
program_builder.AddStep(
181+
ExpressionStep::MakeConstant(cel::BoolValue(false), left_condition.id()));
184182
program_builder.ExitSubexpression(&left_condition);
185183

186184
// right
187185
program_builder.EnterSubexpression(&right_condition);
188-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
189-
CreateConstValueStep(cel::BoolValue(true)), right_condition.id()));
186+
program_builder.AddStep(
187+
ExpressionStep::MakeConstant(cel::BoolValue(true), right_condition.id()));
190188
program_builder.ExitSubexpression(&right_condition);
191189

192190
// op
193191
// Just a placeholder.
194-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
195-
CreateConstValueStep(cel::NullValue()), call.id()));
192+
program_builder.AddStep(
193+
ExpressionStep::MakeConstant(cel::NullValue(), call.id()));
196194
program_builder.ExitSubexpression(&call);
197195

198196
std::shared_ptr<google::protobuf::Arena> arena;
@@ -234,20 +232,20 @@ TEST_F(UpdatedConstantFoldingTest, SkipsAnd) {
234232

235233
// left
236234
program_builder.EnterSubexpression(&left_condition);
237-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
238-
CreateConstValueStep(cel::BoolValue(true)), left_condition.id()));
235+
program_builder.AddStep(
236+
ExpressionStep::MakeConstant(cel::BoolValue(true), left_condition.id()));
239237
program_builder.ExitSubexpression(&left_condition);
240238

241239
// right
242240
program_builder.EnterSubexpression(&right_condition);
243-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
244-
CreateConstValueStep(cel::BoolValue(false)), right_condition.id()));
241+
program_builder.AddStep(ExpressionStep::MakeConstant(cel::BoolValue(false),
242+
right_condition.id()));
245243
program_builder.ExitSubexpression(&right_condition);
246244

247245
// op
248246
// Just a placeholder.
249-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
250-
CreateConstValueStep(cel::NullValue()), call.id()));
247+
program_builder.AddStep(
248+
ExpressionStep::MakeConstant(cel::NullValue(), call.id()));
251249
program_builder.ExitSubexpression(&call);
252250

253251
std::shared_ptr<google::protobuf::Arena> arena;
@@ -289,14 +287,14 @@ TEST_F(UpdatedConstantFoldingTest, CreatesList) {
289287

290288
// elem one
291289
program_builder.EnterSubexpression(&elem_one);
292-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
293-
CreateConstValueStep(cel::IntValue(1L)), elem_one.id()));
290+
program_builder.AddStep(
291+
ExpressionStep::MakeConstant(cel::IntValue(1L), elem_one.id()));
294292
program_builder.ExitSubexpression(&elem_one);
295293

296294
// elem two
297295
program_builder.EnterSubexpression(&elem_two);
298-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
299-
CreateConstValueStep(cel::IntValue(2L)), elem_two.id()));
296+
program_builder.AddStep(
297+
ExpressionStep::MakeConstant(cel::IntValue(2L), elem_two.id()));
300298
program_builder.ExitSubexpression(&elem_two);
301299

302300
// createlist
@@ -349,32 +347,32 @@ TEST_F(UpdatedConstantFoldingTest, CreatesLargeList) {
349347

350348
// 0
351349
ASSERT_TRUE(program_builder.EnterSubexpression(&elem0) != nullptr);
352-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
353-
CreateConstValueStep(cel::IntValue(1L)), elem0.id()));
350+
program_builder.AddStep(
351+
ExpressionStep::MakeConstant(cel::IntValue(1L), elem0.id()));
354352
program_builder.ExitSubexpression(&elem0);
355353

356354
// 1
357355
ASSERT_TRUE(program_builder.EnterSubexpression(&elem1));
358-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
359-
CreateConstValueStep(cel::IntValue(2L)), elem1.id()));
356+
program_builder.AddStep(
357+
ExpressionStep::MakeConstant(cel::IntValue(2L), elem1.id()));
360358
program_builder.ExitSubexpression(&elem1);
361359

362360
// 2
363361
ASSERT_TRUE(program_builder.EnterSubexpression(&elem2) != nullptr);
364-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
365-
CreateConstValueStep(cel::IntValue(3L)), elem2.id()));
362+
program_builder.AddStep(
363+
ExpressionStep::MakeConstant(cel::IntValue(3L), elem2.id()));
366364
program_builder.ExitSubexpression(&elem2);
367365

368366
// 3
369367
ASSERT_TRUE(program_builder.EnterSubexpression(&elem3) != nullptr);
370-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
371-
CreateConstValueStep(cel::IntValue(4L)), elem3.id()));
368+
program_builder.AddStep(
369+
ExpressionStep::MakeConstant(cel::IntValue(4L), elem3.id()));
372370
program_builder.ExitSubexpression(&elem3);
373371

374372
// 4
375373
ASSERT_TRUE(program_builder.EnterSubexpression(&elem4) != nullptr);
376-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
377-
CreateConstValueStep(cel::IntValue(5L)), elem4.id()));
374+
program_builder.AddStep(
375+
ExpressionStep::MakeConstant(cel::IntValue(5L), elem4.id()));
378376
program_builder.ExitSubexpression(&elem4);
379377

380378
// createlist
@@ -428,14 +426,14 @@ TEST_F(UpdatedConstantFoldingTest, CreatesMap) {
428426

429427
// key
430428
program_builder.EnterSubexpression(&key);
431-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
432-
CreateConstValueStep(cel::IntValue(1L)), key.id()));
429+
program_builder.AddStep(
430+
ExpressionStep::MakeConstant(cel::IntValue(1L), key.id()));
433431
program_builder.ExitSubexpression(&key);
434432

435433
// value
436434
program_builder.EnterSubexpression(&value);
437-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
438-
CreateConstValueStep(cel::IntValue(2L)), value.id()));
435+
program_builder.AddStep(
436+
ExpressionStep::MakeConstant(cel::IntValue(2L), value.id()));
439437
program_builder.ExitSubexpression(&value);
440438

441439
// create map
@@ -484,14 +482,14 @@ TEST_F(UpdatedConstantFoldingTest, CreatesInvalidMap) {
484482

485483
// key
486484
program_builder.EnterSubexpression(&key);
487-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
488-
CreateConstValueStep(cel::DoubleValue(1.0)), key.id()));
485+
program_builder.AddStep(
486+
ExpressionStep::MakeConstant(cel::DoubleValue(1.0), key.id()));
489487
program_builder.ExitSubexpression(&key);
490488

491489
// value
492490
program_builder.EnterSubexpression(&value);
493-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
494-
CreateConstValueStep(cel::IntValue(2L)), value.id()));
491+
program_builder.AddStep(
492+
ExpressionStep::MakeConstant(cel::IntValue(2L), value.id()));
495493
program_builder.ExitSubexpression(&value);
496494

497495
// create map
@@ -539,20 +537,20 @@ TEST_F(UpdatedConstantFoldingTest, ErrorsOnUnexpectedOrder) {
539537
program_builder.EnterSubexpression(&call);
540538
// left
541539
program_builder.EnterSubexpression(&left_condition);
542-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
543-
CreateConstValueStep(cel::BoolValue(true)), left_condition.id()));
540+
program_builder.AddStep(
541+
ExpressionStep::MakeConstant(cel::BoolValue(true), left_condition.id()));
544542
program_builder.ExitSubexpression(&left_condition);
545543

546544
// right
547545
program_builder.EnterSubexpression(&right_condition);
548-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
549-
CreateConstValueStep(cel::BoolValue(false)), right_condition.id()));
546+
program_builder.AddStep(ExpressionStep::MakeConstant(cel::BoolValue(false),
547+
right_condition.id()));
550548
program_builder.ExitSubexpression(&right_condition);
551549

552550
// op
553551
// Just a placeholder.
554-
program_builder.AddStep(ExpressionStep::MakeGenericStep(
555-
CreateConstValueStep(cel::NullValue()), call.id()));
552+
program_builder.AddStep(
553+
ExpressionStep::MakeConstant(cel::NullValue(), call.id()));
556554
program_builder.ExitSubexpression(&call);
557555

558556
std::shared_ptr<google::protobuf::Arena> arena;

‎eval/compiler/flat_expr_builder.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -730,8 +730,8 @@ class FlatExprVisitor : public cel::AstVisitor {
730730
return;
731731
}
732732

733-
AddStep(CreateConstValueStep(std::move(converted_value).value()),
734-
expr.id());
733+
AddStep(ExpressionStep::MakeConstant(std::move(converted_value).value(),
734+
expr.id()));
735735
}
736736

737737
struct SlotLookupResult {

‎eval/compiler/flat_expr_builder_extensions_test.cc‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,17 @@ using ::testing::ElementsAre;
5555
using ::testing::IsEmpty;
5656
using ::testing::Optional;
5757

58+
class TestStepLogic : public ExpressionStepLogic {
59+
public:
60+
absl::Status Evaluate(ExecutionFrame* frame) const override {
61+
return absl::OkStatus();
62+
}
63+
};
64+
65+
std::unique_ptr<ExpressionStepLogic> MakeTestStepLogic() {
66+
return std::make_unique<TestStepLogic>();
67+
}
68+
5869
using Subexpression = ProgramBuilder::Subexpression;
5970

6071
class PlannerContextTest : public testing::Test {
@@ -94,9 +105,9 @@ struct SimpleTreeSteps {
94105
absl::StatusOr<SimpleTreeSteps> InitSimpleTree(
95106
const Expr& a, const Expr& b, const Expr& c,
96107
ProgramBuilder& program_builder) {
97-
auto a_step = CreateConstValueStep(cel::NullValue());
98-
auto b_step = CreateConstValueStep(cel::NullValue());
99-
auto c_step = CreateConstValueStep(cel::NullValue());
108+
auto a_step = MakeTestStepLogic();
109+
auto b_step = MakeTestStepLogic();
110+
auto c_step = MakeTestStepLogic();
100111

101112
SimpleTreeSteps result{a_step.get(), b_step.get(), c_step.get()};
102113

@@ -163,7 +174,7 @@ TEST_F(PlannerContextTest, ReplacePlan) {
163174

164175
ExecutionPath new_a;
165176

166-
auto new_a_step = CreateConstValueStep(cel::NullValue());
177+
auto new_a_step = MakeTestStepLogic();
167178
const ExpressionStepLogic* new_a_step_ptr = new_a_step.get();
168179
new_a.push_back(ExpressionStep::MakeGenericStep(std::move(new_a_step), -1));
169180

@@ -253,10 +264,10 @@ TEST_F(PlannerContextTest, ReplacePlanUpdatesSibling) {
253264

254265
ExecutionPath new_b;
255266

256-
auto b1_step = CreateConstValueStep(cel::NullValue());
267+
auto b1_step = MakeTestStepLogic();
257268
const ExpressionStepLogic* b1_step_ptr = b1_step.get();
258269
new_b.push_back(ExpressionStep::MakeGenericStep(std::move(b1_step), -1));
259-
auto b2_step = CreateConstValueStep(cel::NullValue());
270+
auto b2_step = MakeTestStepLogic();
260271
const ExpressionStepLogic* b2_step_ptr = b2_step.get();
261272
new_b.push_back(ExpressionStep::MakeGenericStep(std::move(b2_step), -1));
262273

@@ -302,7 +313,7 @@ TEST_F(PlannerContextTest, AddSubplanStep) {
302313
ASSERT_OK_AND_ASSIGN(auto plan_steps,
303314
InitSimpleTree(a, b, c, program_builder));
304315

305-
auto b2_step = CreateConstValueStep(cel::NullValue());
316+
auto b2_step = MakeTestStepLogic();
306317

307318
const ExpressionStepLogic* b2_step_ptr = b2_step.get();
308319

@@ -331,7 +342,7 @@ TEST_F(PlannerContextTest, AddSubplanStepFailsOnUnknownNode) {
331342

332343
ASSERT_THAT(InitSimpleTree(a, b, c, program_builder).status(), IsOk());
333344

334-
auto b2_step = CreateConstValueStep(cel::NullValue());
345+
auto b2_step = MakeTestStepLogic();
335346

336347
std::shared_ptr<google::protobuf::Arena> arena;
337348
PlannerContext context(env_, resolver_, options_,
@@ -478,7 +489,7 @@ TEST_F(ProgramBuilderTest, ExtractWorks) {
478489
program_builder.EnterSubexpression(&b);
479490
program_builder.ExitSubexpression(&b);
480491

481-
auto a_step = CreateConstValueStep(cel::NullValue());
492+
auto a_step = MakeTestStepLogic();
482493
program_builder.AddStep(
483494
ExpressionStep::MakeGenericStep(std::move(a_step), -1));
484495
program_builder.EnterSubexpression(&c);

‎eval/compiler/regex_precompilation_optimization.cc‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -194,12 +194,11 @@ class RegexPrecompilationOptimization : public ProgramOptimizer {
194194
} else {
195195
// otherwise stack-machine program.
196196
ExecutionPathView re_plan = context.GetSubplan(re_expr);
197-
if (re_plan.size() == 1 && re_plan[0].IsGenericStep() &&
198-
re_plan[0].GetGenericStep()->GetNativeTypeId() ==
199-
NativeTypeId::For<CompilerConstantStep>()) {
200-
constant =
201-
down_cast<const CompilerConstantStep*>(re_plan[0].GetGenericStep())
202-
->value();
197+
if (re_plan.size() == 1) {
198+
cel::Value val;
199+
if (GetIfConstant(re_plan[0], val)) {
200+
constant = std::move(val);
201+
}
203202
}
204203
}
205204

0 commit comments

Comments
 (0)