Skip to content

Commit a527907

Browse files
jckingcopybara-github
jcking
authored and
copybara-github
committed
Remove ValueManager from Activation
PiperOrigin-RevId: 726940095
1 parent 6aa0f1d commit a527907

19 files changed

+342
-225
lines changed

‎common/values/legacy_list_value.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,9 +93,6 @@ class LegacyListValue final
9393
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> result) const;
9494
using ListValueMixin::Equal;
9595

96-
absl::Status Contains(ValueManager& value_manager, const Value& other,
97-
Value& result) const;
98-
9996
bool IsZeroValue() const { return IsEmpty(); }
10097

10198
bool IsEmpty() const;

‎eval/compiler/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -617,6 +617,7 @@ cc_test(
617617
"//runtime:type_registry",
618618
"//runtime/internal:runtime_env",
619619
"//runtime/internal:runtime_env_testing",
620+
"//runtime/internal:runtime_value_manager",
620621
"@com_google_absl//absl/base:nullability",
621622
"@com_google_absl//absl/container:flat_hash_map",
622623
"@com_google_absl//absl/status",

‎eval/compiler/instrumentation_test.cc‎

Lines changed: 24 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
#include "runtime/function_registry.h"
3838
#include "runtime/internal/runtime_env.h"
3939
#include "runtime/internal/runtime_env_testing.h"
40+
#include "runtime/internal/runtime_value_manager.h"
4041
#include "runtime/managed_value_factory.h"
4142
#include "runtime/runtime_options.h"
4243
#include "runtime/standard_functions.h"
@@ -62,9 +63,8 @@ class InstrumentationTest : public ::testing::Test {
6263
: env_(NewTestingRuntimeEnv()),
6364
function_registry_(env_->function_registry),
6465
type_registry_(env_->type_registry),
65-
managed_value_factory_(
66-
type_registry_.GetComposedTypeProvider(),
67-
cel::extensions::ProtoMemoryManagerRef(&arena_)) {}
66+
value_manager_(&arena_, env_->descriptor_pool.get(),
67+
env_->MutableMessageFactory()) {}
6868
void SetUp() override {
6969
ASSERT_OK(cel::RegisterStandardFunctions(function_registry_, options_));
7070
}
@@ -75,7 +75,7 @@ class InstrumentationTest : public ::testing::Test {
7575
cel::FunctionRegistry& function_registry_;
7676
cel::TypeRegistry& type_registry_;
7777
google::protobuf::Arena arena_;
78-
cel::ManagedValueFactory managed_value_factory_;
78+
cel::runtime_internal::RuntimeValueManager value_manager_;
7979
};
8080

8181
MATCHER_P(IsIntValue, expected, "") {
@@ -106,7 +106,7 @@ TEST_F(InstrumentationTest, Basic) {
106106
builder.CreateExpressionImpl(std::move(ast),
107107
/*issues=*/nullptr));
108108

109-
auto state = plan.MakeEvaluatorState(managed_value_factory_.get());
109+
auto state = plan.MakeEvaluatorState(value_manager_);
110110
cel::Activation activation;
111111

112112
ASSERT_OK_AND_ASSIGN(
@@ -152,7 +152,7 @@ TEST_F(InstrumentationTest, BasicWithConstFolding) {
152152
Pair(2, IsIntValue(3)), Pair(5, IsIntValue(3))));
153153
expr_id_to_value.clear();
154154

155-
auto state = plan.MakeEvaluatorState(managed_value_factory_.get());
155+
auto state = plan.MakeEvaluatorState(value_manager_);
156156
cel::Activation activation;
157157

158158
ASSERT_OK_AND_ASSIGN(
@@ -190,22 +190,19 @@ TEST_F(InstrumentationTest, AndShortCircuit) {
190190
builder.CreateExpressionImpl(std::move(ast),
191191
/*issues=*/nullptr));
192192

193-
auto state = plan.MakeEvaluatorState(managed_value_factory_.get());
193+
auto state = plan.MakeEvaluatorState(value_manager_);
194194
cel::Activation activation;
195195

196-
activation.InsertOrAssignValue(
197-
"a", managed_value_factory_.get().CreateBoolValue(true));
198-
activation.InsertOrAssignValue(
199-
"b", managed_value_factory_.get().CreateBoolValue(false));
196+
activation.InsertOrAssignValue("a", value_manager_.CreateBoolValue(true));
197+
activation.InsertOrAssignValue("b", value_manager_.CreateBoolValue(false));
200198

201199
ASSERT_OK_AND_ASSIGN(
202200
auto value,
203201
plan.EvaluateWithCallback(activation, EvaluationListener(), state));
204202

205203
EXPECT_THAT(expr_ids, ElementsAre(1, 2, 3));
206204

207-
activation.InsertOrAssignValue(
208-
"a", managed_value_factory_.get().CreateBoolValue(false));
205+
activation.InsertOrAssignValue("a", value_manager_.CreateBoolValue(false));
209206

210207
ASSERT_OK_AND_ASSIGN(value, plan.EvaluateWithCallback(
211208
activation, EvaluationListener(), state));
@@ -235,22 +232,19 @@ TEST_F(InstrumentationTest, OrShortCircuit) {
235232
builder.CreateExpressionImpl(std::move(ast),
236233
/*issues=*/nullptr));
237234

238-
auto state = plan.MakeEvaluatorState(managed_value_factory_.get());
235+
auto state = plan.MakeEvaluatorState(value_manager_);
239236
cel::Activation activation;
240237

241-
activation.InsertOrAssignValue(
242-
"a", managed_value_factory_.get().CreateBoolValue(false));
243-
activation.InsertOrAssignValue(
244-
"b", managed_value_factory_.get().CreateBoolValue(true));
238+
activation.InsertOrAssignValue("a", value_manager_.CreateBoolValue(false));
239+
activation.InsertOrAssignValue("b", value_manager_.CreateBoolValue(true));
245240

246241
ASSERT_OK_AND_ASSIGN(
247242
auto value,
248243
plan.EvaluateWithCallback(activation, EvaluationListener(), state));
249244

250245
EXPECT_THAT(expr_ids, ElementsAre(1, 2, 3));
251246
expr_ids.clear();
252-
activation.InsertOrAssignValue(
253-
"a", managed_value_factory_.get().CreateBoolValue(true));
247+
activation.InsertOrAssignValue("a", value_manager_.CreateBoolValue(true));
254248

255249
ASSERT_OK_AND_ASSIGN(value, plan.EvaluateWithCallback(
256250
activation, EvaluationListener(), state));
@@ -280,15 +274,12 @@ TEST_F(InstrumentationTest, Ternary) {
280274
builder.CreateExpressionImpl(std::move(ast),
281275
/*issues=*/nullptr));
282276

283-
auto state = plan.MakeEvaluatorState(managed_value_factory_.get());
277+
auto state = plan.MakeEvaluatorState(value_manager_);
284278
cel::Activation activation;
285279

286-
activation.InsertOrAssignValue(
287-
"c", managed_value_factory_.get().CreateBoolValue(true));
288-
activation.InsertOrAssignValue(
289-
"a", managed_value_factory_.get().CreateIntValue(1));
290-
activation.InsertOrAssignValue(
291-
"b", managed_value_factory_.get().CreateIntValue(2));
280+
activation.InsertOrAssignValue("c", value_manager_.CreateBoolValue(true));
281+
activation.InsertOrAssignValue("a", value_manager_.CreateIntValue(1));
282+
activation.InsertOrAssignValue("b", value_manager_.CreateIntValue(2));
292283

293284
ASSERT_OK_AND_ASSIGN(
294285
auto value,
@@ -301,8 +292,7 @@ TEST_F(InstrumentationTest, Ternary) {
301292
EXPECT_THAT(expr_ids, ElementsAre(1, 3, 2));
302293
expr_ids.clear();
303294

304-
activation.InsertOrAssignValue(
305-
"c", managed_value_factory_.get().CreateBoolValue(false));
295+
activation.InsertOrAssignValue("c", value_manager_.CreateBoolValue(false));
306296

307297
ASSERT_OK_AND_ASSIGN(value, plan.EvaluateWithCallback(
308298
activation, EvaluationListener(), state));
@@ -336,7 +326,7 @@ TEST_F(InstrumentationTest, OptimizedStepsNotEvaluated) {
336326
builder.CreateExpressionImpl(std::move(ast),
337327
/*issues=*/nullptr));
338328

339-
auto state = plan.MakeEvaluatorState(managed_value_factory_.get());
329+
auto state = plan.MakeEvaluatorState(value_manager_);
340330
cel::Activation activation;
341331

342332
ASSERT_OK_AND_ASSIGN(
@@ -362,15 +352,12 @@ TEST_F(InstrumentationTest, NoopSkipped) {
362352
builder.CreateExpressionImpl(std::move(ast),
363353
/*issues=*/nullptr));
364354

365-
auto state = plan.MakeEvaluatorState(managed_value_factory_.get());
355+
auto state = plan.MakeEvaluatorState(value_manager_);
366356
cel::Activation activation;
367357

368-
activation.InsertOrAssignValue(
369-
"c", managed_value_factory_.get().CreateBoolValue(true));
370-
activation.InsertOrAssignValue(
371-
"a", managed_value_factory_.get().CreateIntValue(1));
372-
activation.InsertOrAssignValue(
373-
"b", managed_value_factory_.get().CreateIntValue(2));
358+
activation.InsertOrAssignValue("c", value_manager_.CreateBoolValue(true));
359+
activation.InsertOrAssignValue("a", value_manager_.CreateIntValue(1));
360+
activation.InsertOrAssignValue("b", value_manager_.CreateIntValue(2));
374361

375362
ASSERT_OK_AND_ASSIGN(
376363
auto value,

‎eval/eval/BUILD‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -713,11 +713,14 @@ cc_test(
713713
"//eval/public:activation",
714714
"//eval/public:cel_attribute",
715715
"//internal:testing",
716+
"//internal:testing_descriptor_pool",
717+
"//internal:testing_message_factory",
716718
"//runtime:activation",
717-
"//runtime:managed_value_factory",
718719
"//runtime:runtime_options",
719720
"//runtime/internal:runtime_env_testing",
721+
"//runtime/internal:runtime_value_manager",
720722
"@com_google_absl//absl/status",
723+
"@com_google_protobuf//:protobuf",
721724
],
722725
)
723726

‎eval/eval/ident_step.cc‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,10 @@ absl::Status LookupIdent(const std::string& name, ExecutionFrameBase& frame,
5858
}
5959
}
6060

61-
CEL_ASSIGN_OR_RETURN(auto found, frame.activation().FindVariable(
62-
frame.value_manager(), name, result));
61+
CEL_ASSIGN_OR_RETURN(
62+
auto found, frame.activation().FindVariable(name, frame.descriptor_pool(),
63+
frame.message_factory(),
64+
frame.arena(), &result));
6365

6466
if (found) {
6567
return absl::OkStatus();

‎eval/eval/ident_step_test.cc‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,13 @@
1616
#include "eval/public/activation.h"
1717
#include "eval/public/cel_attribute.h"
1818
#include "internal/testing.h"
19+
#include "internal/testing_descriptor_pool.h"
20+
#include "internal/testing_message_factory.h"
1921
#include "runtime/activation.h"
2022
#include "runtime/internal/runtime_env_testing.h"
21-
#include "runtime/managed_value_factory.h"
23+
#include "runtime/internal/runtime_value_manager.h"
2224
#include "runtime/runtime_options.h"
25+
#include "google/protobuf/arena.h"
2326

2427
namespace google::api::expr::runtime {
2528

@@ -30,7 +33,6 @@ using ::cel::Cast;
3033
using ::cel::ErrorValue;
3134
using ::cel::InstanceOf;
3235
using ::cel::IntValue;
33-
using ::cel::ManagedValueFactory;
3436
using ::cel::MemoryManagerRef;
3537
using ::cel::RuntimeOptions;
3638
using ::cel::TypeProvider;
@@ -233,14 +235,16 @@ TEST(IdentStepTest, TestIdentStepUnknownAttribute) {
233235
}
234236

235237
TEST(DirectIdentStepTest, Basic) {
236-
ManagedValueFactory value_factory(TypeProvider::Builtin(),
237-
MemoryManagerRef::ReferenceCounting());
238+
google::protobuf::Arena arena;
239+
cel::runtime_internal::RuntimeValueManager value_factory(
240+
&arena, cel::internal::GetTestingDescriptorPool(),
241+
cel::internal::GetTestingMessageFactory());
238242
cel::Activation activation;
239243
RuntimeOptions options;
240244

241245
activation.InsertOrAssignValue("var1", IntValue(42));
242246

243-
ExecutionFrameBase frame(activation, options, value_factory.get());
247+
ExecutionFrameBase frame(activation, options, value_factory);
244248
Value result;
245249
AttributeTrail trail;
246250

@@ -253,16 +257,18 @@ TEST(DirectIdentStepTest, Basic) {
253257
}
254258

255259
TEST(DirectIdentStepTest, UnknownAttribute) {
256-
ManagedValueFactory value_factory(TypeProvider::Builtin(),
257-
MemoryManagerRef::ReferenceCounting());
260+
google::protobuf::Arena arena;
261+
cel::runtime_internal::RuntimeValueManager value_factory(
262+
&arena, cel::internal::GetTestingDescriptorPool(),
263+
cel::internal::GetTestingMessageFactory());
258264
cel::Activation activation;
259265
RuntimeOptions options;
260266
options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly;
261267

262268
activation.InsertOrAssignValue("var1", IntValue(42));
263269
activation.SetUnknownPatterns({CreateCelAttributePattern("var1", {})});
264270

265-
ExecutionFrameBase frame(activation, options, value_factory.get());
271+
ExecutionFrameBase frame(activation, options, value_factory);
266272
Value result;
267273
AttributeTrail trail;
268274

@@ -275,16 +281,18 @@ TEST(DirectIdentStepTest, UnknownAttribute) {
275281
}
276282

277283
TEST(DirectIdentStepTest, MissingAttribute) {
278-
ManagedValueFactory value_factory(TypeProvider::Builtin(),
279-
MemoryManagerRef::ReferenceCounting());
284+
google::protobuf::Arena arena;
285+
cel::runtime_internal::RuntimeValueManager value_factory(
286+
&arena, cel::internal::GetTestingDescriptorPool(),
287+
cel::internal::GetTestingMessageFactory());
280288
cel::Activation activation;
281289
RuntimeOptions options;
282290
options.enable_missing_attribute_errors = true;
283291

284292
activation.InsertOrAssignValue("var1", IntValue(42));
285293
activation.SetMissingPatterns({CreateCelAttributePattern("var1", {})});
286294

287-
ExecutionFrameBase frame(activation, options, value_factory.get());
295+
ExecutionFrameBase frame(activation, options, value_factory);
288296
Value result;
289297
AttributeTrail trail;
290298

@@ -298,12 +306,14 @@ TEST(DirectIdentStepTest, MissingAttribute) {
298306
}
299307

300308
TEST(DirectIdentStepTest, NotFound) {
301-
ManagedValueFactory value_factory(TypeProvider::Builtin(),
302-
MemoryManagerRef::ReferenceCounting());
309+
google::protobuf::Arena arena;
310+
cel::runtime_internal::RuntimeValueManager value_factory(
311+
&arena, cel::internal::GetTestingDescriptorPool(),
312+
cel::internal::GetTestingMessageFactory());
303313
cel::Activation activation;
304314
RuntimeOptions options;
305315

306-
ExecutionFrameBase frame(activation, options, value_factory.get());
316+
ExecutionFrameBase frame(activation, options, value_factory);
307317
Value result;
308318
AttributeTrail trail;
309319

‎eval/eval/shadowable_value_step.cc‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ absl::Status ShadowableValueStep::Evaluate(ExecutionFrame* frame) const {
3939
cel::Value result;
4040
CEL_ASSIGN_OR_RETURN(auto found,
4141
frame->modern_activation().FindVariable(
42-
frame->value_factory(), identifier_, result));
42+
identifier_, frame->descriptor_pool(),
43+
frame->message_factory(), frame->arena(), &result));
4344
if (found) {
4445
frame->value_stack().Push(std::move(result));
4546
} else {
@@ -70,8 +71,9 @@ class DirectShadowableValueStep : public DirectExpressionStep {
7071
absl::Status DirectShadowableValueStep::Evaluate(
7172
ExecutionFrameBase& frame, Value& result, AttributeTrail& attribute) const {
7273
CEL_ASSIGN_OR_RETURN(auto found,
73-
frame.activation().FindVariable(frame.value_manager(),
74-
identifier_, result));
74+
frame.activation().FindVariable(
75+
identifier_, frame.descriptor_pool(),
76+
frame.message_factory(), frame.arena(), &result));
7577
if (!found) {
7678
result = value_;
7779
}

‎eval/internal/BUILD‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,14 +82,13 @@ cc_library(
8282
deps = [
8383
":interop",
8484
"//base:attributes",
85-
"//common:memory",
8685
"//common:value",
8786
"//eval/public:base_activation",
8887
"//eval/public:cel_value",
89-
"//extensions/protobuf:memory_manager",
9088
"//internal:status_macros",
9189
"//runtime:activation_interface",
9290
"//runtime:function_overload_reference",
91+
"@com_google_absl//absl/base:nullability",
9392
"@com_google_absl//absl/status:statusor",
9493
"@com_google_absl//absl/strings",
9594
"@com_google_absl//absl/types:optional",

‎eval/internal/adapter_activation_impl.cc‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,33 +16,37 @@
1616

1717
#include <vector>
1818

19+
#include "absl/base/nullability.h"
1920
#include "absl/status/statusor.h"
2021
#include "absl/strings/string_view.h"
2122
#include "absl/types/optional.h"
23+
#include "common/value.h"
2224
#include "eval/internal/interop.h"
2325
#include "eval/public/cel_value.h"
24-
#include "extensions/protobuf/memory_manager.h"
2526
#include "internal/status_macros.h"
2627
#include "runtime/function_overload_reference.h"
2728
#include "google/protobuf/arena.h"
29+
#include "google/protobuf/descriptor.h"
30+
#include "google/protobuf/message.h"
2831

2932
namespace cel::interop_internal {
3033

3134
using ::google::api::expr::runtime::CelFunction;
3235

3336
absl::StatusOr<bool> AdapterActivationImpl::FindVariable(
34-
ValueManager& value_factory, absl::string_view name, Value& result) const {
37+
absl::string_view name,
38+
absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
39+
absl::Nonnull<google::protobuf::MessageFactory*> message_factory,
40+
absl::Nonnull<google::protobuf::Arena*> arena, absl::Nonnull<Value*> result) const {
3541
// This implementation should only be used during interop, when we can
3642
// always assume the memory manager is backed by a protobuf arena.
37-
google::protobuf::Arena* arena =
38-
extensions::ProtoMemoryManagerArena(value_factory.GetMemoryManager());
3943

4044
absl::optional<google::api::expr::runtime::CelValue> legacy_value =
4145
legacy_activation_.FindValue(name, arena);
4246
if (!legacy_value.has_value()) {
4347
return false;
4448
}
45-
CEL_RETURN_IF_ERROR(ModernValue(arena, *legacy_value, result));
49+
CEL_RETURN_IF_ERROR(ModernValue(arena, *legacy_value, *result));
4650
return true;
4751
}
4852

0 commit comments

Comments
 (0)