Description
openedon Dec 4, 2023
Context
Currently, it is not possible to run vtproto multiple times for the same package on different files because it would generate multiple times the same helpers, so the code would not compile.
As an example, let's take the pool
package of this repository with the following proto files: pool.proto
, pool_with_slice_reuse.proto
, and pool_with_oneof.proto
. As long as we run vtproto once with all those files, it works fine. However, if we run vtproto three times (once for each file), then the generated code doesn't compile because there are three declarations of the same functions / variables:
encodeVarint
sov
soz
skip
ErrInvalidLength
ErrIntOverflow
ErrUnexpectedEndOfGroup
I believe this is an issue because this behavior contrasts with the standard protobuf compiler, causing difficulties to onboard automation that assumes the ability to run the compiler for one file at a time. As a user, I'm currently facing this challenge and I would love to fix this issue to improve compatibility with automation and ensure a seamless transition for new users as well. 😃
Possible solutions
I can think of at least three possible solutions to tackle this issue. Before working on an implementation, I would like to gauge interest in the different solutions. If there is a preferred one, I can provide a Pull Request. My recommended one is solution A. There may also be other ways that I haven't think about.
A. Add helpers to vtprotobuf as a package and use them from the generated code (recommended)
We could expose public helpers (e.g. EncodeVarint
) as part of the vtprotobuf
package, as is currently done for vtprotobuf/types/known/...
for supporting well-known types. The generated _vtproto.pb.go
files of users would then have a dependency on a vtprotobuf/protohelpers
package (for example) to use these helpers.
I think the dependency is fine but, if this is not acceptable for some users, we could add an option to generate today's no-dependency helpers instead. There is an existing --go-vtproto_opt=wkt=false
option to avoid depending on vtprotobuf
. We could consolidate both options under something like --go-vtproto_opt=vtproto_dep=false
, which would disable both support for well-known types and usage of global helpers.
B. Generate a different file containing all helpers for the package
Besides files generated from source proto
files, when running vtproto, we could generate an additional file defining the helpers, that we could name vtproto_helpers.go
for instance. This way, when running vtproto once for each file, that file would be regenerated and left unchanged.
With this, there is no need for a dependency to vtprotobuf
. However, one potential issue that I see is that we would lose the 1:1 mapping between source proto
files and generated files, which may not be friendly to automation either. Indeed, the regular proto compiler generates one file for each source file and each generated file corresponds to one source file: there is an exact 1:1 mapping. This would no longer be the case for vtproto with this implementation.
C. Generate different helpers for each file
We could generate the same helpers with different names for different files. For instance (still taking the pool
package of this repository), pool.proto
would generate encodeVarint_pool
, sov_pool
, soz_pool
, etc. and pool_with_slice_reuse.proto
would generate encodeVarint_pool_with_slice_reuse
, sov_pool_with_slice_reuse
, soz_pool_with_slice_reuse
.
I see two issues with this approach, though:
- the generated code is more verbose and less readable;
- some helpers are public so they are exposed to users. Not only would a public
ErrUnexpectedEndOfGroup_pool_with_slice_reuse
be less readable, it would also be confusing to have the same public error defined in several places under different names.- note that these public errors are currently generated for each package, so they are already defined in several places (with the same name). Defining them only once in
vtprotobuf
might be an additional benefit of solution A.
- note that these public errors are currently generated for each package, so they are already defined in several places (with the same name). Defining them only once in