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

fix(build): Correctly convert Empty to () #734

Merged
merged 4 commits into from Oct 22, 2021

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Jul 30, 2021

Motivation

#521 covers the work for Tonic to compile the well-known protobuf types instead of relying on the prost-types crate. (One reason to do that is for projects that want google.protobuf.Any to use Bytes instead of Vec<u8> for binary data.)

#522 added initial support for Tonic to configure Prost and itself to compile the well-known protobuf types.

That support is incomplete, however: tonic-build fails to compile when configured to convert google.protobuf.Empty to (). This configuration occurs if the user calls config.extern_path(".google.protobuf.Empty", "()") where config is an instance of prost_build::Config. The error occurs because the existing code tries to interpret () as a Rust path and parsing, for example, super::() as a Rust path will always fail.

Solution

This PR fixes the conversion of google.protobuf.Empty to () by specifically allowing the use of () as a valid Rust conversion without attempting to prepend any Rust path.

There are several stacked commits in this PR:

  1. Update the wellknown-compiled integration test to configure the google.protobuf.Empty -> () conversion. This first commit fails to compile without the fix.
  2. Refactor tonic_build::prost::Service::request_response_name to remove the duplication of logic between converting request and response types.
  3. Fix the conversion by introducing an allow list of types (consisting only of () for now) and just generate a token stream in that case without trying to interpret as a Rust path.

@LucioFranco
Copy link
Member

@tdyas if you can rebase this against master I think we can get this merged soon, thanks.

@tdyas
Copy link
Contributor Author

tdyas commented Oct 20, 2021

Rebased cleanly and pushed.

@tdyas
Copy link
Contributor Author

tdyas commented Oct 21, 2021

@LucioFranco: Tests pass after rebase; this should be ready to go.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@LucioFranco LucioFranco added this to the 0.6 milestone Oct 22, 2021
@LucioFranco LucioFranco changed the title fix converting google.protobuf.Empty to () when compiling well-known protobuf types fix(build): Correctly convert Empty to () Oct 22, 2021
@LucioFranco LucioFranco merged commit ff6a690 into hyperium:master Oct 22, 2021
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.

None yet

2 participants