Protobuf: APIv2: reflect/protoreflect: decide descriptor immutability approach

[dsnet]

The descriptor API is intended to be immutable for all intents and purposes. For that reason, it heavily uses opaque types that only has accessor methods prevent users from possibly mutating the underlying values.

However, the API leaks in its immutability abstraction in the following areas:

• Descriptor.Options and MessageDescriptor.ExtensionRangeOptions return a pointer to the options message (e.g., *descriptorpb.FileOptions).
• FieldDescriptor.Default returns a reference to the default []byte for bytes field.
• SourceLocation.Path and SourceLocation.LeadingDetachingComments return a []int32 and []string.

In the above situations, it simply documents that the user must not mutate the returned value. This issue is to decide whether to keep that behavior. Whatever we do, I'd prefer if we were consistent in how immutability is handled in all the above situations.

Option 1: Documented "immutability"

We could stay with what we have today and simply document that the user must not mutate the returned values.

Properties:

• :heavy_check_mark: This is simple.
• :heavy_check_mark: This is performant since we don't need to copy the values.
• :heavy_multiplication_x: Accidental mutations can have obscure effects on remote parts of the program, which can be very hard to debug.

Options 2: Opaque read-only types

We could define more opaque types that are read-only abstractions for the above:

• For options, we could return a protoreflect.Message that is wrapped to be read-only. If a user wants a concrete options message (e.g., *descriptorpb.FileOptions they will have to clone the message.
• For default bytes, we could just use a Go string to represent both the protobuf string and bytes types in our reflection API.
• For source locations, we make the SourcePath an opaque type with a Len and Get method. We do something similar to represent a []string.

Properties:

• :heavy_check_mark: This is often performant since we normally don't need to copy the values. However, it does not fix all cases.
• :heavy_multiplication_x: It's complicated and cumbersome
• :heavy_check_mark: It provides water-tight immutability.

Options 3: Copy mutable values

We could just copy all values that are mutable so that user mutations do not affect the original backing store.

Properties:

• :heavy_check_mark: This is simple.
• :heavy_multiplication_x: This is not performant.
• :heavy_check_mark: It provides water-tight immutability.

---

Vote using emojis on the following comments.

\cc @neild @cybrcodr @jhump @puellanivis @pedgeio

[dsnet]

Vote for Option 1: Documented "immutability"

[dsnet]

Vote for Option 2: Opaque read-only types

[dsnet]

Vote for Option 3: Copy mutable values

[bufdev]

• Vote for Option 1 for Descriptor.Options
• Vote for Option 2 for FieldDescriptor.Default
• Vote for either Option 2 or 3 for SourceLocation.LeadingDetachedComments (I'm on the fence)
• Vote to delete SourceLocation.Path

Reasoning:

• I think Descriptor.Options returning the underlying proto.Message is understood in general to be something you shouldn't mutate, maybe I use Protobuf APIs too much.
• I think FieldDescriptor.Default will be used infrequently enough that it's ok to make it a little more complicated to use.
• I don't know about SourceLocation.LeadingDetachedComments, maybe same as FieldDescriptor.Default in that I think it'll be used infrequently enough that maybe it's worth just wrapping.
• I don't think SourceLocation.Path serves much of a purpose, or perhaps if it does and someone wants it in the future, you can re-add it.

Edit: I didn't read the manual ie voting on the options...but then again, I guess I'm voting for both 1 and 2 in a way heh. If I had to choose only one, it'd be 2.

[dsnet]

I don't think SourceLocation.Path serves much of a purpose, or perhaps if it does and someone wants it in the future, you can re-add it.

It's necessary to round trip descriptorpb.FileDescriptor <-> protoreflect.FileDescriptor since you need to obtain the path somehow when converting.

[bufdev]

Ah, that is true, and annoying, missed that.

Re: https://go-review.googlesource.com/c/protobuf/+/183157/1/reflect/protoreflect/source.go#15 I think we could all be happy with option 2 since it'll result in an interface in effect to match the rest of the API

[neild]

I vote for option 1. The loss of performance and convenience in the other options is too high.

We can hope for futures ways of improving the safety of option 1. Perhaps someday we add read-only messages (which would make many people happy), which would address Descriptor.Options. Perhaps someday Go gets read-only slices, which would address FieldDescriptor.Default.

[bufdev]

I vote for option 1. The loss of performance and convenience in the other options is too high.

We can hope for futures ways of improving the safety of option 1. Perhaps someday we add read-only messages (which would make many people happy), which would address Descriptor.Options. Perhaps someday Go gets read-only slices, which would address FieldDescriptor.Default.

I think for Descriptor.Options this makes sense for sure - really option 1 is fine for anything, I don't think many of these things are going to be used much and if you're using them, you should know what you are doing. However, I still vote option 2 for SourceLocation just because it makes it an interface which satisfies my concerns about an empty SourceLocation being returned :-)

[puellanivis]

Everyone makes good arguments here, in general I like Option 1. We can only hold put up fences so much, until we start making people’s lives miserable in an effort to “protect” them from themselves.

[jhump]

My preference is also option 1. The performance hit of option 3 seems way too high (just a feel, no measurements).

Option 2 is also appealing though. I agree about having consistency throughout the APIs, whichever pattern is chosen. To some degree, that may be an argument for option 2, since it basically extends existing patterns to more places in the model.

[dsnet]

Marking as closed. We're going with option 1.