protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

protoc-gen-go: generate ExtensionDescriptor instead of ExtensionType

Open dsnet opened this issue 5 years ago • 4 comments

For extensions, we currently generate E_ExtensionField variables of the ExtensionType type. This is unfortunate since there thousands of usages where people want to access the extension name or number. The new API would require people to do something like:

foopb.E_MyExtension.TypeDescriptor().Number()

Given how common it is that people want access to descriptor information, it seems unfortunate to keep going through the TypeDescriptor method. Perhaps we should generate something that implements the ExtensionTypeDescriptor type instead. Thus, people can directly access the information they want as:

foopb.E_MyExtension.Number()

Of course, we can't change the type of E_MyExtension, so this will need a new name (perhaps MyExtension_field?).

If newly generated variable to represent extension fields is an ExtensionTypeDescriptor, then perhaps the GetExtension, SetExtension, HasExtension, ClearExtension functions in the proto package should keyed by ExtensionTypeDescriptor instead of ExtensionType.

dsnet avatar Jan 16 '20 00:01 dsnet

It’s hard to say there is any empirically better choice here. Yet Another Field that duplicates some of the utility of another preexisting field? Or a regularly used and annoying call signature because the utility of the preexisting field is only accessible via transitive access through a method.

puellanivis avatar Jan 16 '20 08:01 puellanivis

My thinking was to introduce the MyExtension_field variable as the long-term replacement for E_MyExtension and mark E_MyExtension as deprecated.

dsnet avatar Jan 16 '20 18:01 dsnet

Sounds like a reasonable path towards a good solution. The E_ prefix is not exactly transparent, after all.

puellanivis avatar Jan 17 '20 13:01 puellanivis

We decided that this does not block v2 release. We still want to do this, but not at the present moment.

Our rationale is as follows:

  • It is already the case that generated code have a E_MyExtension variable that is a protoreflect.ExtensionType. It is also already the case that users are expected to interact with extensions using the proto.{Get,Set,Has,Clear}Extension functions using that variable. Therefore, the top-level extension functions taking in a protoreflect.ExtensionType is probably the right API.
  • In the future, we would still want to generate a different variable (e.g., MyExtension_field) that implements protoreflect.ExtensionTypeDescriptor, but also has methods on it that allows a user to access the extension field in a type-safe way. If we do that, users won't need the top-level proto functions for dealing with extensions, and it won't matter whether they take in a protoreflect.ExtensionTypeDescriptor or not. See https://go-review.googlesource.com/c/protobuf/+/183337 for an example of how the generated code can directly make experience with extension fields much better.

dsnet avatar Feb 11 '20 23:02 dsnet