Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

desc.Descriptor wraps protoreflect.Descriptor #354

Merged
merged 17 commits into from
Jan 11, 2023
Merged

Conversation

jhump
Copy link
Owner

@jhump jhump commented Sep 12, 2020

This is a major overhaul that enables full interop with the protoreflect package in the newer Go protobuf API. It makes it easy to convert between desc.Descriptor types and protoreflect.Descriptor types.

It was also necessary to make significant changes to protoparse, because it would build desc.Descriptor instances during linking, that were intermediate artifacts. But that can't work now that desc.Descriptor is a wrapper around protoreflect.Descriptor because the functions to create the protoreflect.Descriptor instances do more validation that cannot succeed at this phase of linking (because options have not yet been interpreted). I had already solved this by creating unexported implementations of the various protoreflect.Descriptor interfaces, that wrap descriptor protos and linker state. This means that interpreting options uses dynamicpb instead of the dynamic package in this repo. All of that is actually implemented in protoparse's successor: http://github.com/bufbuild/protocompile. So protoparse in this repo, as of this PR, now wraps protocompile. 🤯

While fixing some strange issues in the protoprint package that occurred after the protoparse overhaul, I ended up re-writing how it handles rendering custom options and message literals. It no longer uses the proto package's text marshaling functionality but instead has its own custom test marshaling (which also allows it to address how to render qualified names in extensions in message literals and also provides much greater control over formatting and where to put
line breaks, allowing the output to be both readable and more compact).

Resolves #301, #444, and #531.

@timruffles
Copy link

I've found this library really useful so far, but would love to be able to use it with the newer APIs (e.g. google.golang.org/protobuf/types/dynamicpb). Looks like this got paused a while back. Any plans to pick it up again, or would this be usable as it stands with the provisos you've noted?

@jhump
Copy link
Owner Author

jhump commented Aug 1, 2022

Looks like this got paused a while back. Any plans to pick it up again, or would this be usable as it stands with the provisos you've noted?

@timruffles, sorry for the very late reply. I was taking a sabbatical for several months, including in April when you wrote this.

My plan is to focus more on https://github.com/jhump/protocompile, a new API for parsing/compiling protos that directly uses the new API v2 libraries. Once that is done and I've got it integrated into https://github.com/bufbuild/buf (replacing the use of desc/protoparse in this repo), I will come back and revisit this arc of work.

@jhump
Copy link
Owner Author

jhump commented Nov 1, 2022

I still need to run apidiff on this to make sure that I haven't accidentally broken the exported API.

I don't want this to be a v2 of these packages: for a v2, I plan to intentionally break backwards-compatibility and even remove stuff in here that now overlaps with the v2 API of the protobuf runtime (desc, codec, and dynamic).

jhump and others added 13 commits January 11, 2023 09:35
- canonicalized imports to use google.golang.org/protobuf/types
- some other minor changes related to using no protobuf runtime
- most other packages still need updates
- adds new entry points to desc/sourceinfo so the new wrapped
  descriptor caching works correctly and still returns source
  code info if available
…ge sets; extra work in converting an API v2 message with extensions)

- updates to sourceinfo: can't unconditionally wrap or else things fall done in desc
  with wrapper caches
@jhump jhump changed the title WIP: desc.Descriptor wraps protoreflect.Descriptor desc.Descriptor wraps protoreflect.Descriptor Jan 11, 2023
@jhump
Copy link
Owner Author

jhump commented Jan 11, 2023

I've finally confirmed with apidiff that this should be a compatible change -- no incompatible changes made to exported APIs. The only changes are below (all additions, which are compatible changes):

github.com/jhump/protoreflect/desc:
Compatible changes:
- (*EnumDescriptor).Unwrap: added
- (*EnumDescriptor).UnwrapEnum: added
- (*EnumValueDescriptor).Unwrap: added
- (*EnumValueDescriptor).UnwrapEnumValue: added
- (*FieldDescriptor).Unwrap: added
- (*FieldDescriptor).UnwrapField: added
- (*FileDescriptor).Unwrap: added
- (*FileDescriptor).UnwrapFile: added
- (*MessageDescriptor).Unwrap: added
- (*MessageDescriptor).UnwrapMessage: added
- (*MethodDescriptor).Unwrap: added
- (*MethodDescriptor).UnwrapMethod: added
- (*OneOfDescriptor).Unwrap: added
- (*OneOfDescriptor).UnwrapOneOf: added
- (*ServiceDescriptor).Unwrap: added
- (*ServiceDescriptor).UnwrapService: added
- DescriptorWrapper: added
- WrapDescriptor: added
- WrapEnum: added
- WrapEnumValue: added
- WrapField: added
- WrapFile: added
- WrapFiles: added
- WrapMessage: added
- WrapMethod: added
- WrapOneOf: added
- WrapService: added

github.com/jhump/protoreflect/desc/sourceinfo:
Compatible changes:
- GlobalTypes: added
- TypeResolver: added
- WrapEnum: added
- WrapExtensionType: added
- WrapFile: added
- WrapMessage: added
- WrapMessageType: added
- WrapService: added

Despite the significant overhauling of desc/protoparse, I've successfully avoided any API incompatibility.

So I will be merging this and awaiting input from folks to see if there is any fall-out. I suspect there will be some performance impact and that the desc/builder package will now be much more strict, just due to now wrapping the Go runtime's protoreflect.Descriptor implementations (which are more strict). But I think these are both manageable side effects. 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compatibility between protoprint and "google.golang.org/protobuf/reflect/protoreflect"
2 participants