Refactor: parameter order for ConvertTypeSpecToType

common/signature.cc

@@ -634,7 +634,7 @@ absl::StatusOr<TypeSpec> ParseTypeSpec(std::string_view signature) {
 absl::StatusOr<Type> ParseType(std::string_view signature, google::protobuf::Arena* arena,
                                const google::protobuf::DescriptorPool& pool) {
   CEL_ASSIGN_OR_RETURN(auto type_spec, ParseTypeSpec(signature));
-  return cel::ConvertTypeSpecToType(type_spec, arena, pool);
+  return cel::ConvertTypeSpecToType(type_spec, pool, arena);
 }
 
 }  // namespace cel

common/signature_test.cc

@@ -85,7 +85,7 @@ TEST_P(TypeSignatureTest, TypeSignature) {
     EXPECT_THAT(signature, IsOkAndHolds(param.expected_signature));
 
     absl::StatusOr<Type> type = ConvertTypeSpecToType(
-        param.type, GetTestArena(), *GetTestingDescriptorPool());
+        param.type, *GetTestingDescriptorPool(), GetTestArena());
     ASSERT_THAT(type, ::absl_testing::IsOk());
     EXPECT_THAT(MakeTypeSignature(*type),
                 IsOkAndHolds(param.expected_signature));
@@ -285,9 +285,10 @@ TEST_P(TypeSignatureTest, ParseTypeCheck) {
     auto parsed = ParseType(param.expected_signature, GetTestArena(),
                             *GetTestingDescriptorPool());
     ASSERT_THAT(parsed, ::absl_testing::IsOk());
-    ASSERT_OK_AND_ASSIGN(auto expected_type,
-                         ConvertTypeSpecToType(param.type, GetTestArena(),
-                                               *GetTestingDescriptorPool()));
+    ASSERT_OK_AND_ASSIGN(
+        auto expected_type,
+        ConvertTypeSpecToType(param.type, *GetTestingDescriptorPool(),
+                              GetTestArena()));
     VerifyTypesEqual(*parsed, expected_type);
   }
 }

common/type_spec_resolver.cc

@@ -33,8 +33,8 @@
 namespace cel {
 
 absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
-                                           google::protobuf::Arena* arena,
-                                           const google::protobuf::DescriptorPool& pool) {
+                                           const google::protobuf::DescriptorPool& pool,
+                                           google::protobuf::Arena* arena) {
   if (type_spec.has_null()) return Type(NullType{});
   if (type_spec.has_dyn()) return Type(DynType{});
 
@@ -94,7 +94,7 @@ absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
     if (type_spec.list_type().elem_type().is_specified()) {
       CEL_ASSIGN_OR_RETURN(
           elem_type, ConvertTypeSpecToType(type_spec.list_type().elem_type(),
-                                           arena, pool));
+                                           pool, arena));
     }
     return Type(ListType(arena, elem_type));
   }
@@ -104,14 +104,14 @@ absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
     if (type_spec.map_type().key_type().is_specified()) {
       CEL_ASSIGN_OR_RETURN(
           key_type,
-          ConvertTypeSpecToType(type_spec.map_type().key_type(), arena, pool));
+          ConvertTypeSpecToType(type_spec.map_type().key_type(), pool, arena));
     }
 
     Type value_type;
     if (type_spec.map_type().value_type().is_specified()) {
       CEL_ASSIGN_OR_RETURN(
           value_type, ConvertTypeSpecToType(type_spec.map_type().value_type(),
-                                            arena, pool));
+                                            pool, arena));
     }
     return Type(MapType(arena, key_type, value_type));
   }
@@ -122,13 +122,13 @@ absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
     if (func_spec.result_type().is_specified()) {
       CEL_ASSIGN_OR_RETURN(
           result_type,
-          ConvertTypeSpecToType(func_spec.result_type(), arena, pool));
+          ConvertTypeSpecToType(func_spec.result_type(), pool, arena));
     }
     std::vector<Type> arg_types;
     arg_types.reserve(func_spec.arg_types().size());
     for (const auto& arg_spec : func_spec.arg_types()) {
       CEL_ASSIGN_OR_RETURN(auto arg_type,
-                           ConvertTypeSpecToType(arg_spec, arena, pool));
+                           ConvertTypeSpecToType(arg_spec, pool, arena));
       arg_types.push_back(std::move(arg_type));
     }
     return Type(FunctionType(arena, result_type, arg_types));
@@ -178,7 +178,7 @@ absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
     std::vector<Type> params;
     for (const auto& param_spec : type_spec.abstract_type().parameter_types()) {
       CEL_ASSIGN_OR_RETURN(auto param,
-                           ConvertTypeSpecToType(param_spec, arena, pool));
+                           ConvertTypeSpecToType(param_spec, pool, arena));
       params.push_back(std::move(param));
     }
     auto* allocated_name = google::protobuf::Arena::Create<std::string>(arena, name);
@@ -187,7 +187,7 @@ absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
 
   if (type_spec.has_type()) {
     CEL_ASSIGN_OR_RETURN(auto contained_type,
-                         ConvertTypeSpecToType(type_spec.type(), arena, pool));
+                         ConvertTypeSpecToType(type_spec.type(), pool, arena));
     return Type(TypeType(arena, contained_type));
   }
 

common/type_spec_resolver.h

@@ -29,8 +29,8 @@ namespace cel {
 // properties of the type when used in CEL. Returns a status with code
 // `InvalidArgument` if the input cannot be resolved to a type.
 absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
-                                           google::protobuf::Arena* arena,
-                                           const google::protobuf::DescriptorPool& pool);
+                                           const google::protobuf::DescriptorPool& pool,
+                                           google::protobuf::Arena* arena);
 
 // Resolves a `cel::Type` to a `cel::TypeSpec`.
 absl::StatusOr<TypeSpec> ConvertTypeToTypeSpec(const Type& type);

common/type_spec_resolver_test.cc

@@ -49,15 +49,15 @@ google::protobuf::Arena* GetTestArena() {
 TEST(TypeSpecResolverTest, NullTypeSpec) {
   TypeSpec spec(NullTypeSpec{});
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsNull());
 }
 
 TEST(TypeSpecResolverTest, DynTypeSpec) {
   TypeSpec spec(DynTypeSpec{});
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsDyn());
 }
@@ -66,8 +66,9 @@ using ConversionTest = testing::TestWithParam<std::tuple<TypeSpec, TypeKind>>;
 
 TEST_P(ConversionTest, TestTypeSpecConversion) {
   ASSERT_OK_AND_ASSIGN(
-      auto t, ConvertTypeSpecToType(std::get<0>(GetParam()), GetTestArena(),
-                                    *GetTestingDescriptorPool()));
+      auto t,
+      ConvertTypeSpecToType(std::get<0>(GetParam()),
+                            *GetTestingDescriptorPool(), GetTestArena()));
   EXPECT_EQ(t.kind(), std::get<1>(GetParam()));
   EXPECT_THAT(ConvertTypeToTypeSpec(t), IsOkAndHolds(std::get<0>(GetParam())));
 }
@@ -103,7 +104,7 @@ TEST(TypeSpecResolverTest, ListTypeConversion) {
   auto elem = std::make_unique<TypeSpec>(PrimitiveType::kInt64);
   TypeSpec spec(ListTypeSpec(std::move(elem)));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsList());
   EXPECT_TRUE(t->GetList().element().IsInt());
@@ -116,7 +117,7 @@ TEST(TypeSpecResolverTest, MapTypeConversion) {
   auto val = std::make_unique<TypeSpec>(PrimitiveType::kBytes);
   TypeSpec spec(MapTypeSpec(std::move(key), std::move(val)));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsMap());
   EXPECT_TRUE(t->GetMap().key().IsString());
@@ -131,7 +132,7 @@ TEST(TypeSpecResolverTest, FunctionTypeConversion) {
   args.push_back(TypeSpec(PrimitiveType::kString));
   TypeSpec spec(FunctionTypeSpec(std::move(result), std::move(args)));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsFunction());
   EXPECT_EQ(t->GetFunction().args().size(), 1);
@@ -143,7 +144,7 @@ TEST(TypeSpecResolverTest, FunctionTypeConversion) {
 TEST(TypeSpecResolverTest, TypeParamConversion) {
   TypeSpec spec(ParamTypeSpec("T"));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsTypeParam());
   EXPECT_EQ(t->GetTypeParam().name(), "T");
@@ -155,7 +156,7 @@ TEST(TypeSpecResolverTest, MessageTypeConversion) {
   TypeSpec spec(
       AbstractType("cel.expr.conformance.proto3.TestAllTypes", /*params=*/{}));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsMessage());
   EXPECT_EQ(t->name(), "cel.expr.conformance.proto3.TestAllTypes");
@@ -171,7 +172,7 @@ TEST(TypeSpecResolverTest, MessageTypeWithParamsError) {
   TypeSpec spec(AbstractType("cel.expr.conformance.proto3.TestAllTypes",
                              std::move(params)));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument,
                           HasSubstr("cannot have type parameters")));
 }
@@ -181,7 +182,7 @@ TEST(TypeSpecResolverTest, UnresolvedAbstractTypeFallbackToOpaque) {
   params.push_back(TypeSpec(PrimitiveType::kInt64));
   TypeSpec spec(AbstractType("my.custom.OpaqueType", std::move(params)));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsOpaque());
   EXPECT_EQ(t->name(), "my.custom.OpaqueType");
@@ -196,7 +197,7 @@ TEST(TypeSpecResolverTest, OptionalType) {
   params.push_back(TypeSpec(PrimitiveType::kInt64));
   TypeSpec spec(AbstractType("optional_type", std::move(params)));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsOpaque());
   EXPECT_EQ(t->name(), "optional_type");
@@ -211,7 +212,7 @@ TEST(TypeSpecResolverTest, TypeTypeConversion) {
   auto nested = std::make_unique<TypeSpec>(PrimitiveType::kInt64);
   TypeSpec spec(std::move(nested));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsType());
   EXPECT_TRUE(t->GetType().GetType().IsInt());
@@ -222,7 +223,7 @@ TEST(TypeSpecResolverTest, TypeTypeConversion) {
 TEST(TypeSpecResolverTest, ErrorTypeConversion) {
   TypeSpec spec(ErrorTypeSpec::kValue);
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsError());
   ASSERT_OK_AND_ASSIGN(auto spec2, ConvertTypeToTypeSpec(*t));
@@ -232,7 +233,7 @@ TEST(TypeSpecResolverTest, ErrorTypeConversion) {
 TEST(TypeSpecResolverTest, MessageTypeSpecConversion) {
   TypeSpec spec(MessageTypeSpec("cel.expr.conformance.proto3.TestAllTypes"));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsMessage());
   EXPECT_EQ(t->name(), "cel.expr.conformance.proto3.TestAllTypes");
@@ -243,7 +244,7 @@ TEST(TypeSpecResolverTest, MessageTypeSpecConversion) {
 TEST(TypeSpecResolverTest, MessageTypeSpecNotFoundError) {
   TypeSpec spec(MessageTypeSpec("cel.expr.conformance.proto3.NonExistentType"));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument,
                           HasSubstr("not found in descriptor pool")));
 }
@@ -252,7 +253,7 @@ TEST(TypeSpecResolverTest, EnumTypeConversion) {
   TypeSpec spec(AbstractType(
       "cel.expr.conformance.proto3.TestAllTypes.NestedEnum", /*params=*/{}));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   ASSERT_THAT(t, IsOk());
   EXPECT_TRUE(t->IsEnum());
   EXPECT_EQ(t->name(), "cel.expr.conformance.proto3.TestAllTypes.NestedEnum");
@@ -267,15 +268,15 @@ TEST(TypeSpecResolverTest, EnumTypeWithParamsError) {
       AbstractType("cel.expr.conformance.proto3.TestAllTypes.NestedEnum",
                    std::move(params)));
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument,
                           HasSubstr("cannot have type parameters")));
 }
 
 TEST(TypeSpecResolverTest, UnknownTypeSpecKindError) {
   TypeSpec spec;
   auto t =
-      ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
+      ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
   EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument,
                           HasSubstr("Unknown TypeSpec kind")));
 }

eval/compiler/flat_expr_builder.cc

@@ -976,14 +976,14 @@ class FlatExprVisitor : public cel::AstVisitor {
       return;
     }
 
+    StringValue field = cel::StringValue(select_expr.field());
     if (auto depth = RecursionEligible(); depth.has_value()) {
       auto deps = ExtractRecursiveDependencies();
       if (deps.size() != 1) {
         SetProgressStatusIfError(absl::InternalError(
             "unexpected number of dependencies for select operation."));
         return;
       }
-      StringValue field = cel::StringValue(select_expr.field());
 
       SetRecursiveStep(
           CreateDirectSelectStep(std::move(deps[0]), std::move(field),
@@ -994,9 +994,9 @@ class FlatExprVisitor : public cel::AstVisitor {
       return;
     }
 
-    AddStep(CreateSelectStep(select_expr, expr.id(),
-                             options_.enable_empty_wrapper_null_unboxing,
-                             enable_optional_types_));
+    AddStep(CreateSelectStep(
+        std::move(field), select_expr.test_only(), expr.id(),
+        options_.enable_empty_wrapper_null_unboxing, enable_optional_types_));
   }
 
   // Call node handler group.