Skip to content

Conversation

@daft-panda
Copy link

Motivation

Google WKT were resolving incorrectly (e.g. google.protobuf.BoolValue resolved to super::bool).

Solution

Fixed by using the resolved types from prost, added test cases.

@sauravzg sauravzg requested a review from LucioFranco December 8, 2025 15:52
@sauravzg
Copy link
Collaborator

sauravzg commented Dec 8, 2025

Not familiar with prost builds. @LucioFranco is likely to be better equipped for this review

rpc EmptyCall(google.protobuf.Empty) returns (google.protobuf.Empty);
rpc StringCall(google.protobuf.StringValue) returns (google.protobuf.Empty);
rpc AnyCall(google.protobuf.Any) returns (google.protobuf.Empty);
// Wrapper types that map to primitives
Copy link
Member

Choose a reason for hiding this comment

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

Are these new? In what version were they added?

Copy link
Author

Choose a reason for hiding this comment

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

They have been around at least since 2015 (https://github.com/protocolbuffers/protobuf/commits/main/src/google/protobuf/wrappers.proto). These wrappers used to work with prost 0.13.

Copy link
Member

Choose a reason for hiding this comment

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

They used to work? huh I have no memory of seeing them but that legit could just be me. Can you expand on what you mean by it worked and what changed?

Copy link
Author

Choose a reason for hiding this comment

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

Can't expand that much I'm afraid, got pointed in this direction by Claude and considering the handling of other wrapper types this seemed like the place to be for a fix. My best guess would be this change in prost-build from 0.13 to 0.14: tokio-rs/prost#1228 .

Copy link
Author

Choose a reason for hiding this comment

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

@LucioFranco would be great if this got merged, let me know if there is anything still missing.

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.

3 participants