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

New CodeGenerator protocol should allow customising the Google_Protobuf_Compiler_CodeGeneratorRequest #1677

Closed
gjcairo opened this issue Jul 11, 2024 · 7 comments · Fixed by #1679

Comments

@gjcairo
Copy link
Contributor

gjcairo commented Jul 11, 2024

When building a code generator which conforms to the CodeGenerator protocol, we are asked to implement a generate function.
The CodeGenerator protocol defines an extension which gives us the main function, which calls our generate function.
This main function creates a Google_Protobuf_Compiler_CodeGeneratorRequest under the hood, which is ultimately passed to DescriptorSet (see here).

There is currently no way to customise the Google_Protobuf_Compiler_CodeGeneratorRequest, for example to add compiler extensions:

let request = try Google_Protobuf_Compiler_CodeGeneratorRequest(
    serializedBytes: FileHandle.standardInput.readDataToEndOfFile(),
    extensions: [some extensions here]
)

This forces us to re-implement main, duplicating all the logic, just so that we can add extensions.

Note that, prior to 1.27, this problem did not exist because CodeGenerator did not exist. We had to manually build the entire code generation logic by hand.
However, that path is now deprecated with warnings encouraging us to move to using CodeGenerator. Therefore, it would be ideal for CodeGenerator to be flexible enough to support all use-cases.

Potential solutions

  • The CodeGenerator protocol could allow the implementor to specify a list of extensions, similar to how we can currently specify a list of supportedFeatures.
  • The main function logic could take an optional argument of a closure or other means of customising the request.
@thomasvl
Copy link
Collaborator

thomasvl commented Jul 11, 2024

I have some local notes, that I need to circle back for the Editions work to allow custom Editions Features; is that the case you are thinking of for needing extensions, or is it something else?

Edit: I'm not immediately seeing why protoc would add any extensions, or are you talking about something else? The C++ plugin interface doesn't have support for this, so that also makes me wonder about the use case.

@gjcairo
Copy link
Contributor Author

gjcairo commented Jul 11, 2024

This is in the plugin library - you may want your generator to extend certain proto messages with custom options used at the time of code generation.

@thomasvl
Copy link
Collaborator

Ah, the custom options from proto2 (and sorta proto3) syntax? I think those are going away in favor of the editions concepts. Yea, I guess adding a registry is the only way to deal with this, C++ probably dodges this with lack of the need for a registry. Is there an example some places of a generator that was using this?

@gjcairo
Copy link
Contributor Author

gjcairo commented Jul 12, 2024

Ah, the custom options from proto2 (and sorta proto3) syntax?

Yes.

I think those are going away in favor of the editions concepts.

Hm - how does editions have any impact on this? You mean you can just extend any proto (not just the custom options)? Or something else?

Is there an example some places of a generator that was using this?

Sadly nothing I can share.

@thomasvl
Copy link
Collaborator

Ah, the custom options from proto2 (and sorta proto3) syntax?

Yes.

I think those are going away in favor of the editions concepts.

Hm - how does editions have any impact on this? You mean you can just extend any proto (not just the custom options)? Or something else?

From my POV, editions are a replacement for custom options (and official ones). You can define what scope they can be used (file, message, enum, field, etc.), and then they also support inheritance (set at the file rolls down to all the messages/enums/etc.) or you can just set it on one message/field/etc. with the file to use it more limited.

When I did the initial Editions work I wired all this up (it was needed to just to support the core Editions things), but all my remaining work is about defining custom feature for a generator. The unittest I put in already test all this support, it just isn't exposed at the generator lever yet.

If you haven't seen it, they've pushed some docs about Editions already: https://protobuf.dev/editions/overview/

Is there an example some places of a generator that was using this?

Sadly nothing I can share.

No worries, independent of Editions, we should look at adding something to the allow this. I think I might be able to find some time next week depending on how my time goes with some other things.

@thomasvl
Copy link
Collaborator

Did a little more thinking about this, and just adding a way to get a registry in to the parsing of the request likely isn't enough. We also have to expose the extensions from all the Descriptor types, and with things moving in the direction of not exposing the raw protos, I'm not sure how that best makes sense.

I'll try to dig back through the C++ plugin interface to see how they did it for ideas.

@thomasvl
Copy link
Collaborator

Did a little more thinking about this, and just adding a way to get a registry in to the parsing of the request likely isn't enough. We also have to expose the extensions from all the Descriptor types, and with things moving in the direction of not exposing the raw protos, I'm not sure how that best makes sense.

I'll try to dig back through the C++ plugin interface to see how they did it for ideas.

I take that back, I was thinking some were on the *Proto objects, but they are all on the *Options objects, and the api exposes those just like the C++, so just getting things into the parsing flow looks like all that would be needed.

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Jul 15, 2024
This will get those extensions recognized when the request from protoc is
parsed, and then a generator can use the existing SwiftProtobuf extension
related apis to access them.

Fixes apple#1677
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Jul 16, 2024
This will get those extensions recognized when the request from protoc is
parsed, and then a generator can use the existing SwiftProtobuf extension
related apis to access them.

Fixes apple#1677
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 a pull request may close this issue.

2 participants