[protobuf-go/V2 API] Provide an extension mechanism for protoimpl

[silasdavis]

tl;dr provide a way to override the mapping (i.e. the Converter) between Go reflect and Protobuf protoreflect types so protoimpl can be reused by other implementations

Problem and background

The protoreflect interfaces behaviourally describe protobuf messages and are intended, amongst other things, to allow for custom code generation (for which there is strong demand, but an uncertain future: gogo/protobuf#691). However implementing the entire interface while supporting concurrency and fast path methods is a large undertaking.

Many of the low-hanging fruit (e.g. supporting fixed-width named type byte arrays) would need relatively minor changes to existing code to work but custom implementations find they need to fork large amounts of protobuf-go in order to reuse the code in protoimpl because of the structure of internal packages (see below).

An approach for extending the nice functionality provided by protoimpl that is conceptually appealing is to embed a MessageState (*MessageInfo) object in a struct and intercept protoreflect.Message calls where needed before passing along to the underlying MessageInfo object (continuing the earlier example, for example, taking a slice over a fixed-width byte array). However this approach does not work because MessageInfo.initOnce() uses the go type to establish a Converter the implementation of which is hard-coded and will throw errors when it does not see the Go type it expects for the protobuf type it is converting (for instance if a proto message field is not a pointer or a proto bytes field is not a slice).

Possible solution

I understand why the protobuf-go team has wanted to limit their public API, but I would like to propose that some small incision be made into protoimpl in order to allow dependees to extend it and reuse the protoimpl implementation.

My hunch is that if the protoimpl.TypeBuilder as used by generated code were to take a user-providable Converter implementation and pass it through the protoimpl implementation then that might be enough to reuse most of the protoimpl code when serialising to custom types. Another option would be to introduce a Converter() Converter method to the protoreflect.Message or another protoreflect interface and have that optionally return a custom converter (otherwise returning nil, like with ProtoMethods()), another option still would be to add it to the ProtoMethods() return type. Athough this would not be about 'fast path', it might be convenient since the return type is already weakly typed.

Alternatives

I have attempted to only minimally fork protobuf-go as suggested by @dsnet here: xen0n/protobuf-gogogo#1 with some success, the results of which are here: https://github.com/monax/peptide/

However, as described above, in order to extend the protoimpl machinery this is not enough.

Another alternative would be to push most of the work of protoimpl into the code generator itself and not use go reflection (strong static types and generated methods everywhere). This would probably also provide some performance benefits, but I am happy with protobuf-go performance and I would rather minimise the surface area that needs to be maintained for some simple idiomatic type mappings.

Context

Please see my initial ramble on this topic here: xen0n/protobuf-gogogo#1 (comment) including some code examples around how a wrapper/decorator implementation of protoreflect.Message on top of MessageInfo might work.

A further remark having read #526. It may be possible that protobuf-go improves its generated code API to 'close the gap' that gogo proto has previously filled, which I am all in favour of, but there are clearly significant trade-offs, legacy, and consistency issues that the present project has to deal with being at the apex of the protobuf ecosystem.

I think it could be much more efficient for protobuf-go to let a thousand flowers bloom here, provide a slightly larger public API that includes some limited access into protoimpl, while hopefully still allowing you to evolve the internal implementation without making breaking changes. Projects like gogogo and peptide have more freedom to iterate on the code generation side and in doing so we can proof out any interface boundaries around protoimpl, which protobuf-go itself can make use of to provide a better generated Go API down the line.

Work

I had hoped to continue with my 'minimal fork' approach but it looks like the dependency tree will force me to fork many internal packages of protobuf-go. I am open to guidance on what would be a good use of time, I could try and implement a proof-of-concept for the Converter idea against a plain fork for protobuf-go if it has any chance of getting accepted. Alternatively I could continue to hack and slash through my minimal fork version, but I'm not convinced I'll end up with something I want to maintain.

[neild]

I'm afraid that there is little chance that we will make any of the implementation details in internal/impl public. The external APIs in the protoreflect package are designed to permit as much flexibility as possible without constraining future development. The internal/impl package, in contrast, is not.

As a specific example, you mention the possibility of a user-provided Converter implementation. If we allowed for that, it would prevent us from ever adding methods to Converter in the future, since this would be a breaking change. It would prevent us from removing Converter entirely and replacing it with something else. Both of these changes are real possibilities; we've added a method to Converter since releasing this module, and it's quite plausible that some future refactoring of the implementation will remove it entirely.

Please note also that while the "internal/impl".MessageInfo type is exported via the "google.golang.org/runtime/protoimpl" package, nothing about it is covered by the module compatibility guarantee. Any approach based on using it in any way is almost certain to break unexpectedly at some point in the future.

[silasdavis]

Thanks for the quick response.

Another option would be to release protoimpl as a distinct versioned package with as a reference implementation against the more stable protoreflect API. This would introduce the maintenance burden of maintaining two public packages but would mean you could independently semantically version both packages you would be able to make frequent breaking changes to protoimpl. The benefit would be it would in lowering the bar significantly for new custom implementations of protoreflect. A downstream implementation could continue to use an older version of protoimpl if they so chose.

[dsnet]

I recommend forking protoimpl.

As @neild mentioned, there is a potential future where the Converter type is changed or even removed. Making Converter a part of the public API removes our ability to internally refactor implementation details like this. If you believe that the Converter API today is a good fit for implementing the features of gogo/protobuf, then I'd say that's a good reason to fork protoimpl as it exists today rather than the potential future protoimpl when we simplify the implementation to exactly handle the types that we've promised to handle (and no longer use Converter).

[silasdavis]

I understand. Clearly removing you ability to refactor internal code is not an option. My previous suggestion comes down to whether I maintain the fork or you do. A Google-maintained implementation fork has the great virtue of having more eyes on it (if protobuf-go were to depend on it) and the even greater virtue of me not having to do the work :)

But if you can't be persuaded by the compelling argument of having more work to do then I suppose I will have to roll my sleeves up. For reference my current attempt is here: https://github.com/monax/peptide.

Thanks for the consideration and speedy responses much appreciated.