From 8d2bf66b16fa53233c8c4b177de45d659abac4a4 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Mon, 20 Jul 2026 11:14:56 -0700 Subject: [PATCH] Refactor: parameter order for ConvertTypeSpecToType For internal consistency (output parameter last). PiperOrigin-RevId: 950948076 --- common/signature.cc | 2 +- common/signature_test.cc | 9 +++---- common/type_spec_resolver.cc | 18 +++++++------- common/type_spec_resolver.h | 4 ++-- common/type_spec_resolver_test.cc | 39 ++++++++++++++++--------------- 5 files changed, 37 insertions(+), 35 deletions(-) diff --git a/common/signature.cc b/common/signature.cc index 54d312777..3f4ea8d29 100644 --- a/common/signature.cc +++ b/common/signature.cc @@ -634,7 +634,7 @@ absl::StatusOr ParseTypeSpec(std::string_view signature) { absl::StatusOr 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 diff --git a/common/signature_test.cc b/common/signature_test.cc index ea51eb566..e157d5be0 100644 --- a/common/signature_test.cc +++ b/common/signature_test.cc @@ -85,7 +85,7 @@ TEST_P(TypeSignatureTest, TypeSignature) { EXPECT_THAT(signature, IsOkAndHolds(param.expected_signature)); absl::StatusOr 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); } } diff --git a/common/type_spec_resolver.cc b/common/type_spec_resolver.cc index 90c9930a8..c3aa3d5a2 100644 --- a/common/type_spec_resolver.cc +++ b/common/type_spec_resolver.cc @@ -33,8 +33,8 @@ namespace cel { absl::StatusOr 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 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 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 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 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 ConvertTypeSpecToType(const TypeSpec& type_spec, std::vector 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(arena, name); @@ -187,7 +187,7 @@ absl::StatusOr 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)); } diff --git a/common/type_spec_resolver.h b/common/type_spec_resolver.h index edbfa3bde..2cd860f02 100644 --- a/common/type_spec_resolver.h +++ b/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 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 ConvertTypeToTypeSpec(const Type& type); diff --git a/common/type_spec_resolver_test.cc b/common/type_spec_resolver_test.cc index 1cda7280f..dbde63e6d 100644 --- a/common/type_spec_resolver_test.cc +++ b/common/type_spec_resolver_test.cc @@ -49,7 +49,7 @@ 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()); } @@ -57,7 +57,7 @@ TEST(TypeSpecResolverTest, NullTypeSpec) { 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>; 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(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(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(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,7 +268,7 @@ 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"))); } @@ -275,7 +276,7 @@ TEST(TypeSpecResolverTest, EnumTypeWithParamsError) { 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"))); }