Change Activation attribute pattern mutators to return absl::Status

eval/compiler/BUILD

@@ -511,6 +511,7 @@ cc_test(
         "//runtime/internal:runtime_env_testing",
         "@com_google_absl//absl/log:absl_check",
         "@com_google_absl//absl/status",
+        "@com_google_absl//absl/status:status_matchers",
         "@com_google_absl//absl/strings",
         "@com_google_protobuf//:protobuf",
     ],

eval/compiler/flat_expr_builder_comprehensions_test.cc

@@ -43,6 +43,7 @@ namespace google::api::expr::runtime {
 
 namespace {
 
+using ::absl_testing::IsOk;
 using ::absl_testing::StatusIs;
 using ::cel::runtime_internal::NewTestingRuntimeEnv;
 using ::cel::expr::CheckedExpr;
@@ -152,9 +153,11 @@ TEST_P(CelExpressionBuilderFlatImplComprehensionsTest, ListCompWithUnknowns) {
                                                 &parsed_expr.source_info()));
 
   Activation activation;
-  activation.set_unknown_attribute_patterns({CelAttributePattern{
-      "items",
-      {CreateCelAttributeQualifierPattern(CelValue::CreateInt64(1))}}});
+  ASSERT_THAT(
+      activation.SetUnknownAttributePatterns({CelAttributePattern{
+          "items",
+          {CreateCelAttributeQualifierPattern(CelValue::CreateInt64(1))}}}),
+      IsOk());
   ContainerBackedListImpl list_impl = ContainerBackedListImpl({
       CelValue::CreateInt64(1),
       // element items[1] is marked unknown, so the computation should produce

eval/compiler/flat_expr_builder_short_circuiting_conformance_test.cc

@@ -6,6 +6,7 @@
 
 #include "absl/log/absl_check.h"
 #include "absl/status/status.h"
+#include "absl/status/status_matchers.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 #include "eval/compiler/cel_expression_builder_flat_impl.h"
@@ -26,6 +27,7 @@ namespace google::api::expr::runtime {
 
 namespace {
 
+using ::absl_testing::IsOk;
 using ::cel::runtime_internal::NewTestingRuntimeEnv;
 using ::cel::expr::Expr;
 using ::google::api::expr::parser::Parse;
@@ -187,7 +189,9 @@ TEST_P(ShortCircuitingTest, UnknownAnd) {
   auto builder = GetBuilder(/* enable_unknowns=*/true);
   absl::Status error = absl::InternalError("error");
 
-  activation.set_unknown_attribute_patterns({CelAttributePattern("var1", {})});
+  ASSERT_THAT(
+      activation.SetUnknownAttributePatterns({CelAttributePattern("var1", {})}),
+      IsOk());
   activation.InsertValue("var2", CelValue::CreateError(&error));
   activation.InsertValue("var3", CelValue::CreateBool(false));
 
@@ -217,7 +221,9 @@ TEST_P(ShortCircuitingTest, UnknownOr) {
   auto builder = GetBuilder(/* enable_unknowns=*/true);
   absl::Status error = absl::InternalError("error");
 
-  activation.set_unknown_attribute_patterns({CelAttributePattern("var1", {})});
+  ASSERT_THAT(
+      activation.SetUnknownAttributePatterns({CelAttributePattern("var1", {})}),
+      IsOk());
   activation.InsertValue("var2", CelValue::CreateError(&error));
   activation.InsertValue("var3", CelValue::CreateBool(true));
 
@@ -307,7 +313,9 @@ TEST_P(ShortCircuitingTest, TernaryUnknownCondHandling) {
   activation.InsertValue("arg1", CelValue::CreateError(&error));
   activation.InsertValue("arg2", CelValue::CreateInt64(-1));
 
-  activation.set_unknown_attribute_patterns({CelAttributePattern("cond", {})});
+  ASSERT_THAT(
+      activation.SetUnknownAttributePatterns({CelAttributePattern("cond", {})}),
+      IsOk());
 
   CelValue result;
   ASSERT_NO_FATAL_FAILURE(
@@ -319,9 +327,11 @@ TEST_P(ShortCircuitingTest, TernaryUnknownCondHandling) {
   EXPECT_THAT(attrs.begin()->variable_name(), Eq("cond"));
 
   // Unknown branches are discarded if condition is unknown
-  activation.set_unknown_attribute_patterns({CelAttributePattern("cond", {}),
-                                             CelAttributePattern("arg1", {}),
-                                             CelAttributePattern("arg2", {})});
+  ASSERT_THAT(
+      activation.SetUnknownAttributePatterns({CelAttributePattern("cond", {}),
+                                              CelAttributePattern("arg1", {}),
+                                              CelAttributePattern("arg2", {})}),
+      IsOk());
 
   ASSERT_NO_FATAL_FAILURE(
       BuildAndEval(builder.get(), expr, activation, &arena, &result));
@@ -344,7 +354,9 @@ TEST_P(ShortCircuitingTest, TernaryUnknownArgsHandling) {
   activation.InsertValue("arg2", CelValue::CreateInt64(-1));
 
   // Unknown arg is discarded if condition chooses other branch.
-  activation.set_unknown_attribute_patterns({CelAttributePattern("arg1", {})});
+  ASSERT_THAT(
+      activation.SetUnknownAttributePatterns({CelAttributePattern("arg1", {})}),
+      IsOk());
 
   CelValue result;
 
@@ -354,8 +366,10 @@ TEST_P(ShortCircuitingTest, TernaryUnknownArgsHandling) {
   EXPECT_EQ(result.Int64OrDie(), -1);
 
   // Branches won't merge if both are unknown.
-  activation.set_unknown_attribute_patterns(
-      {CelAttributePattern("arg1", {}), CelAttributePattern("arg2", {})});
+  ASSERT_THAT(
+      activation.SetUnknownAttributePatterns(
+          {CelAttributePattern("arg1", {}), CelAttributePattern("arg2", {})}),
+      IsOk());
 
   ASSERT_NO_FATAL_FAILURE(
       BuildAndEval(builder.get(), expr, activation, &arena, &result));
@@ -378,8 +392,10 @@ TEST_P(ShortCircuitingTest, TernaryUnknownAndErrorHandling) {
   activation.InsertValue("arg2", CelValue::CreateInt64(-1));
 
   // Error cond discards args
-  activation.set_unknown_attribute_patterns(
-      {CelAttributePattern("arg1", {}), CelAttributePattern("arg2", {})});
+  ASSERT_THAT(
+      activation.SetUnknownAttributePatterns(
+          {CelAttributePattern("arg1", {}), CelAttributePattern("arg2", {})}),
+      IsOk());
 
   CelValue result;
 
@@ -389,7 +405,9 @@ TEST_P(ShortCircuitingTest, TernaryUnknownAndErrorHandling) {
   EXPECT_EQ(*result.ErrorOrDie(), error);
 
   // Error arg discarded if condition unknown
-  activation.set_unknown_attribute_patterns({CelAttributePattern("cond", {})});
+  ASSERT_THAT(
+      activation.SetUnknownAttributePatterns({CelAttributePattern("cond", {})}),
+      IsOk());
   ASSERT_TRUE(activation.RemoveValueEntry("arg1"));
   activation.InsertValue("arg1", CelValue::CreateError(&error));
 

eval/compiler/flat_expr_builder_test.cc

@@ -2954,7 +2954,9 @@ TEST_P(FlatExprBuilderVariadicLogicalTest, Evaluate) {
   insert_value("c", test_case.c_val);
 
   if (!unknown_patterns.empty()) {
-    activation.set_unknown_attribute_patterns(std::move(unknown_patterns));
+    ASSERT_THAT(
+        activation.SetUnknownAttributePatterns(std::move(unknown_patterns)),
+        IsOk());
   }
 
   ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena));

eval/eval/BUILD

@@ -418,7 +418,6 @@ cc_test(
     deps = [
         ":attribute_trail",
         ":direct_expression_step",
-        ":equality_steps",
         ":evaluator_core",
         "//base:attributes",
         "//common:value",
@@ -452,7 +451,6 @@ cc_test(
         ":direct_expression_step",
         ":evaluator_core",
         ":expression_step_base",
-        ":ident_step",
         "//base:data",
         "//common:expr",
         "//common:value",
@@ -469,7 +467,6 @@ cc_test(
         "//runtime:runtime_options",
         "//runtime/internal:runtime_env_testing",
         "//runtime/internal:runtime_type_provider",
-        "@com_google_absl//absl/memory",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
@@ -519,7 +516,6 @@ cc_test(
         ":container_access_step",
         ":direct_expression_step",
         ":evaluator_core",
-        ":ident_step",
         "//base:builtins",
         "//base:data",
         "//common:ast",
@@ -540,6 +536,7 @@ cc_test(
         "//runtime/internal:runtime_env",
         "//runtime/internal:runtime_env_testing",
         "@com_google_absl//absl/base:nullability",
+        "@com_google_absl//absl/log:absl_check",
         "@com_google_absl//absl/status",
         "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto",
         "@com_google_protobuf//:protobuf",
@@ -608,8 +605,6 @@ cc_test(
         ":const_value_step",
         ":direct_expression_step",
         ":evaluator_core",
-        ":function_step",
-        ":ident_step",
         "//base:builtins",
         "//base:data",
         "//common:constant",

eval/eval/comprehension_step_test.cc

@@ -113,11 +113,13 @@ TEST_F(ListKeysStepTest, MapPartiallyUnknown) {
   (*value.mutable_fields())["key3"].set_number_value(3.0);
 
   activation.InsertValue("var", CelProtoWrapper::CreateMessage(&value, &arena));
-  activation.set_unknown_attribute_patterns({CelAttributePattern(
-      "var",
-      {CreateCelAttributeQualifierPattern(CelValue::CreateStringView("key2")),
-       CreateCelAttributeQualifierPattern(CelValue::CreateStringView("foo")),
-       CelAttributeQualifierPattern::CreateWildcard()})});
+  ASSERT_THAT(activation.SetUnknownAttributePatterns({CelAttributePattern(
+                  "var", {CreateCelAttributeQualifierPattern(
+                              CelValue::CreateStringView("key2")),
+                          CreateCelAttributeQualifierPattern(
+                              CelValue::CreateStringView("foo")),
+                          CelAttributeQualifierPattern::CreateWildcard()})}),
+              IsOk());
 
   auto eval_result = expression->Evaluate(activation, &arena);
 
@@ -171,7 +173,9 @@ TEST_F(ListKeysStepTest, UnknownSetPassedThrough) {
   Activation activation;
   Arena arena;
 
-  activation.set_unknown_attribute_patterns({CelAttributePattern("var", {})});
+  ASSERT_THAT(
+      activation.SetUnknownAttributePatterns({CelAttributePattern("var", {})}),
+      IsOk());
 
   auto eval_result = expression->Evaluate(activation, &arena);
 

eval/eval/container_access_step_test.cc

@@ -9,6 +9,7 @@
 #include "cel/expr/syntax.pb.h"
 #include "google/protobuf/struct.pb.h"
 #include "absl/base/nullability.h"
+#include "absl/log/absl_check.h"
 #include "absl/status/status.h"
 #include "base/builtins.h"
 #include "base/type_provider.h"
@@ -101,7 +102,7 @@ CelValue EvaluateAttributeHelper(
   activation.InsertValue("container", container);
   activation.InsertValue("key", key);
 
-  activation.set_unknown_attribute_patterns(patterns);
+  ABSL_CHECK_OK(activation.SetUnknownAttributePatterns(patterns));  // Crash OK
   auto result = cel_expr.Evaluate(activation, arena);
   return *result;
 }

eval/eval/create_list_step_test.cc

@@ -423,8 +423,10 @@ TEST(CreateDirectListStep, MissingAttribute) {
   cel::RuntimeOptions options;
   options.enable_missing_attribute_errors = true;
 
-  activation.SetMissingPatterns({cel::AttributePattern(
-      "var1", {cel::AttributeQualifierPattern::OfString("field1")})});
+  ASSERT_THAT(
+      activation.SetMissingPatterns({cel::AttributePattern(
+          "var1", {cel::AttributeQualifierPattern::OfString("field1")})}),
+      IsOk());
 
   ExecutionFrameBase frame(activation, options, type_provider,
                            cel::internal::GetTestingDescriptorPool(),
@@ -519,8 +521,10 @@ TEST(CreateDirectListStep, PartialUnknown) {
   cel::Activation activation;
   cel::RuntimeOptions options;
   options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly;
-  activation.SetUnknownPatterns({cel::AttributePattern(
-      "var1", {cel::AttributeQualifierPattern::OfString("field1")})});
+  ASSERT_THAT(
+      activation.SetUnknownPatterns({cel::AttributePattern(
+          "var1", {cel::AttributeQualifierPattern::OfString("field1")})}),
+      IsOk());
 
   ExecutionFrameBase frame(activation, options, type_provider,
                            cel::internal::GetTestingDescriptorPool(),

eval/eval/equality_steps_test.cc

@@ -94,8 +94,9 @@ TEST(RecursiveTest, PartialAttrUnknown) {
       std::make_unique<ValueStep>(IntValue(1), cel::Attribute("foo")),
       std::make_unique<ValueStep>(IntValue(2)), false, -1);
 
-  activation.SetUnknownPatterns({cel::AttributePattern(
-      "foo", {cel::AttributeQualifierPattern::OfString("bar")})});
+  ASSERT_THAT(activation.SetUnknownPatterns({cel::AttributePattern(
+                  "foo", {cel::AttributeQualifierPattern::OfString("bar")})}),
+              IsOk());
 
   ExecutionFrameBase frame(activation, opts, type_provider,
                            cel::internal::GetTestingDescriptorPool(),
@@ -120,8 +121,9 @@ TEST(RecursiveTest, PartialAttrUnknownDisabled) {
       std::make_unique<ValueStep>(IntValue(1), cel::Attribute("foo")),
       std::make_unique<ValueStep>(IntValue(2)), false, -1);
 
-  activation.SetUnknownPatterns({cel::AttributePattern(
-      "foo", {cel::AttributeQualifierPattern::OfString("bar")})});
+  ASSERT_THAT(activation.SetUnknownPatterns({cel::AttributePattern(
+                  "foo", {cel::AttributeQualifierPattern::OfString("bar")})}),
+              IsOk());
   ExecutionFrameBase frame(activation, opts, type_provider,
                            cel::internal::GetTestingDescriptorPool(),
                            cel::internal::GetTestingMessageFactory(), &arena);
@@ -154,8 +156,9 @@ TEST(IterativeTest, PartialAttrUnknown) {
       std::make_unique<ValueStep>(IntValue(2))));
   steps.push_back(ExpressionStep::MakeFastEqualStep());
 
-  activation.SetUnknownPatterns({cel::AttributePattern(
-      "foo", {cel::AttributeQualifierPattern::OfString("bar")})});
+  ASSERT_THAT(activation.SetUnknownPatterns({cel::AttributePattern(
+                  "foo", {cel::AttributeQualifierPattern::OfString("bar")})}),
+              IsOk());
 
   ExecutionFrame frame(steps, activation, opts, state);
 
@@ -185,8 +188,9 @@ TEST(IterativeTest, PartialAttrUnknownDisabled) {
       std::make_unique<ValueStep>(IntValue(2))));
   steps.push_back(ExpressionStep::MakeFastEqualStep());
 
-  activation.SetUnknownPatterns({cel::AttributePattern(
-      "foo", {cel::AttributeQualifierPattern::OfString("bar")})});
+  ASSERT_THAT(activation.SetUnknownPatterns({cel::AttributePattern(
+                  "foo", {cel::AttributeQualifierPattern::OfString("bar")})}),
+              IsOk());
   ExecutionFrame frame(steps, activation, opts, state);
 
   ASSERT_OK_AND_ASSIGN(Value result, frame.Evaluate());

eval/eval/function_step_test.cc

@@ -693,7 +693,7 @@ TEST_P(FunctionStepTestUnknowns, PartialUnknownHandlingTest) {
   // Set attribute pattern that marks attribute "param[true]" as unknown.
   // It should result in "param" being handled as partially unknown, which is
   // is handled as fully unknown when used as function input argument.
-  activation.set_unknown_attribute_patterns({pattern});
+  ASSERT_THAT(activation.SetUnknownAttributePatterns({pattern}), IsOk());
 
   ASSERT_OK_AND_ASSIGN(CelValue value, impl->Evaluate(activation, &arena));
   ASSERT_TRUE(value.IsUnknownSet());

eval/eval/ident_step_test.cc

@@ -114,7 +114,7 @@ TEST(IdentStepTest, DisableMissingAttributeErrorsOK) {
   EXPECT_THAT(result.StringOrDie().value(), Eq("test"));
 
   const CelAttributePattern pattern("name0", {});
-  activation.set_missing_attribute_patterns({pattern});
+  ASSERT_THAT(activation.SetMissingAttributePatterns({pattern}), IsOk());
 
   status0 = impl.Evaluate(activation, &arena);
   ASSERT_THAT(status0, IsOk());
@@ -151,7 +151,7 @@ TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) {
   EXPECT_THAT(result.StringOrDie().value(), Eq("test"));
 
   CelAttributePattern pattern("name0", {});
-  activation.set_missing_attribute_patterns({pattern});
+  ASSERT_THAT(activation.SetMissingAttributePatterns({pattern}), IsOk());
 
   status0 = impl.Evaluate(activation, &arena);
   ASSERT_THAT(status0, IsOk());
@@ -182,7 +182,7 @@ TEST(IdentStepTest, TestIdentStepUnknownAttribute) {
   std::vector<CelAttributePattern> unknown_patterns;
   unknown_patterns.push_back(CelAttributePattern("name_bad", {}));
 
-  activation.set_unknown_attribute_patterns(unknown_patterns);
+  ASSERT_THAT(activation.SetUnknownAttributePatterns(unknown_patterns), IsOk());
   auto status0 = impl.Evaluate(activation, &arena);
   ASSERT_THAT(status0, IsOk());
 
@@ -193,7 +193,7 @@ TEST(IdentStepTest, TestIdentStepUnknownAttribute) {
 
   unknown_patterns.push_back(CelAttributePattern("name0", {}));
 
-  activation.set_unknown_attribute_patterns(unknown_patterns);
+  ASSERT_THAT(activation.SetUnknownAttributePatterns(unknown_patterns), IsOk());
   status0 = impl.Evaluate(activation, &arena);
   ASSERT_THAT(status0, IsOk());
 
@@ -234,7 +234,9 @@ TEST(DirectIdentStepTest, UnknownAttribute) {
   options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly;
 
   activation.InsertOrAssignValue("var1", IntValue(42));
-  activation.SetUnknownPatterns({CreateCelAttributePattern("var1", {})});
+  ASSERT_THAT(
+      activation.SetUnknownPatterns({CreateCelAttributePattern("var1", {})}),
+      IsOk());
 
   ExecutionFrameBase frame(activation, options, type_provider,
                            cel::internal::GetTestingDescriptorPool(),
@@ -259,7 +261,9 @@ TEST(DirectIdentStepTest, MissingAttribute) {
   options.enable_missing_attribute_errors = true;
 
   activation.InsertOrAssignValue("var1", IntValue(42));
-  activation.SetMissingPatterns({CreateCelAttributePattern("var1", {})});
+  ASSERT_THAT(
+      activation.SetMissingPatterns({CreateCelAttributePattern("var1", {})}),
+      IsOk());
 
   ExecutionFrameBase frame(activation, options, type_provider,
                            cel::internal::GetTestingDescriptorPool(),