Return somewhat expected errors as cel::ErrorValue instead of absl::Status

PiperOrigin-RevId: 688594318

Author: jcking

common/values/error_value.cc

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <cstddef>
 #include <string>
 #include <utility>
 
@@ -79,6 +80,16 @@ ErrorValue TypeConversionError(const Type& from, const Type& to) {
   return TypeConversionError(from.DebugString(), to.DebugString());
 }
 
+ErrorValue IndexOutOfBoundsError(size_t index) {
+  return ErrorValue(
+      absl::InvalidArgumentError(absl::StrCat("index out of bounds: ", index)));
+}
+
+ErrorValue IndexOutOfBoundsError(ptrdiff_t index) {
+  return ErrorValue(
+      absl::InvalidArgumentError(absl::StrCat("index out of bounds: ", index)));
+}
+
 bool IsNoSuchField(const ErrorValue& value) {
   return absl::IsNotFound(value.NativeValue()) &&
          absl::StartsWith(value.NativeValue().message(), "no_such_field");

common/values/error_value.h

@@ -18,8 +18,10 @@
 #ifndef THIRD_PARTY_CEL_CPP_COMMON_VALUES_ERROR_VALUE_H_
 #define THIRD_PARTY_CEL_CPP_COMMON_VALUES_ERROR_VALUE_H_
 
+#include <cstddef>
 #include <ostream>
 #include <string>
+#include <type_traits>
 #include <utility>
 
 #include "absl/base/nullability.h"
@@ -123,6 +125,29 @@ ErrorValue TypeConversionError(absl::string_view from, absl::string_view to);
 
 ErrorValue TypeConversionError(const Type& from, const Type& to);
 
+ErrorValue IndexOutOfBoundsError(size_t index);
+
+ErrorValue IndexOutOfBoundsError(ptrdiff_t index);
+
+// Catch other integrals and forward them to the above ones. This is needed to
+// avoid ambiguous overload issues for smaller integral types like `int`.
+template <typename T>
+std::enable_if_t<std::conjunction_v<std::is_integral<T>, std::is_unsigned<T>,
+                                    std::negation<std::is_same<T, size_t>>>,
+                 ErrorValue>
+IndexOutOfBoundsError(T index) {
+  static_assert(sizeof(T) <= sizeof(size_t));
+  return IndexOutOfBoundsError(static_cast<size_t>(index));
+}
+template <typename T>
+std::enable_if_t<std::conjunction_v<std::is_integral<T>, std::is_signed<T>,
+                                    std::negation<std::is_same<T, ptrdiff_t>>>,
+                 ErrorValue>
+IndexOutOfBoundsError(T index) {
+  static_assert(sizeof(T) <= sizeof(ptrdiff_t));
+  return IndexOutOfBoundsError(static_cast<ptrdiff_t>(index));
+}
+
 inline std::ostream& operator<<(std::ostream& out, const ErrorValue& value) {
   return out << value.DebugString();
 }

common/values/list_value_test.cc

@@ -24,7 +24,6 @@
 #include "common/json.h"
 #include "common/memory.h"
 #include "common/type.h"
-#include "common/type_factory.h"
 #include "common/value.h"
 #include "common/value_testing.h"
 #include "internal/status_macros.h"
@@ -35,6 +34,7 @@ namespace {
 
 using ::absl_testing::IsOkAndHolds;
 using ::absl_testing::StatusIs;
+using ::cel::test::ErrorValueIs;
 using ::testing::ElementsAreArray;
 using ::testing::TestParamInfo;
 
@@ -107,8 +107,9 @@ TEST_P(ListValueTest, Get) {
   ASSERT_OK_AND_ASSIGN(element, value.Get(value_manager(), 2));
   ASSERT_TRUE(InstanceOf<IntValue>(element));
   ASSERT_EQ(Cast<IntValue>(element).NativeValue(), 2);
-  EXPECT_THAT(value.Get(value_manager(), 3),
-              StatusIs(absl::StatusCode::kInvalidArgument));
+  EXPECT_THAT(
+      value.Get(value_manager(), 3),
+      IsOkAndHolds(ErrorValueIs(StatusIs(absl::StatusCode::kInvalidArgument))));
 }
 
 TEST_P(ListValueTest, ForEach) {

common/values/map_value_test.cc

@@ -37,6 +37,7 @@ namespace {
 using ::absl_testing::IsOk;
 using ::absl_testing::IsOkAndHolds;
 using ::absl_testing::StatusIs;
+using ::cel::test::ErrorValueIs;
 using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::TestParamInfo;
@@ -150,8 +151,9 @@ TEST_P(MapValueTest, Get) {
   ASSERT_OK_AND_ASSIGN(value, map_value.Get(value_manager(), IntValue(2)));
   ASSERT_TRUE(InstanceOf<DoubleValue>(value));
   ASSERT_EQ(Cast<DoubleValue>(value).NativeValue(), 5.0);
-  EXPECT_THAT(map_value.Get(value_manager(), IntValue(3)),
-              StatusIs(absl::StatusCode::kNotFound));
+  EXPECT_THAT(
+      map_value.Get(value_manager(), IntValue(3)),
+      IsOkAndHolds(ErrorValueIs(StatusIs(absl::StatusCode::kNotFound))));
 }
 
 TEST_P(MapValueTest, Find) {

common/values/mutable_list_value_test.cc

@@ -38,6 +38,7 @@ namespace {
 using ::absl_testing::IsOk;
 using ::absl_testing::IsOkAndHolds;
 using ::absl_testing::StatusIs;
+using ::cel::test::ErrorValueIs;
 using ::cel::test::StringValueIs;
 using ::testing::IsEmpty;
 using ::testing::Pair;
@@ -156,8 +157,9 @@ TEST_P(MutableListValueTest, Get) {
   auto mutable_list_value = NewMutableListValue(allocator());
   mutable_list_value->Reserve(1);
   Value value;
-  EXPECT_THAT(mutable_list_value->Get(value_manager(), 0, value),
-              StatusIs(absl::StatusCode::kInvalidArgument));
+  EXPECT_THAT(mutable_list_value->Get(value_manager(), 0, value), IsOk());
+  EXPECT_THAT(value,
+              ErrorValueIs(StatusIs(absl::StatusCode::kInvalidArgument)));
   EXPECT_THAT(mutable_list_value->Append(StringValue("foo")), IsOk());
   EXPECT_THAT(mutable_list_value->Get(value_manager(), 0, value), IsOk());
   EXPECT_THAT(value, StringValueIs("foo"));

common/values/parsed_json_list_value.cc

@@ -149,13 +149,15 @@ size_t ParsedJsonListValue::Size() const {
 absl::Status ParsedJsonListValue::Get(ValueManager& value_manager, size_t index,
                                       Value& result) const {
   if (value_ == nullptr) {
-    return absl::InvalidArgumentError("index out of bounds");
+    result = IndexOutOfBoundsError(index);
+    return absl::OkStatus();
   }
   const auto reflection =
       well_known_types::GetListValueReflectionOrDie(value_->GetDescriptor());
   if (ABSL_PREDICT_FALSE(index >=
                          static_cast<size_t>(reflection.ValuesSize(*value_)))) {
-    return absl::InvalidArgumentError("index out of bounds");
+    result = IndexOutOfBoundsError(index);
+    return absl::OkStatus();
   }
   result = common_internal::ParsedJsonValue(
       value_manager.GetMemoryManager().arena(),

common/values/parsed_json_list_value_test.cc

@@ -50,6 +50,7 @@ using ::absl_testing::StatusIs;
 using ::cel::internal::GetTestingDescriptorPool;
 using ::cel::internal::GetTestingMessageFactory;
 using ::cel::test::BoolValueIs;
+using ::cel::test::ErrorValueIs;
 using ::cel::test::IsNullValue;
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
@@ -193,8 +194,9 @@ TEST_P(ParsedJsonListValueTest, Get_Dynamic) {
   EXPECT_THAT(valid_value.Get(value_manager(), 0), IsOkAndHolds(IsNullValue()));
   EXPECT_THAT(valid_value.Get(value_manager(), 1),
               IsOkAndHolds(BoolValueIs(true)));
-  EXPECT_THAT(valid_value.Get(value_manager(), 2),
-              StatusIs(absl::StatusCode::kInvalidArgument));
+  EXPECT_THAT(
+      valid_value.Get(value_manager(), 2),
+      IsOkAndHolds(ErrorValueIs(StatusIs(absl::StatusCode::kInvalidArgument))));
 }
 
 TEST_P(ParsedJsonListValueTest, ForEach_Dynamic) {

common/values/parsed_json_map_value.cc

@@ -26,7 +26,6 @@
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/cord.h"
-#include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 #include "common/allocator.h"
 #include "common/json.h"
@@ -153,8 +152,7 @@ absl::Status ParsedJsonMapValue::Get(ValueManager& value_manager,
                                      const Value& key, Value& result) const {
   CEL_ASSIGN_OR_RETURN(bool ok, Find(value_manager, key, result));
   if (ABSL_PREDICT_FALSE(!ok) && !(result.IsError() || result.IsUnknown())) {
-    return absl::NotFoundError(
-        absl::StrCat("Key not found in map : ", key.DebugString()));
+    result = NoSuchKeyError(key.DebugString());
   }
   return absl::OkStatus();
 }

common/values/parsed_json_map_value_test.cc

@@ -50,6 +50,7 @@ using ::absl_testing::StatusIs;
 using ::cel::internal::GetTestingDescriptorPool;
 using ::cel::internal::GetTestingMessageFactory;
 using ::cel::test::BoolValueIs;
+using ::cel::test::ErrorValueIs;
 using ::cel::test::IsNullValue;
 using ::cel::test::StringValueIs;
 using ::testing::AnyOf;
@@ -202,14 +203,16 @@ TEST_P(ParsedJsonMapValueTest, Get_Dynamic) {
                  key: "bar"
                  value: { bool_value: true }
                })pb"));
-  EXPECT_THAT(valid_value.Get(value_manager(), BoolValue()),
-              StatusIs(absl::StatusCode::kNotFound));
+  EXPECT_THAT(
+      valid_value.Get(value_manager(), BoolValue()),
+      IsOkAndHolds(ErrorValueIs(StatusIs(absl::StatusCode::kNotFound))));
   EXPECT_THAT(valid_value.Get(value_manager(), StringValue("foo")),
               IsOkAndHolds(IsNullValue()));
   EXPECT_THAT(valid_value.Get(value_manager(), StringValue("bar")),
               IsOkAndHolds(BoolValueIs(true)));
-  EXPECT_THAT(valid_value.Get(value_manager(), StringValue("baz")),
-              StatusIs(absl::StatusCode::kNotFound));
+  EXPECT_THAT(
+      valid_value.Get(value_manager(), StringValue("baz")),
+      IsOkAndHolds(ErrorValueIs(StatusIs(absl::StatusCode::kNotFound))));
 }
 
 TEST_P(ParsedJsonMapValueTest, Find_Dynamic) {

common/values/parsed_list_value.cc

@@ -134,7 +134,8 @@ absl::Status ParsedListValueInterface::SerializeTo(
 absl::Status ParsedListValueInterface::Get(ValueManager& value_manager,
                                            size_t index, Value& result) const {
   if (ABSL_PREDICT_FALSE(index >= Size())) {
-    return absl::InvalidArgumentError("index out of bounds");
+    result = IndexOutOfBoundsError(index);
+    return absl::OkStatus();
   }
   return GetImpl(value_manager, index, result);
 }